From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <3c9d4c04-762c-730d-cc93-125cf7bc6f39@linux.intel.com> Date: Fri, 28 Jul 2023 15:46:33 +0100 MIME-Version: 1.0 Content-Language: en-US From: Tvrtko Ursulin To: Kamil Konieczny , igt-dev@lists.freedesktop.org, Intel-gfx@lists.freedesktop.org, Tvrtko Ursulin , Rob Clark , Chris Healy References: <20230727092025.1895728-1-tvrtko.ursulin@linux.intel.com> <20230727092025.1895728-3-tvrtko.ursulin@linux.intel.com> <20230727141059.lu76jilirgvcjb5f@kamilkon-desk1> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [igt-dev] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 >>> >>> 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 >>> Cc: Rob Clark >>> Cc: Chris Healy >>> Cc: Kamil Konieczny >>> --- >>>   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