[02/55] drm/i915: Reserve ring buffer space for i915_add_request() commands

Commit Message

John Harrison June 4, 2015, 12:06 p.m. UTC

From: John Harrison <[e-mail]>

It is a bad idea for i915_add_request() to fail. The work will already have been
send to the ring and will be processed, but there will not be any tracking or
management of that work.

The only way the add request call can fail is if it can't write its epilogue
commands to the ring (cache flushing, seqno updates, interrupt signalling). The
reasons for that are mostly down to running out of ring buffer space and the
problems associated with trying to get some more. This patch prevents that
situation from happening in the first place.

When a request is created, it marks sufficient space as reserved for the
epilogue commands. Thus guaranteeing that by the time the epilogue is written,
there will be plenty of space for it. Note that a ring_begin() call is required
to actually reserve the space (and do any potential waiting). However, that is
not currently done at request creation time. This is because the ring_begin()
code can allocate a request. Hence calling begin() from the request allocation
code would lead to infinite recursion! Later patches in this series remove the
need for begin() to do the allocate. At that point, it becomes safe for the
allocate to call begin() and really reserve the space.

Until then, there is a potential for insufficient space to be available at the
point of calling i915_add_request(). However, that would only be in the case
where the request was created and immediately submitted without ever calling
ring_begin() and adding any work to that request. Which should never happen. And
even if it does, and if that request happens to fall down the tiny window of
opportunity for failing due to being out of ring space then does it really
matter because the request wasn't doing anything in the first place?

v2: Updated the 'reserved space too small' warning to include the offending
sizes. Added a 'cancel' operation to clean up when a request is abandoned. Added
re-initialisation of tracking state after a buffer wrap to keep the sanity
checks accurate.

v3: Incremented the reserved size to accommodate Ironlake (after finally
managing to run on an ILK system). Also fixed missing wrap code in LRC mode.

For: VIZ-5115
Signed-off-by: John Harrison <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h         |    1 +
 drivers/gpu/drm/i915/i915_gem.c         |   37 +++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c        |   18 ++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c |   68 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h |   25 ++++++++++++
 5 files changed, 147 insertions(+), 2 deletions(-)

Comments

Tomas Elf June 9, 2015, 4 p.m. UTC | #1

On 04/06/2015 13:06, [e-mail] wrote:
> From: John Harrison <[e-mail]>
>
> It is a bad idea for i915_add_request() to fail. The work will already have been
> send to the ring and will be processed, but there will not be any tracking or
> management of that work.
>
> The only way the add request call can fail is if it can't write its epilogue
> commands to the ring (cache flushing, seqno updates, interrupt signalling). The
> reasons for that are mostly down to running out of ring buffer space and the
> problems associated with trying to get some more. This patch prevents that
> situation from happening in the first place.
>
> When a request is created, it marks sufficient space as reserved for the
> epilogue commands. Thus guaranteeing that by the time the epilogue is written,
> there will be plenty of space for it. Note that a ring_begin() call is required
> to actually reserve the space (and do any potential waiting). However, that is
> not currently done at request creation time. This is because the ring_begin()
> code can allocate a request. Hence calling begin() from the request allocation
> code would lead to infinite recursion! Later patches in this series remove the
> need for begin() to do the allocate. At that point, it becomes safe for the
> allocate to call begin() and really reserve the space.
>
> Until then, there is a potential for insufficient space to be available at the
> point of calling i915_add_request(). However, that would only be in the case
> where the request was created and immediately submitted without ever calling
> ring_begin() and adding any work to that request. Which should never happen. And
> even if it does, and if that request happens to fall down the tiny window of
> opportunity for failing due to being out of ring space then does it really
> matter because the request wasn't doing anything in the first place?
>
> v2: Updated the 'reserved space too small' warning to include the offending
> sizes. Added a 'cancel' operation to clean up when a request is abandoned. Added
> re-initialisation of tracking state after a buffer wrap to keep the sanity
> checks accurate.
>
> v3: Incremented the reserved size to accommodate Ironlake (after finally
> managing to run on an ILK system). Also fixed missing wrap code in LRC mode.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <[e-mail]>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |    1 +
>   drivers/gpu/drm/i915/i915_gem.c         |   37 +++++++++++++++++
>   drivers/gpu/drm/i915/intel_lrc.c        |   18 ++++++++
>   drivers/gpu/drm/i915/intel_ringbuffer.c |   68 ++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/i915/intel_ringbuffer.h |   25 ++++++++++++
>   5 files changed, 147 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e9d76f3..44dee31 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2187,6 +2187,7 @@ struct drm_i915_gem_request {
>
>   int i915_gem_request_alloc(struct intel_engine_cs *ring,
>   			   struct intel_context *ctx);
> +void i915_gem_request_cancel(struct drm_i915_gem_request *req);
>   void i915_gem_request_free(struct kref *req_ref);
>
>   static inline uint32_t
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 78f6a89..516e9b7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2495,6 +2495,13 @@ int __i915_add_request(struct intel_engine_cs *ring,
>   	} else
>   		ringbuf = ring->buffer;
>
> +	/*
> +	 * To ensure that this call will not fail, space for its emissions
> +	 * should already have been reserved in the ring buffer. Let the ring
> +	 * know that it is time to use that space up.
> +	 */
> +	intel_ring_reserved_space_use(ringbuf);
> +
>   	request_start = intel_ring_get_tail(ringbuf);
>   	/*
>   	 * Emit any outstanding flushes - execbuf can fail to emit the flush
> @@ -2577,6 +2584,9 @@ int __i915_add_request(struct intel_engine_cs *ring,
>   			   round_jiffies_up_relative(HZ));
>   	intel_mark_busy(dev_priv->dev);
>
> +	/* Sanity check that the reserved size was large enough. */
> +	intel_ring_reserved_space_end(ringbuf);
> +
>   	return 0;
>   }
>
> @@ -2676,6 +2686,26 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
>   	if (ret)
>   		goto err;
>
> +	/*
> +	 * Reserve space in the ring buffer for all the commands required to
> +	 * eventually emit this request. This is to guarantee that the
> +	 * i915_add_request() call can't fail. Note that the reserve may need
> +	 * to be redone if the request is not actually submitted straight
> +	 * away, e.g. because a GPU scheduler has deferred it.
> +	 *
> +	 * Note further that this call merely notes the reserve request. A
> +	 * subsequent call to *_ring_begin() is required to actually ensure
> +	 * that the reservation is available. Without the begin, if the
> +	 * request creator immediately submitted the request without adding
> +	 * any commands to it then there might not actually be sufficient
> +	 * room for the submission commands. Unfortunately, the current
> +	 * *_ring_begin() implementations potentially call back here to
> +	 * i915_gem_request_alloc(). Thus calling _begin() here would lead to
> +	 * infinite recursion! Until that back call path is removed, it is
> +	 * necessary to do a manual _begin() outside.
> +	 */
> +	intel_ring_reserved_space_reserve(req->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
> +
>   	ring->outstanding_lazy_request = req;
>   	return 0;
>
> @@ -2684,6 +2714,13 @@ err:
>   	return ret;
>   }
>
> +void i915_gem_request_cancel(struct drm_i915_gem_request *req)
> +{
> +	intel_ring_reserved_space_cancel(req->ringbuf);
> +
> +	i915_gem_request_unreference(req);
> +}
> +
>   struct drm_i915_gem_request *
>   i915_gem_find_active_request(struct intel_engine_cs *ring)
>   {
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 6a5ed07..42a756d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -687,6 +687,9 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
>   	unsigned space;
>   	int ret;
>
> +	/* The whole point of reserving space is to not wait! */
> +	WARN_ON(ringbuf->reserved_in_use);
> +
>   	if (intel_ring_space(ringbuf) >= bytes)
>   		return 0;
>
> @@ -747,6 +750,9 @@ static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf,
>   	uint32_t __iomem *virt;
>   	int rem = ringbuf->size - ringbuf->tail;
>
> +	/* Can't wrap if space has already been reserved! */
> +	WARN_ON(ringbuf->reserved_in_use);
> +
>   	if (ringbuf->space < rem) {
>   		int ret = logical_ring_wait_for_space(ringbuf, ctx, rem);
>
> @@ -770,10 +776,22 @@ static int logical_ring_prepare(struct intel_ringbuffer *ringbuf,
>   {
>   	int ret;
>
> +	if (!ringbuf->reserved_in_use)
> +		bytes += ringbuf->reserved_size;

This line right here is the main integration point between the buffer 
reservation scheme and the existing infrastructure. Please point this 
out in a comment down together with the space reservation prototypes 
where you describe _where_ this reserved ring space size actually comes 
to matter in the end. Or include a comment here. Or both. It's kinda 
important to know where the reserved space ends up in the end and where 
it integrates.

> +
>   	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> +		WARN_ON(ringbuf->reserved_in_use);

This WARN_ON is already done in logical_ring_wrap_buffer. Unless there 
is a reason for a second warning please remove this one.

> +
>   		ret = logical_ring_wrap_buffer(ringbuf, ctx);
>   		if (unlikely(ret))
>   			return ret;
> +
> +		if(ringbuf->reserved_size) {
> +			uint32_t size = ringbuf->reserved_size;
> +
> +			intel_ring_reserved_space_cancel(ringbuf);
> +			intel_ring_reserved_space_reserve(ringbuf, size);
> +		}
>   	}
>
>   	if (unlikely(ringbuf->space < bytes)) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d934f85..74c2222 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2103,6 +2103,9 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>   	unsigned space;
>   	int ret;
>
> +	/* The whole point of reserving space is to not wait! */
> +	WARN_ON(ringbuf->reserved_in_use);
> +
>   	if (intel_ring_space(ringbuf) >= n)
>   		return 0;
>
> @@ -2130,6 +2133,9 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
>   	struct intel_ringbuffer *ringbuf = ring->buffer;
>   	int rem = ringbuf->size - ringbuf->tail;
>
> +	/* Can't wrap if space has already been reserved! */
> +	WARN_ON(ringbuf->reserved_in_use);
> +
>   	if (ringbuf->space < rem) {
>   		int ret = ring_wait_for_space(ring, rem);
>   		if (ret)
> @@ -2180,16 +2186,74 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>   	return 0;
>   }
>
> -static int __intel_ring_prepare(struct intel_engine_cs *ring,
> -				int bytes)
> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size)
> +{
> +	/* NB: Until request management is fully tidied up and the OLR is
> +	 * removed, there are too many ways for get false hits on this
> +	 * anti-recursion check! */
> +	/*WARN_ON(ringbuf->reserved_size);*/
> +	WARN_ON(ringbuf->reserved_in_use);
> +
> +	ringbuf->reserved_size = size;
> +
> +	/*
> +	 * Really need to call _begin() here but that currently leads to
> +	 * recursion problems! This will be fixed later but for now just
> +	 * return and hope for the best. Note that there is only a real
> +	 * problem if the create of the request never actually calls _begin()
> +	 * but if they are not submitting any work then why did they create
> +	 * the request in the first place?
> +	 */
> +}
> +
> +void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf)
> +{
> +	WARN_ON(ringbuf->reserved_in_use);
> +
> +	ringbuf->reserved_size   = 0;
> +	ringbuf->reserved_in_use = false;
> +}
> +
> +void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf)
> +{
> +	WARN_ON(ringbuf->reserved_in_use);
> +
> +	ringbuf->reserved_in_use = true;
> +	ringbuf->reserved_tail   = ringbuf->tail;
> +}
> +
> +void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
> +{
> +	WARN_ON(!ringbuf->reserved_in_use);
> +	WARN(ringbuf->tail > ringbuf->reserved_tail + ringbuf->reserved_size,
> +	     "request reserved size too small: %d vs %d!\n",
> +	     ringbuf->tail - ringbuf->reserved_tail, ringbuf->reserved_size);
> +
> +	ringbuf->reserved_size   = 0;
> +	ringbuf->reserved_in_use = false;
> +}
> +
> +static int __intel_ring_prepare(struct intel_engine_cs *ring, int bytes)
>   {
>   	struct intel_ringbuffer *ringbuf = ring->buffer;
>   	int ret;
>
> +	if (!ringbuf->reserved_in_use)
> +		bytes += ringbuf->reserved_size;
> +
>   	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> +		WARN_ON(ringbuf->reserved_in_use);
> +

This WARN_ON is already done in intel_wrap_ring_buffer. Unless there is 
a reason for a second warning please remove this one.

Thanks,
Tomas

>   		ret = intel_wrap_ring_buffer(ring);
>   		if (unlikely(ret))
>   			return ret;
> +
> +		if(ringbuf->reserved_size) {
> +			uint32_t size = ringbuf->reserved_size;
> +
> +			intel_ring_reserved_space_cancel(ringbuf);
> +			intel_ring_reserved_space_reserve(ringbuf, size);
> +		}
>   	}
>
>   	if (unlikely(ringbuf->space < bytes)) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 39f6dfc..bf2ac28 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -105,6 +105,9 @@ struct intel_ringbuffer {
>   	int space;
>   	int size;
>   	int effective_size;
> +	int reserved_size;
> +	int reserved_tail;
> +	bool reserved_in_use;
>
>   	/** We track the position of the requests in the ring buffer, and
>   	 * when each is retired we increment last_retired_head as the GPU
> @@ -450,4 +453,26 @@ intel_ring_get_request(struct intel_engine_cs *ring)
>   	return ring->outstanding_lazy_request;
>   }
>
> +/*
> + * Arbitrary size for largest possible 'add request' sequence. The code paths
> + * are complex and variable. Empirical measurement shows that the worst case
> + * is ILK at 136 words. Reserving too much is better than reserving too little
> + * as that allows for corner cases that might have been missed. So the figure
> + * has been rounded up to 160 words.
> + */
> +#define MIN_SPACE_FOR_ADD_REQUEST	160
> +
> +/*
> + * Reserve space in the ring to guarantee that the i915_add_request() call
> + * will always have sufficient room to do its stuff. The request creation
> + * code calls this automatically.
> + */
> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size);
> +/* Cancel the reservation, e.g. because the request is being discarded. */
> +void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf);
> +/* Use the reserved space - for use by i915_add_request() only. */
> +void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf);
> +/* Finish with the reserved space - for use by i915_add_request() only. */
> +void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf);
> +
>   #endif /* _INTEL_RINGBUFFER_H_ */
>

Daniel Vetter June 17, 2015, 2:04 p.m. UTC | #2

On Thu, Jun 04, 2015 at 01:06:34PM +0100, [e-mail] wrote:
> From: John Harrison <[e-mail]>
> 
> It is a bad idea for i915_add_request() to fail. The work will already have been
> send to the ring and will be processed, but there will not be any tracking or
> management of that work.
> 
> The only way the add request call can fail is if it can't write its epilogue
> commands to the ring (cache flushing, seqno updates, interrupt signalling). The
> reasons for that are mostly down to running out of ring buffer space and the
> problems associated with trying to get some more. This patch prevents that
> situation from happening in the first place.
> 
> When a request is created, it marks sufficient space as reserved for the
> epilogue commands. Thus guaranteeing that by the time the epilogue is written,
> there will be plenty of space for it. Note that a ring_begin() call is required
> to actually reserve the space (and do any potential waiting). However, that is
> not currently done at request creation time. This is because the ring_begin()
> code can allocate a request. Hence calling begin() from the request allocation
> code would lead to infinite recursion! Later patches in this series remove the
> need for begin() to do the allocate. At that point, it becomes safe for the
> allocate to call begin() and really reserve the space.
> 
> Until then, there is a potential for insufficient space to be available at the
> point of calling i915_add_request(). However, that would only be in the case
> where the request was created and immediately submitted without ever calling
> ring_begin() and adding any work to that request. Which should never happen. And
> even if it does, and if that request happens to fall down the tiny window of
> opportunity for failing due to being out of ring space then does it really
> matter because the request wasn't doing anything in the first place?
> 
> v2: Updated the 'reserved space too small' warning to include the offending
> sizes. Added a 'cancel' operation to clean up when a request is abandoned. Added
> re-initialisation of tracking state after a buffer wrap to keep the sanity
> checks accurate.
> 
> v3: Incremented the reserved size to accommodate Ironlake (after finally
> managing to run on an ILK system). Also fixed missing wrap code in LRC mode.
> 
> For: VIZ-5115
> Signed-off-by: John Harrison <[e-mail]>

From the last review round there's still my question wrt the correctness
of the reservation overflow vs. wrapping outstanding:

http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/56575

Also when resending patches, especially after such a long delay please
leave some indication of what you've decided to do wrt review comments.
Either as a reply in the review discussion (preferred) or at least as an
update in the cover letter or per-patch changelog. Otherwise reviewers
need to reverse-engineer what you have or haven't done by diffing patches,
which is just not that efficient.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |    1 +
>  drivers/gpu/drm/i915/i915_gem.c         |   37 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.c        |   18 ++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   68 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_ringbuffer.h |   25 ++++++++++++
>  5 files changed, 147 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e9d76f3..44dee31 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2187,6 +2187,7 @@ struct drm_i915_gem_request {
>  
>  int i915_gem_request_alloc(struct intel_engine_cs *ring,
>  			   struct intel_context *ctx);
> +void i915_gem_request_cancel(struct drm_i915_gem_request *req);
>  void i915_gem_request_free(struct kref *req_ref);
>  
>  static inline uint32_t
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 78f6a89..516e9b7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2495,6 +2495,13 @@ int __i915_add_request(struct intel_engine_cs *ring,
>  	} else
>  		ringbuf = ring->buffer;
>  
> +	/*
> +	 * To ensure that this call will not fail, space for its emissions
> +	 * should already have been reserved in the ring buffer. Let the ring
> +	 * know that it is time to use that space up.
> +	 */
> +	intel_ring_reserved_space_use(ringbuf);
> +
>  	request_start = intel_ring_get_tail(ringbuf);
>  	/*
>  	 * Emit any outstanding flushes - execbuf can fail to emit the flush
> @@ -2577,6 +2584,9 @@ int __i915_add_request(struct intel_engine_cs *ring,
>  			   round_jiffies_up_relative(HZ));
>  	intel_mark_busy(dev_priv->dev);
>  
> +	/* Sanity check that the reserved size was large enough. */
> +	intel_ring_reserved_space_end(ringbuf);
> +
>  	return 0;
>  }
>  
> @@ -2676,6 +2686,26 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
>  	if (ret)
>  		goto err;
>  
> +	/*
> +	 * Reserve space in the ring buffer for all the commands required to
> +	 * eventually emit this request. This is to guarantee that the
> +	 * i915_add_request() call can't fail. Note that the reserve may need
> +	 * to be redone if the request is not actually submitted straight
> +	 * away, e.g. because a GPU scheduler has deferred it.
> +	 *
> +	 * Note further that this call merely notes the reserve request. A
> +	 * subsequent call to *_ring_begin() is required to actually ensure
> +	 * that the reservation is available. Without the begin, if the
> +	 * request creator immediately submitted the request without adding
> +	 * any commands to it then there might not actually be sufficient
> +	 * room for the submission commands. Unfortunately, the current
> +	 * *_ring_begin() implementations potentially call back here to
> +	 * i915_gem_request_alloc(). Thus calling _begin() here would lead to
> +	 * infinite recursion! Until that back call path is removed, it is
> +	 * necessary to do a manual _begin() outside.
> +	 */
> +	intel_ring_reserved_space_reserve(req->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
> +
>  	ring->outstanding_lazy_request = req;
>  	return 0;
>  
> @@ -2684,6 +2714,13 @@ err:
>  	return ret;
>  }
>  
> +void i915_gem_request_cancel(struct drm_i915_gem_request *req)
> +{
> +	intel_ring_reserved_space_cancel(req->ringbuf);
> +
> +	i915_gem_request_unreference(req);
> +}
> +
>  struct drm_i915_gem_request *
>  i915_gem_find_active_request(struct intel_engine_cs *ring)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 6a5ed07..42a756d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -687,6 +687,9 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
>  	unsigned space;
>  	int ret;
>  
> +	/* The whole point of reserving space is to not wait! */
> +	WARN_ON(ringbuf->reserved_in_use);
> +
>  	if (intel_ring_space(ringbuf) >= bytes)
>  		return 0;
>  
> @@ -747,6 +750,9 @@ static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf,
>  	uint32_t __iomem *virt;
>  	int rem = ringbuf->size - ringbuf->tail;
>  
> +	/* Can't wrap if space has already been reserved! */
> +	WARN_ON(ringbuf->reserved_in_use);
> +
>  	if (ringbuf->space < rem) {
>  		int ret = logical_ring_wait_for_space(ringbuf, ctx, rem);
>  
> @@ -770,10 +776,22 @@ static int logical_ring_prepare(struct intel_ringbuffer *ringbuf,
>  {
>  	int ret;
>  
> +	if (!ringbuf->reserved_in_use)
> +		bytes += ringbuf->reserved_size;
> +
>  	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> +		WARN_ON(ringbuf->reserved_in_use);
> +
>  		ret = logical_ring_wrap_buffer(ringbuf, ctx);
>  		if (unlikely(ret))
>  			return ret;
> +
> +		if(ringbuf->reserved_size) {
> +			uint32_t size = ringbuf->reserved_size;
> +
> +			intel_ring_reserved_space_cancel(ringbuf);
> +			intel_ring_reserved_space_reserve(ringbuf, size);
> +		}
>  	}
>  
>  	if (unlikely(ringbuf->space < bytes)) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d934f85..74c2222 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2103,6 +2103,9 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>  	unsigned space;
>  	int ret;
>  
> +	/* The whole point of reserving space is to not wait! */
> +	WARN_ON(ringbuf->reserved_in_use);
> +
>  	if (intel_ring_space(ringbuf) >= n)
>  		return 0;
>  
> @@ -2130,6 +2133,9 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
>  	struct intel_ringbuffer *ringbuf = ring->buffer;
>  	int rem = ringbuf->size - ringbuf->tail;
>  
> +	/* Can't wrap if space has already been reserved! */
> +	WARN_ON(ringbuf->reserved_in_use);
> +
>  	if (ringbuf->space < rem) {
>  		int ret = ring_wait_for_space(ring, rem);
>  		if (ret)
> @@ -2180,16 +2186,74 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>  	return 0;
>  }
>  
> -static int __intel_ring_prepare(struct intel_engine_cs *ring,
> -				int bytes)
> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size)
> +{
> +	/* NB: Until request management is fully tidied up and the OLR is
> +	 * removed, there are too many ways for get false hits on this
> +	 * anti-recursion check! */
> +	/*WARN_ON(ringbuf->reserved_size);*/
> +	WARN_ON(ringbuf->reserved_in_use);
> +
> +	ringbuf->reserved_size = size;
> +
> +	/*
> +	 * Really need to call _begin() here but that currently leads to
> +	 * recursion problems! This will be fixed later but for now just
> +	 * return and hope for the best. Note that there is only a real
> +	 * problem if the create of the request never actually calls _begin()
> +	 * but if they are not submitting any work then why did they create
> +	 * the request in the first place?
> +	 */
> +}
> +
> +void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf)
> +{
> +	WARN_ON(ringbuf->reserved_in_use);
> +
> +	ringbuf->reserved_size   = 0;
> +	ringbuf->reserved_in_use = false;
> +}
> +
> +void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf)
> +{
> +	WARN_ON(ringbuf->reserved_in_use);
> +
> +	ringbuf->reserved_in_use = true;
> +	ringbuf->reserved_tail   = ringbuf->tail;
> +}
> +
> +void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
> +{
> +	WARN_ON(!ringbuf->reserved_in_use);
> +	WARN(ringbuf->tail > ringbuf->reserved_tail + ringbuf->reserved_size,
> +	     "request reserved size too small: %d vs %d!\n",
> +	     ringbuf->tail - ringbuf->reserved_tail, ringbuf->reserved_size);
> +
> +	ringbuf->reserved_size   = 0;
> +	ringbuf->reserved_in_use = false;
> +}
> +
> +static int __intel_ring_prepare(struct intel_engine_cs *ring, int bytes)
>  {
>  	struct intel_ringbuffer *ringbuf = ring->buffer;
>  	int ret;
>  
> +	if (!ringbuf->reserved_in_use)
> +		bytes += ringbuf->reserved_size;
> +
>  	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
> +		WARN_ON(ringbuf->reserved_in_use);
> +
>  		ret = intel_wrap_ring_buffer(ring);
>  		if (unlikely(ret))
>  			return ret;
> +
> +		if(ringbuf->reserved_size) {
> +			uint32_t size = ringbuf->reserved_size;
> +
> +			intel_ring_reserved_space_cancel(ringbuf);
> +			intel_ring_reserved_space_reserve(ringbuf, size);
> +		}
>  	}
>  
>  	if (unlikely(ringbuf->space < bytes)) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 39f6dfc..bf2ac28 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -105,6 +105,9 @@ struct intel_ringbuffer {
>  	int space;
>  	int size;
>  	int effective_size;
> +	int reserved_size;
> +	int reserved_tail;
> +	bool reserved_in_use;
>  
>  	/** We track the position of the requests in the ring buffer, and
>  	 * when each is retired we increment last_retired_head as the GPU
> @@ -450,4 +453,26 @@ intel_ring_get_request(struct intel_engine_cs *ring)
>  	return ring->outstanding_lazy_request;
>  }
>  
> +/*
> + * Arbitrary size for largest possible 'add request' sequence. The code paths
> + * are complex and variable. Empirical measurement shows that the worst case
> + * is ILK at 136 words. Reserving too much is better than reserving too little
> + * as that allows for corner cases that might have been missed. So the figure
> + * has been rounded up to 160 words.
> + */
> +#define MIN_SPACE_FOR_ADD_REQUEST	160
> +
> +/*
> + * Reserve space in the ring to guarantee that the i915_add_request() call
> + * will always have sufficient room to do its stuff. The request creation
> + * code calls this automatically.
> + */
> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size);
> +/* Cancel the reservation, e.g. because the request is being discarded. */
> +void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf);
> +/* Use the reserved space - for use by i915_add_request() only. */
> +void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf);
> +/* Finish with the reserved space - for use by i915_add_request() only. */
> +void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf);
> +
>  #endif /* _INTEL_RINGBUFFER_H_ */
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

John Harrison June 18, 2015, 10:43 a.m. UTC | #3

On 17/06/2015 15:04, Daniel Vetter wrote:
> On Thu, Jun 04, 2015 at 01:06:34PM +0100, [e-mail] wrote:
>> From: John Harrison <[e-mail]>
>>
>> It is a bad idea for i915_add_request() to fail. The work will already have been
>> send to the ring and will be processed, but there will not be any tracking or
>> management of that work.
>>
>> The only way the add request call can fail is if it can't write its epilogue
>> commands to the ring (cache flushing, seqno updates, interrupt signalling). The
>> reasons for that are mostly down to running out of ring buffer space and the
>> problems associated with trying to get some more. This patch prevents that
>> situation from happening in the first place.
>>
>> When a request is created, it marks sufficient space as reserved for the
>> epilogue commands. Thus guaranteeing that by the time the epilogue is written,
>> there will be plenty of space for it. Note that a ring_begin() call is required
>> to actually reserve the space (and do any potential waiting). However, that is
>> not currently done at request creation time. This is because the ring_begin()
>> code can allocate a request. Hence calling begin() from the request allocation
>> code would lead to infinite recursion! Later patches in this series remove the
>> need for begin() to do the allocate. At that point, it becomes safe for the
>> allocate to call begin() and really reserve the space.
>>
>> Until then, there is a potential for insufficient space to be available at the
>> point of calling i915_add_request(). However, that would only be in the case
>> where the request was created and immediately submitted without ever calling
>> ring_begin() and adding any work to that request. Which should never happen. And
>> even if it does, and if that request happens to fall down the tiny window of
>> opportunity for failing due to being out of ring space then does it really
>> matter because the request wasn't doing anything in the first place?
>>
>> v2: Updated the 'reserved space too small' warning to include the offending
>> sizes. Added a 'cancel' operation to clean up when a request is abandoned. Added
>> re-initialisation of tracking state after a buffer wrap to keep the sanity
>> checks accurate.
>>
>> v3: Incremented the reserved size to accommodate Ironlake (after finally
>> managing to run on an ILK system). Also fixed missing wrap code in LRC mode.
>>
>> For: VIZ-5115
>> Signed-off-by: John Harrison <[e-mail]>
>  From the last review round there's still my question wrt the correctness
> of the reservation overflow vs. wrapping outstanding:
>
> http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/56575

v2 - 'added re-intialisation of tracking state after a buffer wrap to 
keep the sanity checks accurate'. Does that not address your issue with 
wrapping?


> Also when resending patches, especially after such a long delay please
> leave some indication of what you've decided to do wrt review comments.
> Either as a reply in the review discussion (preferred) or at least as an
> update in the cover letter or per-patch changelog. Otherwise reviewers
> need to reverse-engineer what you have or haven't done by diffing patches,
> which is just not that efficient.
> -Daniel
>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h         |    1 +
>>   drivers/gpu/drm/i915/i915_gem.c         |   37 +++++++++++++++++
>>   drivers/gpu/drm/i915/intel_lrc.c        |   18 ++++++++
>>   drivers/gpu/drm/i915/intel_ringbuffer.c |   68 ++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |   25 ++++++++++++
>>   5 files changed, 147 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index e9d76f3..44dee31 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2187,6 +2187,7 @@ struct drm_i915_gem_request {
>>   
>>   int i915_gem_request_alloc(struct intel_engine_cs *ring,
>>   			   struct intel_context *ctx);
>> +void i915_gem_request_cancel(struct drm_i915_gem_request *req);
>>   void i915_gem_request_free(struct kref *req_ref);
>>   
>>   static inline uint32_t
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 78f6a89..516e9b7 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2495,6 +2495,13 @@ int __i915_add_request(struct intel_engine_cs *ring,
>>   	} else
>>   		ringbuf = ring->buffer;
>>   
>> +	/*
>> +	 * To ensure that this call will not fail, space for its emissions
>> +	 * should already have been reserved in the ring buffer. Let the ring
>> +	 * know that it is time to use that space up.
>> +	 */
>> +	intel_ring_reserved_space_use(ringbuf);
>> +
>>   	request_start = intel_ring_get_tail(ringbuf);
>>   	/*
>>   	 * Emit any outstanding flushes - execbuf can fail to emit the flush
>> @@ -2577,6 +2584,9 @@ int __i915_add_request(struct intel_engine_cs *ring,
>>   			   round_jiffies_up_relative(HZ));
>>   	intel_mark_busy(dev_priv->dev);
>>   
>> +	/* Sanity check that the reserved size was large enough. */
>> +	intel_ring_reserved_space_end(ringbuf);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -2676,6 +2686,26 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
>>   	if (ret)
>>   		goto err;
>>   
>> +	/*
>> +	 * Reserve space in the ring buffer for all the commands required to
>> +	 * eventually emit this request. This is to guarantee that the
>> +	 * i915_add_request() call can't fail. Note that the reserve may need
>> +	 * to be redone if the request is not actually submitted straight
>> +	 * away, e.g. because a GPU scheduler has deferred it.
>> +	 *
>> +	 * Note further that this call merely notes the reserve request. A
>> +	 * subsequent call to *_ring_begin() is required to actually ensure
>> +	 * that the reservation is available. Without the begin, if the
>> +	 * request creator immediately submitted the request without adding
>> +	 * any commands to it then there might not actually be sufficient
>> +	 * room for the submission commands. Unfortunately, the current
>> +	 * *_ring_begin() implementations potentially call back here to
>> +	 * i915_gem_request_alloc(). Thus calling _begin() here would lead to
>> +	 * infinite recursion! Until that back call path is removed, it is
>> +	 * necessary to do a manual _begin() outside.
>> +	 */
>> +	intel_ring_reserved_space_reserve(req->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
>> +
>>   	ring->outstanding_lazy_request = req;
>>   	return 0;
>>   
>> @@ -2684,6 +2714,13 @@ err:
>>   	return ret;
>>   }
>>   
>> +void i915_gem_request_cancel(struct drm_i915_gem_request *req)
>> +{
>> +	intel_ring_reserved_space_cancel(req->ringbuf);
>> +
>> +	i915_gem_request_unreference(req);
>> +}
>> +
>>   struct drm_i915_gem_request *
>>   i915_gem_find_active_request(struct intel_engine_cs *ring)
>>   {
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 6a5ed07..42a756d 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -687,6 +687,9 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
>>   	unsigned space;
>>   	int ret;
>>   
>> +	/* The whole point of reserving space is to not wait! */
>> +	WARN_ON(ringbuf->reserved_in_use);
>> +
>>   	if (intel_ring_space(ringbuf) >= bytes)
>>   		return 0;
>>   
>> @@ -747,6 +750,9 @@ static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf,
>>   	uint32_t __iomem *virt;
>>   	int rem = ringbuf->size - ringbuf->tail;
>>   
>> +	/* Can't wrap if space has already been reserved! */
>> +	WARN_ON(ringbuf->reserved_in_use);
>> +
>>   	if (ringbuf->space < rem) {
>>   		int ret = logical_ring_wait_for_space(ringbuf, ctx, rem);
>>   
>> @@ -770,10 +776,22 @@ static int logical_ring_prepare(struct intel_ringbuffer *ringbuf,
>>   {
>>   	int ret;
>>   
>> +	if (!ringbuf->reserved_in_use)
>> +		bytes += ringbuf->reserved_size;
>> +
>>   	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
>> +		WARN_ON(ringbuf->reserved_in_use);
>> +
>>   		ret = logical_ring_wrap_buffer(ringbuf, ctx);
>>   		if (unlikely(ret))
>>   			return ret;
>> +
>> +		if(ringbuf->reserved_size) {
>> +			uint32_t size = ringbuf->reserved_size;
>> +
>> +			intel_ring_reserved_space_cancel(ringbuf);
>> +			intel_ring_reserved_space_reserve(ringbuf, size);
>> +		}
>>   	}
>>   
>>   	if (unlikely(ringbuf->space < bytes)) {
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index d934f85..74c2222 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -2103,6 +2103,9 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>>   	unsigned space;
>>   	int ret;
>>   
>> +	/* The whole point of reserving space is to not wait! */
>> +	WARN_ON(ringbuf->reserved_in_use);
>> +
>>   	if (intel_ring_space(ringbuf) >= n)
>>   		return 0;
>>   
>> @@ -2130,6 +2133,9 @@ static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
>>   	struct intel_ringbuffer *ringbuf = ring->buffer;
>>   	int rem = ringbuf->size - ringbuf->tail;
>>   
>> +	/* Can't wrap if space has already been reserved! */
>> +	WARN_ON(ringbuf->reserved_in_use);
>> +
>>   	if (ringbuf->space < rem) {
>>   		int ret = ring_wait_for_space(ring, rem);
>>   		if (ret)
>> @@ -2180,16 +2186,74 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>>   	return 0;
>>   }
>>   
>> -static int __intel_ring_prepare(struct intel_engine_cs *ring,
>> -				int bytes)
>> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size)
>> +{
>> +	/* NB: Until request management is fully tidied up and the OLR is
>> +	 * removed, there are too many ways for get false hits on this
>> +	 * anti-recursion check! */
>> +	/*WARN_ON(ringbuf->reserved_size);*/
>> +	WARN_ON(ringbuf->reserved_in_use);
>> +
>> +	ringbuf->reserved_size = size;
>> +
>> +	/*
>> +	 * Really need to call _begin() here but that currently leads to
>> +	 * recursion problems! This will be fixed later but for now just
>> +	 * return and hope for the best. Note that there is only a real
>> +	 * problem if the create of the request never actually calls _begin()
>> +	 * but if they are not submitting any work then why did they create
>> +	 * the request in the first place?
>> +	 */
>> +}
>> +
>> +void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf)
>> +{
>> +	WARN_ON(ringbuf->reserved_in_use);
>> +
>> +	ringbuf->reserved_size   = 0;
>> +	ringbuf->reserved_in_use = false;
>> +}
>> +
>> +void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf)
>> +{
>> +	WARN_ON(ringbuf->reserved_in_use);
>> +
>> +	ringbuf->reserved_in_use = true;
>> +	ringbuf->reserved_tail   = ringbuf->tail;
>> +}
>> +
>> +void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
>> +{
>> +	WARN_ON(!ringbuf->reserved_in_use);
>> +	WARN(ringbuf->tail > ringbuf->reserved_tail + ringbuf->reserved_size,
>> +	     "request reserved size too small: %d vs %d!\n",
>> +	     ringbuf->tail - ringbuf->reserved_tail, ringbuf->reserved_size);
>> +
>> +	ringbuf->reserved_size   = 0;
>> +	ringbuf->reserved_in_use = false;
>> +}
>> +
>> +static int __intel_ring_prepare(struct intel_engine_cs *ring, int bytes)
>>   {
>>   	struct intel_ringbuffer *ringbuf = ring->buffer;
>>   	int ret;
>>   
>> +	if (!ringbuf->reserved_in_use)
>> +		bytes += ringbuf->reserved_size;
>> +
>>   	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
>> +		WARN_ON(ringbuf->reserved_in_use);
>> +
>>   		ret = intel_wrap_ring_buffer(ring);
>>   		if (unlikely(ret))
>>   			return ret;
>> +
>> +		if(ringbuf->reserved_size) {
>> +			uint32_t size = ringbuf->reserved_size;
>> +
>> +			intel_ring_reserved_space_cancel(ringbuf);
>> +			intel_ring_reserved_space_reserve(ringbuf, size);
>> +		}
>>   	}
>>   
>>   	if (unlikely(ringbuf->space < bytes)) {
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 39f6dfc..bf2ac28 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -105,6 +105,9 @@ struct intel_ringbuffer {
>>   	int space;
>>   	int size;
>>   	int effective_size;
>> +	int reserved_size;
>> +	int reserved_tail;
>> +	bool reserved_in_use;
>>   
>>   	/** We track the position of the requests in the ring buffer, and
>>   	 * when each is retired we increment last_retired_head as the GPU
>> @@ -450,4 +453,26 @@ intel_ring_get_request(struct intel_engine_cs *ring)
>>   	return ring->outstanding_lazy_request;
>>   }
>>   
>> +/*
>> + * Arbitrary size for largest possible 'add request' sequence. The code paths
>> + * are complex and variable. Empirical measurement shows that the worst case
>> + * is ILK at 136 words. Reserving too much is better than reserving too little
>> + * as that allows for corner cases that might have been missed. So the figure
>> + * has been rounded up to 160 words.
>> + */
>> +#define MIN_SPACE_FOR_ADD_REQUEST	160
>> +
>> +/*
>> + * Reserve space in the ring to guarantee that the i915_add_request() call
>> + * will always have sufficient room to do its stuff. The request creation
>> + * code calls this automatically.
>> + */
>> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size);
>> +/* Cancel the reservation, e.g. because the request is being discarded. */
>> +void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf);
>> +/* Use the reserved space - for use by i915_add_request() only. */
>> +void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf);
>> +/* Finish with the reserved space - for use by i915_add_request() only. */
>> +void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf);
>> +
>>   #endif /* _INTEL_RINGBUFFER_H_ */
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e9d76f3..44dee31 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2187,6 +2187,7 @@  struct drm_i915_gem_request {
 
 int i915_gem_request_alloc(struct intel_engine_cs *ring,
 			   struct intel_context *ctx);
+void i915_gem_request_cancel(struct drm_i915_gem_request *req);
 void i915_gem_request_free(struct kref *req_ref);
 
 static inline uint32_t
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 78f6a89..516e9b7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2495,6 +2495,13 @@  int __i915_add_request(struct intel_engine_cs *ring,
 	} else
 		ringbuf = ring->buffer;
 
+	/*
+	 * To ensure that this call will not fail, space for its emissions
+	 * should already have been reserved in the ring buffer. Let the ring
+	 * know that it is time to use that space up.
+	 */
+	intel_ring_reserved_space_use(ringbuf);
+
 	request_start = intel_ring_get_tail(ringbuf);
 	/*
 	 * Emit any outstanding flushes - execbuf can fail to emit the flush
@@ -2577,6 +2584,9 @@  int __i915_add_request(struct intel_engine_cs *ring,
 			   round_jiffies_up_relative(HZ));
 	intel_mark_busy(dev_priv->dev);
 
+	/* Sanity check that the reserved size was large enough. */
+	intel_ring_reserved_space_end(ringbuf);
+
 	return 0;
 }
 
@@ -2676,6 +2686,26 @@  int i915_gem_request_alloc(struct intel_engine_cs *ring,
 	if (ret)
 		goto err;
 
+	/*
+	 * Reserve space in the ring buffer for all the commands required to
+	 * eventually emit this request. This is to guarantee that the
+	 * i915_add_request() call can't fail. Note that the reserve may need
+	 * to be redone if the request is not actually submitted straight
+	 * away, e.g. because a GPU scheduler has deferred it.
+	 *
+	 * Note further that this call merely notes the reserve request. A
+	 * subsequent call to *_ring_begin() is required to actually ensure
+	 * that the reservation is available. Without the begin, if the
+	 * request creator immediately submitted the request without adding
+	 * any commands to it then there might not actually be sufficient
+	 * room for the submission commands. Unfortunately, the current
+	 * *_ring_begin() implementations potentially call back here to
+	 * i915_gem_request_alloc(). Thus calling _begin() here would lead to
+	 * infinite recursion! Until that back call path is removed, it is
+	 * necessary to do a manual _begin() outside.
+	 */
+	intel_ring_reserved_space_reserve(req->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
+
 	ring->outstanding_lazy_request = req;
 	return 0;
 
@@ -2684,6 +2714,13 @@  err:
 	return ret;
 }
 
+void i915_gem_request_cancel(struct drm_i915_gem_request *req)
+{
+	intel_ring_reserved_space_cancel(req->ringbuf);
+
+	i915_gem_request_unreference(req);
+}
+
 struct drm_i915_gem_request *
 i915_gem_find_active_request(struct intel_engine_cs *ring)
 {
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6a5ed07..42a756d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -687,6 +687,9 @@  static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
 	unsigned space;
 	int ret;
 
+	/* The whole point of reserving space is to not wait! */
+	WARN_ON(ringbuf->reserved_in_use);
+
 	if (intel_ring_space(ringbuf) >= bytes)
 		return 0;
 
@@ -747,6 +750,9 @@  static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf,
 	uint32_t __iomem *virt;
 	int rem = ringbuf->size - ringbuf->tail;
 
+	/* Can't wrap if space has already been reserved! */
+	WARN_ON(ringbuf->reserved_in_use);
+
 	if (ringbuf->space < rem) {
 		int ret = logical_ring_wait_for_space(ringbuf, ctx, rem);
 
@@ -770,10 +776,22 @@  static int logical_ring_prepare(struct intel_ringbuffer *ringbuf,
 {
 	int ret;
 
+	if (!ringbuf->reserved_in_use)
+		bytes += ringbuf->reserved_size;
+
 	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
+		WARN_ON(ringbuf->reserved_in_use);
+
 		ret = logical_ring_wrap_buffer(ringbuf, ctx);
 		if (unlikely(ret))
 			return ret;
+
+		if(ringbuf->reserved_size) {
+			uint32_t size = ringbuf->reserved_size;
+
+			intel_ring_reserved_space_cancel(ringbuf);
+			intel_ring_reserved_space_reserve(ringbuf, size);
+		}
 	}
 
 	if (unlikely(ringbuf->space < bytes)) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index d934f85..74c2222 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2103,6 +2103,9 @@  static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
 	unsigned space;
 	int ret;
 
+	/* The whole point of reserving space is to not wait! */
+	WARN_ON(ringbuf->reserved_in_use);
+
 	if (intel_ring_space(ringbuf) >= n)
 		return 0;
 
@@ -2130,6 +2133,9 @@  static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
 	struct intel_ringbuffer *ringbuf = ring->buffer;
 	int rem = ringbuf->size - ringbuf->tail;
 
+	/* Can't wrap if space has already been reserved! */
+	WARN_ON(ringbuf->reserved_in_use);
+
 	if (ringbuf->space < rem) {
 		int ret = ring_wait_for_space(ring, rem);
 		if (ret)
@@ -2180,16 +2186,74 @@  int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
 	return 0;
 }
 
-static int __intel_ring_prepare(struct intel_engine_cs *ring,
-				int bytes)
+void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size)
+{
+	/* NB: Until request management is fully tidied up and the OLR is
+	 * removed, there are too many ways for get false hits on this
+	 * anti-recursion check! */
+	/*WARN_ON(ringbuf->reserved_size);*/
+	WARN_ON(ringbuf->reserved_in_use);
+
+	ringbuf->reserved_size = size;
+
+	/*
+	 * Really need to call _begin() here but that currently leads to
+	 * recursion problems! This will be fixed later but for now just
+	 * return and hope for the best. Note that there is only a real
+	 * problem if the create of the request never actually calls _begin()
+	 * but if they are not submitting any work then why did they create
+	 * the request in the first place?
+	 */
+}
+
+void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf)
+{
+	WARN_ON(ringbuf->reserved_in_use);
+
+	ringbuf->reserved_size   = 0;
+	ringbuf->reserved_in_use = false;
+}
+
+void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf)
+{
+	WARN_ON(ringbuf->reserved_in_use);
+
+	ringbuf->reserved_in_use = true;
+	ringbuf->reserved_tail   = ringbuf->tail;
+}
+
+void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
+{
+	WARN_ON(!ringbuf->reserved_in_use);
+	WARN(ringbuf->tail > ringbuf->reserved_tail + ringbuf->reserved_size,
+	     "request reserved size too small: %d vs %d!\n",
+	     ringbuf->tail - ringbuf->reserved_tail, ringbuf->reserved_size);
+
+	ringbuf->reserved_size   = 0;
+	ringbuf->reserved_in_use = false;
+}
+
+static int __intel_ring_prepare(struct intel_engine_cs *ring, int bytes)
 {
 	struct intel_ringbuffer *ringbuf = ring->buffer;
 	int ret;
 
+	if (!ringbuf->reserved_in_use)
+		bytes += ringbuf->reserved_size;
+
 	if (unlikely(ringbuf->tail + bytes > ringbuf->effective_size)) {
+		WARN_ON(ringbuf->reserved_in_use);
+
 		ret = intel_wrap_ring_buffer(ring);
 		if (unlikely(ret))
 			return ret;
+
+		if(ringbuf->reserved_size) {
+			uint32_t size = ringbuf->reserved_size;
+
+			intel_ring_reserved_space_cancel(ringbuf);
+			intel_ring_reserved_space_reserve(ringbuf, size);
+		}
 	}
 
 	if (unlikely(ringbuf->space < bytes)) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 39f6dfc..bf2ac28 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -105,6 +105,9 @@  struct intel_ringbuffer {
 	int space;
 	int size;
 	int effective_size;
+	int reserved_size;
+	int reserved_tail;
+	bool reserved_in_use;
 
 	/** We track the position of the requests in the ring buffer, and
 	 * when each is retired we increment last_retired_head as the GPU
@@ -450,4 +453,26 @@  intel_ring_get_request(struct intel_engine_cs *ring)
 	return ring->outstanding_lazy_request;
 }
 
+/*
+ * Arbitrary size for largest possible 'add request' sequence. The code paths
+ * are complex and variable. Empirical measurement shows that the worst case
+ * is ILK at 136 words. Reserving too much is better than reserving too little
+ * as that allows for corner cases that might have been missed. So the figure
+ * has been rounded up to 160 words.
+ */
+#define MIN_SPACE_FOR_ADD_REQUEST	160
+
+/*
+ * Reserve space in the ring to guarantee that the i915_add_request() call
+ * will always have sufficient room to do its stuff. The request creation
+ * code calls this automatically.
+ */
+void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size);
+/* Cancel the reservation, e.g. because the request is being discarded. */
+void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf);
+/* Use the reserved space - for use by i915_add_request() only. */
+void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf);
+/* Finish with the reserved space - for use by i915_add_request() only. */
+void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf);
+
 #endif /* _INTEL_RINGBUFFER_H_ */