From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 893502B411 for ; Tue, 26 Jul 2022 14:27:57 +0000 (UTC) Date: Tue, 26 Jul 2022 16:27:54 +0200 From: Kamil Konieczny To: igt-dev@lists.freedesktop.org Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [igt-dev] [PATCH i-g-t v2 1/2] tests/gem_exec_fence: Check stored values only for valid workloads List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Chris Wilson Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: Hi Karolina, On 2022-07-26 at 12:13:11 +0200, Karolina Drobnik wrote: > From: Chris Wilson > > 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 As I understand it, reset can happen only after hang was detected ? > 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 > Signed-off-by: Karolina Drobnik > --- > tests/i915/gem_exec_fence.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c > index d46914c2..260aa82c 100644 > --- a/tests/i915/gem_exec_fence.c > +++ b/tests/i915/gem_exec_fence.c > @@ -350,18 +350,20 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx, > /* Long, but not too long to anger preemption disable checks */ > 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)); I tested this on drm-tip and this assert here did not trigger. It is not blocker (it can be moved into hang-only block), so Reviewed-by: Kamil Konieczny Regards, Kamil > - for (int n = 0; n < i; n++) > - igt_assert_eq_u32(out[n], 0); > + if ((flags & HANG) == 0) { > + /* 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_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 >