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

Mika Kuoppala
2019-05-23T12:02:30Z

Chris Wilson <chris@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@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@intel.com>
>> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
>> > Reviewed-by: Andi Shyti <andi.shyti@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?

-Mika