On 30/03/2022 20:52, Umesh Nerlige Ramappa wrote:
On Tue, Feb 22, 2022 at 01:55:55PM +0000, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin tvrtko.ursulin@intel.com
Tests and intel_gpu_top will share common code for parsing this file.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
lib/igt_drm_fdinfo.c | 183 +++++++++++++++++++++++++++++++++++++++++++ lib/igt_drm_fdinfo.h | 48 ++++++++++++ lib/meson.build | 7 ++ 3 files changed, 238 insertions(+) create mode 100644 lib/igt_drm_fdinfo.c create mode 100644 lib/igt_drm_fdinfo.h
diff --git a/lib/igt_drm_fdinfo.c b/lib/igt_drm_fdinfo.c new file mode 100644 index 000000000000..28c1bdbda08e --- /dev/null +++ b/lib/igt_drm_fdinfo.c @@ -0,0 +1,183 @@ +/*
- Copyright © 2022 Intel Corporation
- Permission is hereby granted, free of charge, to any person
obtaining a
- copy of this software and associated documentation files (the
"Software"),
- to deal in the Software without restriction, including without
limitation
- the rights to use, copy, modify, merge, publish, distribute,
sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice (including
the next
- paragraph) shall be included in all copies or substantial portions
of the
- Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
SHALL
- THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
ARISING
- FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
OTHER DEALINGS
- IN THE SOFTWARE.
- */
+#include <ctype.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <stdio.h> +#include <string.h> +#include <stdlib.h> +#include <unistd.h>
+#include "drmtest.h"
+#include "igt_drm_fdinfo.h"
+static size_t read_fdinfo(char *buf, const size_t sz, int at, const char *name) +{ + size_t count; + int fd;
+ fd = openat(at, name, O_RDONLY); + if (fd < 0) + return 0;
+ buf[sz - 1] = 0;
Wondering if this ^ should be after the read() in case 4096 bytes are read.
Yes, I guess even before and after so it's guaranteed to be null terminated no matter what.
+ count = read(fd, buf, sz); + close(fd);
+ return count; +}
+static int parse_engine(char *line, struct drm_client_fdinfo *info, + size_t prefix_len, uint64_t *val) +{ + static const char *e2class[] = { + "render", + "copy", + "video", + "video-enhance", + }; + ssize_t name_len; + char *name, *p; + 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;
+ for (i = 0; i < ARRAY_SIZE(e2class); i++) { + if (!strncmp(name, e2class[i], name_len)) { + found = i; + break; + } + }
+ if (found >= 0) { + while (*++p && isspace(*p)); + *val = strtoull(p, NULL, 10); + }
+ return found; +}
+static const char *find_kv(const char *buf, const char *key, size_t keylen) +{ + const char *p = buf;
+ p = index(buf, ':'); + if (!p || p == buf) + return NULL;
+ if ((p - buf) != keylen) + return NULL;
+ while (*++p && isspace(*p)); + if (*p && !strncmp(buf, key, keylen))
nit: why not just do the strncmp early in this function since buf, key, keylen have not changed?
I see what you mean, first check that the key/prefix matches and do the other checks, right?
I honestly don't remember if there was a special reason why I laid it out like it is. I know it is an annoyingly hot path and I did do many tweaks to bring the CPU utilisation low.
But thinking about it now, the strncmp at the start should indeed be more efficient so I will change it.
+ return p;
+ return NULL; +}
+bool +__igt_parse_drm_fdinfo(int dir, const char *fd, struct drm_client_fdinfo *info) +{ + char buf[4096], *_buf = buf; + char *l, *ctx = NULL; + unsigned int good = 0; + size_t count;
Should buf be zeroed out here?
Right, either here or in read_fdinfo as mentioned above.
+ count = read_fdinfo(buf, sizeof(buf), dir, fd); + if (!count) + return false;
+ while ((l = strtok_r(_buf, "\n", &ctx))) { + uint64_t val = 0; + 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-pdev", strlen("drm-pdev")))) { + strncpy(info->pdev, v, sizeof(info->pdev) - 1); + } else if ((v = find_kv(l, "drm-client-id", + strlen("drm-client-id")))) { + info->id = atol(v); + good++; + } else if (!strncmp(l, "drm-engine-", 11) && + strncmp(l, "drm-engine-capacity-", 20)) { + idx = parse_engine(l, info, strlen("drm-engine-"), + &val); + if (idx >= 0) { + if (!info->capacity[idx]) + info->capacity[idx] = 1; + info->busy[idx] = val; + info->num_engines++; + } + } else if (!strncmp(l, "drm-engine-capacity-", 20)) { + idx = parse_engine(l, info, + strlen("drm-engine-capacity-"), + &val); + if (idx >= 0) + info->capacity[idx] = val; + } + }
+ if (good < 2 || !info->num_engines) + return false; /* fdinfo format not as expected */
+ return true; +}
+bool igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info) +{ + char fd[64]; + int dir, ret; + bool res;
+ ret = snprintf(fd, sizeof(fd), "%u", drm_fd); + if (ret < 0 || ret == sizeof(fd)) + return false;
+ dir = open("/proc/self/fdinfo", O_DIRECTORY | O_RDONLY); + if (dir < 0) + return false;
+ res = __igt_parse_drm_fdinfo(dir, fd, info);
+ close(dir);
+ return res; +} diff --git a/lib/igt_drm_fdinfo.h b/lib/igt_drm_fdinfo.h new file mode 100644 index 000000000000..c527bab9a204 --- /dev/null +++ b/lib/igt_drm_fdinfo.h @@ -0,0 +1,48 @@ +/*
- Copyright © 2022 Intel Corporation
- Permission is hereby granted, free of charge, to any person
obtaining a
- copy of this software and associated documentation files (the
"Software"),
- to deal in the Software without restriction, including without
limitation
- the rights to use, copy, modify, merge, publish, distribute,
sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice (including
the next
- paragraph) shall be included in all copies or substantial portions
of the
- Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
SHALL
- THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
ARISING
- FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
OTHER DEALINGS
- IN THE SOFTWARE.
- */
+#ifndef IGT_DRM_FDINFO_H +#define IGT_DRM_FDINFO_H
+#include <sys/types.h> +#include <dirent.h> +#include <stdint.h> +#include <stdbool.h>
+struct drm_client_fdinfo { + char driver[128]; + char pdev[128]; + unsigned long id;
+ unsigned int num_engines; + unsigned int capacity[16]; + uint64_t busy[16];
I see 16 in a lot of places, wondering if it could use a #define?
I can see them here and one mention in igt_drm_clients. The latter it looks I can un-export igt_drm_client_update and make it take struct drm_client_fdinfo to better hide this.
And for here I will add a define like DRM_CLIENT_FDINFO_MAX_ENGINES.
Regards,
Tvrtko
Thanks, Umesh
+};
+bool igt_parse_drm_fdinfo(int drm_fd, struct drm_client_fdinfo *info);
+bool __igt_parse_drm_fdinfo(int dir, const char *fd, + struct drm_client_fdinfo *info);
+#endif /* IGT_DRM_FDINFO_H */ diff --git a/lib/meson.build b/lib/meson.build index 3e43316d1e36..d00817dc58bc 100644 --- a/lib/meson.build +++ b/lib/meson.build @@ -16,6 +16,7 @@ lib_sources = [ 'igt_debugfs.c', 'igt_device.c', 'igt_device_scan.c', + 'igt_drm_fdinfo.c', 'igt_aux.c', 'igt_gt.c', 'igt_halffloat.c', @@ -216,6 +217,12 @@ lib_igt_device_scan_build = static_library('igt_device_scan', lib_igt_device_scan = declare_dependency(link_with : lib_igt_device_scan_build, include_directories : inc)
+lib_igt_drm_fdinfo_build = static_library('igt_drm_fdinfo', + ['igt_drm_fdinfo.c'], + include_directories : inc)
+lib_igt_drm_fdinfo = declare_dependency(link_with : lib_igt_drm_fdinfo_build, + include_directories : inc) i915_perf_files = [ 'igt_list.c', 'i915/perf.c', -- 2.32.0