[Intel-gfx] [igt-dev] [PATCH igt] igt/gem_ctx_isolation: Reset a scratch context

[Tvrtko Ursulin]

[resend for typo in cc]

On 27/03/2018 14:59, Chris Wilson wrote:
> Quoting Chris Wilson (2018-03-27 14:52:20)
>> Quoting Chris Wilson (2018-03-27 14:48:43)
>>> If we inject a reset into the target context, there is a risk that the
>>> register state is never saved back to memory. The exact interaction
>>> between reset, the context image and the precise timing of our execution
>>> are not well defined. Since we cannot ensure that the context image
>>> remains valid, force a context switch prior to the reset.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105270
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105457
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105545
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> ---
>>>   tests/gem_ctx_isolation.c | 28 +++++++++++++++++++++++++++-
>>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/gem_ctx_isolation.c b/tests/gem_ctx_isolation.c
>>> index d8109aa0..4968e367 100644
>>> --- a/tests/gem_ctx_isolation.c
>>> +++ b/tests/gem_ctx_isolation.c
>>> @@ -522,6 +522,32 @@ static void isolation(int fd,
>>>   #define S4 (4 << 8)
>>>   #define SLEEP_MASK (0xf << 8)
>>>   
>>> +static void inject_reset_context(int fd, unsigned int engine)
>>> +{
>>> +       igt_spin_t *spin;
>>> +       uint32_t ctx;
>>> +
>>> +       /*
>>> +        * Force a context switch before triggering the reset, or else
>>> +        * we risk corrupting the target context and we can't blame the
>>> +        * HW for screwing up if the context was already broken.
>>> +        */
>>> +
>>> +       ctx = gem_context_create(fd);
>>> +       if (gem_can_store_dword(fd, engine)) {
>>> +               spin = __igt_spin_batch_new_poll(fd, ctx, engine);
>>> +               igt_spin_busywait_until_running(spin);
>>> +       } else {
>>> +               spin = __igt_spin_batch_new(fd, ctx, engine, 0);
>>> +               usleep(1000); /* better than nothing */
>>> +       }
>>
>> Tvrtko, maybe we want igt_spin_batch_run()? Not sure though, so far we
>> have an example where you need precise control and a couple of examples
>> where we just want a running spinner.

I wasn't sure it is in good taste to put a thing with that usleep in 
lib/, since it incurs a delay to unsuspecting callers. And I couldn't 
decide how to handle it better - skip from lib/? Not so great. Return 
value and make callers handle it - even more cumbersome.

It only affects old gens, but do we want tests there suddenly take much 
longer because someone spotted a cool new API and went to use it?

But it is a third user now so not great to copy paste it all around. I 
don't know. Make the lib functions skip where unsupported and add double 
underscore prefix flavour which sleeps where not supported?

> Also this whole function might want to make its way into lib/i915 as the
> basis of all hang/reset injection. Some improvement required to set
> context parameters (e.g. disabling error state capturing) and skipping
> if no reset is available or disabled (although that's the job for the
> fixture, so may not be required for the library function). I'm sure
> someone will catch me reusing this chunk over and over again...

Sounds OK to me.

Regards,

Tvrtko