Re: [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/07/2023 16:17, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 27/07/2023 15:10, Kamil Konieczny wrote:
>> Hi Tvrtko,
>>
>> On 2023-07-27 at 10:20:24 +0100, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Define the storage structure and copy over memory data as parsed by the
>>> fdinfo helpers.
>>>
>>> v2:
>>>   * Fix empty region map entry skip condition. (Kamil, Chris)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Rob Clark <robdclark@chromium.org>
>>> Cc: Chris Healy <cphealy@gmail.com>
>>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>> ---
>>>   lib/igt_drm_clients.c | 32 ++++++++++++++++++++++++++++++++
>>>   lib/igt_drm_clients.h | 11 +++++++++++
>>>   2 files changed, 43 insertions(+)
>>>
>>> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
>>> index fdea42752a81..47ad137d5f1f 100644
>>> --- a/lib/igt_drm_clients.c
>>> +++ b/lib/igt_drm_clients.c
>>> @@ -103,6 +103,8 @@ igt_drm_client_update(struct igt_drm_client *c, 
>>> unsigned int pid, char *name,
>>>               c->clients->max_name_len = len;
>>>       }
>>> +    /* Engines */
>>> +
>>>       c->last_runtime = 0;
>>>       c->total_runtime = 0;
>>> @@ -118,6 +120,13 @@ igt_drm_client_update(struct igt_drm_client *c, 
>>> unsigned int pid, char *name,
>>>           c->last[i] = info->busy[i];
>>>       }
>>> +    /* Memory regions */
>>> +    for (i = 0; i <= c->regions->max_region_id; i++) {
>>> +        assert(i < ARRAY_SIZE(info->region_mem));
>>> +
>>> +        c->memory[i] = info->region_mem[i];
>>> +    }
>>> +
>>>       c->samples++;
>>>       c->status = IGT_DRM_CLIENT_ALIVE;
>>>   }
>>> @@ -154,6 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>>>       c->id = info->id;
>>>       c->drm_minor = drm_minor;
>>>       c->clients = clients;
>>> +
>>> +    /* Engines */
>>>       c->engines = malloc(sizeof(*c->engines));
>>>       assert(c->engines);
>>>       memset(c->engines, 0, sizeof(*c->engines));
>>> @@ -178,6 +189,27 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>>>       c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
>>>       assert(c->val && c->last);
>>> +    /* Memory regions */
>>> +    c->regions = malloc(sizeof(*c->regions));
>>> +    assert(c->regions);
>>> +    memset(c->regions, 0, sizeof(*c->regions));
>>> +    c->regions->names = calloc(info->last_region_index + 1,
>>> +                   sizeof(*c->regions->names));
>>> +    assert(c->regions->names);
>>> +
>>> +    for (i = 0; i <= info->last_region_index; i++) {
>>> +        /* Region map is allowed to be sparse. */
>>> +        if (!info->region_names[i][0])
>>> +            continue;
>>> +
>>> +        c->regions->names[i] = strdup(info->region_names[i]);
>> --------------------------------- ^
>> Should this be c->regions->num_regions?
> 
> No, it was supposed to carry over the same memory region index from the 
> region map provided to igt_parse_drm_fdinfo.
> 
> I copy pasted that concept from engine names (class names for i915) but, 
> given it is unused, maybe I should drop it.
> 
> Gputop does not need it, at least not yet, and I haven't thought much if 
> it will be useful for intel_gpu_top. Point is, it allows passing in 
> fixed region id to name mapping, which can simplify things and guarantee 
> order of memory regions in the arrays. (Otherwise the order would depend 
> on the order of keys in the fdinfo text.)

I think I'd like to keep this functionality for now. I do promise to rip 
it out later, should I end up not needing it for intel_gpu_top after all.

Regards,

Tvrtko