Re: [PATCH i-g-t v7 3/5] tools/gputop.src/xe_gputop: Refactor xe_populate_engines to close card_fd and use direct ioctl calls

Purkait, Soham
2026-02-18T15:27:24Z

[-- Attachment #1: Type: text/plain, Size: 8491 bytes --]

Hi Vinay

On 12-02-2026 07:11, Belgaumkar, Vinay wrote:
>
>
> On 1/30/2026 1:53 AM, Soham Purkait wrote:
>> During GPUTOP engine initialization, the opened card fd is required to
>> query engines and obtain configurations for PMU busyness calculation, but
>> leaving it open causes the process to appear as a DRM client, which
>> conflicts with KMS test requirements. To avoid this, card_fd is closed
>> immediately after engine discovery, and engine queries are refactored to
>> remove the lib/xe/* dependency in favor of direct ioctl calls, simplifying
>> the implementation and reducing igt test library dependencies.
>>
>> v1:
>> - Initialize pmu_device_obj to null.
>> v2:
>> - Remove dependency on lib/xe/* libraries. (Kamil)
>>
>> Fixes: c8106465683f ("tools/gputop/xe_gputop: Add gputop support for xe specific devices")
>> Signed-off-by: Soham Purkait<soham.purkait@intel.com>
>> ---
>> tools/gputop.src/xe_gputop.c | 100 ++++++++++++++++++++++++++++-------
>> tools/gputop.src/xe_gputop.h | 3 +-
>> 2 files changed, 81 insertions(+), 22 deletions(-)
>>
>> diff --git a/tools/gputop.src/xe_gputop.c b/tools/gputop.src/xe_gputop.c
>> index bb2caa6ea..7793cd62f 100644
>> --- a/tools/gputop.src/xe_gputop.c
>> +++ b/tools/gputop.src/xe_gputop.c
>> @@ -1,8 +1,10 @@
>> // SPDX-License-Identifier: MIT
>> /*
>> - * Copyright © 2025 Intel Corporation
>> + * Copyright © 2025-2026 Intel Corporation
>> */
>>
>> +#include <sys/ioctl.h>
>> +
>> #include "xe_gputop.h"
>>
>> #define engine_ptr(pmu_device, n) (&(pmu_device)->engine + (n))
>> @@ -92,6 +94,7 @@ void xe_gputop_init(void *ptr, int index,
>>
>> obj = ((struct xe_gputop *)ptr) + index;
>> obj->card = card;
>> + obj->pmu_device_obj = NULL;
>> }
>>
>> static int pmu_format_shift(int xe, const char *name)
>> @@ -121,20 +124,54 @@ static int engine_cmp(const void *__a, const void *__b)
>> return a->drm_xe_engine.engine_instance - b->drm_xe_engine.engine_instance;
>> }
>>
>> +static int engine_query(int fd, struct drm_xe_query_engines **engine_q)
>> +{
>> + struct drm_xe_device_query q;
>> + int ret = 0, num_eng;
>> +
>> + memset(&q, 0, sizeof(q));
>> + q.query = DRM_XE_DEVICE_QUERY_ENGINES;
>> +
>> + ret = ioctl(fd, DRM_IOCTL_XE_DEVICE_QUERY, &q);
>> + if (ret != 0) {
>> + if ((errno != ENOSPC && errno != E2BIG) || q.size == 0)
> why are we ignoring ENOSPC and E2BIG?
I have rectified this check. Seems like the driver is not returning any
of these during size check.
>> + return -1;
>> + }
>> + if (q.size == 0)
> we can check q.size just once instead of twice?
Sure.
>> + return 0;
>> +
>> + num_eng = (int)((q.size - sizeof(struct drm_xe_query_engines)) /
>> + sizeof(struct drm_xe_engine_class_instance));
>> +
>> + *engine_q = (struct drm_xe_query_engines *)calloc(1, q.size);
>> + if (!*engine_q)
>> + return -1;
>> +
>> + q.data = (uintptr_t)(*engine_q);
> better to use to_user_pointer from the lib?
For igt "tools" it has been asked to not use any library function from
lib as these are meant to be used only with IGT tests.
>> +
>> + ret = ioctl(fd, DRM_IOCTL_XE_DEVICE_QUERY, &q);
>> + if (ret != 0) {
>> + free(*engine_q);
>> + return -1;
>> + }
>> +
>> + return num_eng;
>> +}
>> +
>> void *xe_populate_engines(const void *obj, int index)
>> {
>> struct xe_gputop *ptr = ((struct xe_gputop *)obj) + index;
>> struct igt_device_card *card = ptr->card;
>> uint64_t engine_active_config, engine_total_config;
>> uint64_t engine_class, engine_instance, gt_shift;
>> - struct drm_xe_engine_class_instance *hwe;
>> + struct drm_xe_query_engines *engine_q;
>> struct xe_pmu_device *engines;
>> char device[30];
>> - int ret = 0;
>> - int card_fd;
>> + int ret = 0, num_eng;
>> + int card_fd = -1;
>>
>> if (!card || !strlen(card->card) || !strlen(card->render))
>> - return NULL;
>> + goto err;
>>
>> if (strlen(card->card)) {
>> card_fd = igt_open_card(card);
>> @@ -142,39 +179,51 @@ void *xe_populate_engines(const void *obj, int index)
>> card_fd = igt_open_render(card);
>> } else {
>> fprintf(stderr, "Failed to detect device!\n");
>> - return NULL;
>> + goto err;
>> }
>> - xe_device_get(card_fd);
>> +
>> + num_eng = engine_query(card_fd, &engine_q);
>> + if (num_eng <= 0) {
>> + fprintf(stderr, "Engine query failed!\n");
>> + goto err;
>> + }
>> +
>> engines = malloc(sizeof(struct xe_pmu_device) +
>> - xe_number_engines(card_fd) * sizeof(struct xe_engine));
>> + num_eng * sizeof(struct xe_engine));
>> if (!engines)
>> - return NULL;
>> + goto err;
>>
>> memset(engines, 0, sizeof(struct xe_pmu_device) +
>> - xe_number_engines(card_fd) * sizeof(struct xe_engine));
>> + num_eng * sizeof(struct xe_engine));
>>
>> engines->num_engines = 0;
>> gt_shift = pmu_format_shift(card_fd, "gt");
>> engine_class = pmu_format_shift(card_fd, "engine_class");
>> engine_instance = pmu_format_shift(card_fd, "engine_instance");
>> xe_perf_device(card_fd, device, sizeof(device));
>> + close(card_fd);
>> + card_fd = -1;
>> +
>> engines->device = strdup(device);
>> ret = perf_event_config(device, "engine-active-ticks", &engine_active_config);
>> if (ret < 0)
>> - return NULL;
>> + goto err;
>>
>> ret = perf_event_config(device, "engine-total-ticks", &engine_total_config);
>> if (ret < 0)
>> - return NULL;
>> + goto err;
>>
>> - xe_for_each_engine(card_fd, hwe) {
>> + while (engines->num_engines < engine_q->num_engines) {
>> uint64_t param_config;
>> struct xe_engine *engine;
>> + struct drm_xe_engine_class_instance hwe;
>> +
>> + hwe = engine_q->engines[engines->num_engines].instance;
>>
>> engine = engine_ptr(engines, engines->num_engines);
>> - param_config = (uint64_t)hwe->gt_id << gt_shift | hwe->engine_class << engine_class
>> - | hwe->engine_instance << engine_instance;
>> - engine->drm_xe_engine = *hwe;
>> + param_config = (uint64_t)hwe.gt_id << gt_shift | hwe.engine_class << engine_class
>> + | hwe.engine_instance << engine_instance;
>> + engine->drm_xe_engine = hwe;
>> engine->engine_active_ticks.config = engine_active_config | param_config;
>> engine->engine_total_ticks.config = engine_total_config | param_config;
>>
>> @@ -185,7 +234,7 @@ void *xe_populate_engines(const void *obj, int index)
>> }
>>
>> ret = asprintf(&engine->display_name, "GT:%u %s/%u",
>> - hwe->gt_id,
>> + hwe.gt_id,
>> class_display_name(engine->drm_xe_engine.engine_class),
>> engine->drm_xe_engine.engine_instance);
>>
>> @@ -197,9 +246,11 @@ void *xe_populate_engines(const void *obj, int index)
>> engines->num_engines++;
>> }
>>
>> + free(engine_q);
>> +
>> if (!ret) {
>
> Not part of this patch, but looks like ret is overloaded here for
> multiple things. Usually ret = 0 is a good thing? Should use num_bytes
> or something if we expect asprintf above to return a positive value.
> We should fix this as a separate patch.
>
Sure.

Thanks,
Soham

> Thanks,
>
> Vinay.
>
>> errno = ret;
>> - return NULL;
>> + goto err;
>> }
>>
>> qsort(engine_ptr(engines, 0), engines->num_engines,
>> @@ -208,6 +259,15 @@ void *xe_populate_engines(const void *obj, int index)
>> ptr->pmu_device_obj = engines;
>>
>> return engines;
>> +
>> +err:
>> + if (card_fd >= 0)
>> + close(card_fd);
>> + if (engines)
>> + free(engines);
>> + if (engine_q)
>> + free(engine_q);
>> + return NULL;
>> }
>>
>> static uint64_t pmu_read_multi(int fd, unsigned int num, uint64_t *val)
>> @@ -263,11 +323,11 @@ int xe_pmu_init(const void *obj, int index)
>> fd = _open_pmu(type, &engines->num_counters, &engine->engine_active_ticks,
>> &engines->fd);
>> if (fd < 0)
>> - return -1;
>> + return fd;
>> fd = _open_pmu(type, &engines->num_counters, &engine->engine_total_ticks,
>> &engines->fd);
>> if (fd < 0)
>> - return -1;
>> + return fd;
>> }
>> return 0;
>> }
>> diff --git a/tools/gputop.src/xe_gputop.h b/tools/gputop.src/xe_gputop.h
>> index 1e3856071..dcd82de7d 100644
>> --- a/tools/gputop.src/xe_gputop.h
>> +++ b/tools/gputop.src/xe_gputop.h
>> @@ -7,11 +7,10 @@
>> #define __XE_GPUTOP_H__
>>
>> #include <dirent.h>
>> +#include <xe_drm.h>
>>
>> -#include "igt_device_scan.h"
>> #include "igt_perf.h"
>> #include "utils.h"
>> -#include "xe/xe_query.h"
>>
>> struct xe_pmu_pair {
>> uint64_t cur;

[-- Attachment #2: Type: text/html, Size: 9943 bytes --]