On Tue, 27 Nov 2012 11:03:20 +0200, Mika Kuoppala <mika.kuoppala at linux.intel.com> wrote: > On Thu, 22 Nov 2012 13:07:21 +0000, Chris Wilson <chris at chris-wilson.co.uk> wrote: > > Based on the work by Mika Kuoppala, we realised that we need to handle > > seqno wraparound prior to committing our changes to the ring. The most > > obvious point then is to grab the seqno inside intel_ring_begin(), and > > then to reuse that seqno for all ring operations until the next request. > > As intel_ring_begin() can fail, the callers must already be prepared to > > handle such failure and so we can safely add further checks. > > > > This patch looks like it should be split up into the interface > > changes and the tweaks to move seqno wrapping from the execbuffer into > > the core seqno increment. However, I found no easy way to break it into > > incremental steps without introducing further broken behaviour. > > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala at intel.com> > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=863861 > > --- > > drivers/gpu/drm/i915/i915_drv.h | 5 +- > > drivers/gpu/drm/i915/i915_gem.c | 73 +++++++++++++++++++--------- > > drivers/gpu/drm/i915/i915_gem_context.c | 3 +- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 30 +++--------- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 48 +++++++++--------- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 10 ++-- > > 6 files changed, 92 insertions(+), 77 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 87c06f9..e473e5d 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1417,8 +1417,7 @@ int __must_check i915_mutex_lock_interruptible(struct drm_device *dev); > > int i915_gem_object_sync(struct drm_i915_gem_object *obj, > > struct intel_ring_buffer *to); > > void i915_gem_object_move_to_active(struct drm_i915_gem_object *obj, > > - struct intel_ring_buffer *ring, > > - u32 seqno); > > + struct intel_ring_buffer *ring); > > > > int i915_gem_dumb_create(struct drm_file *file_priv, > > struct drm_device *dev, > > @@ -1436,7 +1435,7 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2) > > return (int32_t)(seq1 - seq2) >= 0; > > } > > > > -u32 i915_gem_next_request_seqno(struct intel_ring_buffer *ring); > > +extern int i915_gem_get_seqno(struct drm_device *dev, u32 *seqno); > > > > int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj); > > int __must_check i915_gem_object_put_fence(struct drm_i915_gem_object *obj); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 9be450e..8b9a356 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -1857,11 +1857,11 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj) > > > > void > > i915_gem_object_move_to_active(struct drm_i915_gem_object *obj, > > - struct intel_ring_buffer *ring, > > - u32 seqno) > > + struct intel_ring_buffer *ring) > > { > > struct drm_device *dev = obj->base.dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > + u32 seqno = intel_ring_get_seqno(ring); > > > > BUG_ON(ring == NULL); > > obj->ring = ring; > > @@ -1922,26 +1922,58 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) > > WARN_ON(i915_verify_lists(dev)); > > } > > > > -static u32 > > -i915_gem_get_seqno(struct drm_device *dev) > > +static int > > +i915_gem_handle_seqno_wrap(struct drm_device *dev) > > { > > - drm_i915_private_t *dev_priv = dev->dev_private; > > - u32 seqno = dev_priv->next_seqno; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct intel_ring_buffer *ring; > > + int ret, i, j; > > > > - /* reserve 0 for non-seqno */ > > - if (++dev_priv->next_seqno == 0) > > - dev_priv->next_seqno = 1; > > + /* The hardware uses various monotonic 32-bit counters, if we > > + * detect that they will wraparound we need to idle the GPU > > + * and reset those counters. > > + */ > > + > > + ret = 0; > > + for_each_ring(ring, dev_priv, i) { > > + for (j = 0; j < ARRAY_SIZE(ring->sync_seqno); j++) { > > + ret |= ring->sync_seqno[j] != 0; > > + break; > > + } > > + } > > If we don't sync (using hw semaphores) across wrap boundary, we > dont need to retire requests if we wrap? Correct, at the moment we only need to worry about hw semaphores. > Nevertheless, that break there still seems highly suspicious. Brainfart, thanks. > > > + if (ret == 0) > > + return ret; > > + > > + ret = i915_gpu_idle(dev); > > + if (ret) > > + return ret; > > > > - return seqno; > > + i915_gem_retire_requests(dev); > > + > > + for_each_ring(ring, dev_priv, i) { > > + for (j = 0; j < ARRAY_SIZE(ring->sync_seqno); j++) > > + ring->sync_seqno[j] = 0; > > + } > > i915_gem_retire_requests_ring should set syn_seqnos to 0. > Why not BUG_ON(ring->sync_seqno[j]) instead? Oh boy, because i915_gem_retire_requests_ring() is buggy. -Chris -- Chris Wilson, Intel Open Source Technology Centre
