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

Fri May 3 18:25:53 UTC 2024

On Fri, May 3, 2024 at 1:06 PM Tvrtko Ursulin <tvrtko.ursulin at igalia.com> wrote:
>
>
> On 03/05/2024 16:58, Alex Deucher wrote:
> > On Fri, May 3, 2024 at 11:33 AM Daniel Vetter <daniel at 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 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 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 at igalia.com>
> >>>>> Cc: Alex Deucher <alexander.deucher at amd.com>
> >>>>> Cc: Christian König <christian.keonig at amd.com>
> >>>>
> >>>> Mea culpa, I copied the mistake from
> >>>> 77d17c4cd0bf52eacfad88e63e8932eb45d643c5. :)
> >>>>
> >>>> Regards,
> >>>>
> >>>> Tvrtko
> >>>>
> >>>>> Cc: Rob Clark <robdclark at 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