[igt-dev] [Intel-gfx] [PATCH i-g-t] i915/gem_workarounds: Require GPU resets

[Tvrtko Ursulin]

On 28/01/2019 11:12, Chris Wilson wrote:
> Quoting Chris Wilson (2019-01-28 11:07:40)
>> Quoting Tvrtko Ursulin (2019-01-28 11:03:30)
>>>
>>> On 27/01/2019 13:06, Chris Wilson wrote:
>>>> Check that we are allowed to reset the GPU prior to execution.
>>>>
>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>> ---
>>>>    tests/i915/gem_workarounds.c | 6 +++++-
>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tests/i915/gem_workarounds.c b/tests/i915/gem_workarounds.c
>>>> index 78478ad2c..0e9ab2386 100644
>>>> --- a/tests/i915/gem_workarounds.c
>>>> +++ b/tests/i915/gem_workarounds.c
>>>> @@ -192,7 +192,11 @@ static void check_workarounds(int fd, enum operation op, unsigned int flags)
>>>>    
>>>>        switch (op) {
>>>>        case GPU_RESET:
>>>> -             igt_force_gpu_reset(fd);
>>>> +             {
>>>> +                     igt_hang_t hang = igt_allow_hang(fd, ctx, 0);
>>>> +                     igt_force_gpu_reset(fd);
>>>> +                     igt_disallow_hang(fd, hang);
>>>> +             }
>>>>                break;
>>>>    
>>>>        case SUSPEND_RESUME:
>>>>
>>>
>>> If it doesn't make sense to add the checks into igt_force_gpu_reset (so
>>> force means force), should we have igt_try_gpu_reset to avoid having to
>>> wrap it everywhere?
>>
>> Ugh. igt_try_gpu_reset_with_many_many_options_depending_on_test().
>>
>> I like my requires as high up in the chain as possible, I've been bitten
>> too many times by hiding them.
> 
> The key benefit from doing it higher up, is that we should be doing it
> as early as possible and should be more descriptive about why.

Agreed on that, but worry who will notice it in review.

Regards,

Tvrtko