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

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 2/19/2026 1:48 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)
> v3:
>   - Refactor ioctl failure check. (Vinay)
>   - Check q.size only once. (Vinay)
>   - Add num_bytes for asprintf return value. (Vinay)
>
> 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 | 108 ++++++++++++++++++++++++++---------
>   tools/gputop.src/xe_gputop.h |   3 +-
>   2 files changed, 83 insertions(+), 28 deletions(-)
>
> diff --git a/tools/gputop.src/xe_gputop.c b/tools/gputop.src/xe_gputop.c
> index bb2caa6ea..d8892c587 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,50 @@ 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 || q.size == 0)
> +		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);
> +
> +	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))
Need to check if card->card is null first before strlen? This is not 
part of this patch, so can fix in a separate patch.
> -		return NULL;
> +		goto err;
>   
>   	if (strlen(card->card)) {
>   		card_fd = igt_open_card(card);
> @@ -142,39 +175,52 @@ 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) +

again, can use calloc in place of malloc/memset?

Other than that,

LGTM,

Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

> -	       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) {
> +		int num_bytes = 0;
>   		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;
>   
> @@ -184,12 +230,12 @@ void *xe_populate_engines(const void *obj, int index)
>   			break;
>   		}
>   
> -		ret = asprintf(&engine->display_name, "GT:%u %s/%u",
> -			       hwe->gt_id,
> -			       class_display_name(engine->drm_xe_engine.engine_class),
> -			       engine->drm_xe_engine.engine_instance);
> +		num_bytes = asprintf(&engine->display_name, "GT:%u %s/%u",
> +				     hwe.gt_id,
> +				     class_display_name(engine->drm_xe_engine.engine_class),
> +				     engine->drm_xe_engine.engine_instance);
>   
> -		if (ret <= 0) {
> +		if (num_bytes <= 0) {
>   			ret = errno;
>   			break;
>   		}
> @@ -197,9 +243,11 @@ void *xe_populate_engines(const void *obj, int index)
>   		engines->num_engines++;
>   	}
>   
> -	if (!ret) {
> +	free(engine_q);
> +
> +	if (ret) {
>   		errno = ret;
> -		return NULL;
> +		goto err;
>   	}
>   
>   	qsort(engine_ptr(engines, 0), engines->num_engines,
> @@ -208,6 +256,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 +320,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;
>   }
> @@ -359,4 +416,3 @@ int xe_print_engines(const void *obj, int index, int lines, int w, int h)
>   
>   	return lines;
>   }
> -
> 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;