[PATCH 1/2] Documentation/gpu: Document the situation with unqualified drm-memory-

[Tvrtko Ursulin]

On 06/09/2024 19:12, Alex Deucher wrote:
> On Wed, Sep 4, 2024 at 4:36 AM Tvrtko Ursulin <tvrtko.ursulin at igalia.com> wrote:
>>
>>
>> On 21/08/2024 21:47, Alex Deucher wrote:
>>> On Tue, Aug 13, 2024 at 9:57 AM Tvrtko Ursulin <tursulin at igalia.com> wrote:
>>>>
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>>
>>>> Currently it is not well defined what is drm-memory- compared to other
>>>> categories.
>>>>
>>>> In practice the only driver which emits these keys is amdgpu and in them
>>>> exposes the current resident buffer object memory (including shared).
>>>>
>>>> To prevent any confusion, document that drm-memory- is deprecated and an
>>>> alias for drm-resident-memory-.
>>>>
>>>> While at it also clarify that the reserved sub-string 'memory' refers to
>>>> the memory region component, and also clarify the intended semantics of
>>>> other memory categories.
>>>>
>>>> v2:
>>>>    * Also mark drm-memory- as deprecated.
>>>>    * Add some more text describing memory categories. (Alex)
>>>>
>>>> v3:
>>>>    * Semantics of the amdgpu drm-memory is actually as drm-resident.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>>> Cc: Christian König <christian.keonig at amd.com>
>>>> Cc: Rob Clark <robdclark at chromium.org>
>>>> ---
>>>>    Documentation/gpu/drm-usage-stats.rst | 25 ++++++++++++++++++++++---
>>>>    1 file changed, 22 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
>>>> index a80f95ca1b2f..ff964c707754 100644
>>>> --- a/Documentation/gpu/drm-usage-stats.rst
>>>> +++ b/Documentation/gpu/drm-usage-stats.rst
>>>> @@ -144,7 +144,9 @@ Memory
>>>>
>>>>    Each possible memory type which can be used to store buffer objects by the
>>>>    GPU in question shall be given a stable and unique name to be returned as the
>>>> -string here.  The name "memory" is reserved to refer to normal system memory.
>>>> +string here.
>>>> +
>>>> +The region name "memory" is reserved to refer to normal system memory.
>>>>
>>>>    Value shall reflect the amount of storage currently consumed by the buffer
>>>>    objects belong to this client, in the respective memory region.
>>>> @@ -152,6 +154,9 @@ objects belong to this client, in the respective memory region.
>>>>    Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB'
>>>>    indicating kibi- or mebi-bytes.
>>>>
>>>> +This key is deprecated and is an alias for drm-resident-<region>. Only one of
>>>> +the two should be present in the output.
>>>> +
>>>
>>> I'm not sure how best to handle this.  What should amdgpu do?  We have
>>> customers out in the field using these existing fields and then with
>>> patch 2, they go away.  Arguably we'd want both for backwards
>>> compatibility.
>>
>> Exactly, so it looks you maybe missed that 2/2 is not removing the
>> amdgpu "legacy" drm-memory-. It keeps outputting it and also duplicating
>> under drm-resident-. This is mentioned in the commit paragraph:
>>
>> """
>> Legacy keys have been preserved, with the outlook of only potentially
>> removing only the drm-memory- when the time gets right.
>> """
>>
>> Put differently, I don't think 2/2 should break the existing
>> tools/parsers. Only if they have hardcoded assumptions about the order
>> of keys perhaps?
> 
> You're right.  I totally missed that part.  The series is:
> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
> Happy to take the patches through my tree or patch 2 via mine and
> patch 1 via drm-misc.

Thanks!

As to how to merge I don't have a strong preference. Adding Maxime and 
Thomas to check if they would want to take 1/2 via drm-misc-next? But 
IMO not splitting the patches between trees also kind of makes sense.

Regards,

Tvrtko

> 
> Alex
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>>>    - drm-shared-<region>: <uint> [KiB|MiB]
>>>>
>>>>    The total size of buffers that are shared with another file (e.g., have more
>>>> @@ -159,20 +164,34 @@ than a single handle).
>>>>
>>>>    - drm-total-<region>: <uint> [KiB|MiB]
>>>>
>>>> -The total size of buffers that including shared and private memory.
>>>> +The total size of all created buffers including shared and private memory. The
>>>> +backing store for the buffers does not have to be currently instantiated to be
>>>> +counted under this category.
>>>>
>>>>    - drm-resident-<region>: <uint> [KiB|MiB]
>>>>
>>>> -The total size of buffers that are resident in the specified region.
>>>> +The total size of buffers that are resident (have their backing store present or
>>>> +instantiated) in the specified region.
>>>> +
>>>> +This is an alias for drm-memory-<region> and only one of the two should be
>>>> +present in the output.
>>>>
>>>>    - drm-purgeable-<region>: <uint> [KiB|MiB]
>>>>
>>>>    The total size of buffers that are purgeable.
>>>>
>>>> +For example drivers which implement a form of 'madvise' like functionality can
>>>> +here count buffers which have instantiated backing store, but have been marked
>>>> +with an equivalent of MADV_DONTNEED.
>>>> +
>>>>    - drm-active-<region>: <uint> [KiB|MiB]
>>>>
>>>>    The total size of buffers that are active on one or more engines.
>>>>
>>>> +One practical example of this can be presence of unsignaled fences in an GEM
>>>> +buffer reservation object. Therefore the active category is a subset of
>>>> +resident.
>>>> +
>>>>    Implementation Details
>>>>    ======================
>>>>
>>>> --
>>>> 2.44.0
>>>>