[igt-dev] [PATCH i-g-t 06/13] i915/gem_exec_schedule: Measure semaphore power consumption

[Katarzyna Dec]

On Tue, Feb 05, 2019 at 12:53:14PM +0000, Chris Wilson wrote:
> Quoting Katarzyna Dec (2019-02-05 12:50:38)
> > On Mon, Feb 04, 2019 at 08:36:07AM +0000, Chris Wilson wrote:
> > > How much energy does spinning on a semaphore consume relative to plain
> > > old spinning?
> > > 
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Is this patch relying on something more then:
> > bad9d8d0 lib: Add GPU power measurement ? Because I got errors on compilation:
> > 
> > '../tests/i915/gem_exec_schedule.c:1417:16: error: implicit declaration of
> > function ‘gem_scheduler_has_semaphores’; did you mean
> > ‘gem_scheduler_has_preemption’? [-Werror=implicit-function-declaration]'
> > I've seen some patches on intel-gfx, but not in this series.
> 
> That'll be patch 4/13.
I new I've seen the code somewhere :)

> 
> > > ---
> > >  tests/i915/gem_exec_schedule.c | 72 +++++++++++++++++++++++++++++++++-
> > >  1 file changed, 71 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
> > > index 0462ce84f..184ceb7d6 100644
> > > --- a/tests/i915/gem_exec_schedule.c
> > > +++ b/tests/i915/gem_exec_schedule.c
> > > @@ -29,9 +29,10 @@
> > >  #include <signal.h>
> > >  
> > >  #include "igt.h"
> > > -#include "igt_vgem.h"
> > > +#include "igt_gpu_power.h"
> > >  #include "igt_rand.h"
> > >  #include "igt_sysfs.h"
> > > +#include "igt_vgem.h"
> > >  #include "i915/gem_ring.h"
> > >  
> > >  #define LO 0
> > > @@ -1191,6 +1192,65 @@ static void test_pi_ringfull(int fd, unsigned int engine)
> > >       munmap(result, 4096);
> > >  }
> > >  
> > > +static void measure_semaphore_power(int i915)
> > > +{
> > > +     struct gpu_power power;
> > Where power struct is initialized?
> > > +     unsigned int engine, signaler;
> > How does signaler differ from engine (in usage)?
> 
> signaler tells engines to go.

> 
> > > +     igt_require(gpu_power_open(&power) == 0);
> 
> This initialises the struct gpu_power.
> 
> > > +     for_each_physical_engine(i915, signaler) {
> > > +             struct gpu_power_sample s_spin[2];
> > > +             struct gpu_power_sample s_sema[2];
> > > +             double baseline, total;
> > > +             int64_t jiffie = 1;
> > > +             igt_spin_t *spin;
> > > +
> > > +             spin = __igt_spin_batch_new(i915,
> > > +                                         .engine = signaler,
> > > +                                         .flags = IGT_SPIN_POLL_RUN);
> > > +             gem_wait(i915, spin->handle, &jiffie); /* waitboost */
> > > +             igt_assert(spin->running);
> > > +             igt_spin_busywait_until_running(spin);
> > > +
> > > +             gpu_power_read(&power, &s_spin[0]);
> > > +             usleep(100*1000);
> > > +             gpu_power_read(&power, &s_spin[1]);
> > Shouldn't we be checking results of gpu_power_read in both cases?
> 
> Why? If it fails the output is garbage, which is apparent in the info.
Fair enough.
> -Chris

I guess all LGTM.
Reviewed-by: Katarzyna Dec <katarzyna.dec at intel.com>
Kasia :)