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

Tvrtko Ursulin
2019-01-28T13:44:47Z

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@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