[PATCH i-g-t v11 0/5] Add per-device engine activity stats in GPUTOP

[PATCH i-g-t v11 0/5] Add per-device engine activity stats in GPUTOP

[Soham Purkait]

     Add per-device engine activity stat support in GPUTOP.
This leverages the PMU interface to display the activity of
engine instances for the array of requested or all devices.
    This patch refactors GPUTOP to be vendor-agnostic, 
laying the groundwork for future support of multiple 
GPU vendors.
    Currently, GPUTOP supports GPUs with Xe driver only
and can monitor the engine activities of multiple GPU
devices simultaneously through abstracting vendor 
specific code into a common interface and implementing 
vendor-neutral APIs for monitoring.

DRIVER: xe || BDF: 0000:00:02.0
ENGINES         BUSY
Render/3D/0   | 96.5% ███████████████████████████████████████ |
Blitter/0     | 91.6% █████████████████████████████████████   |
Video/0       | 56.2% ███████████████████████████             |
VideoEnhance/0| 97.7% ████████████████████████████████████████|
Compute/0     | 48.5% ███████████████████████                 |

Soham Purkait (5):
  lib/igt_device_scan: Add support for the device filter
  lib/igt_device_scan: Enable finding all matched IGT devices
  tools/gputop/utils: Add gputop utility functions common to all drivers
  tools/gputop/xe_gputop: Add gputop support for xe specific devices
  tools/gputop/gputop: Enable support for multiple GPUs and instances

 lib/igt_device_scan.c       | 128 ++++++++++++
 lib/igt_device_scan.h       |   1 +
 tools/{ => gputop}/gputop.c | 223 +++++++++++++++++----
 tools/gputop/meson.build    |   6 +
 tools/gputop/utils.c        |  51 +++++
 tools/gputop/utils.h        |  64 ++++++
 tools/gputop/xe_gputop.c    | 378 ++++++++++++++++++++++++++++++++++++
 tools/gputop/xe_gputop.h    |  62 ++++++
 tools/meson.build           |   6 +-
 9 files changed, 875 insertions(+), 44 deletions(-)
 rename tools/{ => gputop}/gputop.c (66%)
 create mode 100644 tools/gputop/meson.build
 create mode 100644 tools/gputop/utils.c
 create mode 100644 tools/gputop/utils.h
 create mode 100644 tools/gputop/xe_gputop.c
 create mode 100644 tools/gputop/xe_gputop.h

-- 
2.34.1

[PATCH i-g-t v11 1/5] lib/igt_device_scan: Add support for the device filter

[Soham Purkait]

Add support for the device filter based on
driver string, device type (integrated or discrete)
and card number.

v5 : Add device filter to filter out
     matching devices.           (Zbigniew)

v6 : Move device filter with Separate
     commit.                     (Zbigniew)

v7 : Fix interpretation of card numbering
     and add 'all' option for all the cards.
                                 (Zbigniew)

v8 : Fix for card filter output. (Zbigniew)

v10 : Fix render node issue with 'subsystem' filter
      option.                    (Zbigniew)

Signed-off-by: Soham Purkait <soham.purkait@intel.com>

Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
---
 lib/igt_device_scan.c | 80 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
index 3f26a1737..a250e7ee9 100644
--- a/lib/igt_device_scan.c
+++ b/lib/igt_device_scan.c
@@ -1434,6 +1434,7 @@ struct filter {
 		char *driver;
 		char *pf;
 		char *vf;
+		char *subsystem;
 	} data;
 };
 
@@ -1453,6 +1454,7 @@ static void fill_filter_data(struct filter *filter, const char *key, const char
 	__fill_key(driver);
 	__fill_key(pf);
 	__fill_key(vf);
+	__fill_key(subsystem);
 #undef __fill_key
 
 }
@@ -1709,6 +1711,77 @@ static struct igt_list_head *filter_sriov(const struct filter_class *fcls,
 	return &igt_devs.filtered;
 }
 
+/*
+ * Find appropriate gpu device through matching driver, device type and
+ * card filter arguments.
+ */
+static struct igt_list_head *filter_device(const struct filter_class *fcls,
+					   const struct filter *filter)
+{
+	struct igt_device *dev;
+	bool allcards = false;
+	int card = 0;
+	(void)fcls;
+
+	DBG("filter device\n");
+	if (filter->data.card) {
+		char crdop[5] = {0};
+
+		if (sscanf(filter->data.card, "%d", &card) == 1) {
+			if (card < 0)
+				return &igt_devs.filtered;
+		} else {
+			card = 0;
+			if (sscanf(filter->data.card, "%4s", crdop) == 1) {
+				if (!strcmp(crdop, "all"))
+					allcards = true;
+				else
+					return &igt_devs.filtered;
+			} else {
+				return &igt_devs.filtered;
+			}
+		}
+	} else {
+		card = 0;
+	}
+
+	igt_list_for_each_entry(dev, &igt_devs.all, link) {
+		/* Skip if 'driver' doesn't match */
+		if (filter->data.driver && !strequal(filter->data.driver, dev->driver))
+			continue;
+
+		/* Skip if 'device' doesn't match */
+		if (filter->data.device && !is_device_matched(dev, filter->data.device))
+			continue;
+
+		/* Skip if 'subsystem' doesn't match */
+		if (filter->data.subsystem && strcmp(filter->data.subsystem, "all")) {
+			if (strcmp(filter->data.subsystem, get_prop_subsystem(dev)))
+				continue;
+		}
+
+		/* We get n-th card */
+		if (!allcards && !card) {
+			struct igt_device *dup = duplicate_device(dev);
+
+			igt_list_add_tail(&dup->link, &igt_devs.filtered);
+			break;
+		} else if (!allcards) {
+			card--;
+		}
+		/* Include all the cards */
+		else if (allcards) {
+			struct igt_device *dup = duplicate_device(dev);
+
+			igt_list_add(&dup->link, &igt_devs.filtered);
+		}
+	}
+
+	DBG("Filter device filtered size: %d\n", igt_list_length(&igt_devs.filtered));
+
+	return &igt_devs.filtered;
+}
+
 static bool sys_path_valid(const struct filter_class *fcls,
 			   const struct filter *filter)
 {
@@ -1750,6 +1823,13 @@ static struct filter_class filter_definition_list[] = {
 		.help = "sriov:[vendor=%04x/name][,device=%04x][,card=%d][,pf=%d][,vf=%d]",
 		.detail = "find pf or vf\n",
 	},
+	{
+		.name = "device",
+		.filter_function = filter_device,
+		.help =
+		"device:[driver=name][,subsystem=all|<subsystem>][,device=type][,card=%d|all]",
+		.detail = "find device by driver name, subsystem, device type and card number\n",
+	},
 	{
 		.name = NULL,
 	},
-- 
2.34.1

[PATCH i-g-t v11 2/5] lib/igt_device_scan: Enable finding all matched IGT devices

[Soham Purkait]

Use filter to find all the available
GPUs or few among them by driver name
and card type or card number.

v2 : Fix for refactoring GPUTOP into a
     vendor-agnostic tool. (Lucas)

v3 : Separate commit for lib. (Kamil)

v4 : Refactor to use composition strategy
     for driver and device type filtering.
     Refactor code to improve memory
     allocation and error handling. (Lucas)

v5 : Introduce device card match function
     to return collection of matching
     devices using device filter.

v6 : Separate commit for device card match
     function.                     (Zbigniew)
     Function description modification for device
     card match function.          (Zbigniew)

v7 : Single return for card match function.
                                  (Krzysztof)

v8 : Removed 'drivers' array as card match
     function parameter.          (Zbigniew)

v9 : Fixed allocation in card match instead of
     multiple realloc.            (Zbigniew)

Signed-off-by: Soham Purkait <soham.purkait@intel.com>

Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com>
---
 lib/igt_device_scan.c | 48 +++++++++++++++++++++++++++++++++++++++++++
 lib/igt_device_scan.h |  1 +
 2 files changed, 49 insertions(+)

diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c
index a250e7ee9..f11ebf02e 100644
--- a/lib/igt_device_scan.c
+++ b/lib/igt_device_scan.c
@@ -2143,6 +2143,54 @@ bool igt_device_card_match_pci(const char *filter,
        return __igt_device_card_match(filter, card, true);
 }
 
+/**
+ * igt_device_card_match_all
+ * @filter: filter string.
+ * @card: double pointer to igt_device_card structure, containing
+ * an array of igt_device_card structures upon successful return.
+ *
+ * Function applies filter to match device from device array.
+ *
+ * Returns: the number of cards found.
+ *
+ * Note: The caller is responsible for freeing the memory which is
+ * dynamically allocated for the array of igt_device_card structures
+ * upon successful return.
+ */
+int igt_device_card_match_all(const char *filter, struct igt_device_card **card)
+{
+	struct igt_device *dev = NULL;
+	struct igt_device_card *crd = NULL;
+	int count = 0;
+
+	igt_devices_scan();
+
+	if (igt_device_filter_apply(filter) == false)
+		return 0;
+
+	if (igt_list_empty(&igt_devs.filtered))
+		return 0;
+
+	igt_list_for_each_entry(dev, &igt_devs.filtered, link) {
+		count++;
+	}
+
+	crd = calloc(count, sizeof(struct igt_device_card));
+	if (!crd)
+		return 0;
+
+	count = 0;
+
+	igt_list_for_each_entry(dev, &igt_devs.filtered, link) {
+		__copy_dev_to_card(dev, crd + count++);
+	}
+
+	if (count)
+		*card = crd;
+
+	return count;
+}
+
 /**
  * igt_device_get_pretty_name
  * @card: pointer to igt_device_card struct
diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h
index f1cd3b1e9..e6e31e799 100644
--- a/lib/igt_device_scan.h
+++ b/lib/igt_device_scan.h
@@ -89,6 +89,7 @@ int igt_device_filter_pci(void);
 bool igt_device_card_match(const char *filter, struct igt_device_card *card);
 bool igt_device_card_match_pci(const char *filter,
 	struct igt_device_card *card);
+int igt_device_card_match_all(const char *filter, struct igt_device_card **card);
 bool igt_device_find_first_i915_discrete_card(struct igt_device_card *card);
 bool igt_device_find_integrated_card(struct igt_device_card *card);
 bool igt_device_find_first_xe_discrete_card(struct igt_device_card *card);
-- 
2.34.1

[PATCH i-g-t v11 3/5] tools/gputop/utils: Add gputop utility functions common to all drivers

[Soham Purkait]

Implement utility functions in gputop for common
operations and data handling across different drivers.

v2 : Fix for refactoring GPUTOP into a
     vendor-agnostic tool. (Lucas)

v3 : Headers in alphabetical order. (Kamil, Riana)

v4 : Fix source file naming and remove driver
     specific codes. (Riana)

v7 : Fix per-client engine width value with
     a macro replacing magic number. (Krzysztof)

Signed-off-by: Soham Purkait <soham.purkait@intel.com>
Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com>
---
 tools/gputop/utils.c | 51 +++++++++++++++++++++++++++++++++++
 tools/gputop/utils.h | 64 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)
 create mode 100644 tools/gputop/utils.c
 create mode 100644 tools/gputop/utils.h

diff --git a/tools/gputop/utils.c b/tools/gputop/utils.c
new file mode 100644
index 000000000..7f260dc05
--- /dev/null
+++ b/tools/gputop/utils.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+#include <assert.h>
+
+#include "utils.h"
+
+static const char * const bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
+
+void n_spaces(const unsigned int n)
+{
+	unsigned int i;
+
+	for (i = 0; i < n; i++)
+		putchar(' ');
+}
+
+void print_percentage_bar(double percent, int max_len)
+{
+	int bar_len, i, len = max_len - 1;
+	const int w = PERCLIENT_ENGINE_WIDTH;
+
+	len -= printf("|%5.1f%% ", percent);
+
+	/* no space left for bars, do what we can */
+	if (len < 0)
+		len = 0;
+
+	bar_len = ceil(w * percent * len / 100.0);
+	if (bar_len > w * len)
+		bar_len = w * len;
+
+	for (i = bar_len; i >= w; i -= w)
+		printf("%s", bars[w]);
+	if (i)
+		printf("%s", bars[i]);
+
+	len -= (bar_len + (w - 1)) / w;
+	n_spaces(len);
+
+	putchar('|');
+}
+
+int print_engines_footer(int lines, int con_w, int con_h)
+{
+	if (lines++ < con_h)
+		printf("\n");
+
+	return lines;
+}
diff --git a/tools/gputop/utils.h b/tools/gputop/utils.h
new file mode 100644
index 000000000..3c62f1c47
--- /dev/null
+++ b/tools/gputop/utils.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+
+#ifndef COMMON_GPUTOP_H
+#define COMMON_GPUTOP_H
+
+#include <glib.h>
+#include <math.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "igt_device_scan.h"
+
+#define ANSI_HEADER "\033[7m"
+#define ANSI_RESET "\033[0m"
+
+#define PERCLIENT_ENGINE_WIDTH 8
+
+/**
+ * struct gputop_device
+ *
+ * @driver_present: It is set if at least a
+ * single device of the respective driver is
+ * found
+ * @len: Number of total device discovered
+ * of the respective driver
+ * @instances: pointer to the array of
+ * discovered instances of the devices
+ * of the same driver
+ */
+struct gputop_device {
+	bool driver_present;
+	int len;
+	void *instances;
+};
+
+/**
+ * struct device_operations - Structure to hold function
+ * pointers for device specific operations for each individual driver.
+ * @gputop_init: Function to initialize GPUTOP object
+ * @init_engines: Function to initialize engines for the respective driver.
+ * @pmu_init: Function to initialize the PMU (Performance Monitoring Unit).
+ * @pmu_sample: Function to sample PMU data.
+ * @print_engines: Function to print engine business.
+ * @clean_up: Function to release resources.
+ */
+struct device_operations {
+	void (*gputop_init)(void *ptr,
+			    struct igt_device_card *card);
+	void *(*init_engines)(const void *obj);
+	int (*pmu_init)(const void *obj);
+	void (*pmu_sample)(const void *obj);
+	int (*print_engines)(const void *obj, int lines, int w, int h);
+	void (*clean_up)(void *obj, int len);
+};
+
+void print_percentage_bar(double percent, int max_len);
+int print_engines_footer(int lines, int con_w, int con_h);
+void n_spaces(const unsigned int n);
+
+#endif  /* COMMON_GPUTOP_H */
-- 
2.34.1

[PATCH i-g-t v11 4/5] tools/gputop/xe_gputop: Add gputop support for xe specific devices

[Soham Purkait]

Add gputop support for xe-specific devices. Separate
driver-specific code into respective source files.

v2 : Fix for refactoring GPUTOP into a
     vendor-agnostic tool. (Lucas)

v3 : Separate commit. (Kamil)

v4 : Headers in alphabetical order
     Engines memory allocation at
     the beginning all at once.
     Removed PMU normalization. (Riana)

v5 : Refactor to eliminate redundant
     and unused code segments.
     Fix for proper resource cleanup. (Riana)

v8 : Allocated card structure memory inplace and
     accordingly modified the clean up code.

v11 : Loop optimization in xe_populate_engines.
      Removed short_name.
      PMU fds are closed on cleanup.
      Removed unnecessary comments. (Riana)

Signed-off-by: Soham Purkait <soham.purkait@intel.com>
---
 tools/gputop/xe_gputop.c | 378 +++++++++++++++++++++++++++++++++++++++
 tools/gputop/xe_gputop.h |  62 +++++++
 2 files changed, 440 insertions(+)
 create mode 100644 tools/gputop/xe_gputop.c
 create mode 100644 tools/gputop/xe_gputop.h

diff --git a/tools/gputop/xe_gputop.c b/tools/gputop/xe_gputop.c
new file mode 100644
index 000000000..ac3ed76e6
--- /dev/null
+++ b/tools/gputop/xe_gputop.c
@@ -0,0 +1,378 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+
+#include "xe_gputop.h"
+
+#define engine_ptr(engines, n) (&(engines)->engine + (n))
+
+static void __update_sample(struct xe_pmu_counter *counter, uint64_t val)
+{
+	counter->val.prev = counter->val.cur;
+	counter->val.cur = val;
+}
+
+static void update_sample(struct xe_pmu_counter *counter, uint64_t *val)
+{
+	if (counter->present)
+		__update_sample(counter, val[counter->idx]);
+}
+
+static const char *class_display_name(unsigned int class)
+{
+	switch (class) {
+	case DRM_XE_ENGINE_CLASS_RENDER:
+		return "Render/3D";
+	case DRM_XE_ENGINE_CLASS_COPY:
+		return "Blitter";
+	case DRM_XE_ENGINE_CLASS_VIDEO_DECODE:
+		return "Video";
+	case DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE:
+		return "VideoEnhance";
+	case DRM_XE_ENGINE_CLASS_COMPUTE:
+		return "Compute";
+	default:
+		return "[unknown]";
+	}
+}
+
+void xe_clean_up(void *obj, int len)
+{
+	struct xe_engine *eng;
+	struct xe_pmu_counter pmu;
+	struct xe_gputop *dev = (struct xe_gputop *)obj;	
+
+	for (int i = 0; i < len; i++) {
+		if ((dev + i)->card)
+			free((dev + i)->card);
+		if ((dev + i)->eng_obj) {
+
+			for(int j = 0; j < ((struct xe_pmu_device*)(dev + i)->eng_obj)->num_engines ; j++) {
+				eng = engine_ptr((struct xe_pmu_device*)(dev + i)->eng_obj, j);
+				if (eng->display_name)
+					free(eng->display_name);
+
+				pmu = eng->engine_active_ticks;
+				if (pmu.present)
+					close(pmu.fd);
+
+				pmu = eng->engine_total_ticks;
+				if (pmu.present)
+					close(pmu.fd);
+			}
+			free(dev->eng_obj);
+		}			
+		if ((dev + i)->pmu_device)
+			free(dev->pmu_device);
+	}
+}
+
+static char *pmu_name(struct igt_device_card *card)
+{
+	int card_fd;
+	char device[30];
+	char *path;
+
+	if (strlen(card->card))
+		card_fd = igt_open_card(card);
+	else if (strlen(card->render))
+		card_fd = igt_open_render(card);
+
+	if (card_fd == -1)
+		return NULL;
+
+	xe_perf_device(card_fd, device, sizeof(device));
+	path = strdup(device);
+	close(card_fd);
+	return path;
+}
+
+static int _open_pmu(uint64_t type, unsigned int *cnt, struct xe_pmu_counter *pmu, int *fd)
+{
+	int fd__ = igt_perf_open_group(type, pmu->config, *fd);
+
+	if (fd__ >= 0) {
+		if (*fd == -1)
+			*fd = fd__;
+		pmu->present = true;
+		pmu->idx = (*cnt)++;
+		pmu->fd = fd__;
+	}
+
+	return fd__;
+}
+
+void xe_gputop_init(void *ptr,
+		    struct igt_device_card *card)
+{
+	struct xe_gputop *obj = (struct xe_gputop *)ptr;
+
+	obj->pmu_device = pmu_name(card);
+	if (!obj->pmu_device) {
+		fprintf(stderr, "%s : pmu_device path returned NULL", card->pci_slot_name);
+		exit(EXIT_FAILURE);
+	}
+	obj->card = card;
+}
+
+static int pmu_format_shift(int xe, const char *name)
+{
+	uint32_t start;
+	int format;
+	char device[80];
+
+	format = perf_event_format(xe_perf_device(xe, device, sizeof(device)),
+				   name, &start);
+	if (format)
+		return 0;
+
+	return start;
+}
+
+static int engine_cmp(const void *__a, const void *__b)
+{
+	const struct xe_engine *a = (struct xe_engine *)__a;
+	const struct xe_engine *b = (struct xe_engine *)__b;
+
+	if (a->drm_xe_engine.engine_class != b->drm_xe_engine.engine_class)
+		return a->drm_xe_engine.engine_class - b->drm_xe_engine.engine_class;
+	else
+		return a->drm_xe_engine.engine_instance - b->drm_xe_engine.engine_instance;
+}
+
+void *xe_populate_engines(const void *obj)
+{
+	struct igt_device_card *card = ((struct xe_gputop *)obj)->card;
+	struct xe_pmu_device *engines;
+	int ret = 0;
+	char device[30];
+	struct drm_xe_engine_class_instance *hwe;
+	int card_fd;
+	uint64_t engine_class, engine_instance, gt_shift;
+	uint64_t engine_active_config, engine_total_config;
+
+	if (!card || !strlen(card->card) || !strlen(card->render))
+		return NULL;
+
+	if (strlen(card->card)) {
+		card_fd = igt_open_card(card);
+	} else if (strlen(card->render)) {
+		card_fd = igt_open_render(card);
+	} else {
+		fprintf(stderr, "Failed to detect device!\n");
+		return NULL;
+	}
+	xe_device_get(card_fd);
+	engines = malloc(sizeof(struct xe_pmu_device) +
+			 xe_number_engines(card_fd) * sizeof(struct xe_engine));
+	if (!engines)
+		return NULL;
+
+	memset(engines, 0, sizeof(struct xe_pmu_device) +
+	       xe_number_engines(card_fd) * sizeof(struct xe_engine));
+
+	engines->num_engines = 0;
+	engines->device = ((struct xe_gputop *)obj)->pmu_device;
+	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));
+	ret = perf_event_config(device,
+				"engine-active-ticks",
+				&engine_active_config);
+	if (ret < 0)
+		return NULL;
+	ret = perf_event_config(device,
+				"engine-total-ticks",
+				&engine_total_config);
+	if (ret < 0)
+		return NULL;
+	xe_for_each_engine(card_fd, hwe) {
+		uint64_t  param_config;
+		struct xe_engine *engine;
+
+		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;
+		engine->engine_active_ticks.config = engine_active_config | param_config;
+		engine->engine_total_ticks.config = engine_total_config | param_config;
+
+		if (engine->engine_active_ticks.config == -1 ||
+		    engine->engine_total_ticks.config == -1) {
+			ret = ENOENT;
+			break;
+		}
+
+		ret = asprintf(&engine->display_name, "%s/%u",
+			       class_display_name(engine->drm_xe_engine.engine_class),
+			       engine->drm_xe_engine.engine_instance);
+
+		if (ret <= 0) {
+			ret = errno;
+			break;
+		}
+
+		engines->num_engines++;
+	}
+
+	if (!ret) {
+		errno = ret;
+		return NULL;
+	}
+
+	qsort(engine_ptr(engines, 0), engines->num_engines,
+	      sizeof(struct xe_engine), engine_cmp);
+
+	((struct xe_gputop *)obj)->eng_obj = engines;
+
+	return engines;
+}
+
+static uint64_t pmu_read_multi(int fd, unsigned int num, uint64_t *val)
+{
+	uint64_t buf[2 + num];
+	unsigned int i;
+	ssize_t len;
+
+	memset(buf, 0, sizeof(buf));
+
+	len = read(fd, buf, sizeof(buf));
+	assert(len == sizeof(buf));
+
+	for (i = 0; i < num; i++)
+		val[i] = buf[2 + i];
+
+	return buf[1];
+}
+
+void xe_pmu_sample(const void *obj)
+{
+	struct xe_pmu_device *engines = ((struct xe_gputop *)obj)->eng_obj;
+	const int num_val = engines->num_counters;
+	uint64_t val[2 + num_val];
+	unsigned int i;
+
+	pmu_read_multi(engines->fd, num_val, val);
+
+	for (i = 0; i < engines->num_engines; i++) {
+		struct xe_engine *engine = engine_ptr(engines, i);
+
+		update_sample(&engine->engine_active_ticks, val);
+		update_sample(&engine->engine_total_ticks, val);
+	}
+}
+
+int xe_pmu_init(const void *obj)
+{
+	struct xe_pmu_device *engines = ((struct xe_gputop *)obj)->eng_obj;
+	unsigned int i;
+	int fd;
+	struct xe_engine *engine;
+	uint64_t type = igt_perf_type_id(engines->device);
+
+	engines->fd = -1;
+	engines->num_counters = 0;
+
+	for (i = 0; i < engines->num_engines; i++) {
+		engine = engine_ptr(engines, i);
+		fd = _open_pmu(type, &engines->num_counters, &engine->engine_active_ticks,
+			       &engines->fd);
+		if (fd < 0)
+			return -1;
+		fd = _open_pmu(type, &engines->num_counters, &engine->engine_total_ticks,
+			       &engines->fd);
+		if (fd < 0)
+			return -1;
+	}
+	return 0;
+}
+
+static double pmu_active_percentage(struct xe_engine *engine)
+{
+	double pmu_active_ticks = engine->engine_active_ticks.val.cur -
+				  engine->engine_active_ticks.val.prev;
+	double pmu_total_ticks = engine->engine_total_ticks.val.cur -
+				 engine->engine_total_ticks.val.prev;
+	double percentage;
+
+	percentage = (pmu_active_ticks * 100) / pmu_total_ticks;
+	return percentage;
+}
+
+static int
+print_device_description(const void *obj, int lines, int w, int h)
+{
+	char *desc;
+	int len;
+
+	len = asprintf(&desc, "DRIVER: %s || BDF: %s",
+		       ((struct xe_gputop *)obj)->card->driver,
+		       ((struct xe_gputop *)obj)->card->pci_slot_name);
+
+	printf("\033[7m%s%*s\033[0m\n",
+	       desc,
+	       (int)(w - len), " ");
+	lines++;
+	free(desc);
+	return lines;
+}
+
+static int
+print_engines_header(struct xe_pmu_device *engines,
+		     int lines, int con_w, int con_h)
+{
+	const char *a;
+
+	for (unsigned int i = 0;
+	     i < engines->num_engines && lines < con_h;
+	     i++) {
+		struct xe_engine *engine = engine_ptr(engines, i);
+
+		if (!engine->num_counters)
+			continue;
+
+		a = "            ENGINES   ACTIVITY  ";
+
+		printf("\033[7m%s%*s\033[0m\n",
+		       a,
+		       (int)(con_w - strlen(a)), " ");
+		lines++;
+
+		break;
+	}
+
+	return lines;
+}
+
+static int
+print_engine(struct xe_pmu_device *engines, unsigned int i,
+	     int lines, int con_w, int con_h)
+{
+	struct xe_engine *engine = engine_ptr(engines, i);
+	double percentage = pmu_active_percentage(engine);
+
+	printf("%*s", (int)(strlen("            ENGINES")), engine->display_name);
+	print_percentage_bar(percentage, con_w - strlen("            ENGINES"));
+	printf("\n");
+
+	return ++lines;
+}
+
+int xe_print_engines(const void *obj, int lines, int w, int h)
+{
+	struct xe_pmu_device *show = ((struct xe_gputop *)obj)->eng_obj;
+
+	lines = print_device_description(obj, lines, w, h);
+
+	lines = print_engines_header(show, lines, w,  h);
+
+	for (unsigned int i = 0; i < show->num_engines && lines < h; i++)
+		lines = print_engine(show, i, lines, w, h);
+
+	lines = print_engines_footer(lines, w, h);
+
+	return lines;
+}
+
diff --git a/tools/gputop/xe_gputop.h b/tools/gputop/xe_gputop.h
new file mode 100644
index 000000000..825ac7e34
--- /dev/null
+++ b/tools/gputop/xe_gputop.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2025 Intel Corporation
+ */
+
+#ifndef __XE_GPUTOP_H__
+#define __XE_GPUTOP_H__
+
+#include <dirent.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;
+	uint64_t prev;
+};
+
+struct xe_pmu_counter {
+	uint64_t type;	
+	uint64_t config;
+	unsigned int idx;
+	struct xe_pmu_pair val;
+	int fd;
+	bool present;
+};
+
+struct xe_engine {
+	const char *name;
+	char *display_name;
+	struct drm_xe_engine_class_instance drm_xe_engine;
+	unsigned int num_counters;
+	struct xe_pmu_counter engine_active_ticks;
+	struct xe_pmu_counter engine_total_ticks;
+};
+
+struct xe_pmu_device {
+	unsigned int num_engines;
+	unsigned int num_counters;
+	int fd;
+	char *device;
+	struct xe_engine engine;
+};
+
+struct xe_gputop {
+	char *pmu_device;
+	struct igt_device_card *card;
+	struct xe_pmu_device *eng_obj;
+};
+
+void xe_gputop_init(void *ptr,
+		    struct igt_device_card *card);
+void xe_populate_device_instances(struct gputop_device *dv);
+void *xe_populate_engines(const void *obj);
+void xe_pmu_sample(const void *obj);
+int xe_pmu_init(const void *obj);
+int xe_print_engines(const void *obj, int lines, int w, int h);
+void xe_clean_up(void *obj, int len);
+
+#endif /* __XE_GPUTOP_H__ */
-- 
2.34.1

[PATCH i-g-t v11 5/5] tools/gputop/gputop: Enable support for multiple GPUs and instances

[Soham Purkait]

Introduce vendor-agnostic support for
handling multiple GPUs and instances
in gputop. Improve the tool's adaptability
to various GPU configurations.

v2 : Fix for refactoring GPUTOP into a
     vendor-agnostic tool. (Lucas)

v3 : New year included in copyright. (Kamil, Riana)
     Removed caps in function name. (Riana)
     Struct for driver specific operations. (Riana)
     Headers in alphabetical order. (Kamil, Riana)

v4 : Commit description and signed-off included.

v5 : Fix for proper resource cleanup. (Riana)
     Use "dev_type" enum for card_type. (Krzysztof)
     Add new filter to return collection
     of matching devices.               (Zbigniew)

v6 : Use device filter to populate the array of
     cards for all supported drivers.   (Zbigniew)

v7 : Use filter to find all the cards.  (Zbigniew)

v8 : Removed 'drivers' array parameter from
     card match function.               (Zbigniew)

v10 : Resolved 'populate_devices' call with
      pci subsystem filtering.          (Zbigniew)

v11 : Add space after /* and before */ for better
      readability.                      (Zbigniew)
      Comments wrapped at 75/100.       (Riana)

Signed-off-by: Soham Purkait <soham.purkait@intel.com>

Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
---
 tools/{ => gputop}/gputop.c | 223 +++++++++++++++++++++++++++++-------
 tools/gputop/meson.build    |   6 +
 tools/meson.build           |   6 +-
 3 files changed, 191 insertions(+), 44 deletions(-)
 rename tools/{ => gputop}/gputop.c (66%)
 create mode 100644 tools/gputop/meson.build

diff --git a/tools/gputop.c b/tools/gputop/gputop.c
similarity index 66%
rename from tools/gputop.c
rename to tools/gputop/gputop.c
index 43b01f566..fc3ccb83b 100644
--- a/tools/gputop.c
+++ b/tools/gputop/gputop.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: MIT
 /*
- * Copyright © 2023 Intel Corporation
+ * Copyright © 2023-2025 Intel Corporation
  */
 
 #include <assert.h>
@@ -14,66 +14,145 @@
 #include <math.h>
 #include <poll.h>
 #include <signal.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/sysmacros.h>
 #include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/types.h>
-#include <unistd.h>
 #include <termios.h>
-#include <sys/sysmacros.h>
-#include <stdbool.h>
+#include <unistd.h>
 
+#include "drmtest.h"
 #include "igt_core.h"
 #include "igt_drm_clients.h"
 #include "igt_drm_fdinfo.h"
+#include "igt_perf.h"
 #include "igt_profiling.h"
-#include "drmtest.h"
+#include "xe_gputop.h"
+#include "xe/xe_query.h"
+
+/**
+ * Supported Drivers
+ *
+ * Adhere to the following requirements when implementing support for the
+ * new driver:
+ * @drivers: Update drivers[] with driver string.
+ * @total_count: Update NUM_DRIVER with the total number of supported drivers.
+ * @operations: Update the respective operations of the new driver:
+ * gputop_init,
+ * discover_engines,
+ * pmu_init,
+ * pmu_sample,
+ * print_engines,
+ * clean_up
+ * @devices: Update devices[] array of type "struct gputop_device" with the
+ * initial values.
+ */
+static const char * const drivers[] = {
+	"xe",
+    /* Keep the last one as NULL */
+	NULL
+};
+
+/**
+ * Number of supported drivers needs to be adjusted as per the length of
+ * the drivers[] array.
+ */
+#define NUM_DRIVER 1
+
+/**
+ * Supported operations on driver instances. Update the oprs[] array for
+ * each individual driver specific function. Maintain the sequence as per
+ * drivers[] array.
+ */
+struct device_operations oprs[NUM_DRIVER] = {
+	{
+		xe_gputop_init,
+		xe_populate_engines,
+		xe_pmu_init,
+		xe_pmu_sample,
+		xe_print_engines,
+		xe_clean_up
+	}
+};
+
+/*
+ * devices[] array of type struct gputop_device
+ */
+struct gputop_device devices[] = {
+	{false, 0, NULL}
+};
 
 enum utilization_type {
 	UTILIZATION_TYPE_ENGINE_TIME,
 	UTILIZATION_TYPE_TOTAL_CYCLES,
 };
 
-static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
-
-#define ANSI_HEADER "\033[7m"
-#define ANSI_RESET "\033[0m"
-
-static void n_spaces(const unsigned int n)
+static void gputop_clean_up(void)
 {
-	unsigned int i;
-
-	for (i = 0; i < n; i++)
-		putchar(' ');
+	for (int i = 0; drivers[i]; i++) {
+		oprs[i].clean_up(devices[i].instances, devices[i].len);
+		free(devices[i].instances);
+		devices[i].driver_present = false;
+		devices[i].len = 0;
+	}
 }
 
-static void print_percentage_bar(double percent, int max_len)
+static int find_driver(struct igt_device_card *card)
 {
-	int bar_len, i, len = max_len - 1;
-	const int w = 8;
-
-	len -= printf("|%5.1f%% ", percent);
-
-	/* no space left for bars, do what we can */
-	if (len < 0)
-		len = 0;
-
-	bar_len = ceil(w * percent * len / 100.0);
-	if (bar_len > w * len)
-		bar_len = w * len;
+	for (int i = 0; drivers[i]; i++) {
+		if (strcmp(drivers[i], card->driver) == 0)
+			return i;
+	}
+	return -1;
+}
 
-	for (i = bar_len; i >= w; i -= w)
-		printf("%s", bars[w]);
-	if (i)
-		printf("%s", bars[i]);
+/*
+ * If filter is not NULL i will be ignored.
+ */
+static int populate_device_instances(const char *filter)
+{
+	struct igt_device_card *cards = NULL;
+	struct igt_device_card *card_inplace = NULL;
+	struct gputop_device *dev =  NULL;
+	int driver_no;
+	int count, final_count = 0;
+
+	count = igt_device_card_match_all(filter, &cards);
+	for (int j = 0; j < count; j++) {
+		if (strcmp((cards + j)->subsystem, "pci") != 0)
+			continue;
 
-	len -= (bar_len + (w - 1)) / w;
-	n_spaces(len);
+		driver_no = find_driver(cards + j);
+		if (driver_no < 0)
+			continue;
 
-	putchar('|');
+		dev = devices + driver_no;
+		if (!dev->driver_present)
+			dev->driver_present = true;
+		dev->len++;
+		dev->instances = realloc(dev->instances,
+					 dev->len * sizeof(struct xe_gputop));
+		if (!dev->instances) {
+			fprintf(stderr,
+				"Device instance realloc failed (%s)\n",
+				strerror(errno));
+			exit(EXIT_FAILURE);
+		}
+		card_inplace = (struct igt_device_card *)
+				calloc(1, sizeof(struct igt_device_card));
+		memcpy(card_inplace, cards + j, sizeof(struct igt_device_card));
+		oprs[driver_no].gputop_init((struct xe_gputop *)(dev->instances + dev->len - 1),
+			card_inplace);
+		final_count++;
+	}
+	if (count)
+		free(cards);
+	return final_count;
 }
 
 static int
@@ -333,6 +412,7 @@ static void clrscr(void)
 struct gputop_args {
 	long n_iter;
 	unsigned long delay_usec;
+	char *device;
 };
 
 static void help(void)
@@ -343,16 +423,18 @@ static void help(void)
 	       "\t-h, --help                show this help\n"
 	       "\t-d, --delay =SEC[.TENTHS] iterative delay as SECS [.TENTHS]\n"
 	       "\t-n, --iterations =NUMBER  number of executions\n"
+	       "\t-D, --device              Device filter\n"
 	       , program_invocation_short_name);
 }
 
 static int parse_args(int argc, char * const argv[], struct gputop_args *args)
 {
-	static const char cmdopts_s[] = "hn:d:";
+	static const char cmdopts_s[] = "hn:d:D:";
 	static const struct option cmdopts[] = {
 	       {"help", no_argument, 0, 'h'},
 	       {"delay", required_argument, 0, 'd'},
 	       {"iterations", required_argument, 0, 'n'},
+	       {"device", required_argument, 0, 'D'},
 	       { }
 	};
 
@@ -360,6 +442,7 @@ static int parse_args(int argc, char * const argv[], struct gputop_args *args)
 	memset(args, 0, sizeof(*args));
 	args->n_iter = -1;
 	args->delay_usec = 2 * USEC_PER_SEC;
+	args->device = NULL;
 
 	for (;;) {
 		int c, idx = 0;
@@ -383,6 +466,9 @@ static int parse_args(int argc, char * const argv[], struct gputop_args *args)
 				return -1;
 			}
 			break;
+		case 'D':
+			args->device = optarg;
+			break;
 		case 'h':
 			help();
 			return 0;
@@ -422,6 +508,52 @@ int main(int argc, char **argv)
 	n = args.n_iter;
 	period_us = args.delay_usec;
 
+	if (!populate_device_instances(args.device ? args.device
+				       : "device:subsystem=pci,card=all")) {
+		printf("No device found.\n");
+		gputop_clean_up();
+		exit(1);
+	}
+
+	for (int i = 0; drivers[i]; i++) {
+		if (devices[i].driver_present) {
+			for (int j = 0; j < devices[i].len; j++) {
+				if (!oprs[i].init_engines(devices[i].instances + j)) {
+					fprintf(stderr,
+						"Failed to initialize engines! (%s)\n",
+						strerror(errno));
+						gputop_clean_up();
+					return EXIT_FAILURE;
+				}
+				ret = oprs[i].pmu_init(devices[i].instances + j);
+
+				if (ret) {
+					fprintf(stderr,
+						"Failed to initialize PMU! (%s)\n",
+						strerror(errno));
+					if (errno == EACCES && geteuid())
+						fprintf(stderr,
+							"\n"
+							"When running as a normal user CAP_PERFMON is required to access performance\n"
+							"monitoring. See \"man 7 capabilities\", \"man 8 setcap\", or contact your\n"
+							"distribution vendor for assistance.\n"
+							"\n"
+							"More information can be found at 'Perf events and tool security' document:\n"
+							"https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
+
+					igt_devices_free();
+					gputop_clean_up();
+					return EXIT_FAILURE;
+				}
+			}
+		}
+	}
+
+	for (int i = 0; drivers[i]; i++) {
+		for (int j = 0; devices[i].driver_present && j < devices[i].len; j++)
+			oprs[i].pmu_sample(devices[i].instances + j);
+	}
+
 	clients = igt_drm_clients_init(NULL);
 	if (!clients)
 		exit(1);
@@ -442,14 +574,27 @@ int main(int argc, char **argv)
 
 	while ((n != 0) && !stop_top) {
 		struct igt_drm_client *c, *prevc = NULL;
-		int i, engine_w = 0, lines = 0;
+		int k, engine_w = 0, lines = 0;
 
 		igt_drm_clients_scan(clients, NULL, NULL, 0, NULL, 0);
+
+		for (int i = 0; drivers[i]; i++) {
+			for (int j = 0; devices[i].driver_present && j < devices[i].len; j++)
+				oprs[i].pmu_sample(devices[i].instances + j);
+		}
+
 		igt_drm_clients_sort(clients, client_cmp);
 
 		update_console_size(&con_w, &con_h);
 		clrscr();
 
+		for (int i = 0; drivers[i]; i++) {
+			for (int j = 0; devices[i].driver_present && j < devices[i].len; j++) {
+				lines = oprs[i].print_engines(devices[i].instances + j,
+							 lines, con_w, con_h);
+			}
+		}
+
 		if (!clients->num_clients) {
 			const char *msg = " (No GPU clients yet. Start workload to see stats)";
 
@@ -457,7 +602,7 @@ int main(int argc, char **argv)
 			       (int)(con_w - strlen(msg) - 1), msg);
 		}
 
-		igt_for_each_drm_client(clients, c, i) {
+		igt_for_each_drm_client(clients, c, k) {
 			assert(c->status != IGT_DRM_CLIENT_PROBE);
 			if (c->status != IGT_DRM_CLIENT_ALIVE)
 				break; /* Active clients are first in the array. */
@@ -481,11 +626,11 @@ int main(int argc, char **argv)
 	}
 
 	igt_drm_clients_free(clients);
+	gputop_clean_up();
 
 	if (profiled_devices != NULL) {
 		igt_devices_configure_profiling(profiled_devices, false);
 		igt_devices_free_profiling(profiled_devices);
 	}
-
 	return 0;
 }
diff --git a/tools/gputop/meson.build b/tools/gputop/meson.build
new file mode 100644
index 000000000..4766d8496
--- /dev/null
+++ b/tools/gputop/meson.build
@@ -0,0 +1,6 @@
+gputop_src = [ 'gputop.c', 'utils.c', 'xe_gputop.c']
+executable('gputop', sources : gputop_src,
+           install : true,
+           install_rpath : bindir_rpathdir,
+           dependencies : [igt_deps,lib_igt_perf,lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math],
+	   install: true)
diff --git a/tools/meson.build b/tools/meson.build
index de866c392..8002f707d 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -69,11 +69,6 @@ if libudev.found()
 		   install : true)
 endif
 
-executable('gputop', 'gputop.c',
-           install : true,
-           install_rpath : bindir_rpathdir,
-           dependencies : [lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math])
-
 intel_l3_parity_src = [ 'intel_l3_parity.c', 'intel_l3_udev_listener.c' ]
 executable('intel_l3_parity', sources : intel_l3_parity_src,
 	   dependencies : tool_deps,
@@ -122,3 +117,4 @@ endif
 subdir('i915-perf')
 subdir('xe-perf')
 subdir('null_state_gen')
+subdir('gputop')
-- 
2.34.1

✗ Fi.CI.BUILD: failure for Add per-device engine activity stats in GPUTOP (rev7)

[Patchwork]

== Series Details ==

Series: Add per-device engine activity stats in GPUTOP (rev7)
URL   : https://patchwork.freedesktop.org/series/146756/
State : failure

== Summary ==

Applying: lib/igt_device_scan: Add support for the device filter
Applying: lib/igt_device_scan: Enable finding all matched IGT devices
Applying: tools/gputop/utils: Add gputop utility functions common to all drivers
Applying: tools/gputop/xe_gputop: Add gputop support for xe specific devices
Applying: tools/gputop/gputop: Enable support for multiple GPUs and instances
Using index info to reconstruct a base tree...
M	tools/gputop.c
Falling back to patching base and 3-way merge...
Auto-merging tools/gputop/gputop.c
CONFLICT (content): Merge conflict in tools/gputop/gputop.c
Patch failed at 0005 tools/gputop/gputop: Enable support for multiple GPUs and instances
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [PATCH i-g-t v11 5/5] tools/gputop/gputop: Enable support for multiple GPUs and instances

[Riana Tauro]

Hi Soham

Code looks good. Have one question and few minor comments below

On 5/22/2025 9:14 PM, Soham Purkait wrote:
> Introduce vendor-agnostic support for
> handling multiple GPUs and instances
> in gputop. Improve the tool's adaptability
> to various GPU configurations.
> 
> v2 : Fix for refactoring GPUTOP into a
>       vendor-agnostic tool. (Lucas)
> 
> v3 : New year included in copyright. (Kamil, Riana)
>       Removed caps in function name. (Riana)
>       Struct for driver specific operations. (Riana)
>       Headers in alphabetical order. (Kamil, Riana)
> 
> v4 : Commit description and signed-off included.
> 
> v5 : Fix for proper resource cleanup. (Riana)
>       Use "dev_type" enum for card_type. (Krzysztof)
>       Add new filter to return collection
>       of matching devices.               (Zbigniew)
> 
> v6 : Use device filter to populate the array of
>       cards for all supported drivers.   (Zbigniew)
> 
> v7 : Use filter to find all the cards.  (Zbigniew)
> 
> v8 : Removed 'drivers' array parameter from
>       card match function.               (Zbigniew)
> 
> v10 : Resolved 'populate_devices' call with
>        pci subsystem filtering.          (Zbigniew)
> 
> v11 : Add space after /* and before */ for better
>        readability.                      (Zbigniew)
>        Comments wrapped at 75/100.       (Riana)
> 
> Signed-off-by: Soham Purkait <soham.purkait@intel.com>
> 
> Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> ---
>   tools/{ => gputop}/gputop.c | 223 +++++++++++++++++++++++++++++-------
>   tools/gputop/meson.build    |   6 +
>   tools/meson.build           |   6 +-
>   3 files changed, 191 insertions(+), 44 deletions(-)
>   rename tools/{ => gputop}/gputop.c (66%)
>   create mode 100644 tools/gputop/meson.build
> 
> diff --git a/tools/gputop.c b/tools/gputop/gputop.c
> similarity index 66%
> rename from tools/gputop.c
> rename to tools/gputop/gputop.c
> index 43b01f566..fc3ccb83b 100644
> --- a/tools/gputop.c
> +++ b/tools/gputop/gputop.c
> @@ -1,6 +1,6 @@
>   // SPDX-License-Identifier: MIT
>   /*
> - * Copyright © 2023 Intel Corporation
> + * Copyright © 2023-2025 Intel Corporation
>    */
>   
>   #include <assert.h>
> @@ -14,66 +14,145 @@
>   #include <math.h>
>   #include <poll.h>
>   #include <signal.h>
> +#include <stdbool.h>
>   #include <stdint.h>
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <string.h>
> +#include <sys/sysmacros.h>

alphabetical

>   #include <sys/ioctl.h>
>   #include <sys/stat.h>
>   #include <sys/types.h>
> -#include <unistd.h>
>   #include <termios.h>
> -#include <sys/sysmacros.h>
> -#include <stdbool.h>
> +#include <unistd.h>
>   
> +#include "drmtest.h"
>   #include "igt_core.h"
>   #include "igt_drm_clients.h"
>   #include "igt_drm_fdinfo.h"
> +#include "igt_perf.h"
>   #include "igt_profiling.h"
> -#include "drmtest.h"
> +#include "xe_gputop.h"
> +#include "xe/xe_query.h"
> +
> +/**
> + * Supported Drivers
> + *
> + * Adhere to the following requirements when implementing support for the
> + * new driver:
> + * @drivers: Update drivers[] with driver string.
> + * @total_count: Update NUM_DRIVER with the total number of supported drivers.
> + * @operations: Update the respective operations of the new driver:
> + * gputop_init,
> + * discover_engines,
> + * pmu_init,
> + * pmu_sample,
> + * print_engines,
> + * clean_up
> + * @devices: Update devices[] array of type "struct gputop_device" with the
> + * initial values.
> + */
> +static const char * const drivers[] = {
> +	"xe",
> +    /* Keep the last one as NULL */
> +	NULL
> +};
> +
> +/**
> + * Number of supported drivers needs to be adjusted as per the length of
> + * the drivers[] array.
> + */
> +#define NUM_DRIVER 1
> +
> +/**
> + * Supported operations on driver instances. Update the oprs[] array for
> + * each individual driver specific function. Maintain the sequence as per
> + * drivers[] array.
> + */
> +struct device_operations oprs[NUM_DRIVER] = {

Replace oprs with ops.. generally use ops for operations
> +	{
> +		xe_gputop_init,
> +		xe_populate_engines,
> +		xe_pmu_init,
> +		xe_pmu_sample,
> +		xe_print_engines,
> +		xe_clean_up
> +	}
> +};
> +
> +/*
> + * devices[] array of type struct gputop_device
> + */
> +struct gputop_device devices[] = {

I must have commented here before. As i see this is one entry per driver 
right? why is it named device?

> +	{false, 0, NULL}
> +};
>   
>   enum utilization_type {
>   	UTILIZATION_TYPE_ENGINE_TIME,
>   	UTILIZATION_TYPE_TOTAL_CYCLES,
>   };
>   
> -static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
> -
> -#define ANSI_HEADER "\033[7m"
> -#define ANSI_RESET "\033[0m"
> -
> -static void n_spaces(const unsigned int n)
> +static void gputop_clean_up(void)
>   {
> -	unsigned int i;
> -
> -	for (i = 0; i < n; i++)
> -		putchar(' ');
> +	for (int i = 0; drivers[i]; i++) {
> +		oprs[i].clean_up(devices[i].instances, devices[i].len);
> +		free(devices[i].instances);
> +		devices[i].driver_present = false;
> +		devices[i].len = 0;
> +	}
>   }
>   
> -static void print_percentage_bar(double percent, int max_len)
> +static int find_driver(struct igt_device_card *card)
>   {
> -	int bar_len, i, len = max_len - 1;
> -	const int w = 8;
> -
> -	len -= printf("|%5.1f%% ", percent);
> -
> -	/* no space left for bars, do what we can */
> -	if (len < 0)
> -		len = 0;
> -
> -	bar_len = ceil(w * percent * len / 100.0);
> -	if (bar_len > w * len)
> -		bar_len = w * len;
> +	for (int i = 0; drivers[i]; i++) {
> +		if (strcmp(drivers[i], card->driver) == 0)
> +			return i;
> +	}
> +	return -1;
> +}
>   
> -	for (i = bar_len; i >= w; i -= w)
> -		printf("%s", bars[w]);
> -	if (i)
> -		printf("%s", bars[i]);
> +/*
> + * If filter is not NULL i will be ignored.
> + */
> +static int populate_device_instances(const char *filter)
> +{
> +	struct igt_device_card *cards = NULL;
> +	struct igt_device_card *card_inplace = NULL;
> +	struct gputop_device *dev =  NULL;
> +	int driver_no;
> +	int count, final_count = 0;

%s/final_count/card_count

> +
> +	count = igt_device_card_match_all(filter, &cards);
> +	for (int j = 0; j < count; j++) {
> +		if (strcmp((cards + j)->subsystem, "pci") != 0)
> +			continue;
>   
> -	len -= (bar_len + (w - 1)) / w;
> -	n_spaces(len);
> +		driver_no = find_driver(cards + j);
> +		if (driver_no < 0)
> +			continue;
>   
> -	putchar('|');
> +		dev = devices + driver_no;
> +		if (!dev->driver_present)
> +			dev->driver_present = true;
> +		dev->len++;
> +		dev->instances = realloc(dev->instances,
> +					 dev->len * sizeof(struct xe_gputop));
> +		if (!dev->instances) {
> +			fprintf(stderr,
> +				"Device instance realloc failed (%s)\n",
> +				strerror(errno));
> +			exit(EXIT_FAILURE);
> +		}
> +		card_inplace = (struct igt_device_card *)
> +				calloc(1, sizeof(struct igt_device_card));
> +		memcpy(card_inplace, cards + j, sizeof(struct igt_device_card));
> +		oprs[driver_no].gputop_init((struct xe_gputop *)(dev->instances + dev->len - 1),
> +			card_inplace);
> +		final_count++;
> +	}
> +	if (count)
> +		free(cards);
> +	return final_count;
>   }
>   
>   static int
> @@ -333,6 +412,7 @@ static void clrscr(void)
>   struct gputop_args {
>   	long n_iter;
>   	unsigned long delay_usec;
> +	char *device;
>   };
>   
>   static void help(void)
> @@ -343,16 +423,18 @@ static void help(void)
>   	       "\t-h, --help                show this help\n"
>   	       "\t-d, --delay =SEC[.TENTHS] iterative delay as SECS [.TENTHS]\n"
>   	       "\t-n, --iterations =NUMBER  number of executions\n"
> +	       "\t-D, --device              Device filter\n"
>   	       , program_invocation_short_name);
>   }
>   
>   static int parse_args(int argc, char * const argv[], struct gputop_args *args)
>   {
> -	static const char cmdopts_s[] = "hn:d:";
> +	static const char cmdopts_s[] = "hn:d:D:";
>   	static const struct option cmdopts[] = {
>   	       {"help", no_argument, 0, 'h'},
>   	       {"delay", required_argument, 0, 'd'},
>   	       {"iterations", required_argument, 0, 'n'},
> +	       {"device", required_argument, 0, 'D'},
>   	       { }
>   	};
>   
> @@ -360,6 +442,7 @@ static int parse_args(int argc, char * const argv[], struct gputop_args *args)
>   	memset(args, 0, sizeof(*args));
>   	args->n_iter = -1;
>   	args->delay_usec = 2 * USEC_PER_SEC;
> +	args->device = NULL;
>   
>   	for (;;) {
>   		int c, idx = 0;
> @@ -383,6 +466,9 @@ static int parse_args(int argc, char * const argv[], struct gputop_args *args)
>   				return -1;
>   			}
>   			break;
> +		case 'D':
> +			args->device = optarg;
> +			break;
>   		case 'h':
>   			help();
>   			return 0;
> @@ -422,6 +508,52 @@ int main(int argc, char **argv)
>   	n = args.n_iter;
>   	period_us = args.delay_usec;
>   
> +	if (!populate_device_instances(args.device ? args.device
> +				       : "device:subsystem=pci,card=all")) {
> +		printf("No device found.\n");
> +		gputop_clean_up();
> +		exit(1);
> +	}
> +
> +	for (int i = 0; drivers[i]; i++) {
> +		if (devices[i].driver_present) {

if not present continue

Thanks
Riana

> +			for (int j = 0; j < devices[i].len; j++) {
> +				if (!oprs[i].init_engines(devices[i].instances + j)) {
> +					fprintf(stderr,
> +						"Failed to initialize engines! (%s)\n",
> +						strerror(errno));
> +						gputop_clean_up();
> +					return EXIT_FAILURE;
> +				}
> +				ret = oprs[i].pmu_init(devices[i].instances + j);
> +
> +				if (ret) {
> +					fprintf(stderr,
> +						"Failed to initialize PMU! (%s)\n",
> +						strerror(errno));
> +					if (errno == EACCES && geteuid())
> +						fprintf(stderr,
> +							"\n"
> +							"When running as a normal user CAP_PERFMON is required to access performance\n"
> +							"monitoring. See \"man 7 capabilities\", \"man 8 setcap\", or contact your\n"
> +							"distribution vendor for assistance.\n"
> +							"\n"
> +							"More information can be found at 'Perf events and tool security' document:\n"
> +							"https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
> +
> +					igt_devices_free();
> +					gputop_clean_up();
> +					return EXIT_FAILURE;
> +				}
> +			}
> +		}
> +	}
> +
> +	for (int i = 0; drivers[i]; i++) {
> +		for (int j = 0; devices[i].driver_present && j < devices[i].len; j++)
> +			oprs[i].pmu_sample(devices[i].instances + j);
> +	}
> +
>   	clients = igt_drm_clients_init(NULL);
>   	if (!clients)
>   		exit(1);
> @@ -442,14 +574,27 @@ int main(int argc, char **argv)
>   
>   	while ((n != 0) && !stop_top) {
>   		struct igt_drm_client *c, *prevc = NULL;
> -		int i, engine_w = 0, lines = 0;
> +		int k, engine_w = 0, lines = 0;
>   
>   		igt_drm_clients_scan(clients, NULL, NULL, 0, NULL, 0);
> +
> +		for (int i = 0; drivers[i]; i++) {
> +			for (int j = 0; devices[i].driver_present && j < devices[i].len; j++)
> +				oprs[i].pmu_sample(devices[i].instances + j);
> +		}
> +
>   		igt_drm_clients_sort(clients, client_cmp);
>   
>   		update_console_size(&con_w, &con_h);
>   		clrscr();
>   
> +		for (int i = 0; drivers[i]; i++) {
> +			for (int j = 0; devices[i].driver_present && j < devices[i].len; j++) {
> +				lines = oprs[i].print_engines(devices[i].instances + j,
> +							 lines, con_w, con_h);
> +			}
> +		}
> +
>   		if (!clients->num_clients) {
>   			const char *msg = " (No GPU clients yet. Start workload to see stats)";
>   
> @@ -457,7 +602,7 @@ int main(int argc, char **argv)
>   			       (int)(con_w - strlen(msg) - 1), msg);
>   		}
>   
> -		igt_for_each_drm_client(clients, c, i) {
> +		igt_for_each_drm_client(clients, c, k) {
>   			assert(c->status != IGT_DRM_CLIENT_PROBE);
>   			if (c->status != IGT_DRM_CLIENT_ALIVE)
>   				break; /* Active clients are first in the array. */
> @@ -481,11 +626,11 @@ int main(int argc, char **argv)
>   	}
>   
>   	igt_drm_clients_free(clients);
> +	gputop_clean_up();
>   
>   	if (profiled_devices != NULL) {
>   		igt_devices_configure_profiling(profiled_devices, false);
>   		igt_devices_free_profiling(profiled_devices);
>   	}
> -
>   	return 0;
>   }
> diff --git a/tools/gputop/meson.build b/tools/gputop/meson.build
> new file mode 100644
> index 000000000..4766d8496
> --- /dev/null
> +++ b/tools/gputop/meson.build
> @@ -0,0 +1,6 @@
> +gputop_src = [ 'gputop.c', 'utils.c', 'xe_gputop.c']
> +executable('gputop', sources : gputop_src,
> +           install : true,
> +           install_rpath : bindir_rpathdir,
> +           dependencies : [igt_deps,lib_igt_perf,lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math],
> +	   install: true)
> diff --git a/tools/meson.build b/tools/meson.build
> index de866c392..8002f707d 100644
> --- a/tools/meson.build
> +++ b/tools/meson.build
> @@ -69,11 +69,6 @@ if libudev.found()
>   		   install : true)
>   endif
>   
> -executable('gputop', 'gputop.c',
> -           install : true,
> -           install_rpath : bindir_rpathdir,
> -           dependencies : [lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math])
> -
>   intel_l3_parity_src = [ 'intel_l3_parity.c', 'intel_l3_udev_listener.c' ]
>   executable('intel_l3_parity', sources : intel_l3_parity_src,
>   	   dependencies : tool_deps,
> @@ -122,3 +117,4 @@ endif
>   subdir('i915-perf')
>   subdir('xe-perf')
>   subdir('null_state_gen')
> +subdir('gputop')

Re: [PATCH i-g-t v11 4/5] tools/gputop/xe_gputop: Add gputop support for xe specific devices

[Riana Tauro]

Hi Soham

Some minor comments

On 5/22/2025 9:14 PM, Soham Purkait wrote:
> Add gputop support for xe-specific devices. Separate
> driver-specific code into respective source files.
> 
> v2 : Fix for refactoring GPUTOP into a
>       vendor-agnostic tool. (Lucas)
> 
> v3 : Separate commit. (Kamil)
> 
> v4 : Headers in alphabetical order
>       Engines memory allocation at
>       the beginning all at once.
>       Removed PMU normalization. (Riana)
> 
> v5 : Refactor to eliminate redundant
>       and unused code segments.
>       Fix for proper resource cleanup. (Riana)
> 
> v8 : Allocated card structure memory inplace and
>       accordingly modified the clean up code.
> 
> v11 : Loop optimization in xe_populate_engines.
>        Removed short_name.
>        PMU fds are closed on cleanup.
>        Removed unnecessary comments. (Riana)

lot of versions maybe squash all the comments into one and start new 
series. But upto you

> 
> Signed-off-by: Soham Purkait <soham.purkait@intel.com>
> ---
>   tools/gputop/xe_gputop.c | 378 +++++++++++++++++++++++++++++++++++++++
>   tools/gputop/xe_gputop.h |  62 +++++++
>   2 files changed, 440 insertions(+)
>   create mode 100644 tools/gputop/xe_gputop.c
>   create mode 100644 tools/gputop/xe_gputop.h
> 
> diff --git a/tools/gputop/xe_gputop.c b/tools/gputop/xe_gputop.c
> new file mode 100644
> index 000000000..ac3ed76e6
> --- /dev/null
> +++ b/tools/gputop/xe_gputop.c
> @@ -0,0 +1,378 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#include "xe_gputop.h"
> +
> +#define engine_ptr(engines, n) (&(engines)->engine + (n))
> +
> +static void __update_sample(struct xe_pmu_counter *counter, uint64_t val)
> +{
> +	counter->val.prev = counter->val.cur;
> +	counter->val.cur = val;
> +}
> +
> +static void update_sample(struct xe_pmu_counter *counter, uint64_t *val)
> +{
> +	if (counter->present)
> +		__update_sample(counter, val[counter->idx]);
> +}
> +
> +static const char *class_display_name(unsigned int class)
> +{
> +	switch (class) {
> +	case DRM_XE_ENGINE_CLASS_RENDER:
> +		return "Render/3D";
> +	case DRM_XE_ENGINE_CLASS_COPY:
> +		return "Blitter";
> +	case DRM_XE_ENGINE_CLASS_VIDEO_DECODE:
> +		return "Video";
> +	case DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE:
> +		return "VideoEnhance";
> +	case DRM_XE_ENGINE_CLASS_COMPUTE:
> +		return "Compute";
> +	default:
> +		return "[unknown]";
> +	}
> +}
> +
> +void xe_clean_up(void *obj, int len)
> +{
> +	struct xe_engine *eng;
> +	struct xe_pmu_counter pmu;
> +	struct xe_gputop *dev = (struct xe_gputop *)obj;

Use inverted xmas tree

> +
> +	for (int i = 0; i < len; i++) {
> +		if ((dev + i)->card)
> +			free((dev + i)->card);
> +		if ((dev + i)->eng_obj) {
> +
> +			for(int j = 0; j < ((struct xe_pmu_device*)(dev + i)->eng_obj)->num_engines ; j++) {
> +				eng = engine_ptr((struct xe_pmu_device*)(dev + i)->eng_obj, j);
> +				if (eng->display_name)
> +					free(eng->display_name);
> +
> +				pmu = eng->engine_active_ticks;
> +				if (pmu.present)
> +					close(pmu.fd);
> +
> +				pmu = eng->engine_total_ticks;
> +				if (pmu.present)
> +					close(pmu.fd);
> +			}
> +			free(dev->eng_obj);
> +		}			
> +		if ((dev + i)->pmu_device)
> +			free(dev->pmu_device);
> +	}
> +}
> +
> +static char *pmu_name(struct igt_device_card *card)
> +{
> +	int card_fd;
> +	char device[30];
> +	char *path;
> +
> +	if (strlen(card->card))
> +		card_fd = igt_open_card(card);
> +	else if (strlen(card->render))
> +		card_fd = igt_open_render(card);
> +
> +	if (card_fd == -1)
> +		return NULL;
> +
> +	xe_perf_device(card_fd, device, sizeof(device));
> +	path = strdup(device);
> +	close(card_fd);
> +	return path;
> +}
> +
> +static int _open_pmu(uint64_t type, unsigned int *cnt, struct xe_pmu_counter *pmu, int *fd)
> +{
> +	int fd__ = igt_perf_open_group(type, pmu->config, *fd);
> +
> +	if (fd__ >= 0) {
> +		if (*fd == -1)
> +			*fd = fd__;
> +		pmu->present = true;
> +		pmu->idx = (*cnt)++;
> +		pmu->fd = fd__;
> +	}
> +
> +	return fd__;
> +}
> +
> +void xe_gputop_init(void *ptr,
> +		    struct igt_device_card *card)
> +{
> +	struct xe_gputop *obj = (struct xe_gputop *)ptr;
> +
> +	obj->pmu_device = pmu_name(card);
> +	if (!obj->pmu_device) {
> +		fprintf(stderr, "%s : pmu_device path returned NULL", card->pci_slot_name);
> +		exit(EXIT_FAILURE);
> +	}
> +	obj->card = card;
> +}
> +
> +static int pmu_format_shift(int xe, const char *name)
> +{
> +	uint32_t start;
> +	int format;
> +	char device[80];
> +
> +	format = perf_event_format(xe_perf_device(xe, device, sizeof(device)),
> +				   name, &start);
> +	if (format)
> +		return 0;
> +
> +	return start;
> +}
> +
> +static int engine_cmp(const void *__a, const void *__b)
> +{
> +	const struct xe_engine *a = (struct xe_engine *)__a;
> +	const struct xe_engine *b = (struct xe_engine *)__b;
> +
> +	if (a->drm_xe_engine.engine_class != b->drm_xe_engine.engine_class)
> +		return a->drm_xe_engine.engine_class - b->drm_xe_engine.engine_class;
> +	else
> +		return a->drm_xe_engine.engine_instance - b->drm_xe_engine.engine_instance;
> +}
> +
> +void *xe_populate_engines(const void *obj)
> +{
> +	struct igt_device_card *card = ((struct xe_gputop *)obj)->card;
> +	struct xe_pmu_device *engines;
> +	int ret = 0;
> +	char device[30];
> +	struct drm_xe_engine_class_instance *hwe;
> +	int card_fd;
> +	uint64_t engine_class, engine_instance, gt_shift;
> +	uint64_t engine_active_config, engine_total_config;

Use inverted xmas tree

> +
> +	if (!card || !strlen(card->card) || !strlen(card->render))
> +		return NULL;
> +
> +	if (strlen(card->card)) {
> +		card_fd = igt_open_card(card);
> +	} else if (strlen(card->render)) {
> +		card_fd = igt_open_render(card);
> +	} else {
> +		fprintf(stderr, "Failed to detect device!\n");
> +		return NULL;
> +	}
> +	xe_device_get(card_fd);
> +	engines = malloc(sizeof(struct xe_pmu_device) +
> +			 xe_number_engines(card_fd) * sizeof(struct xe_engine));
> +	if (!engines)
> +		return NULL;
> +
> +	memset(engines, 0, sizeof(struct xe_pmu_device) +
> +	       xe_number_engines(card_fd) * sizeof(struct xe_engine));
> +
> +	engines->num_engines = 0;
> +	engines->device = ((struct xe_gputop *)obj)->pmu_device;
> +	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));
> +	ret = perf_event_config(device,
> +				"engine-active-ticks",
> +				&engine_active_config);

wrap around at 100

> +	if (ret < 0)
> +		return NULL;
> +	ret = perf_event_config(device,
> +				"engine-total-ticks",
> +				&engine_total_config);
> +	if (ret < 0)
> +		return NULL;

add blank line

> +	xe_for_each_engine(card_fd, hwe) {
> +		uint64_t  param_config;
> +		struct xe_engine *engine;
> +
> +		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;
> +		engine->engine_active_ticks.config = engine_active_config | param_config;
> +		engine->engine_total_ticks.config = engine_total_config | param_config;
> +
> +		if (engine->engine_active_ticks.config == -1 ||
> +		    engine->engine_total_ticks.config == -1) {
> +			ret = ENOENT;
> +			break;
> +		}
> +
> +		ret = asprintf(&engine->display_name, "%s/%u",
> +			       class_display_name(engine->drm_xe_engine.engine_class),
> +			       engine->drm_xe_engine.engine_instance);
> +
> +		if (ret <= 0) {
> +			ret = errno;
> +			break;
> +		}
> +
> +		engines->num_engines++;
> +	}
> +
> +	if (!ret) {
> +		errno = ret;
> +		return NULL;
> +	}
> +
> +	qsort(engine_ptr(engines, 0), engines->num_engines,
> +	      sizeof(struct xe_engine), engine_cmp);
> +
> +	((struct xe_gputop *)obj)->eng_obj = engines;
> +
> +	return engines;
> +}
> +
> +static uint64_t pmu_read_multi(int fd, unsigned int num, uint64_t *val)
> +{
> +	uint64_t buf[2 + num];
> +	unsigned int i;
> +	ssize_t len;
> +
> +	memset(buf, 0, sizeof(buf));
> +
> +	len = read(fd, buf, sizeof(buf));
> +	assert(len == sizeof(buf));
> +
> +	for (i = 0; i < num; i++)
> +		val[i] = buf[2 + i];
> +
> +	return buf[1];
> +}
> +
> +void xe_pmu_sample(const void *obj)
> +{
> +	struct xe_pmu_device *engines = ((struct xe_gputop *)obj)->eng_obj;
> +	const int num_val = engines->num_counters;
> +	uint64_t val[2 + num_val];
> +	unsigned int i;
> +
> +	pmu_read_multi(engines->fd, num_val, val);
> +
> +	for (i = 0; i < engines->num_engines; i++) {
> +		struct xe_engine *engine = engine_ptr(engines, i);
> +
> +		update_sample(&engine->engine_active_ticks, val);
> +		update_sample(&engine->engine_total_ticks, val);
> +	}
> +}
> +
> +int xe_pmu_init(const void *obj)
> +{
> +	struct xe_pmu_device *engines = ((struct xe_gputop *)obj)->eng_obj;
> +	unsigned int i;
> +	int fd;
> +	struct xe_engine *engine;
> +	uint64_t type = igt_perf_type_id(engines->device);
> +
> +	engines->fd = -1;
> +	engines->num_counters = 0;
> +
> +	for (i = 0; i < engines->num_engines; i++) {
> +		engine = engine_ptr(engines, i);
> +		fd = _open_pmu(type, &engines->num_counters, &engine->engine_active_ticks,
> +			       &engines->fd);
> +		if (fd < 0)
> +			return -1;
> +		fd = _open_pmu(type, &engines->num_counters, &engine->engine_total_ticks,
> +			       &engines->fd);
> +		if (fd < 0)
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +static double pmu_active_percentage(struct xe_engine *engine)
> +{
> +	double pmu_active_ticks = engine->engine_active_ticks.val.cur -
> +				  engine->engine_active_ticks.val.prev;
> +	double pmu_total_ticks = engine->engine_total_ticks.val.cur -
> +				 engine->engine_total_ticks.val.prev;
> +	double percentage;
> +
> +	percentage = (pmu_active_ticks * 100) / pmu_total_ticks;
> +	return percentage;
> +}
> +
> +static int
> +print_device_description(const void *obj, int lines, int w, int h)
> +{
> +	char *desc;
> +	int len;
> +
> +	len = asprintf(&desc, "DRIVER: %s || BDF: %s",
> +		       ((struct xe_gputop *)obj)->card->driver,
> +		       ((struct xe_gputop *)obj)->card->pci_slot_name);
> +
> +	printf("\033[7m%s%*s\033[0m\n",
> +	       desc,
> +	       (int)(w - len), " ");
> +	lines++;
> +	free(desc);
> +	return lines;
> +}
> +
> +static int
> +print_engines_header(struct xe_pmu_device *engines,
> +		     int lines, int con_w, int con_h)
> +{
> +	const char *a;
> +
> +	for (unsigned int i = 0;
> +	     i < engines->num_engines && lines < con_h;
> +	     i++) {
> +		struct xe_engine *engine = engine_ptr(engines, i);
> +
> +		if (!engine->num_counters)
> +			continue;
> +
> +		a = "            ENGINES   ACTIVITY  ";
> +
> +		printf("\033[7m%s%*s\033[0m\n",
> +		       a,
> +		       (int)(con_w - strlen(a)), " ");
> +		lines++;
> +
> +		break;
> +	}
> +
> +	return lines;
> +}
> +
> +static int
> +print_engine(struct xe_pmu_device *engines, unsigned int i,
> +	     int lines, int con_w, int con_h)
> +{
> +	struct xe_engine *engine = engine_ptr(engines, i);
> +	double percentage = pmu_active_percentage(engine);
> +
> +	printf("%*s", (int)(strlen("            ENGINES")), engine->display_name);
> +	print_percentage_bar(percentage, con_w - strlen("            ENGINES"));
> +	printf("\n");
> +
> +	return ++lines;
> +}
> +
> +int xe_print_engines(const void *obj, int lines, int w, int h)
> +{
> +	struct xe_pmu_device *show = ((struct xe_gputop *)obj)->eng_obj;
> +
> +	lines = print_device_description(obj, lines, w, h);
> +
> +	lines = print_engines_header(show, lines, w,  h);
> +
> +	for (unsigned int i = 0; i < show->num_engines && lines < h; i++)
> +		lines = print_engine(show, i, lines, w, h);
> +
> +	lines = print_engines_footer(lines, w, h);
> +
> +	return lines;
> +}
> +
> diff --git a/tools/gputop/xe_gputop.h b/tools/gputop/xe_gputop.h
> new file mode 100644
> index 000000000..825ac7e34
> --- /dev/null
> +++ b/tools/gputop/xe_gputop.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#ifndef __XE_GPUTOP_H__
> +#define __XE_GPUTOP_H__
> +
> +#include <dirent.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;
> +	uint64_t prev;
> +};

Is an extra struct needed here. you can add it in below struct

> +
> +struct xe_pmu_counter {
> +	uint64_t type;	
> +	uint64_t config;
> +	unsigned int idx;
> +	struct xe_pmu_pair val;
> +	int fd;
> +	bool present;
> +};
> +
> +struct xe_engine {
> +	const char *name;
> +	char *display_name;
> +	struct drm_xe_engine_class_instance drm_xe_engine;

%s/drm_xe_engine/engine

> +	unsigned int num_counters;
> +	struct xe_pmu_counter engine_active_ticks;
> +	struct xe_pmu_counter engine_total_ticks;
> +};
> +
> +struct xe_pmu_device {
> +	unsigned int num_engines;
> +	unsigned int num_counters;
> +	int fd;
> +	char *device;
> +	struct xe_engine engine;
> +};
> +
> +struct xe_gputop {
> +	char *pmu_device;
> +	struct igt_device_card *card;
> +	struct xe_pmu_device *eng_obj;
> +};
> +
> +void xe_gputop_init(void *ptr,
> +		    struct igt_device_card *card);

Wrap around at 100

Thanks
Riana

> +void xe_populate_device_instances(struct gputop_device *dv);
> +void *xe_populate_engines(const void *obj);
> +void xe_pmu_sample(const void *obj);
> +int xe_pmu_init(const void *obj);
> +int xe_print_engines(const void *obj, int lines, int w, int h);
> +void xe_clean_up(void *obj, int len);
> +
> +#endif /* __XE_GPUTOP_H__ */

Re: [PATCH i-g-t v11 5/5] tools/gputop/gputop: Enable support for multiple GPUs and instances

[Purkait, Soham]

On 05-06-2025 22:27, Riana Tauro wrote:
> Hi Soham
>
> Code looks good. Have one question and few minor comments below
>
> On 5/22/2025 9:14 PM, Soham Purkait wrote:
>> Introduce vendor-agnostic support for
>> handling multiple GPUs and instances
>> in gputop. Improve the tool's adaptability
>> to various GPU configurations.
>>
>> v2 : Fix for refactoring GPUTOP into a
>>       vendor-agnostic tool. (Lucas)
>>
>> v3 : New year included in copyright. (Kamil, Riana)
>>       Removed caps in function name. (Riana)
>>       Struct for driver specific operations. (Riana)
>>       Headers in alphabetical order. (Kamil, Riana)
>>
>> v4 : Commit description and signed-off included.
>>
>> v5 : Fix for proper resource cleanup. (Riana)
>>       Use "dev_type" enum for card_type. (Krzysztof)
>>       Add new filter to return collection
>>       of matching devices.               (Zbigniew)
>>
>> v6 : Use device filter to populate the array of
>>       cards for all supported drivers.   (Zbigniew)
>>
>> v7 : Use filter to find all the cards.  (Zbigniew)
>>
>> v8 : Removed 'drivers' array parameter from
>>       card match function.               (Zbigniew)
>>
>> v10 : Resolved 'populate_devices' call with
>>        pci subsystem filtering.          (Zbigniew)
>>
>> v11 : Add space after /* and before */ for better
>>        readability.                      (Zbigniew)
>>        Comments wrapped at 75/100.       (Riana)
>>
>> Signed-off-by: Soham Purkait <soham.purkait@intel.com>
>>
>> Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>> ---
>>   tools/{ => gputop}/gputop.c | 223 +++++++++++++++++++++++++++++-------
>>   tools/gputop/meson.build    |   6 +
>>   tools/meson.build           |   6 +-
>>   3 files changed, 191 insertions(+), 44 deletions(-)
>>   rename tools/{ => gputop}/gputop.c (66%)
>>   create mode 100644 tools/gputop/meson.build
>>
>> diff --git a/tools/gputop.c b/tools/gputop/gputop.c
>> similarity index 66%
>> rename from tools/gputop.c
>> rename to tools/gputop/gputop.c
>> index 43b01f566..fc3ccb83b 100644
>> --- a/tools/gputop.c
>> +++ b/tools/gputop/gputop.c
>> @@ -1,6 +1,6 @@
>>   // SPDX-License-Identifier: MIT
>>   /*
>> - * Copyright © 2023 Intel Corporation
>> + * Copyright © 2023-2025 Intel Corporation
>>    */
>>     #include <assert.h>
>> @@ -14,66 +14,145 @@
>>   #include <math.h>
>>   #include <poll.h>
>>   #include <signal.h>
>> +#include <stdbool.h>
>>   #include <stdint.h>
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   #include <string.h>
>> +#include <sys/sysmacros.h>
>
> alphabetical
>
>>   #include <sys/ioctl.h>
>>   #include <sys/stat.h>
>>   #include <sys/types.h>
>> -#include <unistd.h>
>>   #include <termios.h>
>> -#include <sys/sysmacros.h>
>> -#include <stdbool.h>
>> +#include <unistd.h>
>>   +#include "drmtest.h"
>>   #include "igt_core.h"
>>   #include "igt_drm_clients.h"
>>   #include "igt_drm_fdinfo.h"
>> +#include "igt_perf.h"
>>   #include "igt_profiling.h"
>> -#include "drmtest.h"
>> +#include "xe_gputop.h"
>> +#include "xe/xe_query.h"
>> +
>> +/**
>> + * Supported Drivers
>> + *
>> + * Adhere to the following requirements when implementing support 
>> for the
>> + * new driver:
>> + * @drivers: Update drivers[] with driver string.
>> + * @total_count: Update NUM_DRIVER with the total number of 
>> supported drivers.
>> + * @operations: Update the respective operations of the new driver:
>> + * gputop_init,
>> + * discover_engines,
>> + * pmu_init,
>> + * pmu_sample,
>> + * print_engines,
>> + * clean_up
>> + * @devices: Update devices[] array of type "struct gputop_device" 
>> with the
>> + * initial values.
>> + */
>> +static const char * const drivers[] = {
>> +    "xe",
>> +    /* Keep the last one as NULL */
>> +    NULL
>> +};
>> +
>> +/**
>> + * Number of supported drivers needs to be adjusted as per the 
>> length of
>> + * the drivers[] array.
>> + */
>> +#define NUM_DRIVER 1
>> +
>> +/**
>> + * Supported operations on driver instances. Update the oprs[] array 
>> for
>> + * each individual driver specific function. Maintain the sequence 
>> as per
>> + * drivers[] array.
>> + */
>> +struct device_operations oprs[NUM_DRIVER] = {
>
> Replace oprs with ops.. generally use ops for operations
Isn't ops more sound like options ?
>> +    {
>> +        xe_gputop_init,
>> +        xe_populate_engines,
>> +        xe_pmu_init,
>> +        xe_pmu_sample,
>> +        xe_print_engines,
>> +        xe_clean_up
>> +    }
>> +};
>> +
>> +/*
>> + * devices[] array of type struct gputop_device
>> + */
>> +struct gputop_device devices[] = {
>
> I must have commented here before. As i see this is one entry per 
> driver right? why is it named device?

Actually this is keeping track of the devices for each driver, so the name.

I have updated the description for better readability.

>
>> +    {false, 0, NULL}
>> +};
>>     enum utilization_type {
>>       UTILIZATION_TYPE_ENGINE_TIME,
>>       UTILIZATION_TYPE_TOTAL_CYCLES,
>>   };
>>   -static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", 
>> "▉", "█" };
>> -
>> -#define ANSI_HEADER "\033[7m"
>> -#define ANSI_RESET "\033[0m"
>> -
>> -static void n_spaces(const unsigned int n)
>> +static void gputop_clean_up(void)
>>   {
>> -    unsigned int i;
>> -
>> -    for (i = 0; i < n; i++)
>> -        putchar(' ');
>> +    for (int i = 0; drivers[i]; i++) {
>> +        oprs[i].clean_up(devices[i].instances, devices[i].len);
>> +        free(devices[i].instances);
>> +        devices[i].driver_present = false;
>> +        devices[i].len = 0;
>> +    }
>>   }
>>   -static void print_percentage_bar(double percent, int max_len)
>> +static int find_driver(struct igt_device_card *card)
>>   {
>> -    int bar_len, i, len = max_len - 1;
>> -    const int w = 8;
>> -
>> -    len -= printf("|%5.1f%% ", percent);
>> -
>> -    /* no space left for bars, do what we can */
>> -    if (len < 0)
>> -        len = 0;
>> -
>> -    bar_len = ceil(w * percent * len / 100.0);
>> -    if (bar_len > w * len)
>> -        bar_len = w * len;
>> +    for (int i = 0; drivers[i]; i++) {
>> +        if (strcmp(drivers[i], card->driver) == 0)
>> +            return i;
>> +    }
>> +    return -1;
>> +}
>>   -    for (i = bar_len; i >= w; i -= w)
>> -        printf("%s", bars[w]);
>> -    if (i)
>> -        printf("%s", bars[i]);
>> +/*
>> + * If filter is not NULL i will be ignored.
>> + */
>> +static int populate_device_instances(const char *filter)
>> +{
>> +    struct igt_device_card *cards = NULL;
>> +    struct igt_device_card *card_inplace = NULL;
>> +    struct gputop_device *dev =  NULL;
>> +    int driver_no;
>> +    int count, final_count = 0;
>
> %s/final_count/card_count
Here count and final_count both are keeping track of the cards but 
final_count is the count of supported filtered devices, so the name.
>
>> +
>> +    count = igt_device_card_match_all(filter, &cards);
>> +    for (int j = 0; j < count; j++) {
>> +        if (strcmp((cards + j)->subsystem, "pci") != 0)
>> +            continue;
>>   -    len -= (bar_len + (w - 1)) / w;
>> -    n_spaces(len);
>> +        driver_no = find_driver(cards + j);
>> +        if (driver_no < 0)
>> +            continue;
>>   -    putchar('|');
>> +        dev = devices + driver_no;
>> +        if (!dev->driver_present)
>> +            dev->driver_present = true;
>> +        dev->len++;
>> +        dev->instances = realloc(dev->instances,
>> +                     dev->len * sizeof(struct xe_gputop));
>> +        if (!dev->instances) {
>> +            fprintf(stderr,
>> +                "Device instance realloc failed (%s)\n",
>> +                strerror(errno));
>> +            exit(EXIT_FAILURE);
>> +        }
>> +        card_inplace = (struct igt_device_card *)
>> +                calloc(1, sizeof(struct igt_device_card));
>> +        memcpy(card_inplace, cards + j, sizeof(struct 
>> igt_device_card));
>> +        oprs[driver_no].gputop_init((struct xe_gputop 
>> *)(dev->instances + dev->len - 1),
>> +            card_inplace);
>> +        final_count++;
>> +    }
>> +    if (count)
>> +        free(cards);
>> +    return final_count;
>>   }
>>     static int
>> @@ -333,6 +412,7 @@ static void clrscr(void)
>>   struct gputop_args {
>>       long n_iter;
>>       unsigned long delay_usec;
>> +    char *device;
>>   };
>>     static void help(void)
>> @@ -343,16 +423,18 @@ static void help(void)
>>              "\t-h, --help                show this help\n"
>>              "\t-d, --delay =SEC[.TENTHS] iterative delay as SECS 
>> [.TENTHS]\n"
>>              "\t-n, --iterations =NUMBER  number of executions\n"
>> +           "\t-D, --device              Device filter\n"
>>              , program_invocation_short_name);
>>   }
>>     static int parse_args(int argc, char * const argv[], struct 
>> gputop_args *args)
>>   {
>> -    static const char cmdopts_s[] = "hn:d:";
>> +    static const char cmdopts_s[] = "hn:d:D:";
>>       static const struct option cmdopts[] = {
>>              {"help", no_argument, 0, 'h'},
>>              {"delay", required_argument, 0, 'd'},
>>              {"iterations", required_argument, 0, 'n'},
>> +           {"device", required_argument, 0, 'D'},
>>              { }
>>       };
>>   @@ -360,6 +442,7 @@ static int parse_args(int argc, char * const 
>> argv[], struct gputop_args *args)
>>       memset(args, 0, sizeof(*args));
>>       args->n_iter = -1;
>>       args->delay_usec = 2 * USEC_PER_SEC;
>> +    args->device = NULL;
>>         for (;;) {
>>           int c, idx = 0;
>> @@ -383,6 +466,9 @@ static int parse_args(int argc, char * const 
>> argv[], struct gputop_args *args)
>>                   return -1;
>>               }
>>               break;
>> +        case 'D':
>> +            args->device = optarg;
>> +            break;
>>           case 'h':
>>               help();
>>               return 0;
>> @@ -422,6 +508,52 @@ int main(int argc, char **argv)
>>       n = args.n_iter;
>>       period_us = args.delay_usec;
>>   +    if (!populate_device_instances(args.device ? args.device
>> +                       : "device:subsystem=pci,card=all")) {
>> +        printf("No device found.\n");
>> +        gputop_clean_up();
>> +        exit(1);
>> +    }
>> +
>> +    for (int i = 0; drivers[i]; i++) {
>> +        if (devices[i].driver_present) {
>
> if not present continue
It will be at the end of the outer loop if not present so it eventually  
continues.
>
> Thanks
> Riana
>
>> +            for (int j = 0; j < devices[i].len; j++) {
>> +                if (!oprs[i].init_engines(devices[i].instances + j)) {
>> +                    fprintf(stderr,
>> +                        "Failed to initialize engines! (%s)\n",
>> +                        strerror(errno));
>> +                        gputop_clean_up();
>> +                    return EXIT_FAILURE;
>> +                }
>> +                ret = oprs[i].pmu_init(devices[i].instances + j);
>> +
>> +                if (ret) {
>> +                    fprintf(stderr,
>> +                        "Failed to initialize PMU! (%s)\n",
>> +                        strerror(errno));
>> +                    if (errno == EACCES && geteuid())
>> +                        fprintf(stderr,
>> +                            "\n"
>> +                            "When running as a normal user 
>> CAP_PERFMON is required to access performance\n"
>> +                            "monitoring. See \"man 7 capabilities\", 
>> \"man 8 setcap\", or contact your\n"
>> +                            "distribution vendor for assistance.\n"
>> +                            "\n"
>> +                            "More information can be found at 'Perf 
>> events and tool security' document:\n"
>> + 
>> "https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html\n");
>> +
>> +                    igt_devices_free();
>> +                    gputop_clean_up();
>> +                    return EXIT_FAILURE;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    for (int i = 0; drivers[i]; i++) {
>> +        for (int j = 0; devices[i].driver_present && j < 
>> devices[i].len; j++)
>> +            oprs[i].pmu_sample(devices[i].instances + j);
>> +    }
>> +
>>       clients = igt_drm_clients_init(NULL);
>>       if (!clients)
>>           exit(1);
>> @@ -442,14 +574,27 @@ int main(int argc, char **argv)
>>         while ((n != 0) && !stop_top) {
>>           struct igt_drm_client *c, *prevc = NULL;
>> -        int i, engine_w = 0, lines = 0;
>> +        int k, engine_w = 0, lines = 0;
>>             igt_drm_clients_scan(clients, NULL, NULL, 0, NULL, 0);
>> +
>> +        for (int i = 0; drivers[i]; i++) {
>> +            for (int j = 0; devices[i].driver_present && j < 
>> devices[i].len; j++)
>> +                oprs[i].pmu_sample(devices[i].instances + j);
>> +        }
>> +
>>           igt_drm_clients_sort(clients, client_cmp);
>>             update_console_size(&con_w, &con_h);
>>           clrscr();
>>   +        for (int i = 0; drivers[i]; i++) {
>> +            for (int j = 0; devices[i].driver_present && j < 
>> devices[i].len; j++) {
>> +                lines = oprs[i].print_engines(devices[i].instances + j,
>> +                             lines, con_w, con_h);
>> +            }
>> +        }
>> +
>>           if (!clients->num_clients) {
>>               const char *msg = " (No GPU clients yet. Start workload 
>> to see stats)";
>>   @@ -457,7 +602,7 @@ int main(int argc, char **argv)
>>                      (int)(con_w - strlen(msg) - 1), msg);
>>           }
>>   -        igt_for_each_drm_client(clients, c, i) {
>> +        igt_for_each_drm_client(clients, c, k) {
>>               assert(c->status != IGT_DRM_CLIENT_PROBE);
>>               if (c->status != IGT_DRM_CLIENT_ALIVE)
>>                   break; /* Active clients are first in the array. */
>> @@ -481,11 +626,11 @@ int main(int argc, char **argv)
>>       }
>>         igt_drm_clients_free(clients);
>> +    gputop_clean_up();
>>         if (profiled_devices != NULL) {
>>           igt_devices_configure_profiling(profiled_devices, false);
>>           igt_devices_free_profiling(profiled_devices);
>>       }
>> -
>>       return 0;
>>   }
>> diff --git a/tools/gputop/meson.build b/tools/gputop/meson.build
>> new file mode 100644
>> index 000000000..4766d8496
>> --- /dev/null
>> +++ b/tools/gputop/meson.build
>> @@ -0,0 +1,6 @@
>> +gputop_src = [ 'gputop.c', 'utils.c', 'xe_gputop.c']
>> +executable('gputop', sources : gputop_src,
>> +           install : true,
>> +           install_rpath : bindir_rpathdir,
>> +           dependencies : 
>> [igt_deps,lib_igt_perf,lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math],
>> +       install: true)
>> diff --git a/tools/meson.build b/tools/meson.build
>> index de866c392..8002f707d 100644
>> --- a/tools/meson.build
>> +++ b/tools/meson.build
>> @@ -69,11 +69,6 @@ if libudev.found()
>>              install : true)
>>   endif
>>   -executable('gputop', 'gputop.c',
>> -           install : true,
>> -           install_rpath : bindir_rpathdir,
>> -           dependencies : 
>> [lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math])
>> -
>>   intel_l3_parity_src = [ 'intel_l3_parity.c', 
>> 'intel_l3_udev_listener.c' ]
>>   executable('intel_l3_parity', sources : intel_l3_parity_src,
>>          dependencies : tool_deps,
>> @@ -122,3 +117,4 @@ endif
>>   subdir('i915-perf')
>>   subdir('xe-perf')
>>   subdir('null_state_gen')
>> +subdir('gputop')
>

Re: [PATCH i-g-t v11 5/5] tools/gputop/gputop: Enable support for multiple GPUs and instances

[Riana Tauro]

Hi Soham

On 6/11/2025 12:41 PM, Purkait, Soham wrote:
> 
> On 05-06-2025 22:27, Riana Tauro wrote:
>> Hi Soham
>>
>> Code looks good. Have one question and few minor comments below
>>
>> On 5/22/2025 9:14 PM, Soham Purkait wrote:
>>> Introduce vendor-agnostic support for
>>> handling multiple GPUs and instances
>>> in gputop. Improve the tool's adaptability
>>> to various GPU configurations.
>>>
>>> v2 : Fix for refactoring GPUTOP into a
>>>       vendor-agnostic tool. (Lucas)
>>>
>>> v3 : New year included in copyright. (Kamil, Riana)
>>>       Removed caps in function name. (Riana)
>>>       Struct for driver specific operations. (Riana)
>>>       Headers in alphabetical order. (Kamil, Riana)
>>>
>>> v4 : Commit description and signed-off included.
>>>
>>> v5 : Fix for proper resource cleanup. (Riana)
>>>       Use "dev_type" enum for card_type. (Krzysztof)
>>>       Add new filter to return collection
>>>       of matching devices.               (Zbigniew)
>>>
>>> v6 : Use device filter to populate the array of
>>>       cards for all supported drivers.   (Zbigniew)
>>>
>>> v7 : Use filter to find all the cards.  (Zbigniew)
>>>
>>> v8 : Removed 'drivers' array parameter from
>>>       card match function.               (Zbigniew)
>>>
>>> v10 : Resolved 'populate_devices' call with
>>>        pci subsystem filtering.          (Zbigniew)
>>>
>>> v11 : Add space after /* and before */ for better
>>>        readability.                      (Zbigniew)
>>>        Comments wrapped at 75/100.       (Riana)
>>>
>>> Signed-off-by: Soham Purkait <soham.purkait@intel.com>
>>>
>>> Acked-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
>>> ---
>>>   tools/{ => gputop}/gputop.c | 223 +++++++++++++++++++++++++++++-------
>>>   tools/gputop/meson.build    |   6 +
>>>   tools/meson.build           |   6 +-
>>>   3 files changed, 191 insertions(+), 44 deletions(-)
>>>   rename tools/{ => gputop}/gputop.c (66%)
>>>   create mode 100644 tools/gputop/meson.build
>>>
>>> diff --git a/tools/gputop.c b/tools/gputop/gputop.c
>>> similarity index 66%
>>> rename from tools/gputop.c
>>> rename to tools/gputop/gputop.c
>>> index 43b01f566..fc3ccb83b 100644
>>> --- a/tools/gputop.c
>>> +++ b/tools/gputop/gputop.c
>>> @@ -1,6 +1,6 @@
>>>   // SPDX-License-Identifier: MIT
>>>   /*
>>> - * Copyright © 2023 Intel Corporation
>>> + * Copyright © 2023-2025 Intel Corporation
>>>    */
>>>     #include <assert.h>
>>> @@ -14,66 +14,145 @@
>>>   #include <math.h>
>>>   #include <poll.h>
>>>   #include <signal.h>
>>> +#include <stdbool.h>
>>>   #include <stdint.h>
>>>   #include <stdio.h>
>>>   #include <stdlib.h>
>>>   #include <string.h>
>>> +#include <sys/sysmacros.h>
>>
>> alphabetical
>>
>>>   #include <sys/ioctl.h>
>>>   #include <sys/stat.h>
>>>   #include <sys/types.h>
>>> -#include <unistd.h>
>>>   #include <termios.h>
>>> -#include <sys/sysmacros.h>
>>> -#include <stdbool.h>
>>> +#include <unistd.h>
>>>   +#include "drmtest.h"
>>>   #include "igt_core.h"
>>>   #include "igt_drm_clients.h"
>>>   #include "igt_drm_fdinfo.h"
>>> +#include "igt_perf.h"
>>>   #include "igt_profiling.h"
>>> -#include "drmtest.h"
>>> +#include "xe_gputop.h"
>>> +#include "xe/xe_query.h"
>>> +
>>> +/**
>>> + * Supported Drivers
>>> + *
>>> + * Adhere to the following requirements when implementing support 
>>> for the
>>> + * new driver:
>>> + * @drivers: Update drivers[] with driver string.
>>> + * @total_count: Update NUM_DRIVER with the total number of 
>>> supported drivers.
>>> + * @operations: Update the respective operations of the new driver:
>>> + * gputop_init,
>>> + * discover_engines,
>>> + * pmu_init,
>>> + * pmu_sample,
>>> + * print_engines,
>>> + * clean_up
>>> + * @devices: Update devices[] array of type "struct gputop_device" 
>>> with the
>>> + * initial values.
>>> + */
>>> +static const char * const drivers[] = {
>>> +    "xe",
>>> +    /* Keep the last one as NULL */
>>> +    NULL
>>> +};
>>> +
>>> +/**
>>> + * Number of supported drivers needs to be adjusted as per the 
>>> length of
>>> + * the drivers[] array.
>>> + */
>>> +#define NUM_DRIVER 1
>>> +
>>> +/**
>>> + * Supported operations on driver instances. Update the oprs[] array 
>>> for
>>> + * each individual driver specific function. Maintain the sequence 
>>> as per
>>> + * drivers[] array.
>>> + */
>>> +struct device_operations oprs[NUM_DRIVER] = {
>>
>> Replace oprs with ops.. generally use ops for operations
> Isn't ops more sound like options ?

Some examples like fops, pm_ops in kernel are for operations
Trivial comment upto you

>>> +    {
>>> +        xe_gputop_init,
>>> +        xe_populate_engines,
>>> +        xe_pmu_init,
>>> +        xe_pmu_sample,
>>> +        xe_print_engines,
>>> +        xe_clean_up
>>> +    }
>>> +};
>>> +
>>> +/*
>>> + * devices[] array of type struct gputop_device
>>> + */
>>> +struct gputop_device devices[] = {
>>
>> I must have commented here before. As i see this is one entry per 
>> driver right? why is it named device?
> 
> Actually this is keeping track of the devices for each driver, so the name.

yeah it tracks device count per driver. The array entries are per driver 
not device. Each entry is not a gputop_device as the name suggests

> 
> I have updated the description for better readability.
> >>
>>> +    {false, 0, NULL}
>>> +};
>>>     enum utilization_type {
>>>       UTILIZATION_TYPE_ENGINE_TIME,
>>>       UTILIZATION_TYPE_TOTAL_CYCLES,
>>>   };
>>>   -static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", 
>>> "▉", "█" };
>>> -
>>> -#define ANSI_HEADER "\033[7m"
>>> -#define ANSI_RESET "\033[0m"
>>> -
>>> -static void n_spaces(const unsigned int n)
>>> +static void gputop_clean_up(void)
>>>   {
>>> -    unsigned int i;
>>> -
>>> -    for (i = 0; i < n; i++)
>>> -        putchar(' ');
>>> +    for (int i = 0; drivers[i]; i++) {
>>> +        oprs[i].clean_up(devices[i].instances, devices[i].len);
>>> +        free(devices[i].instances);
>>> +        devices[i].driver_present = false;
>>> +        devices[i].len = 0;
>>> +    }
>>>   }
>>>   -static void print_percentage_bar(double percent, int max_len)
>>> +static int find_driver(struct igt_device_card *card)
>>>   {
>>> -    int bar_len, i, len = max_len - 1;
>>> -    const int w = 8;
>>> -
>>> -    len -= printf("|%5.1f%% ", percent);
>>> -
>>> -    /* no space left for bars, do what we can */
>>> -    if (len < 0)
>>> -        len = 0;
>>> -
>>> -    bar_len = ceil(w * percent * len / 100.0);
>>> -    if (bar_len > w * len)
>>> -        bar_len = w * len;
>>> +    for (int i = 0; drivers[i]; i++) {
>>> +        if (strcmp(drivers[i], card->driver) == 0)
>>> +            return i;
>>> +    }
>>> +    return -1;
>>> +}
>>>   -    for (i = bar_len; i >= w; i -= w)
>>> -        printf("%s", bars[w]);
>>> -    if (i)
>>> -        printf("%s", bars[i]);
>>> +/*
>>> + * If filter is not NULL i will be ignored.
>>> + */
>>> +static int populate_device_instances(const char *filter)
>>> +{
>>> +    struct igt_device_card *cards = NULL;
>>> +    struct igt_device_card *card_inplace = NULL;
>>> +    struct gputop_device *dev =  NULL;
>>> +    int driver_no;
>>> +    int count, final_count = 0;
>>
>> %s/final_count/card_count
> Here count and final_count both are keeping track of the cards but 
> final_count is the count of supported filtered devices, so the name.

Trivial comment . upto you

>>
>>> +
>>> +    count = igt_device_card_match_all(filter, &cards);
>>> +    for (int j = 0; j < count; j++) {
>>> +        if (strcmp((cards + j)->subsystem, "pci") != 0)
>>> +            continue;
>>>   -    len -= (bar_len + (w - 1)) / w;
>>> -    n_spaces(len);
>>> +        driver_no = find_driver(cards + j);
>>> +        if (driver_no < 0)
>>> +            continue;
>>>   -    putchar('|');
>>> +        dev = devices + driver_no;
>>> +        if (!dev->driver_present)
>>> +            dev->driver_present = true;
>>> +        dev->len++;
>>> +        dev->instances = realloc(dev->instances,
>>> +                     dev->len * sizeof(struct xe_gputop));
>>> +        if (!dev->instances) {
>>> +            fprintf(stderr,
>>> +                "Device instance realloc failed (%s)\n",
>>> +                strerror(errno));
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>> +        card_inplace = (struct igt_device_card *)
>>> +                calloc(1, sizeof(struct igt_device_card));
>>> +        memcpy(card_inplace, cards + j, sizeof(struct 
>>> igt_device_card));
>>> +        oprs[driver_no].gputop_init((struct xe_gputop *)(dev- 
>>> >instances + dev->len - 1),
>>> +            card_inplace);
>>> +        final_count++;
>>> +    }
>>> +    if (count)
>>> +        free(cards);
>>> +    return final_count;
>>>   }
>>>     static int
>>> @@ -333,6 +412,7 @@ static void clrscr(void)
>>>   struct gputop_args {
>>>       long n_iter;
>>>       unsigned long delay_usec;
>>> +    char *device;
>>>   };
>>>     static void help(void)
>>> @@ -343,16 +423,18 @@ static void help(void)
>>>              "\t-h, --help                show this help\n"
>>>              "\t-d, --delay =SEC[.TENTHS] iterative delay as SECS 
>>> [.TENTHS]\n"
>>>              "\t-n, --iterations =NUMBER  number of executions\n"
>>> +           "\t-D, --device              Device filter\n"
>>>              , program_invocation_short_name);
>>>   }
>>>     static int parse_args(int argc, char * const argv[], struct 
>>> gputop_args *args)
>>>   {
>>> -    static const char cmdopts_s[] = "hn:d:";
>>> +    static const char cmdopts_s[] = "hn:d:D:";
>>>       static const struct option cmdopts[] = {
>>>              {"help", no_argument, 0, 'h'},
>>>              {"delay", required_argument, 0, 'd'},
>>>              {"iterations", required_argument, 0, 'n'},
>>> +           {"device", required_argument, 0, 'D'},
>>>              { }
>>>       };
>>>   @@ -360,6 +442,7 @@ static int parse_args(int argc, char * const 
>>> argv[], struct gputop_args *args)
>>>       memset(args, 0, sizeof(*args));
>>>       args->n_iter = -1;
>>>       args->delay_usec = 2 * USEC_PER_SEC;
>>> +    args->device = NULL;
>>>         for (;;) {
>>>           int c, idx = 0;
>>> @@ -383,6 +466,9 @@ static int parse_args(int argc, char * const 
>>> argv[], struct gputop_args *args)
>>>                   return -1;
>>>               }
>>>               break;
>>> +        case 'D':
>>> +            args->device = optarg;
>>> +            break;
>>>           case 'h':
>>>               help();
>>>               return 0;
>>> @@ -422,6 +508,52 @@ int main(int argc, char **argv)
>>>       n = args.n_iter;
>>>       period_us = args.delay_usec;
>>>   +    if (!populate_device_instances(args.device ? args.device
>>> +                       : "device:subsystem=pci,card=all")) {
>>> +        printf("No device found.\n");
>>> +        gputop_clean_up();
>>> +        exit(1);
>>> +    }
>>> +
>>> +    for (int i = 0; drivers[i]; i++) {
>>> +        if (devices[i].driver_present) {
>>
>> if not present continue
> It will be at the end of the outer loop if not present so it eventually 
> continues.

The comment was to reduce the indentation and improve readability

Thanks
Riana

>>>> Thanks
>> Riana
>>
>>> +            for (int j = 0; j < devices[i].len; j++) {
>>> +                if (!oprs[i].init_engines(devices[i].instances + j)) {
>>> +                    fprintf(stderr,
>>> +                        "Failed to initialize engines! (%s)\n",
>>> +                        strerror(errno));
>>> +                        gputop_clean_up();
>>> +                    return EXIT_FAILURE;
>>> +                }
>>> +                ret = oprs[i].pmu_init(devices[i].instances + j);
>>> +
>>> +                if (ret) {
>>> +                    fprintf(stderr,
>>> +                        "Failed to initialize PMU! (%s)\n",
>>> +                        strerror(errno));
>>> +                    if (errno == EACCES && geteuid())
>>> +                        fprintf(stderr,
>>> +                            "\n"
>>> +                            "When running as a normal user 
>>> CAP_PERFMON is required to access performance\n"
>>> +                            "monitoring. See \"man 7 capabilities\", 
>>> \"man 8 setcap\", or contact your\n"
>>> +                            "distribution vendor for assistance.\n"
>>> +                            "\n"
>>> +                            "More information can be found at 'Perf 
>>> events and tool security' document:\n"
>>> + "https://www.kernel.org/doc/html/latest/admin-guide/perf- 
>>> security.html\n");
>>> +
>>> +                    igt_devices_free();
>>> +                    gputop_clean_up();
>>> +                    return EXIT_FAILURE;
>>> +                }
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    for (int i = 0; drivers[i]; i++) {
>>> +        for (int j = 0; devices[i].driver_present && j < 
>>> devices[i].len; j++)
>>> +            oprs[i].pmu_sample(devices[i].instances + j);
>>> +    }
>>> +
>>>       clients = igt_drm_clients_init(NULL);
>>>       if (!clients)
>>>           exit(1);
>>> @@ -442,14 +574,27 @@ int main(int argc, char **argv)
>>>         while ((n != 0) && !stop_top) {
>>>           struct igt_drm_client *c, *prevc = NULL;
>>> -        int i, engine_w = 0, lines = 0;
>>> +        int k, engine_w = 0, lines = 0;
>>>             igt_drm_clients_scan(clients, NULL, NULL, 0, NULL, 0);
>>> +
>>> +        for (int i = 0; drivers[i]; i++) {
>>> +            for (int j = 0; devices[i].driver_present && j < 
>>> devices[i].len; j++)
>>> +                oprs[i].pmu_sample(devices[i].instances + j);
>>> +        }
>>> +
>>>           igt_drm_clients_sort(clients, client_cmp);
>>>             update_console_size(&con_w, &con_h);
>>>           clrscr();
>>>   +        for (int i = 0; drivers[i]; i++) {
>>> +            for (int j = 0; devices[i].driver_present && j < 
>>> devices[i].len; j++) {
>>> +                lines = oprs[i].print_engines(devices[i].instances + j,
>>> +                             lines, con_w, con_h);
>>> +            }
>>> +        }
>>> +
>>>           if (!clients->num_clients) {
>>>               const char *msg = " (No GPU clients yet. Start workload 
>>> to see stats)";
>>>   @@ -457,7 +602,7 @@ int main(int argc, char **argv)
>>>                      (int)(con_w - strlen(msg) - 1), msg);
>>>           }
>>>   -        igt_for_each_drm_client(clients, c, i) {
>>> +        igt_for_each_drm_client(clients, c, k) {
>>>               assert(c->status != IGT_DRM_CLIENT_PROBE);
>>>               if (c->status != IGT_DRM_CLIENT_ALIVE)
>>>                   break; /* Active clients are first in the array. */
>>> @@ -481,11 +626,11 @@ int main(int argc, char **argv)
>>>       }
>>>         igt_drm_clients_free(clients);
>>> +    gputop_clean_up();
>>>         if (profiled_devices != NULL) {
>>>           igt_devices_configure_profiling(profiled_devices, false);
>>>           igt_devices_free_profiling(profiled_devices);
>>>       }
>>> -
>>>       return 0;
>>>   }
>>> diff --git a/tools/gputop/meson.build b/tools/gputop/meson.build
>>> new file mode 100644
>>> index 000000000..4766d8496
>>> --- /dev/null
>>> +++ b/tools/gputop/meson.build
>>> @@ -0,0 +1,6 @@
>>> +gputop_src = [ 'gputop.c', 'utils.c', 'xe_gputop.c']
>>> +executable('gputop', sources : gputop_src,
>>> +           install : true,
>>> +           install_rpath : bindir_rpathdir,
>>> +           dependencies : 
>>> [igt_deps,lib_igt_perf,lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math],
>>> +       install: true)
>>> diff --git a/tools/meson.build b/tools/meson.build
>>> index de866c392..8002f707d 100644
>>> --- a/tools/meson.build
>>> +++ b/tools/meson.build
>>> @@ -69,11 +69,6 @@ if libudev.found()
>>>              install : true)
>>>   endif
>>>   -executable('gputop', 'gputop.c',
>>> -           install : true,
>>> -           install_rpath : bindir_rpathdir,
>>> -           dependencies : 
>>> [lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math])
>>> -
>>>   intel_l3_parity_src = [ 'intel_l3_parity.c', 
>>> 'intel_l3_udev_listener.c' ]
>>>   executable('intel_l3_parity', sources : intel_l3_parity_src,
>>>          dependencies : tool_deps,
>>> @@ -122,3 +117,4 @@ endif
>>>   subdir('i915-perf')
>>>   subdir('xe-perf')
>>>   subdir('null_state_gen')
>>> +subdir('gputop')
>>

Re: [PATCH i-g-t v11 4/5] tools/gputop/xe_gputop: Add gputop support for xe specific devices

[Purkait, Soham]

On 05-06-2025 22:46, Riana Tauro wrote:
> Hi Soham
>
> Some minor comments
>
> On 5/22/2025 9:14 PM, Soham Purkait wrote:
>> Add gputop support for xe-specific devices. Separate
>> driver-specific code into respective source files.
>>
>> v2 : Fix for refactoring GPUTOP into a
>>       vendor-agnostic tool. (Lucas)
>>
>> v3 : Separate commit. (Kamil)
>>
>> v4 : Headers in alphabetical order
>>       Engines memory allocation at
>>       the beginning all at once.
>>       Removed PMU normalization. (Riana)
>>
>> v5 : Refactor to eliminate redundant
>>       and unused code segments.
>>       Fix for proper resource cleanup. (Riana)
>>
>> v8 : Allocated card structure memory inplace and
>>       accordingly modified the clean up code.
>>
>> v11 : Loop optimization in xe_populate_engines.
>>        Removed short_name.
>>        PMU fds are closed on cleanup.
>>        Removed unnecessary comments. (Riana)
>
> lot of versions maybe squash all the comments into one and start new 
> series. But upto you
>
>>
>> Signed-off-by: Soham Purkait <soham.purkait@intel.com>
>> ---
>>   tools/gputop/xe_gputop.c | 378 +++++++++++++++++++++++++++++++++++++++
>>   tools/gputop/xe_gputop.h |  62 +++++++
>>   2 files changed, 440 insertions(+)
>>   create mode 100644 tools/gputop/xe_gputop.c
>>   create mode 100644 tools/gputop/xe_gputop.h
>>
>> diff --git a/tools/gputop/xe_gputop.c b/tools/gputop/xe_gputop.c
>> new file mode 100644
>> index 000000000..ac3ed76e6
>> --- /dev/null
>> +++ b/tools/gputop/xe_gputop.c
>> @@ -0,0 +1,378 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#include "xe_gputop.h"
>> +
>> +#define engine_ptr(engines, n) (&(engines)->engine + (n))
>> +
>> +static void __update_sample(struct xe_pmu_counter *counter, uint64_t 
>> val)
>> +{
>> +    counter->val.prev = counter->val.cur;
>> +    counter->val.cur = val;
>> +}
>> +
>> +static void update_sample(struct xe_pmu_counter *counter, uint64_t 
>> *val)
>> +{
>> +    if (counter->present)
>> +        __update_sample(counter, val[counter->idx]);
>> +}
>> +
>> +static const char *class_display_name(unsigned int class)
>> +{
>> +    switch (class) {
>> +    case DRM_XE_ENGINE_CLASS_RENDER:
>> +        return "Render/3D";
>> +    case DRM_XE_ENGINE_CLASS_COPY:
>> +        return "Blitter";
>> +    case DRM_XE_ENGINE_CLASS_VIDEO_DECODE:
>> +        return "Video";
>> +    case DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE:
>> +        return "VideoEnhance";
>> +    case DRM_XE_ENGINE_CLASS_COMPUTE:
>> +        return "Compute";
>> +    default:
>> +        return "[unknown]";
>> +    }
>> +}
>> +
>> +void xe_clean_up(void *obj, int len)
>> +{
>> +    struct xe_engine *eng;
>> +    struct xe_pmu_counter pmu;
>> +    struct xe_gputop *dev = (struct xe_gputop *)obj;
>
> Use inverted xmas tree
>
>> +
>> +    for (int i = 0; i < len; i++) {
>> +        if ((dev + i)->card)
>> +            free((dev + i)->card);
>> +        if ((dev + i)->eng_obj) {
>> +
>> +            for(int j = 0; j < ((struct xe_pmu_device*)(dev + 
>> i)->eng_obj)->num_engines ; j++) {
>> +                eng = engine_ptr((struct xe_pmu_device*)(dev + 
>> i)->eng_obj, j);
>> +                if (eng->display_name)
>> +                    free(eng->display_name);
>> +
>> +                pmu = eng->engine_active_ticks;
>> +                if (pmu.present)
>> +                    close(pmu.fd);
>> +
>> +                pmu = eng->engine_total_ticks;
>> +                if (pmu.present)
>> +                    close(pmu.fd);
>> +            }
>> +            free(dev->eng_obj);
>> +        }
>> +        if ((dev + i)->pmu_device)
>> +            free(dev->pmu_device);
>> +    }
>> +}
>> +
>> +static char *pmu_name(struct igt_device_card *card)
>> +{
>> +    int card_fd;
>> +    char device[30];
>> +    char *path;
>> +
>> +    if (strlen(card->card))
>> +        card_fd = igt_open_card(card);
>> +    else if (strlen(card->render))
>> +        card_fd = igt_open_render(card);
>> +
>> +    if (card_fd == -1)
>> +        return NULL;
>> +
>> +    xe_perf_device(card_fd, device, sizeof(device));
>> +    path = strdup(device);
>> +    close(card_fd);
>> +    return path;
>> +}
>> +
>> +static int _open_pmu(uint64_t type, unsigned int *cnt, struct 
>> xe_pmu_counter *pmu, int *fd)
>> +{
>> +    int fd__ = igt_perf_open_group(type, pmu->config, *fd);
>> +
>> +    if (fd__ >= 0) {
>> +        if (*fd == -1)
>> +            *fd = fd__;
>> +        pmu->present = true;
>> +        pmu->idx = (*cnt)++;
>> +        pmu->fd = fd__;
>> +    }
>> +
>> +    return fd__;
>> +}
>> +
>> +void xe_gputop_init(void *ptr,
>> +            struct igt_device_card *card)
>> +{
>> +    struct xe_gputop *obj = (struct xe_gputop *)ptr;
>> +
>> +    obj->pmu_device = pmu_name(card);
>> +    if (!obj->pmu_device) {
>> +        fprintf(stderr, "%s : pmu_device path returned NULL", 
>> card->pci_slot_name);
>> +        exit(EXIT_FAILURE);
>> +    }
>> +    obj->card = card;
>> +}
>> +
>> +static int pmu_format_shift(int xe, const char *name)
>> +{
>> +    uint32_t start;
>> +    int format;
>> +    char device[80];
>> +
>> +    format = perf_event_format(xe_perf_device(xe, device, 
>> sizeof(device)),
>> +                   name, &start);
>> +    if (format)
>> +        return 0;
>> +
>> +    return start;
>> +}
>> +
>> +static int engine_cmp(const void *__a, const void *__b)
>> +{
>> +    const struct xe_engine *a = (struct xe_engine *)__a;
>> +    const struct xe_engine *b = (struct xe_engine *)__b;
>> +
>> +    if (a->drm_xe_engine.engine_class != b->drm_xe_engine.engine_class)
>> +        return a->drm_xe_engine.engine_class - 
>> b->drm_xe_engine.engine_class;
>> +    else
>> +        return a->drm_xe_engine.engine_instance - 
>> b->drm_xe_engine.engine_instance;
>> +}
>> +
>> +void *xe_populate_engines(const void *obj)
>> +{
>> +    struct igt_device_card *card = ((struct xe_gputop *)obj)->card;
>> +    struct xe_pmu_device *engines;
>> +    int ret = 0;
>> +    char device[30];
>> +    struct drm_xe_engine_class_instance *hwe;
>> +    int card_fd;
>> +    uint64_t engine_class, engine_instance, gt_shift;
>> +    uint64_t engine_active_config, engine_total_config;
>
> Use inverted xmas tree
>
>> +
>> +    if (!card || !strlen(card->card) || !strlen(card->render))
>> +        return NULL;
>> +
>> +    if (strlen(card->card)) {
>> +        card_fd = igt_open_card(card);
>> +    } else if (strlen(card->render)) {
>> +        card_fd = igt_open_render(card);
>> +    } else {
>> +        fprintf(stderr, "Failed to detect device!\n");
>> +        return NULL;
>> +    }
>> +    xe_device_get(card_fd);
>> +    engines = malloc(sizeof(struct xe_pmu_device) +
>> +             xe_number_engines(card_fd) * sizeof(struct xe_engine));
>> +    if (!engines)
>> +        return NULL;
>> +
>> +    memset(engines, 0, sizeof(struct xe_pmu_device) +
>> +           xe_number_engines(card_fd) * sizeof(struct xe_engine));
>> +
>> +    engines->num_engines = 0;
>> +    engines->device = ((struct xe_gputop *)obj)->pmu_device;
>> +    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));
>> +    ret = perf_event_config(device,
>> +                "engine-active-ticks",
>> +                &engine_active_config);
>
> wrap around at 100
>
>> +    if (ret < 0)
>> +        return NULL;
>> +    ret = perf_event_config(device,
>> +                "engine-total-ticks",
>> +                &engine_total_config);
>> +    if (ret < 0)
>> +        return NULL;
>
> add blank line
>
>> +    xe_for_each_engine(card_fd, hwe) {
>> +        uint64_t  param_config;
>> +        struct xe_engine *engine;
>> +
>> +        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;
>> +        engine->engine_active_ticks.config = engine_active_config | 
>> param_config;
>> +        engine->engine_total_ticks.config = engine_total_config | 
>> param_config;
>> +
>> +        if (engine->engine_active_ticks.config == -1 ||
>> +            engine->engine_total_ticks.config == -1) {
>> +            ret = ENOENT;
>> +            break;
>> +        }
>> +
>> +        ret = asprintf(&engine->display_name, "%s/%u",
>> + class_display_name(engine->drm_xe_engine.engine_class),
>> +                   engine->drm_xe_engine.engine_instance);
>> +
>> +        if (ret <= 0) {
>> +            ret = errno;
>> +            break;
>> +        }
>> +
>> +        engines->num_engines++;
>> +    }
>> +
>> +    if (!ret) {
>> +        errno = ret;
>> +        return NULL;
>> +    }
>> +
>> +    qsort(engine_ptr(engines, 0), engines->num_engines,
>> +          sizeof(struct xe_engine), engine_cmp);
>> +
>> +    ((struct xe_gputop *)obj)->eng_obj = engines;
>> +
>> +    return engines;
>> +}
>> +
>> +static uint64_t pmu_read_multi(int fd, unsigned int num, uint64_t *val)
>> +{
>> +    uint64_t buf[2 + num];
>> +    unsigned int i;
>> +    ssize_t len;
>> +
>> +    memset(buf, 0, sizeof(buf));
>> +
>> +    len = read(fd, buf, sizeof(buf));
>> +    assert(len == sizeof(buf));
>> +
>> +    for (i = 0; i < num; i++)
>> +        val[i] = buf[2 + i];
>> +
>> +    return buf[1];
>> +}
>> +
>> +void xe_pmu_sample(const void *obj)
>> +{
>> +    struct xe_pmu_device *engines = ((struct xe_gputop *)obj)->eng_obj;
>> +    const int num_val = engines->num_counters;
>> +    uint64_t val[2 + num_val];
>> +    unsigned int i;
>> +
>> +    pmu_read_multi(engines->fd, num_val, val);
>> +
>> +    for (i = 0; i < engines->num_engines; i++) {
>> +        struct xe_engine *engine = engine_ptr(engines, i);
>> +
>> +        update_sample(&engine->engine_active_ticks, val);
>> +        update_sample(&engine->engine_total_ticks, val);
>> +    }
>> +}
>> +
>> +int xe_pmu_init(const void *obj)
>> +{
>> +    struct xe_pmu_device *engines = ((struct xe_gputop *)obj)->eng_obj;
>> +    unsigned int i;
>> +    int fd;
>> +    struct xe_engine *engine;
>> +    uint64_t type = igt_perf_type_id(engines->device);
>> +
>> +    engines->fd = -1;
>> +    engines->num_counters = 0;
>> +
>> +    for (i = 0; i < engines->num_engines; i++) {
>> +        engine = engine_ptr(engines, i);
>> +        fd = _open_pmu(type, &engines->num_counters, 
>> &engine->engine_active_ticks,
>> +                   &engines->fd);
>> +        if (fd < 0)
>> +            return -1;
>> +        fd = _open_pmu(type, &engines->num_counters, 
>> &engine->engine_total_ticks,
>> +                   &engines->fd);
>> +        if (fd < 0)
>> +            return -1;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static double pmu_active_percentage(struct xe_engine *engine)
>> +{
>> +    double pmu_active_ticks = engine->engine_active_ticks.val.cur -
>> +                  engine->engine_active_ticks.val.prev;
>> +    double pmu_total_ticks = engine->engine_total_ticks.val.cur -
>> +                 engine->engine_total_ticks.val.prev;
>> +    double percentage;
>> +
>> +    percentage = (pmu_active_ticks * 100) / pmu_total_ticks;
>> +    return percentage;
>> +}
>> +
>> +static int
>> +print_device_description(const void *obj, int lines, int w, int h)
>> +{
>> +    char *desc;
>> +    int len;
>> +
>> +    len = asprintf(&desc, "DRIVER: %s || BDF: %s",
>> +               ((struct xe_gputop *)obj)->card->driver,
>> +               ((struct xe_gputop *)obj)->card->pci_slot_name);
>> +
>> +    printf("\033[7m%s%*s\033[0m\n",
>> +           desc,
>> +           (int)(w - len), " ");
>> +    lines++;
>> +    free(desc);
>> +    return lines;
>> +}
>> +
>> +static int
>> +print_engines_header(struct xe_pmu_device *engines,
>> +             int lines, int con_w, int con_h)
>> +{
>> +    const char *a;
>> +
>> +    for (unsigned int i = 0;
>> +         i < engines->num_engines && lines < con_h;
>> +         i++) {
>> +        struct xe_engine *engine = engine_ptr(engines, i);
>> +
>> +        if (!engine->num_counters)
>> +            continue;
>> +
>> +        a = "            ENGINES   ACTIVITY  ";
>> +
>> +        printf("\033[7m%s%*s\033[0m\n",
>> +               a,
>> +               (int)(con_w - strlen(a)), " ");
>> +        lines++;
>> +
>> +        break;
>> +    }
>> +
>> +    return lines;
>> +}
>> +
>> +static int
>> +print_engine(struct xe_pmu_device *engines, unsigned int i,
>> +         int lines, int con_w, int con_h)
>> +{
>> +    struct xe_engine *engine = engine_ptr(engines, i);
>> +    double percentage = pmu_active_percentage(engine);
>> +
>> +    printf("%*s", (int)(strlen("            ENGINES")), 
>> engine->display_name);
>> +    print_percentage_bar(percentage, con_w - strlen("            
>> ENGINES"));
>> +    printf("\n");
>> +
>> +    return ++lines;
>> +}
>> +
>> +int xe_print_engines(const void *obj, int lines, int w, int h)
>> +{
>> +    struct xe_pmu_device *show = ((struct xe_gputop *)obj)->eng_obj;
>> +
>> +    lines = print_device_description(obj, lines, w, h);
>> +
>> +    lines = print_engines_header(show, lines, w,  h);
>> +
>> +    for (unsigned int i = 0; i < show->num_engines && lines < h; i++)
>> +        lines = print_engine(show, i, lines, w, h);
>> +
>> +    lines = print_engines_footer(lines, w, h);
>> +
>> +    return lines;
>> +}
>> +
>> diff --git a/tools/gputop/xe_gputop.h b/tools/gputop/xe_gputop.h
>> new file mode 100644
>> index 000000000..825ac7e34
>> --- /dev/null
>> +++ b/tools/gputop/xe_gputop.h
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef __XE_GPUTOP_H__
>> +#define __XE_GPUTOP_H__
>> +
>> +#include <dirent.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;
>> +    uint64_t prev;
>> +};
>
> Is an extra struct needed here. you can add it in below struct

Not technically but imo it helps in better readability.

Thanks,
Soham

>
>> +
>> +struct xe_pmu_counter {
>> +    uint64_t type;
>> +    uint64_t config;
>> +    unsigned int idx;
>> +    struct xe_pmu_pair val;
>> +    int fd;
>> +    bool present;
>> +};
>> +
>> +struct xe_engine {
>> +    const char *name;
>> +    char *display_name;
>> +    struct drm_xe_engine_class_instance drm_xe_engine;
>
> %s/drm_xe_engine/engine
>
>> +    unsigned int num_counters;
>> +    struct xe_pmu_counter engine_active_ticks;
>> +    struct xe_pmu_counter engine_total_ticks;
>> +};
>> +
>> +struct xe_pmu_device {
>> +    unsigned int num_engines;
>> +    unsigned int num_counters;
>> +    int fd;
>> +    char *device;
>> +    struct xe_engine engine;
>> +};
>> +
>> +struct xe_gputop {
>> +    char *pmu_device;
>> +    struct igt_device_card *card;
>> +    struct xe_pmu_device *eng_obj;
>> +};
>> +
>> +void xe_gputop_init(void *ptr,
>> +            struct igt_device_card *card);
>
> Wrap around at 100
>
> Thanks
> Riana
>
>> +void xe_populate_device_instances(struct gputop_device *dv);
>> +void *xe_populate_engines(const void *obj);
>> +void xe_pmu_sample(const void *obj);
>> +int xe_pmu_init(const void *obj);
>> +int xe_print_engines(const void *obj, int lines, int w, int h);
>> +void xe_clean_up(void *obj, int len);
>> +
>> +#endif /* __XE_GPUTOP_H__ */
>