Message-ID: <lzvil3iy7cq4t3kz3dl5g2j5n6lr4wd5qwtmqt2l2hwsqyzd3q@sumaho6upjvv>
Date: Mon, 6 Jan 2025 16:53:35 +0000
From: Adrián Martínez Larumbe <adrian.larumbe@...labora.com>
To: Tvrtko Ursulin <tursulin@...ulin.net>
Cc: Boris Brezillon <boris.brezillon@...labora.com>, 
	Steven Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....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>, kernel@...labora.com, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, Mihail Atanassov <mihail.atanassov@....com>
Subject: Re: [PATCH v5 2/2] Documentation/gpu: Add fdinfo meanings of
 drm-*-internal memory tags

On 03.01.2025 10:49, Tvrtko Ursulin wrote:
> On 02/01/2025 22:18, Adrián Martínez Larumbe wrote:
> > On 02.01.2025 21:59, Tvrtko Ursulin wrote:
> > > 
> > > On 18/12/2024 18:18, Adrián Martínez Larumbe wrote:
> > > > From: Adrián Larumbe <adrian.larumbe@...labora.com>
> > > > 
> > > > A previous commit enabled display of driver-internal kernel BO sizes
> > > > through the device file's fdinfo interface.
> > > > 
> > > > Expand the description of the relevant driver-specific key:value pairs
> > > > with the definitions of the new drm-*-internal ones.
> > > > 
> > > > Signed-off-by: Adrián Larumbe <adrian.larumbe@...labora.com>
> > > > Reviewed-by: Mihail Atanassov <mihail.atanassov@....com>
> > > > ---
> > > >    Documentation/gpu/panthor.rst | 14 ++++++++++++++
> > > >    1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/Documentation/gpu/panthor.rst b/Documentation/gpu/panthor.rst
> > > > index 3f8979fa2b86..23aa3d67c9d2 100644
> > > > --- a/Documentation/gpu/panthor.rst
> > > > +++ b/Documentation/gpu/panthor.rst
> > > > @@ -26,6 +26,10 @@ the currently possible format options:
> > > >         drm-cycles-panthor:     94439687187
> > > >         drm-maxfreq-panthor:    1000000000 Hz
> > > >         drm-curfreq-panthor:    1000000000 Hz
> > > > +     drm-total-internal:     10396 KiB
> > > > +     drm-shared-internal:    0
> > > > +     drm-active-internal:    10396 KiB
> > > > +     drm-resident-internal:  10396 KiB
> > > >         drm-total-memory:       16480 KiB
> > > >         drm-shared-memory:      0
> > > >         drm-active-memory:      16200 KiB
> > > > @@ -44,3 +48,13 @@ driver by writing into the appropriate sysfs node::
> > > >    Where `N` is a bit mask where cycle and timestamp sampling are respectively
> > > >    enabled by the first and second bits.
> > > > +
> > > > +Possible `drm-*-internal` keys are: `total`, `active`, `resident` and `shared`.
> > > > +These values convey the sizes of the internal driver-owned shmem BO's that
> > > > +aren't exposed to user-space through a DRM handle, like queue ring buffers,
> > > > +sync object arrays and heap chunks. Because they are all allocated and pinned
> > > > +at creation time, `drm-resident-internal` and `drm-total-internal` should always
> > > > +be equal. `drm-active-internal` shows the size of kernel BO's associated with
> > > > +VM's and groups currently being scheduled for execution by the GPU.
> > > > +`drm-shared-internal` is unused at present, but in the future it might stand for
> > > > +the size of executable FW regions, since they do not belong to an open file context.
> > > 
> > > The description is way too specific, too tied to some of the implementations.
> > 
> > These are panthor-specific key:value pairs. I was in the belief that drivers
> > could define their own when it suits their interest beyond the DRM-wide ones
> > defined in the drm-fdinfo spec.
> > 
> > > I also don't remember that you ever explained why totting up the internal
> > > objects into existing regions isn't good enough. I keep asking, you keep not
> > > explaining. Or I missed your emails somehow.
> > 
> > It's not that it's not good enough, but rather that it cannot be done in the
> > current state of affairs. drm_show_memory_stats() defines its own
> > drm_memory_stats struct as an automatic variable so we don't have access to it
> > from anywhere else in the driver. In a previous revision of the patch series I
> > had come up with a workaround that would let drivers pass a function pointer to
> > drm_show_memory_stats() which would gather those numbers in a driver-specific
> > way, but it didn't seem to get any traction.
> 
> Side note - i915 and amdgpu manage to do it so it is not that it is not
> possible.
> 
> > > And you keep not copying me on the thread. Copying people who expressed
> > > interest, gave past feedback, etc should be the norm.
> > 
> > I did not CC you on this series because these are all panthor-specific changes
> > which do not touch on any DRM fdinfo-wide code, and also because I didn't think
> > that driver-specific key:value pairs needed the approval of the drm-fdinfo core
> > maintainers.
> 
> Ah my bad.. sorry! I saw drm-internal-* and did not spot it is actually *in*
> panthor.rst. So I think you just need to rename those to panthor- prefix. Same
> as amdgpu has its own private keys amd-evicted-vram etc.

This complicates things because that means I can no longer use
drm_print_memory_stat(), since print_size() will prefix every single key with
"drm-". But then not using print_size() means I'm giving up on the nice unit
size selection loop, which I guess I could just copy and paste inside Panthor,
but I do remember a recent patch series where the unit selection criteria
changed slightly so wouldn't like to keep these manually sync'd.

There's also the thing that the units I'm displaying here match up nicely with
those representing the size of DRM objects with a UM-facing handle, so crafting
my own function to display these when the only difference is a single key prefix
seems like an overkill. I guess drm_print_memory_stats() and the functions
underneath should offer more flexibility, but I guess that's something that can
be discussed in a later patch series.

And then there's the following statement in Documentation/gpu/drm-usage-stats.rst:24:

"- All keys shall be prefixed with `drm-`."

It doesn't say Driver-specific keys should begin with the name of the driver. In
fact, it seems neither AMD nor Intel drivers have theirs documented.

In light of all this, I'd much rather not modify the names of Panthor-specific
fdinfo's internal memory keys.

> Regards,
> 
> Tvrtko
> 
> > > Until we can clarify the above points I don't think this can go in.
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > 
> > Adrian Larumbe

Adrian Larumbe

Powered by blists - more mailing lists