[Intel-gfx] [PATCH i-g-t 1/3] lib/igt_drm_fdinfo: Parse memory usage

Tvrtko Ursulin tvrtko.ursulin at linux.intel.com
Thu Jul 27 08:51:42 UTC 2023


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 at chromium.org>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Done.

> On 2023-07-05 at 17:31:03 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at 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 at chromium.org>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at 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
>>


More information about the Intel-gfx mailing list