[Intel-gfx] [igt-dev] [PATCH i-g-t] i915/perf: Find the associated perf-type for a particular device

[Chris Wilson]

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 at chris-wilson.co.uk>
> > Cc: "Robert M. Fosha" <robert.m.fosha at intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > ---
> >   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/51

How 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