Re: [PATCH] drm/v3d: Add DRM_IOCTL_V3D_PERFMON_SET_GLOBAL

["Maíra Canal" <mcanal@igalia.com>]

On 10/20/24 17:41, Christian Gmeiner wrote:
> From: Christian Gmeiner <cgmeiner@igalia.com>
> 
> This patch adds a new ioctl, DRM_IOCTL_V3D_PERFMON_SET_GLOBAL, which
> allows the configuration of a global performance monitor (perfmon).
> The global perfmon is used for all jobs, ensuring consistent performance
> tracking across submissions.
> 
> Signed-off-by: Christian Gmeiner <cgmeiner@igalia.com>
> ---
>   drivers/gpu/drm/v3d/v3d_drv.c                 |  3 ++
>   drivers/gpu/drm/v3d/v3d_drv.h                 | 10 ++++
>   drivers/gpu/drm/v3d/v3d_perfmon.c             | 49 +++++++++++++++++++
>   .../gpu/drm/v3d/v3d_performance_counters.h    |  6 +++
>   drivers/gpu/drm/v3d/v3d_sched.c               | 10 +++-
>   include/uapi/drm/v3d_drm.h                    | 15 ++++++
>   6 files changed, 91 insertions(+), 2 deletions(-)
> 

Hi Christian,

I have one major issue with this approach: I don't think we should
introduce a `global_perfmon` in `struct v3d_perfmon_info`. `struct
v3d_perfmon_info` was created to store information about the counters,
such as total number of perfcnts supported and the description of the
counters.

I believe you should use `active_perfmon` in your implementation and
don't create `global_perfmon`. This is going to make the code less
tricky to understand and it's going to make sure that the hardware inner
working is transparent in software.

Only one perfmon can be active in a given moment of time, therefore,
let's use `active_perfmon` to represent it.

I couple more things came to my attention. First, I don't think we need
to limit the creation of other perfmons. We can create perfmons and
don't use it for a while. We only need to make sure that the user can't
attach perfmons to jobs, when the global perfmon is enabled.

For sure, if we go through this strategy, there is no need to have a
count of all the perfmons that V3D has.

I would prefer to treat the global perfmon as a state. Ideally, we would
enable and disable this state through the IOCTL.

One last thing is: don't forget to stop the perfmons when you don't use
it anymore :)

Best Regards,
- Maíra