[PATCH v8 3/5] Documentation/gpu: Clarify drm memory stats definition

[Li, Yunxiang (Teddy)]

[Public]

> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
> Sent: Monday, November 18, 2024 9:38
> On 16/11/2024 04:44, Yunxiang Li wrote:
> > Define how to handle buffers with multiple possible placement so we
> > don't get incompatible implementations. Callout the resident
> > requirement for drm-purgeable- explicitly. Remove the requirement for
> > there to be only drm-memory- or only drm-resident-, it's not what's
> > implemented and having both is better for back-compat. Also re-order
> > the paragraphs to flow better.
> >
> > Signed-off-by: Yunxiang Li <Yunxiang.Li at amd.com>
> > CC: dri-devel at lists.freedesktop.org
> > ---
> >   Documentation/gpu/drm-usage-stats.rst | 36 ++++++++++++---------------
> >   1 file changed, 16 insertions(+), 20 deletions(-)
> >
> > diff --git a/Documentation/gpu/drm-usage-stats.rst
> > b/Documentation/gpu/drm-usage-stats.rst
> > index ff964c707754a..973663f91a292 100644
> > --- a/Documentation/gpu/drm-usage-stats.rst
> > +++ b/Documentation/gpu/drm-usage-stats.rst
> > @@ -140,13 +140,9 @@ both.
> >   Memory
> >   ^^^^^^
> >
> > -- drm-memory-<region>: <uint> [KiB|MiB]
> > -
> > -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 region name "memory" is reserved to refer to normal system 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 used as
> the "<region>"
> > +string. 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.
> > @@ -154,31 +150,27 @@ 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.
>
> IMO the second sentence should stay in principle (I mean at the new location,
> where you moved it). Intent is to avoid new implementations adding both keys. The
> fact amdgpu has both is not relevant for that purpose. We don't want someone just
> reading it is an alias and having to have any doubt whether they need to output both
> or not.

I see, yeah I will mention in the drm-memory- part that that tag is legacy amdgpu only behavior.

> > +- drm-total-<region>: <uint> [KiB|MiB]
> > +
> > +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 count under this category. To avoid double
> > +counting, if a buffer falls under multiple regions, the
> > +implementation should pick only one of the regions, and do so in a consistent
> manner.
>
> Addition feels fine to me in principle. I would only suggest rewording it a bit to avoid
> ambiguity about what it means to "fall under". Perhaps like this:
>
> To avoid double counting when buffers can be instantiated in one of the multiple
> allowed memory regions, the implementation should account the total against only
> one of the regions, and should pick this region in a consistent manner.

Ack

> >
> >   - drm-shared-<region>: <uint> [KiB|MiB]
> >
> >   The total size of buffers that are shared with another file (e.g.,
> > have more -than a single handle).
> > -
> > -- drm-total-<region>: <uint> [KiB|MiB]
> > -
> > -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.
> > +than a single handle). Same caveat as drm-total- applies.
>
> I suggest to explicitly point out the caveat is the multiple region one.

and Ack

> >
> >   - drm-resident-<region>: <uint> [KiB|MiB]
> >
> >   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.
>
> I think it does not harm to keep this note at both keys. Or at least make one
> reference the other for this point specifically.

Might be easier to just have drm-memory- as a foot note here, instead of its own section

> > -
> >   - drm-purgeable-<region>: <uint> [KiB|MiB]
> >
> > -The total size of buffers that are purgeable.
> > +The total size of buffers that are resident and purgeable.
>
> Is it not redundant? How could something not resident be purgeable in the first
> place?

There is the possible confusion between buffers having a purgeable bit and buffers in a state that is purgeable, I feel like it's worth an explicit callout since there's also code comments about this difference.

> >   For example drivers which implement a form of 'madvise' like functionality can
> >   here count buffers which have instantiated backing store, but have
> > been marked @@ -192,6 +184,10 @@ 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.
> >
> > +- drm-memory-<region>: <uint> [KiB|MiB]
> > +
> > +This key is deprecated and is an alias for drm-resident-<region> if present.
> > +
> >   Implementation Details
> >   ======================
> >
>
> Regards,
>
> Tvrtko