[PATCH i-g-t 0/8] Refactors and fixes for drm_clients

[PATCH i-g-t 0/8] Refactors and fixes for drm_clients

[Lucas De Marchi]

Some refactors and fixes I did while adding support for
gputop in xe. I will send those later on top, but I think
these can already be reviewed.

Last patch should NOT be applied for now. In my mind the lazy approach
makes total sense and should be faster. But reality doesn't agree with
me and after timing igt_drm_clients_scan() it shows a ~10% slow down.
Hard to explain. Maybe I didn't have enough coffee when benchmarking it. 

I don't like much the strstartswith() with and extra output param.
Maybe changing its name to something else would make it better.

Lucas De Marchi (8):
  lib/igt_drm_clients: Use calloc
  lib/igt_drm_clients: Fix sizeof calculation
  lib/igt_drm_clients: Fix leaks
  gputop: Free clients on exit
  lib/igt_drm_fdinfo: Simplify find_kv()
  lib/igt_drm_fdinfo: Stop passing key twice
  lib/igt_drm_fdinfo: Remove prefix arg from parse functions
  lib/igt_drm_clients: lazy stat process

 lib/igt_drm_clients.c |  74 ++++++++++++-----------
 lib/igt_drm_fdinfo.c  | 137 ++++++++++++++++++++----------------------
 tools/gputop.c        |   2 +
 3 files changed, 108 insertions(+), 105 deletions(-)

-- 
2.43.0

[PATCH i-g-t 1/8] lib/igt_drm_clients: Use calloc

[Lucas De Marchi]

Replace malloc + memset with a single calloc as it's shorter and there's
no point keeping them separate.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 lib/igt_drm_clients.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index 025d60c51..90ca6d61c 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -35,12 +35,10 @@ struct igt_drm_clients *igt_drm_clients_init(void *private_data)
 {
 	struct igt_drm_clients *clients;
 
-	clients = malloc(sizeof(*clients));
+	clients = calloc(1, sizeof(*clients));
 	if (!clients)
 		return NULL;
 
-	memset(clients, 0, sizeof(*clients));
-
 	clients->private_data = private_data;
 
 	return clients;
@@ -165,9 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
 	c->clients = clients;
 
 	/* Engines */
-	c->engines = malloc(sizeof(*c->engines));
+	c->engines = calloc(1, sizeof(*c->engines));
 	assert(c->engines);
-	memset(c->engines, 0, sizeof(*c->engines));
 	c->engines->capacity = calloc(info->last_engine_index + 1,
 				      sizeof(*c->engines->capacity));
 	assert(c->engines->capacity);
@@ -190,9 +187,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
 	assert(c->val && c->last);
 
 	/* Memory regions */
-	c->regions = malloc(sizeof(*c->regions));
+	c->regions = calloc(1, 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);
-- 
2.43.0

[PATCH i-g-t 2/8] lib/igt_drm_clients: Fix sizeof calculation

[Lucas De Marchi]

val and last are arrays of certain types. For the latter, it's
underallocated on 32-bits since it should be sizeof(u64) not the size of
a pointer. When running on 64-bits, no real bug.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 lib/igt_drm_clients.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index 90ca6d61c..c8dc21d6a 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -182,8 +182,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
 		c->engines->num_engines++;
 		c->engines->max_engine_id = i;
 	}
-	c->val = calloc(c->engines->max_engine_id + 1, sizeof(c->val));
-	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
+	c->val = calloc(c->engines->max_engine_id + 1, sizeof(*c->val));
+	c->last = calloc(c->engines->max_engine_id + 1, sizeof(*c->last));
 	assert(c->val && c->last);
 
 	/* Memory regions */
-- 
2.43.0

[PATCH i-g-t 3/8] lib/igt_drm_clients: Fix leaks

[Lucas De Marchi]

Stop leaking memory-related fields.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 lib/igt_drm_clients.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index c8dc21d6a..c174c96ab 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -221,9 +221,19 @@ void igt_drm_client_free(struct igt_drm_client *c, bool clear)
 		free(c->engines->names);
 	}
 	free(c->engines);
+
 	free(c->val);
 	free(c->last);
 
+	if (c->regions) {
+		for (i = 0; i <= c->regions->max_region_id; i++)
+			free(c->regions->names[i]);
+		free(c->regions->names);
+	}
+	free(c->regions);
+
+	free(c->memory);
+
 	if (clear)
 		memset(c, 0, sizeof(*c));
 }
-- 
2.43.0

[PATCH i-g-t 4/8] gputop: Free clients on exit

[Lucas De Marchi]

So it's easily checked with valgrind if we there's something leaking.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 tools/gputop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/gputop.c b/tools/gputop.c
index 71e28f43e..b13044b50 100644
--- a/tools/gputop.c
+++ b/tools/gputop.c
@@ -293,5 +293,7 @@ int main(int argc, char **argv)
 		usleep(period_us);
 	}
 
+	igt_drm_clients_free(clients);
+
 	return 0;
 }
-- 
2.43.0

[PATCH i-g-t 5/8] lib/igt_drm_fdinfo: Simplify find_kv()

[Lucas De Marchi]

index() is deprecate from libc and strchr() should rather be used.
Moreover the return value is NULL or a pointer to the occurrence, never
the initial string and `(p - buf) != keylen` can only be true if ':' is
the next char after the key.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 lib/igt_drm_fdinfo.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
index 17cac4009..15f71e172 100644
--- a/lib/igt_drm_fdinfo.c
+++ b/lib/igt_drm_fdinfo.c
@@ -106,20 +106,17 @@ static int parse_engine(char *line, struct drm_client_fdinfo *info,
 
 static const char *find_kv(const char *buf, const char *key, size_t keylen)
 {
-	const char *p = buf;
+	const char *p;
 
 	if (strncmp(buf, key, keylen))
 		return NULL;
 
-	p = index(buf, ':');
-	if (!p || p == buf)
-		return NULL;
-	if ((p - buf) != keylen)
+	p = buf + keylen;
+	if (*p != ':')
 		return NULL;
 
-	p++;
-	while (*p && isspace(*p))
-		p++;
+	for (p++; *p && isspace(*p); p++)
+		;
 
 	return *p ? p : NULL;
 }
@@ -233,8 +230,7 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
 		if ((v = find_kv(l, "drm-driver", strlen("drm-driver")))) {
 			strncpy(info->driver, v, sizeof(info->driver) - 1);
 			good++;
-		}  else if ((v = find_kv(l, "drm-client-id",
-					 strlen("drm-client-id")))) {
+		}  else if ((v = find_kv(l, "drm-client-id", strlen("drm-client-id")))) {
 			info->id = atol(v);
 			good++;
 		} else if ((v = find_kv(l, "drm-pdev", strlen("drm-pdev")))) {
-- 
2.43.0

[PATCH i-g-t 6/8] lib/igt_drm_fdinfo: Stop passing key twice

[Lucas De Marchi]

Change strstartswith() so it also returns the length, which then can be
used inside the branch when it matches. A good compiler shall optimize
out the strlen call since the argument is a constant literal.

With this, the find_kv() is not needed anymore and the difference with
regard the other branches can be summarized with a new ignore_space()
helper and the fact it matches the entire key by appending the ':'. The
helper is added on top of the file so it can be reused later.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 lib/igt_drm_fdinfo.c | 84 +++++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 36 deletions(-)

diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
index 15f71e172..a0b41e956 100644
--- a/lib/igt_drm_fdinfo.c
+++ b/lib/igt_drm_fdinfo.c
@@ -53,6 +53,14 @@ static size_t read_fdinfo(char *buf, const size_t sz, int at, const char *name)
 	return count > 0 ? count : 0;
 }
 
+static const char *ignore_space(const char *s)
+{
+	for (; *s && isspace(*s); s++)
+		;
+
+	return s;
+}
+
 static int parse_engine(char *line, struct drm_client_fdinfo *info,
 			size_t prefix_len,
 			const char **name_map, unsigned int map_entries,
@@ -104,21 +112,12 @@ static int parse_engine(char *line, struct drm_client_fdinfo *info,
 	return found;
 }
 
-static const char *find_kv(const char *buf, const char *key, size_t keylen)
+static const char *ignore_space(const char *s)
 {
-	const char *p;
-
-	if (strncmp(buf, key, keylen))
-		return NULL;
-
-	p = buf + keylen;
-	if (*p != ':')
-		return NULL;
-
-	for (p++; *p && isspace(*p); p++)
+	for (; *s && isspace(*s); s++)
 		;
 
-	return *p ? p : NULL;
+	return s;
 }
 
 static int parse_region(char *line, struct drm_client_fdinfo *info,
@@ -205,6 +204,11 @@ out:
 		}							\
 	} while (0)
 
+#define strstartswith(a, b, plen__) ({					\
+		*plen__ = strlen(b);					\
+		strncmp(a, b, *plen__) == 0;				\
+})
+
 unsigned int
 __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
 		       const char **name_map, unsigned int map_entries,
@@ -222,30 +226,39 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
 
 	while ((l = strtok_r(_buf, "\n", &ctx))) {
 		uint64_t val = 0;
+		size_t keylen;
 		const char *v;
 		int idx;
 
 		_buf = NULL;
 
-		if ((v = find_kv(l, "drm-driver", strlen("drm-driver")))) {
-			strncpy(info->driver, v, sizeof(info->driver) - 1);
-			good++;
-		}  else if ((v = find_kv(l, "drm-client-id", strlen("drm-client-id")))) {
-			info->id = atol(v);
-			good++;
-		} else if ((v = find_kv(l, "drm-pdev", strlen("drm-pdev")))) {
-			/* optional */
+		if (strstartswith(l, "drm-driver:", &keylen)) {
+			v = l + keylen;
+			v = ignore_space(v);
+			if (*v) {
+				strncpy(info->driver, v, sizeof(info->driver) - 1);
+				good++;
+			}
+		}  else if (strstartswith(l, "drm-client-id:", &keylen)) {
+			v = l + keylen;
+			v = ignore_space(v);
+			if (*v) {
+				info->id = atol(v);
+				good++;
+			}
+		} else if (strstartswith(l, "drm-pdev:", &keylen)) {
+			v = l + keylen;
+			v = ignore_space(v);
 			strncpy(info->pdev, v, sizeof(info->pdev) - 1);
-		} else if (!strncmp(l, "drm-engine-capacity-", 20)) {
-			idx = parse_engine(l, info,
-					   strlen("drm-engine-capacity-"),
+		} else if (strstartswith(l, "drm-engine-capacity-", &keylen)) {
+			idx = parse_engine(l, info, keylen,
 					   name_map, map_entries, &val);
 			if (idx >= 0) {
 				info->capacity[idx] = val;
 				num_capacity++;
 			}
-		} else if (!strncmp(l, "drm-engine-", 11)) {
-			idx = parse_engine(l, info, strlen("drm-engine-"),
+		} else if (strstartswith(l, "drm-engine-", &keylen)) {
+			idx = parse_engine(l, info, keylen,
 					   name_map, map_entries, &val);
 			if (idx >= 0) {
 				if (!info->capacity[idx])
@@ -255,25 +268,24 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
 				if (idx > info->last_engine_index)
 					info->last_engine_index = idx;
 			}
-		} else if (!strncmp(l, "drm-total-", 10)) {
-			idx = parse_region(l, info, strlen("drm-total-"),
+		} else if (strstartswith(l, "drm-total-", &keylen)) {
+			idx = parse_region(l, info, keylen,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, total, val);
-		} else if (!strncmp(l, "drm-shared-", 11)) {
-			idx = parse_region(l, info, strlen("drm-shared-"),
+		} else if (strstartswith(l, "drm-shared-", &keylen)) {
+			idx = parse_region(l, info, keylen,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, shared, val);
-
-		} else if (!strncmp(l, "drm-resident-", 13)) {
-			idx = parse_region(l, info, strlen("drm-resident-"),
+		} else if (strstartswith(l, "drm-resident-", &keylen)) {
+			idx = parse_region(l, info, keylen,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, resident, val);
-		} else if (!strncmp(l, "drm-purgeable-", 14)) {
-			idx = parse_region(l, info, strlen("drm-purgeable-"),
+		} else if (strstartswith(l, "drm-purgeable-", &keylen)) {
+			idx = parse_region(l, info, keylen,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, purgeable, val);
-		} else if (!strncmp(l, "drm-active-", 11)) {
-			idx = parse_region(l, info, strlen("drm-active-"),
+		} else if (strstartswith(l, "drm-active-", &keylen)) {
+			idx = parse_region(l, info, keylen,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, active, val);
 		}
-- 
2.43.0

[PATCH i-g-t 7/8] lib/igt_drm_fdinfo: Remove prefix arg from parse functions

[Lucas De Marchi]

The prefix is not really needed and __igt_parse_drm_fdinfo()
can simply pass l + keylen as argument which simplifies the parse
functions.

While refactoring this, it also replaces the remaining calls to index()
to use strchr().

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 lib/igt_drm_fdinfo.c | 69 ++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 41 deletions(-)

diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
index a0b41e956..7a9ae94f0 100644
--- a/lib/igt_drm_fdinfo.c
+++ b/lib/igt_drm_fdinfo.c
@@ -61,25 +61,23 @@ static const char *ignore_space(const char *s)
 	return s;
 }
 
-static int parse_engine(char *line, struct drm_client_fdinfo *info,
-			size_t prefix_len,
+static int parse_engine(const char *name, struct drm_client_fdinfo *info,
 			const char **name_map, unsigned int map_entries,
 			uint64_t *val)
 {
-	ssize_t name_len;
-	char *name, *p;
+	const char *p;
+	size_t name_len;
 	int found = -1;
 	unsigned int i;
 
-	p = index(line, ':');
-	if (!p || p == line)
+	p = strchr(name, ':');
+	if (!p)
 		return -1;
 
-	name_len = p - line - prefix_len;
+	name_len = p - name;
 	if (name_len < 1)
 		return -1;
-
-	name = line + prefix_len;
+	p++;
 
 	if (name_map) {
 		for (i = 0; i < map_entries; i++) {
@@ -105,40 +103,30 @@ static int parse_engine(char *line, struct drm_client_fdinfo *info,
 	}
 
 	if (found >= 0) {
-		while (*++p && isspace(*p));
+		p = ignore_space(p);
 		*val = strtoull(p, NULL, 10);
 	}
 
 	return found;
 }
 
-static const char *ignore_space(const char *s)
-{
-	for (; *s && isspace(*s); s++)
-		;
-
-	return s;
-}
-
-static int parse_region(char *line, struct drm_client_fdinfo *info,
-			size_t prefix_len,
+static int parse_region(const char *name, struct drm_client_fdinfo *info,
 			const char **region_map, unsigned int region_entries,
 			uint64_t *val)
 {
-	char *name, *p, *unit = NULL;
-	ssize_t name_len;
+	const char *p;
+	size_t name_len;
 	int found = -1;
 	unsigned int i;
 
-	p = index(line, ':');
-	if (!p || p == line)
+	p = strchr(name, ':');
+	if (!p)
 		return -1;
 
-	name_len = p - line - prefix_len;
+	name_len = p - name;
 	if (name_len < 1)
 		return -1;
-
-	name = line + prefix_len;
+	p++;
 
 	if (region_map) {
 		for (i = 0; i < region_entries; i++) {
@@ -170,20 +158,19 @@ static int parse_region(char *line, struct drm_client_fdinfo *info,
 	if (found < 0)
 		goto out;
 
-	while (*++p && isspace(*p))
-		;
+	p = ignore_space(p);
 	*val = strtoull(p, NULL, 10);
 
-	p = index(p, ' ');
+	p = strchr(p, ' ');
 	if (!p)
 		goto out;
+	p++;
 
-	unit = ++p;
-	if (!strcmp(unit, "KiB")) {
+	if (!strcmp(p, "KiB")) {
 		*val *= 1024;
-	} else if (!strcmp(unit, "MiB")) {
+	} else if (!strcmp(p, "MiB")) {
 		*val *= 1024 * 1024;
-	} else if (!strcmp(unit, "GiB")) {
+	} else if (!strcmp(p, "GiB")) {
 		*val *= 1024 * 1024 * 1024;
 	}
 
@@ -251,14 +238,14 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
 			v = ignore_space(v);
 			strncpy(info->pdev, v, sizeof(info->pdev) - 1);
 		} else if (strstartswith(l, "drm-engine-capacity-", &keylen)) {
-			idx = parse_engine(l, info, keylen,
+			idx = parse_engine(l + keylen, info,
 					   name_map, map_entries, &val);
 			if (idx >= 0) {
 				info->capacity[idx] = val;
 				num_capacity++;
 			}
 		} else if (strstartswith(l, "drm-engine-", &keylen)) {
-			idx = parse_engine(l, info, keylen,
+			idx = parse_engine(l + keylen, info,
 					   name_map, map_entries, &val);
 			if (idx >= 0) {
 				if (!info->capacity[idx])
@@ -269,23 +256,23 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
 					info->last_engine_index = idx;
 			}
 		} else if (strstartswith(l, "drm-total-", &keylen)) {
-			idx = parse_region(l, info, keylen,
+			idx = parse_region(l + keylen, info,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, total, val);
 		} else if (strstartswith(l, "drm-shared-", &keylen)) {
-			idx = parse_region(l, info, keylen,
+			idx = parse_region(l + keylen, info,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, shared, val);
 		} else if (strstartswith(l, "drm-resident-", &keylen)) {
-			idx = parse_region(l, info, keylen,
+			idx = parse_region(l + keylen, info,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, resident, val);
 		} else if (strstartswith(l, "drm-purgeable-", &keylen)) {
-			idx = parse_region(l, info, keylen,
+			idx = parse_region(l + keylen, info,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, purgeable, val);
 		} else if (strstartswith(l, "drm-active-", &keylen)) {
-			idx = parse_region(l, info, keylen,
+			idx = parse_region(l + keylen, info,
 					   region_map, region_entries, &val);
 			UPDATE_REGION(idx, active, val);
 		}
-- 
2.43.0

[PATCH i-g-t 8/8] lib/igt_drm_clients: lazy stat process

[Lucas De Marchi]

Only try to get task data like pid and name when that is required.
Typically most of the processes in a system don't have a drm fd open.
So only open and use /proc/<pid>/stat when it's needed.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 lib/igt_drm_clients.c | 50 +++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
index c174c96ab..1e9781c68 100644
--- a/lib/igt_drm_clients.c
+++ b/lib/igt_drm_clients.c
@@ -367,26 +367,33 @@ static size_t readat2buf(int at, const char *name, char *buf, const size_t sz)
 	}
 }
 
-static bool get_task_name(const char *buffer, char *out, unsigned long sz)
+static void get_task_data(int pid_dir, unsigned int *pid, char *task, size_t tasksz)
 {
-	char *s = index(buffer, '(');
-	char *e = rindex(buffer, ')');
-	unsigned int len;
+	char buf[4096];
+	char *s, *e;
+	size_t len;
 
+	if (!readat2buf(pid_dir, "stat", buf, sizeof(buf)))
+		return;
+
+	s = strchr(buf, '(');
+	e = strchr(s, ')');
 	if (!s || !e)
-		return false;
-	assert(e >= s);
+		return;
 
 	len = e - ++s;
-	if(!len || (len + 1) >= sz)
-		return false;
+	if (!len)
+		return;
 
-	strncpy(out, s, len);
-	out[len] = 0;
+	if (len + 1 > tasksz)
+		len = tasksz - 1;
 
-	return true;
+	strncpy(task, s, len);
+	task[len] = 0;
+	*pid = atoi(buf);
 }
 
+
 static bool is_drm_fd(int fd_dir, const char *name, unsigned int *minor)
 {
 	struct stat stat;
@@ -480,13 +487,11 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
 		return clients;
 
 	while ((proc_dent = readdir(proc_dir)) != NULL) {
-		unsigned int client_pid, minor = 0;
+		unsigned int client_pid = 0, minor = 0;
 		int pid_dir = -1, fd_dir = -1;
 		struct dirent *fdinfo_dent;
 		char client_name[64] = { };
 		DIR *fdinfo_dir = NULL;
-		char buf[4096];
-		size_t count;
 
 		if (proc_dent->d_type != DT_DIR)
 			continue;
@@ -498,17 +503,6 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
 		if (pid_dir < 0)
 			continue;
 
-		count = readat2buf(pid_dir, "stat", buf, sizeof(buf));
-		if (!count)
-			goto next;
-
-		client_pid = atoi(buf);
-		if (!client_pid)
-			goto next;
-
-		if (!get_task_name(buf, client_name, sizeof(client_name)))
-			goto next;
-
 		fd_dir = openat(pid_dir, "fd", O_DIRECTORY | O_RDONLY);
 		if (fd_dir < 0)
 			goto next;
@@ -541,6 +535,12 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
 						 minor, info.id))
 				continue; /* Skip duplicate fds. */
 
+			if (!client_pid) {
+				get_task_data(pid_dir, &client_pid, client_name,
+					      sizeof(client_name));
+				assert(client_pid > 0);
+			}
+
 			c = igt_drm_clients_find(clients, IGT_DRM_CLIENT_PROBE,
 						 minor, info.id);
 			if (!c)
-- 
2.43.0

✗ Fi.CI.BAT: failure for Refactors and fixes for drm_clients

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 15366 bytes --]

== Series Details ==

Series: Refactors and fixes for drm_clients
URL   : https://patchwork.freedesktop.org/series/131956/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14519 -> IGTPW_10967
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_10967 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_10967, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/index.html

Participating hosts (34 -> 36)
------------------------------

  Additional (5): fi-glk-j4005 fi-kbl-8809g fi-bsw-nick bat-mtlp-8 bat-arls-3 
  Missing    (3): bat-dg1-7 fi-cfl-8109u fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_10967:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@execlists:
    - bat-adls-6:         [PASS][1] -> [TIMEOUT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14519/bat-adls-6/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-adls-6/igt@i915_selftest@live@execlists.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@gem_contexts:
    - {bat-arls-4}:       [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14519/bat-arls-4/igt@i915_selftest@live@gem_contexts.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-4/igt@i915_selftest@live@gem_contexts.html

  
Known issues
------------

  Here are the changes found in IGTPW_10967 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-mtlp-8:         NOTRUN -> [SKIP][5] ([i915#9318])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@debugfs_test@basic-hwmon.html
    - bat-arls-3:         NOTRUN -> [SKIP][6] ([i915#9318])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@debugfs_test@basic-hwmon.html

  * igt@gem_huc_copy@huc-copy:
    - fi-glk-j4005:       NOTRUN -> [SKIP][7] ([i915#2190])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/fi-glk-j4005/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-glk-j4005:       NOTRUN -> [SKIP][8] ([i915#4613]) +3 other tests skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/fi-glk-j4005/igt@gem_lmem_swapping@basic.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - fi-bsw-nick:        NOTRUN -> [SKIP][9] +19 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/fi-bsw-nick/igt@gem_lmem_swapping@parallel-random-engines.html
    - bat-arls-3:         NOTRUN -> [SKIP][10] ([i915#10213]) +3 other tests skip
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_lmem_swapping@verify-random:
    - bat-mtlp-8:         NOTRUN -> [SKIP][11] ([i915#4613]) +3 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@gem_lmem_swapping@verify-random.html

  * igt@gem_mmap@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][12] ([i915#4083])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@gem_mmap@basic.html
    - bat-arls-3:         NOTRUN -> [SKIP][13] ([i915#4083])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][14] ([i915#4077]) +2 other tests skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@gem_mmap_gtt@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-mtlp-8:         NOTRUN -> [SKIP][15] ([i915#4079]) +1 other test skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@gem_render_tiled_blits@basic.html
    - bat-arls-3:         NOTRUN -> [SKIP][16] ([i915#10197] / [i915#10211] / [i915#4079])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_blits@basic:
    - bat-arls-3:         NOTRUN -> [SKIP][17] ([i915#10196] / [i915#4077]) +2 other tests skip
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@gem_tiled_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-arls-3:         NOTRUN -> [SKIP][18] ([i915#10206] / [i915#4079])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-mtlp-8:         NOTRUN -> [SKIP][19] ([i915#6621])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@i915_pm_rps@basic-api.html
    - bat-arls-3:         NOTRUN -> [SKIP][20] ([i915#10209])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@dmabuf:
    - bat-dg2-14:         [PASS][21] -> [ABORT][22] ([i915#10366])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14519/bat-dg2-14/igt@i915_selftest@live@dmabuf.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-dg2-14/igt@i915_selftest@live@dmabuf.html

  * igt@kms_addfb_basic@addfb25-x-tiled-legacy:
    - bat-arls-3:         NOTRUN -> [SKIP][23] ([i915#10200]) +9 other tests skip
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@kms_addfb_basic@addfb25-x-tiled-legacy.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][24] ([i915#5190])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][25] ([i915#4212]) +8 other tests skip
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-glk-j4005:       NOTRUN -> [SKIP][26] +10 other tests skip
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/fi-glk-j4005/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
    - bat-arls-3:         NOTRUN -> [SKIP][27] ([i915#10202]) +1 other test skip
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-mtlp-8:         NOTRUN -> [SKIP][28] ([i915#4213]) +1 other test skip
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-arls-3:         NOTRUN -> [SKIP][29] ([i915#9886])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@kms_dsc@dsc-basic.html
    - bat-mtlp-8:         NOTRUN -> [SKIP][30] ([i915#3555] / [i915#3840] / [i915#9159])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-connector-state:
    - bat-dg2-9:          [PASS][31] -> [ABORT][32] ([i915#10583])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14519/bat-dg2-9/igt@kms_force_connector_basic@force-connector-state.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-dg2-9/igt@kms_force_connector_basic@force-connector-state.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-arls-3:         NOTRUN -> [SKIP][33] ([i915#10207])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@kms_force_connector_basic@force-load-detect.html
    - bat-mtlp-8:         NOTRUN -> [SKIP][34]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-mtlp-8:         NOTRUN -> [SKIP][35] ([i915#5274])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-arls-3:         NOTRUN -> [SKIP][36] ([i915#9812])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-primary-mmap-gtt:
    - bat-arls-3:         NOTRUN -> [SKIP][37] ([i915#9732]) +3 other tests skip
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@kms_psr@psr-primary-mmap-gtt.html

  * igt@kms_psr@psr-primary-mmap-gtt@edp-1:
    - bat-mtlp-8:         NOTRUN -> [SKIP][38] ([i915#4077] / [i915#9688])
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@kms_psr@psr-primary-mmap-gtt@edp-1.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-mtlp-8:         NOTRUN -> [SKIP][39] ([i915#3555] / [i915#8809])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@kms_setmode@basic-clone-single-crtc.html
    - bat-arls-3:         NOTRUN -> [SKIP][40] ([i915#10208] / [i915#8809])
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-arls-3:         NOTRUN -> [SKIP][41] ([i915#10196] / [i915#3708] / [i915#4077]) +1 other test skip
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@prime_vgem@basic-fence-mmap.html
    - bat-mtlp-8:         NOTRUN -> [SKIP][42] ([i915#3708] / [i915#4077]) +1 other test skip
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-fence-read:
    - bat-mtlp-8:         NOTRUN -> [SKIP][43] ([i915#3708]) +1 other test skip
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@prime_vgem@basic-fence-read.html
    - bat-arls-3:         NOTRUN -> [SKIP][44] ([i915#10212] / [i915#3708])
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@prime_vgem@basic-fence-read.html

  * igt@prime_vgem@basic-read:
    - bat-arls-3:         NOTRUN -> [SKIP][45] ([i915#10214] / [i915#3708])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-write:
    - bat-mtlp-8:         NOTRUN -> [SKIP][46] ([i915#10216] / [i915#3708])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-8/igt@prime_vgem@basic-write.html
    - bat-arls-3:         NOTRUN -> [SKIP][47] ([i915#10216] / [i915#3708])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-arls-3/igt@prime_vgem@basic-write.html

  * igt@runner@aborted:
    - fi-kbl-8809g:       NOTRUN -> [FAIL][48] ([i915#4991])
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/fi-kbl-8809g/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_timelines:
    - bat-dg2-11:         [ABORT][49] ([i915#10366]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14519/bat-dg2-11/igt@i915_selftest@live@gt_timelines.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-dg2-11/igt@i915_selftest@live@gt_timelines.html

  * igt@i915_selftest@live@hangcheck:
    - {bat-mtlp-9}:       [DMESG-WARN][51] ([i915#9522]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14519/bat-mtlp-9/igt@i915_selftest@live@hangcheck.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/bat-mtlp-9/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#10196]: https://gitlab.freedesktop.org/drm/intel/issues/10196
  [i915#10197]: https://gitlab.freedesktop.org/drm/intel/issues/10197
  [i915#10200]: https://gitlab.freedesktop.org/drm/intel/issues/10200
  [i915#10202]: https://gitlab.freedesktop.org/drm/intel/issues/10202
  [i915#10206]: https://gitlab.freedesktop.org/drm/intel/issues/10206
  [i915#10207]: https://gitlab.freedesktop.org/drm/intel/issues/10207
  [i915#10208]: https://gitlab.freedesktop.org/drm/intel/issues/10208
  [i915#10209]: https://gitlab.freedesktop.org/drm/intel/issues/10209
  [i915#10211]: https://gitlab.freedesktop.org/drm/intel/issues/10211
  [i915#10212]: https://gitlab.freedesktop.org/drm/intel/issues/10212
  [i915#10213]: https://gitlab.freedesktop.org/drm/intel/issues/10213
  [i915#10214]: https://gitlab.freedesktop.org/drm/intel/issues/10214
  [i915#10216]: https://gitlab.freedesktop.org/drm/intel/issues/10216
  [i915#10366]: https://gitlab.freedesktop.org/drm/intel/issues/10366
  [i915#10435]: https://gitlab.freedesktop.org/drm/intel/issues/10435
  [i915#10583]: https://gitlab.freedesktop.org/drm/intel/issues/10583
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4991]: https://gitlab.freedesktop.org/drm/intel/issues/4991
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
  [i915#9157]: https://gitlab.freedesktop.org/drm/intel/issues/9157
  [i915#9159]: https://gitlab.freedesktop.org/drm/intel/issues/9159
  [i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
  [i915#9522]: https://gitlab.freedesktop.org/drm/intel/issues/9522
  [i915#9688]: https://gitlab.freedesktop.org/drm/intel/issues/9688
  [i915#9732]: https://gitlab.freedesktop.org/drm/intel/issues/9732
  [i915#9812]: https://gitlab.freedesktop.org/drm/intel/issues/9812
  [i915#9886]: https://gitlab.freedesktop.org/drm/intel/issues/9886


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7796 -> IGTPW_10967

  CI-20190529: 20190529
  CI_DRM_14519: f4b306bc42834d24085b45a5a3d1521a451b2abf @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_10967: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/index.html
  IGT_7796: 2cfed18f6aa776c1593d7cc328d23225dd61bdf9 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/index.html

[-- Attachment #2: Type: text/html, Size: 18068 bytes --]

✓ CI.xeBAT: success for Refactors and fixes for drm_clients

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6071 bytes --]

== Series Details ==

Series: Refactors and fixes for drm_clients
URL   : https://patchwork.freedesktop.org/series/131956/
State : success

== Summary ==

CI Bug Log - changes from XEIGT_7796_BAT -> XEIGTPW_10967_BAT
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (5 -> 5)
------------------------------

  No changes in participating hosts

Known issues
------------

  Here are the changes found in XEIGTPW_10967_BAT that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_addfb_basic@addfb25-x-tiled-legacy:
    - bat-pvc-2:          NOTRUN -> [SKIP][1] ([i915#6077]) +30 other tests skip
   [1]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@kms_addfb_basic@addfb25-x-tiled-legacy.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-atomic:
    - bat-pvc-2:          NOTRUN -> [SKIP][2] ([Intel XE#1024] / [Intel XE#782]) +5 other tests skip
   [2]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html

  * igt@kms_dsc@dsc-basic:
    - bat-pvc-2:          NOTRUN -> [SKIP][3] ([Intel XE#1024] / [Intel XE#784])
   [3]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@kms_dsc@dsc-basic.html

  * igt@kms_flip@basic-flip-vs-wf_vblank:
    - bat-pvc-2:          NOTRUN -> [SKIP][4] ([Intel XE#1024] / [Intel XE#947]) +3 other tests skip
   [4]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@kms_flip@basic-flip-vs-wf_vblank.html

  * igt@kms_force_connector_basic@force-connector-state:
    - bat-pvc-2:          NOTRUN -> [SKIP][5] ([Intel XE#540]) +3 other tests skip
   [5]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@kms_force_connector_basic@force-connector-state.html

  * igt@kms_frontbuffer_tracking@basic:
    - bat-pvc-2:          NOTRUN -> [SKIP][6] ([Intel XE#1024] / [Intel XE#783])
   [6]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pipe_crc_basic@nonblocking-crc:
    - bat-pvc-2:          NOTRUN -> [SKIP][7] ([Intel XE#829]) +6 other tests skip
   [7]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@kms_pipe_crc_basic@nonblocking-crc.html

  * igt@kms_prop_blob@basic:
    - bat-pvc-2:          NOTRUN -> [SKIP][8] ([Intel XE#780])
   [8]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@kms_prop_blob@basic.html

  * igt@kms_psr@psr-cursor-plane-move:
    - bat-pvc-2:          NOTRUN -> [SKIP][9] ([Intel XE#1024]) +2 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@kms_psr@psr-cursor-plane-move.html

  * igt@xe_gt_freq@freq_range_idle:
    - bat-pvc-2:          NOTRUN -> [SKIP][10] ([Intel XE#1021]) +1 other test skip
   [10]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@xe_gt_freq@freq_range_idle.html

  * igt@xe_huc_copy@huc_copy:
    - bat-pvc-2:          NOTRUN -> [SKIP][11] ([Intel XE#255])
   [11]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@xe_huc_copy@huc_copy.html

  * igt@xe_intel_bb@render:
    - bat-pvc-2:          NOTRUN -> [SKIP][12] ([Intel XE#532])
   [12]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@xe_intel_bb@render.html

  * igt@xe_pat@pat-index-xe2:
    - bat-pvc-2:          NOTRUN -> [SKIP][13] ([Intel XE#977]) +1 other test skip
   [13]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@xe_pat@pat-index-xe2.html

  * igt@xe_pat@pat-index-xehpc@render:
    - bat-pvc-2:          NOTRUN -> [SKIP][14] ([Intel XE#976])
   [14]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@xe_pat@pat-index-xehpc@render.html

  * igt@xe_pat@pat-index-xelpg:
    - bat-pvc-2:          NOTRUN -> [SKIP][15] ([Intel XE#979])
   [15]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@xe_pat@pat-index-xelpg.html

  * igt@xe_pm_residency@gt-c6-on-idle:
    - bat-pvc-2:          NOTRUN -> [SKIP][16] ([Intel XE#531])
   [16]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/bat-pvc-2/igt@xe_pm_residency@gt-c6-on-idle.html

  
  [Intel XE#1021]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1021
  [Intel XE#1024]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1024
  [Intel XE#255]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/255
  [Intel XE#531]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/531
  [Intel XE#532]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/532
  [Intel XE#540]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/540
  [Intel XE#780]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/780
  [Intel XE#782]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/782
  [Intel XE#783]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/783
  [Intel XE#784]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/784
  [Intel XE#829]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/829
  [Intel XE#947]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/947
  [Intel XE#976]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/976
  [Intel XE#977]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/977
  [Intel XE#979]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/979
  [i915#6077]: https://gitlab.freedesktop.org/drm/intel/issues/6077


Build changes
-------------

  * IGT: IGT_7796 -> IGTPW_10967
  * Linux: xe-1025-f54ea7473cd118eb39978f2e946b17558b5ff46d -> xe-1029-f4b306bc42834d24085b45a5a3d1521a451b2abf

  IGTPW_10967: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10967/index.html
  IGT_7796: 2cfed18f6aa776c1593d7cc328d23225dd61bdf9 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  xe-1025-f54ea7473cd118eb39978f2e946b17558b5ff46d: f54ea7473cd118eb39978f2e946b17558b5ff46d
  xe-1029-f4b306bc42834d24085b45a5a3d1521a451b2abf: f4b306bc42834d24085b45a5a3d1521a451b2abf

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10967/index.html

[-- Attachment #2: Type: text/html, Size: 7294 bytes --]

Re: [PATCH i-g-t 1/8] lib/igt_drm_clients: Use calloc

[Umesh Nerlige Ramappa]

On Tue, Apr 02, 2024 at 03:17:09PM -0700, Lucas De Marchi wrote:
>Replace malloc + memset with a single calloc as it's shorter and there's
>no point keeping them separate.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

LGTM,
Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

>---
> lib/igt_drm_clients.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
>diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
>index 025d60c51..90ca6d61c 100644
>--- a/lib/igt_drm_clients.c
>+++ b/lib/igt_drm_clients.c
>@@ -35,12 +35,10 @@ struct igt_drm_clients *igt_drm_clients_init(void *private_data)
> {
> 	struct igt_drm_clients *clients;
>
>-	clients = malloc(sizeof(*clients));
>+	clients = calloc(1, sizeof(*clients));
> 	if (!clients)
> 		return NULL;
>
>-	memset(clients, 0, sizeof(*clients));
>-
> 	clients->private_data = private_data;
>
> 	return clients;
>@@ -165,9 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
> 	c->clients = clients;
>
> 	/* Engines */
>-	c->engines = malloc(sizeof(*c->engines));
>+	c->engines = calloc(1, sizeof(*c->engines));
> 	assert(c->engines);
>-	memset(c->engines, 0, sizeof(*c->engines));
> 	c->engines->capacity = calloc(info->last_engine_index + 1,
> 				      sizeof(*c->engines->capacity));
> 	assert(c->engines->capacity);
>@@ -190,9 +187,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
> 	assert(c->val && c->last);
>
> 	/* Memory regions */
>-	c->regions = malloc(sizeof(*c->regions));
>+	c->regions = calloc(1, 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);
>-- 
>2.43.0
>

Re: [PATCH i-g-t 2/8] lib/igt_drm_clients: Fix sizeof calculation

[Umesh Nerlige Ramappa]

On Tue, Apr 02, 2024 at 03:17:10PM -0700, Lucas De Marchi wrote:
>val and last are arrays of certain types. For the latter, it's
>underallocated on 32-bits since it should be sizeof(u64) not the size of
>a pointer. When running on 64-bits, no real bug.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

LGTM,
Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

>---
> lib/igt_drm_clients.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
>index 90ca6d61c..c8dc21d6a 100644
>--- a/lib/igt_drm_clients.c
>+++ b/lib/igt_drm_clients.c
>@@ -182,8 +182,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
> 		c->engines->num_engines++;
> 		c->engines->max_engine_id = i;
> 	}
>-	c->val = calloc(c->engines->max_engine_id + 1, sizeof(c->val));
>-	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
>+	c->val = calloc(c->engines->max_engine_id + 1, sizeof(*c->val));
>+	c->last = calloc(c->engines->max_engine_id + 1, sizeof(*c->last));
> 	assert(c->val && c->last);
>
> 	/* Memory regions */
>-- 
>2.43.0
>

Re: [PATCH i-g-t 3/8] lib/igt_drm_clients: Fix leaks

[Umesh Nerlige Ramappa]

On Tue, Apr 02, 2024 at 03:17:11PM -0700, Lucas De Marchi wrote:
>Stop leaking memory-related fields.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> lib/igt_drm_clients.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
>index c8dc21d6a..c174c96ab 100644
>--- a/lib/igt_drm_clients.c
>+++ b/lib/igt_drm_clients.c
>@@ -221,9 +221,19 @@ void igt_drm_client_free(struct igt_drm_client *c, bool clear)
> 		free(c->engines->names);
> 	}
> 	free(c->engines);
>+
> 	free(c->val);
> 	free(c->last);
>
>+	if (c->regions) {
>+		for (i = 0; i <= c->regions->max_region_id; i++)
>+			free(c->regions->names[i]);
>+		free(c->regions->names);
>+	}
>+	free(c->regions);
>+
>+	free(c->memory);
>+

Looks correct as I don't see this being freed elsewhere.

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

> 	if (clear)
> 		memset(c, 0, sizeof(*c));
> }
>-- 
>2.43.0
>

Re: [PATCH i-g-t 4/8] gputop: Free clients on exit

[Umesh Nerlige Ramappa]

On Tue, Apr 02, 2024 at 03:17:12PM -0700, Lucas De Marchi wrote:
>So it's easily checked with valgrind if we there's something leaking.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> tools/gputop.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/tools/gputop.c b/tools/gputop.c
>index 71e28f43e..b13044b50 100644
>--- a/tools/gputop.c
>+++ b/tools/gputop.c
>@@ -293,5 +293,7 @@ int main(int argc, char **argv)
> 		usleep(period_us);
> 	}
>
>+	igt_drm_clients_free(clients);
>+

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

> 	return 0;
> }
>-- 
>2.43.0
>

Re: [PATCH i-g-t 3/8] lib/igt_drm_clients: Fix leaks

[Tvrtko Ursulin]

On 03/04/2024 16:39, Umesh Nerlige Ramappa wrote:
> On Tue, Apr 02, 2024 at 03:17:11PM -0700, Lucas De Marchi wrote:
>> Stop leaking memory-related fields.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>> lib/igt_drm_clients.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
>> index c8dc21d6a..c174c96ab 100644
>> --- a/lib/igt_drm_clients.c
>> +++ b/lib/igt_drm_clients.c
>> @@ -221,9 +221,19 @@ void igt_drm_client_free(struct igt_drm_client 
>> *c, bool clear)
>>         free(c->engines->names);
>>     }
>>     free(c->engines);
>> +
>>     free(c->val);
>>     free(c->last);
>>
>> +    if (c->regions) {
>> +        for (i = 0; i <= c->regions->max_region_id; i++)
>> +            free(c->regions->names[i]);
>> +        free(c->regions->names);
>> +    }
>> +    free(c->regions);
>> +
>> +    free(c->memory);
>> +
> 
> Looks correct as I don't see this being freed elsewhere.

I concur - good catch!

Regards,

Tvrtko

> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> 
>>     if (clear)
>>         memset(c, 0, sizeof(*c));
>> }
>> -- 
>> 2.43.0
>>

Re: [PATCH i-g-t 4/8] gputop: Free clients on exit

[Tvrtko Ursulin]

On 02/04/2024 23:17, Lucas De Marchi wrote:
> So it's easily checked with valgrind if we there's something leaking.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   tools/gputop.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tools/gputop.c b/tools/gputop.c
> index 71e28f43e..b13044b50 100644
> --- a/tools/gputop.c
> +++ b/tools/gputop.c
> @@ -293,5 +293,7 @@ int main(int argc, char **argv)
>   		usleep(period_us);
>   	}
>   
> +	igt_drm_clients_free(clients);
> +
>   	return 0;
>   }

Indeed.. intel_gpu_top even has it already which makes me think for some 
reason I forgot to test with Valgrid when adding memory stats. Strange 
becasuse previously when working on these tools I found it 
indispensable. Anyway, thanks for fixing it!

Reviewed-by: Tvrtko Ursulin <tursulin@ursulin.net>

Regards,

Tvrtko

Re: [PATCH i-g-t 8/8] lib/igt_drm_clients: lazy stat process

[Tvrtko Ursulin]

On 02/04/2024 23:17, Lucas De Marchi wrote:
> Only try to get task data like pid and name when that is required.
> Typically most of the processes in a system don't have a drm fd open.
> So only open and use /proc/<pid>/stat when it's needed.

This sounds like a very good optimisation to me too and I also don't 
understand how it could be making things less efficient (as you say in 
the cover letter). Unfortunately I will not have time to cross check in 
the next week or so.

Regards,

Tvrtko

> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   lib/igt_drm_clients.c | 50 +++++++++++++++++++++----------------------
>   1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index c174c96ab..1e9781c68 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -367,26 +367,33 @@ static size_t readat2buf(int at, const char *name, char *buf, const size_t sz)
>   	}
>   }
>   
> -static bool get_task_name(const char *buffer, char *out, unsigned long sz)
> +static void get_task_data(int pid_dir, unsigned int *pid, char *task, size_t tasksz)
>   {
> -	char *s = index(buffer, '(');
> -	char *e = rindex(buffer, ')');
> -	unsigned int len;
> +	char buf[4096];
> +	char *s, *e;
> +	size_t len;
>   
> +	if (!readat2buf(pid_dir, "stat", buf, sizeof(buf)))
> +		return;
> +
> +	s = strchr(buf, '(');
> +	e = strchr(s, ')');
>   	if (!s || !e)
> -		return false;
> -	assert(e >= s);
> +		return;
>   
>   	len = e - ++s;
> -	if(!len || (len + 1) >= sz)
> -		return false;
> +	if (!len)
> +		return;
>   
> -	strncpy(out, s, len);
> -	out[len] = 0;
> +	if (len + 1 > tasksz)
> +		len = tasksz - 1;
>   
> -	return true;
> +	strncpy(task, s, len);
> +	task[len] = 0;
> +	*pid = atoi(buf);
>   }
>   
> +
>   static bool is_drm_fd(int fd_dir, const char *name, unsigned int *minor)
>   {
>   	struct stat stat;
> @@ -480,13 +487,11 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
>   		return clients;
>   
>   	while ((proc_dent = readdir(proc_dir)) != NULL) {
> -		unsigned int client_pid, minor = 0;
> +		unsigned int client_pid = 0, minor = 0;
>   		int pid_dir = -1, fd_dir = -1;
>   		struct dirent *fdinfo_dent;
>   		char client_name[64] = { };
>   		DIR *fdinfo_dir = NULL;
> -		char buf[4096];
> -		size_t count;
>   
>   		if (proc_dent->d_type != DT_DIR)
>   			continue;
> @@ -498,17 +503,6 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
>   		if (pid_dir < 0)
>   			continue;
>   
> -		count = readat2buf(pid_dir, "stat", buf, sizeof(buf));
> -		if (!count)
> -			goto next;
> -
> -		client_pid = atoi(buf);
> -		if (!client_pid)
> -			goto next;
> -
> -		if (!get_task_name(buf, client_name, sizeof(client_name)))
> -			goto next;
> -
>   		fd_dir = openat(pid_dir, "fd", O_DIRECTORY | O_RDONLY);
>   		if (fd_dir < 0)
>   			goto next;
> @@ -541,6 +535,12 @@ igt_drm_clients_scan(struct igt_drm_clients *clients,
>   						 minor, info.id))
>   				continue; /* Skip duplicate fds. */
>   
> +			if (!client_pid) {
> +				get_task_data(pid_dir, &client_pid, client_name,
> +					      sizeof(client_name));
> +				assert(client_pid > 0);
> +			}
> +
>   			c = igt_drm_clients_find(clients, IGT_DRM_CLIENT_PROBE,
>   						 minor, info.id);
>   			if (!c)

Re: [PATCH i-g-t 1/8] lib/igt_drm_clients: Use calloc

[Kamil Konieczny]

Hi Lucas,
On 2024-04-02 at 15:17:09 -0700, Lucas De Marchi wrote:
> Replace malloc + memset with a single calloc as it's shorter and there's
> no point keeping them separate.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

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

> ---
>  lib/igt_drm_clients.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index 025d60c51..90ca6d61c 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -35,12 +35,10 @@ struct igt_drm_clients *igt_drm_clients_init(void *private_data)
>  {
>  	struct igt_drm_clients *clients;
>  
> -	clients = malloc(sizeof(*clients));
> +	clients = calloc(1, sizeof(*clients));
>  	if (!clients)
>  		return NULL;
>  
> -	memset(clients, 0, sizeof(*clients));
> -
>  	clients->private_data = private_data;
>  
>  	return clients;
> @@ -165,9 +163,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>  	c->clients = clients;
>  
>  	/* Engines */
> -	c->engines = malloc(sizeof(*c->engines));
> +	c->engines = calloc(1, sizeof(*c->engines));
>  	assert(c->engines);
> -	memset(c->engines, 0, sizeof(*c->engines));
>  	c->engines->capacity = calloc(info->last_engine_index + 1,
>  				      sizeof(*c->engines->capacity));
>  	assert(c->engines->capacity);
> @@ -190,9 +187,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>  	assert(c->val && c->last);
>  
>  	/* Memory regions */
> -	c->regions = malloc(sizeof(*c->regions));
> +	c->regions = calloc(1, 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);
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 2/8] lib/igt_drm_clients: Fix sizeof calculation

[Kamil Konieczny]

Hi Lucas,
On 2024-04-02 at 15:17:10 -0700, Lucas De Marchi wrote:
> val and last are arrays of certain types. For the latter, it's
> underallocated on 32-bits since it should be sizeof(u64) not the size of
> a pointer. When running on 64-bits, no real bug.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  lib/igt_drm_clients.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index 90ca6d61c..c8dc21d6a 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -182,8 +182,8 @@ igt_drm_client_add(struct igt_drm_clients *clients,
>  		c->engines->num_engines++;
>  		c->engines->max_engine_id = i;
>  	}

While you are at this add newline here.

With or without it:
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> -	c->val = calloc(c->engines->max_engine_id + 1, sizeof(c->val));
> -	c->last = calloc(c->engines->max_engine_id + 1, sizeof(c->last));
> +	c->val = calloc(c->engines->max_engine_id + 1, sizeof(*c->val));
> +	c->last = calloc(c->engines->max_engine_id + 1, sizeof(*c->last));
>  	assert(c->val && c->last);
>  
>  	/* Memory regions */
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 3/8] lib/igt_drm_clients: Fix leaks

[Kamil Konieczny]

Hi Lucas,
On 2024-04-02 at 15:17:11 -0700, Lucas De Marchi wrote:
> Stop leaking memory-related fields.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

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

> ---
>  lib/igt_drm_clients.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/igt_drm_clients.c b/lib/igt_drm_clients.c
> index c8dc21d6a..c174c96ab 100644
> --- a/lib/igt_drm_clients.c
> +++ b/lib/igt_drm_clients.c
> @@ -221,9 +221,19 @@ void igt_drm_client_free(struct igt_drm_client *c, bool clear)
>  		free(c->engines->names);
>  	}
>  	free(c->engines);
> +
>  	free(c->val);
>  	free(c->last);
>  
> +	if (c->regions) {
> +		for (i = 0; i <= c->regions->max_region_id; i++)
> +			free(c->regions->names[i]);
> +		free(c->regions->names);
> +	}
> +	free(c->regions);
> +
> +	free(c->memory);
> +
>  	if (clear)
>  		memset(c, 0, sizeof(*c));
>  }
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 4/8] gputop: Free clients on exit

[Kamil Konieczny]

Hi Lucas,
On 2024-04-02 at 15:17:12 -0700, Lucas De Marchi wrote:
> So it's easily checked with valgrind if we there's something leaking.

s/we there's something/memory is/

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

> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  tools/gputop.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/gputop.c b/tools/gputop.c
> index 71e28f43e..b13044b50 100644
> --- a/tools/gputop.c
> +++ b/tools/gputop.c
> @@ -293,5 +293,7 @@ int main(int argc, char **argv)
>  		usleep(period_us);
>  	}
>  
> +	igt_drm_clients_free(clients);
> +
>  	return 0;
>  }
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 5/8] lib/igt_drm_fdinfo: Simplify find_kv()

[Kamil Konieczny]

Hi Lucas,
On 2024-04-02 at 15:17:13 -0700, Lucas De Marchi wrote:
> index() is deprecate from libc and strchr() should rather be used.
> Moreover the return value is NULL or a pointer to the occurrence, never
> the initial string and `(p - buf) != keylen` can only be true if ':' is
> the next char after the key.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  lib/igt_drm_fdinfo.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
> index 17cac4009..15f71e172 100644
> --- a/lib/igt_drm_fdinfo.c
> +++ b/lib/igt_drm_fdinfo.c
> @@ -106,20 +106,17 @@ static int parse_engine(char *line, struct drm_client_fdinfo *info,
>  
>  static const char *find_kv(const char *buf, const char *key, size_t keylen)
>  {
> -	const char *p = buf;
> +	const char *p;
>  
>  	if (strncmp(buf, key, keylen))
>  		return NULL;
>  
> -	p = index(buf, ':');
> -	if (!p || p == buf)
> -		return NULL;
> -	if ((p - buf) != keylen)
> +	p = buf + keylen;
> +	if (*p != ':')
>  		return NULL;
>  
> -	p++;
> -	while (*p && isspace(*p))
> -		p++;
> +	for (p++; *p && isspace(*p); p++)
> +		;
>  
>  	return *p ? p : NULL;
>  }
> @@ -233,8 +230,7 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>  		if ((v = find_kv(l, "drm-driver", strlen("drm-driver")))) {
>  			strncpy(info->driver, v, sizeof(info->driver) - 1);
>  			good++;
> -		}  else if ((v = find_kv(l, "drm-client-id",
> -					 strlen("drm-client-id")))) {
> +		}  else if ((v = find_kv(l, "drm-client-id", strlen("drm-client-id")))) {

This chunk  shouldn't be here, with it removed:

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

>  			info->id = atol(v);
>  			good++;
>  		} else if ((v = find_kv(l, "drm-pdev", strlen("drm-pdev")))) {
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 6/8] lib/igt_drm_fdinfo: Stop passing key twice

[Kamil Konieczny]

Hi Lucas,
On 2024-04-02 at 15:17:14 -0700, Lucas De Marchi wrote:
> Change strstartswith() so it also returns the length, which then can be
> used inside the branch when it matches. A good compiler shall optimize
> out the strlen call since the argument is a constant literal.
> 
> With this, the find_kv() is not needed anymore and the difference with
> regard the other branches can be summarized with a new ignore_space()
> helper and the fact it matches the entire key by appending the ':'. The
> helper is added on top of the file so it can be reused later.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  lib/igt_drm_fdinfo.c | 84 +++++++++++++++++++++++++-------------------
>  1 file changed, 48 insertions(+), 36 deletions(-)
> 
> diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
> index 15f71e172..a0b41e956 100644
> --- a/lib/igt_drm_fdinfo.c
> +++ b/lib/igt_drm_fdinfo.c
> @@ -53,6 +53,14 @@ static size_t read_fdinfo(char *buf, const size_t sz, int at, const char *name)
>  	return count > 0 ? count : 0;
>  }
>  
> +static const char *ignore_space(const char *s)
> +{
> +	for (; *s && isspace(*s); s++)
> +		;
> +
> +	return s;
> +}
> +
>  static int parse_engine(char *line, struct drm_client_fdinfo *info,
>  			size_t prefix_len,
>  			const char **name_map, unsigned int map_entries,
> @@ -104,21 +112,12 @@ static int parse_engine(char *line, struct drm_client_fdinfo *info,
>  	return found;
>  }
>  
> -static const char *find_kv(const char *buf, const char *key, size_t keylen)
> +static const char *ignore_space(const char *s)
--------------------- ^

This should be removed from here, not added.

Regards,
Kamil

>  {
> -	const char *p;
> -
> -	if (strncmp(buf, key, keylen))
> -		return NULL;
> -
> -	p = buf + keylen;
> -	if (*p != ':')
> -		return NULL;
> -
> -	for (p++; *p && isspace(*p); p++)
> +	for (; *s && isspace(*s); s++)
>  		;
>  
> -	return *p ? p : NULL;
> +	return s;
>  }
>  
>  static int parse_region(char *line, struct drm_client_fdinfo *info,
> @@ -205,6 +204,11 @@ out:
>  		}							\
>  	} while (0)
>  
> +#define strstartswith(a, b, plen__) ({					\
> +		*plen__ = strlen(b);					\
> +		strncmp(a, b, *plen__) == 0;				\
> +})
> +
>  unsigned int
>  __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>  		       const char **name_map, unsigned int map_entries,
> @@ -222,30 +226,39 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>  
>  	while ((l = strtok_r(_buf, "\n", &ctx))) {
>  		uint64_t val = 0;
> +		size_t keylen;
>  		const char *v;
>  		int idx;
>  
>  		_buf = NULL;
>  
> -		if ((v = find_kv(l, "drm-driver", strlen("drm-driver")))) {
> -			strncpy(info->driver, v, sizeof(info->driver) - 1);
> -			good++;
> -		}  else if ((v = find_kv(l, "drm-client-id", strlen("drm-client-id")))) {
> -			info->id = atol(v);
> -			good++;
> -		} else if ((v = find_kv(l, "drm-pdev", strlen("drm-pdev")))) {
> -			/* optional */
> +		if (strstartswith(l, "drm-driver:", &keylen)) {
> +			v = l + keylen;
> +			v = ignore_space(v);
> +			if (*v) {
> +				strncpy(info->driver, v, sizeof(info->driver) - 1);
> +				good++;
> +			}
> +		}  else if (strstartswith(l, "drm-client-id:", &keylen)) {
> +			v = l + keylen;
> +			v = ignore_space(v);
> +			if (*v) {
> +				info->id = atol(v);
> +				good++;
> +			}
> +		} else if (strstartswith(l, "drm-pdev:", &keylen)) {
> +			v = l + keylen;
> +			v = ignore_space(v);
>  			strncpy(info->pdev, v, sizeof(info->pdev) - 1);
> -		} else if (!strncmp(l, "drm-engine-capacity-", 20)) {
> -			idx = parse_engine(l, info,
> -					   strlen("drm-engine-capacity-"),
> +		} else if (strstartswith(l, "drm-engine-capacity-", &keylen)) {
> +			idx = parse_engine(l, info, keylen,
>  					   name_map, map_entries, &val);
>  			if (idx >= 0) {
>  				info->capacity[idx] = val;
>  				num_capacity++;
>  			}
> -		} else if (!strncmp(l, "drm-engine-", 11)) {
> -			idx = parse_engine(l, info, strlen("drm-engine-"),
> +		} else if (strstartswith(l, "drm-engine-", &keylen)) {
> +			idx = parse_engine(l, info, keylen,
>  					   name_map, map_entries, &val);
>  			if (idx >= 0) {
>  				if (!info->capacity[idx])
> @@ -255,25 +268,24 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
>  				if (idx > info->last_engine_index)
>  					info->last_engine_index = idx;
>  			}
> -		} else if (!strncmp(l, "drm-total-", 10)) {
> -			idx = parse_region(l, info, strlen("drm-total-"),
> +		} else if (strstartswith(l, "drm-total-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>  					   region_map, region_entries, &val);
>  			UPDATE_REGION(idx, total, val);
> -		} else if (!strncmp(l, "drm-shared-", 11)) {
> -			idx = parse_region(l, info, strlen("drm-shared-"),
> +		} else if (strstartswith(l, "drm-shared-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>  					   region_map, region_entries, &val);
>  			UPDATE_REGION(idx, shared, val);
> -
> -		} else if (!strncmp(l, "drm-resident-", 13)) {
> -			idx = parse_region(l, info, strlen("drm-resident-"),
> +		} else if (strstartswith(l, "drm-resident-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>  					   region_map, region_entries, &val);
>  			UPDATE_REGION(idx, resident, val);
> -		} else if (!strncmp(l, "drm-purgeable-", 14)) {
> -			idx = parse_region(l, info, strlen("drm-purgeable-"),
> +		} else if (strstartswith(l, "drm-purgeable-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>  					   region_map, region_entries, &val);
>  			UPDATE_REGION(idx, purgeable, val);
> -		} else if (!strncmp(l, "drm-active-", 11)) {
> -			idx = parse_region(l, info, strlen("drm-active-"),
> +		} else if (strstartswith(l, "drm-active-", &keylen)) {
> +			idx = parse_region(l, info, keylen,
>  					   region_map, region_entries, &val);
>  			UPDATE_REGION(idx, active, val);
>  		}
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 7/8] lib/igt_drm_fdinfo: Remove prefix arg from parse functions

[Umesh Nerlige Ramappa]

On Tue, Apr 02, 2024 at 03:17:15PM -0700, Lucas De Marchi wrote:
>The prefix is not really needed and __igt_parse_drm_fdinfo()
>can simply pass l + keylen as argument which simplifies the parse
>functions.
>
>While refactoring this, it also replaces the remaining calls to index()
>to use strchr().
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> lib/igt_drm_fdinfo.c | 69 ++++++++++++++++++--------------------------
> 1 file changed, 28 insertions(+), 41 deletions(-)
>
>diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c
>index a0b41e956..7a9ae94f0 100644
>--- a/lib/igt_drm_fdinfo.c
>+++ b/lib/igt_drm_fdinfo.c
>@@ -61,25 +61,23 @@ static const char *ignore_space(const char *s)
> 	return s;
> }
>
>-static int parse_engine(char *line, struct drm_client_fdinfo *info,
>-			size_t prefix_len,
>+static int parse_engine(const char *name, struct drm_client_fdinfo *info,
> 			const char **name_map, unsigned int map_entries,
> 			uint64_t *val)
> {
>-	ssize_t name_len;
>-	char *name, *p;
>+	const char *p;
>+	size_t name_len;
> 	int found = -1;
> 	unsigned int i;
>
>-	p = index(line, ':');
>-	if (!p || p == line)
>+	p = strchr(name, ':');
>+	if (!p)
> 		return -1;
>
>-	name_len = p - line - prefix_len;
>+	name_len = p - name;
> 	if (name_len < 1)
> 		return -1;
>-
>-	name = line + prefix_len;
>+	p++;
>
> 	if (name_map) {
> 		for (i = 0; i < map_entries; i++) {
>@@ -105,40 +103,30 @@ static int parse_engine(char *line, struct drm_client_fdinfo *info,
> 	}
>
> 	if (found >= 0) {
>-		while (*++p && isspace(*p));
>+		p = ignore_space(p);
> 		*val = strtoull(p, NULL, 10);
> 	}
>
> 	return found;
> }
>
>-static const char *ignore_space(const char *s)
>-{
>-	for (; *s && isspace(*s); s++)
>-		;
>-
>-	return s;
>-}

Removal of this ignore_space should be in patch 6 (like Kamil 
mentioned).

The rest of the logic is same as the previous code, so with the above 
change this is

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

>-
>-static int parse_region(char *line, struct drm_client_fdinfo *info,
>-			size_t prefix_len,
>+static int parse_region(const char *name, struct drm_client_fdinfo *info,
> 			const char **region_map, unsigned int region_entries,
> 			uint64_t *val)
> {
>-	char *name, *p, *unit = NULL;
>-	ssize_t name_len;
>+	const char *p;
>+	size_t name_len;
> 	int found = -1;
> 	unsigned int i;
>
>-	p = index(line, ':');
>-	if (!p || p == line)
>+	p = strchr(name, ':');
>+	if (!p)
> 		return -1;
>
>-	name_len = p - line - prefix_len;
>+	name_len = p - name;
> 	if (name_len < 1)
> 		return -1;
>-
>-	name = line + prefix_len;
>+	p++;
>
> 	if (region_map) {
> 		for (i = 0; i < region_entries; i++) {
>@@ -170,20 +158,19 @@ static int parse_region(char *line, struct drm_client_fdinfo *info,
> 	if (found < 0)
> 		goto out;
>
>-	while (*++p && isspace(*p))
>-		;
>+	p = ignore_space(p);
> 	*val = strtoull(p, NULL, 10);
>
>-	p = index(p, ' ');
>+	p = strchr(p, ' ');
> 	if (!p)
> 		goto out;
>+	p++;
>
>-	unit = ++p;
>-	if (!strcmp(unit, "KiB")) {
>+	if (!strcmp(p, "KiB")) {
> 		*val *= 1024;
>-	} else if (!strcmp(unit, "MiB")) {
>+	} else if (!strcmp(p, "MiB")) {
> 		*val *= 1024 * 1024;
>-	} else if (!strcmp(unit, "GiB")) {
>+	} else if (!strcmp(p, "GiB")) {
> 		*val *= 1024 * 1024 * 1024;
> 	}
>
>@@ -251,14 +238,14 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
> 			v = ignore_space(v);
> 			strncpy(info->pdev, v, sizeof(info->pdev) - 1);
> 		} else if (strstartswith(l, "drm-engine-capacity-", &keylen)) {
>-			idx = parse_engine(l, info, keylen,
>+			idx = parse_engine(l + keylen, info,
> 					   name_map, map_entries, &val);
> 			if (idx >= 0) {
> 				info->capacity[idx] = val;
> 				num_capacity++;
> 			}
> 		} else if (strstartswith(l, "drm-engine-", &keylen)) {
>-			idx = parse_engine(l, info, keylen,
>+			idx = parse_engine(l + keylen, info,
> 					   name_map, map_entries, &val);
> 			if (idx >= 0) {
> 				if (!info->capacity[idx])
>@@ -269,23 +256,23 @@ __igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info,
> 					info->last_engine_index = idx;
> 			}
> 		} else if (strstartswith(l, "drm-total-", &keylen)) {
>-			idx = parse_region(l, info, keylen,
>+			idx = parse_region(l + keylen, info,
> 					   region_map, region_entries, &val);
> 			UPDATE_REGION(idx, total, val);
> 		} else if (strstartswith(l, "drm-shared-", &keylen)) {
>-			idx = parse_region(l, info, keylen,
>+			idx = parse_region(l + keylen, info,
> 					   region_map, region_entries, &val);
> 			UPDATE_REGION(idx, shared, val);
> 		} else if (strstartswith(l, "drm-resident-", &keylen)) {
>-			idx = parse_region(l, info, keylen,
>+			idx = parse_region(l + keylen, info,
> 					   region_map, region_entries, &val);
> 			UPDATE_REGION(idx, resident, val);
> 		} else if (strstartswith(l, "drm-purgeable-", &keylen)) {
>-			idx = parse_region(l, info, keylen,
>+			idx = parse_region(l + keylen, info,
> 					   region_map, region_entries, &val);
> 			UPDATE_REGION(idx, purgeable, val);
> 		} else if (strstartswith(l, "drm-active-", &keylen)) {
>-			idx = parse_region(l, info, keylen,
>+			idx = parse_region(l + keylen, info,
> 					   region_map, region_entries, &val);
> 			UPDATE_REGION(idx, active, val);
> 		}
>-- 
>2.43.0
>