[Public] > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx> > 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@xxxxxxx> > > CC: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > --- > > 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
