[igt-dev] [PATCH i-g-t 1/2] tests/gem_exec_fence: Check stored values only for valid workloads

[Kamil Konieczny]

Hi,

one more nit, see below.

On 2022-07-20 at 16:01:03 +0200, Karolina Drobnik wrote:
> From: Chris Wilson <chris at chris-wilson.co.uk>
> 
> test_fence_await verifies if a fence used to pipeline work signals
> correctly. await-hang and nb-await-hang test cases inject GPU hang,
> which causes an erroneous state, meaning the fence will be signaled
> without execution. The error causes an instant reset of the command
> streamer for the hanging workload. This revealed a problem with how
> we verify the fence state and results. The test assumes that the
> error notification happens after a hang is declared, which takes
> longer than submitting the next set of fences, making the test pass
> every time. With the immediate reset, this might not happen, so the
> assertion fails, as there are no active fences in the GPU hang case.
> 
> Move the check for active fence to the path for non-hanging workload,
> and verify results only in this scenario.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Karolina Drobnik <karolina.drobnik at intel.com>
> ---
>  tests/i915/gem_exec_fence.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c
> index d46914c2..c1bd2a38 100644
> --- a/tests/i915/gem_exec_fence.c
> +++ b/tests/i915/gem_exec_fence.c
> @@ -351,17 +351,19 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx,
>  	usleep(50 * 1000); /* 50 ms, typical preempt reset is 150+ms */
> 
>  	/* Check for invalidly completing the task early */
> -	igt_assert(fence_busy(spin->out_fence));
> -	for (int n = 0; n < i; n++)
> -		igt_assert_eq_u32(out[n], 0);
> +	if ((flags & HANG) == 0) {
> +		igt_assert(fence_busy(spin->out_fence));

imho this assert should be before if check, it is valid for
both paths no-hang and hang. We wait only 50ms and hang will be
detected after that time (comment states +150ms) and that will
happen while we wait for childs in igt_wailchildren() below,
or am I missing something ?

--
Kamil

> +		for (int n = 0; n < i; n++)
> +			igt_assert_eq_u32(out[n], 0);
> 
> -	if ((flags & HANG) == 0)
>  		igt_spin_end(spin);
> +	}
> 
>  	igt_waitchildren();
> 
>  	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);
> -	while (i--)
> +	igt_assert(!fence_busy(spin->out_fence));
> +	while ((flags & HANG) == 0 && i--)
>  		igt_assert_eq_u32(out[i], i);
>  	munmap(out, 4096);
> 
> --
> 2.25.1