[igt-dev] [PATCH i-g-t] tests/i915/i915_hangman : Skips test for RCS/CCS on DG2

[Petri Latvala]

On Mon, Jan 24, 2022 at 10:26:20AM -0800, John Harrison wrote:
> On 1/24/2022 10:12, John Harrison wrote:
> > On 1/24/2022 06:42, priyanka.dandamudi at intel.com wrote:
> > > From: Priyanka Dandamudi <priyanka.dandamudi at intel.com>
> > > 
> > > Updated code to skip for RCS/CCS engines on DG2 as the
> > > platform cant handle shared reset domain.
> > Is this based on running the test and seeing it fail? Or just thinking
> > that it is a problem?
> > 
> > As per my update to this test in the internal tree, I do not see it fail
> > on the CCS engine only RCS. That also correlates with the nature of the
> > DG2 workaround - if you hang RCS then CCS cannot switch but if you hang
> > CCS then RCS can still switch quite happily. So if this test is failing
> > on CCS in upstream then we have a bug that needs to be fixed.
> > 
> > John.
> PS: Meant to add that making the skip is only a short term hack to get the
> CI results green. The correct fix is to update the test to be aware of the
> implications of the workaround. I.e. still run the hang test on all engines
> but, on DG2, expect more than one engine to be reset as appropriate to the
> workaround.

Adding a skip just to make results green is papering over
issues. Unless there's a very good reason to do it (the machine
explodes when executing the test etc) just let it fail and track it as
a bug.

> 
> Lastly, just adding a continue to the loop is not the correct way to skip a
> test. That just makes the test vanish with no record that it is being
> skipped. You should use the igt_skip helper. That way, the test is still
> seen to exist can the skip can be tracked.

Generally, the best practices for _dynamic_ subtests is to have the
dynamic subtest vanish. But in this case if the test case is expected
to return, we should let it exist. And as mentioned above, let it
fail.


-- 
Petri Latvala


> 
> John.
> 
> 
> > 
> > > 
> > > Signed-off-by: Priyanka Dandamudi <priyanka.dandamudi at intel.com>
> > > Cc: Melkaveri, Arjun <arjun.melkaveri at intel.com>
> > > ---
> > >   tests/i915/i915_hangman.c | 11 ++++++++++-
> > >   1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/i915/i915_hangman.c b/tests/i915/i915_hangman.c
> > > index 23055c27..abbdaed5 100644
> > > --- a/tests/i915/i915_hangman.c
> > > +++ b/tests/i915/i915_hangman.c
> > > @@ -445,6 +445,9 @@ static void do_tests(const char *name, const
> > > char *prefix,
> > >       snprintf(buff, sizeof(buff), "%s-error-state-capture", prefix);
> > >       igt_subtest_with_dynamic(buff) {
> > >           for_each_ctx_engine(device, ctx, e) {
> > > +            if (IS_DG2(intel_get_drm_devid(device)) &&
> > > +                    (e->class == I915_ENGINE_CLASS_RENDER ||
> > > e->class == I915_ENGINE_CLASS_COMPUTE))
> > > +                                continue;
> > >               igt_dynamic_f("%s", e->name)
> > >                   test_error_state_capture(ctx, e);
> > >           }
> > > @@ -466,7 +469,10 @@ static void do_tests(const char *name, const
> > > char *prefix,
> > >           igt_require(has_gpu_reset > 1);
> > >             for_each_ctx_engine(device, ctx, e) {
> > > -            igt_dynamic_f("%s", e->name)
> > > +            if (IS_DG2(intel_get_drm_devid(device)) &&
> > > +                    (e->class == I915_ENGINE_CLASS_RENDER ||
> > > e->class == I915_ENGINE_CLASS_COMPUTE))
> > > +                continue;
> > > +                igt_dynamic_f("%s", e->name)
> > >                   test_engine_hang(ctx, e, 0);
> > >           }
> > >       }
> > > @@ -486,6 +492,9 @@ static void do_tests(const char *name, const
> > > char *prefix,
> > >           igt_require(has_gpu_reset > 1);
> > >             for_each_ctx_engine(device, ctx, e) {
> > > +            if (IS_DG2(intel_get_drm_devid(device)) &&
> > > +                    (e->class == I915_ENGINE_CLASS_RENDER ||
> > > e->class == I915_ENGINE_CLASS_COMPUTE))
> > > +                continue;
> > >               igt_dynamic_f("%s", e->name)
> > >                   test_engine_hang(ctx, e, IGT_SPIN_INVALID_CS);
> > >           }
> > 
>