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