Re: [PATCH v2 3/3] drm/i915: Consolidate ring freespace calculations

[Deepak S <deepak.s@linux.intel.com>]

On Wednesday 19 November 2014 01:37 AM, Dave Gordon wrote:
> There are numerous places in the code where the driver's idea of
> how much space is left in a ring is updated using the driver's
> latest notions of the positions of 'head' and 'tail' for the ring.
> Among them are some that update one or both of these values before
> (re)doing the calculation. In particular, there are four different
> places in the code where 'last_retired_head' is copied to 'head'
> and then set to -1; and two of these do not have a guard to check
> that it has actually been updated since last time it was consumed,
> leaving the possibility that the dummy -1 can be transferred from
> 'last_retired_head' to 'head', causing the space calculation to
> produce 'impossible' results (previously seen on Android/VLV).
>
> This code therefore consolidates all the calculation and updating of
> these values, such that there is only one place where the ring space
> is updated, and it ALWAYS uses (and consumes) 'last_retired_head' if
> (and ONLY if) it has been updated since the last call.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_dma.c         |    5 ++-
>   drivers/gpu/drm/i915/intel_lrc.c        |   25 +++++----------
>   drivers/gpu/drm/i915/intel_ringbuffer.c |   51 ++++++++++++++++---------------
>   drivers/gpu/drm/i915/intel_ringbuffer.h |    1 +
>   4 files changed, 37 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5dc37f0..4a98399 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -154,11 +154,10 @@ void i915_kernel_lost_context(struct drm_device *dev)
>   	if (drm_core_check_feature(dev, DRIVER_MODESET))
>   		return;
>   
> +	ringbuf->last_retired_head = -1;
>   	ringbuf->head = I915_READ_HEAD(ring) & HEAD_ADDR;
>   	ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR;
> -	ringbuf->space = ringbuf->head - (ringbuf->tail + I915_RING_FREE_SPACE);
> -	if (ringbuf->space < 0)
> -		ringbuf->space += ringbuf->size;
> +	intel_ring_update_space(ringbuf);
>   
>   	if (!dev->primary->master)
>   		return;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index ad31373..c9a5227 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -825,14 +825,8 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
>   	u32 seqno = 0;
>   	int ret;
>   
> -	if (ringbuf->last_retired_head != -1) {
> -		ringbuf->head = ringbuf->last_retired_head;
> -		ringbuf->last_retired_head = -1;
> -
> -		ringbuf->space = intel_ring_space(ringbuf);
> -		if (ringbuf->space >= bytes)
> -			return 0;
> -	}
> +	if (intel_ring_space(ringbuf) >= bytes)
> +		return 0;
>   
>   	list_for_each_entry(request, &ring->request_list, list) {
>   		/*
> @@ -860,11 +854,8 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
>   		return ret;
>   
>   	i915_gem_retire_requests_ring(ring);
> -	ringbuf->head = ringbuf->last_retired_head;
> -	ringbuf->last_retired_head = -1;
>   
> -	ringbuf->space = intel_ring_space(ringbuf);
> -	return 0;
> +	return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;
>   }
>   
>   static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
> @@ -890,12 +881,10 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
>   	 * case by choosing an insanely large timeout. */
>   	end = jiffies + 60 * HZ;
>   
> +	ret = 0;
>   	do {
> -		ringbuf->space = intel_ring_space(ringbuf);
> -		if (ringbuf->space >= bytes) {
> -			ret = 0;
> +		if (intel_ring_space(ringbuf) >= bytes)
>   			break;
> -		}
>   
>   		msleep(1);
>   
> @@ -936,7 +925,7 @@ static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf)
>   		iowrite32(MI_NOOP, virt++);
>   
>   	ringbuf->tail = 0;
> -	ringbuf->space = intel_ring_space(ringbuf);
> +	intel_ring_update_space(ringbuf);
>   
>   	return 0;
>   }
> @@ -1777,8 +1766,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
>   	ringbuf->effective_size = ringbuf->size;
>   	ringbuf->head = 0;
>   	ringbuf->tail = 0;
> -	ringbuf->space = ringbuf->size;
>   	ringbuf->last_retired_head = -1;
> +	intel_ring_update_space(ringbuf);
>   
>   	/* TODO: For now we put this in the mappable region so that we can reuse
>   	 * the existing ringbuffer code which ioremaps it. When we start
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ae09258..a08ae65 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -52,16 +52,27 @@ intel_ring_initialized(struct intel_engine_cs *ring)
>   
>   int __intel_ring_space(int head, int tail, int size)
>   {
> -	int space = head - (tail + I915_RING_FREE_SPACE);
> -	if (space < 0)
> +	int space = head - tail;
> +	if (space <= 0)
>   		space += size;
> -	return space;
> +	return space - I915_RING_FREE_SPACE;
> +}
> +
> +void intel_ring_update_space(struct intel_ringbuffer *ringbuf)
> +{
> +	if (ringbuf->last_retired_head != -1) {
> +		ringbuf->head = ringbuf->last_retired_head;
> +		ringbuf->last_retired_head = -1;
> +	}
> +
> +	ringbuf->space = __intel_ring_space(ringbuf->head & HEAD_ADDR,
> +					    ringbuf->tail, ringbuf->size);
>   }
>   
>   int intel_ring_space(struct intel_ringbuffer *ringbuf)
>   {
> -	return __intel_ring_space(ringbuf->head & HEAD_ADDR,
> -				  ringbuf->tail, ringbuf->size);
> +	intel_ring_update_space(ringbuf);
> +	return ringbuf->space;
>   }
>   
>   bool intel_ring_stopped(struct intel_engine_cs *ring)
> @@ -592,10 +603,10 @@ static int init_ring_common(struct intel_engine_cs *ring)
>   	if (!drm_core_check_feature(ring->dev, DRIVER_MODESET))
>   		i915_kernel_lost_context(ring->dev);
>   	else {
> +		ringbuf->last_retired_head = -1;
>   		ringbuf->head = I915_READ_HEAD(ring);
>   		ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR;
> -		ringbuf->space = intel_ring_space(ringbuf);
> -		ringbuf->last_retired_head = -1;
> +		intel_ring_update_space(ringbuf);
>   	}
>   
>   	memset(&ring->hangcheck, 0, sizeof(ring->hangcheck));
> @@ -1880,14 +1891,8 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
>   	u32 seqno = 0;
>   	int ret;
>   
> -	if (ringbuf->last_retired_head != -1) {
> -		ringbuf->head = ringbuf->last_retired_head;
> -		ringbuf->last_retired_head = -1;
> -
> -		ringbuf->space = intel_ring_space(ringbuf);
> -		if (ringbuf->space >= n)
> -			return 0;
> -	}
> +	if (intel_ring_space(ringbuf) >= n)
> +		return 0;
>   
>   	list_for_each_entry(request, &ring->request_list, list) {
>   		if (__intel_ring_space(request->tail, ringbuf->tail,
> @@ -1905,10 +1910,7 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
>   		return ret;
>   
>   	i915_gem_retire_requests_ring(ring);
> -	ringbuf->head = ringbuf->last_retired_head;
> -	ringbuf->last_retired_head = -1;
>   
> -	ringbuf->space = intel_ring_space(ringbuf);
>   	return 0;
>   }
>   
> @@ -1934,14 +1936,14 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>   	 * case by choosing an insanely large timeout. */
>   	end = jiffies + 60 * HZ;
>   
> +	ret = 0;
>   	trace_i915_ring_wait_begin(ring);
>   	do {
> +		if (intel_ring_space(ringbuf) >= n)
> +			break;
>   		ringbuf->head = I915_READ_HEAD(ring);
> -		ringbuf->space = intel_ring_space(ringbuf);
> -		if (ringbuf->space >= n) {
> -			ret = 0;
> +		if (intel_ring_space(ringbuf) >= n)
>   			break;
> -		}
>   
>   		if (!drm_core_check_feature(dev, DRIVER_MODESET) &&
>   		    dev->primary->master) {
> @@ -1989,7 +1991,7 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
>   		iowrite32(MI_NOOP, virt++);
>   
>   	ringbuf->tail = 0;
> -	ringbuf->space = intel_ring_space(ringbuf);
> +	intel_ring_update_space(ringbuf);
>   
>   	return 0;
>   }
> @@ -2061,6 +2063,7 @@ int intel_ring_begin(struct intel_engine_cs *ring,
>   		     int num_dwords)
>   {
>   	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct intel_ringbuffer *ringbuf = ring->buffer;
>   	int ret;
>   
>   	ret = i915_gem_check_wedge(&dev_priv->gpu_error,
> @@ -2077,7 +2080,7 @@ int intel_ring_begin(struct intel_engine_cs *ring,
>   	if (ret)
>   		return ret;
>   
> -	ring->buffer->space -= num_dwords * sizeof(uint32_t);
> +	ringbuf->space -= num_dwords * sizeof(uint32_t);
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index aab2e2f..24a5723 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -404,6 +404,7 @@ static inline void intel_ring_advance(struct intel_engine_cs *ring)
>   	ringbuf->tail &= ringbuf->size - 1;
>   }
>   int __intel_ring_space(int head, int tail, int size);
> +void intel_ring_update_space(struct intel_ringbuffer *ringbuf);
>   int intel_ring_space(struct intel_ringbuffer *ringbuf);
>   bool intel_ring_stopped(struct intel_engine_cs *ring);
>   void __intel_ring_advance(struct intel_engine_cs *ring);

Looks fine to me
Reviewed-by: Deepak S<deepak.s@linux.intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx