| Message ID | 20190127130615.12917-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] i915/gem_workarounds: Require GPU resets | expand |
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? Regards, Tvrtko
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. -Chris
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. -Chris
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
Quoting Tvrtko Ursulin (2019-01-28 13:44:47) > > On 28/01/2019 11:12, Chris Wilson wrote: > Agreed on that, but worry who will notice it in review. For the next week of struct_mutex removal (haha), CI will tell us. -Chris
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:
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(-)