intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc
@ 2023-09-22 13:44 Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 01/12] tests/i915/drm_fdinfo: Stress test context close versus fdinfo reads Tvrtko Ursulin
                   ` (11 more replies)
  0 siblings, 12 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Some basic testst for fdinfo memory stats, intel_gpu_top memory stats support
(first draft) and a couple fixlets.

Primarily sending to use as "Test-with" but review is also welcome.

Tvrtko Ursulin (12):
  tests/i915/drm_fdinfo: Stress test context close versus fdinfo reads
  tests/i915/drm_fdinfo: Add some memory info tests
  tools/intel_gpu_top: Restore user friendly error message
  tools/intel_gpu_top: Fix clients header width when no clients
  tools/intel_gpu_top: Fix client layout on first sample period
  tools/intel_gpu_top: Optimise interactive display a bit
  lib/igt_drm_fdinfo: Copy over region map name on match
  lib/igt_drm_clients: Fix client id type confusion
  lib/igt_drm_clients: Allow passing in the memory region map
  tools/intel_gpu_top: Fully wrap clients operations
  tools/intel_gpu_top: Add per client memory info
  tools/intel_gpu_top: Add ability to show memory region breakdown

 lib/igt_drm_clients.c    |   7 +-
 lib/igt_drm_clients.h    |   5 +-
 lib/igt_drm_fdinfo.c     |   4 +
 man/intel_gpu_top.rst    |   4 +
 tests/intel/drm_fdinfo.c | 269 +++++++++++++++++++++++++++++++++++++++
 tools/gputop.c           |   4 +-
 tools/intel_gpu_top.c    | 260 +++++++++++++++++++++++++++++++------
 7 files changed, 508 insertions(+), 45 deletions(-)

-- 
2.39.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 01/12] tests/i915/drm_fdinfo: Stress test context close versus fdinfo reads
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 02/12] tests/i915/drm_fdinfo: Add some memory info tests Tvrtko Ursulin
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A short smoke tests to exercise fdinfo reads in parallel to contexts
getting created and destroyed.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/intel/drm_fdinfo.c | 68 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/tests/intel/drm_fdinfo.c b/tests/intel/drm_fdinfo.c
index aca19db50680..a9910900358d 100644
--- a/tests/intel/drm_fdinfo.c
+++ b/tests/intel/drm_fdinfo.c
@@ -22,11 +22,14 @@
  *
  */
 
+#include <fcntl.h>
+
 #include "igt.h"
 #include "igt_core.h"
 #include "igt_device.h"
 #include "igt_drm_fdinfo.h"
 #include "i915/gem.h"
+#include "i915/gem_create.h"
 #include "i915/gem_vm.h"
 #include "intel_ctx.h"
 /**
@@ -72,6 +75,8 @@
  * SUBTEST: virtual-busy-idle-all
  *
  * SUBTEST: virtual-idle
+ *
+ * SUBTEST: context-close-stress
  */
 
 IGT_TEST_DESCRIPTION("Test the i915 drm fdinfo data");
@@ -717,6 +722,56 @@ virtual_all(int i915, const intel_ctx_cfg_t *base_cfg, unsigned int flags)
 	}
 }
 
+static void stress_context_close(int i915)
+{
+	const uint32_t bbe = MI_BATCH_BUFFER_END;
+	struct igt_helper_process reader = { };
+	struct drm_client_fdinfo info;
+	uint32_t batch;
+	int dir, ret;
+	char buf[64];
+
+	ret = snprintf(buf, sizeof(buf), "%u", i915);
+	igt_assert(ret > 0 && ret < sizeof(buf));
+
+	dir = open("/proc/self/fdinfo", O_DIRECTORY | O_RDONLY);
+	igt_assert_fd(dir);
+
+	memset(&info, 0, sizeof(info));
+	ret = __igt_parse_drm_fdinfo(dir, buf, &info, NULL, 0, NULL, 0);
+	igt_assert(ret > 0);
+	igt_require(info.num_regions);
+
+	batch = gem_create(i915, 4096);
+	gem_write(i915, batch, 0, &bbe, sizeof(bbe));
+
+	igt_fork_helper(&reader) {
+		for (;;) {
+			memset(&info, 0, sizeof(info));
+			ret = __igt_parse_drm_fdinfo(dir, buf, &info,
+						     NULL, 0, NULL, 0);
+			igt_assert(ret > 0);
+		}
+	}
+
+	igt_until_timeout(10) {
+		struct drm_i915_gem_exec_object2 obj = {
+			.handle = batch,
+		};
+		struct drm_i915_gem_execbuffer2 eb = {
+			.buffers_ptr = to_user_pointer(&obj),
+			.buffer_count = 1,
+		};
+
+		eb.rsvd1 = gem_context_create(i915);
+		igt_assert(eb.rsvd1);
+		gem_execbuf(i915, &eb);
+		gem_context_destroy(i915, eb.rsvd1);
+	}
+
+	igt_stop_helper(&reader);
+}
+
 #define test_each_engine(T, i915, ctx, e) \
 	igt_subtest_with_dynamic(T) for_each_ctx_engine(i915, ctx, e) \
 		igt_dynamic_f("%s", e->name)
@@ -847,6 +902,19 @@ igt_main
 	test_each_engine("isolation", i915, ctx, e)
 		single(i915, ctx, e, TEST_BUSY | TEST_ISOLATION);
 
+	igt_subtest_group {
+		int newfd;
+
+		igt_fixture
+			newfd = drm_reopen_driver(i915);
+
+		igt_subtest("context-close-stress")
+			stress_context_close(newfd);
+
+		igt_fixture
+			drm_close_driver(newfd);
+	}
+
 	igt_fixture {
 		intel_ctx_destroy(i915, ctx);
 		drm_close_driver(i915);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 02/12] tests/i915/drm_fdinfo: Add some memory info tests
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 01/12] tests/i915/drm_fdinfo: Stress test context close versus fdinfo reads Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-27 13:20   ` [Intel-gfx] [PATCH i-g-t v2 " Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 03/12] tools/intel_gpu_top: Restore user friendly error message Tvrtko Ursulin
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A few basic smoke tests to check per client memory info looks legit.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/intel/drm_fdinfo.c | 201 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)

diff --git a/tests/intel/drm_fdinfo.c b/tests/intel/drm_fdinfo.c
index a9910900358d..8e0d04bde62b 100644
--- a/tests/intel/drm_fdinfo.c
+++ b/tests/intel/drm_fdinfo.c
@@ -23,6 +23,7 @@
  */
 
 #include <fcntl.h>
+#include <sys/ioctl.h>
 
 #include "igt.h"
 #include "igt_core.h"
@@ -76,6 +77,16 @@
  *
  * SUBTEST: virtual-idle
  *
+ * SUBTEST: memory-info-idle
+ *
+ * SUBTEST: memory-info-active
+ *
+ * SUBTEST: memory-info-resident
+ *
+ * SUBTEST: memory-info-purgeable
+ *
+ * SUBTEST: memory-info-shared
+ *
  * SUBTEST: context-close-stress
  */
 
@@ -143,6 +154,11 @@ static unsigned int measured_usleep(unsigned int usec)
 #define FLAG_HANG (8)
 #define TEST_ISOLATION (16)
 
+#define TEST_ACTIVE TEST_BUSY
+#define TEST_RESIDENT (32)
+#define TEST_PURGEABLE (64)
+#define TEST_SHARED (128)
+
 static void end_spin(int fd, igt_spin_t *spin, unsigned int flags)
 {
 	if (!spin)
@@ -772,6 +788,156 @@ static void stress_context_close(int i915)
 	igt_stop_helper(&reader);
 }
 
+static size_t read_fdinfo(char *buf, const size_t sz, int at, const char *name)
+{
+	size_t count;
+	int fd;
+
+	fd = openat(at, name, O_RDONLY);
+	if (fd < 0)
+		return 0;
+
+	count = read(fd, buf, sz - 1);
+	if (count > 0)
+		buf[count - 1] = 0;
+	close(fd);
+
+	return count > 0 ? count : 0;
+}
+
+#define fdinfo_assert(cond, d, sz, total) \
+	igt_assert_f(cond, "\ndelta=%"PRIu64" sz=%"PRIu64" total=%"PRIu64"\n%s\n", d, sz, total, fdinfo_buf)
+
+static void
+test_memory(int i915, struct gem_memory_region *r, unsigned int flags)
+{
+	struct drm_client_fdinfo prev_info = { };
+	struct drm_client_fdinfo info = { };
+	const uint64_t max_mem = 512ull * 1024 * 1024;
+	const uint64_t max_bo = 16ull * 1024 * 1024;
+	uint64_t total = 0, sz, d;
+	char buf[64], fdinfo_buf[4096];
+	igt_spin_t *spin = NULL;
+	int ret, dir;
+
+	i915 = drm_reopen_driver(i915);
+
+	ret = snprintf(buf, sizeof(buf), "%u", i915);
+	igt_assert(ret > 0 && ret < sizeof(buf));
+
+	dir = open("/proc/self/fdinfo", O_DIRECTORY | O_RDONLY);
+	igt_assert_fd(dir);
+
+	gem_quiescent_gpu(i915);
+	ret =  __igt_parse_drm_fdinfo(dir, buf, &info, NULL, 0, NULL, 0);
+	igt_assert(ret > 0);
+	igt_require(info.num_regions);
+	memcpy(&prev_info, &info, sizeof(info));
+
+	while (total < max_mem) {
+		static const char *region_map[] = {
+			"system0",
+			"local0",
+		};
+		uint32_t bo;
+		int j;
+
+		/* Align to 1MiB to work around drm_print_memory_stats */
+		sz = 1ull * 1024 * 1024 + random() % (max_bo - 1ull * 1024 * 1024);
+		sz &= ~((1ull * 1024 * 1024) - 1);
+		ret = __gem_create_in_memory_region_list(i915, &bo, &sz, 0, &r->ci, 1);
+		igt_assert_eq(ret, 0);
+		total += sz;
+
+		if (flags & (TEST_RESIDENT | TEST_PURGEABLE)) {
+			struct drm_i915_gem_exec_object2 obj[2];
+			struct drm_i915_gem_execbuffer2 eb = {
+				.buffers_ptr = to_user_pointer(obj),
+				.buffer_count = 2,
+			};
+			const uint32_t bbe = MI_BATCH_BUFFER_END;
+
+			obj[0].handle = bo;
+			obj[1].handle = gem_create(i915, 4096);
+			gem_write(i915, obj[1].handle, 0, &bbe, sizeof(bbe));
+			gem_execbuf(i915, &eb);
+			gem_sync(i915, obj[1].handle);
+			gem_close(i915, obj[1].handle);
+		}
+
+		if (flags & TEST_PURGEABLE)
+			gem_madvise(i915, bo, I915_MADV_DONTNEED);
+
+		if (flags & TEST_SHARED) {
+			struct drm_gem_open open_struct;
+			struct drm_gem_flink flink;
+
+			flink.handle = bo;
+			ret = ioctl(i915, DRM_IOCTL_GEM_FLINK, &flink);
+			igt_assert_eq(ret, 0);
+
+			open_struct.name = flink.name;
+			ret = ioctl(i915, DRM_IOCTL_GEM_OPEN, &open_struct);
+			igt_assert_eq(ret, 0);
+			igt_assert(open_struct.handle != 0);
+		}
+
+		if (flags & TEST_ACTIVE)
+			spin = igt_spin_new(i915, .dependency = bo);
+
+		memset(&info, 0, sizeof(info));
+		ret =  __igt_parse_drm_fdinfo(dir, buf, &info,
+					      NULL, 0,
+					      region_map, ARRAY_SIZE(region_map));
+		igt_assert(ret > 0);
+		igt_assert(info.num_regions);
+
+		read_fdinfo(fdinfo_buf, sizeof(fdinfo_buf), dir, buf);
+
+		/* See region map */
+		j = r->ci.memory_class == I915_MEMORY_CLASS_SYSTEM ? 0 : 1;
+
+		fdinfo_assert(info.region_mem[j].total > prev_info.region_mem[j].total, d, sz, total);
+
+		d = info.region_mem[j].total - prev_info.region_mem[j].total;
+		/* >= to account for objects out of our control */
+		fdinfo_assert(d >= sz, d, sz, total);
+
+		d = info.region_mem[j].resident - prev_info.region_mem[j].resident;
+		if (flags & (TEST_RESIDENT | TEST_PURGEABLE | TEST_ACTIVE))
+			fdinfo_assert(d >= sz, d, sz, total);
+		else
+			fdinfo_assert(d < sz, d, sz, total);
+
+		d = info.region_mem[j].shared - prev_info.region_mem[j].shared;
+		if (flags & TEST_SHARED)
+			fdinfo_assert(d >= sz, d, sz, total);
+		else
+			fdinfo_assert(d == 0, d, sz, total);
+
+		d = info.region_mem[j].purgeable - prev_info.region_mem[j].purgeable;
+		if (flags & TEST_PURGEABLE)
+			fdinfo_assert(d >= sz, d, sz, total);
+		else
+			fdinfo_assert(d < sz, d, sz, total);
+
+		/* Active is one at a time so can't use delta */
+		if (flags & TEST_ACTIVE)
+			fdinfo_assert(info.region_mem[j].active >= sz, d, sz, total);
+		else
+			fdinfo_assert(info.region_mem[j].active < sz, d, sz, total);
+
+		memcpy(&prev_info, &info, sizeof(info));
+
+		if (flags & TEST_ACTIVE) {
+			igt_spin_free(i915, spin);
+			gem_quiescent_gpu(i915);
+		}
+	}
+
+	close(i915);
+}
+
 #define test_each_engine(T, i915, ctx, e) \
 	igt_subtest_with_dynamic(T) for_each_ctx_engine(i915, ctx, e) \
 		igt_dynamic_f("%s", e->name)
@@ -902,6 +1068,41 @@ igt_main
 	test_each_engine("isolation", i915, ctx, e)
 		single(i915, ctx, e, TEST_BUSY | TEST_ISOLATION);
 
+	igt_subtest_with_dynamic("memory-info-idle") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, 0);
+		}
+	}
+
+	igt_subtest_with_dynamic("memory-info-resident") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, TEST_RESIDENT);
+		}
+	}
+
+	igt_subtest_with_dynamic("memory-info-purgeable") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, TEST_PURGEABLE);
+		}
+	}
+
+	igt_subtest_with_dynamic("memory-info-active") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, TEST_ACTIVE);
+		}
+	}
+
+	igt_subtest_with_dynamic("memory-info-shared") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, TEST_SHARED);
+		}
+	}
+
 	igt_subtest_group {
 		int newfd;
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 03/12] tools/intel_gpu_top: Restore user friendly error message
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 01/12] tests/i915/drm_fdinfo: Stress test context close versus fdinfo reads Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 02/12] tests/i915/drm_fdinfo: Add some memory info tests Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-27 20:13   ` Umesh Nerlige Ramappa
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 04/12] tools/intel_gpu_top: Fix clients header width when no clients Tvrtko Ursulin
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We have a nice error message displayed when an user with insufficient
permissions tries to run the tool, but that got lost while Meteorlake
support was added. Bring it back in.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
---
 tools/intel_gpu_top.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 87e9681e53b4..e01355f90458 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -554,9 +554,11 @@ static int get_num_gts(uint64_t type)
 
 		close(fd);
 	}
-	assert(!errno || errno == ENOENT);
-	assert(cnt > 0);
-	errno = 0;
+
+	if (!cnt)
+		cnt = errno;
+	else
+		errno = 0;
 
 	return cnt;
 }
@@ -590,6 +592,8 @@ static int pmu_init(struct engines *engines)
 	engines->fd = -1;
 	engines->num_counters = 0;
 	engines->num_gts = get_num_gts(type);
+	if (engines->num_gts <= 0)
+		return -1;
 
 	engines->irq.config = I915_PMU_INTERRUPTS;
 	fd = _open_pmu(type, engines->num_counters, &engines->irq, engines->fd);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 04/12] tools/intel_gpu_top: Fix clients header width when no clients
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
                   ` (2 preceding siblings ...)
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 03/12] tools/intel_gpu_top: Restore user friendly error message Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 05/12] tools/intel_gpu_top: Fix client layout on first sample period Tvrtko Ursulin
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Recent refactoring broke the clients header in cases when there are no
clients displayed. To fix it we need to account the width of the "NAME"
label.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tools/intel_gpu_top.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index e01355f90458..76956619eaae 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -1969,6 +1969,8 @@ print_clients_header(struct igt_drm_clients *clients, int lines,
 		     int con_w, int con_h, int *class_w)
 {
 	struct intel_clients *iclients = clients->private_data;
+	const int max_name_len = clients->max_name_len < 4 ?
+				 4 : clients->max_name_len; /* At least "NAME" */
 
 	if (output_mode == INTERACTIVE) {
 		unsigned int num_active = 0;
@@ -1992,9 +1994,8 @@ print_clients_header(struct igt_drm_clients *clients, int lines,
 					num_active++;
 			}
 
-			*class_w = width =
-				(con_w - len - clients->max_name_len - 1) /
-				num_active;
+			*class_w = width = (con_w - len - max_name_len - 1) /
+					   num_active;
 
 			for (i = 0; i <= iclients->classes.max_engine_id; i++) {
 				const char *name = iclients->classes.names[i];
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 05/12] tools/intel_gpu_top: Fix client layout on first sample period
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
                   ` (3 preceding siblings ...)
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 04/12] tools/intel_gpu_top: Fix clients header width when no clients Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 06/12] tools/intel_gpu_top: Optimise interactive display a bit Tvrtko Ursulin
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

When I moved the client name to be last, I did not account for the fact
current code skips showing engine utilisation until at least two sampling
periods have passed. Consequence of this is that client name gets printed
as the second field and not under the "NAME" column header.

Fix it by emitting spaces instead of engine utilisation until two samples
have been collected.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tools/intel_gpu_top.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 76956619eaae..6363f460c892 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -930,12 +930,14 @@ static void free_display_clients(struct igt_drm_clients *clients)
 
 static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
 
-static void n_spaces(const unsigned int n)
+static unsigned int n_spaces(const unsigned int n)
 {
 	unsigned int i;
 
 	for (i = 0; i < n; i++)
 		putchar(' ');
+
+	return n;
 }
 
 static void
@@ -2045,14 +2047,17 @@ print_client(struct igt_drm_client *c, struct engines *engines, double t, int li
 
 		len = printf("%*s ", clients->max_pid_len, c->pid_str);
 
-		for (i = 0;
-		     c->samples > 1 && i <= iclients->classes.max_engine_id;
-		     i++) {
+		for (i = 0; i <= iclients->classes.max_engine_id; i++) {
 			double pct, max;
 
 			if (!iclients->classes.capacity[i])
 				continue;
 
+			if (c->samples < 2) {
+				len += n_spaces(*class_w);
+				continue;
+			}
+
 			pct = (double)c->val[i] / period_us / 1e3 * 100;
 
 			/*
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 06/12] tools/intel_gpu_top: Optimise interactive display a bit
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
                   ` (4 preceding siblings ...)
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 05/12] tools/intel_gpu_top: Fix client layout on first sample period Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 07/12] lib/igt_drm_fdinfo: Copy over region map name on match Tvrtko Ursulin
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Padding the percentage bars and table columns with spaces happens quite a
lot so lets do better than putchar at a time. Have a table of visually
empty strings and build the required length out of those chunks.

While at it, also move the percentage bar table into its function scope.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tools/intel_gpu_top.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 6363f460c892..a7a24e527f01 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -928,14 +928,40 @@ static void free_display_clients(struct igt_drm_clients *clients)
 	free(clients);
 }
 
-static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
-
 static unsigned int n_spaces(const unsigned int n)
 {
-	unsigned int i;
+	static const char *spaces[] = {
+		" ",
+		"  ",
+		"   ",
+		"    ",
+		"     ",
+		"      ",
+		"       ",
+		"        ",
+		"         ",
+		"          ",
+		"           ",
+		"            ",
+		"             ",
+		"              ",
+		"               ",
+		"                ",
+		"                 ",
+		"                  ",
+		"                   ",
+#define MAX_SPACES 19
+	};
+	unsigned int i, r = n;
 
-	for (i = 0; i < n; i++)
-		putchar(' ');
+	while (r) {
+		if (r > MAX_SPACES)
+			i = MAX_SPACES - 1;
+		else
+			i = r - 1;
+		fputs(spaces[i], stdout);
+		r -= i + 1;
+	}
 
 	return n;
 }
@@ -943,6 +969,8 @@ static unsigned int n_spaces(const unsigned int n)
 static void
 print_percentage_bar(double percent, double max, int max_len, bool numeric)
 {
+	static const char *bars[] =
+		{ " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
 	int bar_len, i, len = max_len - 2;
 	const int w = 8;
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 07/12] lib/igt_drm_fdinfo: Copy over region map name on match
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
                   ` (5 preceding siblings ...)
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 06/12] tools/intel_gpu_top: Optimise interactive display a bit Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 08/12] lib/igt_drm_clients: Fix client id type confusion Tvrtko Ursulin
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I will need some record of which regions were found for intel_gpu_top so
lets just copy over the region name from the map on the first match.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_drm_fdinfo.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
index b72822894782..222ccbfb1fd6 100644
--- a/lib/igt_drm_fdinfo.c
+++ b/lib/igt_drm_fdinfo.c
@@ -148,6 +148,10 @@ static int parse_region(char *line, struct drm_client_fdinfo *info,
 		for (i = 0; i < region_entries; i++) {
 			if (!strncmp(name, region_map[i], name_len)) {
 				found = i;
+				if (!info->region_names[info->num_regions][0]) {
+					assert(name_len < sizeof(info->region_names[i]));
+					strncpy(info->region_names[i], name, name_len);
+				}
 				break;
 			}
 		}
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 08/12] lib/igt_drm_clients: Fix client id type confusion
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
                   ` (6 preceding siblings ...)
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 07/12] lib/igt_drm_fdinfo: Copy over region map name on match Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 09/12] lib/igt_drm_clients: Allow passing in the memory region map Tvrtko Ursulin
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Igt_drm_fdinfo defines it as an unsigned long so it is best that it
matches here as well.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_drm_clients.c | 2 +-
 lib/igt_drm_clients.h | 2 +-
 tools/intel_gpu_top.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index 47ad137d5f1f..da51d7335b2b 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -49,7 +49,7 @@ struct igt_drm_clients *igt_drm_clients_init(void *private_data)
 static struct igt_drm_client *
 igt_drm_clients_find(struct igt_drm_clients *clients,
 		     enum igt_drm_client_status status,
-		     unsigned int drm_minor, unsigned int id)
+		     unsigned int drm_minor, unsigned long id)
 {
 	unsigned int start, num;
 	struct igt_drm_client *c;
diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
index 07bd236d43bf..cd37f8508b20 100644
--- a/lib/igt_drm_clients.h
+++ b/lib/igt_drm_clients.h
@@ -56,7 +56,7 @@ struct igt_drm_client {
 	enum igt_drm_client_status status;
 	struct igt_drm_client_regions *regions; /* Memory regions present in this client, to map with memory usage. */
 	struct igt_drm_client_engines *engines; /* Engines used by this client, to map with busynees data. */
-	unsigned int id; /* DRM client id from fdinfo. */
+	unsigned long id; /* DRM client id from fdinfo. */
 	unsigned int drm_minor; /* DRM minor of this client. */
 	unsigned int pid; /* PID which has this DRM fd open. */
 	char pid_str[10]; /* Cached PID representation. */
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index a7a24e527f01..e18ec25e8036 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -2106,7 +2106,7 @@ print_client(struct igt_drm_client *c, struct engines *engines, double t, int li
 	} else if (output_mode == JSON) {
 		char buf[64];
 
-		snprintf(buf, sizeof(buf), "%u", c->id);
+		snprintf(buf, sizeof(buf), "%lu", c->id);
 		pops->open_struct(buf);
 
 		__json_add_member("name", c->print_name);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 09/12] lib/igt_drm_clients: Allow passing in the memory region map
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
                   ` (7 preceding siblings ...)
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 08/12] lib/igt_drm_clients: Fix client id type confusion Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 10/12] tools/intel_gpu_top: Fully wrap clients operations Tvrtko Ursulin
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Same concept as with the engine map, allowing callers to pass in fixed
map of names to indices, simplifying their implementation and avoiding
auto-detection while parsing.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_drm_clients.c | 5 +++--
 lib/igt_drm_clients.h | 3 ++-
 tools/gputop.c        | 4 ++--
 tools/intel_gpu_top.c | 8 +++++---
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index da51d7335b2b..025d60c51503 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -445,7 +445,8 @@ struct igt_drm_clients *
 igt_drm_clients_scan(struct igt_drm_clients *clients,
 		     bool (*filter_client)(const struct igt_drm_clients *,
 					   const struct drm_client_fdinfo *),
-		     const char **name_map, unsigned int map_entries)
+		     const char **name_map, unsigned int map_entries,
+		     const char **region_map, unsigned int region_entries)
 {
 	struct dirent *proc_dent;
 	struct igt_drm_client *c;
@@ -524,7 +525,7 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
 			if (!__igt_parse_drm_fdinfo(dirfd(fdinfo_dir),
 						    fdinfo_dent->d_name, &info,
 						    name_map, map_entries,
-						    NULL, 0))
+						    region_map, region_entries))
 				continue;
 
 			if (filter_client && !filter_client(clients, &info))
diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
index cd37f8508b20..52888aedc25a 100644
--- a/lib/igt_drm_clients.h
+++ b/lib/igt_drm_clients.h
@@ -93,7 +93,8 @@ struct igt_drm_clients *
 igt_drm_clients_scan(struct igt_drm_clients *clients,
 		     bool (*filter_client)(const struct igt_drm_clients *,
 					   const struct drm_client_fdinfo *),
-		     const char **name_map, unsigned int map_entries);
+		     const char **name_map, unsigned int map_entries,
+		     const char **region_map, unsigned int region_entries);
 
 struct igt_drm_clients *
 igt_drm_clients_sort(struct igt_drm_clients *clients,
diff --git a/tools/gputop.c b/tools/gputop.c
index ea95e0333dd2..71e28f43ee4c 100644
--- a/tools/gputop.c
+++ b/tools/gputop.c
@@ -253,7 +253,7 @@ int main(int argc, char **argv)
 	if (!clients)
 		exit(1);
 
-	igt_drm_clients_scan(clients, NULL, NULL, 0);
+	igt_drm_clients_scan(clients, NULL, NULL, 0, NULL, 0);
 
 	for (;;) {
 		struct igt_drm_client *c, *prevc = NULL;
@@ -270,7 +270,7 @@ int main(int argc, char **argv)
 			}
 		}
 
-		igt_drm_clients_scan(clients, NULL, NULL, 0);
+		igt_drm_clients_scan(clients, NULL, NULL, 0, NULL, 0);
 		igt_drm_clients_sort(clients, client_cmp);
 
 		printf("\033[H\033[J");
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index e18ec25e8036..eb0ef00abeaf 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -2619,8 +2619,9 @@ int main(int argc, char **argv)
 	}
 
 	pmu_sample(engines);
-	igt_drm_clients_scan(clients, client_match, engine_map,
-			     ARRAY_SIZE(engine_map));
+	igt_drm_clients_scan(clients, client_match,
+			     engine_map, ARRAY_SIZE(engine_map),
+			     NULL, 0);
 	gettime(&ts);
 
 	if (output_mode == JSON)
@@ -2655,7 +2656,8 @@ int main(int argc, char **argv)
 			display_clients(igt_drm_clients_scan(clients,
 							     client_match,
 							     engine_map,
-							     ARRAY_SIZE(engine_map)));
+							     ARRAY_SIZE(engine_map),
+							     NULL, 0));
 		scan_us = elapsed_us(&ts, period_us);
 
 		if (stop_top)
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 10/12] tools/intel_gpu_top: Fully wrap clients operations
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
                   ` (8 preceding siblings ...)
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 09/12] lib/igt_drm_clients: Allow passing in the memory region map Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 11/12] tools/intel_gpu_top: Add per client memory info Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 12/12] tools/intel_gpu_top: Add ability to show memory region breakdown Tvrtko Ursulin
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Wrap all operations on clients via the Intel specific wrappers in order to
simplify upcoming work.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tools/intel_gpu_top.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index eb0ef00abeaf..27503ac03ebd 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -132,6 +132,7 @@ struct engines {
 struct intel_clients {
 	const char *pci_slot;
 	struct igt_drm_client_engines classes;
+	struct igt_drm_clients *clients;
 };
 
 static struct termios termios_orig;
@@ -2436,19 +2437,22 @@ intel_init_clients(struct intel_clients *iclients,
 		iclients->classes.capacity[i] = engines->class[i].num_engines;
 		iclients->classes.names[i] = strdup(engines->class[i].name);
 	}
+
+	iclients->clients = igt_drm_clients_init(iclients);
 }
 
 static void intel_free_clients(struct intel_clients *iclients)
 {
+	if (iclients->clients)
+		igt_drm_clients_free(iclients->clients);
+
 	free((void *)iclients->pci_slot);
 	free(iclients->classes.capacity);
 	free(iclients->classes.names);
 }
 
-int main(int argc, char **argv)
+static void intel_scan_clients(struct intel_clients *iclients)
 {
-	unsigned int period_us = DEFAULT_PERIOD_MS * 1000;
-	struct igt_drm_clients *clients = NULL;
 	static const char *engine_map[] = {
 		"render",
 		"copy",
@@ -2456,6 +2460,15 @@ int main(int argc, char **argv)
 		"video-enhance",
 		"compute",
 	};
+
+	igt_drm_clients_scan(iclients->clients, client_match,
+			     engine_map, ARRAY_SIZE(engine_map),
+			     NULL, 0);
+}
+
+int main(int argc, char **argv)
+{
+	unsigned int period_us = DEFAULT_PERIOD_MS * 1000;
 	bool physical_engines = false;
 	struct intel_clients iclients;
 	int con_w = -1, con_h = -1;
@@ -2613,15 +2626,11 @@ int main(int argc, char **argv)
 
 	init_engine_classes(engines);
 
-	if (has_drm_fdinfo(&card)) {
+	if (has_drm_fdinfo(&card))
 		intel_init_clients(&iclients, &card, engines);
-		clients = igt_drm_clients_init(&iclients);
-	}
 
 	pmu_sample(engines);
-	igt_drm_clients_scan(clients, client_match,
-			     engine_map, ARRAY_SIZE(engine_map),
-			     NULL, 0);
+	intel_scan_clients(&iclients);
 	gettime(&ts);
 
 	if (output_mode == JSON)
@@ -2652,12 +2661,8 @@ int main(int argc, char **argv)
 		pmu_sample(engines);
 		t = (double)(engines->ts.cur - engines->ts.prev) / 1e9;
 
-		disp_clients =
-			display_clients(igt_drm_clients_scan(clients,
-							     client_match,
-							     engine_map,
-							     ARRAY_SIZE(engine_map),
-							     NULL, 0));
+		intel_scan_clients(&iclients);
+		disp_clients = display_clients(iclients.clients);
 		scan_us = elapsed_us(&ts, period_us);
 
 		if (stop_top)
@@ -2708,7 +2713,7 @@ int main(int argc, char **argv)
 			pops->close_struct();
 		}
 
-		if (disp_clients != clients)
+		if (disp_clients != iclients.clients)
 			free_display_clients(disp_clients);
 
 		if (stop_top)
@@ -2723,10 +2728,7 @@ int main(int argc, char **argv)
 	if (output_mode == JSON)
 		printf("]\n");
 
-	if (clients) {
-		igt_drm_clients_free(clients);
-		intel_free_clients(&iclients);
-	}
+	intel_free_clients(&iclients);
 
 	free(codename);
 err_pmu:
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 11/12] tools/intel_gpu_top: Add per client memory info
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
                   ` (9 preceding siblings ...)
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 10/12] tools/intel_gpu_top: Fully wrap clients operations Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 12/12] tools/intel_gpu_top: Add ability to show memory region breakdown Tvrtko Ursulin
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

JSON output has the full breakdown but for now the interactive mode only
shows total and resident aggregated for all memory regions.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tools/intel_gpu_top.c | 114 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 112 insertions(+), 2 deletions(-)

diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index 27503ac03ebd..c239a0d4f350 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -133,11 +133,24 @@ struct intel_clients {
 	const char *pci_slot;
 	struct igt_drm_client_engines classes;
 	struct igt_drm_clients *clients;
+	struct igt_drm_client_regions *regions; /* Borrowed from first client */
 };
 
 static struct termios termios_orig;
 static bool class_view;
 
+/* Maps i915 fdinfo names to indices */
+static const char *memory_region_map[] = {
+	"system0",
+	"local0",
+};
+
+/* For JSON, 1:1 with indices above. */
+static const char *json_memory_region_names[] = {
+	"system",
+	"local",
+};
+
 __attribute__((format(scanf,3,4)))
 static int igt_sysfs_scanf(int dir, const char *attr, const char *fmt, ...)
 {
@@ -884,6 +897,9 @@ static struct igt_drm_clients *display_clients(struct igt_drm_clients *clients)
 			ac->val = calloc(c->engines->max_engine_id + 1,
 					 sizeof(ac->val[0]));
 			assert(ac->val);
+			ac->regions = c->regions;
+			ac->memory = calloc(c->regions->max_region_id + 1,
+					    sizeof(ac->memory[0]));
 			ac->samples = 1;
 		}
 
@@ -898,6 +914,14 @@ static struct igt_drm_clients *display_clients(struct igt_drm_clients *clients)
 
 		for (i = 0; i <= c->engines->max_engine_id; i++)
 			ac->val[i] += c->val[i];
+
+		for (i = 0; i <= c->regions->max_region_id; i++) {
+			ac->memory[i].total += c->memory[i].total;
+			ac->memory[i].shared += c->memory[i].shared;
+			ac->memory[i].resident += c->memory[i].resident;
+			ac->memory[i].purgeable += c->memory[i].purgeable;
+			ac->memory[i].active += c->memory[i].active;
+		}
 	}
 
 	aggregated->num_clients = num;
@@ -922,8 +946,10 @@ static void free_display_clients(struct igt_drm_clients *clients)
 	 * "display" clients are not proper clients and have un-initialized
 	 * or borrowed fields which we don't want the library to try and free.
 	 */
-	igt_for_each_drm_client(clients, c, tmp)
+	igt_for_each_drm_client(clients, c, tmp) {
 		free(c->val);
+		free(c->memory);
+	}
 
 	free(clients->client);
 	free(clients);
@@ -2016,6 +2042,9 @@ print_clients_header(struct igt_drm_clients *clients, int lines,
 		if (lines++ >= con_h || len >= con_w)
 			return lines;
 
+		if (iclients->regions)
+			len += printf("     MEM      RSS ");
+
 		if (iclients->classes.num_engines) {
 			unsigned int i;
 			int width;
@@ -2059,6 +2088,20 @@ print_clients_header(struct igt_drm_clients *clients, int lines,
 static bool numeric_clients;
 static bool filter_idle;
 
+static int print_size(uint64_t sz)
+{
+	char units[] = { ' ', 'K', 'M', 'G' };
+	unsigned int u;
+
+	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
+		if (sz & 1023 || sz < 1024)
+			break;
+		sz /= 1024;
+	}
+
+	return printf("%7"PRIu64"%c ", sz, units[u]);
+}
+
 static int
 print_client(struct igt_drm_client *c, struct engines *engines, double t, int lines,
 	     int con_w, int con_h, unsigned int period_us, int *class_w)
@@ -2076,6 +2119,18 @@ print_client(struct igt_drm_client *c, struct engines *engines, double t, int li
 
 		len = printf("%*s ", clients->max_pid_len, c->pid_str);
 
+		if (iclients->regions) {
+			uint64_t sz;
+
+			for (sz = 0, i = 0; i <= c->regions->max_region_id; i++)
+				sz += c->memory[i].total;
+			len += print_size(sz);
+
+			for (sz = 0, i = 0; i <= c->regions->max_region_id; i++)
+				sz += c->memory[i].resident;
+			len += print_size(sz);
+		}
+
 		for (i = 0; i <= iclients->classes.max_engine_id; i++) {
 			double pct, max;
 
@@ -2115,6 +2170,42 @@ print_client(struct igt_drm_client *c, struct engines *engines, double t, int li
 		snprintf(buf, sizeof(buf), "%u", c->pid);
 		__json_add_member("pid", buf);
 
+		if (iclients->regions) {
+			pops->open_struct("memory");
+
+			for (i = 0; i < ARRAY_SIZE(json_memory_region_names);
+			     i++) {
+				if (i > c->regions->max_region_id)
+					break;
+
+				pops->open_struct(json_memory_region_names[i]);
+
+				snprintf(buf, sizeof(buf), "%" PRIu64,
+					 c->memory[i].total);
+				__json_add_member("total", buf);
+
+				snprintf(buf, sizeof(buf), "%" PRIu64,
+					 c->memory[i].shared);
+				__json_add_member("shared", buf);
+
+				snprintf(buf, sizeof(buf), "%" PRIu64,
+					 c->memory[i].resident);
+				__json_add_member("resident", buf);
+
+				snprintf(buf, sizeof(buf), "%" PRIu64,
+					 c->memory[i].purgeable);
+				__json_add_member("purgeable", buf);
+
+				snprintf(buf, sizeof(buf), "%" PRIu64,
+					 c->memory[i].active);
+				__json_add_member("active", buf);
+
+				pops->close_struct();
+			}
+
+			pops->close_struct();
+		}
+
 		if (c->samples > 1) {
 			pops->open_struct("engine-classes");
 
@@ -2460,10 +2551,29 @@ static void intel_scan_clients(struct intel_clients *iclients)
 		"video-enhance",
 		"compute",
 	};
+	struct igt_drm_client *c;
+	unsigned int i;
 
 	igt_drm_clients_scan(iclients->clients, client_match,
 			     engine_map, ARRAY_SIZE(engine_map),
-			     NULL, 0);
+			     memory_region_map, ARRAY_SIZE(memory_region_map));
+
+	iclients->regions = NULL;
+
+	if (!iclients->clients)
+		return;
+
+	/*
+	 * Borrow memory region data from first client so we can use it
+	 * when displaying stuff. All regions are the same due our client_match.
+	 */
+	igt_for_each_drm_client(iclients->clients, c, i) {
+		if (c->status == IGT_DRM_CLIENT_ALIVE) {
+			if (c->regions->num_regions)
+				iclients->regions = c->regions;
+			break;
+		}
+	}
 }
 
 int main(int argc, char **argv)
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 12/12] tools/intel_gpu_top: Add ability to show memory region breakdown
  2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
                   ` (10 preceding siblings ...)
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 11/12] tools/intel_gpu_top: Add per client memory info Tvrtko Ursulin
@ 2023-09-22 13:44 ` Tvrtko Ursulin
  11 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-22 13:44 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Similar as we can toggle between aggregated engines and clients, add the
capability to toggle between aggregated and per memory region stats.

It starts in aggregated mode by default and interactive command 'm' and
command line switch '-m' can be used to toggle that.

Both only affect the interactive view, while JSON output always contains
separate memory regions.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 man/intel_gpu_top.rst |  4 ++++
 tools/intel_gpu_top.c | 56 +++++++++++++++++++++++++++++++++----------
 2 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/man/intel_gpu_top.rst b/man/intel_gpu_top.rst
index 9499f87f1b4d..44a54a5f219d 100644
--- a/man/intel_gpu_top.rst
+++ b/man/intel_gpu_top.rst
@@ -55,6 +55,9 @@ OPTIONS
 -p
    Default to showing physical engines instead of aggregated classes.
 
+-m
+   Default to showing all memory regions separately.
+
 RUNTIME CONTROL
 ===============
 
@@ -68,6 +71,7 @@ Supported keys:
 |    's'    Toggle between sort modes (runtime, total runtime, pid, client id).
 |    'i'    Toggle display of clients which used no GPU time.
 |    'H'    Toggle between per PID aggregation and individual clients.
+|    'm'    Toggle between aggregated memory regions and full breakdown.
 
 DEVICE SELECTION
 ================
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index c239a0d4f350..3b45fcc21331 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -138,6 +138,7 @@ struct intel_clients {
 
 static struct termios termios_orig;
 static bool class_view;
+static bool aggregate_regions;
 
 /* Maps i915 fdinfo names to indices */
 static const char *memory_region_map[] = {
@@ -1049,6 +1050,7 @@ usage(const char *appname)
 		"\t[-L]            List all cards.\n"
 		"\t[-d <device>]   Device filter, please check manual page for more details.\n"
 		"\t[-p]            Default to showing physical engines instead of classes.\n"
+		"\t[-m]            Default to showing all memory regions.\n"
 		"\n",
 		appname, DEFAULT_PERIOD_MS);
 	igt_device_print_filter_types();
@@ -2030,7 +2032,7 @@ print_clients_header(struct igt_drm_clients *clients, int lines,
 				 4 : clients->max_name_len; /* At least "NAME" */
 
 	if (output_mode == INTERACTIVE) {
-		unsigned int num_active = 0;
+		unsigned int num_active = 0, i;
 		int len;
 
 		if (lines++ >= con_h)
@@ -2042,11 +2044,17 @@ print_clients_header(struct igt_drm_clients *clients, int lines,
 		if (lines++ >= con_h || len >= con_w)
 			return lines;
 
-		if (iclients->regions)
-			len += printf("     MEM      RSS ");
+		if (iclients->regions) {
+			if (aggregate_regions) {
+				len += printf("     MEM      RSS ");
+			} else {
+				len += printf("     RAM      RSS ");
+				if (iclients->regions->num_regions > 1)
+					len += printf("    VRAM     VRSS ");
+			}
+		}
 
 		if (iclients->classes.num_engines) {
-			unsigned int i;
 			int width;
 
 			for (i = 0; i <= iclients->classes.max_engine_id; i++) {
@@ -2120,15 +2128,26 @@ print_client(struct igt_drm_client *c, struct engines *engines, double t, int li
 		len = printf("%*s ", clients->max_pid_len, c->pid_str);
 
 		if (iclients->regions) {
-			uint64_t sz;
+			if (aggregate_regions) {
+				uint64_t sz;
 
-			for (sz = 0, i = 0; i <= c->regions->max_region_id; i++)
-				sz += c->memory[i].total;
-			len += print_size(sz);
+				for (sz = 0, i = 0;
+				     i <= c->regions->max_region_id; i++)
+					sz += c->memory[i].total;
+				len += print_size(sz);
 
-			for (sz = 0, i = 0; i <= c->regions->max_region_id; i++)
-				sz += c->memory[i].resident;
-			len += print_size(sz);
+				for (sz = 0, i = 0;
+				     i <= c->regions->max_region_id; i++)
+					sz += c->memory[i].resident;
+				len += print_size(sz);
+			} else {
+				len += print_size(c->memory[0].total);
+				len += print_size(c->memory[0].resident);
+				if (c->regions->num_regions > 1) {
+					len += print_size(c->memory[1].total);
+					len += print_size(c->memory[1].resident);
+				}
+			}
 		}
 
 		for (i = 0; i <= iclients->classes.max_engine_id; i++) {
@@ -2405,6 +2424,13 @@ static void process_normal_stdin(void)
 			else
 				header_msg = "Showing individual clients.";
 			break;
+		case 'm':
+			aggregate_regions ^= true;
+			if (aggregate_regions)
+				header_msg = "Aggregating memory regions.";
+			else
+				header_msg = "Showing memory regions.";
+			break;
 		};
 	}
 }
@@ -2453,6 +2479,7 @@ static void show_help_screen(void)
 "    's'    Toggle between sort modes (runtime, total runtime, pid, client id).\n"
 "    'i'    Toggle display of clients which used no GPU time.\n"
 "    'H'    Toggle between per PID aggregation and individual clients.\n"
+"    'm'    Toggle between aggregated memory regions and full breakdown.\n"
 "\n"
 "    'h' or 'q'    Exit interactive help.\n"
 "\n");
@@ -2580,6 +2607,7 @@ int main(int argc, char **argv)
 {
 	unsigned int period_us = DEFAULT_PERIOD_MS * 1000;
 	bool physical_engines = false;
+	bool separate_regions = false;
 	struct intel_clients iclients;
 	int con_w = -1, con_h = -1;
 	char *output_path = NULL;
@@ -2592,7 +2620,7 @@ int main(int argc, char **argv)
 	struct timespec ts;
 
 	/* Parse options */
-	while ((ch = getopt(argc, argv, "o:s:d:pcJLlh")) != -1) {
+	while ((ch = getopt(argc, argv, "o:s:d:mpcJLlh")) != -1) {
 		switch (ch) {
 		case 'o':
 			output_path = optarg;
@@ -2606,6 +2634,9 @@ int main(int argc, char **argv)
 		case 'p':
 			physical_engines = true;
 			break;
+		case 'm':
+			separate_regions = true;
+			break;
 		case 'c':
 			output_mode = CSV;
 			break;
@@ -2649,6 +2680,7 @@ int main(int argc, char **argv)
 		fprintf(stderr, "Failed to install signal handler!\n");
 
 	class_view = !physical_engines;
+	aggregate_regions = !separate_regions;
 
 	switch (output_mode) {
 	case INTERACTIVE:
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t v2 02/12] tests/i915/drm_fdinfo: Add some memory info tests
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 02/12] tests/i915/drm_fdinfo: Add some memory info tests Tvrtko Ursulin
@ 2023-09-27 13:20   ` Tvrtko Ursulin
  0 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-27 13:20 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A few basic smoke tests to check per client memory info looks legit.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/intel/drm_fdinfo.c | 217 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 217 insertions(+)

diff --git a/tests/intel/drm_fdinfo.c b/tests/intel/drm_fdinfo.c
index a9910900358d..392ad510c34d 100644
--- a/tests/intel/drm_fdinfo.c
+++ b/tests/intel/drm_fdinfo.c
@@ -23,6 +23,7 @@
  */
 
 #include <fcntl.h>
+#include <sys/ioctl.h>
 
 #include "igt.h"
 #include "igt_core.h"
@@ -76,6 +77,16 @@
  *
  * SUBTEST: virtual-idle
  *
+ * SUBTEST: memory-info-idle
+ *
+ * SUBTEST: memory-info-active
+ *
+ * SUBTEST: memory-info-resident
+ *
+ * SUBTEST: memory-info-purgeable
+ *
+ * SUBTEST: memory-info-shared
+ *
  * SUBTEST: context-close-stress
  */
 
@@ -143,6 +154,11 @@ static unsigned int measured_usleep(unsigned int usec)
 #define FLAG_HANG (8)
 #define TEST_ISOLATION (16)
 
+#define TEST_ACTIVE TEST_BUSY
+#define TEST_RESIDENT (32)
+#define TEST_PURGEABLE (64)
+#define TEST_SHARED (128)
+
 static void end_spin(int fd, igt_spin_t *spin, unsigned int flags)
 {
 	if (!spin)
@@ -772,6 +788,172 @@ static void stress_context_close(int i915)
 	igt_stop_helper(&reader);
 }
 
+static size_t read_fdinfo(char *buf, const size_t sz, int at, const char *name)
+{
+	size_t count;
+	int fd;
+
+	fd = openat(at, name, O_RDONLY);
+	if (fd < 0)
+		return 0;
+
+	count = read(fd, buf, sz - 1);
+	if (count > 0)
+		buf[count - 1] = 0;
+	close(fd);
+
+	return count > 0 ? count : 0;
+}
+
+/*
+ * At least this much, but maybe less if we started with a driver internal
+ * baseline which can go away behind our back.
+ */
+#define fdinfo_assert_gte(cur, prev, sz, base) \
+({ \
+	int64_t __sz = (sz) - (base); \
+	int64_t __d = (cur) - (prev); \
+	igt_assert_f(__d >= __sz, \
+		     "prev=%"PRIu64" cur=%"PRIu64" delta=%"PRId64" sz=%"PRIu64" baseline=%"PRIu64"\n%s\n", \
+		     (prev), (cur), __d, (sz), (base), fdinfo_buf); \
+})
+
+#define fdinfo_assert_eq(cur, prev, sz, base) \
+({ \
+	int64_t __d = (cur) - (prev); \
+	igt_assert_f(__d == 0, \
+		     "prev=%"PRIu64" cur=%"PRIu64" delta=%"PRId64" sz=%"PRIu64" baseline=%"PRIu64"\n%s\n", \
+		     (prev), (cur), __d, (sz), (base), fdinfo_buf); \
+})
+
+static void
+test_memory(int i915, struct gem_memory_region *mr, unsigned int flags)
+{
+	const unsigned int r = mr->ci.memory_class == I915_MEMORY_CLASS_SYSTEM ? 0 : 1; /* See region map */
+	const uint64_t max_mem = 512ull * 1024 * 1024;
+	const uint64_t max_bo = 16ull * 1024 * 1024;
+	struct drm_client_fdinfo base_info, prev_info = { };
+	struct drm_client_fdinfo info = { };
+	char buf[64], fdinfo_buf[4096];
+	igt_spin_t *spin = NULL;
+	uint64_t total = 0, sz;
+	uint64_t ahnd;
+	int ret, dir;
+
+	i915 = drm_reopen_driver(i915);
+
+	ahnd = get_reloc_ahnd(i915, 0);
+
+	ret = snprintf(buf, sizeof(buf), "%u", i915);
+	igt_assert(ret > 0 && ret < sizeof(buf));
+
+	dir = open("/proc/self/fdinfo", O_DIRECTORY | O_RDONLY);
+	igt_assert_fd(dir);
+
+	gem_quiescent_gpu(i915);
+	ret =  __igt_parse_drm_fdinfo(dir, buf, &info, NULL, 0, NULL, 0);
+	igt_assert(ret > 0);
+	igt_require(info.num_regions);
+	memcpy(&prev_info, &info, sizeof(info));
+	memcpy(&base_info, &info, sizeof(info));
+
+	while (total < max_mem) {
+		static const char *region_map[] = {
+			"system0",
+			"local0",
+		};
+		uint32_t bo;
+
+		sz = random() % max_bo;
+		ret = __gem_create_in_memory_region_list(i915, &bo, &sz, 0,
+							 &mr->ci, 1);
+		igt_assert_eq(ret, 0);
+		total += sz;
+
+		if (flags & (TEST_RESIDENT | TEST_PURGEABLE | TEST_ACTIVE))
+			spin = igt_spin_new(i915,
+					    .dependency = bo,
+					    .ahnd = ahnd);
+		else
+			spin = NULL;
+
+		if (flags & TEST_PURGEABLE) {
+			gem_madvise(i915, bo, I915_MADV_DONTNEED);
+			igt_spin_free(i915, spin);
+			gem_quiescent_gpu(i915);
+			spin = NULL;
+		}
+
+		if (flags & TEST_SHARED) {
+			struct drm_gem_open open_struct;
+			struct drm_gem_flink flink;
+
+			flink.handle = bo;
+			ret = ioctl(i915, DRM_IOCTL_GEM_FLINK, &flink);
+			igt_assert_eq(ret, 0);
+
+			open_struct.name = flink.name;
+			ret = ioctl(i915, DRM_IOCTL_GEM_OPEN, &open_struct);
+			igt_assert_eq(ret, 0);
+			igt_assert(open_struct.handle != 0);
+		}
+
+		memset(&info, 0, sizeof(info));
+		ret =  __igt_parse_drm_fdinfo(dir, buf, &info,
+					      NULL, 0,
+					      region_map, ARRAY_SIZE(region_map));
+		igt_assert(ret > 0);
+		igt_assert(info.num_regions);
+
+		read_fdinfo(fdinfo_buf, sizeof(fdinfo_buf), dir, buf);
+
+		/* >= to account for objects out of our control */
+		fdinfo_assert_gte(info.region_mem[r].total,
+				  prev_info.region_mem[r].total,
+				  sz,
+				  base_info.region_mem[r].total);
+
+		if (flags & TEST_SHARED)
+			fdinfo_assert_gte(info.region_mem[r].shared,
+					  prev_info.region_mem[r].shared,
+					  sz,
+					  base_info.region_mem[r].shared);
+		else
+			fdinfo_assert_eq(info.region_mem[r].shared,
+					 prev_info.region_mem[r].shared,
+					 sz,
+					 base_info.region_mem[r].shared);
+
+		if (flags & TEST_PURGEABLE)
+			fdinfo_assert_gte(info.region_mem[r].purgeable,
+					  prev_info.region_mem[r].purgeable,
+					  sz,
+					  base_info.region_mem[r].purgeable);
+
+		if (flags & (TEST_RESIDENT | TEST_PURGEABLE | TEST_ACTIVE))
+			fdinfo_assert_gte(info.region_mem[r].resident,
+					  (uint64_t)0, /* We can only be sure the current buffer is resident. */
+					  sz,
+					  (uint64_t)0);
+
+		if (flags & TEST_ACTIVE)
+			fdinfo_assert_gte(info.region_mem[r].active,
+					  (uint64_t)0, /* We can only be sure the current buffer is active. */
+					  sz,
+					  (uint64_t)0);
+
+		memcpy(&prev_info, &info, sizeof(info));
+
+		if (spin) {
+			igt_spin_free(i915, spin);
+			gem_quiescent_gpu(i915);
+		}
+	}
+
+	put_ahnd(ahnd);
+	close(i915);
+}
+
 #define test_each_engine(T, i915, ctx, e) \
 	igt_subtest_with_dynamic(T) for_each_ctx_engine(i915, ctx, e) \
 		igt_dynamic_f("%s", e->name)
@@ -902,6 +1084,41 @@ igt_main
 	test_each_engine("isolation", i915, ctx, e)
 		single(i915, ctx, e, TEST_BUSY | TEST_ISOLATION);
 
+	igt_subtest_with_dynamic("memory-info-idle") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, 0);
+		}
+	}
+
+	igt_subtest_with_dynamic("memory-info-resident") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, TEST_RESIDENT);
+		}
+	}
+
+	igt_subtest_with_dynamic("memory-info-purgeable") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, TEST_PURGEABLE);
+		}
+	}
+
+	igt_subtest_with_dynamic("memory-info-active") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, TEST_ACTIVE);
+		}
+	}
+
+	igt_subtest_with_dynamic("memory-info-shared") {
+		for_each_memory_region(r, i915) {
+			igt_dynamic_f("%s", r->name)
+				test_memory(i915, r, TEST_SHARED);
+		}
+	}
+
 	igt_subtest_group {
 		int newfd;
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 03/12] tools/intel_gpu_top: Restore user friendly error message
  2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 03/12] tools/intel_gpu_top: Restore user friendly error message Tvrtko Ursulin
@ 2023-09-27 20:13   ` Umesh Nerlige Ramappa
  2023-09-28  8:16     ` Tvrtko Ursulin
  0 siblings, 1 reply; 19+ messages in thread
From: Umesh Nerlige Ramappa @ 2023-09-27 20:13 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx

On Fri, Sep 22, 2023 at 02:44:28PM +0100, Tvrtko Ursulin wrote:
>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
>We have a nice error message displayed when an user with insufficient
>permissions tries to run the tool, but that got lost while Meteorlake
>support was added. Bring it back in.
>
>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>---
> tools/intel_gpu_top.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
>diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>index 87e9681e53b4..e01355f90458 100644
>--- a/tools/intel_gpu_top.c
>+++ b/tools/intel_gpu_top.c
>@@ -554,9 +554,11 @@ static int get_num_gts(uint64_t type)
>
> 		close(fd);
> 	}
>-	assert(!errno || errno == ENOENT);
>-	assert(cnt > 0);
>-	errno = 0;
>+
>+	if (!cnt)
>+		cnt = errno;
>+	else
>+		errno = 0;

ENOENT is the only way this logic is checking for num_gts.

In this case error is propagated only if cnt == 0. What if cnt=1 and we 
get an error (other than ENOENT)? Should we ignore that?

I had something like this in mind for the regression (and sorry this 
fell through the cracks)

https://patchwork.freedesktop.org/patch/541406/?series=118973&rev=1

Regards,
Umesh

>
> 	return cnt;
> }
>@@ -590,6 +592,8 @@ static int pmu_init(struct engines *engines)
> 	engines->fd = -1;
> 	engines->num_counters = 0;
> 	engines->num_gts = get_num_gts(type);
>+	if (engines->num_gts <= 0)
>+		return -1;
>
> 	engines->irq.config = I915_PMU_INTERRUPTS;
> 	fd = _open_pmu(type, engines->num_counters, &engines->irq, engines->fd);
>-- 
>2.39.2
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 03/12] tools/intel_gpu_top: Restore user friendly error message
  2023-09-27 20:13   ` Umesh Nerlige Ramappa
@ 2023-09-28  8:16     ` Tvrtko Ursulin
  2023-09-28 21:31       ` Umesh Nerlige Ramappa
  0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-28  8:16 UTC (permalink / raw)
  To: Umesh Nerlige Ramappa; +Cc: igt-dev, Intel-gfx


On 27/09/2023 21:13, Umesh Nerlige Ramappa wrote:
> On Fri, Sep 22, 2023 at 02:44:28PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> We have a nice error message displayed when an user with insufficient
>> permissions tries to run the tool, but that got lost while Meteorlake
>> support was added. Bring it back in.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>> tools/intel_gpu_top.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>> index 87e9681e53b4..e01355f90458 100644
>> --- a/tools/intel_gpu_top.c
>> +++ b/tools/intel_gpu_top.c
>> @@ -554,9 +554,11 @@ static int get_num_gts(uint64_t type)
>>
>>         close(fd);
>>     }
>> -    assert(!errno || errno == ENOENT);
>> -    assert(cnt > 0);
>> -    errno = 0;
>> +
>> +    if (!cnt)
>> +        cnt = errno;
>> +    else
>> +        errno = 0;
> 
> ENOENT is the only way this logic is checking for num_gts.
> 
> In this case error is propagated only if cnt == 0. What if cnt=1 and we 
> get an error (other than ENOENT)? Should we ignore that?

If cnt >= 1 then at least one tile was found so the errno happened while 
probing for further tiles. So on single tile parts it can be ignored. On 
multi-tile parts it cannot really happen, or even if it happens 
situation would simply be "why is only one tile showing". If we want to 
cover this impossible/unlikely case then maybe like this:

	if (!cnt || (errno && errno != ENOENT))
		cnt = -errno;

> I had something like this in mind for the regression (and sorry this 
> fell through the cracks)
> 
> https://patchwork.freedesktop.org/patch/541406/?series=118973&rev=1

Oh back in June!

I think yours work too, in which case it's a matter of a style choice 
with which one to go. I don't have a strong preference - above would be 
a bit more compact, while I think it still succinctly expresses the 
failure condition ("nothing found or unexpected error while probing for 
remote tiles").

Regards,

Tvrtko

> 
> Regards,
> Umesh
> 
>>
>>     return cnt;
>> }
>> @@ -590,6 +592,8 @@ static int pmu_init(struct engines *engines)
>>     engines->fd = -1;
>>     engines->num_counters = 0;
>>     engines->num_gts = get_num_gts(type);
>> +    if (engines->num_gts <= 0)
>> +        return -1;
>>
>>     engines->irq.config = I915_PMU_INTERRUPTS;
>>     fd = _open_pmu(type, engines->num_counters, &engines->irq, 
>> engines->fd);
>> -- 
>> 2.39.2
>>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 03/12] tools/intel_gpu_top: Restore user friendly error message
  2023-09-28  8:16     ` Tvrtko Ursulin
@ 2023-09-28 21:31       ` Umesh Nerlige Ramappa
  2023-09-29 11:11         ` Tvrtko Ursulin
  0 siblings, 1 reply; 19+ messages in thread
From: Umesh Nerlige Ramappa @ 2023-09-28 21:31 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: igt-dev, Intel-gfx

On Thu, Sep 28, 2023 at 09:16:23AM +0100, Tvrtko Ursulin wrote:
>
>On 27/09/2023 21:13, Umesh Nerlige Ramappa wrote:
>>On Fri, Sep 22, 2023 at 02:44:28PM +0100, Tvrtko Ursulin wrote:
>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>>We have a nice error message displayed when an user with insufficient
>>>permissions tries to run the tool, but that got lost while Meteorlake
>>>support was added. Bring it back in.
>>>
>>>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>>---
>>>tools/intel_gpu_top.c | 10 +++++++---
>>>1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>>diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>>>index 87e9681e53b4..e01355f90458 100644
>>>--- a/tools/intel_gpu_top.c
>>>+++ b/tools/intel_gpu_top.c
>>>@@ -554,9 +554,11 @@ static int get_num_gts(uint64_t type)
>>>
>>>        close(fd);
>>>    }
>>>-    assert(!errno || errno == ENOENT);
>>>-    assert(cnt > 0);
>>>-    errno = 0;
>>>+
>>>+    if (!cnt)
>>>+        cnt = errno;
>>>+    else
>>>+        errno = 0;
>>
>>ENOENT is the only way this logic is checking for num_gts.
>>
>>In this case error is propagated only if cnt == 0. What if cnt=1 and 
>>we get an error (other than ENOENT)? Should we ignore that?
>
>If cnt >= 1 then at least one tile was found so the errno happened 
>while probing for further tiles. So on single tile parts it can be 
>ignored.

I am actually only referring to single tile parts. The for loop iterates 
over MAX_GTs (4), so I am expecting an ENOENT from a single tile part 
when cnt >= 1. Anything else is an error/failure that we should flag.

> On multi-tile parts it cannot really happen, or even if it happens 
>situation would simply be "why is only one tile showing". If we want to 
>cover this impossible/unlikely case then maybe like this:
>
>	if (!cnt || (errno && errno != ENOENT))
>		cnt = -errno;

If you agree to the above logic, then this condition should do the 
trick.

Regards,
Umesh
>
>>I had something like this in mind for the regression (and sorry this 
>>fell through the cracks)
>>
>>https://patchwork.freedesktop.org/patch/541406/?series=118973&rev=1
>
>Oh back in June!
>
>I think yours work too, in which case it's a matter of a style choice 
>with which one to go. I don't have a strong preference - above would 
>be a bit more compact, while I think it still succinctly expresses the 
>failure condition ("nothing found or unexpected error while probing 
>for remote tiles").
>
>Regards,
>
>Tvrtko
>
>>
>>Regards,
>>Umesh
>>
>>>
>>>    return cnt;
>>>}
>>>@@ -590,6 +592,8 @@ static int pmu_init(struct engines *engines)
>>>    engines->fd = -1;
>>>    engines->num_counters = 0;
>>>    engines->num_gts = get_num_gts(type);
>>>+    if (engines->num_gts <= 0)
>>>+        return -1;
>>>
>>>    engines->irq.config = I915_PMU_INTERRUPTS;
>>>    fd = _open_pmu(type, engines->num_counters, &engines->irq, 
>>>engines->fd);
>>>-- 
>>>2.39.2
>>>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH i-g-t 03/12] tools/intel_gpu_top: Restore user friendly error message
  2023-09-28 21:31       ` Umesh Nerlige Ramappa
@ 2023-09-29 11:11         ` Tvrtko Ursulin
  0 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-29 11:11 UTC (permalink / raw)
  To: Umesh Nerlige Ramappa; +Cc: igt-dev, Intel-gfx


On 28/09/2023 22:31, Umesh Nerlige Ramappa wrote:
> On Thu, Sep 28, 2023 at 09:16:23AM +0100, Tvrtko Ursulin wrote:
>>
>> On 27/09/2023 21:13, Umesh Nerlige Ramappa wrote:
>>> On Fri, Sep 22, 2023 at 02:44:28PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> We have a nice error message displayed when an user with insufficient
>>>> permissions tries to run the tool, but that got lost while Meteorlake
>>>> support was added. Bring it back in.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>>> ---
>>>> tools/intel_gpu_top.c | 10 +++++++---
>>>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>>>> index 87e9681e53b4..e01355f90458 100644
>>>> --- a/tools/intel_gpu_top.c
>>>> +++ b/tools/intel_gpu_top.c
>>>> @@ -554,9 +554,11 @@ static int get_num_gts(uint64_t type)
>>>>
>>>>         close(fd);
>>>>     }
>>>> -    assert(!errno || errno == ENOENT);
>>>> -    assert(cnt > 0);
>>>> -    errno = 0;
>>>> +
>>>> +    if (!cnt)
>>>> +        cnt = errno;
>>>> +    else
>>>> +        errno = 0;
>>>
>>> ENOENT is the only way this logic is checking for num_gts.
>>>
>>> In this case error is propagated only if cnt == 0. What if cnt=1 and 
>>> we get an error (other than ENOENT)? Should we ignore that?
>>
>> If cnt >= 1 then at least one tile was found so the errno happened 
>> while probing for further tiles. So on single tile parts it can be 
>> ignored.
> 
> I am actually only referring to single tile parts. The for loop iterates 
> over MAX_GTs (4), so I am expecting an ENOENT from a single tile part 
> when cnt >= 1. Anything else is an error/failure that we should flag.

Yes I think that worked fine in v1. Only thing I did not do is bother to 
pass on the unexpected errno on multi-tile. Anyway, I have sent v2 with 
the below condition.

Regards,

Tvrtko

> 
>> On multi-tile parts it cannot really happen, or even if it happens 
>> situation would simply be "why is only one tile showing". If we want 
>> to cover this impossible/unlikely case then maybe like this:
>>
>>     if (!cnt || (errno && errno != ENOENT))
>>         cnt = -errno;
> 
> If you agree to the above logic, then this condition should do the trick.
> 
> Regards,
> Umesh
>>
>>> I had something like this in mind for the regression (and sorry this 
>>> fell through the cracks)
>>>
>>> https://patchwork.freedesktop.org/patch/541406/?series=118973&rev=1
>>
>> Oh back in June!
>>
>> I think yours work too, in which case it's a matter of a style choice 
>> with which one to go. I don't have a strong preference - above would 
>> be a bit more compact, while I think it still succinctly expresses the 
>> failure condition ("nothing found or unexpected error while probing 
>> for remote tiles").
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Regards,
>>> Umesh
>>>
>>>>
>>>>     return cnt;
>>>> }
>>>> @@ -590,6 +592,8 @@ static int pmu_init(struct engines *engines)
>>>>     engines->fd = -1;
>>>>     engines->num_counters = 0;
>>>>     engines->num_gts = get_num_gts(type);
>>>> +    if (engines->num_gts <= 0)
>>>> +        return -1;
>>>>
>>>>     engines->irq.config = I915_PMU_INTERRUPTS;
>>>>     fd = _open_pmu(type, engines->num_counters, &engines->irq, 
>>>> engines->fd);
>>>> -- 
>>>> 2.39.2
>>>>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc
@ 2023-09-29 12:25 Tvrtko Ursulin
  0 siblings, 0 replies; 19+ messages in thread
From: Tvrtko Ursulin @ 2023-09-29 12:25 UTC (permalink / raw)
  To: igt-dev, Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Some basic testst for fdinfo memory stats, intel_gpu_top memory stats support
(first draft) and a couple fixlets.

Tvrtko Ursulin (12):
  tests/i915/drm_fdinfo: Check engine info is supported
  tests/i915/drm_fdinfo: Stress test context close versus fdinfo reads
  tests/i915/drm_fdinfo: Add some memory info tests
  tools/intel_gpu_top: Fix clients header width when no clients
  tools/intel_gpu_top: Fix client layout on first sample period
  tools/intel_gpu_top: Optimise interactive display a bit
  lib/igt_drm_fdinfo: Copy over region map name on match
  lib/igt_drm_clients: Fix client id type confusion
  lib/igt_drm_clients: Allow passing in the memory region map
  tools/intel_gpu_top: Fully wrap clients operations
  tools/intel_gpu_top: Add per client memory info
  tools/intel_gpu_top: Add ability to show memory region breakdown

 lib/igt_drm_clients.c    |   7 +-
 lib/igt_drm_clients.h    |   5 +-
 lib/igt_drm_fdinfo.c     |   4 +
 man/intel_gpu_top.rst    |   4 +
 tests/intel/drm_fdinfo.c | 286 +++++++++++++++++++++++++++++++++++++++
 tools/gputop.c           |   4 +-
 tools/intel_gpu_top.c    | 250 +++++++++++++++++++++++++++++-----
 7 files changed, 518 insertions(+), 42 deletions(-)

-- 
2.39.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2023-09-29 12:25 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-22 13:44 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 01/12] tests/i915/drm_fdinfo: Stress test context close versus fdinfo reads Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 02/12] tests/i915/drm_fdinfo: Add some memory info tests Tvrtko Ursulin
2023-09-27 13:20   ` [Intel-gfx] [PATCH i-g-t v2 " Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 03/12] tools/intel_gpu_top: Restore user friendly error message Tvrtko Ursulin
2023-09-27 20:13   ` Umesh Nerlige Ramappa
2023-09-28  8:16     ` Tvrtko Ursulin
2023-09-28 21:31       ` Umesh Nerlige Ramappa
2023-09-29 11:11         ` Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 04/12] tools/intel_gpu_top: Fix clients header width when no clients Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 05/12] tools/intel_gpu_top: Fix client layout on first sample period Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 06/12] tools/intel_gpu_top: Optimise interactive display a bit Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 07/12] lib/igt_drm_fdinfo: Copy over region map name on match Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 08/12] lib/igt_drm_clients: Fix client id type confusion Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 09/12] lib/igt_drm_clients: Allow passing in the memory region map Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 10/12] tools/intel_gpu_top: Fully wrap clients operations Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 11/12] tools/intel_gpu_top: Add per client memory info Tvrtko Ursulin
2023-09-22 13:44 ` [Intel-gfx] [PATCH i-g-t 12/12] tools/intel_gpu_top: Add ability to show memory region breakdown Tvrtko Ursulin
2023-09-29 12:25 [Intel-gfx] [PATCH i-g-t 00/12] fdinfo tests, intel_gpu_top memory support, etc Tvrtko Ursulin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).