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" <soham.purkait@intel.com>]

[-- 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 --]