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

Daniel Vetter daniel at ffwll.ch
Mon May 6 12:56:09 UTC 2024 

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 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.

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

More information about the amd-gfx mailing list