[igt-dev] [PATCH i-g-t v3] i915/i915_hangman: fail only after freeing spinners

[Kamil Konieczny]

Hi Zbigniew,

On 2022-08-29 at 10:44:08 +0200, Zbigniew Kempczyński wrote:
> On Tue, Aug 23, 2022 at 11:56:45AM +0200, Kamil Konieczny wrote:
> > Failed checks may cause following tests fail, so check for
> > errors only after all spinners are released.
> > 
> > v2: be verbose about what failed (Chris)
> > v3: simplify declaration, add separate wait var (Zbigniew)
> > 
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > ---
> >  tests/i915/i915_hangman.c | 25 ++++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tests/i915/i915_hangman.c b/tests/i915/i915_hangman.c
> > index c7d69fdd..3b60ff46 100644
> > --- a/tests/i915/i915_hangman.c
> > +++ b/tests/i915/i915_hangman.c
> > @@ -295,6 +295,12 @@ static void context_unban(int fd, unsigned ctx)
> >  	gem_context_set_param(fd, &param);
> >  }
> >  
> > +#define ERR_HANG_WAIT  0
> > +#define ERR_HANG_STAT  1
> > +#define ERR_FENCE_BUSY 2
> > +#define ERR_FENCE_END  3
> > +#define ERR_FENCE_STAT 4
> > +
> >  static void
> >  test_engine_hang(const intel_ctx_t *ctx,
> >  		 const struct intel_execution_engine2 *e, unsigned int flags)
> > @@ -305,6 +311,7 @@ test_engine_hang(const intel_ctx_t *ctx,
> >  	IGT_LIST_HEAD(list);
> >  	uint64_t ahnd = get_reloc_ahnd(device, ctx->id), ahndN;
> >  	int num_ctx;
> > +	int err[ERR_FENCE_STAT + 1] = {};
> 
> bool err[ERR_FENCE_STAT + 1];
> 
> 
> >  
> >  	igt_skip_on(flags & IGT_SPIN_INVALID_CS &&
> >  		    gem_engine_has_cmdparser(device, &ctx->cfg, e->flags));
> > @@ -340,18 +347,21 @@ test_engine_hang(const intel_ctx_t *ctx,
> >  				      flags));
> >  
> >  	/* Wait for the hangcheck to terminate the hanger */
> > -	igt_assert(sync_fence_wait(spin->out_fence, 30000) == 0); /* 30s */
> > -	igt_assert_eq(sync_fence_status(spin->out_fence), -EIO);
> > +	err[ERR_HANG_WAIT] = sync_fence_wait(spin->out_fence, 30000) << 8; /* 30s */

You are right about that leftover "<< 8" (from your previous
letter), I will correct this in v4.

> 
> err[ERR_HANG_WAIT] = sync_fence_wait(spin->out_fence, 30000) != 0;
> 
> > +	err[ERR_HANG_STAT] = sync_fence_status(spin->out_fence); /* -EIO */
> 
> err[ERR_HANG_STAT] = sync_fence_status(spin->out_fence) != EIO;
> 
> 
> 
> >  	igt_spin_free(device, spin);
> >  
> >  	/* But no other engines/clients should be affected */
> >  	igt_list_for_each_entry_safe(spin, next, &list, link) {
> >  		ahndN = spin->opts.ahnd;
> > -		igt_assert(sync_fence_wait(spin->out_fence, 0) == -ETIME);
> > +		if (sync_fence_wait(spin->out_fence, 0) != -ETIME)
> > +			err[ERR_FENCE_BUSY]++;
> 
> err[ERR_FENCE_BUSY] = sync_fence_wait(spin->out_fence, 0) != -ETIME;
> 
> and so on? We could remove all 'if's then.

It will help with one-time errors, but will complicate for-loop
error accumulate, like
		if(!err[ERR_FENCE_BUSY]) // no error so far
			err[ERR_FENCE_BUSY] = sync_fence_wait(spin->out_fence, 0) != -ETIME;

or even more strange op like bit or on boolean results:
	err[ERR_FENCE_BUSY] |= sync_fence_wait(spin->out_fence, 0) != -ETIME;

The downside is that we lost error code when using boolean. Maybe
good idea would be to remember one unexpected error code in
for-loop ?

--
Kamil

> 
> --
> Zbigniew
> 
> >  		igt_spin_end(spin);
> >  
> > -		igt_assert(sync_fence_wait(spin->out_fence, 500) == 0);
> > -		igt_assert_eq(sync_fence_status(spin->out_fence), 1);
> > +		if (sync_fence_wait(spin->out_fence, 500) != 0)
> > +			err[ERR_FENCE_END]++;
> > +		if (sync_fence_status(spin->out_fence) != 1)
> > +			err[ERR_FENCE_STAT]++;
> >  		igt_spin_free(device, spin);
> >  		put_ahnd(ahndN);
> >  	}
> > @@ -360,6 +370,11 @@ test_engine_hang(const intel_ctx_t *ctx,
> >  	while (num_ctx)
> >  		intel_ctx_destroy(device, local_ctx[--num_ctx]);
> >  
> > +	igt_assert_f(err[ERR_HANG_WAIT] == 0, "hanged spinner wait failed\n");
> > +	igt_assert_f(err[ERR_HANG_STAT] == -EIO, "hanged spinner failed\n");
> > +	igt_assert_f(err[ERR_FENCE_BUSY] == 0, "background spinner not busy\n");
> > +	igt_assert_f(err[ERR_FENCE_END] == 0, "background spinner not terminated\n");
> > +	igt_assert_f(err[ERR_FENCE_STAT] == 0, "background fence not signalled\n");
> >  	check_alive();
> >  }
> >  
> > -- 
> > 2.34.1
> >