[PATCH i-g-t v2 0/4] tools/gputop: Add PMU stats

[PATCH i-g-t v2 0/4] tools/gputop: Add PMU stats

[Vinay Belgaumkar]

Use the PMU support being added in
https://patchwork.freedesktop.org/series/139121/ to add freq/c6 stats.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

Vinay Belgaumkar (4):
  tools/gputop: Define data structs for PMU stats
  lib/igt_drm_clients: Add pdev and driver info
  lib/igt_perf: Add utils to extract PMU event info
  tools/gputop: Add GT freq and c6 stats

 lib/igt_drm_clients.c |   6 ++
 lib/igt_drm_clients.h |   4 +
 lib/igt_perf.c        |  68 +++++++++++++
 lib/igt_perf.h        |   2 +
 tools/gputop.c        | 225 ++++++++++++++++++++++++++++++++++++++++++
 tools/meson.build     |   2 +-
 6 files changed, 306 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCH i-g-t v2 1/4] tools/gputop: Define data structs for PMU stats

[Vinay Belgaumkar]

Prep work for adding PMU support in gputop.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
---
 tools/gputop.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/gputop.c b/tools/gputop.c
index 43b01f566..4e3663417 100644
--- a/tools/gputop.c
+++ b/tools/gputop.c
@@ -42,6 +42,34 @@ static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "
 #define ANSI_HEADER "\033[7m"
 #define ANSI_RESET "\033[0m"
 
+struct pmu_pair {
+	uint64_t cur;
+	uint64_t prev;
+};
+
+struct pmu_counter {
+	uint64_t type;
+	uint64_t config;
+	unsigned int idx;
+	struct pmu_pair val;
+	double scale;
+	const char *units;
+	bool present;
+};
+
+#define MAX_GTS 4
+
+struct pmu_info {
+	char device_events_path[300];
+	int pmu_fd;
+	struct pmu_counter req_freq[MAX_GTS];
+	struct pmu_counter act_freq[MAX_GTS];
+	struct pmu_counter c6[MAX_GTS];
+	int num_gts;
+	int num_counters;
+	uint64_t ts_cur, ts_prev;
+};
+
 static void n_spaces(const unsigned int n)
 {
 	unsigned int i;
-- 
2.38.1

[PATCH i-g-t v2 2/4] lib/igt_drm_clients: Add pdev and driver info

[Vinay Belgaumkar]

This will be used for gathering PMU event information.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

This will be used for gathering PMU event information.

Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
/igt_debugfs.c.orig
---
 lib/igt_drm_clients.c | 6 ++++++
 lib/igt_drm_clients.h | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index 858cd3645..e30e3243e 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -190,6 +190,12 @@ igt_drm_client_add(struct igt_drm_clients *clients,
 	c->drm_minor = drm_minor;
 	c->clients = clients;
 
+	/* Save driver and pdev info for later use */
+	if (info->driver && info->pdev) {
+		strncpy(c->driver, info->driver, sizeof(c->driver));
+		strncpy(c->pdev, info->pdev, sizeof(c->pdev));
+	}
+
 	/* Engines */
 	c->engines = calloc(1, sizeof(*c->engines));
 	assert(c->engines);
diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
index 946d709de..6d2a48997 100644
--- a/lib/igt_drm_clients.h
+++ b/lib/igt_drm_clients.h
@@ -86,6 +86,10 @@ struct igt_drm_client {
 		uint64_t last_total_cycles; /* Engine total cycles data as parsed from fdinfo. */
 	} *utilization; /* Array of engine utilization */
 
+	void *pmu_info; /* Pointer to structure that holds client PMU info */
+	char driver[128];
+	char pdev[128];
+
 	struct drm_client_meminfo *memory; /* Array of region memory utilisation as parsed from fdinfo. */
 };
 
-- 
2.38.1

[PATCH i-g-t v2 3/4] lib/igt_perf: Add utils to extract PMU event info

[Vinay Belgaumkar]

Functions to parse config ID and GT bit shift for PMU events.

Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
---
 lib/igt_perf.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_perf.h |  2 ++
 2 files changed, 70 insertions(+)

diff --git a/lib/igt_perf.c b/lib/igt_perf.c
index 3866c6d77..fec07b2d3 100644
--- a/lib/igt_perf.c
+++ b/lib/igt_perf.c
@@ -92,6 +92,74 @@ const char *xe_perf_device(int xe, char *buf, int buflen)
 	return buf;
 }
 
+/**
+ * perf_xe_format_gt_id:
+ * @device: Device string in driver:pci format
+ * Returns: The number of bits GT id is shifted in config
+ *
+ */
+int perf_xe_format_gt_shift(const char *device)
+{
+	char buf[150];
+	ssize_t ret;
+	int fd, start, end;
+
+	snprintf(buf, sizeof(buf),
+		 "/sys/bus/event_source/devices/%s/format/gt_id",
+		 device);
+
+	fd = open(buf, O_RDONLY);
+	if (fd < 0)
+		return -EINVAL;
+
+	ret = read(fd, buf, sizeof(buf) - 1);
+	close(fd);
+	if (ret < 1)
+		return ret;
+
+	buf[ret] = '\0';
+	ret = sscanf(buf, "config:%d-%d", &start, &end);
+	if (ret != 2)
+		return -EINVAL;
+
+	return start;
+}
+
+/**
+ * perf_xe_event_config:
+ * @device: Device string in driver:pci format
+ * @event: The event name
+ * @config: Pointer to the config
+ * Returns: 0 for success, negative value on error
+ */
+int perf_xe_event_config(const char *device, const char *event, uint64_t *config)
+{
+	char buf[150];
+	ssize_t ret;
+	int fd;
+
+	snprintf(buf, sizeof(buf),
+		 "/sys/bus/event_source/devices/%s/events/%s",
+		 device,
+		 event);
+
+	fd = open(buf, O_RDONLY);
+	if (fd < 0)
+		return -EINVAL;
+
+	ret = read(fd, buf, sizeof(buf) - 1);
+	close(fd);
+	if (ret < 1)
+		return ret;
+
+	buf[ret] = '\0';
+	ret = sscanf(buf, "config=0x%lx", config);
+	if (ret != 1)
+		return -EINVAL;
+
+	return 0;
+}
+
 uint64_t xe_perf_type_id(int xe)
 {
 	char buf[80];
diff --git a/lib/igt_perf.h b/lib/igt_perf.h
index 3d9ba2917..2505e7a45 100644
--- a/lib/igt_perf.h
+++ b/lib/igt_perf.h
@@ -71,5 +71,7 @@ int perf_i915_open(int i915, uint64_t config);
 int perf_i915_open_group(int i915, uint64_t config, int group);
 
 int perf_xe_open(int xe, uint64_t config);
+int perf_xe_event_config(const char *device, const char *event, uint64_t *config);
+int perf_xe_format_gt_shift(const char *device);
 
 #endif /* I915_PERF_H */
-- 
2.38.1

[PATCH i-g-t v2 4/4] tools/gputop: Add GT freq and c6 stats

[Vinay Belgaumkar]

Add GT C6 and Frequency support. These will use the PMU interface
and are displayed per GT/device in the header.

GT: 0, c6:  94.54% req_freq:  750.63 MHz act_freq:    0.00 MHz
GT: 1, c6:   2.75% req_freq: 1200.71 MHz act_freq: 1112.66 MHz

v2: Split patch into logical units and other review
comments (Rodrigo, Kamil)

Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
---
 tools/gputop.c    | 197 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/meson.build |   2 +-
 2 files changed, 198 insertions(+), 1 deletion(-)

diff --git a/tools/gputop.c b/tools/gputop.c
index 4e3663417..df038bdbb 100644
--- a/tools/gputop.c
+++ b/tools/gputop.c
@@ -29,6 +29,7 @@
 #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"
 
@@ -104,6 +105,200 @@ static void print_percentage_bar(double percent, int max_len)
 	putchar('|');
 }
 
+static int
+get_num_gts(uint64_t type, uint64_t config, int gt_shift)
+{
+	int fd, gt_id;
+
+	errno = 0;
+	for (gt_id = 0; gt_id < MAX_GTS; gt_id++) {
+		config |= (uint64_t)gt_id << gt_shift;
+		fd = igt_perf_open(type, config);
+		if (fd < 0)
+			break;
+		close(fd);
+	}
+
+	if (!gt_id || (errno && errno != ENOENT))
+		gt_id = -errno;
+
+	return gt_id;
+}
+
+
+#define _open_pmu(type, cnt, pmu, fd) \
+({ \
+	int fd__; \
+\
+	fd__ = igt_perf_open_group((type), (pmu)->config, (fd)); \
+	if (fd__ >= 0) { \
+		if ((fd) == -1) \
+			(fd) = fd__; \
+		(pmu)->present = true; \
+		(pmu)->idx = (cnt)++; \
+	} \
+\
+	fd__; \
+})
+
+static int pmu_init(struct igt_drm_client *c)
+{
+	struct pmu_info *info;
+	unsigned int i, num_cntr = 0;
+	int fd = -1, ret;
+	/* TODO  get device events path*/
+	char *path;
+	uint64_t type;
+	uint64_t config;
+	int gt_shift;
+	char event_str[100];
+
+	info = (struct pmu_info *)malloc(sizeof(struct pmu_info));
+	if (!info)
+		return -ENOMEM;
+
+	snprintf(info->device_events_path, sizeof(info->device_events_path) - 1,
+		 "%s_%s", c->driver, c->pdev);
+	path = info->device_events_path;
+	for (; *path; ++path)
+		if (*path == ':')
+			*path = '_';
+
+	type = igt_perf_type_id(info->device_events_path);
+
+	/* Get a sample event config which can be used to find num_gts */
+	ret = perf_xe_event_config(info->device_events_path, "actual-frequency", &config);
+	if (ret < 0)
+		return -EINVAL;
+
+	gt_shift = perf_xe_format_gt_shift(info->device_events_path);
+	if (ret < 0)
+		return -EINVAL;
+
+	info->num_gts = get_num_gts(type, config, gt_shift);
+
+	for (i = 0; i < info->num_gts; i++) {
+		snprintf(event_str, sizeof(event_str), "c6-residency");
+		ret = perf_xe_event_config(info->device_events_path, event_str,
+					   &info->c6[i].config);
+		assert(ret >= 0);
+		info->c6[i].config |= (uint64_t)i << gt_shift;
+		_open_pmu(type, num_cntr, &info->c6[i], fd);
+
+		snprintf(event_str, sizeof(event_str), "actual-frequency");
+		ret = perf_xe_event_config(info->device_events_path, event_str,
+					   &info->act_freq[i].config);
+		assert(ret >= 0);
+		info->act_freq[i].config |= (uint64_t)i << gt_shift;
+		_open_pmu(type, num_cntr, &info->act_freq[i], fd);
+
+		snprintf(event_str, sizeof(event_str), "requested-frequency");
+		ret = perf_xe_event_config(info->device_events_path, event_str,
+					   &info->req_freq[i].config);
+		assert(ret >= 0);
+		info->req_freq[i].config |= (uint64_t)i << gt_shift;
+		_open_pmu(type, num_cntr, &info->req_freq[i], fd);
+	}
+
+	if (fd < 0)
+		return -EINVAL;
+	info->pmu_fd = fd;
+	info->num_counters = num_cntr;
+
+	/* Save PMU info in the client */
+	c->pmu_info = info;
+
+	return 0;
+}
+
+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));
+	if (len != sizeof(buf))
+		return 0;
+
+	for (i = 0; i < num; i++)
+		val[i] = buf[2 + i];
+
+	return buf[1];
+}
+
+
+static void __update_sample(struct pmu_counter *counter, uint64_t val)
+{
+	counter->val.prev = counter->val.cur;
+	counter->val.cur = val;
+}
+
+static void update_sample(struct pmu_counter *counter, uint64_t *val)
+{
+	if (counter->present)
+		__update_sample(counter, val[counter->idx]);
+}
+
+static void
+calc_c6_pct(struct pmu_info *info, unsigned int gt, unsigned long t)
+{
+	unsigned long c6_diff = info->c6[gt].val.cur - info->c6[gt].val.prev;
+
+	printf("GT: %d, c6: %6.2lf%%", gt, 100 * (double)((1e6 * c6_diff) / (double)t));
+}
+
+static void
+calc_freq(struct pmu_info *info, uint8_t gt, uint64_t t)
+{
+	uint64_t req_freq_diff = 1e9 * (info->req_freq[gt].val.cur - info->req_freq[gt].val.prev);
+	uint64_t act_freq_diff = 1e9 * (info->act_freq[gt].val.cur - info->act_freq[gt].val.prev);
+
+	printf(" req_freq: %7.2lf MHz", (double)req_freq_diff / (double)t);
+	printf(" act_freq: %7.2lf MHz", (double)act_freq_diff / (double)t);
+}
+
+static void
+print_pmu_stats(struct igt_drm_client *c, int *lines)
+{
+	struct pmu_info *info;
+	int i;
+	uint64_t *val;
+	uint64_t ts_diff;
+
+	if (!c->pmu_info)
+		if (pmu_init(c))
+			return;
+
+	info = (struct pmu_info *)c->pmu_info;
+	if (info->num_counters <= 0)
+		return;
+
+	val = (uint64_t *)malloc(info->num_counters * sizeof(uint64_t));
+	if (!val)
+		return;
+
+	/* Calculate timestamp diffs */
+	info->ts_prev = info->ts_cur;
+	info->ts_cur = pmu_read_multi(info->pmu_fd, info->num_counters, val);
+	ts_diff = info->ts_cur - info->ts_prev;
+
+	for (i = 0; i < info->num_gts; i++) {
+		update_sample(&info->c6[i], val);
+		update_sample(&info->req_freq[i], val);
+		update_sample(&info->act_freq[i], val);
+		calc_c6_pct(info, i, ts_diff);
+		calc_freq(info, i, ts_diff);
+		putchar('\n');
+		(*lines)++;
+	}
+
+	if (val)
+		free(val);
+}
+
 static int
 print_client_header(struct igt_drm_client *c, int lines, int con_w, int con_h,
 		    int *engine_w)
@@ -120,6 +315,8 @@ print_client_header(struct igt_drm_client *c, int lines, int con_w, int con_h,
 	if (lines++ >= con_h)
 		return lines;
 
+	print_pmu_stats(c, &lines);
+
 	putchar('\n');
 	if (c->regions->num_regions)
 		len = printf("%*s      MEM      RSS ",
diff --git a/tools/meson.build b/tools/meson.build
index 38b04851c..9e6c8546a 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -70,7 +70,7 @@ endif
 executable('gputop', 'gputop.c',
            install : true,
            install_rpath : bindir_rpathdir,
-           dependencies : [lib_igt_drm_clients,lib_igt_drm_fdinfo,lib_igt_profiling,math])
+           dependencies : [lib_igt_perf,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,
-- 
2.38.1

Re: [PATCH i-g-t v2 0/4] tools/gputop: Add PMU stats

[Tvrtko Ursulin]

On 20/12/2024 00:37, Vinay Belgaumkar wrote:
> Use the PMU support being added in
> https://patchwork.freedesktop.org/series/139121/ to add freq/c6 stats.

Brace yourself for the customary "why". So yes, why?

Gputop so far was a reference showcase for DRM fdinfo, nothing more. Why 
add a _subset_ of PMU stats to it? Any other drivers could be wired up? 
How far do people intend to grow it, considering alternatives with nicer 
UI and more featureful exist?

Or in case the conclusion ends up being "yes", then lets at least share 
some more code between intel_gpu_top and this work. Ie. make it in a way 
gputop completely subsumes and replaces intel_gpu_top might be an idea.

Regards,

Tvrtko

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> 
> Vinay Belgaumkar (4):
>    tools/gputop: Define data structs for PMU stats
>    lib/igt_drm_clients: Add pdev and driver info
>    lib/igt_perf: Add utils to extract PMU event info
>    tools/gputop: Add GT freq and c6 stats
> 
>   lib/igt_drm_clients.c |   6 ++
>   lib/igt_drm_clients.h |   4 +
>   lib/igt_perf.c        |  68 +++++++++++++
>   lib/igt_perf.h        |   2 +
>   tools/gputop.c        | 225 ++++++++++++++++++++++++++++++++++++++++++
>   tools/meson.build     |   2 +-
>   6 files changed, 306 insertions(+), 1 deletion(-)
> 

Re: [PATCH i-g-t v2 0/4] tools/gputop: Add PMU stats

[Rodrigo Vivi]

On Fri, Dec 20, 2024 at 10:15:04AM +0000, Tvrtko Ursulin wrote:
> 
> On 20/12/2024 00:37, Vinay Belgaumkar wrote:
> > Use the PMU support being added in
> > https://patchwork.freedesktop.org/series/139121/ to add freq/c6 stats.
> 
> Brace yourself for the customary "why". So yes, why?
> 
> Gputop so far was a reference showcase for DRM fdinfo, nothing more. Why add
> a _subset_ of PMU stats to it? Any other drivers could be wired up? How far
> do people intend to grow it, considering alternatives with nicer UI and more
> featureful exist?

Well, currently intel_gpu_top doesn't support Xe and we really don't
have any intention to add xe support there.

We don't believe it is a better UI and more features.

Hopefully someday we can get qmassa [1] part of the distros and make that to
be our default to use the uapis that we add in Xe.

But for now we were in the hope that we could use gputop for that which is
the current tool that supports Xe metrics. But well, I just noticed that
at least in Fedora it is not packaged :/

$ rpm -ql igt-gpu-tools-1.27.1-0.4.20230215git45da871.fc40.x86_64 | grep top
/usr/bin/intel_gpu_top
/usr/share/man/man1/intel_gpu_top.1.gz

[1] - https://github.com/ulissesf/qmassa

So, our options are:

1. Add all the Xe support in intel_gpu_top
2. Convince distros to build and package gputop along with intel_gpu_top in igt
3. Convince distros to adopt qmaasa

Meanwhile our PMU are blocked...

Lucas, Thomas, thoughts?

> 
> Or in case the conclusion ends up being "yes", then lets at least share some
> more code between intel_gpu_top and this work. Ie. make it in a way gputop
> completely subsumes and replaces intel_gpu_top might be an idea.

with this I agree as well.

> 
> Regards,
> 
> Tvrtko
> 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > 
> > Vinay Belgaumkar (4):
> >    tools/gputop: Define data structs for PMU stats
> >    lib/igt_drm_clients: Add pdev and driver info
> >    lib/igt_perf: Add utils to extract PMU event info
> >    tools/gputop: Add GT freq and c6 stats
> > 
> >   lib/igt_drm_clients.c |   6 ++
> >   lib/igt_drm_clients.h |   4 +
> >   lib/igt_perf.c        |  68 +++++++++++++
> >   lib/igt_perf.h        |   2 +
> >   tools/gputop.c        | 225 ++++++++++++++++++++++++++++++++++++++++++
> >   tools/meson.build     |   2 +-
> >   6 files changed, 306 insertions(+), 1 deletion(-)
> > 

Re: [PATCH i-g-t v2 0/4] tools/gputop: Add PMU stats

[Lucas De Marchi]

On Fri, Dec 20, 2024 at 02:13:26PM -0500, Rodrigo Vivi wrote:
>On Fri, Dec 20, 2024 at 10:15:04AM +0000, Tvrtko Ursulin wrote:
>>
>> On 20/12/2024 00:37, Vinay Belgaumkar wrote:
>> > Use the PMU support being added in
>> > https://patchwork.freedesktop.org/series/139121/ to add freq/c6 stats.
>>
>> Brace yourself for the customary "why". So yes, why?
>>
>> Gputop so far was a reference showcase for DRM fdinfo, nothing more. Why add
>> a _subset_ of PMU stats to it? Any other drivers could be wired up? How far
>> do people intend to grow it, considering alternatives with nicer UI and more
>> featureful exist?
>
>Well, currently intel_gpu_top doesn't support Xe and we really don't
>have any intention to add xe support there.
>
>We don't believe it is a better UI and more features.
>
>Hopefully someday we can get qmassa [1] part of the distros and make that to
>be our default to use the uapis that we add in Xe.
>
>But for now we were in the hope that we could use gputop for that which is
>the current tool that supports Xe metrics. But well, I just noticed that
>at least in Fedora it is not packaged :/
>
>$ rpm -ql igt-gpu-tools-1.27.1-0.4.20230215git45da871.fc40.x86_64 | grep top
>/usr/bin/intel_gpu_top
>/usr/share/man/man1/intel_gpu_top.1.gz
>
>[1] - https://github.com/ulissesf/qmassa
>
>So, our options are:
>
>1. Add all the Xe support in intel_gpu_top
>2. Convince distros to build and package gputop along with intel_gpu_top in igt
>3. Convince distros to adopt qmaasa


I think we should handle gputop as a reference implementation for a
"top-like implementation for GPU".  I think end users have tools with
better UIs like qmassa, or nvtop, or htop or other graphical
applications and we shouldn't try to block that - doing something
beautiful in gputop would be a lot of effort for little benefit if end
users are already served by other tools.  Letting gputop as a reference
impl for these tools to borrow code or consult, would be ideal IMO.


As Tvrtko said, currently gputop is a reference showcase for DRM fdinfo.
I think it can grow some capabilities and be a reference
implementation for top-like application. If that means adding pmu, then
be it.  However the pmu support needs to be added in a proper way so
the tool always continue to be vendor-agnostic and that it's easy to
add support for events from other vendors. That probably means that
adding pmu-related things in the fdinfo or drm-client libs are probably
not the way to go as a) it's crossing unrelated abstraction and b)
breaking the vendor-agnostic nature of the tool.

>
>Meanwhile our PMU are blocked...

I don't think they should be blocked. The kernel impl was blocked for a
long time in other things, not having PMU included somewhere like
gputop.  If you want to read pmu, the #1 application is perf

I think we can add pmu in gputop as a reference. If someone can grow
gputop to have all the intel_gpu_top capabilities, but doing it in a
proper vendor-agnostic way, awesome. At that time we may then retire
intel_gpu_top and only use gputop as reference.

Lucas De Marchi

>
>Lucas, Thomas, thoughts?
>
>>
>> Or in case the conclusion ends up being "yes", then lets at least share some
>> more code between intel_gpu_top and this work. Ie. make it in a way gputop
>> completely subsumes and replaces intel_gpu_top might be an idea.
>
>with this I agree as well.
>
>>
>> Regards,
>>
>> Tvrtko
>>
>> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> >
>> > Vinay Belgaumkar (4):
>> >    tools/gputop: Define data structs for PMU stats
>> >    lib/igt_drm_clients: Add pdev and driver info
>> >    lib/igt_perf: Add utils to extract PMU event info
>> >    tools/gputop: Add GT freq and c6 stats
>> >
>> >   lib/igt_drm_clients.c |   6 ++
>> >   lib/igt_drm_clients.h |   4 +
>> >   lib/igt_perf.c        |  68 +++++++++++++
>> >   lib/igt_perf.h        |   2 +
>> >   tools/gputop.c        | 225 ++++++++++++++++++++++++++++++++++++++++++
>> >   tools/meson.build     |   2 +-
>> >   6 files changed, 306 insertions(+), 1 deletion(-)
>> >

Re: [PATCH i-g-t v2 0/4] tools/gputop: Add PMU stats

[Tvrtko Ursulin]

On 20/12/2024 19:32, Lucas De Marchi wrote:
> On Fri, Dec 20, 2024 at 02:13:26PM -0500, Rodrigo Vivi wrote:
>> On Fri, Dec 20, 2024 at 10:15:04AM +0000, Tvrtko Ursulin wrote:
>>>
>>> On 20/12/2024 00:37, Vinay Belgaumkar wrote:
>>> > Use the PMU support being added in
>>> > https://patchwork.freedesktop.org/series/139121/ to add freq/c6 stats.
>>>
>>> Brace yourself for the customary "why". So yes, why?
>>>
>>> Gputop so far was a reference showcase for DRM fdinfo, nothing more. 
>>> Why add
>>> a _subset_ of PMU stats to it? Any other drivers could be wired up? 
>>> How far
>>> do people intend to grow it, considering alternatives with nicer UI 
>>> and more
>>> featureful exist?
>>
>> Well, currently intel_gpu_top doesn't support Xe and we really don't
>> have any intention to add xe support there.
>>
>> We don't believe it is a better UI and more features.

Hm what? :) With "nicer UI and more featureful" I was referring to the 
alternatives Lucas later listed. All are nicer than intel_gpu_top, not 
to mention than gputop.

>> Hopefully someday we can get qmassa [1] part of the distros and make 
>> that to
>> be our default to use the uapis that we add in Xe.
>>
>> But for now we were in the hope that we could use gputop for that 
>> which is
>> the current tool that supports Xe metrics. But well, I just noticed that
>> at least in Fedora it is not packaged :/
>>
>> $ rpm -ql igt-gpu-tools-1.27.1-0.4.20230215git45da871.fc40.x86_64 | 
>> grep top
>> /usr/bin/intel_gpu_top
>> /usr/share/man/man1/intel_gpu_top.1.gz
>>
>> [1] - https://github.com/ulissesf/qmassa
>>
>> So, our options are:
>>
>> 1. Add all the Xe support in intel_gpu_top
>> 2. Convince distros to build and package gputop along with 
>> intel_gpu_top in igt
>> 3. Convince distros to adopt qmaasa
> 
> 
> I think we should handle gputop as a reference implementation for a
> "top-like implementation for GPU".  I think end users have tools with
> better UIs like qmassa, or nvtop, or htop or other graphical
> applications and we shouldn't try to block that - doing something
> beautiful in gputop would be a lot of effort for little benefit if end
> users are already served by other tools.  Letting gputop as a reference
> impl for these tools to borrow code or consult, would be ideal IMO.
> 
> 
> As Tvrtko said, currently gputop is a reference showcase for DRM fdinfo.
> I think it can grow some capabilities and be a reference
> implementation for top-like application. If that means adding pmu, then
> be it.  However the pmu support needs to be added in a proper way so
> the tool always continue to be vendor-agnostic and that it's easy to
> add support for events from other vendors. That probably means that
> adding pmu-related things in the fdinfo or drm-client libs are probably
> not the way to go as a) it's crossing unrelated abstraction and b)
> breaking the vendor-agnostic nature of the tool.

100%.

>>
>> Meanwhile our PMU are blocked...
> 
> I don't think they should be blocked. The kernel impl was blocked for a
> long time in other things, not having PMU included somewhere like
> gputop.  If you want to read pmu, the #1 application is perf

Yes, perf was always enough and should still be.

> I think we can add pmu in gputop as a reference. If someone can grow
> gputop to have all the intel_gpu_top capabilities, but doing it in a
> proper vendor-agnostic way, awesome. At that time we may then retire
> intel_gpu_top and only use gputop as reference.

My 2p is that adding PMU support to gputop only makes sense if it will 
work with more than only xe and if people kind of see that would make 
having two top-like tools in IGT not great at that point. And that the 
road to completely subsume intel_gpu_top in gputop is not a long one 
once the first part is done properly.

I wasn't arguing for some nice UI if that was misunderstood somehow. 
(Don't know how.)

It may even come naturally because many of the nicer features of 
intel_gpu_top came by user demand. So assuming some PMU support gets 
added to xe users then might ask - "Hey can we have JSON/CSV output 
modes so we can use it like we used intel_gpu_top?". Or any other 
feature like sort modes or whatever.

Regards,

Tvrtko

> 
> Lucas De Marchi
> 
>>
>> Lucas, Thomas, thoughts?
>>
>>>
>>> Or in case the conclusion ends up being "yes", then lets at least 
>>> share some
>>> more code between intel_gpu_top and this work. Ie. make it in a way 
>>> gputop
>>> completely subsumes and replaces intel_gpu_top might be an idea.
>>
>> with this I agree as well.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>>> >
>>> > Vinay Belgaumkar (4):
>>> >    tools/gputop: Define data structs for PMU stats
>>> >    lib/igt_drm_clients: Add pdev and driver info
>>> >    lib/igt_perf: Add utils to extract PMU event info
>>> >    tools/gputop: Add GT freq and c6 stats
>>> >
>>> >   lib/igt_drm_clients.c |   6 ++
>>> >   lib/igt_drm_clients.h |   4 +
>>> >   lib/igt_perf.c        |  68 +++++++++++++
>>> >   lib/igt_perf.h        |   2 +
>>> >   tools/gputop.c        | 225 
>>> ++++++++++++++++++++++++++++++++++++++++++
>>> >   tools/meson.build     |   2 +-
>>> >   6 files changed, 306 insertions(+), 1 deletion(-)
>>> >

Re: [PATCH i-g-t v2 0/4] tools/gputop: Add PMU stats

[Belgaumkar, Vinay]

On 12/20/2024 11:32 AM, Lucas De Marchi wrote:
> On Fri, Dec 20, 2024 at 02:13:26PM -0500, Rodrigo Vivi wrote:
>> On Fri, Dec 20, 2024 at 10:15:04AM +0000, Tvrtko Ursulin wrote:
>>>
>>> On 20/12/2024 00:37, Vinay Belgaumkar wrote:
>>> > Use the PMU support being added in
>>> > https://patchwork.freedesktop.org/series/139121/ to add freq/c6 
>>> stats.
>>>
>>> Brace yourself for the customary "why". So yes, why?
>>>
>>> Gputop so far was a reference showcase for DRM fdinfo, nothing more. 
>>> Why add
>>> a _subset_ of PMU stats to it? Any other drivers could be wired up? 
>>> How far
>>> do people intend to grow it, considering alternatives with nicer UI 
>>> and more
>>> featureful exist?
>>
>> Well, currently intel_gpu_top doesn't support Xe and we really don't
>> have any intention to add xe support there.
>>
>> We don't believe it is a better UI and more features.
>>
>> Hopefully someday we can get qmassa [1] part of the distros and make 
>> that to
>> be our default to use the uapis that we add in Xe.
>>
>> But for now we were in the hope that we could use gputop for that 
>> which is
>> the current tool that supports Xe metrics. But well, I just noticed that
>> at least in Fedora it is not packaged :/
>>
>> $ rpm -ql igt-gpu-tools-1.27.1-0.4.20230215git45da871.fc40.x86_64 | 
>> grep top
>> /usr/bin/intel_gpu_top
>> /usr/share/man/man1/intel_gpu_top.1.gz
>>
>> [1] - https://github.com/ulissesf/qmassa
>>
>> So, our options are:
>>
>> 1. Add all the Xe support in intel_gpu_top
>> 2. Convince distros to build and package gputop along with 
>> intel_gpu_top in igt
>> 3. Convince distros to adopt qmaasa
>
>
> I think we should handle gputop as a reference implementation for a
> "top-like implementation for GPU".  I think end users have tools with
> better UIs like qmassa, or nvtop, or htop or other graphical
> applications and we shouldn't try to block that - doing something
> beautiful in gputop would be a lot of effort for little benefit if end
> users are already served by other tools.  Letting gputop as a reference
> impl for these tools to borrow code or consult, would be ideal IMO.
>
>
> As Tvrtko said, currently gputop is a reference showcase for DRM fdinfo.
> I think it can grow some capabilities and be a reference
> implementation for top-like application. If that means adding pmu, then
> be it.  However the pmu support needs to be added in a proper way so
> the tool always continue to be vendor-agnostic and that it's easy to
> add support for events from other vendors. That probably means that
> adding pmu-related things in the fdinfo or drm-client libs are probably
> not the way to go as a) it's crossing unrelated abstraction and b)
> breaking the vendor-agnostic nature of the tool.

The pmu_info added to drm_client in these patches is just a void pointer 
to the info. So, if a client doesn't support it, the freq/c6 stuff will 
not get displayed. Tool should still work in that case.

Thanks,

Vinay.

>
>>
>> Meanwhile our PMU are blocked...
>
> I don't think they should be blocked. The kernel impl was blocked for a
> long time in other things, not having PMU included somewhere like
> gputop.  If you want to read pmu, the #1 application is perf
>
> I think we can add pmu in gputop as a reference. If someone can grow
> gputop to have all the intel_gpu_top capabilities, but doing it in a
> proper vendor-agnostic way, awesome. At that time we may then retire
> intel_gpu_top and only use gputop as reference.
>
> Lucas De Marchi
>
>>
>> Lucas, Thomas, thoughts?
>>
>>>
>>> Or in case the conclusion ends up being "yes", then lets at least 
>>> share some
>>> more code between intel_gpu_top and this work. Ie. make it in a way 
>>> gputop
>>> completely subsumes and replaces intel_gpu_top might be an idea.
>>
>> with this I agree as well.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>>> >
>>> > Vinay Belgaumkar (4):
>>> >    tools/gputop: Define data structs for PMU stats
>>> >    lib/igt_drm_clients: Add pdev and driver info
>>> >    lib/igt_perf: Add utils to extract PMU event info
>>> >    tools/gputop: Add GT freq and c6 stats
>>> >
>>> >   lib/igt_drm_clients.c |   6 ++
>>> >   lib/igt_drm_clients.h |   4 +
>>> >   lib/igt_perf.c        |  68 +++++++++++++
>>> >   lib/igt_perf.h        |   2 +
>>> >   tools/gputop.c        | 225 
>>> ++++++++++++++++++++++++++++++++++++++++++
>>> >   tools/meson.build     |   2 +-
>>> >   6 files changed, 306 insertions(+), 1 deletion(-)
>>> >