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

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).

Document that drm-memory- and drm-total-memory- are aliases to prevent any
confusion in the future.

While at it also clarify that the reserved sub-string 'memory' refers to
the memory region component.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.keonig@amd.com>
Cc: Rob Clark <robdclark@chromium.org>
---
 Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index 6dc299343b48..ef5c0a0aa477 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -128,7 +128,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.
@@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two should be
+present.
+
 - drm-shared-<region>: <uint> [KiB|MiB]
 
 The total size of buffers that are shared with another file (e.g., have more
@@ -145,6 +150,9 @@ than a single handle).
 
 The total size of buffers that including shared and private memory.
 
+This is an alias for drm-memory-<region> and only one of the two should be
+present.
+
 - drm-resident-<region>: <uint> [KiB|MiB]
 
 The total size of buffers that are resident in the specified region.
-- 
2.44.0

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

[Tvrtko Ursulin]

[Correcting Christian's email]

On 03/05/2024 13:36, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).
> 
> Document that drm-memory- and drm-total-memory- are aliases to prevent any
> confusion in the future.
> 
> While at it also clarify that the reserved sub-string 'memory' refers to
> the memory region component.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.keonig@amd.com>

Mea culpa, I copied the mistake from 
77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)

Regards,

Tvrtko

> Cc: Rob Clark <robdclark@chromium.org>
> ---
>   Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> index 6dc299343b48..ef5c0a0aa477 100644
> --- a/Documentation/gpu/drm-usage-stats.rst
> +++ b/Documentation/gpu/drm-usage-stats.rst
> @@ -128,7 +128,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.
> @@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two should be
> +present.
> +
>   - drm-shared-<region>: <uint> [KiB|MiB]
>   
>   The total size of buffers that are shared with another file (e.g., have more
> @@ -145,6 +150,9 @@ than a single handle).
>   
>   The total size of buffers that including shared and private memory.
>   
> +This is an alias for drm-memory-<region> and only one of the two should be
> +present.
> +
>   - drm-resident-<region>: <uint> [KiB|MiB]
>   
>   The total size of buffers that are resident in the specified region.

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

[Tvrtko Ursulin]

[And I forgot dri-devel.. doing well!]

On 03/05/2024 13:40, Tvrtko Ursulin wrote:
> 
> [Correcting Christian's email]
> 
> On 03/05/2024 13:36, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).
>>
>> Document that drm-memory- and drm-total-memory- are aliases to prevent 
>> any
>> confusion in the future.
>>
>> While at it also clarify that the reserved sub-string 'memory' refers to
>> the memory region component.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Christian König <christian.keonig@amd.com>
> 
> Mea culpa, I copied the mistake from 
> 77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)
> 
> Regards,
> 
> Tvrtko
> 
>> Cc: Rob Clark <robdclark@chromium.org>
>> ---
>>   Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/gpu/drm-usage-stats.rst 
>> b/Documentation/gpu/drm-usage-stats.rst
>> index 6dc299343b48..ef5c0a0aa477 100644
>> --- a/Documentation/gpu/drm-usage-stats.rst
>> +++ b/Documentation/gpu/drm-usage-stats.rst
>> @@ -128,7 +128,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.
>> @@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two 
>> should be
>> +present.
>> +
>>   - drm-shared-<region>: <uint> [KiB|MiB]
>>   The total size of buffers that are shared with another file (e.g., 
>> have more
>> @@ -145,6 +150,9 @@ than a single handle).
>>   The total size of buffers that including shared and private memory.
>> +This is an alias for drm-memory-<region> and only one of the two 
>> should be
>> +present.
>> +
>>   - drm-resident-<region>: <uint> [KiB|MiB]
>>   The total size of buffers that are resident in the specified region.

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

[Alex Deucher]

On Fri, May 3, 2024 at 8:58 AM Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:
>
>
> [And I forgot dri-devel.. doing well!]
>
> On 03/05/2024 13:40, Tvrtko Ursulin wrote:
> >
> > [Correcting Christian's email]
> >
> > On 03/05/2024 13:36, Tvrtko Ursulin wrote:
> >> From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).
> >>
> >> Document that drm-memory- and drm-total-memory- are aliases to prevent
> >> any
> >> confusion in the future.
> >>
> >> While at it also clarify that the reserved sub-string 'memory' refers to
> >> the memory region component.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> >> Cc: Alex Deucher <alexander.deucher@amd.com>
> >> Cc: Christian König <christian.keonig@amd.com>
> >
> > Mea culpa, I copied the mistake from
> > 77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)
> >

I'm not following.  What is the mistake from that commit?

> > Regards,
> >
> > Tvrtko
> >
> >> Cc: Rob Clark <robdclark@chromium.org>
> >> ---
> >>   Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
> >>   1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/gpu/drm-usage-stats.rst
> >> b/Documentation/gpu/drm-usage-stats.rst
> >> index 6dc299343b48..ef5c0a0aa477 100644
> >> --- a/Documentation/gpu/drm-usage-stats.rst
> >> +++ b/Documentation/gpu/drm-usage-stats.rst
> >> @@ -128,7 +128,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.

Is this supposed to mean drm-memory-memory?  That was my impression,
but that seems sort of weird.  Maybe we should just drop that
sentence.

Alex

> >>   Value shall reflect the amount of storage currently consumed by the
> >> buffer
> >>   objects belong to this client, in the respective memory region.
> >> @@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two
> >> should be
> >> +present.
> >> +
> >>   - drm-shared-<region>: <uint> [KiB|MiB]
> >>   The total size of buffers that are shared with another file (e.g.,
> >> have more
> >> @@ -145,6 +150,9 @@ than a single handle).
> >>   The total size of buffers that including shared and private memory.
> >> +This is an alias for drm-memory-<region> and only one of the two
> >> should be
> >> +present.
> >> +
> >>   - drm-resident-<region>: <uint> [KiB|MiB]
> >>   The total size of buffers that are resident in the specified region.

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

[Tvrtko Ursulin]

On 03/05/2024 14:39, Alex Deucher wrote:
> On Fri, May 3, 2024 at 8:58 AM Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:
>>
>>
>> [And I forgot dri-devel.. doing well!]
>>
>> On 03/05/2024 13:40, Tvrtko Ursulin wrote:
>>>
>>> [Correcting Christian's email]
>>>
>>> On 03/05/2024 13:36, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).
>>>>
>>>> Document that drm-memory- and drm-total-memory- are aliases to prevent
>>>> any
>>>> confusion in the future.
>>>>
>>>> While at it also clarify that the reserved sub-string 'memory' refers to
>>>> the memory region component.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>> Cc: Christian König <christian.keonig@amd.com>
>>>
>>> Mea culpa, I copied the mistake from
>>> 77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)
>>>
> 
> I'm not following.  What is the mistake from that commit?

Just the spelling of Christian's last name in the email address, nothing 
in the code itself. I failed to spot both that when copying the email 
for git commit, and also failed to cc dri-devel so I am having a bad day.

Regards,

Tvrtko

> 
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>> Cc: Rob Clark <robdclark@chromium.org>
>>>> ---
>>>>    Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/gpu/drm-usage-stats.rst
>>>> b/Documentation/gpu/drm-usage-stats.rst
>>>> index 6dc299343b48..ef5c0a0aa477 100644
>>>> --- a/Documentation/gpu/drm-usage-stats.rst
>>>> +++ b/Documentation/gpu/drm-usage-stats.rst
>>>> @@ -128,7 +128,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.
> 
> Is this supposed to mean drm-memory-memory?  That was my impression,
> but that seems sort of weird.  Maybe we should just drop that
> sentence.
> 
> Alex
> 
>>>>    Value shall reflect the amount of storage currently consumed by the
>>>> buffer
>>>>    objects belong to this client, in the respective memory region.
>>>> @@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two
>>>> should be
>>>> +present.
>>>> +
>>>>    - drm-shared-<region>: <uint> [KiB|MiB]
>>>>    The total size of buffers that are shared with another file (e.g.,
>>>> have more
>>>> @@ -145,6 +150,9 @@ than a single handle).
>>>>    The total size of buffers that including shared and private memory.
>>>> +This is an alias for drm-memory-<region> and only one of the two
>>>> should be
>>>> +present.
>>>> +
>>>>    - drm-resident-<region>: <uint> [KiB|MiB]
>>>>    The total size of buffers that are resident in the specified region.

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

[Daniel Vetter]

On Fri, May 03, 2024 at 01:58:38PM +0100, Tvrtko Ursulin wrote:
> 
> [And I forgot dri-devel.. doing well!]
> 
> On 03/05/2024 13:40, Tvrtko Ursulin wrote:
> > 
> > [Correcting Christian's email]
> > 
> > On 03/05/2024 13:36, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).
> > > 
> > > Document that drm-memory- and drm-total-memory- are aliases to
> > > prevent any
> > > confusion in the future.
> > > 
> > > While at it also clarify that the reserved sub-string 'memory' refers to
> > > the memory region component.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Christian König <christian.keonig@amd.com>
> > 
> > Mea culpa, I copied the mistake from
> > 77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)
> > 
> > Regards,
> > 
> > Tvrtko
> > 
> > > Cc: Rob Clark <robdclark@chromium.org>
> > > ---
> > >   Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
> > >   1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/gpu/drm-usage-stats.rst
> > > b/Documentation/gpu/drm-usage-stats.rst
> > > index 6dc299343b48..ef5c0a0aa477 100644
> > > --- a/Documentation/gpu/drm-usage-stats.rst
> > > +++ b/Documentation/gpu/drm-usage-stats.rst
> > > @@ -128,7 +128,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.
> > > @@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two
> > > should be
> > > +present.

This feels a bit awkward and seems to needlessly complicate fdinfo uapi.

- Could we just patch amdgpu to follow everyone else, and avoid the
  special case? If there's no tool that relies on the special amdgpu
  prefix then that would be a lot easier.

- If that's not on the table, could we make everyone (with a suitable
  helper or something) just print both variants, so that we again have
  consisent fdinfo output? Or breaks that a different set of existing
  tools.

- Finally maybe could we get away with fixing amd by adding the common
  format there, deprecating the old, fixing the tools that would break and
  then maybe if we're lucky, remove the old one from amdgpu in a year or
  so?

Uapi that's "either do $foo or on this one driver, do $bar" is just
guaranteed to fragement the ecosystem, so imo that should be the absolute
last resort.
-Sima

> > > +
> > >   - drm-shared-<region>: <uint> [KiB|MiB]
> > >   The total size of buffers that are shared with another file (e.g.,
> > > have more
> > > @@ -145,6 +150,9 @@ than a single handle).
> > >   The total size of buffers that including shared and private memory.
> > > +This is an alias for drm-memory-<region> and only one of the two
> > > should be
> > > +present.
> > > +
> > >   - drm-resident-<region>: <uint> [KiB|MiB]
> > >   The total size of buffers that are resident in the specified region.

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Alex Deucher]

On Fri, May 3, 2024 at 11:33 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, May 03, 2024 at 01:58:38PM +0100, Tvrtko Ursulin wrote:
> >
> > [And I forgot dri-devel.. doing well!]
> >
> > On 03/05/2024 13:40, Tvrtko Ursulin wrote:
> > >
> > > [Correcting Christian's email]
> > >
> > > On 03/05/2024 13:36, Tvrtko Ursulin wrote:
> > > > From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).
> > > >
> > > > Document that drm-memory- and drm-total-memory- are aliases to
> > > > prevent any
> > > > confusion in the future.
> > > >
> > > > While at it also clarify that the reserved sub-string 'memory' refers to
> > > > the memory region component.
> > > >
> > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > Cc: Christian König <christian.keonig@amd.com>
> > >
> > > Mea culpa, I copied the mistake from
> > > 77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)
> > >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > > > Cc: Rob Clark <robdclark@chromium.org>
> > > > ---
> > > >   Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
> > > >   1 file changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/gpu/drm-usage-stats.rst
> > > > b/Documentation/gpu/drm-usage-stats.rst
> > > > index 6dc299343b48..ef5c0a0aa477 100644
> > > > --- a/Documentation/gpu/drm-usage-stats.rst
> > > > +++ b/Documentation/gpu/drm-usage-stats.rst
> > > > @@ -128,7 +128,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.
> > > > @@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two
> > > > should be
> > > > +present.
>
> This feels a bit awkward and seems to needlessly complicate fdinfo uapi.
>
> - Could we just patch amdgpu to follow everyone else, and avoid the
>   special case? If there's no tool that relies on the special amdgpu
>   prefix then that would be a lot easier.
>
> - If that's not on the table, could we make everyone (with a suitable
>   helper or something) just print both variants, so that we again have
>   consisent fdinfo output? Or breaks that a different set of existing
>   tools.
>
> - Finally maybe could we get away with fixing amd by adding the common
>   format there, deprecating the old, fixing the tools that would break and
>   then maybe if we're lucky, remove the old one from amdgpu in a year or
>   so?

I'm not really understanding what amdgpu is doing wrong.  It seems to
be following the documentation.  Is the idea that we would like to
deprecate drm-memory-<region> in favor of drm-total-<region>?
If that's the case, I think the 3rd option is probably the best.  We
have a lot of tools and customers using this.  It would have also been
nice to have "memory" in the string for the newer ones to avoid
conflicts with other things that might be a total or shared in the
future, but I guess that ship has sailed.  We should also note that
drm-memory-<region> is deprecated.  While we are here, maybe we should
clarify the semantics of resident, purgeable, and active.  For
example, isn't resident just a duplicate of total?  If the memory was
not resident, it would be in a different region.

Alex

>
> Uapi that's "either do $foo or on this one driver, do $bar" is just
> guaranteed to fragement the ecosystem, so imo that should be the absolute
> last resort.
> -Sima
>
> > > > +
> > > >   - drm-shared-<region>: <uint> [KiB|MiB]
> > > >   The total size of buffers that are shared with another file (e.g.,
> > > > have more
> > > > @@ -145,6 +150,9 @@ than a single handle).
> > > >   The total size of buffers that including shared and private memory.
> > > > +This is an alias for drm-memory-<region> and only one of the two
> > > > should be
> > > > +present.
> > > > +
> > > >   - drm-resident-<region>: <uint> [KiB|MiB]
> > > >   The total size of buffers that are resident in the specified region.
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

[Tvrtko Ursulin]

On 03/05/2024 16:58, Alex Deucher wrote:
> On Fri, May 3, 2024 at 11:33 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Fri, May 03, 2024 at 01:58:38PM +0100, Tvrtko Ursulin wrote:
>>>
>>> [And I forgot dri-devel.. doing well!]
>>>
>>> On 03/05/2024 13:40, Tvrtko Ursulin wrote:
>>>>
>>>> [Correcting Christian's email]
>>>>
>>>> On 03/05/2024 13:36, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).
>>>>>
>>>>> Document that drm-memory- and drm-total-memory- are aliases to
>>>>> prevent any
>>>>> confusion in the future.
>>>>>
>>>>> While at it also clarify that the reserved sub-string 'memory' refers to
>>>>> the memory region component.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>> Cc: Christian König <christian.keonig@amd.com>
>>>>
>>>> Mea culpa, I copied the mistake from
>>>> 77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>> Cc: Rob Clark <robdclark@chromium.org>
>>>>> ---
>>>>>    Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
>>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/gpu/drm-usage-stats.rst
>>>>> b/Documentation/gpu/drm-usage-stats.rst
>>>>> index 6dc299343b48..ef5c0a0aa477 100644
>>>>> --- a/Documentation/gpu/drm-usage-stats.rst
>>>>> +++ b/Documentation/gpu/drm-usage-stats.rst
>>>>> @@ -128,7 +128,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.
>>>>> @@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two
>>>>> should be
>>>>> +present.
>>
>> This feels a bit awkward and seems to needlessly complicate fdinfo uapi.
>>
>> - Could we just patch amdgpu to follow everyone else, and avoid the
>>    special case? If there's no tool that relies on the special amdgpu
>>    prefix then that would be a lot easier.
>>
>> - If that's not on the table, could we make everyone (with a suitable
>>    helper or something) just print both variants, so that we again have
>>    consisent fdinfo output? Or breaks that a different set of existing
>>    tools.
>>
>> - Finally maybe could we get away with fixing amd by adding the common
>>    format there, deprecating the old, fixing the tools that would break and
>>    then maybe if we're lucky, remove the old one from amdgpu in a year or
>>    so?
> 
> I'm not really understanding what amdgpu is doing wrong.  It seems to
> be following the documentation.  Is the idea that we would like to
> deprecate drm-memory-<region> in favor of drm-total-<region>?
> If that's the case, I think the 3rd option is probably the best.  We
> have a lot of tools and customers using this.  It would have also been
> nice to have "memory" in the string for the newer ones to avoid
> conflicts with other things that might be a total or shared in the
> future, but I guess that ship has sailed.  We should also note that
> drm-memory-<region> is deprecated.  While we are here, maybe we should
> clarify the semantics of resident, purgeable, and active.  For
> example, isn't resident just a duplicate of total?  If the memory was
> not resident, it would be in a different region.

Amdgpu isn't doing anything wrong. It just appears when the format was 
discussed no one noticed (me included) that the two keys are not clearly 
described. And it looks there also wasn't a plan to handle the uncelar 
duality in the future.

For me deprecating sounds fine, the 3rd option. I understand we would 
only make amdgpu emit both sets of keys and then remove drm-memory- in 
due time.

With regards to key naming, yeah, memory in the name would have been 
nice. We had a lot of discussion on this topic but ship has indeed 
sailed. It is probably workarble for anything new that might come to add 
their prefix. As long as it does not clash with the memory categories is 
should be fine.

In terms of resident semantics, think of it as VIRT vs RES in top(1). It 
is for drivers which allocate backing store lazily, on first use.

Purgeable is for drivers which have a form of MADV_DONTNEED ie. 
currently have backing store but userspace has indicated it can be 
dropped without preserving the content on memory pressure.

Active is when reservation object says there is activity on the buffer.

Regards,

Tvrtko

> 
> Alex
> 
>>
>> Uapi that's "either do $foo or on this one driver, do $bar" is just
>> guaranteed to fragement the ecosystem, so imo that should be the absolute
>> last resort.
>> -Sima
>>
>>>>> +
>>>>>    - drm-shared-<region>: <uint> [KiB|MiB]
>>>>>    The total size of buffers that are shared with another file (e.g.,
>>>>> have more
>>>>> @@ -145,6 +150,9 @@ than a single handle).
>>>>>    The total size of buffers that including shared and private memory.
>>>>> +This is an alias for drm-memory-<region> and only one of the two
>>>>> should be
>>>>> +present.
>>>>> +
>>>>>    - drm-resident-<region>: <uint> [KiB|MiB]
>>>>>    The total size of buffers that are resident in the specified region.
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

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

[Alex Deucher]

On Fri, May 3, 2024 at 1:06 PM Tvrtko Ursulin <tvrtko.ursulin@igalia.com> wrote:
>
>
> On 03/05/2024 16:58, Alex Deucher wrote:
> > On Fri, May 3, 2024 at 11:33 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Fri, May 03, 2024 at 01:58:38PM +0100, Tvrtko Ursulin wrote:
> >>>
> >>> [And I forgot dri-devel.. doing well!]
> >>>
> >>> On 03/05/2024 13:40, Tvrtko Ursulin wrote:
> >>>>
> >>>> [Correcting Christian's email]
> >>>>
> >>>> On 03/05/2024 13:36, Tvrtko Ursulin wrote:
> >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).
> >>>>>
> >>>>> Document that drm-memory- and drm-total-memory- are aliases to
> >>>>> prevent any
> >>>>> confusion in the future.
> >>>>>
> >>>>> While at it also clarify that the reserved sub-string 'memory' refers to
> >>>>> the memory region component.
> >>>>>
> >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> >>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
> >>>>> Cc: Christian König <christian.keonig@amd.com>
> >>>>
> >>>> Mea culpa, I copied the mistake from
> >>>> 77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)
> >>>>
> >>>> Regards,
> >>>>
> >>>> Tvrtko
> >>>>
> >>>>> Cc: Rob Clark <robdclark@chromium.org>
> >>>>> ---
> >>>>>    Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
> >>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/Documentation/gpu/drm-usage-stats.rst
> >>>>> b/Documentation/gpu/drm-usage-stats.rst
> >>>>> index 6dc299343b48..ef5c0a0aa477 100644
> >>>>> --- a/Documentation/gpu/drm-usage-stats.rst
> >>>>> +++ b/Documentation/gpu/drm-usage-stats.rst
> >>>>> @@ -128,7 +128,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.
> >>>>> @@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two
> >>>>> should be
> >>>>> +present.
> >>
> >> This feels a bit awkward and seems to needlessly complicate fdinfo uapi.
> >>
> >> - Could we just patch amdgpu to follow everyone else, and avoid the
> >>    special case? If there's no tool that relies on the special amdgpu
> >>    prefix then that would be a lot easier.
> >>
> >> - If that's not on the table, could we make everyone (with a suitable
> >>    helper or something) just print both variants, so that we again have
> >>    consisent fdinfo output? Or breaks that a different set of existing
> >>    tools.
> >>
> >> - Finally maybe could we get away with fixing amd by adding the common
> >>    format there, deprecating the old, fixing the tools that would break and
> >>    then maybe if we're lucky, remove the old one from amdgpu in a year or
> >>    so?
> >
> > I'm not really understanding what amdgpu is doing wrong.  It seems to
> > be following the documentation.  Is the idea that we would like to
> > deprecate drm-memory-<region> in favor of drm-total-<region>?
> > If that's the case, I think the 3rd option is probably the best.  We
> > have a lot of tools and customers using this.  It would have also been
> > nice to have "memory" in the string for the newer ones to avoid
> > conflicts with other things that might be a total or shared in the
> > future, but I guess that ship has sailed.  We should also note that
> > drm-memory-<region> is deprecated.  While we are here, maybe we should
> > clarify the semantics of resident, purgeable, and active.  For
> > example, isn't resident just a duplicate of total?  If the memory was
> > not resident, it would be in a different region.
>
> Amdgpu isn't doing anything wrong. It just appears when the format was
> discussed no one noticed (me included) that the two keys are not clearly
> described. And it looks there also wasn't a plan to handle the uncelar
> duality in the future.
>
> For me deprecating sounds fine, the 3rd option. I understand we would
> only make amdgpu emit both sets of keys and then remove drm-memory- in
> due time.
>
> With regards to key naming, yeah, memory in the name would have been
> nice. We had a lot of discussion on this topic but ship has indeed
> sailed. It is probably workarble for anything new that might come to add
> their prefix. As long as it does not clash with the memory categories is
> should be fine.
>
> In terms of resident semantics, think of it as VIRT vs RES in top(1). It
> is for drivers which allocate backing store lazily, on first use.
>
> Purgeable is for drivers which have a form of MADV_DONTNEED ie.
> currently have backing store but userspace has indicated it can be
> dropped without preserving the content on memory pressure.
>
> Active is when reservation object says there is activity on the buffer.


I think you have the makings for a good patch right here :)

Alex

>
> Regards,
>
> Tvrtko
>
> >
> > Alex
> >
> >>
> >> Uapi that's "either do $foo or on this one driver, do $bar" is just
> >> guaranteed to fragement the ecosystem, so imo that should be the absolute
> >> last resort.
> >> -Sima
> >>
> >>>>> +
> >>>>>    - drm-shared-<region>: <uint> [KiB|MiB]
> >>>>>    The total size of buffers that are shared with another file (e.g.,
> >>>>> have more
> >>>>> @@ -145,6 +150,9 @@ than a single handle).
> >>>>>    The total size of buffers that including shared and private memory.
> >>>>> +This is an alias for drm-memory-<region> and only one of the two
> >>>>> should be
> >>>>> +present.
> >>>>> +
> >>>>>    - drm-resident-<region>: <uint> [KiB|MiB]
> >>>>>    The total size of buffers that are resident in the specified region.
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch

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

[Daniel Vetter]

On Fri, May 03, 2024 at 06:06:03PM +0100, Tvrtko Ursulin wrote:
> 
> On 03/05/2024 16:58, Alex Deucher wrote:
> > On Fri, May 3, 2024 at 11:33 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > 
> > > On Fri, May 03, 2024 at 01:58:38PM +0100, Tvrtko Ursulin wrote:
> > > > 
> > > > [And I forgot dri-devel.. doing well!]
> > > > 
> > > > On 03/05/2024 13:40, Tvrtko Ursulin wrote:
> > > > > 
> > > > > [Correcting Christian's email]
> > > > > 
> > > > > On 03/05/2024 13:36, Tvrtko Ursulin wrote:
> > > > > > From: Tvrtko Ursulin <tvrtko.ursulin@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 total memory use (including shared).
> > > > > > 
> > > > > > Document that drm-memory- and drm-total-memory- are aliases to
> > > > > > prevent any
> > > > > > confusion in the future.
> > > > > > 
> > > > > > While at it also clarify that the reserved sub-string 'memory' refers to
> > > > > > the memory region component.
> > > > > > 
> > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > > Cc: Christian König <christian.keonig@amd.com>
> > > > > 
> > > > > Mea culpa, I copied the mistake from
> > > > > 77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Tvrtko
> > > > > 
> > > > > > Cc: Rob Clark <robdclark@chromium.org>
> > > > > > ---
> > > > > >    Documentation/gpu/drm-usage-stats.rst | 10 +++++++++-
> > > > > >    1 file changed, 9 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/Documentation/gpu/drm-usage-stats.rst
> > > > > > b/Documentation/gpu/drm-usage-stats.rst
> > > > > > index 6dc299343b48..ef5c0a0aa477 100644
> > > > > > --- a/Documentation/gpu/drm-usage-stats.rst
> > > > > > +++ b/Documentation/gpu/drm-usage-stats.rst
> > > > > > @@ -128,7 +128,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.
> > > > > > @@ -136,6 +138,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 is an alias for drm-total-<region> and only one of the two
> > > > > > should be
> > > > > > +present.
> > > 
> > > This feels a bit awkward and seems to needlessly complicate fdinfo uapi.
> > > 
> > > - Could we just patch amdgpu to follow everyone else, and avoid the
> > >    special case? If there's no tool that relies on the special amdgpu
> > >    prefix then that would be a lot easier.
> > > 
> > > - If that's not on the table, could we make everyone (with a suitable
> > >    helper or something) just print both variants, so that we again have
> > >    consisent fdinfo output? Or breaks that a different set of existing
> > >    tools.
> > > 
> > > - Finally maybe could we get away with fixing amd by adding the common
> > >    format there, deprecating the old, fixing the tools that would break and
> > >    then maybe if we're lucky, remove the old one from amdgpu in a year or
> > >    so?
> > 
> > I'm not really understanding what amdgpu is doing wrong.  It seems to
> > be following the documentation.  Is the idea that we would like to
> > deprecate drm-memory-<region> in favor of drm-total-<region>?
> > If that's the case, I think the 3rd option is probably the best.  We
> > have a lot of tools and customers using this.  It would have also been
> > nice to have "memory" in the string for the newer ones to avoid
> > conflicts with other things that might be a total or shared in the
> > future, but I guess that ship has sailed.  We should also note that
> > drm-memory-<region> is deprecated.  While we are here, maybe we should
> > clarify the semantics of resident, purgeable, and active.  For
> > example, isn't resident just a duplicate of total?  If the memory was
> > not resident, it would be in a different region.
> 
> Amdgpu isn't doing anything wrong. It just appears when the format was
> discussed no one noticed (me included) that the two keys are not clearly
> described. And it looks there also wasn't a plan to handle the uncelar
> duality in the future.

Yeah I didnt want to imply that amdgpu did anything wrong, just that if we
have a spec where everyone does one thing, except one driver, that's a
really unfortunate situation that will cause endless amounts of pains to
userspace people.

Like entirely different example, but vmwgfx started out as a driver not
using gem buffer ids for it's per-fd buffer objects. And after a decade
they switched because aside from their own vmwgfx specific userspace just
about no-one got the memo. Despite that it was all documented and designed
to allow that case, and we tried to tilt that windmill for years
educating userspace.

Anyway I think you have I plan, I'm out :-)
-Sima

> For me deprecating sounds fine, the 3rd option. I understand we would only
> make amdgpu emit both sets of keys and then remove drm-memory- in due time.
> 
> With regards to key naming, yeah, memory in the name would have been nice.
> We had a lot of discussion on this topic but ship has indeed sailed. It is
> probably workarble for anything new that might come to add their prefix. As
> long as it does not clash with the memory categories is should be fine.
> 
> In terms of resident semantics, think of it as VIRT vs RES in top(1). It is
> for drivers which allocate backing store lazily, on first use.
> 
> Purgeable is for drivers which have a form of MADV_DONTNEED ie. currently
> have backing store but userspace has indicated it can be dropped without
> preserving the content on memory pressure.
> 
> Active is when reservation object says there is activity on the buffer.
> 
> Regards,
> 
> Tvrtko
> 
> > 
> > Alex
> > 
> > > 
> > > Uapi that's "either do $foo or on this one driver, do $bar" is just
> > > guaranteed to fragement the ecosystem, so imo that should be the absolute
> > > last resort.
> > > -Sima
> > > 
> > > > > > +
> > > > > >    - drm-shared-<region>: <uint> [KiB|MiB]
> > > > > >    The total size of buffers that are shared with another file (e.g.,
> > > > > > have more
> > > > > > @@ -145,6 +150,9 @@ than a single handle).
> > > > > >    The total size of buffers that including shared and private memory.
> > > > > > +This is an alias for drm-memory-<region> and only one of the two
> > > > > > should be
> > > > > > +present.
> > > > > > +
> > > > > >    - drm-resident-<region>: <uint> [KiB|MiB]
> > > > > >    The total size of buffers that are resident in the specified region.
> > > 
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch