On 10/20/24 17:41, Christian Gmeiner wrote:
From: Christian Gmeiner <cgmeiner@xxxxxxxxxx> 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@xxxxxxxxxx> --- 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
