From: Daniel Vetter <daniel@ffwll.ch>
To: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: "Alex Deucher" <alexdeucher@gmail.com>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Tvrtko Ursulin" <tursulin@igalia.com>,
amd-gfx@lists.freedesktop.org, kernel-dev@igalia.com,
"Alex Deucher" <alexander.deucher@amd.com>,
"Rob Clark" <robdclark@chromium.org>,
"Christian König" <christian.koenig@amd.com>,
dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] Documentation/gpu: Document the situation with unqualified drm-memory-
Date: Mon, 6 May 2024 14:56:09 +0200 [thread overview]
Message-ID: <ZjjTaeZYNqVSj2y-@phenom.ffwll.local> (raw)
In-Reply-To: <4705c6e4-04e3-4f97-9f9a-629b6495e92a@igalia.com>
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
prev parent reply other threads:[~2024-05-06 12:56 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240503123657.9441-1-tursulin@igalia.com>
[not found] ` <736ba0a2-035b-4727-bbcc-437029420377@igalia.com>
2024-05-03 12:58 ` [PATCH] Documentation/gpu: Document the situation with unqualified drm-memory- Tvrtko Ursulin
2024-05-03 13:39 ` Alex Deucher
2024-05-03 13:55 ` Tvrtko Ursulin
2024-05-03 15:33 ` Daniel Vetter
2024-05-03 15:58 ` Alex Deucher
2024-05-03 17:06 ` Tvrtko Ursulin
2024-05-03 18:25 ` Alex Deucher
2024-05-06 12:56 ` Daniel Vetter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjjTaeZYNqVSj2y-@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=alexander.deucher@amd.com \
--cc=alexdeucher@gmail.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=christian.koenig@amd.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=kernel-dev@igalia.com \
--cc=robdclark@chromium.org \
--cc=tursulin@igalia.com \
--cc=tvrtko.ursulin@igalia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).