[PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[Mika Kuoppala]

If seqno has wrapped, normal compare operation will give wrong results.
i915_seqno_passed can handle the wrap so use it instead.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cdcf19d..35d5db8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2138,7 +2138,7 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
 	seqno = ring->get_seqno(ring, true);
 
 	for (i = 0; i < ARRAY_SIZE(ring->sync_seqno); i++)
-		if (seqno >= ring->sync_seqno[i])
+		if (i915_seqno_passed(seqno, ring->sync_seqno[i]))
 			ring->sync_seqno[i] = 0;
 
 	while (!list_empty(&ring->request_list)) {
@@ -2376,7 +2376,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 	idx = intel_ring_sync_index(from, to);
 
 	seqno = obj->last_read_seqno;
-	if (seqno <= from->sync_seqno[idx])
+	if (i915_seqno_passed(from->sync_seqno[idx], seqno))
 		return 0;
 
 	ret = i915_gem_check_olr(obj->ring, seqno);
-- 
1.7.9.5

Re: [PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[Chris Wilson]

On Tue, 13 Nov 2012 15:51:36 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> If seqno has wrapped, normal compare operation will give wrong results.
> i915_seqno_passed can handle the wrap so use it instead.

I'm still a little wary of this patch, as it means that we are emitting
requests that are not associated with an execbuffer and so the
wraparound detection is lost. Perhaps if you were to make the seqno
wraparound handling explicit we could all sleep more soundly...

And then I'd accept this patch for making seqno handling consistent at
least.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[Ben Widawsky]

On Tue, 13 Nov 2012 15:51:36 +0200
Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:

> If seqno has wrapped, normal compare operation will give wrong results.
> i915_seqno_passed can handle the wrap so use it instead.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index cdcf19d..35d5db8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2138,7 +2138,7 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  	seqno = ring->get_seqno(ring, true);
>  
>  	for (i = 0; i < ARRAY_SIZE(ring->sync_seqno); i++)
> -		if (seqno >= ring->sync_seqno[i])
> +		if (i915_seqno_passed(seqno, ring->sync_seqno[i]))
>  			ring->sync_seqno[i] = 0;
>  
>  	while (!list_empty(&ring->request_list)) {
> @@ -2376,7 +2376,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>  	idx = intel_ring_sync_index(from, to);
>  
>  	seqno = obj->last_read_seqno;
> -	if (seqno <= from->sync_seqno[idx])
> +	if (i915_seqno_passed(from->sync_seqno[idx], seqno))
>  		return 0;
>  
>  	ret = i915_gem_check_olr(obj->ring, seqno);

Can you add another patch on top of this to have the starting seqno be a
near pre-wrap value so we can catch bugs even without igt.

Also as an overall comment, I want the patches to guarantee to catch
the bug you found, which I think with the randomness of
gem_stress - isn't. Specifically, we want the waiting ring to be
waiting on a pre-wrapped value. Maybe I missed that guarantee, but if
there is a quick/dirty way to make that happen, that would better than
running an arbitrary number of gem_stress tests.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[Daniel Vetter]

On Tue, Nov 13, 2012 at 5:39 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>
> Also as an overall comment, I want the patches to guarantee to catch
> the bug you found, which I think with the randomness of
> gem_stress - isn't. Specifically, we want the waiting ring to be
> waiting on a pre-wrapped value. Maybe I missed that guarantee, but if
> there is a quick/dirty way to make that happen, that would better than
> running an arbitrary number of gem_stress tests.

I think running just gem_stress is ok - as long as the test has a
reasonable good chance of blowing up. On future platforms something
else than semaphores might blow up, or we might simply botch a seqno
comparison. So imo having a test that just beats a bit on the
systems+the wrap-around after each boot/resume should give us
excellent coverage, and trying to engineer a perfect test for the
single failure mode we now have in front of us might actually reduce
coverage.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[Ben Widawsky]

On Tue, 13 Nov 2012 17:45:14 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Nov 13, 2012 at 5:39 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> >
> > Also as an overall comment, I want the patches to guarantee to catch
> > the bug you found, which I think with the randomness of
> > gem_stress - isn't. Specifically, we want the waiting ring to be
> > waiting on a pre-wrapped value. Maybe I missed that guarantee, but if
> > there is a quick/dirty way to make that happen, that would better than
> > running an arbitrary number of gem_stress tests.
> 
> I think running just gem_stress is ok - as long as the test has a
> reasonable good chance of blowing up. On future platforms something
> else than semaphores might blow up, or we might simply botch a seqno
> comparison. So imo having a test that just beats a bit on the
> systems+the wrap-around after each boot/resume should give us
> excellent coverage, and trying to engineer a perfect test for the
> single failure mode we now have in front of us might actually reduce
> coverage.
> -Daniel

I didn't say don't run gem_stress...

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[Chris Wilson]

On Tue, 13 Nov 2012 17:45:14 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> trying to engineer a perfect test for the
> single failure mode we now have in front of us might actually reduce
> coverage.

This is a critical point worth remembering when writing tests: Having a
test case that detects a past bug is great, but a test case that
detects a future bug is better.
-Chris.

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[Mika Kuoppala]

On Tue, 13 Nov 2012 08:39:26 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> Can you add another patch on top of this to have the starting seqno be a
> near pre-wrap value so we can catch bugs even without igt.

I tried that but failed. I suspect that there is something in hw/sw
that doesn't like if first seqno is >0x80000000. Something in hw needs
to initialized explicitly if seqno is not starting from 1?

Re: [PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[Ben Widawsky]

On Mon, 19 Nov 2012 12:55:22 +0200
Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:

> 
> On Tue, 13 Nov 2012 08:39:26 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> > Can you add another patch on top of this to have the starting seqno be a
> > near pre-wrap value so we can catch bugs even without igt.
> 
> I tried that but failed. I suspect that there is something in hw/sw
> that doesn't like if first seqno is >0x80000000. Something in hw needs
> to initialized explicitly if seqno is not starting from 1?
> 

Nothing that I am aware of. If you have the first be 0, and then add a
huge jump right after that, does it work?

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Handle sync_seqno correctly when seqno has wrapped.

[Mika Kuoppala]

On Mon, 19 Nov 2012 09:21:48 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Mon, 19 Nov 2012 12:55:22 +0200
> Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> 
> > 
> > On Tue, 13 Nov 2012 08:39:26 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > Can you add another patch on top of this to have the starting seqno be a
> > > near pre-wrap value so we can catch bugs even without igt.
> > 
> > I tried that but failed. I suspect that there is something in hw/sw
> > that doesn't like if first seqno is >0x80000000. Something in hw needs
> > to initialized explicitly if seqno is not starting from 1?
> > 
> 
> Nothing that I am aware of. If you have the first be 0, and then add a
> huge jump right after that, does it work?

We can't jump more than 0x80000000-1 as i915_seqno_passed() breaks.
But with the patches for preallocate seqnos now in, I was able to
get things working when first seqno was 0xFFFF0000. 

-Mika