[Intel-gfx] [PATCH i-g-t 0/3] gputop memory usage

public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed

* [Intel-gfx] [PATCH i-g-t 0/3] gputop memory usage
@ 2023-07-05 16:31 Tvrtko Ursulin
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 1/3] lib/igt_drm_fdinfo: Parse " Tvrtko Ursulin
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-05 16:31 UTC (permalink / raw)
  To: igt-dev, Intel-gfx; +Cc: Rob Clark

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

Same general approach as with engine utilisation, adding parsing of the relevant
fdinfo fields and plumbing to client discovery helpers to finally present some
of the available data in gputop:

DRM minor 0
 PID      MEM      RSS  render    copy     video   NAME
8098     124M     124M |███████||       ||       ||       | neverball
 888      76M      75M |▊      ||       ||       ||       | Xorg
 966      12M      12M |       ||       ||       ||       | xfwm4

There is probably scope to consolidate some of the igt_drm_fdinfo code and to
improve on the presentation but this is a start.

For now, out of the available memory categories, only total and resident are
displayed, and also all discovered memory regions are summed up and shown under
a single heading.

Cc: Rob Clark <robdclark@chromium.org>

Tvrtko Ursulin (3):
  lib/igt_drm_fdinfo: Parse memory usage
  lib/igt_drm_clients: Store memory info in the client
  gputop: Add memory information

 lib/igt_drm_clients.c   |  34 +++++++++-
 lib/igt_drm_clients.h   |  11 ++++
 lib/igt_drm_fdinfo.c    | 142 ++++++++++++++++++++++++++++++++++++++--
 lib/igt_drm_fdinfo.h    |  24 ++++++-
 tests/i915/drm_fdinfo.c |   8 +--
 tools/gputop.c          |  34 +++++++++-
 tools/intel_gpu_top.c   |   2 +-
 7 files changed, 240 insertions(+), 15 deletions(-)

-- 
2.39.2


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

* [Intel-gfx] [PATCH i-g-t 1/3] lib/igt_drm_fdinfo: Parse memory usage
  2023-07-05 16:31 [Intel-gfx] [PATCH i-g-t 0/3] gputop memory usage Tvrtko Ursulin
@ 2023-07-05 16:31 ` Tvrtko Ursulin
  2023-07-26 17:28   ` Kamil Konieczny
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client Tvrtko Ursulin
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 3/3] gputop: Add memory information Tvrtko Ursulin
  2 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-05 16:31 UTC (permalink / raw)
  To: igt-dev, Intel-gfx; +Cc: Rob Clark

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

Add parsing and memory storage for the memory usage related fdinfo stats.

Uses the same approach as the engine utilization code by either auto-
discovering different memory regions, or allowing for the caller to pass
in a map with predefined index to name relationship.

Co-developed-by: Rob Clark <robdclark@chromium.org>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_drm_clients.c   |   3 +-
 lib/igt_drm_fdinfo.c    | 142 ++++++++++++++++++++++++++++++++++++++--
 lib/igt_drm_fdinfo.h    |  24 ++++++-
 tests/i915/drm_fdinfo.c |   8 +--
 tools/intel_gpu_top.c   |   2 +-
 5 files changed, 165 insertions(+), 14 deletions(-)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index f0294ba81c42..fdea42752a81 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -491,7 +491,8 @@ 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))
+						    name_map, map_entries,
+						    NULL, 0))
 				continue;
 
 			if (filter_client && !filter_client(clients, &info))
diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
index b5f8a8679a71..d08632dfb690 100644
--- a/lib/igt_drm_fdinfo.c
+++ b/lib/igt_drm_fdinfo.c
@@ -124,13 +124,81 @@ static const char *find_kv(const char *buf, const char *key, size_t keylen)
 	return *p ? p : NULL;
 }
 
+static int parse_region(char *line, struct drm_client_fdinfo *info,
+			size_t prefix_len,
+			const char **region_map, unsigned int region_entries,
+			uint64_t *val)
+{
+	char *name, *p, *unit = NULL;
+	ssize_t name_len;
+	int found = -1;
+	unsigned int i;
+
+	p = index(line, ':');
+	if (!p || p == line)
+		return -1;
+
+	name_len = p - line - prefix_len;
+	if (name_len < 1)
+		return -1;
+
+	name = line + prefix_len;
+
+	if (region_map) {
+		for (i = 0; i < region_entries; i++) {
+			if (!strncmp(name, region_map[i], name_len)) {
+				found = i;
+				break;
+			}
+		}
+	} else {
+		for (i = 0; i < info->num_regions; i++) {
+			if (!strncmp(name, info->region_names[i], name_len)) {
+				found = i;
+				break;
+			}
+		}
+
+		if (found < 0) {
+			assert((info->num_regions + 1) < ARRAY_SIZE(info->region_names));
+			assert((strlen(name) + 1) < sizeof(info->region_names[0]));
+			strncpy(info->region_names[info->num_regions], name, name_len);
+			found = info->num_regions;
+		}
+	}
+
+	if (found < 0)
+		goto out;
+
+	while (*++p && isspace(*p));
+	*val = strtoull(p, NULL, 10);
+
+	p = index(p, ' ');
+	if (!p)
+		goto out;
+
+	unit = ++p;
+	if (!strcmp(unit, "KiB")) {
+		*val *= 1024;
+	} else if (!strcmp(unit, "MiB")) {
+		*val *= 1024 * 1024;
+	} else if (!strcmp(unit, "GiB")) {
+		*val *= 1024 * 1024 * 1024;
+	}
+
+out:
+	return found;
+}
+
 unsigned int
 __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
-		       const char **name_map, unsigned int map_entries)
+		       const char **name_map, unsigned int map_entries,
+		       const char **region_map, unsigned int region_entries)
 {
+	bool regions_found[DRM_CLIENT_FDINFO_MAX_REGIONS] = { };
+	unsigned int good = 0, num_capacity = 0;
 	char buf[4096], *_buf = buf;
 	char *l, *ctx = NULL;
-	unsigned int good = 0, num_capacity = 0;
 	size_t count;
 
 	count = read_fdinfo(buf, sizeof(buf), dir, fd);
@@ -173,18 +241,79 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
 				info->capacity[idx] = val;
 				num_capacity++;
 			}
+		} else if (!strncmp(l, "drm-total-", 10)) {
+			idx = parse_region(l, info, strlen("drm-total-"),
+					   region_map, region_entries, &val);
+			if (idx >= 0) {
+				info->region_mem[idx].total = val;
+				if (!regions_found[idx]) {
+					info->num_regions++;
+					regions_found[idx] = true;
+					if (idx > info->last_region_index)
+						info->last_region_index = idx;
+				}
+			}
+		} else if (!strncmp(l, "drm-shared-", 11)) {
+			idx = parse_region(l, info, strlen("drm-shared-"),
+					   region_map, region_entries, &val);
+			if (idx >= 0) {
+				info->region_mem[idx].shared = val;
+				if (!regions_found[idx]) {
+					info->num_regions++;
+					regions_found[idx] = true;
+					if (idx > info->last_region_index)
+						info->last_region_index = idx;
+				}
+			}
+		} else if (!strncmp(l, "drm-resident-", 13)) {
+			idx = parse_region(l, info, strlen("drm-resident-"),
+					   region_map, region_entries, &val);
+			if (idx >= 0) {
+				info->region_mem[idx].resident = val;
+				if (!regions_found[idx]) {
+					info->num_regions++;
+					regions_found[idx] = true;
+					if (idx > info->last_region_index)
+						info->last_region_index = idx;
+				}
+			}
+		} else if (!strncmp(l, "drm-purgeable-", 14)) {
+			idx = parse_region(l, info, strlen("drm-purgeable-"),
+					   region_map, region_entries, &val);
+			if (idx >= 0) {
+				info->region_mem[idx].purgeable = val;
+				if (!regions_found[idx]) {
+					info->num_regions++;
+					regions_found[idx] = true;
+					if (idx > info->last_region_index)
+						info->last_region_index = idx;
+				}
+			}
+		} else if (!strncmp(l, "drm-active-", 11)) {
+			idx = parse_region(l, info, strlen("drm-active-"),
+					   region_map, region_entries, &val);
+			if (idx >= 0) {
+				info->region_mem[idx].active = val;
+				if (!regions_found[idx]) {
+					info->num_regions++;
+					regions_found[idx] = true;
+					if (idx > info->last_region_index)
+						info->last_region_index = idx;
+				}
+			}
 		}
 	}
 
-	if (good < 2 || !info->num_engines)
+	if (good < 2 || (!info->num_engines && !info->num_regions))
 		return 0; /* fdinfo format not as expected */
 
-	return good + info->num_engines + num_capacity;
+	return good + info->num_engines + num_capacity + info->num_regions;
 }
 
 unsigned int
 igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
-		     const char **name_map, unsigned int map_entries)
+		     const char **name_map, unsigned int map_entries,
+		     const char **region_map, unsigned int region_entries)
 {
 	unsigned int res;
 	char fd[64];
@@ -198,7 +327,8 @@ igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
 	if (dir < 0)
 		return false;
 
-	res = __igt_parse_drm_fdinfo(dir, fd, info, name_map, map_entries);
+	res = __igt_parse_drm_fdinfo(dir, fd, info, name_map, map_entries,
+				     region_map, region_entries);
 
 	close(dir);
 
diff --git a/lib/igt_drm_fdinfo.h b/lib/igt_drm_fdinfo.h
index 6284e05e868a..1999c4f2b857 100644
--- a/lib/igt_drm_fdinfo.h
+++ b/lib/igt_drm_fdinfo.h
@@ -30,8 +30,17 @@
 #include <stdint.h>
 #include <stdbool.h>
 
+#define DRM_CLIENT_FDINFO_MAX_REGIONS 16
 #define DRM_CLIENT_FDINFO_MAX_ENGINES 16
 
+struct drm_client_meminfo {
+	uint64_t total;
+	uint64_t shared;
+	uint64_t resident;
+	uint64_t purgeable;
+	uint64_t active;
+};
+
 struct drm_client_fdinfo {
 	char driver[128];
 	char pdev[128];
@@ -42,6 +51,11 @@ struct drm_client_fdinfo {
 	unsigned int capacity[DRM_CLIENT_FDINFO_MAX_ENGINES];
 	char names[DRM_CLIENT_FDINFO_MAX_ENGINES][256];
 	uint64_t busy[DRM_CLIENT_FDINFO_MAX_ENGINES];
+
+	unsigned int num_regions;
+	unsigned int last_region_index;
+	char region_names[DRM_CLIENT_FDINFO_MAX_REGIONS][256];
+	struct drm_client_meminfo region_mem[DRM_CLIENT_FDINFO_MAX_REGIONS];
 };
 
 /**
@@ -51,13 +65,16 @@ struct drm_client_fdinfo {
  * @info: Structure to populate with read data. Must be zeroed.
  * @name_map: Optional array of strings representing engine names
  * @map_entries: Number of strings in the names array
+ * @region_map: Optional array of strings representing memory regions
+ * @region_entries: Number of strings in the region map
  *
  * Returns the number of valid drm fdinfo keys found or zero if not all
  * mandatory keys were present or no engines found.
  */
 unsigned int
 igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
-		     const char **name_map, unsigned int map_entries);
+		     const char **name_map, unsigned int map_entries,
+		     const char **region_map, unsigned int region_entries);
 
 /**
  * __igt_parse_drm_fdinfo: Parses the drm fdinfo file
@@ -67,6 +84,8 @@ igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
  * @info: Structure to populate with read data. Must be zeroed.
  * @name_map: Optional array of strings representing engine names
  * @map_entries: Number of strings in the names array
+ * @region_map: Optional array of strings representing memory regions
+ * @region_entries: Number of strings in the region map
  *
  * Returns the number of valid drm fdinfo keys found or zero if not all
  * mandatory keys were present or no engines found.
@@ -74,6 +93,7 @@ igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
 unsigned int
 __igt_parse_drm_fdinfo(int dir, const char *fd,
 		       struct drm_client_fdinfo *info,
-		       const char **name_map, unsigned int map_entries);
+		       const char **name_map, unsigned int map_entries,
+		       const char **region_map, unsigned int region_entries);
 
 #endif /* IGT_DRM_FDINFO_H */
diff --git a/tests/i915/drm_fdinfo.c b/tests/i915/drm_fdinfo.c
index c0e0ba1905f1..01526c0e19de 100644
--- a/tests/i915/drm_fdinfo.c
+++ b/tests/i915/drm_fdinfo.c
@@ -104,7 +104,7 @@ static void basics(int i915, unsigned int num_classes)
 	unsigned int ret;
 
 	ret = igt_parse_drm_fdinfo(i915, &info, engine_map,
-				   ARRAY_SIZE(engine_map));
+				   ARRAY_SIZE(engine_map), NULL, 0);
 	igt_assert(ret);
 
 	igt_assert(!strcmp(info.driver, "i915"));
@@ -236,7 +236,7 @@ static uint64_t read_busy(int i915, unsigned int class)
 	struct drm_client_fdinfo info = { };
 
 	igt_assert(igt_parse_drm_fdinfo(i915, &info, engine_map,
-					ARRAY_SIZE(engine_map)));
+					ARRAY_SIZE(engine_map), NULL, 0));
 
 	return info.busy[class];
 }
@@ -326,7 +326,7 @@ static void read_busy_all(int i915, uint64_t *val)
 	struct drm_client_fdinfo info = { };
 
 	igt_assert(igt_parse_drm_fdinfo(i915, &info, engine_map,
-					ARRAY_SIZE(engine_map)));
+					ARRAY_SIZE(engine_map), NULL, 0));
 
 	memcpy(val, info.busy, sizeof(info.busy));
 }
@@ -797,7 +797,7 @@ igt_main
 		i915 = __drm_open_driver(DRIVER_INTEL);
 
 		igt_require_gem(i915);
-		igt_require(igt_parse_drm_fdinfo(i915, &info, NULL, 0));
+		igt_require(igt_parse_drm_fdinfo(i915, &info, NULL, 0, NULL, 0));
 
 		ctx = intel_ctx_create_all_physical(i915);
 
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
index cef1d3c7fa9f..87e9681e53b4 100644
--- a/tools/intel_gpu_top.c
+++ b/tools/intel_gpu_top.c
@@ -2307,7 +2307,7 @@ static bool has_drm_fdinfo(const struct igt_device_card *card)
 	if (fd < 0)
 		return false;
 
-	cnt = igt_parse_drm_fdinfo(fd, &info, NULL, 0);
+	cnt = igt_parse_drm_fdinfo(fd, &info, NULL, 0, NULL, 0);
 
 	close(fd);
 
-- 
2.39.2


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

* Re: [Intel-gfx] [PATCH i-g-t 1/3] lib/igt_drm_fdinfo: Parse memory usage
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 1/3] lib/igt_drm_fdinfo: Parse " Tvrtko Ursulin
@ 2023-07-26 17:28   ` Kamil Konieczny
  2023-07-27  8:51     ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Kamil Konieczny @ 2023-07-26 17:28 UTC (permalink / raw)
  To: igt-dev; +Cc: Rob Clark, Intel-gfx

Hi Tvrtko,

please check your patches with Linux kernel script checkpatch.pl
Some of it's warnings seems strange, like:

WARNING: Co-developed-by and Signed-off-by: name/email do not match
#12:
Co-developed-by: Rob Clark <robdclark@chromium.org>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

On 2023-07-05 at 17:31:03 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Add parsing and memory storage for the memory usage related fdinfo stats.
> 
> Uses the same approach as the engine utilization code by either auto-
> discovering different memory regions, or allowing for the caller to pass
> in a map with predefined index to name relationship.
> 
> Co-developed-by: Rob Clark <robdclark@chromium.org>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  lib/igt_drm_clients.c   |   3 +-
>  lib/igt_drm_fdinfo.c    | 142 ++++++++++++++++++++++++++++++++++++++--
>  lib/igt_drm_fdinfo.h    |  24 ++++++-
>  tests/i915/drm_fdinfo.c |   8 +--
>  tools/intel_gpu_top.c   |   2 +-
>  5 files changed, 165 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index f0294ba81c42..fdea42752a81 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -491,7 +491,8 @@ 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))
> +						    name_map, map_entries,
> +						    NULL, 0))
>  				continue;
>  
>  			if (filter_client && !filter_client(clients, &info))
> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
> index b5f8a8679a71..d08632dfb690 100644
> --- a/lib/igt_drm_fdinfo.c
> +++ b/lib/igt_drm_fdinfo.c
> @@ -124,13 +124,81 @@ static const char *find_kv(const char *buf, const char *key, size_t keylen)
>  	return *p ? p : NULL;
>  }
>  
> +static int parse_region(char *line, struct drm_client_fdinfo *info,
> +			size_t prefix_len,
> +			const char **region_map, unsigned int region_entries,
> +			uint64_t *val)
> +{
> +	char *name, *p, *unit = NULL;
> +	ssize_t name_len;
> +	int found = -1;
> +	unsigned int i;
> +
> +	p = index(line, ':');
> +	if (!p || p == line)
> +		return -1;
> +
> +	name_len = p - line - prefix_len;
> +	if (name_len < 1)
> +		return -1;
> +
> +	name = line + prefix_len;
> +
> +	if (region_map) {
> +		for (i = 0; i < region_entries; i++) {
> +			if (!strncmp(name, region_map[i], name_len)) {
> +				found = i;
> +				break;
> +			}
> +		}
> +	} else {
> +		for (i = 0; i < info->num_regions; i++) {
> +			if (!strncmp(name, info->region_names[i], name_len)) {
> +				found = i;
> +				break;
> +			}
> +		}
> +
> +		if (found < 0) {
> +			assert((info->num_regions + 1) < ARRAY_SIZE(info->region_names));
> +			assert((strlen(name) + 1) < sizeof(info->region_names[0]));
> +			strncpy(info->region_names[info->num_regions], name, name_len);
> +			found = info->num_regions;
> +		}
> +	}
> +
> +	if (found < 0)
> +		goto out;
> +
> +	while (*++p && isspace(*p));
---------------------------------- ^
According to checkpatch:
	while (*++p && isspace(*p))
		;

> +	*val = strtoull(p, NULL, 10);
> +
> +	p = index(p, ' ');
> +	if (!p)
> +		goto out;
> +
> +	unit = ++p;
> +	if (!strcmp(unit, "KiB")) {
> +		*val *= 1024;
> +	} else if (!strcmp(unit, "MiB")) {
------- ^^^^^^
No need for separate 'else':
	if (!strcmp(unit, "MiB")) {

> +		*val *= 1024 * 1024;
> +	} else if (!strcmp(unit, "GiB")) {
------- ^^^^^^
Same here.

Regards,
Kamil

> +		*val *= 1024 * 1024 * 1024;
> +	}
> +
> +out:
> +	return found;
> +}
> +
>  unsigned int
>  __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
> -		       const char **name_map, unsigned int map_entries)
> +		       const char **name_map, unsigned int map_entries,
> +		       const char **region_map, unsigned int region_entries)
>  {
> +	bool regions_found[DRM_CLIENT_FDINFO_MAX_REGIONS] = { };
> +	unsigned int good = 0, num_capacity = 0;
>  	char buf[4096], *_buf = buf;
>  	char *l, *ctx = NULL;
> -	unsigned int good = 0, num_capacity = 0;
>  	size_t count;
>  
>  	count = read_fdinfo(buf, sizeof(buf), dir, fd);
> @@ -173,18 +241,79 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>  				info->capacity[idx] = val;
>  				num_capacity++;
>  			}
> +		} else if (!strncmp(l, "drm-total-", 10)) {
> +			idx = parse_region(l, info, strlen("drm-total-"),
> +					   region_map, region_entries, &val);
> +			if (idx >= 0) {
> +				info->region_mem[idx].total = val;
> +				if (!regions_found[idx]) {
> +					info->num_regions++;
> +					regions_found[idx] = true;
> +					if (idx > info->last_region_index)
> +						info->last_region_index = idx;
> +				}
> +			}
> +		} else if (!strncmp(l, "drm-shared-", 11)) {
> +			idx = parse_region(l, info, strlen("drm-shared-"),
> +					   region_map, region_entries, &val);
> +			if (idx >= 0) {
> +				info->region_mem[idx].shared = val;
> +				if (!regions_found[idx]) {
> +					info->num_regions++;
> +					regions_found[idx] = true;
> +					if (idx > info->last_region_index)
> +						info->last_region_index = idx;
> +				}
> +			}
> +		} else if (!strncmp(l, "drm-resident-", 13)) {
> +			idx = parse_region(l, info, strlen("drm-resident-"),
> +					   region_map, region_entries, &val);
> +			if (idx >= 0) {
> +				info->region_mem[idx].resident = val;
> +				if (!regions_found[idx]) {
> +					info->num_regions++;
> +					regions_found[idx] = true;
> +					if (idx > info->last_region_index)
> +						info->last_region_index = idx;
> +				}
> +			}
> +		} else if (!strncmp(l, "drm-purgeable-", 14)) {
> +			idx = parse_region(l, info, strlen("drm-purgeable-"),
> +					   region_map, region_entries, &val);
> +			if (idx >= 0) {
> +				info->region_mem[idx].purgeable = val;
> +				if (!regions_found[idx]) {
> +					info->num_regions++;
> +					regions_found[idx] = true;
> +					if (idx > info->last_region_index)
> +						info->last_region_index = idx;
> +				}
> +			}
> +		} else if (!strncmp(l, "drm-active-", 11)) {
> +			idx = parse_region(l, info, strlen("drm-active-"),
> +					   region_map, region_entries, &val);
> +			if (idx >= 0) {
> +				info->region_mem[idx].active = val;
> +				if (!regions_found[idx]) {
> +					info->num_regions++;
> +					regions_found[idx] = true;
> +					if (idx > info->last_region_index)
> +						info->last_region_index = idx;
> +				}
> +			}
>  		}
>  	}
>  
> -	if (good < 2 || !info->num_engines)
> +	if (good < 2 || (!info->num_engines && !info->num_regions))
>  		return 0; /* fdinfo format not as expected */
>  
> -	return good + info->num_engines + num_capacity;
> +	return good + info->num_engines + num_capacity + info->num_regions;
>  }
>  
>  unsigned int
>  igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
> -		     const char **name_map, unsigned int map_entries)
> +		     const char **name_map, unsigned int map_entries,
> +		     const char **region_map, unsigned int region_entries)
>  {
>  	unsigned int res;
>  	char fd[64];
> @@ -198,7 +327,8 @@ igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
>  	if (dir < 0)
>  		return false;
>  
> -	res = __igt_parse_drm_fdinfo(dir, fd, info, name_map, map_entries);
> +	res = __igt_parse_drm_fdinfo(dir, fd, info, name_map, map_entries,
> +				     region_map, region_entries);
>  
>  	close(dir);
>  
> diff --git a/lib/igt_drm_fdinfo.h b/lib/igt_drm_fdinfo.h
> index 6284e05e868a..1999c4f2b857 100644
> --- a/lib/igt_drm_fdinfo.h
> +++ b/lib/igt_drm_fdinfo.h
> @@ -30,8 +30,17 @@
>  #include <stdint.h>
>  #include <stdbool.h>
>  
> +#define DRM_CLIENT_FDINFO_MAX_REGIONS 16
>  #define DRM_CLIENT_FDINFO_MAX_ENGINES 16
>  
> +struct drm_client_meminfo {
> +	uint64_t total;
> +	uint64_t shared;
> +	uint64_t resident;
> +	uint64_t purgeable;
> +	uint64_t active;
> +};
> +
>  struct drm_client_fdinfo {
>  	char driver[128];
>  	char pdev[128];
> @@ -42,6 +51,11 @@ struct drm_client_fdinfo {
>  	unsigned int capacity[DRM_CLIENT_FDINFO_MAX_ENGINES];
>  	char names[DRM_CLIENT_FDINFO_MAX_ENGINES][256];
>  	uint64_t busy[DRM_CLIENT_FDINFO_MAX_ENGINES];
> +
> +	unsigned int num_regions;
> +	unsigned int last_region_index;
> +	char region_names[DRM_CLIENT_FDINFO_MAX_REGIONS][256];
> +	struct drm_client_meminfo region_mem[DRM_CLIENT_FDINFO_MAX_REGIONS];
>  };
>  
>  /**
> @@ -51,13 +65,16 @@ struct drm_client_fdinfo {
>   * @info: Structure to populate with read data. Must be zeroed.
>   * @name_map: Optional array of strings representing engine names
>   * @map_entries: Number of strings in the names array
> + * @region_map: Optional array of strings representing memory regions
> + * @region_entries: Number of strings in the region map
>   *
>   * Returns the number of valid drm fdinfo keys found or zero if not all
>   * mandatory keys were present or no engines found.
>   */
>  unsigned int
>  igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
> -		     const char **name_map, unsigned int map_entries);
> +		     const char **name_map, unsigned int map_entries,
> +		     const char **region_map, unsigned int region_entries);
>  
>  /**
>   * __igt_parse_drm_fdinfo: Parses the drm fdinfo file
> @@ -67,6 +84,8 @@ igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
>   * @info: Structure to populate with read data. Must be zeroed.
>   * @name_map: Optional array of strings representing engine names
>   * @map_entries: Number of strings in the names array
> + * @region_map: Optional array of strings representing memory regions
> + * @region_entries: Number of strings in the region map
>   *
>   * Returns the number of valid drm fdinfo keys found or zero if not all
>   * mandatory keys were present or no engines found.
> @@ -74,6 +93,7 @@ igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
>  unsigned int
>  __igt_parse_drm_fdinfo(int dir, const char *fd,
>  		       struct drm_client_fdinfo *info,
> -		       const char **name_map, unsigned int map_entries);
> +		       const char **name_map, unsigned int map_entries,
> +		       const char **region_map, unsigned int region_entries);
>  
>  #endif /* IGT_DRM_FDINFO_H */
> diff --git a/tests/i915/drm_fdinfo.c b/tests/i915/drm_fdinfo.c
> index c0e0ba1905f1..01526c0e19de 100644
> --- a/tests/i915/drm_fdinfo.c
> +++ b/tests/i915/drm_fdinfo.c
> @@ -104,7 +104,7 @@ static void basics(int i915, unsigned int num_classes)
>  	unsigned int ret;
>  
>  	ret = igt_parse_drm_fdinfo(i915, &info, engine_map,
> -				   ARRAY_SIZE(engine_map));
> +				   ARRAY_SIZE(engine_map), NULL, 0);
>  	igt_assert(ret);
>  
>  	igt_assert(!strcmp(info.driver, "i915"));
> @@ -236,7 +236,7 @@ static uint64_t read_busy(int i915, unsigned int class)
>  	struct drm_client_fdinfo info = { };
>  
>  	igt_assert(igt_parse_drm_fdinfo(i915, &info, engine_map,
> -					ARRAY_SIZE(engine_map)));
> +					ARRAY_SIZE(engine_map), NULL, 0));
>  
>  	return info.busy[class];
>  }
> @@ -326,7 +326,7 @@ static void read_busy_all(int i915, uint64_t *val)
>  	struct drm_client_fdinfo info = { };
>  
>  	igt_assert(igt_parse_drm_fdinfo(i915, &info, engine_map,
> -					ARRAY_SIZE(engine_map)));
> +					ARRAY_SIZE(engine_map), NULL, 0));
>  
>  	memcpy(val, info.busy, sizeof(info.busy));
>  }
> @@ -797,7 +797,7 @@ igt_main
>  		i915 = __drm_open_driver(DRIVER_INTEL);
>  
>  		igt_require_gem(i915);
> -		igt_require(igt_parse_drm_fdinfo(i915, &info, NULL, 0));
> +		igt_require(igt_parse_drm_fdinfo(i915, &info, NULL, 0, NULL, 0));
>  
>  		ctx = intel_ctx_create_all_physical(i915);
>  
> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
> index cef1d3c7fa9f..87e9681e53b4 100644
> --- a/tools/intel_gpu_top.c
> +++ b/tools/intel_gpu_top.c
> @@ -2307,7 +2307,7 @@ static bool has_drm_fdinfo(const struct igt_device_card *card)
>  	if (fd < 0)
>  		return false;
>  
> -	cnt = igt_parse_drm_fdinfo(fd, &info, NULL, 0);
> +	cnt = igt_parse_drm_fdinfo(fd, &info, NULL, 0, NULL, 0);
>  
>  	close(fd);
>  
> -- 
> 2.39.2
> 

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

* Re: [Intel-gfx] [PATCH i-g-t 1/3] lib/igt_drm_fdinfo: Parse memory usage
  2023-07-26 17:28   ` Kamil Konieczny
@ 2023-07-27  8:51     ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-27  8:51 UTC (permalink / raw)
  To: Kamil Konieczny, igt-dev, Intel-gfx, Rob Clark


On 26/07/2023 18:28, Kamil Konieczny wrote:
> Hi Tvrtko,
> 
> please check your patches with Linux kernel script checkpatch.pl
> Some of it's warnings seems strange, like:
> 
> WARNING: Co-developed-by and Signed-off-by: name/email do not match
> #12:
> Co-developed-by: Rob Clark <robdclark@chromium.org>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Done.

> On 2023-07-05 at 17:31:03 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Add parsing and memory storage for the memory usage related fdinfo stats.
>>
>> Uses the same approach as the engine utilization code by either auto-
>> discovering different memory regions, or allowing for the caller to pass
>> in a map with predefined index to name relationship.
>>
>> Co-developed-by: Rob Clark <robdclark@chromium.org>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   lib/igt_drm_clients.c   |   3 +-
>>   lib/igt_drm_fdinfo.c    | 142 ++++++++++++++++++++++++++++++++++++++--
>>   lib/igt_drm_fdinfo.h    |  24 ++++++-
>>   tests/i915/drm_fdinfo.c |   8 +--
>>   tools/intel_gpu_top.c   |   2 +-
>>   5 files changed, 165 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
>> index f0294ba81c42..fdea42752a81 100644
>> --- a/lib/igt_drm_clients.c
>> +++ b/lib/igt_drm_clients.c
>> @@ -491,7 +491,8 @@ 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))
>> +						    name_map, map_entries,
>> +						    NULL, 0))
>>   				continue;
>>   
>>   			if (filter_client && !filter_client(clients, &info))
>> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
>> index b5f8a8679a71..d08632dfb690 100644
>> --- a/lib/igt_drm_fdinfo.c
>> +++ b/lib/igt_drm_fdinfo.c
>> @@ -124,13 +124,81 @@ static const char *find_kv(const char *buf, const char *key, size_t keylen)
>>   	return *p ? p : NULL;
>>   }
>>   
>> +static int parse_region(char *line, struct drm_client_fdinfo *info,
>> +			size_t prefix_len,
>> +			const char **region_map, unsigned int region_entries,
>> +			uint64_t *val)
>> +{
>> +	char *name, *p, *unit = NULL;
>> +	ssize_t name_len;
>> +	int found = -1;
>> +	unsigned int i;
>> +
>> +	p = index(line, ':');
>> +	if (!p || p == line)
>> +		return -1;
>> +
>> +	name_len = p - line - prefix_len;
>> +	if (name_len < 1)
>> +		return -1;
>> +
>> +	name = line + prefix_len;
>> +
>> +	if (region_map) {
>> +		for (i = 0; i < region_entries; i++) {
>> +			if (!strncmp(name, region_map[i], name_len)) {
>> +				found = i;
>> +				break;
>> +			}
>> +		}
>> +	} else {
>> +		for (i = 0; i < info->num_regions; i++) {
>> +			if (!strncmp(name, info->region_names[i], name_len)) {
>> +				found = i;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (found < 0) {
>> +			assert((info->num_regions + 1) < ARRAY_SIZE(info->region_names));
>> +			assert((strlen(name) + 1) < sizeof(info->region_names[0]));
>> +			strncpy(info->region_names[info->num_regions], name, name_len);
>> +			found = info->num_regions;
>> +		}
>> +	}
>> +
>> +	if (found < 0)
>> +		goto out;
>> +
>> +	while (*++p && isspace(*p));
> ---------------------------------- ^
> According to checkpatch:
> 	while (*++p && isspace(*p))
> 		;

Done.

> 
>> +	*val = strtoull(p, NULL, 10);
>> +
>> +	p = index(p, ' ');
>> +	if (!p)
>> +		goto out;
>> +
>> +	unit = ++p;
>> +	if (!strcmp(unit, "KiB")) {
>> +		*val *= 1024;
>> +	} else if (!strcmp(unit, "MiB")) {
> ------- ^^^^^^
> No need for separate 'else':
> 	if (!strcmp(unit, "MiB")) {
> 
>> +		*val *= 1024 * 1024;
>> +	} else if (!strcmp(unit, "GiB")) {
> ------- ^^^^^^
> Same here.

We don't want to run strcmp multiple times for no reason so I opted to 
leave as is.

Regards,

Tvrtko

> 
> Regards,
> Kamil
> 
>> +		*val *= 1024 * 1024 * 1024;
>> +	}
>> +
>> +out:
>> +	return found;
>> +}
>> +
>>   unsigned int
>>   __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>> -		       const char **name_map, unsigned int map_entries)
>> +		       const char **name_map, unsigned int map_entries,
>> +		       const char **region_map, unsigned int region_entries)
>>   {
>> +	bool regions_found[DRM_CLIENT_FDINFO_MAX_REGIONS] = { };
>> +	unsigned int good = 0, num_capacity = 0;
>>   	char buf[4096], *_buf = buf;
>>   	char *l, *ctx = NULL;
>> -	unsigned int good = 0, num_capacity = 0;
>>   	size_t count;
>>   
>>   	count = read_fdinfo(buf, sizeof(buf), dir, fd);
>> @@ -173,18 +241,79 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>>   				info->capacity[idx] = val;
>>   				num_capacity++;
>>   			}
>> +		} else if (!strncmp(l, "drm-total-", 10)) {
>> +			idx = parse_region(l, info, strlen("drm-total-"),
>> +					   region_map, region_entries, &val);
>> +			if (idx >= 0) {
>> +				info->region_mem[idx].total = val;
>> +				if (!regions_found[idx]) {
>> +					info->num_regions++;
>> +					regions_found[idx] = true;
>> +					if (idx > info->last_region_index)
>> +						info->last_region_index = idx;
>> +				}
>> +			}
>> +		} else if (!strncmp(l, "drm-shared-", 11)) {
>> +			idx = parse_region(l, info, strlen("drm-shared-"),
>> +					   region_map, region_entries, &val);
>> +			if (idx >= 0) {
>> +				info->region_mem[idx].shared = val;
>> +				if (!regions_found[idx]) {
>> +					info->num_regions++;
>> +					regions_found[idx] = true;
>> +					if (idx > info->last_region_index)
>> +						info->last_region_index = idx;
>> +				}
>> +			}
>> +		} else if (!strncmp(l, "drm-resident-", 13)) {
>> +			idx = parse_region(l, info, strlen("drm-resident-"),
>> +					   region_map, region_entries, &val);
>> +			if (idx >= 0) {
>> +				info->region_mem[idx].resident = val;
>> +				if (!regions_found[idx]) {
>> +					info->num_regions++;
>> +					regions_found[idx] = true;
>> +					if (idx > info->last_region_index)
>> +						info->last_region_index = idx;
>> +				}
>> +			}
>> +		} else if (!strncmp(l, "drm-purgeable-", 14)) {
>> +			idx = parse_region(l, info, strlen("drm-purgeable-"),
>> +					   region_map, region_entries, &val);
>> +			if (idx >= 0) {
>> +				info->region_mem[idx].purgeable = val;
>> +				if (!regions_found[idx]) {
>> +					info->num_regions++;
>> +					regions_found[idx] = true;
>> +					if (idx > info->last_region_index)
>> +						info->last_region_index = idx;
>> +				}
>> +			}
>> +		} else if (!strncmp(l, "drm-active-", 11)) {
>> +			idx = parse_region(l, info, strlen("drm-active-"),
>> +					   region_map, region_entries, &val);
>> +			if (idx >= 0) {
>> +				info->region_mem[idx].active = val;
>> +				if (!regions_found[idx]) {
>> +					info->num_regions++;
>> +					regions_found[idx] = true;
>> +					if (idx > info->last_region_index)
>> +						info->last_region_index = idx;
>> +				}
>> +			}
>>   		}
>>   	}
>>   
>> -	if (good < 2 || !info->num_engines)
>> +	if (good < 2 || (!info->num_engines && !info->num_regions))
>>   		return 0; /* fdinfo format not as expected */
>>   
>> -	return good + info->num_engines + num_capacity;
>> +	return good + info->num_engines + num_capacity + info->num_regions;
>>   }
>>   
>>   unsigned int
>>   igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
>> -		     const char **name_map, unsigned int map_entries)
>> +		     const char **name_map, unsigned int map_entries,
>> +		     const char **region_map, unsigned int region_entries)
>>   {
>>   	unsigned int res;
>>   	char fd[64];
>> @@ -198,7 +327,8 @@ igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
>>   	if (dir < 0)
>>   		return false;
>>   
>> -	res = __igt_parse_drm_fdinfo(dir, fd, info, name_map, map_entries);
>> +	res = __igt_parse_drm_fdinfo(dir, fd, info, name_map, map_entries,
>> +				     region_map, region_entries);
>>   
>>   	close(dir);
>>   
>> diff --git a/lib/igt_drm_fdinfo.h b/lib/igt_drm_fdinfo.h
>> index 6284e05e868a..1999c4f2b857 100644
>> --- a/lib/igt_drm_fdinfo.h
>> +++ b/lib/igt_drm_fdinfo.h
>> @@ -30,8 +30,17 @@
>>   #include <stdint.h>
>>   #include <stdbool.h>
>>   
>> +#define DRM_CLIENT_FDINFO_MAX_REGIONS 16
>>   #define DRM_CLIENT_FDINFO_MAX_ENGINES 16
>>   
>> +struct drm_client_meminfo {
>> +	uint64_t total;
>> +	uint64_t shared;
>> +	uint64_t resident;
>> +	uint64_t purgeable;
>> +	uint64_t active;
>> +};
>> +
>>   struct drm_client_fdinfo {
>>   	char driver[128];
>>   	char pdev[128];
>> @@ -42,6 +51,11 @@ struct drm_client_fdinfo {
>>   	unsigned int capacity[DRM_CLIENT_FDINFO_MAX_ENGINES];
>>   	char names[DRM_CLIENT_FDINFO_MAX_ENGINES][256];
>>   	uint64_t busy[DRM_CLIENT_FDINFO_MAX_ENGINES];
>> +
>> +	unsigned int num_regions;
>> +	unsigned int last_region_index;
>> +	char region_names[DRM_CLIENT_FDINFO_MAX_REGIONS][256];
>> +	struct drm_client_meminfo region_mem[DRM_CLIENT_FDINFO_MAX_REGIONS];
>>   };
>>   
>>   /**
>> @@ -51,13 +65,16 @@ struct drm_client_fdinfo {
>>    * @info: Structure to populate with read data. Must be zeroed.
>>    * @name_map: Optional array of strings representing engine names
>>    * @map_entries: Number of strings in the names array
>> + * @region_map: Optional array of strings representing memory regions
>> + * @region_entries: Number of strings in the region map
>>    *
>>    * Returns the number of valid drm fdinfo keys found or zero if not all
>>    * mandatory keys were present or no engines found.
>>    */
>>   unsigned int
>>   igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
>> -		     const char **name_map, unsigned int map_entries);
>> +		     const char **name_map, unsigned int map_entries,
>> +		     const char **region_map, unsigned int region_entries);
>>   
>>   /**
>>    * __igt_parse_drm_fdinfo: Parses the drm fdinfo file
>> @@ -67,6 +84,8 @@ igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
>>    * @info: Structure to populate with read data. Must be zeroed.
>>    * @name_map: Optional array of strings representing engine names
>>    * @map_entries: Number of strings in the names array
>> + * @region_map: Optional array of strings representing memory regions
>> + * @region_entries: Number of strings in the region map
>>    *
>>    * Returns the number of valid drm fdinfo keys found or zero if not all
>>    * mandatory keys were present or no engines found.
>> @@ -74,6 +93,7 @@ igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info,
>>   unsigned int
>>   __igt_parse_drm_fdinfo(int dir, const char *fd,
>>   		       struct drm_client_fdinfo *info,
>> -		       const char **name_map, unsigned int map_entries);
>> +		       const char **name_map, unsigned int map_entries,
>> +		       const char **region_map, unsigned int region_entries);
>>   
>>   #endif /* IGT_DRM_FDINFO_H */
>> diff --git a/tests/i915/drm_fdinfo.c b/tests/i915/drm_fdinfo.c
>> index c0e0ba1905f1..01526c0e19de 100644
>> --- a/tests/i915/drm_fdinfo.c
>> +++ b/tests/i915/drm_fdinfo.c
>> @@ -104,7 +104,7 @@ static void basics(int i915, unsigned int num_classes)
>>   	unsigned int ret;
>>   
>>   	ret = igt_parse_drm_fdinfo(i915, &info, engine_map,
>> -				   ARRAY_SIZE(engine_map));
>> +				   ARRAY_SIZE(engine_map), NULL, 0);
>>   	igt_assert(ret);
>>   
>>   	igt_assert(!strcmp(info.driver, "i915"));
>> @@ -236,7 +236,7 @@ static uint64_t read_busy(int i915, unsigned int class)
>>   	struct drm_client_fdinfo info = { };
>>   
>>   	igt_assert(igt_parse_drm_fdinfo(i915, &info, engine_map,
>> -					ARRAY_SIZE(engine_map)));
>> +					ARRAY_SIZE(engine_map), NULL, 0));
>>   
>>   	return info.busy[class];
>>   }
>> @@ -326,7 +326,7 @@ static void read_busy_all(int i915, uint64_t *val)
>>   	struct drm_client_fdinfo info = { };
>>   
>>   	igt_assert(igt_parse_drm_fdinfo(i915, &info, engine_map,
>> -					ARRAY_SIZE(engine_map)));
>> +					ARRAY_SIZE(engine_map), NULL, 0));
>>   
>>   	memcpy(val, info.busy, sizeof(info.busy));
>>   }
>> @@ -797,7 +797,7 @@ igt_main
>>   		i915 = __drm_open_driver(DRIVER_INTEL);
>>   
>>   		igt_require_gem(i915);
>> -		igt_require(igt_parse_drm_fdinfo(i915, &info, NULL, 0));
>> +		igt_require(igt_parse_drm_fdinfo(i915, &info, NULL, 0, NULL, 0));
>>   
>>   		ctx = intel_ctx_create_all_physical(i915);
>>   
>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c
>> index cef1d3c7fa9f..87e9681e53b4 100644
>> --- a/tools/intel_gpu_top.c
>> +++ b/tools/intel_gpu_top.c
>> @@ -2307,7 +2307,7 @@ static bool has_drm_fdinfo(const struct igt_device_card *card)
>>   	if (fd < 0)
>>   		return false;
>>   
>> -	cnt = igt_parse_drm_fdinfo(fd, &info, NULL, 0);
>> +	cnt = igt_parse_drm_fdinfo(fd, &info, NULL, 0, NULL, 0);
>>   
>>   	close(fd);
>>   
>> -- 
>> 2.39.2
>>

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

* [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client
  2023-07-05 16:31 [Intel-gfx] [PATCH i-g-t 0/3] gputop memory usage Tvrtko Ursulin
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 1/3] lib/igt_drm_fdinfo: Parse " Tvrtko Ursulin
@ 2023-07-05 16:31 ` Tvrtko Ursulin
  2023-07-26 17:00   ` Kamil Konieczny
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 3/3] gputop: Add memory information Tvrtko Ursulin
  2 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-05 16:31 UTC (permalink / raw)
  To: igt-dev, Intel-gfx; +Cc: Rob Clark

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

Define the storage structure and copy over memory data as parsed by the
fdinfo helpers.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Rob Clark <robdclark@chromium.org>
---
 lib/igt_drm_clients.c | 31 +++++++++++++++++++++++++++++++
 lib/igt_drm_clients.h | 11 +++++++++++
 2 files changed, 42 insertions(+)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index fdea42752a81..0db5b64effea 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -103,6 +103,8 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
 			c->clients->max_name_len = len;
 	}
 
+	/* Engines */
+
 	c->last_runtime = 0;
 	c->total_runtime = 0;
 
@@ -118,6 +120,13 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
 		c->last[i] = info->busy[i];
 	}
 
+	/* Memory regions */
+	for (i = 0; i <= c->regions->max_region_id; i++) {
+		assert(i < ARRAY_SIZE(info->region_mem));
+
+		c->memory[i] = info->region_mem[i];
+	}
+
 	c->samples++;
 	c->status = IGT_DRM_CLIENT_ALIVE;
 }
@@ -154,6 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
 	c->id = info->id;
 	c->drm_minor = drm_minor;
 	c->clients = clients;
+
+	/* Engines */
 	c->engines = malloc(sizeof(*c->engines));
 	assert(c->engines);
 	memset(c->engines, 0, sizeof(*c->engines));
@@ -178,6 +189,26 @@ igt_drm_client_add(struct igt_drm_clients *clients,
 	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
 	assert(c->val && c->last);
 
+	/* Memory regions */
+	c->regions = malloc(sizeof(*c->regions));
+	assert(c->regions);
+	memset(c->regions, 0, sizeof(*c->regions));
+	c->regions->names = calloc(info->last_region_index + 1,
+				   sizeof(*c->regions->names));
+	assert(c->regions->names);
+
+	for (i = 0; i <= info->last_region_index; i++) {
+		if (!info->region_names[i])
+			continue;
+
+		c->regions->names[i] = strdup(info->region_names[i]);
+		assert(c->regions->names[i]);
+		c->regions->num_regions++;
+		c->regions->max_region_id = i;
+	}
+	c->memory = calloc(c->regions->max_region_id + 1, sizeof(*c->memory));
+	assert(c->memory);
+
 	igt_drm_client_update(c, pid, name, info);
 }
 
diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
index ed795c193986..07bd236d43bf 100644
--- a/lib/igt_drm_clients.h
+++ b/lib/igt_drm_clients.h
@@ -8,6 +8,8 @@
 
 #include <stdint.h>
 
+#include "lib/igt_drm_fdinfo.h"
+
 /**
  * SECTION:igt_drm_clients
  * @short_description: Parsing driver exposed fdinfo to track DRM clients
@@ -39,12 +41,20 @@ struct igt_drm_client_engines {
 	char **names; /* Array of engine names, either auto-detected or from the passed in engine map. */
 };
 
+struct igt_drm_client_regions {
+	unsigned int num_regions; /* Number of discovered memory_regions. */
+	unsigned int max_region_id; /* Largest memory region index discovered.
+				       (Can differ from num_regions - 1 when using the region map facility.) */
+	char **names; /* Array of region names, either auto-detected or from the passed in region map. */
+};
+
 struct igt_drm_clients;
 
 struct igt_drm_client {
 	struct igt_drm_clients *clients; /* Owning list. */
 
 	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 int drm_minor; /* DRM minor of this client. */
@@ -57,6 +67,7 @@ struct igt_drm_client {
 	unsigned long last_runtime; /* Aggregate busyness on all engines since previous scan. */
 	unsigned long *val; /* Array of engine busyness data, relative to previous scan. */
 	uint64_t *last; /* Array of engine busyness data as parsed from fdinfo. */
+	struct drm_client_meminfo *memory; /* Array of region memory utilisation as parsed from fdinfo. */
 };
 
 struct igt_drm_clients {
-- 
2.39.2


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

* Re: [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client Tvrtko Ursulin
@ 2023-07-26 17:00   ` Kamil Konieczny
  2023-07-27  9:08     ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Kamil Konieczny @ 2023-07-26 17:00 UTC (permalink / raw)
  To: igt-dev; +Cc: Rob Clark, Intel-gfx

Hi Tvrtko,

On 2023-07-05 at 17:31:04 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Define the storage structure and copy over memory data as parsed by the
> fdinfo helpers.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Rob Clark <robdclark@chromium.org>
> ---
>  lib/igt_drm_clients.c | 31 +++++++++++++++++++++++++++++++
>  lib/igt_drm_clients.h | 11 +++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index fdea42752a81..0db5b64effea 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -103,6 +103,8 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>  			c->clients->max_name_len = len;
>  	}
>  
> +	/* Engines */
> +
>  	c->last_runtime = 0;
>  	c->total_runtime = 0;
>  
> @@ -118,6 +120,13 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>  		c->last[i] = info->busy[i];
>  	}
>  
> +	/* Memory regions */
> +	for (i = 0; i <= c->regions->max_region_id; i++) {
> +		assert(i < ARRAY_SIZE(info->region_mem));
> +
> +		c->memory[i] = info->region_mem[i];
> +	}
> +
>  	c->samples++;
>  	c->status = IGT_DRM_CLIENT_ALIVE;
>  }
> @@ -154,6 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>  	c->id = info->id;
>  	c->drm_minor = drm_minor;
>  	c->clients = clients;
> +
> +	/* Engines */
>  	c->engines = malloc(sizeof(*c->engines));
>  	assert(c->engines);
>  	memset(c->engines, 0, sizeof(*c->engines));
> @@ -178,6 +189,26 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>  	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
>  	assert(c->val && c->last);
>  
> +	/* Memory regions */
> +	c->regions = malloc(sizeof(*c->regions));
> +	assert(c->regions);
> +	memset(c->regions, 0, sizeof(*c->regions));
> +	c->regions->names = calloc(info->last_region_index + 1,
> +				   sizeof(*c->regions->names));
> +	assert(c->regions->names);
> +
> +	for (i = 0; i <= info->last_region_index; i++) {
> +		if (!info->region_names[i])

This do not compile:
../lib/igt_drm_clients.c:201:21: error: the comparison will always evaluate as ‘true’ for the address of ‘region_names’ will never be NULL [-Werror=address]
  201 |                 if (!info->region_names[i])
      |                     ^
In file included from ../lib/igt_drm_clients.h:11,
                 from ../lib/igt_drm_clients.c:20:
../lib/igt_drm_fdinfo.h:57:14: note: ‘region_names’ declared here
   57 |         char region_names[DRM_CLIENT_FDINFO_MAX_REGIONS][256];
      |              ^~~~~~~~~~~~

did you mean:

		if (!info->region_names[i][0])

Regards,
Kamil

> +			continue;
> +
> +		c->regions->names[i] = strdup(info->region_names[i]);
> +		assert(c->regions->names[i]);
> +		c->regions->num_regions++;
> +		c->regions->max_region_id = i;
> +	}
> +	c->memory = calloc(c->regions->max_region_id + 1, sizeof(*c->memory));
> +	assert(c->memory);
> +
>  	igt_drm_client_update(c, pid, name, info);
>  }
>  
> diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
> index ed795c193986..07bd236d43bf 100644
> --- a/lib/igt_drm_clients.h
> +++ b/lib/igt_drm_clients.h
> @@ -8,6 +8,8 @@
>  
>  #include <stdint.h>
>  
> +#include "lib/igt_drm_fdinfo.h"
> +
>  /**
>   * SECTION:igt_drm_clients
>   * @short_description: Parsing driver exposed fdinfo to track DRM clients
> @@ -39,12 +41,20 @@ struct igt_drm_client_engines {
>  	char **names; /* Array of engine names, either auto-detected or from the passed in engine map. */
>  };
>  
> +struct igt_drm_client_regions {
> +	unsigned int num_regions; /* Number of discovered memory_regions. */
> +	unsigned int max_region_id; /* Largest memory region index discovered.
> +				       (Can differ from num_regions - 1 when using the region map facility.) */
> +	char **names; /* Array of region names, either auto-detected or from the passed in region map. */
> +};
> +
>  struct igt_drm_clients;
>  
>  struct igt_drm_client {
>  	struct igt_drm_clients *clients; /* Owning list. */
>  
>  	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 int drm_minor; /* DRM minor of this client. */
> @@ -57,6 +67,7 @@ struct igt_drm_client {
>  	unsigned long last_runtime; /* Aggregate busyness on all engines since previous scan. */
>  	unsigned long *val; /* Array of engine busyness data, relative to previous scan. */
>  	uint64_t *last; /* Array of engine busyness data as parsed from fdinfo. */
> +	struct drm_client_meminfo *memory; /* Array of region memory utilisation as parsed from fdinfo. */
>  };
>  
>  struct igt_drm_clients {
> -- 
> 2.39.2
> 

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

* Re: [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client
  2023-07-26 17:00   ` Kamil Konieczny
@ 2023-07-27  9:08     ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-27  9:08 UTC (permalink / raw)
  To: Kamil Konieczny, igt-dev, Intel-gfx, Rob Clark


On 26/07/2023 18:00, Kamil Konieczny wrote:
> Hi Tvrtko,
> 
> On 2023-07-05 at 17:31:04 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Define the storage structure and copy over memory data as parsed by the
>> fdinfo helpers.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Rob Clark <robdclark@chromium.org>
>> ---
>>   lib/igt_drm_clients.c | 31 +++++++++++++++++++++++++++++++
>>   lib/igt_drm_clients.h | 11 +++++++++++
>>   2 files changed, 42 insertions(+)
>>
>> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
>> index fdea42752a81..0db5b64effea 100644
>> --- a/lib/igt_drm_clients.c
>> +++ b/lib/igt_drm_clients.c
>> @@ -103,6 +103,8 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>>   			c->clients->max_name_len = len;
>>   	}
>>   
>> +	/* Engines */
>> +
>>   	c->last_runtime = 0;
>>   	c->total_runtime = 0;
>>   
>> @@ -118,6 +120,13 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>>   		c->last[i] = info->busy[i];
>>   	}
>>   
>> +	/* Memory regions */
>> +	for (i = 0; i <= c->regions->max_region_id; i++) {
>> +		assert(i < ARRAY_SIZE(info->region_mem));
>> +
>> +		c->memory[i] = info->region_mem[i];
>> +	}
>> +
>>   	c->samples++;
>>   	c->status = IGT_DRM_CLIENT_ALIVE;
>>   }
>> @@ -154,6 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>>   	c->id = info->id;
>>   	c->drm_minor = drm_minor;
>>   	c->clients = clients;
>> +
>> +	/* Engines */
>>   	c->engines = malloc(sizeof(*c->engines));
>>   	assert(c->engines);
>>   	memset(c->engines, 0, sizeof(*c->engines));
>> @@ -178,6 +189,26 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>>   	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
>>   	assert(c->val && c->last);
>>   
>> +	/* Memory regions */
>> +	c->regions = malloc(sizeof(*c->regions));
>> +	assert(c->regions);
>> +	memset(c->regions, 0, sizeof(*c->regions));
>> +	c->regions->names = calloc(info->last_region_index + 1,
>> +				   sizeof(*c->regions->names));
>> +	assert(c->regions->names);
>> +
>> +	for (i = 0; i <= info->last_region_index; i++) {
>> +		if (!info->region_names[i])
> 
> This do not compile:
> ../lib/igt_drm_clients.c:201:21: error: the comparison will always evaluate as ‘true’ for the address of ‘region_names’ will never be NULL [-Werror=address]
>    201 |                 if (!info->region_names[i])
>        |                     ^
> In file included from ../lib/igt_drm_clients.h:11,
>                   from ../lib/igt_drm_clients.c:20:
> ../lib/igt_drm_fdinfo.h:57:14: note: ‘region_names’ declared here
>     57 |         char region_names[DRM_CLIENT_FDINFO_MAX_REGIONS][256];
>        |              ^~~~~~~~~~~~
> 
> did you mean:
> 
> 		if (!info->region_names[i][0])

Yes. But it's odd.. I can see the error today. But back then I built the 
thing, ran it, collected the screenshot for the cover letter. Oh well a 
mystery. Fixed now, thanks!

Regards,

Tvrtko

> 
> Regards,
> Kamil
> 
>> +			continue;
>> +
>> +		c->regions->names[i] = strdup(info->region_names[i]);
>> +		assert(c->regions->names[i]);
>> +		c->regions->num_regions++;
>> +		c->regions->max_region_id = i;
>> +	}
>> +	c->memory = calloc(c->regions->max_region_id + 1, sizeof(*c->memory));
>> +	assert(c->memory);
>> +
>>   	igt_drm_client_update(c, pid, name, info);
>>   }
>>   
>> diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
>> index ed795c193986..07bd236d43bf 100644
>> --- a/lib/igt_drm_clients.h
>> +++ b/lib/igt_drm_clients.h
>> @@ -8,6 +8,8 @@
>>   
>>   #include <stdint.h>
>>   
>> +#include "lib/igt_drm_fdinfo.h"
>> +
>>   /**
>>    * SECTION:igt_drm_clients
>>    * @short_description: Parsing driver exposed fdinfo to track DRM clients
>> @@ -39,12 +41,20 @@ struct igt_drm_client_engines {
>>   	char **names; /* Array of engine names, either auto-detected or from the passed in engine map. */
>>   };
>>   
>> +struct igt_drm_client_regions {
>> +	unsigned int num_regions; /* Number of discovered memory_regions. */
>> +	unsigned int max_region_id; /* Largest memory region index discovered.
>> +				       (Can differ from num_regions - 1 when using the region map facility.) */
>> +	char **names; /* Array of region names, either auto-detected or from the passed in region map. */
>> +};
>> +
>>   struct igt_drm_clients;
>>   
>>   struct igt_drm_client {
>>   	struct igt_drm_clients *clients; /* Owning list. */
>>   
>>   	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 int drm_minor; /* DRM minor of this client. */
>> @@ -57,6 +67,7 @@ struct igt_drm_client {
>>   	unsigned long last_runtime; /* Aggregate busyness on all engines since previous scan. */
>>   	unsigned long *val; /* Array of engine busyness data, relative to previous scan. */
>>   	uint64_t *last; /* Array of engine busyness data as parsed from fdinfo. */
>> +	struct drm_client_meminfo *memory; /* Array of region memory utilisation as parsed from fdinfo. */
>>   };
>>   
>>   struct igt_drm_clients {
>> -- 
>> 2.39.2
>>

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

* [Intel-gfx] [PATCH i-g-t 3/3] gputop: Add memory information
  2023-07-05 16:31 [Intel-gfx] [PATCH i-g-t 0/3] gputop memory usage Tvrtko Ursulin
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 1/3] lib/igt_drm_fdinfo: Parse " Tvrtko Ursulin
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client Tvrtko Ursulin
@ 2023-07-05 16:31 ` Tvrtko Ursulin
  2023-07-26 17:32   ` Kamil Konieczny
  2 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-05 16:31 UTC (permalink / raw)
  To: igt-dev, Intel-gfx; +Cc: Rob Clark

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

Show total and resident memory usage for clients which support it.

For simplicity all memory regions are summed up and shown under a single
heading.

Co-developed-by: Rob Clark <robdclark@chromium.org>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tools/gputop.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/gputop.c b/tools/gputop.c
index 681f0a6bb748..b5b360cbb063 100644
--- a/tools/gputop.c
+++ b/tools/gputop.c
@@ -28,6 +28,7 @@
 
 #include "igt_drm_clients.h"
 #include "igt_drm_fdinfo.h"
+#include "drmtest.h"
 
 static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
 
@@ -80,7 +81,11 @@ print_client_header(struct igt_drm_client *c, int lines, int con_w, int con_h,
 		return lines;
 
 	putchar('\n');
-	len = printf("%*s ", c->clients->max_pid_len, "PID");
+	if (c->regions->num_regions)
+		len = printf("%*s      MEM      RSS ",
+			     c->clients->max_pid_len, "PID");
+	else
+		len = printf("%*s ", c->clients->max_pid_len, "PID");
 
 	if (c->engines->num_engines) {
 		unsigned int i;
@@ -121,12 +126,28 @@ newheader(const struct igt_drm_client *c, const struct igt_drm_client *pc)
 	return !pc || c->drm_minor != pc->drm_minor;
 }
 
+static int
+print_size(uint64_t sz)
+{
+	char units[] = {'B', 'K', 'M', 'G'};
+	unsigned u;
+
+	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
+		if (sz < 1024)
+			break;
+		sz /= 1024;
+	}
+
+	return printf("%7"PRIu64"%c ", sz, units[u]);
+}
+
 static int
 print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
 	     double t, int lines, int con_w, int con_h,
 	     unsigned int period_us, int *engine_w)
 {
 	unsigned int i;
+	uint64_t sz;
 	int len;
 
 	/* Filter out idle clients. */
@@ -143,6 +164,17 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
 	*prevc = c;
 
 	len = printf("%*s ", c->clients->max_pid_len, c->pid_str);
+
+	if (c->regions->num_regions) {
+		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);
+	}
+
 	lines++;
 
 	for (i = 0; c->samples > 1 && i <= c->engines->max_engine_id; i++) {
-- 
2.39.2


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

* Re: [Intel-gfx] [PATCH i-g-t 3/3] gputop: Add memory information
  2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 3/3] gputop: Add memory information Tvrtko Ursulin
@ 2023-07-26 17:32   ` Kamil Konieczny
  2023-07-27  9:09     ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Kamil Konieczny @ 2023-07-26 17:32 UTC (permalink / raw)
  To: igt-dev; +Cc: Rob Clark, Intel-gfx

Hi Tvrtko,

On 2023-07-05 at 17:31:05 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Show total and resident memory usage for clients which support it.
> 
> For simplicity all memory regions are summed up and shown under a single
> heading.
> 
> Co-developed-by: Rob Clark <robdclark@chromium.org>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  tools/gputop.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/gputop.c b/tools/gputop.c
> index 681f0a6bb748..b5b360cbb063 100644
> --- a/tools/gputop.c
> +++ b/tools/gputop.c
> @@ -28,6 +28,7 @@
>  
>  #include "igt_drm_clients.h"
>  #include "igt_drm_fdinfo.h"
> +#include "drmtest.h"
>  
>  static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
>  
> @@ -80,7 +81,11 @@ print_client_header(struct igt_drm_client *c, int lines, int con_w, int con_h,
>  		return lines;
>  
>  	putchar('\n');
> -	len = printf("%*s ", c->clients->max_pid_len, "PID");
> +	if (c->regions->num_regions)
> +		len = printf("%*s      MEM      RSS ",
> +			     c->clients->max_pid_len, "PID");
> +	else
> +		len = printf("%*s ", c->clients->max_pid_len, "PID");
>  
>  	if (c->engines->num_engines) {
>  		unsigned int i;
> @@ -121,12 +126,28 @@ newheader(const struct igt_drm_client *c, const struct igt_drm_client *pc)
>  	return !pc || c->drm_minor != pc->drm_minor;
>  }
>  
> +static int
> +print_size(uint64_t sz)
> +{
> +	char units[] = {'B', 'K', 'M', 'G'};
> +	unsigned u;
------- ^
Better:
	unsigned int u;

With that add my r-b tag,

Regards,
Kamil

> +
> +	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> +		if (sz < 1024)
> +			break;
> +		sz /= 1024;
> +	}
> +
> +	return printf("%7"PRIu64"%c ", sz, units[u]);
> +}
> +
>  static int
>  print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>  	     double t, int lines, int con_w, int con_h,
>  	     unsigned int period_us, int *engine_w)
>  {
>  	unsigned int i;
> +	uint64_t sz;
>  	int len;
>  
>  	/* Filter out idle clients. */
> @@ -143,6 +164,17 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>  	*prevc = c;
>  
>  	len = printf("%*s ", c->clients->max_pid_len, c->pid_str);
> +
> +	if (c->regions->num_regions) {
> +		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);
> +	}
> +
>  	lines++;
>  
>  	for (i = 0; c->samples > 1 && i <= c->engines->max_engine_id; i++) {
> -- 
> 2.39.2
> 

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

* Re: [Intel-gfx] [PATCH i-g-t 3/3] gputop: Add memory information
  2023-07-26 17:32   ` Kamil Konieczny
@ 2023-07-27  9:09     ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-27  9:09 UTC (permalink / raw)
  To: Kamil Konieczny, igt-dev, Intel-gfx, Rob Clark


On 26/07/2023 18:32, Kamil Konieczny wrote:
> Hi Tvrtko,
> 
> On 2023-07-05 at 17:31:05 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Show total and resident memory usage for clients which support it.
>>
>> For simplicity all memory regions are summed up and shown under a single
>> heading.
>>
>> Co-developed-by: Rob Clark <robdclark@chromium.org>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   tools/gputop.c | 34 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/gputop.c b/tools/gputop.c
>> index 681f0a6bb748..b5b360cbb063 100644
>> --- a/tools/gputop.c
>> +++ b/tools/gputop.c
>> @@ -28,6 +28,7 @@
>>   
>>   #include "igt_drm_clients.h"
>>   #include "igt_drm_fdinfo.h"
>> +#include "drmtest.h"
>>   
>>   static const char *bars[] = { " ", "▏", "▎", "▍", "▌", "▋", "▊", "▉", "█" };
>>   
>> @@ -80,7 +81,11 @@ print_client_header(struct igt_drm_client *c, int lines, int con_w, int con_h,
>>   		return lines;
>>   
>>   	putchar('\n');
>> -	len = printf("%*s ", c->clients->max_pid_len, "PID");
>> +	if (c->regions->num_regions)
>> +		len = printf("%*s      MEM      RSS ",
>> +			     c->clients->max_pid_len, "PID");
>> +	else
>> +		len = printf("%*s ", c->clients->max_pid_len, "PID");
>>   
>>   	if (c->engines->num_engines) {
>>   		unsigned int i;
>> @@ -121,12 +126,28 @@ newheader(const struct igt_drm_client *c, const struct igt_drm_client *pc)
>>   	return !pc || c->drm_minor != pc->drm_minor;
>>   }
>>   
>> +static int
>> +print_size(uint64_t sz)
>> +{
>> +	char units[] = {'B', 'K', 'M', 'G'};
>> +	unsigned u;
> ------- ^
> Better:
> 	unsigned int u;
> 
> With that add my r-b tag,

Okay, thanks, re-send will be coming shortly, just some final smoke 
tests to do.

Regards,

Tvrtko

> 
> Regards,
> Kamil
> 
>> +
>> +	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
>> +		if (sz < 1024)
>> +			break;
>> +		sz /= 1024;
>> +	}
>> +
>> +	return printf("%7"PRIu64"%c ", sz, units[u]);
>> +}
>> +
>>   static int
>>   print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>>   	     double t, int lines, int con_w, int con_h,
>>   	     unsigned int period_us, int *engine_w)
>>   {
>>   	unsigned int i;
>> +	uint64_t sz;
>>   	int len;
>>   
>>   	/* Filter out idle clients. */
>> @@ -143,6 +164,17 @@ print_client(struct igt_drm_client *c, struct igt_drm_client **prevc,
>>   	*prevc = c;
>>   
>>   	len = printf("%*s ", c->clients->max_pid_len, c->pid_str);
>> +
>> +	if (c->regions->num_regions) {
>> +		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);
>> +	}
>> +
>>   	lines++;
>>   
>>   	for (i = 0; c->samples > 1 && i <= c->engines->max_engine_id; i++) {
>> -- 
>> 2.39.2
>>

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

* [Intel-gfx] [PATCH i-g-t 0/3] gputop memory usage
@ 2023-07-27  9:20 Tvrtko Ursulin
  2023-07-27  9:20 ` [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-27  9:20 UTC (permalink / raw)
  To: igt-dev, Intel-gfx; +Cc: Rob Clark, Chris Healy

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

Same general approach as with engine utilisation, adding parsing of the relevant
fdinfo fields and plumbing to client discovery helpers to finally present some
of the available data in gputop:

DRM minor 0
 PID      MEM      RSS  render    copy     video   NAME
8098     124M     124M |███████||       ||       ||       | neverball
 888      76M      75M |▊      ||       ||       ||       | Xorg
 966      12M      12M |       ||       ||       ||       | xfwm4

Or with Wayland:

DRM minor 0
 PID      MEM      RSS    render        copy         video    video-enhance NAME
2093     191M     191M |▊          ||           ||           ||           | gnome-shell
DRM minor 128
 PID      MEM      RSS    render        copy         video    video-enhance NAME
2551      71M      71M |██▉        ||           ||           ||           | neverball
2553      50M      50M |           ||           ||           ||           | Xwayland

There is probably scope to consolidate some of the igt_drm_fdinfo code and to
improve on the presentation but this is a start.

For now, out of the available memory categories, only total and resident are
displayed, and also all discovered memory regions are summed up and shown under
a single heading.

v2:
 * One style tweak, two missing s-o-b, one build failure.

Cc: Rob Clark <robdclark@chromium.org>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Chris Healy <cphealy@gmail.com>

Tvrtko Ursulin (3):
  lib/igt_drm_fdinfo: Parse memory usage
  lib/igt_drm_clients: Store memory info in the client
  gputop: Add memory information

 lib/igt_drm_clients.c   |  35 +++++++++-
 lib/igt_drm_clients.h   |  11 ++++
 lib/igt_drm_fdinfo.c    | 143 ++++++++++++++++++++++++++++++++++++++--
 lib/igt_drm_fdinfo.h    |  24 ++++++-
 tests/i915/drm_fdinfo.c |   8 +--
 tools/gputop.c          |  34 +++++++++-
 tools/intel_gpu_top.c   |   2 +-
 7 files changed, 242 insertions(+), 15 deletions(-)

-- 
2.39.2


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

* [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client
  2023-07-27  9:20 [Intel-gfx] [PATCH i-g-t 0/3] gputop memory usage Tvrtko Ursulin
@ 2023-07-27  9:20 ` Tvrtko Ursulin
  2023-07-27 14:10   ` Kamil Konieczny
  2023-08-07 14:40   ` Kamil Konieczny
  0 siblings, 2 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-27  9:20 UTC (permalink / raw)
  To: igt-dev, Intel-gfx; +Cc: Rob Clark, Chris Healy

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

Define the storage structure and copy over memory data as parsed by the
fdinfo helpers.

v2:
 * Fix empty region map entry skip condition. (Kamil, Chris)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Rob Clark <robdclark@chromium.org>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/igt_drm_clients.c | 32 ++++++++++++++++++++++++++++++++
 lib/igt_drm_clients.h | 11 +++++++++++
 2 files changed, 43 insertions(+)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index fdea42752a81..47ad137d5f1f 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -103,6 +103,8 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
 			c->clients->max_name_len = len;
 	}
 
+	/* Engines */
+
 	c->last_runtime = 0;
 	c->total_runtime = 0;
 
@@ -118,6 +120,13 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
 		c->last[i] = info->busy[i];
 	}
 
+	/* Memory regions */
+	for (i = 0; i <= c->regions->max_region_id; i++) {
+		assert(i < ARRAY_SIZE(info->region_mem));
+
+		c->memory[i] = info->region_mem[i];
+	}
+
 	c->samples++;
 	c->status = IGT_DRM_CLIENT_ALIVE;
 }
@@ -154,6 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
 	c->id = info->id;
 	c->drm_minor = drm_minor;
 	c->clients = clients;
+
+	/* Engines */
 	c->engines = malloc(sizeof(*c->engines));
 	assert(c->engines);
 	memset(c->engines, 0, sizeof(*c->engines));
@@ -178,6 +189,27 @@ igt_drm_client_add(struct igt_drm_clients *clients,
 	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
 	assert(c->val && c->last);
 
+	/* Memory regions */
+	c->regions = malloc(sizeof(*c->regions));
+	assert(c->regions);
+	memset(c->regions, 0, sizeof(*c->regions));
+	c->regions->names = calloc(info->last_region_index + 1,
+				   sizeof(*c->regions->names));
+	assert(c->regions->names);
+
+	for (i = 0; i <= info->last_region_index; i++) {
+		/* Region map is allowed to be sparse. */
+		if (!info->region_names[i][0])
+			continue;
+
+		c->regions->names[i] = strdup(info->region_names[i]);
+		assert(c->regions->names[i]);
+		c->regions->num_regions++;
+		c->regions->max_region_id = i;
+	}
+	c->memory = calloc(c->regions->max_region_id + 1, sizeof(*c->memory));
+	assert(c->memory);
+
 	igt_drm_client_update(c, pid, name, info);
 }
 
diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
index ed795c193986..07bd236d43bf 100644
--- a/lib/igt_drm_clients.h
+++ b/lib/igt_drm_clients.h
@@ -8,6 +8,8 @@
 
 #include <stdint.h>
 
+#include "lib/igt_drm_fdinfo.h"
+
 /**
  * SECTION:igt_drm_clients
  * @short_description: Parsing driver exposed fdinfo to track DRM clients
@@ -39,12 +41,20 @@ struct igt_drm_client_engines {
 	char **names; /* Array of engine names, either auto-detected or from the passed in engine map. */
 };
 
+struct igt_drm_client_regions {
+	unsigned int num_regions; /* Number of discovered memory_regions. */
+	unsigned int max_region_id; /* Largest memory region index discovered.
+				       (Can differ from num_regions - 1 when using the region map facility.) */
+	char **names; /* Array of region names, either auto-detected or from the passed in region map. */
+};
+
 struct igt_drm_clients;
 
 struct igt_drm_client {
 	struct igt_drm_clients *clients; /* Owning list. */
 
 	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 int drm_minor; /* DRM minor of this client. */
@@ -57,6 +67,7 @@ struct igt_drm_client {
 	unsigned long last_runtime; /* Aggregate busyness on all engines since previous scan. */
 	unsigned long *val; /* Array of engine busyness data, relative to previous scan. */
 	uint64_t *last; /* Array of engine busyness data as parsed from fdinfo. */
+	struct drm_client_meminfo *memory; /* Array of region memory utilisation as parsed from fdinfo. */
 };
 
 struct igt_drm_clients {
-- 
2.39.2


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

* Re: [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client
  2023-07-27  9:20 ` [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client Tvrtko Ursulin
@ 2023-07-27 14:10   ` Kamil Konieczny
  2023-07-27 15:17     ` Tvrtko Ursulin
  2023-08-07 14:40   ` Kamil Konieczny
  1 sibling, 1 reply; 15+ messages in thread
From: Kamil Konieczny @ 2023-07-27 14:10 UTC (permalink / raw)
  To: igt-dev; +Cc: Rob Clark, Intel-gfx, Chris Healy

Hi Tvrtko,

On 2023-07-27 at 10:20:24 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Define the storage structure and copy over memory data as parsed by the
> fdinfo helpers.
> 
> v2:
>  * Fix empty region map entry skip condition. (Kamil, Chris)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  lib/igt_drm_clients.c | 32 ++++++++++++++++++++++++++++++++
>  lib/igt_drm_clients.h | 11 +++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index fdea42752a81..47ad137d5f1f 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -103,6 +103,8 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>  			c->clients->max_name_len = len;
>  	}
>  
> +	/* Engines */
> +
>  	c->last_runtime = 0;
>  	c->total_runtime = 0;
>  
> @@ -118,6 +120,13 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>  		c->last[i] = info->busy[i];
>  	}
>  
> +	/* Memory regions */
> +	for (i = 0; i <= c->regions->max_region_id; i++) {
> +		assert(i < ARRAY_SIZE(info->region_mem));
> +
> +		c->memory[i] = info->region_mem[i];
> +	}
> +
>  	c->samples++;
>  	c->status = IGT_DRM_CLIENT_ALIVE;
>  }
> @@ -154,6 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>  	c->id = info->id;
>  	c->drm_minor = drm_minor;
>  	c->clients = clients;
> +
> +	/* Engines */
>  	c->engines = malloc(sizeof(*c->engines));
>  	assert(c->engines);
>  	memset(c->engines, 0, sizeof(*c->engines));
> @@ -178,6 +189,27 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>  	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
>  	assert(c->val && c->last);
>  
> +	/* Memory regions */
> +	c->regions = malloc(sizeof(*c->regions));
> +	assert(c->regions);
> +	memset(c->regions, 0, sizeof(*c->regions));
> +	c->regions->names = calloc(info->last_region_index + 1,
> +				   sizeof(*c->regions->names));
> +	assert(c->regions->names);
> +
> +	for (i = 0; i <= info->last_region_index; i++) {
> +		/* Region map is allowed to be sparse. */
> +		if (!info->region_names[i][0])
> +			continue;
> +
> +		c->regions->names[i] = strdup(info->region_names[i]);
--------------------------------- ^
Should this be c->regions->num_regions?

Regards,
Kamil

> +		assert(c->regions->names[i]);
> +		c->regions->num_regions++;
> +		c->regions->max_region_id = i;
> +	}
> +	c->memory = calloc(c->regions->max_region_id + 1, sizeof(*c->memory));
> +	assert(c->memory);
> +
>  	igt_drm_client_update(c, pid, name, info);
>  }
>  
> diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
> index ed795c193986..07bd236d43bf 100644
> --- a/lib/igt_drm_clients.h
> +++ b/lib/igt_drm_clients.h
> @@ -8,6 +8,8 @@
>  
>  #include <stdint.h>
>  
> +#include "lib/igt_drm_fdinfo.h"
> +
>  /**
>   * SECTION:igt_drm_clients
>   * @short_description: Parsing driver exposed fdinfo to track DRM clients
> @@ -39,12 +41,20 @@ struct igt_drm_client_engines {
>  	char **names; /* Array of engine names, either auto-detected or from the passed in engine map. */
>  };
>  
> +struct igt_drm_client_regions {
> +	unsigned int num_regions; /* Number of discovered memory_regions. */
> +	unsigned int max_region_id; /* Largest memory region index discovered.
> +				       (Can differ from num_regions - 1 when using the region map facility.) */
> +	char **names; /* Array of region names, either auto-detected or from the passed in region map. */
> +};
> +
>  struct igt_drm_clients;
>  
>  struct igt_drm_client {
>  	struct igt_drm_clients *clients; /* Owning list. */
>  
>  	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 int drm_minor; /* DRM minor of this client. */
> @@ -57,6 +67,7 @@ struct igt_drm_client {
>  	unsigned long last_runtime; /* Aggregate busyness on all engines since previous scan. */
>  	unsigned long *val; /* Array of engine busyness data, relative to previous scan. */
>  	uint64_t *last; /* Array of engine busyness data as parsed from fdinfo. */
> +	struct drm_client_meminfo *memory; /* Array of region memory utilisation as parsed from fdinfo. */
>  };
>  
>  struct igt_drm_clients {
> -- 
> 2.39.2
> 

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

* Re: [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client
  2023-07-27 14:10   ` Kamil Konieczny
@ 2023-07-27 15:17     ` Tvrtko Ursulin
  2023-07-28 14:46       ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-27 15:17 UTC (permalink / raw)
  To: Kamil Konieczny, igt-dev, Intel-gfx, Tvrtko Ursulin, Rob Clark,
	Chris Healy


Hi,

On 27/07/2023 15:10, Kamil Konieczny wrote:
> Hi Tvrtko,
> 
> On 2023-07-27 at 10:20:24 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Define the storage structure and copy over memory data as parsed by the
>> fdinfo helpers.
>>
>> v2:
>>   * Fix empty region map entry skip condition. (Kamil, Chris)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Rob Clark <robdclark@chromium.org>
>> Cc: Chris Healy <cphealy@gmail.com>
>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> ---
>>   lib/igt_drm_clients.c | 32 ++++++++++++++++++++++++++++++++
>>   lib/igt_drm_clients.h | 11 +++++++++++
>>   2 files changed, 43 insertions(+)
>>
>> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
>> index fdea42752a81..47ad137d5f1f 100644
>> --- a/lib/igt_drm_clients.c
>> +++ b/lib/igt_drm_clients.c
>> @@ -103,6 +103,8 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>>   			c->clients->max_name_len = len;
>>   	}
>>   
>> +	/* Engines */
>> +
>>   	c->last_runtime = 0;
>>   	c->total_runtime = 0;
>>   
>> @@ -118,6 +120,13 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>>   		c->last[i] = info->busy[i];
>>   	}
>>   
>> +	/* Memory regions */
>> +	for (i = 0; i <= c->regions->max_region_id; i++) {
>> +		assert(i < ARRAY_SIZE(info->region_mem));
>> +
>> +		c->memory[i] = info->region_mem[i];
>> +	}
>> +
>>   	c->samples++;
>>   	c->status = IGT_DRM_CLIENT_ALIVE;
>>   }
>> @@ -154,6 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>>   	c->id = info->id;
>>   	c->drm_minor = drm_minor;
>>   	c->clients = clients;
>> +
>> +	/* Engines */
>>   	c->engines = malloc(sizeof(*c->engines));
>>   	assert(c->engines);
>>   	memset(c->engines, 0, sizeof(*c->engines));
>> @@ -178,6 +189,27 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>>   	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
>>   	assert(c->val && c->last);
>>   
>> +	/* Memory regions */
>> +	c->regions = malloc(sizeof(*c->regions));
>> +	assert(c->regions);
>> +	memset(c->regions, 0, sizeof(*c->regions));
>> +	c->regions->names = calloc(info->last_region_index + 1,
>> +				   sizeof(*c->regions->names));
>> +	assert(c->regions->names);
>> +
>> +	for (i = 0; i <= info->last_region_index; i++) {
>> +		/* Region map is allowed to be sparse. */
>> +		if (!info->region_names[i][0])
>> +			continue;
>> +
>> +		c->regions->names[i] = strdup(info->region_names[i]);
> --------------------------------- ^
> Should this be c->regions->num_regions?

No, it was supposed to carry over the same memory region index from the 
region map provided to igt_parse_drm_fdinfo.

I copy pasted that concept from engine names (class names for i915) but, 
given it is unused, maybe I should drop it.

Gputop does not need it, at least not yet, and I haven't thought much if 
it will be useful for intel_gpu_top. Point is, it allows passing in 
fixed region id to name mapping, which can simplify things and guarantee 
order of memory regions in the arrays. (Otherwise the order would depend 
on the order of keys in the fdinfo text.)

Regards,

Tvrtko

> 
> Regards,
> Kamil
> 
>> +		assert(c->regions->names[i]);
>> +		c->regions->num_regions++;
>> +		c->regions->max_region_id = i;
>> +	}
>> +	c->memory = calloc(c->regions->max_region_id + 1, sizeof(*c->memory));
>> +	assert(c->memory);
>> +
>>   	igt_drm_client_update(c, pid, name, info);
>>   }
>>   
>> diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
>> index ed795c193986..07bd236d43bf 100644
>> --- a/lib/igt_drm_clients.h
>> +++ b/lib/igt_drm_clients.h
>> @@ -8,6 +8,8 @@
>>   
>>   #include <stdint.h>
>>   
>> +#include "lib/igt_drm_fdinfo.h"
>> +
>>   /**
>>    * SECTION:igt_drm_clients
>>    * @short_description: Parsing driver exposed fdinfo to track DRM clients
>> @@ -39,12 +41,20 @@ struct igt_drm_client_engines {
>>   	char **names; /* Array of engine names, either auto-detected or from the passed in engine map. */
>>   };
>>   
>> +struct igt_drm_client_regions {
>> +	unsigned int num_regions; /* Number of discovered memory_regions. */
>> +	unsigned int max_region_id; /* Largest memory region index discovered.
>> +				       (Can differ from num_regions - 1 when using the region map facility.) */
>> +	char **names; /* Array of region names, either auto-detected or from the passed in region map. */
>> +};
>> +
>>   struct igt_drm_clients;
>>   
>>   struct igt_drm_client {
>>   	struct igt_drm_clients *clients; /* Owning list. */
>>   
>>   	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 int drm_minor; /* DRM minor of this client. */
>> @@ -57,6 +67,7 @@ struct igt_drm_client {
>>   	unsigned long last_runtime; /* Aggregate busyness on all engines since previous scan. */
>>   	unsigned long *val; /* Array of engine busyness data, relative to previous scan. */
>>   	uint64_t *last; /* Array of engine busyness data as parsed from fdinfo. */
>> +	struct drm_client_meminfo *memory; /* Array of region memory utilisation as parsed from fdinfo. */
>>   };
>>   
>>   struct igt_drm_clients {
>> -- 
>> 2.39.2
>>

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

* Re: [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client
  2023-07-27 15:17     ` Tvrtko Ursulin
@ 2023-07-28 14:46       ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2023-07-28 14:46 UTC (permalink / raw)
  To: Kamil Konieczny, igt-dev, Intel-gfx, Tvrtko Ursulin, Rob Clark,
	Chris Healy


On 27/07/2023 16:17, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 27/07/2023 15:10, Kamil Konieczny wrote:
>> Hi Tvrtko,
>>
>> On 2023-07-27 at 10:20:24 +0100, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Define the storage structure and copy over memory data as parsed by the
>>> fdinfo helpers.
>>>
>>> v2:
>>>   * Fix empty region map entry skip condition. (Kamil, Chris)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Rob Clark <robdclark@chromium.org>
>>> Cc: Chris Healy <cphealy@gmail.com>
>>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>> ---
>>>   lib/igt_drm_clients.c | 32 ++++++++++++++++++++++++++++++++
>>>   lib/igt_drm_clients.h | 11 +++++++++++
>>>   2 files changed, 43 insertions(+)
>>>
>>> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
>>> index fdea42752a81..47ad137d5f1f 100644
>>> --- a/lib/igt_drm_clients.c
>>> +++ b/lib/igt_drm_clients.c
>>> @@ -103,6 +103,8 @@ igt_drm_client_update(struct igt_drm_client *c, 
>>> unsigned int pid, char *name,
>>>               c->clients->max_name_len = len;
>>>       }
>>> +    /* Engines */
>>> +
>>>       c->last_runtime = 0;
>>>       c->total_runtime = 0;
>>> @@ -118,6 +120,13 @@ igt_drm_client_update(struct igt_drm_client *c, 
>>> unsigned int pid, char *name,
>>>           c->last[i] = info->busy[i];
>>>       }
>>> +    /* Memory regions */
>>> +    for (i = 0; i <= c->regions->max_region_id; i++) {
>>> +        assert(i < ARRAY_SIZE(info->region_mem));
>>> +
>>> +        c->memory[i] = info->region_mem[i];
>>> +    }
>>> +
>>>       c->samples++;
>>>       c->status = IGT_DRM_CLIENT_ALIVE;
>>>   }
>>> @@ -154,6 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>>>       c->id = info->id;
>>>       c->drm_minor = drm_minor;
>>>       c->clients = clients;
>>> +
>>> +    /* Engines */
>>>       c->engines = malloc(sizeof(*c->engines));
>>>       assert(c->engines);
>>>       memset(c->engines, 0, sizeof(*c->engines));
>>> @@ -178,6 +189,27 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>>>       c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
>>>       assert(c->val && c->last);
>>> +    /* Memory regions */
>>> +    c->regions = malloc(sizeof(*c->regions));
>>> +    assert(c->regions);
>>> +    memset(c->regions, 0, sizeof(*c->regions));
>>> +    c->regions->names = calloc(info->last_region_index + 1,
>>> +                   sizeof(*c->regions->names));
>>> +    assert(c->regions->names);
>>> +
>>> +    for (i = 0; i <= info->last_region_index; i++) {
>>> +        /* Region map is allowed to be sparse. */
>>> +        if (!info->region_names[i][0])
>>> +            continue;
>>> +
>>> +        c->regions->names[i] = strdup(info->region_names[i]);
>> --------------------------------- ^
>> Should this be c->regions->num_regions?
> 
> No, it was supposed to carry over the same memory region index from the 
> region map provided to igt_parse_drm_fdinfo.
> 
> I copy pasted that concept from engine names (class names for i915) but, 
> given it is unused, maybe I should drop it.
> 
> Gputop does not need it, at least not yet, and I haven't thought much if 
> it will be useful for intel_gpu_top. Point is, it allows passing in 
> fixed region id to name mapping, which can simplify things and guarantee 
> order of memory regions in the arrays. (Otherwise the order would depend 
> on the order of keys in the fdinfo text.)

I think I'd like to keep this functionality for now. I do promise to rip 
it out later, should I end up not needing it for intel_gpu_top after all.

Regards,

Tvrtko

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

* Re: [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client
  2023-07-27  9:20 ` [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client Tvrtko Ursulin
  2023-07-27 14:10   ` Kamil Konieczny
@ 2023-08-07 14:40   ` Kamil Konieczny
  1 sibling, 0 replies; 15+ messages in thread
From: Kamil Konieczny @ 2023-08-07 14:40 UTC (permalink / raw)
  To: igt-dev; +Cc: Rob Clark, Intel-gfx, Chris Healy

Hi Tvrtko,

On 2023-07-27 at 10:20:24 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Define the storage structure and copy over memory data as parsed by the
> fdinfo helpers.
> 
> v2:
>  * Fix empty region map entry skip condition. (Kamil, Chris)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Rob Clark <robdclark@chromium.org>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>

Thx for v2 with fix and reply, LGTM,

Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> ---
>  lib/igt_drm_clients.c | 32 ++++++++++++++++++++++++++++++++
>  lib/igt_drm_clients.h | 11 +++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index fdea42752a81..47ad137d5f1f 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -103,6 +103,8 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>  			c->clients->max_name_len = len;
>  	}
>  
> +	/* Engines */
> +
>  	c->last_runtime = 0;
>  	c->total_runtime = 0;
>  
> @@ -118,6 +120,13 @@ igt_drm_client_update(struct igt_drm_client *c, unsigned int pid, char *name,
>  		c->last[i] = info->busy[i];
>  	}
>  
> +	/* Memory regions */
> +	for (i = 0; i <= c->regions->max_region_id; i++) {
> +		assert(i < ARRAY_SIZE(info->region_mem));
> +
> +		c->memory[i] = info->region_mem[i];
> +	}
> +
>  	c->samples++;
>  	c->status = IGT_DRM_CLIENT_ALIVE;
>  }
> @@ -154,6 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>  	c->id = info->id;
>  	c->drm_minor = drm_minor;
>  	c->clients = clients;
> +
> +	/* Engines */
>  	c->engines = malloc(sizeof(*c->engines));
>  	assert(c->engines);
>  	memset(c->engines, 0, sizeof(*c->engines));
> @@ -178,6 +189,27 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>  	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
>  	assert(c->val && c->last);
>  
> +	/* Memory regions */
> +	c->regions = malloc(sizeof(*c->regions));
> +	assert(c->regions);
> +	memset(c->regions, 0, sizeof(*c->regions));
> +	c->regions->names = calloc(info->last_region_index + 1,
> +				   sizeof(*c->regions->names));
> +	assert(c->regions->names);
> +
> +	for (i = 0; i <= info->last_region_index; i++) {
> +		/* Region map is allowed to be sparse. */
> +		if (!info->region_names[i][0])
> +			continue;
> +
> +		c->regions->names[i] = strdup(info->region_names[i]);
> +		assert(c->regions->names[i]);
> +		c->regions->num_regions++;
> +		c->regions->max_region_id = i;
> +	}
> +	c->memory = calloc(c->regions->max_region_id + 1, sizeof(*c->memory));
> +	assert(c->memory);
> +
>  	igt_drm_client_update(c, pid, name, info);
>  }
>  
> diff --git a/lib/igt_drm_clients.h b/lib/igt_drm_clients.h
> index ed795c193986..07bd236d43bf 100644
> --- a/lib/igt_drm_clients.h
> +++ b/lib/igt_drm_clients.h
> @@ -8,6 +8,8 @@
>  
>  #include <stdint.h>
>  
> +#include "lib/igt_drm_fdinfo.h"
> +
>  /**
>   * SECTION:igt_drm_clients
>   * @short_description: Parsing driver exposed fdinfo to track DRM clients
> @@ -39,12 +41,20 @@ struct igt_drm_client_engines {
>  	char **names; /* Array of engine names, either auto-detected or from the passed in engine map. */
>  };
>  
> +struct igt_drm_client_regions {
> +	unsigned int num_regions; /* Number of discovered memory_regions. */
> +	unsigned int max_region_id; /* Largest memory region index discovered.
> +				       (Can differ from num_regions - 1 when using the region map facility.) */
> +	char **names; /* Array of region names, either auto-detected or from the passed in region map. */
> +};
> +
>  struct igt_drm_clients;
>  
>  struct igt_drm_client {
>  	struct igt_drm_clients *clients; /* Owning list. */
>  
>  	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 int drm_minor; /* DRM minor of this client. */
> @@ -57,6 +67,7 @@ struct igt_drm_client {
>  	unsigned long last_runtime; /* Aggregate busyness on all engines since previous scan. */
>  	unsigned long *val; /* Array of engine busyness data, relative to previous scan. */
>  	uint64_t *last; /* Array of engine busyness data as parsed from fdinfo. */
> +	struct drm_client_meminfo *memory; /* Array of region memory utilisation as parsed from fdinfo. */
>  };
>  
>  struct igt_drm_clients {
> -- 
> 2.39.2
> 

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

end of thread, other threads:[~2023-08-07 14:41 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-05 16:31 [Intel-gfx] [PATCH i-g-t 0/3] gputop memory usage Tvrtko Ursulin
2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 1/3] lib/igt_drm_fdinfo: Parse " Tvrtko Ursulin
2023-07-26 17:28   ` Kamil Konieczny
2023-07-27  8:51     ` Tvrtko Ursulin
2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client Tvrtko Ursulin
2023-07-26 17:00   ` Kamil Konieczny
2023-07-27  9:08     ` Tvrtko Ursulin
2023-07-05 16:31 ` [Intel-gfx] [PATCH i-g-t 3/3] gputop: Add memory information Tvrtko Ursulin
2023-07-26 17:32   ` Kamil Konieczny
2023-07-27  9:09     ` Tvrtko Ursulin
  -- strict thread matches above, loose matches on Subject: below --
2023-07-27  9:20 [Intel-gfx] [PATCH i-g-t 0/3] gputop memory usage Tvrtko Ursulin
2023-07-27  9:20 ` [Intel-gfx] [PATCH i-g-t 2/3] lib/igt_drm_clients: Store memory info in the client Tvrtko Ursulin
2023-07-27 14:10   ` Kamil Konieczny
2023-07-27 15:17     ` Tvrtko Ursulin
2023-07-28 14:46       ` Tvrtko Ursulin
2023-08-07 14:40   ` Kamil Konieczny

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox