diff mbox

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

Message ID 1352814696-2154-1-git-send-email-mika.kuoppala@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mika Kuoppala Nov. 13, 2012, 1:51 p.m. UTC
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(-)

Comments

Chris Wilson Nov. 13, 2012, 2:15 p.m. UTC | #1
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
Ben Widawsky Nov. 13, 2012, 4:39 p.m. UTC | #2
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.
Daniel Vetter Nov. 13, 2012, 4:45 p.m. UTC | #3
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
Ben Widawsky Nov. 13, 2012, 4:52 p.m. UTC | #4
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...
Chris Wilson Nov. 13, 2012, 4:54 p.m. UTC | #5
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.
Mika Kuoppala Nov. 19, 2012, 10:55 a.m. UTC | #6
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?
Ben Widawsky Nov. 19, 2012, 5:21 p.m. UTC | #7
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?
Mika Kuoppala Nov. 29, 2012, 8:43 a.m. UTC | #8
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
diff mbox

Patch

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);