[Intel-gfx] [PATCH igt] igt/perf_pmu: Combine rc6/rc6p tests

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-24 08:50:49)
> 
> On 23/11/2017 15:48, Chris Wilson wrote:
> > When in rc6p, rc6 doesn't increment. So on rc6p machines, we can't test
> > rc6 in isolation.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   tests/perf_pmu.c | 50 +++++---------------------------------------------
> >   1 file changed, 5 insertions(+), 45 deletions(-)
> > 
> > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> > index b8800e61..1e441e0e 100644
> > --- a/tests/perf_pmu.c
> > +++ b/tests/perf_pmu.c
> > @@ -998,45 +998,10 @@ test_frequency(int gem_fd)
> >   
> >   static void
> >   test_rc6(int gem_fd)
> > -{
> > -     int64_t duration_ns = 2e9;
> > -     uint64_t idle, busy, prev;
> > -     unsigned int slept;
> > -     int fd, fw;
> > -
> > -     fd = open_pmu(I915_PMU_RC6_RESIDENCY);
> > -
> > -     gem_quiescent_gpu(gem_fd);
> > -     usleep(100e3); /* wait for the rc6 cycle counter to kick in */
> > -
> > -     /* Go idle and check full RC6. */
> > -     prev = pmu_read_single(fd);
> > -     slept = measured_usleep(duration_ns / 1000);
> > -     idle = pmu_read_single(fd);
> > -
> > -     assert_within_epsilon(idle - prev, slept, tolerance);
> > -
> > -     /* Wake up device and check no RC6. */
> > -     fw = igt_open_forcewake_handle(gem_fd);
> > -     igt_assert(fw >= 0);
> > -     usleep(1e3); /* wait for the rc6 cycle counter to stop ticking */
> > -
> > -     prev = pmu_read_single(fd);
> > -     usleep(duration_ns / 1000);
> > -     busy = pmu_read_single(fd);
> > -
> > -     close(fw);
> > -     close(fd);
> > -
> > -     assert_within_epsilon(busy - prev, 0.0, tolerance);
> > -}
> > -
> > -static void
> > -test_rc6p(int gem_fd)
> >   {
> >       int64_t duration_ns = 2e9;
> >       unsigned int num_pmu = 1;
> > -     uint64_t idle[3], busy[3], prev[3];
> > +     uint64_t idle[3], busy[3], prev[3], total;
> >       unsigned int slept, i;
> >       int fd, ret, fw;
> >   
> > @@ -1048,8 +1013,7 @@ test_rc6p(int gem_fd)
> >               if (ret > 0)
> >                       num_pmu++;
> >       }
> > -
> > -     igt_require(num_pmu == 3);
> > +     igt_require(num_pmu >= 1);
> >   
> >       gem_quiescent_gpu(gem_fd);
> >       usleep(100e3); /* wait for the rc6 cycle counter to kick in */
> > @@ -1059,8 +1023,10 @@ test_rc6p(int gem_fd)
> >       slept = measured_usleep(duration_ns / 1000);
> >       pmu_read_multi(fd, num_pmu, idle);
> >   
> > +     total = 0;
> >       for (i = 0; i < num_pmu; i++)
> > -             assert_within_epsilon(idle[i] - prev[i], slept, tolerance);
> > +             total += idle[i] - prev[i];
> > +     assert_within_epsilon(total, slept, tolerance);
> >   
> >       /* Wake up device and check no RC6. */
> >       fw = igt_open_forcewake_handle(gem_fd);
> > @@ -1217,12 +1183,6 @@ igt_main
> >       igt_subtest("rc6")
> >               test_rc6(fd);
> >   
> > -     /**
> > -      * Test RC6p residency reporting.
> > -      */
> > -     igt_subtest("rc6p")
> > -             test_rc6p(fd);
> > -
> >       /**
> >        * Check render nodes are counted.
> >        */
> > 
> 
> You don't think it is worth keeping the single counter rc6 test and only 
> modify the rc6p? From a point of view of separately testing different 
> code paths within the implementation.

How? How do you tell if rc6 is not incrementing because (a) the hw is
busy, (b) the hw is idle but in a different rc6 state, or (c) the pmu is
broken.

What I was thinking was throwing away the rc6p/rc6pp and just have a
single counter for rc?

Also, I need to check if we convert the residency to a full 64b value to
prevent the issue with rollover in 20-50minutes.
-Chris