| Message ID | 1352814696-2154-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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...
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.
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?
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?
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 --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);
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(-)