[Intel-gfx] [PATCH 2/2] drm/i915: Flush request queue when waiting for ring space

[Volkin, Bradley D]

On Mon, May 05, 2014 at 01:07:33AM -0700, Chris Wilson wrote:
> During the review of
> 
> commit 1f70999f9052f5a1b0ce1a55aff3808f2ec9fe42
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Mon Jan 27 22:43:07 2014 +0000
> 
>     drm/i915: Prevent recursion by retiring requests when the ring is full
> 
> Ville raised the point that our interaction with request->tail was
> likely to foul up other uses elsewhere (such as hang check comparing
> ACTHD against requests).
> 
> However, we also need to restore the implicit retire requests that certain
> test cases depend upon (e.g. igt/gem_exec_lut_handle), this raises the
> spectre that the ppgtt will randomly call i915_gpu_idle() and recurse
> back into intel_ring_begin().
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78023

There's one minor cleanup suggested below.

Overall I think the code is better after the patch. I don't really like 
reintroducing the potential recursion, but I suppose that's a separate
issue. With the cleanup...

Reviewed-by: Brad Volkin <bradley.d.volkin at intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/i915_gem.c         |  3 +--
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 36 ++++++++++++---------------------
>  3 files changed, 15 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5f4f631aecef..69a4e161ff37 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2155,6 +2155,7 @@ struct drm_i915_gem_request *
>  i915_gem_find_active_request(struct intel_ring_buffer *ring);
>  
>  bool i915_gem_retire_requests(struct drm_device *dev);
> +void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
>  int __must_check i915_gem_check_wedge(struct i915_gpu_error *error,
>  				      bool interruptible);
>  static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index dae51c3701f3..2f2a668c01ac 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -64,7 +64,6 @@ static unsigned long i915_gem_inactive_scan(struct shrinker *shrinker,
>  static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long target);
>  static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
>  static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
> -static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
>  
>  static bool cpu_cache_is_coherent(struct drm_device *dev,
>  				  enum i915_cache_level level)
> @@ -2448,7 +2447,7 @@ void i915_gem_reset(struct drm_device *dev)
>  /**
>   * This function clears the request list as sequence numbers are passed.
>   */
> -static void
> +void
>  i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  {
>  	uint32_t seqno;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d6b7b884adf9..9dce15cbce73 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -40,14 +40,19 @@
>   */
>  #define CACHELINE_BYTES 64
>  
> -static inline int ring_space(struct intel_ring_buffer *ring)
> +static inline int __ring_space(int head, int tail, int size)
>  {
> -	int space = (ring->head & HEAD_ADDR) - (ring->tail + I915_RING_FREE_SPACE);
> +	int space = head - (tail + I915_RING_FREE_SPACE);
>  	if (space < 0)
> -		space += ring->size;
> +		space += size;
>  	return space;
>  }
>  
> +static inline int ring_space(struct intel_ring_buffer *ring)
> +{
> +	return __ring_space(ring->head & HEAD_ADDR, ring->tail, ring->size);
> +}
> +
>  static bool intel_ring_stopped(struct intel_ring_buffer *ring)
>  {
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> @@ -1495,26 +1500,11 @@ static int intel_ring_wait_request(struct intel_ring_buffer *ring, int n)
>  	}
>  
>  	list_for_each_entry(request, &ring->request_list, list) {
> -		int space;
> -
> -		if (request->tail == -1)
> -			continue;
> -
> -		space = request->tail - (ring->tail + I915_RING_FREE_SPACE);
> -		if (space < 0)
> -			space += ring->size;
> -		if (space >= n) {
> +		if (__ring_space(request->tail, ring->tail, ring->size) >= n) {
>  			seqno = request->seqno;
>  			tail = request->tail;

We don't actually use the local 'tail' anymore.

>  			break;
>  		}
> -
> -		/* Consume this request in case we need more space than
> -		 * is available and so need to prevent a race between
> -		 * updating last_retired_head and direct reads of
> -		 * I915_RING_HEAD. It also provides a nice sanity check.
> -		 */
> -		request->tail = -1;
>  	}
>  
>  	if (seqno == 0)
> @@ -1524,11 +1514,11 @@ static int intel_ring_wait_request(struct intel_ring_buffer *ring, int n)
>  	if (ret)
>  		return ret;
>  
> -	ring->head = tail;
> -	ring->space = ring_space(ring);
> -	if (WARN_ON(ring->space < n))
> -		return -ENOSPC;
> +	i915_gem_retire_requests_ring(ring);
> +	ring->head = ring->last_retired_head;
> +	ring->last_retired_head = -1;
>  
> +	ring->space = ring_space(ring);
>  	return 0;
>  }
>  
> -- 
> 2.0.0.rc0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx