On 1/7/20 2:32 AM, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2020-01-07 09:53:39)+Arek, Saurabhg On 05/01/2020 01:06, Chris Wilson wrote:Since with multiple devices, we may have multiple different perf_pmu each with their own type, we want to find the right one for the job. The tests are run with a specific fd, from which we can extract the appropriate bus-id and find the associated perf-type. The performance monitoring tools are a little more general and not yet ready to probe all device or bind to one in particular, so we just assume the default igfx for the time being. v2: Extract the bus address from out of sysfs Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> Cc: "Robert M. Fosha" <robert.m.fosha@xxxxxxxxx> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Tested-by: Robert M. Fosha <robert.m.fosha@xxxxxxxxx>
--- benchmarks/gem_wsim.c | 4 +- lib/igt_perf.c | 84 +++++++++++++++++++++++++++++++--- lib/igt_perf.h | 13 ++++-- overlay/gem-interrupts.c | 2 +- overlay/gpu-freq.c | 4 +- overlay/gpu-top.c | 12 ++--- overlay/rc6.c | 2 +- tests/i915/gem_ctx_freq.c | 2 +- tests/i915/gem_ctx_sseu.c | 2 +- tests/i915/gem_exec_balancer.c | 18 +++++--- tests/perf_pmu.c | 84 ++++++++++++++++++---------------- tools/intel_gpu_top.c | 2 +- 12 files changed, 159 insertions(+), 70 deletions(-) diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c index 6305e0d7a..9156fdc90 100644 --- a/benchmarks/gem_wsim.c +++ b/benchmarks/gem_wsim.c @@ -2268,8 +2268,8 @@ busy_init(const struct workload_balancer *balancer, struct workload *wrk) for (d = &engines[0]; d->id != VCS; d++) { int pfd;- pfd = perf_i915_open_group(I915_PMU_ENGINE_BUSY(d->class,- d->inst), + pfd = perf_igfx_open_group(I915_PMU_ENGINE_BUSY(d->class, + d->inst), bb->fd); if (pfd < 0) { if (d->id != VCS2) diff --git a/lib/igt_perf.c b/lib/igt_perf.c index e3dec2cc2..840add043 100644 --- a/lib/igt_perf.c +++ b/lib/igt_perf.c @@ -4,17 +4,77 @@ #include <stdlib.h> #include <string.h> #include <errno.h> +#include <sys/stat.h> #include <sys/sysinfo.h> +#include <sys/sysmacros.h>#include "igt_perf.h" -uint64_t i915_type_id(void)+static char *bus_address(int i915, char *path, int pathlen) +{ + struct stat st; + int len = -1; + int dir; + char *s; + + if (fstat(i915, &st) || !S_ISCHR(st.st_mode)) + return NULL; + + snprintf(path, pathlen, "/sys/dev/char/%d:%d", + major(st.st_rdev), minor(st.st_rdev)); + + dir = open(path, O_RDONLY); + if (dir != -1) { + len = readlinkat(dir, "device", path, pathlen - 1); + close(dir); + } + if (len < 0) + return NULL; + + path[len] = '\0'; + + /* strip off the relative path */ + s = strrchr(path, '/'); + if (s) + memmove(path, s + 1, len - (s - path) + 1); + + return path; +} + +const char *i915_perf_device(int i915, char *buf, int buflen) +{ +#define prefix "i915-" +#define plen strlen(prefix) + + if (!buf || buflen < plen) + return "i915"; + + memcpy(buf, prefix, plen); + + if (!bus_address(i915, buf + plen, buflen - plen) || + strcmp(buf + plen, "0000:00:02.0") == 0) /* legacy name for igfx */ + buf[plen - 1] = '\0'; + + return buf; +}So DRM fd -> PCI string conversion, yes? On a glance it looks okay. However Arek probably has this data as part of "[PATCH i-g-t 0/4] device selection && lsgpu" (https://patchwork.freedesktop.org/series/70285/).If the string is known, we can use it. This simple routine is *simple* yet effective :)Also: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/52 https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/51How lightweight are they aiming to be?And VLK-5588. This patch is overlap with #52 and then #51/VLK-5588 is about allowing card selection for tools. How to meld the two with minimum effort? We could put this in and then later replace the PCI name resolve with a library routine and re-adjust tools to allow card selection via some mechanism.Exactly. All we need here is a name to lookup the perf type id. One routine to provide an introspection method for a given fd and assumption of i915, does not prevent better methods :) I do wonder though if we should have perf_name in our sysfs. -Chris
Agree with idea of adding this change now and re-adjusting if other mechanism is added for other tests/tools. If no other concerns from Tvrtko or Arek
Reviewed-by: Robert M. Fosha <robert.m.fosha@xxxxxxxxx> -Rob
_______________________________________________ igt-dev mailing list igt-dev@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
