diff mbox

[55/59] drm/i915: Remove fallback poll for ring buffer space

Message ID 1426768264-16996-56-git-send-email-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Harrison March 19, 2015, 12:31 p.m. UTC
From: John Harrison <John.C.Harrison@Intel.com>

When the ring buffer is full, the driver finds an outstanding request that will
free up sufficient space for the current operation and waits for it to complete.
If no such request can be found, there is a fall back path of just polling until
sufficient space is available.

This path should not be required any more. It is a hangover from the bad days of
OLR such that it was possible for the ring to be completely filled without ever
having submitted a request. This can no longer happen as requests are now
submitted in a timely manner. Hence the entire polling path is obsolete. As it
also causes headaches in LRC land due to nesting faked requests, it is being
removed entirely.

For: VIZ-5115
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        |   65 ++++---------------------------
 drivers/gpu/drm/i915/intel_ringbuffer.c |   62 +++--------------------------
 2 files changed, 13 insertions(+), 114 deletions(-)

Comments

Thomas Daniel March 19, 2015, 3 p.m. UTC | #1
> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of

> John.C.Harrison@Intel.com

> Sent: Thursday, March 19, 2015 12:31 PM

> To: Intel-GFX@Lists.FreeDesktop.Org

> Subject: [Intel-gfx] [PATCH 55/59] drm/i915: Remove fallback poll for ring buffer

> space

> 

> From: John Harrison <John.C.Harrison@Intel.com>

> 

> When the ring buffer is full, the driver finds an outstanding request that will

> free up sufficient space for the current operation and waits for it to complete.

> If no such request can be found, there is a fall back path of just polling until

> sufficient space is available.

> 

> This path should not be required any more. It is a hangover from the bad days of

> OLR such that it was possible for the ring to be completely filled without ever

> having submitted a request. This can no longer happen as requests are now

> submitted in a timely manner. Hence the entire polling path is obsolete. As it

> also causes headaches in LRC land due to nesting faked requests, it is being

> removed entirely.

> 

> For: VIZ-5115

> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

> ---

>  drivers/gpu/drm/i915/intel_lrc.c        |   65 ++++---------------------------

>  drivers/gpu/drm/i915/intel_ringbuffer.c |   62 +++--------------------------

>  2 files changed, 13 insertions(+), 114 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c

> index 60bcf9a..f21f449 100644

> --- a/drivers/gpu/drm/i915/intel_lrc.c

> +++ b/drivers/gpu/drm/i915/intel_lrc.c

> @@ -622,8 +622,9 @@ int intel_logical_ring_alloc_request_extras(struct

> drm_i915_gem_request *request

>  	return 0;

>  }

> 

> -static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,

> -				     int bytes)

> +static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,

> +				       struct intel_context *ctx,

> +				       int bytes)

>  {

>  	struct intel_engine_cs *ring = ringbuf->ring;

>  	struct drm_i915_gem_request *request;

> @@ -652,8 +653,11 @@ static int logical_ring_wait_request(struct

> intel_ringbuffer *ringbuf,

>  			break;

>  	}

> 

> -	if (&request->list == &ring->request_list)

> +	/* It should always be possible to find a suitable request! */

> +	if (&request->list == &ring->request_list) {

> +		WARN_ON(true);

>  		return -ENOSPC;

> +	}

Don’t we normally say
	if (WARN_ON(&request->list == &ring->request_list))
		return -ENOSPC;

> 

>  	ret = i915_wait_request(request);

>  	if (ret)

> @@ -663,7 +667,7 @@ static int logical_ring_wait_request(struct

> intel_ringbuffer *ringbuf,

> 

>  	WARN_ON(intel_ring_space(ringbuf) < new_space);

> 

> -	return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;

> +	return 0;

>  }

> 

>  /*

> @@ -690,59 +694,6 @@ intel_logical_ring_advance_and_submit(struct

> intel_ringbuffer *ringbuf,

>  	execlists_context_queue(ring, ctx, ringbuf->tail, request);

>  }

> 

> -static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,

> -				       struct intel_context *ctx,

> -				       int bytes)

> -{

> -	struct intel_engine_cs *ring = ringbuf->ring;

> -	struct drm_device *dev = ring->dev;

> -	struct drm_i915_private *dev_priv = dev->dev_private;

> -	unsigned long end;

> -	int ret;

> -

> -	/* The whole point of reserving space is to not wait! */

> -	WARN_ON(ringbuf->reserved_in_use);

> -

> -	ret = logical_ring_wait_request(ringbuf, bytes);

> -	if (ret != -ENOSPC)

> -		return ret;

> -

> -	/* Force the context submission in case we have been skipping it */

> -	intel_logical_ring_advance_and_submit(ringbuf, ctx, NULL);

> -

> -	/* With GEM the hangcheck timer should kick us out of the loop,

> -	 * leaving it early runs the risk of corrupting GEM state (due

> -	 * to running on almost untested codepaths). But on resume

> -	 * timers don't work yet, so prevent a complete hang in that

> -	 * case by choosing an insanely large timeout. */

> -	end = jiffies + 60 * HZ;

> -

> -	ret = 0;

> -	do {

> -		if (intel_ring_space(ringbuf) >= bytes)

> -			break;

> -

> -		msleep(1);

> -

> -		if (dev_priv->mm.interruptible && signal_pending(current)) {

> -			ret = -ERESTARTSYS;

> -			break;

> -		}

> -

> -		ret = i915_gem_check_wedge(&dev_priv->gpu_error,

> -					   dev_priv->mm.interruptible);

> -		if (ret)

> -			break;

> -

> -		if (time_after(jiffies, end)) {

> -			ret = -EBUSY;

> -			break;

> -		}

> -	} while (1);

> -

> -	return ret;

> -}

> -

>  static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf,

>  				    struct intel_context *ctx)

>  {

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c

> b/drivers/gpu/drm/i915/intel_ringbuffer.c

> index c5752c4..6099fce 100644

> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c

> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c

> @@ -2063,7 +2063,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs

> *ring)

>  	ring->buffer = NULL;

>  }

> 

> -static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)

> +static int ring_wait_for_space(struct intel_engine_cs *ring, int n)

>  {

>  	struct intel_ringbuffer *ringbuf = ring->buffer;

>  	struct drm_i915_gem_request *request;

> @@ -2082,8 +2082,11 @@ static int intel_ring_wait_request(struct

> intel_engine_cs *ring, int n)

>  			break;

>  	}

> 

> -	if (&request->list == &ring->request_list)

> +	/* It should always be possible to find a suitable request! */

> +	if (&request->list == &ring->request_list) {

> +		WARN_ON(true);

>  		return -ENOSPC;

> +	}

Same thing.

Thomas.

> 

>  	ret = i915_wait_request(request);

>  	if (ret)

> @@ -2096,61 +2099,6 @@ static int intel_ring_wait_request(struct

> intel_engine_cs *ring, int n)

>  	return 0;

>  }

> 

> -static int ring_wait_for_space(struct intel_engine_cs *ring, int n)

> -{

> -	struct drm_device *dev = ring->dev;

> -	struct drm_i915_private *dev_priv = dev->dev_private;

> -	struct intel_ringbuffer *ringbuf = ring->buffer;

> -	unsigned long end;

> -	int ret;

> -

> -	/* The whole point of reserving space is to not wait! */

> -	WARN_ON(ringbuf->reserved_in_use);

> -

> -	ret = intel_ring_wait_request(ring, n);

> -	if (ret != -ENOSPC)

> -		return ret;

> -

> -	/* force the tail write in case we have been skipping them */

> -	__intel_ring_advance(ring);

> -

> -	/* With GEM the hangcheck timer should kick us out of the loop,

> -	 * leaving it early runs the risk of corrupting GEM state (due

> -	 * to running on almost untested codepaths). But on resume

> -	 * timers don't work yet, so prevent a complete hang in that

> -	 * 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);

> -		if (intel_ring_space(ringbuf) >= n)

> -			break;

> -

> -		msleep(1);

> -

> -		if (dev_priv->mm.interruptible && signal_pending(current)) {

> -			ret = -ERESTARTSYS;

> -			break;

> -		}

> -

> -		ret = i915_gem_check_wedge(&dev_priv->gpu_error,

> -					   dev_priv->mm.interruptible);

> -		if (ret)

> -			break;

> -

> -		if (time_after(jiffies, end)) {

> -			ret = -EBUSY;

> -			break;

> -		}

> -	} while (1);

> -	trace_i915_ring_wait_end(ring);

> -	return ret;

> -}

> -

>  static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)

>  {

>  	uint32_t __iomem *virt;

> --

> 1.7.9.5

> 

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula March 19, 2015, 3:16 p.m. UTC | #2
On Thu, 19 Mar 2015, "Daniel, Thomas" <thomas.daniel@intel.com> wrote:
>> -	if (&request->list == &ring->request_list)
>> +	/* It should always be possible to find a suitable request! */
>> +	if (&request->list == &ring->request_list) {
>> +		WARN_ON(true);
>>  		return -ENOSPC;
>> +	}
> Don’t we normally say
> 	if (WARN_ON(&request->list == &ring->request_list))
> 		return -ENOSPC;

Yes, particularly since we've amended WARN_ON within i915 to print out
the condition that failed. "true" is not very useful with that. ;)

BR,
Jani.
John Harrison March 19, 2015, 4:33 p.m. UTC | #3
On 19/03/2015 15:16, Jani Nikula wrote:
> On Thu, 19 Mar 2015, "Daniel, Thomas" <thomas.daniel@intel.com> wrote:
>>> -	if (&request->list == &ring->request_list)
>>> +	/* It should always be possible to find a suitable request! */
>>> +	if (&request->list == &ring->request_list) {
>>> +		WARN_ON(true);
>>>   		return -ENOSPC;
>>> +	}
>> Don’t we normally say
>> 	if (WARN_ON(&request->list == &ring->request_list))
>> 		return -ENOSPC;
> Yes, particularly since we've amended WARN_ON within i915 to print out
> the condition that failed. "true" is not very useful with that. ;)
>
> BR,
> Jani.
>
The issue I have with 'if(WARN_ON(x))' is that it looks like something 
that would disappear in a non debugging build. Whereas, this is a check 
that wants to exist regardless of build options.
Daniel Vetter March 19, 2015, 5:29 p.m. UTC | #4
On Thu, Mar 19, 2015 at 04:33:12PM +0000, John Harrison wrote:
> On 19/03/2015 15:16, Jani Nikula wrote:
> >On Thu, 19 Mar 2015, "Daniel, Thomas" <thomas.daniel@intel.com> wrote:
> >>>-	if (&request->list == &ring->request_list)
> >>>+	/* It should always be possible to find a suitable request! */
> >>>+	if (&request->list == &ring->request_list) {
> >>>+		WARN_ON(true);
> >>>  		return -ENOSPC;
> >>>+	}
> >>Don’t we normally say
> >>	if (WARN_ON(&request->list == &ring->request_list))
> >>		return -ENOSPC;
> >Yes, particularly since we've amended WARN_ON within i915 to print out
> >the condition that failed. "true" is not very useful with that. ;)
> >
> >BR,
> >Jani.
> >
> The issue I have with 'if(WARN_ON(x))' is that it looks like something that
> would disappear in a non debugging build. Whereas, this is a check that
> wants to exist regardless of build options.

Yeah kernel isn't like that, WARN_ON is always executed. We rely on that
all over the place actually by sometimes wrapping full function-calls with
side-effects with a WARN_ON for cases we don't expect anything to ever
fail.
-Daniel
Tomas Elf March 31, 2015, 6:13 p.m. UTC | #5
On 19/03/2015 12:31, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> When the ring buffer is full, the driver finds an outstanding request that will
> free up sufficient space for the current operation and waits for it to complete.
> If no such request can be found, there is a fall back path of just polling until
> sufficient space is available.
>
> This path should not be required any more. It is a hangover from the bad days of
> OLR such that it was possible for the ring to be completely filled without ever
> having submitted a request. This can no longer happen as requests are now
> submitted in a timely manner. Hence the entire polling path is obsolete. As it
> also causes headaches in LRC land due to nesting faked requests, it is being
> removed entirely.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c        |   65 ++++---------------------------
>   drivers/gpu/drm/i915/intel_ringbuffer.c |   62 +++--------------------------
>   2 files changed, 13 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 60bcf9a..f21f449 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -622,8 +622,9 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   	return 0;
>   }
>
> -static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
> -				     int bytes)
> +static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
> +				       struct intel_context *ctx,
> +				       int bytes)
>   {
>   	struct intel_engine_cs *ring = ringbuf->ring;
>   	struct drm_i915_gem_request *request;
> @@ -652,8 +653,11 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
>   			break;
>   	}
>
> -	if (&request->list == &ring->request_list)
> +	/* It should always be possible to find a suitable request! */
> +	if (&request->list == &ring->request_list) {
> +		WARN_ON(true);

I agree with Thomas Daniel's earlier review comment that WARN_ON(true) 
is not very helpful. You could do something like:
	
	WARN_ON(ret = <expression>);
	if (ret)
		return -ENOSPC;

That way you don't have the redundancy of doing

	WARN_ON(expression);
	if (expression)
		return -ENOSPC;


and you don't have to do if (WARN_ON(expression)) if you don't like it.	

Thanks,
Tomas


>   		return -ENOSPC;
> +	}
>
>   	ret = i915_wait_request(request);
>   	if (ret)
> @@ -663,7 +667,7 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
>
>   	WARN_ON(intel_ring_space(ringbuf) < new_space);
>
> -	return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;
> +	return 0;
>   }
>
>   /*
> @@ -690,59 +694,6 @@ intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf,
>   	execlists_context_queue(ring, ctx, ringbuf->tail, request);
>   }
>
> -static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
> -				       struct intel_context *ctx,
> -				       int bytes)
> -{
> -	struct intel_engine_cs *ring = ringbuf->ring;
> -	struct drm_device *dev = ring->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long end;
> -	int ret;
> -
> -	/* The whole point of reserving space is to not wait! */
> -	WARN_ON(ringbuf->reserved_in_use);
> -
> -	ret = logical_ring_wait_request(ringbuf, bytes);
> -	if (ret != -ENOSPC)
> -		return ret;
> -
> -	/* Force the context submission in case we have been skipping it */
> -	intel_logical_ring_advance_and_submit(ringbuf, ctx, NULL);
> -
> -	/* With GEM the hangcheck timer should kick us out of the loop,
> -	 * leaving it early runs the risk of corrupting GEM state (due
> -	 * to running on almost untested codepaths). But on resume
> -	 * timers don't work yet, so prevent a complete hang in that
> -	 * case by choosing an insanely large timeout. */
> -	end = jiffies + 60 * HZ;
> -
> -	ret = 0;
> -	do {
> -		if (intel_ring_space(ringbuf) >= bytes)
> -			break;
> -
> -		msleep(1);
> -
> -		if (dev_priv->mm.interruptible && signal_pending(current)) {
> -			ret = -ERESTARTSYS;
> -			break;
> -		}
> -
> -		ret = i915_gem_check_wedge(&dev_priv->gpu_error,
> -					   dev_priv->mm.interruptible);
> -		if (ret)
> -			break;
> -
> -		if (time_after(jiffies, end)) {
> -			ret = -EBUSY;
> -			break;
> -		}
> -	} while (1);
> -
> -	return ret;
> -}
> -
>   static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf,
>   				    struct intel_context *ctx)
>   {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c5752c4..6099fce 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2063,7 +2063,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
>   	ring->buffer = NULL;
>   }
>
> -static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
> +static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>   {
>   	struct intel_ringbuffer *ringbuf = ring->buffer;
>   	struct drm_i915_gem_request *request;
> @@ -2082,8 +2082,11 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
>   			break;
>   	}
>
> -	if (&request->list == &ring->request_list)
> +	/* It should always be possible to find a suitable request! */
> +	if (&request->list == &ring->request_list) {
> +		WARN_ON(true);
>   		return -ENOSPC;
> +	}
>
>   	ret = i915_wait_request(request);
>   	if (ret)
> @@ -2096,61 +2099,6 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
>   	return 0;
>   }
>
> -static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
> -{
> -	struct drm_device *dev = ring->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_ringbuffer *ringbuf = ring->buffer;
> -	unsigned long end;
> -	int ret;
> -
> -	/* The whole point of reserving space is to not wait! */
> -	WARN_ON(ringbuf->reserved_in_use);
> -
> -	ret = intel_ring_wait_request(ring, n);
> -	if (ret != -ENOSPC)
> -		return ret;
> -
> -	/* force the tail write in case we have been skipping them */
> -	__intel_ring_advance(ring);
> -
> -	/* With GEM the hangcheck timer should kick us out of the loop,
> -	 * leaving it early runs the risk of corrupting GEM state (due
> -	 * to running on almost untested codepaths). But on resume
> -	 * timers don't work yet, so prevent a complete hang in that
> -	 * 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);
> -		if (intel_ring_space(ringbuf) >= n)
> -			break;
> -
> -		msleep(1);
> -
> -		if (dev_priv->mm.interruptible && signal_pending(current)) {
> -			ret = -ERESTARTSYS;
> -			break;
> -		}
> -
> -		ret = i915_gem_check_wedge(&dev_priv->gpu_error,
> -					   dev_priv->mm.interruptible);
> -		if (ret)
> -			break;
> -
> -		if (time_after(jiffies, end)) {
> -			ret = -EBUSY;
> -			break;
> -		}
> -	} while (1);
> -	trace_i915_ring_wait_end(ring);
> -	return ret;
> -}
> -
>   static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
>   {
>   	uint32_t __iomem *virt;
>
Daniel Vetter April 1, 2015, 6:02 a.m. UTC | #6
On Tue, Mar 31, 2015 at 07:13:40PM +0100, Tomas Elf wrote:
> On 19/03/2015 12:31, John.C.Harrison@Intel.com wrote:
> >From: John Harrison <John.C.Harrison@Intel.com>
> >@@ -652,8 +653,11 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
> >  			break;
> >  	}
> >
> >-	if (&request->list == &ring->request_list)
> >+	/* It should always be possible to find a suitable request! */
> >+	if (&request->list == &ring->request_list) {
> >+		WARN_ON(true);
> 
> I agree with Thomas Daniel's earlier review comment that WARN_ON(true) is
> not very helpful. You could do something like:
> 	
> 	WARN_ON(ret = <expression>);
> 	if (ret)
> 		return -ENOSPC;
> 
> That way you don't have the redundancy of doing
> 
> 	WARN_ON(expression);
> 	if (expression)
> 		return -ENOSPC;
> 
> 
> and you don't have to do if (WARN_ON(expression)) if you don't like it.	

if (WARN_ON(expression)) is prefectly acceptable style in i915 (there's
lots more of these) and what I'd go with here.
-Daniel
Chris Wilson April 1, 2015, 8:51 a.m. UTC | #7
On Tue, Mar 31, 2015 at 07:13:40PM +0100, Tomas Elf wrote:
> On 19/03/2015 12:31, John.C.Harrison@Intel.com wrote:
> >From: John Harrison <John.C.Harrison@Intel.com>
> >
> >When the ring buffer is full, the driver finds an outstanding request that will
> >free up sufficient space for the current operation and waits for it to complete.
> >If no such request can be found, there is a fall back path of just polling until
> >sufficient space is available.
> >
> >This path should not be required any more. It is a hangover from the bad days of
> >OLR such that it was possible for the ring to be completely filled without ever
> >having submitted a request.

No. This is wrong. It is from DRI1. We never could have touched the ring
without an OLR.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 60bcf9a..f21f449 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -622,8 +622,9 @@  int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 	return 0;
 }
 
-static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
-				     int bytes)
+static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
+				       struct intel_context *ctx,
+				       int bytes)
 {
 	struct intel_engine_cs *ring = ringbuf->ring;
 	struct drm_i915_gem_request *request;
@@ -652,8 +653,11 @@  static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
 			break;
 	}
 
-	if (&request->list == &ring->request_list)
+	/* It should always be possible to find a suitable request! */
+	if (&request->list == &ring->request_list) {
+		WARN_ON(true);
 		return -ENOSPC;
+	}
 
 	ret = i915_wait_request(request);
 	if (ret)
@@ -663,7 +667,7 @@  static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf,
 
 	WARN_ON(intel_ring_space(ringbuf) < new_space);
 
-	return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC;
+	return 0;
 }
 
 /*
@@ -690,59 +694,6 @@  intel_logical_ring_advance_and_submit(struct intel_ringbuffer *ringbuf,
 	execlists_context_queue(ring, ctx, ringbuf->tail, request);
 }
 
-static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
-				       struct intel_context *ctx,
-				       int bytes)
-{
-	struct intel_engine_cs *ring = ringbuf->ring;
-	struct drm_device *dev = ring->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long end;
-	int ret;
-
-	/* The whole point of reserving space is to not wait! */
-	WARN_ON(ringbuf->reserved_in_use);
-
-	ret = logical_ring_wait_request(ringbuf, bytes);
-	if (ret != -ENOSPC)
-		return ret;
-
-	/* Force the context submission in case we have been skipping it */
-	intel_logical_ring_advance_and_submit(ringbuf, ctx, NULL);
-
-	/* With GEM the hangcheck timer should kick us out of the loop,
-	 * leaving it early runs the risk of corrupting GEM state (due
-	 * to running on almost untested codepaths). But on resume
-	 * timers don't work yet, so prevent a complete hang in that
-	 * case by choosing an insanely large timeout. */
-	end = jiffies + 60 * HZ;
-
-	ret = 0;
-	do {
-		if (intel_ring_space(ringbuf) >= bytes)
-			break;
-
-		msleep(1);
-
-		if (dev_priv->mm.interruptible && signal_pending(current)) {
-			ret = -ERESTARTSYS;
-			break;
-		}
-
-		ret = i915_gem_check_wedge(&dev_priv->gpu_error,
-					   dev_priv->mm.interruptible);
-		if (ret)
-			break;
-
-		if (time_after(jiffies, end)) {
-			ret = -EBUSY;
-			break;
-		}
-	} while (1);
-
-	return ret;
-}
-
 static int logical_ring_wrap_buffer(struct intel_ringbuffer *ringbuf,
 				    struct intel_context *ctx)
 {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c5752c4..6099fce 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2063,7 +2063,7 @@  void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 	ring->buffer = NULL;
 }
 
-static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
+static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
 {
 	struct intel_ringbuffer *ringbuf = ring->buffer;
 	struct drm_i915_gem_request *request;
@@ -2082,8 +2082,11 @@  static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 			break;
 	}
 
-	if (&request->list == &ring->request_list)
+	/* It should always be possible to find a suitable request! */
+	if (&request->list == &ring->request_list) {
+		WARN_ON(true);
 		return -ENOSPC;
+	}
 
 	ret = i915_wait_request(request);
 	if (ret)
@@ -2096,61 +2099,6 @@  static int intel_ring_wait_request(struct intel_engine_cs *ring, int n)
 	return 0;
 }
 
-static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
-{
-	struct drm_device *dev = ring->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_ringbuffer *ringbuf = ring->buffer;
-	unsigned long end;
-	int ret;
-
-	/* The whole point of reserving space is to not wait! */
-	WARN_ON(ringbuf->reserved_in_use);
-
-	ret = intel_ring_wait_request(ring, n);
-	if (ret != -ENOSPC)
-		return ret;
-
-	/* force the tail write in case we have been skipping them */
-	__intel_ring_advance(ring);
-
-	/* With GEM the hangcheck timer should kick us out of the loop,
-	 * leaving it early runs the risk of corrupting GEM state (due
-	 * to running on almost untested codepaths). But on resume
-	 * timers don't work yet, so prevent a complete hang in that
-	 * 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);
-		if (intel_ring_space(ringbuf) >= n)
-			break;
-
-		msleep(1);
-
-		if (dev_priv->mm.interruptible && signal_pending(current)) {
-			ret = -ERESTARTSYS;
-			break;
-		}
-
-		ret = i915_gem_check_wedge(&dev_priv->gpu_error,
-					   dev_priv->mm.interruptible);
-		if (ret)
-			break;
-
-		if (time_after(jiffies, end)) {
-			ret = -EBUSY;
-			break;
-		}
-	} while (1);
-	trace_i915_ring_wait_end(ring);
-	return ret;
-}
-
 static int intel_wrap_ring_buffer(struct intel_engine_cs *ring)
 {
 	uint32_t __iomem *virt;