[Intel-gfx] [igt-dev] [PATCH i-g-t v2 5/5] i915/gem_ctx_isolation.c - If initialization fails, exit

[Petri Latvala]

On Thu, Feb 13, 2020 at 11:29:48AM -0800, Dale B Stimson wrote:
> On 2020-02-13 10:29:55, Petri Latvala wrote:
> > On Wed, Feb 12, 2020 at 05:28:40PM -0800, Dale B Stimson wrote:
> > > At the start of igt_main, failure of the initial tests for successful
> > > initialization transfer control to the end of an igt_fixture block.
> > > From there, execution of the main per-engine loop is attempted.
> > > Instead, the test should be caused to exit.
> > > 
> > > If initialization fails, exit.
> > > 
> > > Signed-off-by: Dale B Stimson <dale.b.stimson at intel.com>
> > > ---
> > >  tests/i915/gem_ctx_isolation.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> > > index 07ffbb84a..b11158dab 100644
> > > --- a/tests/i915/gem_ctx_isolation.c
> > > +++ b/tests/i915/gem_ctx_isolation.c
> > > @@ -898,10 +898,13 @@ igt_main
> > >  	int fd = -1;
> > >  	struct eng_mmio_base_table_s *mbp = NULL;
> > >  	uint32_t mmio_base = 0;
> > > +	/* igt_fixture block is skipped if --list-subtests, so start with true. */
> > > +	bool init_successful = true;
> > >  
> > >  	igt_fixture {
> > >  		int gen;
> > >  
> > > +		init_successful = false;
> > >  		fd = drm_open_driver(DRIVER_INTEL);
> > >  		igt_require_gem(fd);
> > >  		igt_require(gem_has_contexts(fd));
> > > @@ -916,8 +919,20 @@ igt_main
> > >  		igt_skip_on(gen > LAST_KNOWN_GEN);
> > >  
> > >  		mbp = gem_engine_mmio_base_info_get(fd);
> > > +		init_successful = true;
> > >  	}
> > >  
> > > +	if (!init_successful) {
> > > +		igt_exit_early();
> > > +	}
> > > +
> > 
> > NAK. All this dancing around the infrastructure just makes changing
> > the infrastructure later be awkward and produce weird errors.
> > 
> > If something in the fixture failed, with this code you never enter the
> > subtest, making the test result 'notrun' instead of the correct 'skip'
> > or 'fail'.
> > 
> > What is the problem this is trying to solve? Incorrect engine list
> > used? If you have a subtest per static engine, all CI does is execute
> > per static engine. Converting this test to use dynamic subtests for
> > engines is the way forward.
> > 
> > 
> > -- 
> > Petri Latvala
> 
> NAK understood and accepted.
> 
> I will address this in a different way, targeting the underlying issues
> instead of the symptom.  Please see my patch (just sent to ML):
>   lib/i915/gem_engine_topology.c - intel_get_current_engine invalid result
> 
> To answer to your question about what this was trying to solve:
> 
> Briefly, and specifically addressing gem_ctx_isolation:
> 
> As-is observed behavior when open (or debugfs open) fails: per-engine loop
> executes forever:
>     Subtest vecs0-nonpriv: FAIL
>     Subtest vecs0-nonpriv-switch: FAIL
>     Subtest vecs0-clean: FAIL
>     Subtest vecs0-dirty-create: FAIL
>     Subtest vecs0-dirty-switch: FAIL
>     Subtest vecs0-none: FAIL
>     Subtest vecs0-S3: FAIL
>     Subtest vecs0-S4: FAIL
>     Subtest vecs0-reset: FAIL
>     And repeat, ad infinitum for vecs0
> 

Ah, the good old non-progressing engine loop. We already have fixes
for two of the occurrences, you have found a third one. =(


-- 
Petri Latvala