Re: [PATCH v5 2/2] Documentation/gpu: Add fdinfo meanings of drm-*-internal memory tags

[Tvrtko Ursulin <tursulin@ursulin.net>]

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@collabora.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@collabora.com>
>>> Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.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.

Regards,

Tvrtko

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