[Intel-gfx] [igt-dev] [PATCH i-g-t v2] tests/i915: Restore some BAT coverage

[Chris Wilson]

Quoting Mika Kuoppala (2019-05-23 13:02:30)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > Quoting Tvrtko Ursulin (2019-05-23 07:43:12)
> >> 
> >> On 23/05/2019 07:37, Tvrtko Ursulin wrote:
> >> > From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> > 
> >> > Engine enumerated test names have changed so fast-feedback.testlist needs
> >> > to be updated. However listing all engines there won't scale. So instead
> >> > add new tests cases which iterate all engines internally.
> >> > 
> >> > v2:
> >> >   * Fix basic-all test name.
> >> >   * Fix params to basic (bool false to zero).
> >> >   * Use I915_EXEC_DEFAULT in has_extended_busy_ioctl to workaround stateful
> >> >     contexts for now.
> >> >   * Have only basic-all in BAT. (Chris)
> >> > 
> >> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> > Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk> # v1
> >> > Reviewed-by: Andi Shyti <andi.shyti at intel.com> # v1
> >> > ---
> >> >   tests/i915/gem_busy.c                 | 19 +++++++++++----
> >> >   tests/i915/gem_exec_basic.c           | 33 +++++++++++++++++++++++++++
> >> >   tests/intel-ci/fast-feedback.testlist | 31 ++-----------------------
> >> >   3 files changed, 50 insertions(+), 33 deletions(-)
> >> > 
> >> > diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
> >> > index 781a3bfab1d1..f3ebb37a33b4 100644
> >> > --- a/tests/i915/gem_busy.c
> >> > +++ b/tests/i915/gem_busy.c
> >> > @@ -417,7 +417,7 @@ static bool has_semaphores(int fd)
> >> >   
> >> >   static bool has_extended_busy_ioctl(int fd)
> >> >   {
> >> > -     igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_RENDER);
> >> > +     igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_DEFAULT);
> >> 
> >> (This fails on platforms with only rcs0 (no other engines) due context 
> >> has a map now, and I915_EXEC_RENDER == 1 == -EINVAL.)
> >
> > In which case, it probably should be a plain 0 as it no longer has the
> > old EXEC_RING semantics but is just an index, i.e. igt_spin_new(fd);
> >
> >> We need to come up with a robust and easy to remember solution for 
> >> dealing with the fact contexts are stateful now and 
> >> __for_each_physical_engine iterator configures the default one.
> >> 
> >> Could end game for test conversion be to stop passing in eb.flags to 
> >> igt_spin_new and do class:instance instead? That would enable dummyload 
> >> to unambiguously know what to use, depending on get_engines query.
> >
> > Speak to Mika, we abuse igt_spin_t much more by resubmitting the same
> > dummyload to multiple engines and contexts.
> 
> I have wandered there yes. The reason for this abuse is still unclear.
> We could do a clearer interface for creating spinners but is the
> reusage due to latency concerns?

No, just because we are lazy and creating a clean, clear interface takes
time and lots of effort. So we excuse ourselves to get something that
does the job and once we know what we want, we tell the next fool to
come along to fix it neatly. :)
-Chris