linux-kernel - Re: [PATCH] drm/v3d: Add DRM_IOCTL_V3D_PERFMON_SET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f050428-53b0-401f-a60f-3d4732c0a75f@igalia.com>
Date: Tue, 22 Oct 2024 17:12:33 -0300
From: Maíra Canal <mcanal@...lia.com>
To: Christian Gmeiner <christian.gmeiner@...il.com>,
 Melissa Wen <mwen@...lia.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>
Cc: kernel-dev@...lia.com, Christian Gmeiner <cgmeiner@...lia.com>,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/v3d: Add DRM_IOCTL_V3D_PERFMON_SET_GLOBAL

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