[Intel-gfx] [PATCH i-g-t 4/7] intel-gpu-overlay: Catch-up to new i915 PMU

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-09-26 11:56:24)
> 
> On 25/09/2017 16:31, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-09-25 16:14:59)
> >> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> ---
> >> diff --git a/overlay/gpu-top.c b/overlay/gpu-top.c
> >> index 812f47d5aced..61b8f62fd78c 100644
> >> --- a/overlay/gpu-top.c
> >> +++ b/overlay/gpu-top.c
> >> @@ -43,49 +43,57 @@
> >>   #define   RING_WAIT            (1<<11)
> >>   #define   RING_WAIT_SEMAPHORE  (1<<10)
> >>   
> >> -#define __I915_PERF_RING(n) (4*n)
> >> -#define I915_PERF_RING_BUSY(n) (__I915_PERF_RING(n) + 0)
> >> -#define I915_PERF_RING_WAIT(n) (__I915_PERF_RING(n) + 1)
> >> -#define I915_PERF_RING_SEMA(n) (__I915_PERF_RING(n) + 2)
> >> -
> >>   static int perf_init(struct gpu_top *gt)
> >>   {
> >> -       const char *names[] = {
> >> -               "RCS",
> >> -               "BCS",
> >> -               "VCS0",
> >> -               "VCS1",
> >> -               NULL,
> >> +       struct engine_desc {
> >> +               unsigned class, inst;
> >> +               const char *name;
> >> +       } *d, engines[] = {
> >> +               { I915_ENGINE_CLASS_RENDER, 0, "rcs0" },
> >> +               { I915_ENGINE_CLASS_COPY, 0, "bcs0" },
> >> +               { I915_ENGINE_CLASS_VIDEO, 0, "vcs0" },
> >> +               { I915_ENGINE_CLASS_VIDEO, 1, "vcs1" },
> >> +               { I915_ENGINE_CLASS_VIDEO_ENHANCE, 0, "vecs0" },
> > 
> > Hmm, there is some hidden coupling with colours atm, but other than that
> > the order is flexible, iirc.
> 
> What do you mean? First you thought there's some issue but there isn't 
> after all?

Just the way the colours get assigned later makes some assumptions that
no longer hold true. I was thinking it would make more sense to move the
colour field into the ring struct; even more sense to make it all
customisable. (In my defence, it was never meant to be perfect! Just a
snowball.)

> Oh right, VECS wasn't on the list before.. it seems to work anyway. Just 
> haven't tried with five engines.

It should just work, just don't show the result to a UI designer.

> >> diff --git a/overlay/power.c b/overlay/power.c
> >> index dd4aec6bffd9..805f4ca7805c 100644
> >> --- a/overlay/power.c
> >> +++ b/overlay/power.c
> >> @@ -45,9 +45,7 @@ int power_init(struct power *power)
> >>   
> >>          memset(power, 0, sizeof(*power));
> >>   
> >> -       power->fd = perf_i915_open(I915_PERF_ENERGY);
> >> -       if (power->fd != -1)
> >> -               return 0;
> >> +       power->fd = -1;
> > 
> > Hmm, didn't you say that the rapl values were exposed via perf as well?
> 
> Yes, I planned to add this back afterwards but can have it as part of 
> this series as well.

Ok. I was curious to see what it looked like; wondering if it would just
plug right in and if our usage of perf_event looked reasonably similar.
-Chris