Re: [PATCH 2/2] drm/i915: Preallocate next seqno before touching the ring

[Chris Wilson <chris@chris-wilson.co.uk>]

On Tue, 27 Nov 2012 11:03:20 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> On Thu, 22 Nov 2012 13:07:21 +0000, Chris Wilson <chris@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@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@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