Re: [PATCH i-g-t] lib/igt_drm_fdinfo: Handle amdgpu memory stats

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 03/05/2024 14:32, Lucas De Marchi wrote:
> On Fri, May 03, 2024 at 01:37:31PM GMT, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> Code so far only handles the clients using the common DRM helper.
>>
>> Handle the amdgpu driver which uses a slightly different set of keys. 
>> More
>> specifically, outputs drm-memory-<region> instead of drm-total-<region>.
>>
>> With this added gputop starts showing total memory usage for amdgpu.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Christian König <christian.keonig@amd.com>
>> Cc: Rob Clark <robdclark@chromium.org>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>> lib/igt_drm_fdinfo.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
>> index cab677df29a9..89992f1477ec 100644
>> --- a/lib/igt_drm_fdinfo.c
>> +++ b/lib/igt_drm_fdinfo.c
>> @@ -262,6 +262,10 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, 
>> struct drm_client_fdinfo *info,
>>             idx = parse_region(l, info, strlen("drm-total-"),
>>                        region_map, region_entries, &val);
>>             UPDATE_REGION(idx, total, val);
>> +        } else if (!strncmp(l, "drm-memory-", 11)) {
>> +            idx = parse_region(l, info, strlen("drm-memory-"),
>> +                       region_map, region_entries, &val);
>> +            UPDATE_REGION(idx, total, val);
> 
> are they the same thing? Should we clarify the doc that a driver can't
> use both drm-memory- and drm-total-?
> 
>      - 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 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.
>      Default unit shall be bytes with optional unit specifiers of 'KiB' 
> or 'MiB'     indicating kibi- or mebi-bytes.
>      - drm-total-<region>: <uint> [KiB|MiB]
> 
>      The total size of buffers that including shared and private memory.
> 
> If they are the same and the total field can just be re-used,

Yep exactly, they are the same in practice so we are lucky that we are 
able to fix it still. See: 
https://lore.kernel.org/amd-gfx/CADnq5_NAO_Ao0EJTO=MJxvR-KJkF1WCwKGV-9ami7qQdzf029w@mail.gmail.com/T/#t 
(I forgot to CC dri-devel when posting the patch.)

> 
> 
>      Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Thanks! Are you okay if I put this in first since it collides with your 
fdinfo rework?

Although I will wait for the drm-usage-stats.rst patch to get approved 
first.

Regards,

Tvrtko

> Lucas De Marchi
> 
>>         } else if (!strncmp(l, "drm-shared-", 11)) {
>>             idx = parse_region(l, info, strlen("drm-shared-"),
>>                        region_map, region_entries, &val);
>> -- 
>> 2.44.0
>>