On Tue, 27 Nov 2012 11:03:20 +0200, Mika Kuoppala
<[email protected]> wrote:
> On Thu, 22 Nov 2012 13:07:21 +0000, Chris Wilson <[email protected]>
> 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 <[email protected]>
> > Cc: Mika Kuoppala <[email protected]>
> > 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
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
