[PATCH] drm/ivb: Flush outstanding requests before allocating new seqno

[PATCH] drm/ivb: Flush outstanding requests before allocating new seqno

[Chris Wilson]

In very rare cases (such as a memory failure stress test) it is possible
to fill the entire ring without emitting a request. Under this
circumstance, the outstanding request is flushed and waited upon. After
space on the ring is cleared, we return to emitting the new command -
except that we just cleared the seqno allocated for this operation and
trigger the sanity check that a request is only ever emitted with a
valid seqno. The fix is to rearrange the code to make sure the
allocation of the seqno for this operation is after any required flushes
of outstanding operations.

The bug exists since the preallocation was introduced in
commit 9d7730914f4cd496e356acfab95b41075aa8eae8
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Nov 27 16:22:52 2012 +0000

    drm/i915: Preallocate next seqno before touching the ring

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 5744841669e4..6bb914e49bf0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1610,8 +1610,8 @@ intel_ring_alloc_seqno(struct intel_ring_buffer *ring)
 	return i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
 }
 
-static int __intel_ring_begin(struct intel_ring_buffer *ring,
-			      int bytes)
+static int __intel_ring_prepare(struct intel_ring_buffer *ring,
+				int bytes)
 {
 	int ret;
 
@@ -1627,7 +1627,6 @@ static int __intel_ring_begin(struct intel_ring_buffer *ring,
 			return ret;
 	}
 
-	ring->space -= bytes;
 	return 0;
 }
 
@@ -1642,12 +1641,17 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 	if (ret)
 		return ret;
 
+	ret = __intel_ring_prepare(ring, num_dwords * sizeof(uint32_t));
+	if (ret)
+		return ret;
+
 	/* Preallocate the olr before touching the ring */
 	ret = intel_ring_alloc_seqno(ring);
 	if (ret)
 		return ret;
 
-	return __intel_ring_begin(ring, num_dwords * sizeof(uint32_t));
+	ring->space -= num_dwords * sizeof(uint32_t);
+	return 0;
 }
 
 void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno)
-- 
1.8.5.2

Re: [Intel-gfx] [PATCH] drm/ivb: Flush outstanding requests before allocating new seqno

[Jani Nikula]

On Thu, 02 Jan 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> In very rare cases (such as a memory failure stress test) it is possible
> to fill the entire ring without emitting a request. Under this
> circumstance, the outstanding request is flushed and waited upon. After
> space on the ring is cleared, we return to emitting the new command -
> except that we just cleared the seqno allocated for this operation and
> trigger the sanity check that a request is only ever emitted with a
> valid seqno. The fix is to rearrange the code to make sure the
> allocation of the seqno for this operation is after any required flushes
> of outstanding operations.
>
> The bug exists since the preallocation was introduced in
> commit 9d7730914f4cd496e356acfab95b41075aa8eae8
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Nov 27 16:22:52 2012 +0000
>
>     drm/i915: Preallocate next seqno before touching the ring
>

I can't claim to be an expert in the area, but the explanation and code
make sense.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 5744841669e4..6bb914e49bf0 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1610,8 +1610,8 @@ intel_ring_alloc_seqno(struct intel_ring_buffer *ring)
>  	return i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_seqno);
>  }
>  
> -static int __intel_ring_begin(struct intel_ring_buffer *ring,
> -			      int bytes)
> +static int __intel_ring_prepare(struct intel_ring_buffer *ring,
> +				int bytes)
>  {
>  	int ret;
>  
> @@ -1627,7 +1627,6 @@ static int __intel_ring_begin(struct intel_ring_buffer *ring,
>  			return ret;
>  	}
>  
> -	ring->space -= bytes;
>  	return 0;
>  }
>  
> @@ -1642,12 +1641,17 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
>  	if (ret)
>  		return ret;
>  
> +	ret = __intel_ring_prepare(ring, num_dwords * sizeof(uint32_t));
> +	if (ret)
> +		return ret;
> +
>  	/* Preallocate the olr before touching the ring */
>  	ret = intel_ring_alloc_seqno(ring);
>  	if (ret)
>  		return ret;
>  
> -	return __intel_ring_begin(ring, num_dwords * sizeof(uint32_t));
> +	ring->space -= num_dwords * sizeof(uint32_t);
> +	return 0;
>  }
>  
>  void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno)
> -- 
> 1.8.5.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center