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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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? 

Nevertheless, that break there still seems highly suspicious.

> +	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?

No remarks on rest of the patch.

-Mika

> +	return 0;
>  }
>  
> -u32
> -i915_gem_next_request_seqno(struct intel_ring_buffer *ring)
> +int
> +i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
>  {
> -	if (ring->outstanding_lazy_request == 0)
> -		ring->outstanding_lazy_request = i915_gem_get_seqno(ring->dev);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* reserve 0 for non-seqno */
> +	if (dev_priv->next_seqno == 0) {
> +		int ret = i915_gem_handle_seqno_wrap(dev);
> +		if (ret)
> +			return ret;
>  
> -	return ring->outstanding_lazy_request;
> +		dev_priv->next_seqno = 1;
> +	}
> +
> +	*seqno = dev_priv->next_seqno++;
> +	return 0;
>  }
>  
>  int
> @@ -1952,7 +1984,6 @@ i915_add_request(struct intel_ring_buffer *ring,
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
>  	struct drm_i915_gem_request *request;
>  	u32 request_ring_position;
> -	u32 seqno;
>  	int was_empty;
>  	int ret;
>  
> @@ -1971,7 +2002,6 @@ i915_add_request(struct intel_ring_buffer *ring,
>  	if (request == NULL)
>  		return -ENOMEM;
>  
> -	seqno = i915_gem_next_request_seqno(ring);
>  
>  	/* Record the position of the start of the request so that
>  	 * should we detect the updated seqno part-way through the
> @@ -1980,15 +2010,13 @@ i915_add_request(struct intel_ring_buffer *ring,
>  	 */
>  	request_ring_position = intel_ring_get_tail(ring);
>  
> -	ret = ring->add_request(ring, &seqno);
> +	ret = ring->add_request(ring);
>  	if (ret) {
>  		kfree(request);
>  		return ret;
>  	}
>  
> -	trace_i915_gem_request_add(ring, seqno);
> -
> -	request->seqno = seqno;
> +	request->seqno = intel_ring_get_seqno(ring);
>  	request->ring = ring;
>  	request->tail = request_ring_position;
>  	request->emitted_jiffies = jiffies;
> @@ -2006,6 +2034,7 @@ i915_add_request(struct intel_ring_buffer *ring,
>  		spin_unlock(&file_priv->mm.lock);
>  	}
>  
> +	trace_i915_gem_request_add(ring, request->seqno);
>  	ring->outstanding_lazy_request = 0;
>  
>  	if (!dev_priv->mm.suspended) {
> @@ -2022,7 +2051,7 @@ i915_add_request(struct intel_ring_buffer *ring,
>  	}
>  
>  	if (out_seqno)
> -		*out_seqno = seqno;
> +		*out_seqno = request->seqno;
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 0e510df..a3f06bc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -410,9 +410,8 @@ static int do_switch(struct i915_hw_context *to)
>  	 * MI_SET_CONTEXT instead of when the next seqno has completed.
>  	 */
>  	if (from_obj != NULL) {
> -		u32 seqno = i915_gem_next_request_seqno(ring);
>  		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> -		i915_gem_object_move_to_active(from_obj, ring, seqno);
> +		i915_gem_object_move_to_active(from_obj, ring);
>  		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
>  		 * whole damn pipeline, we don't need to explicitly mark the
>  		 * object dirty. The only exception is that the context must be
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 48e4317..f5620db 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -713,8 +713,7 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
>  
>  static void
>  i915_gem_execbuffer_move_to_active(struct list_head *objects,
> -				   struct intel_ring_buffer *ring,
> -				   u32 seqno)
> +				   struct intel_ring_buffer *ring)
>  {
>  	struct drm_i915_gem_object *obj;
>  
> @@ -726,10 +725,10 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
>  		obj->base.write_domain = obj->base.pending_write_domain;
>  		obj->fenced_gpu_access = obj->pending_fenced_gpu_access;
>  
> -		i915_gem_object_move_to_active(obj, ring, seqno);
> +		i915_gem_object_move_to_active(obj, ring);
>  		if (obj->base.write_domain) {
>  			obj->dirty = 1;
> -			obj->last_write_seqno = seqno;
> +			obj->last_write_seqno = intel_ring_get_seqno(ring);
>  			if (obj->pin_count) /* check for potential scanout */
>  				intel_mark_fb_busy(obj);
>  		}
> @@ -789,7 +788,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	struct intel_ring_buffer *ring;
>  	u32 ctx_id = i915_execbuffer2_get_context_id(*args);
>  	u32 exec_start, exec_len;
> -	u32 seqno;
>  	u32 mask;
>  	u32 flags;
>  	int ret, mode, i;
> @@ -994,22 +992,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto err;
>  
> -	seqno = i915_gem_next_request_seqno(ring);
> -	for (i = 0; i < ARRAY_SIZE(ring->sync_seqno); i++) {
> -		if (seqno < ring->sync_seqno[i]) {
> -			/* The GPU can not handle its semaphore value wrapping,
> -			 * so every billion or so execbuffers, we need to stall
> -			 * the GPU in order to reset the counters.
> -			 */
> -			ret = i915_gpu_idle(dev);
> -			if (ret)
> -				goto err;
> -			i915_gem_retire_requests(dev);
> -
> -			BUG_ON(ring->sync_seqno[i]);
> -		}
> -	}
> -
>  	ret = i915_switch_context(ring, file, ctx_id);
>  	if (ret)
>  		goto err;
> @@ -1035,8 +1017,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  	}
>  
> -	trace_i915_gem_ring_dispatch(ring, seqno, flags);
> -
>  	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
>  	exec_len = args->batch_len;
>  	if (cliprects) {
> @@ -1060,7 +1040,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  	}
>  
> -	i915_gem_execbuffer_move_to_active(&objects, ring, seqno);
> +	trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring));
> +
> +	i915_gem_execbuffer_move_to_active(&objects, ring);
>  	i915_gem_execbuffer_retire_commands(dev, file, ring);
>  
>  err:
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 987eb5f..e6dabb9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -555,12 +555,11 @@ static void render_ring_cleanup(struct intel_ring_buffer *ring)
>  
>  static void
>  update_mboxes(struct intel_ring_buffer *ring,
> -	    u32 seqno,
> -	    u32 mmio_offset)
> +	      u32 mmio_offset)
>  {
>  	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
>  	intel_ring_emit(ring, mmio_offset);
> -	intel_ring_emit(ring, seqno);
> +	intel_ring_emit(ring, ring->outstanding_lazy_request);
>  }
>  
>  /**
> @@ -573,8 +572,7 @@ update_mboxes(struct intel_ring_buffer *ring,
>   * This acts like a signal in the canonical semaphore.
>   */
>  static int
> -gen6_add_request(struct intel_ring_buffer *ring,
> -		 u32 *seqno)
> +gen6_add_request(struct intel_ring_buffer *ring)
>  {
>  	u32 mbox1_reg;
>  	u32 mbox2_reg;
> @@ -587,13 +585,11 @@ gen6_add_request(struct intel_ring_buffer *ring,
>  	mbox1_reg = ring->signal_mbox[0];
>  	mbox2_reg = ring->signal_mbox[1];
>  
> -	*seqno = i915_gem_next_request_seqno(ring);
> -
> -	update_mboxes(ring, *seqno, mbox1_reg);
> -	update_mboxes(ring, *seqno, mbox2_reg);
> +	update_mboxes(ring, mbox1_reg);
> +	update_mboxes(ring, mbox2_reg);
>  	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
>  	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
> -	intel_ring_emit(ring, *seqno);
> +	intel_ring_emit(ring, ring->outstanding_lazy_request);
>  	intel_ring_emit(ring, MI_USER_INTERRUPT);
>  	intel_ring_advance(ring);
>  
> @@ -650,10 +646,8 @@ do {									\
>  } while (0)
>  
>  static int
> -pc_render_add_request(struct intel_ring_buffer *ring,
> -		      u32 *result)
> +pc_render_add_request(struct intel_ring_buffer *ring)
>  {
> -	u32 seqno = i915_gem_next_request_seqno(ring);
>  	struct pipe_control *pc = ring->private;
>  	u32 scratch_addr = pc->gtt_offset + 128;
>  	int ret;
> @@ -674,7 +668,7 @@ pc_render_add_request(struct intel_ring_buffer *ring,
>  			PIPE_CONTROL_WRITE_FLUSH |
>  			PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE);
>  	intel_ring_emit(ring, pc->gtt_offset | PIPE_CONTROL_GLOBAL_GTT);
> -	intel_ring_emit(ring, seqno);
> +	intel_ring_emit(ring, ring->outstanding_lazy_request);
>  	intel_ring_emit(ring, 0);
>  	PIPE_CONTROL_FLUSH(ring, scratch_addr);
>  	scratch_addr += 128; /* write to separate cachelines */
> @@ -693,11 +687,10 @@ pc_render_add_request(struct intel_ring_buffer *ring,
>  			PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE |
>  			PIPE_CONTROL_NOTIFY);
>  	intel_ring_emit(ring, pc->gtt_offset | PIPE_CONTROL_GLOBAL_GTT);
> -	intel_ring_emit(ring, seqno);
> +	intel_ring_emit(ring, ring->outstanding_lazy_request);
>  	intel_ring_emit(ring, 0);
>  	intel_ring_advance(ring);
>  
> -	*result = seqno;
>  	return 0;
>  }
>  
> @@ -885,25 +878,20 @@ bsd_ring_flush(struct intel_ring_buffer *ring,
>  }
>  
>  static int
> -i9xx_add_request(struct intel_ring_buffer *ring,
> -		 u32 *result)
> +i9xx_add_request(struct intel_ring_buffer *ring)
>  {
> -	u32 seqno;
>  	int ret;
>  
>  	ret = intel_ring_begin(ring, 4);
>  	if (ret)
>  		return ret;
>  
> -	seqno = i915_gem_next_request_seqno(ring);
> -
>  	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
>  	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
> -	intel_ring_emit(ring, seqno);
> +	intel_ring_emit(ring, ring->outstanding_lazy_request);
>  	intel_ring_emit(ring, MI_USER_INTERRUPT);
>  	intel_ring_advance(ring);
>  
> -	*result = seqno;
>  	return 0;
>  }
>  
> @@ -1338,6 +1326,15 @@ int intel_wait_ring_buffer(struct intel_ring_buffer *ring, int n)
>  	return -EBUSY;
>  }
>  
> +static int
> +intel_ring_alloc_seqno(struct intel_ring_buffer *ring)
> +{
> +	if (ring->outstanding_lazy_request)
> +		return 0;
> +
> +	return i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_request);
> +}
> +
>  int intel_ring_begin(struct intel_ring_buffer *ring,
>  		     int num_dwords)
>  {
> @@ -1349,6 +1346,11 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
>  	if (ret)
>  		return ret;
>  
> +	/* Preallocate the olr before touching the ring */
> +	ret = intel_ring_alloc_seqno(ring);
> +	if (ret)
> +		return ret;
> +
>  	if (unlikely(ring->tail + n > ring->effective_size)) {
>  		ret = intel_wrap_ring_buffer(ring);
>  		if (unlikely(ret))
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 5af65b8..0e61302 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -70,8 +70,7 @@ struct  intel_ring_buffer {
>  	int __must_check (*flush)(struct intel_ring_buffer *ring,
>  				  u32	invalidate_domains,
>  				  u32	flush_domains);
> -	int		(*add_request)(struct intel_ring_buffer *ring,
> -				       u32 *seqno);
> +	int		(*add_request)(struct intel_ring_buffer *ring);
>  	/* Some chipsets are not quite as coherent as advertised and need
>  	 * an expensive kick to force a true read of the up-to-date seqno.
>  	 * However, the up-to-date seqno is not always required and the last
> @@ -205,7 +204,6 @@ static inline void intel_ring_emit(struct intel_ring_buffer *ring,
>  
>  void intel_ring_advance(struct intel_ring_buffer *ring);
>  
> -u32 intel_ring_get_seqno(struct intel_ring_buffer *ring);
>  int intel_ring_flush_all_caches(struct intel_ring_buffer *ring);
>  int intel_ring_invalidate_all_caches(struct intel_ring_buffer *ring);
>  
> @@ -221,6 +219,12 @@ static inline u32 intel_ring_get_tail(struct intel_ring_buffer *ring)
>  	return ring->tail;
>  }
>  
> +static inline u32 intel_ring_get_seqno(struct intel_ring_buffer *ring)
> +{
> +	BUG_ON(ring->outstanding_lazy_request == 0);
> +	return ring->outstanding_lazy_request;
> +}
> +
>  static inline void i915_trace_irq_get(struct intel_ring_buffer *ring, u32 seqno)
>  {
>  	if (ring->trace_irq_seqno == 0 && ring->irq_get(ring))
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux