[46/55] drm/i915: Update intel_ring_begin() to take a request structure

Commit Message

John Harrison May 29, 2015, 4:44 p.m. UTC

From: John Harrison <[e-mail]>

Now that everything above has been converted to use requests, intel_ring_begin()
can be updated to take a request instead of a ring. This also means that it no
longer needs to lazily allocate a request if no-one happens to have done it
earlier.

For: VIZ-5115
Signed-off-by: John Harrison <[e-mail]>
Reviewed-by: Tomas Elf <[e-mail]>
---
 drivers/gpu/drm/i915/i915_gem.c            |    2 +-
 drivers/gpu/drm/i915/i915_gem_context.c    |    2 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    8 +--
 drivers/gpu/drm/i915/i915_gem_gtt.c        |    6 +--
 drivers/gpu/drm/i915/intel_display.c       |   10 ++--
 drivers/gpu/drm/i915/intel_overlay.c       |    8 +--
 drivers/gpu/drm/i915/intel_ringbuffer.c    |   74 ++++++++++++++--------------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |    2 +-
 8 files changed, 55 insertions(+), 57 deletions(-)

Comments

Chris Wilson June 23, 2015, 10:24 a.m. UTC | #1

On Fri, May 29, 2015 at 05:44:07PM +0100, [e-mail] wrote:
> -int intel_ring_begin(struct intel_engine_cs *ring,
> +int intel_ring_begin(struct drm_i915_gem_request *req,
>  		     int num_dwords)
>  {
> -	struct drm_i915_gem_request *req;
> -	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct intel_engine_cs *ring;
> +	struct drm_i915_private *dev_priv;
>  	int ret;
>  
> +	WARN_ON(req == NULL);
> +	ring = req->ring;

What was the point?
-Chris

John Harrison June 23, 2015, 10:37 a.m. UTC | #2

On 23/06/2015 11:24, Chris Wilson wrote:
> On Fri, May 29, 2015 at 05:44:07PM +0100, [e-mail] wrote:
>> -int intel_ring_begin(struct intel_engine_cs *ring,
>> +int intel_ring_begin(struct drm_i915_gem_request *req,
>>   		     int num_dwords)
>>   {
>> -	struct drm_i915_gem_request *req;
>> -	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>> +	struct intel_engine_cs *ring;
>> +	struct drm_i915_private *dev_priv;
>>   	int ret;
>>   
>> +	WARN_ON(req == NULL);
>> +	ring = req->ring;
> What was the point?
> -Chris
>

The point is to remove the OLR. The significant change within 
intel_ring_begin is the next few lines:

-	/* Preallocate the olr before touching the ring */
-	ret = i915_gem_request_alloc(ring, ring->default_context, &req);


That is the part that causes problems by randomly creating a brand new 
request that no-one knows about and squirreling it away in the OLR to 
scoop up random bits of work. This is the whole point of the entire 
patch series - to ensure that all ring work is assigned to a known 
request by whoever instigated that work.

John.

Daniel Vetter June 23, 2015, 1:25 p.m. UTC | #3

On Tue, Jun 23, 2015 at 11:37:53AM +0100, John Harrison wrote:
> On 23/06/2015 11:24, Chris Wilson wrote:
> >On Fri, May 29, 2015 at 05:44:07PM +0100, [e-mail] wrote:
> >>-int intel_ring_begin(struct intel_engine_cs *ring,
> >>+int intel_ring_begin(struct drm_i915_gem_request *req,
> >>  		     int num_dwords)
> >>  {
> >>-	struct drm_i915_gem_request *req;
> >>-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >>+	struct intel_engine_cs *ring;
> >>+	struct drm_i915_private *dev_priv;
> >>  	int ret;
> >>+	WARN_ON(req == NULL);
> >>+	ring = req->ring;
> >What was the point?
> >-Chris
> >
> 
> The point is to remove the OLR. The significant change within
> intel_ring_begin is the next few lines:
> 
> -	/* Preallocate the olr before touching the ring */
> -	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> 
> 
> That is the part that causes problems by randomly creating a brand new
> request that no-one knows about and squirreling it away in the OLR to scoop
> up random bits of work. This is the whole point of the entire patch series -
> to ensure that all ring work is assigned to a known request by whoever
> instigated that work.

I think the point was that a WARN_ON(pointer) followed by an immediate
deref of said pointer doesn't add value. This is the one case where a
BUG_ON is the right joice. Or just let the kernel oops on the NULL deref
without warning first that it'll happen.
-Daniel

John Harrison June 23, 2015, 3:27 p.m. UTC | #4

On 23/06/2015 14:25, Daniel Vetter wrote:
> On Tue, Jun 23, 2015 at 11:37:53AM +0100, John Harrison wrote:
>> On 23/06/2015 11:24, Chris Wilson wrote:
>>> On Fri, May 29, 2015 at 05:44:07PM +0100, [e-mail] wrote:
>>>> -int intel_ring_begin(struct intel_engine_cs *ring,
>>>> +int intel_ring_begin(struct drm_i915_gem_request *req,
>>>>   		     int num_dwords)
>>>>   {
>>>> -	struct drm_i915_gem_request *req;
>>>> -	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>>>> +	struct intel_engine_cs *ring;
>>>> +	struct drm_i915_private *dev_priv;
>>>>   	int ret;
>>>> +	WARN_ON(req == NULL);
>>>> +	ring = req->ring;
>>> What was the point?
>>> -Chris
>>>
>> The point is to remove the OLR. The significant change within
>> intel_ring_begin is the next few lines:
>>
>> -	/* Preallocate the olr before touching the ring */
>> -	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
>>
>>
>> That is the part that causes problems by randomly creating a brand new
>> request that no-one knows about and squirreling it away in the OLR to scoop
>> up random bits of work. This is the whole point of the entire patch series -
>> to ensure that all ring work is assigned to a known request by whoever
>> instigated that work.
> I think the point was that a WARN_ON(pointer) followed by an immediate
> deref of said pointer doesn't add value. This is the one case where a
> BUG_ON is the right joice. Or just let the kernel oops on the NULL deref
> without warning first that it'll happen.
> -Daniel
I thought the edict from yourself was that BUG_ONs should never be used, 
it should always be a WARN_ON and if the driver oopses afterwards then 
so be it. The WARN_ON does add value in that it gives you a line number 
(and file) which in turn gives you exactly what variable was null. 
Whereas the oops just gives you an offset into a function. Unless you 
happen to have the exact same binary that generated the bug report, the 
chances of identifying what was null can be slim.

John.

Daniel Vetter June 23, 2015, 3:34 p.m. UTC | #5

On Tue, Jun 23, 2015 at 04:27:45PM +0100, John Harrison wrote:
> On 23/06/2015 14:25, Daniel Vetter wrote:
> >On Tue, Jun 23, 2015 at 11:37:53AM +0100, John Harrison wrote:
> >>On 23/06/2015 11:24, Chris Wilson wrote:
> >>>On Fri, May 29, 2015 at 05:44:07PM +0100, [e-mail] wrote:
> >>>>-int intel_ring_begin(struct intel_engine_cs *ring,
> >>>>+int intel_ring_begin(struct drm_i915_gem_request *req,
> >>>>  		     int num_dwords)
> >>>>  {
> >>>>-	struct drm_i915_gem_request *req;
> >>>>-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >>>>+	struct intel_engine_cs *ring;
> >>>>+	struct drm_i915_private *dev_priv;
> >>>>  	int ret;
> >>>>+	WARN_ON(req == NULL);
> >>>>+	ring = req->ring;
> >>>What was the point?
> >>>-Chris
> >>>
> >>The point is to remove the OLR. The significant change within
> >>intel_ring_begin is the next few lines:
> >>
> >>-	/* Preallocate the olr before touching the ring */
> >>-	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> >>
> >>
> >>That is the part that causes problems by randomly creating a brand new
> >>request that no-one knows about and squirreling it away in the OLR to scoop
> >>up random bits of work. This is the whole point of the entire patch series -
> >>to ensure that all ring work is assigned to a known request by whoever
> >>instigated that work.
> >I think the point was that a WARN_ON(pointer) followed by an immediate
> >deref of said pointer doesn't add value. This is the one case where a
> >BUG_ON is the right joice. Or just let the kernel oops on the NULL deref
> >without warning first that it'll happen.
> >-Daniel
> I thought the edict from yourself was that BUG_ONs should never be used, it
> should always be a WARN_ON and if the driver oopses afterwards then so be
> it. The WARN_ON does add value in that it gives you a line number (and file)
> which in turn gives you exactly what variable was null. Whereas the oops
> just gives you an offset into a function. Unless you happen to have the
> exact same binary that generated the bug report, the chances of identifying
> what was null can be slim.

I might have gone over the top with screaming against WARN_ONs, so let me
clarify: By default use WARN_ON since if there's a decent chance that the
driver can limp along that greatly improves the chances that devs or users
can get useful backtraces out of a dying machine.

But if you can prove that the driver will oops anyway you can add a BUG_ON
to improve debug output for these cases.

Personally I'm on the fence whether instead a if (WARN_ON()) return
-EINVAL is better or not, I leave that to patch authors. But WARN_ON +
Oops is a bit too much.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d34d5ac..9f3e0717 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4900,7 +4900,7 @@  int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
 	if (!HAS_L3_DPF(dev) || !remap_info)
 		return 0;
 
-	ret = intel_ring_begin(ring, GEN7_L3LOG_SIZE / 4 * 3);
+	ret = intel_ring_begin(req, GEN7_L3LOG_SIZE / 4 * 3);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index b90e4c0..5161747 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -509,7 +509,7 @@  mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
 	if (INTEL_INFO(ring->dev)->gen >= 7)
 		len += 2 + (num_rings ? 4*num_rings + 2 : 0);
 
-	ret = intel_ring_begin(ring, len);
+	ret = intel_ring_begin(req, len);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0b24d8f..805d288 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1074,7 +1074,7 @@  i915_reset_gen7_sol_offsets(struct drm_device *dev,
 		return -EINVAL;
 	}
 
-	ret = intel_ring_begin(ring, 4 * 3);
+	ret = intel_ring_begin(req, 4 * 3);
 	if (ret)
 		return ret;
 
@@ -1105,7 +1105,7 @@  i915_emit_box(struct drm_i915_gem_request *req,
 	}
 
 	if (INTEL_INFO(ring->dev)->gen >= 4) {
-		ret = intel_ring_begin(ring, 4);
+		ret = intel_ring_begin(req, 4);
 		if (ret)
 			return ret;
 
@@ -1114,7 +1114,7 @@  i915_emit_box(struct drm_i915_gem_request *req,
 		intel_ring_emit(ring, ((box->x2 - 1) & 0xffff) | (box->y2 - 1) << 16);
 		intel_ring_emit(ring, DR4);
 	} else {
-		ret = intel_ring_begin(ring, 6);
+		ret = intel_ring_begin(req, 6);
 		if (ret)
 			return ret;
 
@@ -1290,7 +1290,7 @@  i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
 
 	if (ring == &dev_priv->ring[RCS] &&
 			instp_mode != dev_priv->relative_constants_mode) {
-		ret = intel_ring_begin(ring, 4);
+		ret = intel_ring_begin(params->request, 4);
 		if (ret)
 			goto error;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ea522fa..acde25a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -461,7 +461,7 @@  static int gen8_write_pdp(struct drm_i915_gem_request *req,
 
 	BUG_ON(entry >= 4);
 
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret)
 		return ret;
 
@@ -1066,7 +1066,7 @@  static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt,
 	if (ret)
 		return ret;
 
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret)
 		return ret;
 
@@ -1103,7 +1103,7 @@  static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt,
 	if (ret)
 		return ret;
 
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0327628..91e19d0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10643,7 +10643,7 @@  static int intel_gen2_queue_flip(struct drm_device *dev,
 	u32 flip_mask;
 	int ret;
 
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret)
 		return ret;
 
@@ -10678,7 +10678,7 @@  static int intel_gen3_queue_flip(struct drm_device *dev,
 	u32 flip_mask;
 	int ret;
 
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret)
 		return ret;
 
@@ -10711,7 +10711,7 @@  static int intel_gen4_queue_flip(struct drm_device *dev,
 	uint32_t pf, pipesrc;
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
@@ -10750,7 +10750,7 @@  static int intel_gen6_queue_flip(struct drm_device *dev,
 	uint32_t pf, pipesrc;
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
@@ -10826,7 +10826,7 @@  static int intel_gen7_queue_flip(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	ret = intel_ring_begin(ring, len);
+	ret = intel_ring_begin(req, len);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 3f70904..4445426 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -244,7 +244,7 @@  static int intel_overlay_on(struct intel_overlay *overlay)
 	if (ret)
 		return ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret) {
 		i915_gem_request_cancel(req);
 		return ret;
@@ -287,7 +287,7 @@  static int intel_overlay_continue(struct intel_overlay *overlay,
 	if (ret)
 		return ret;
 
-	ret = intel_ring_begin(ring, 2);
+	ret = intel_ring_begin(req, 2);
 	if (ret) {
 		i915_gem_request_cancel(req);
 		return ret;
@@ -353,7 +353,7 @@  static int intel_overlay_off(struct intel_overlay *overlay)
 	if (ret)
 		return ret;
 
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret) {
 		i915_gem_request_cancel(req);
 		return ret;
@@ -427,7 +427,7 @@  static int intel_overlay_release_old_vid(struct intel_overlay *overlay)
 		if (ret)
 			return ret;
 
-		ret = intel_ring_begin(ring, 2);
+		ret = intel_ring_begin(req, 2);
 		if (ret) {
 			i915_gem_request_cancel(req);
 			return ret;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8ebc0d8..bb10fc2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -106,7 +106,7 @@  gen2_render_ring_flush(struct drm_i915_gem_request *req,
 	if (invalidate_domains & I915_GEM_DOMAIN_SAMPLER)
 		cmd |= MI_READ_FLUSH;
 
-	ret = intel_ring_begin(ring, 2);
+	ret = intel_ring_begin(req, 2);
 	if (ret)
 		return ret;
 
@@ -165,7 +165,7 @@  gen4_render_ring_flush(struct drm_i915_gem_request *req,
 	    (IS_G4X(dev) || IS_GEN5(dev)))
 		cmd |= MI_INVALIDATE_ISP;
 
-	ret = intel_ring_begin(ring, 2);
+	ret = intel_ring_begin(req, 2);
 	if (ret)
 		return ret;
 
@@ -220,8 +220,7 @@  intel_emit_post_sync_nonzero_flush(struct drm_i915_gem_request *req)
 	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
 	int ret;
 
-
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret)
 		return ret;
 
@@ -234,7 +233,7 @@  intel_emit_post_sync_nonzero_flush(struct drm_i915_gem_request *req)
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_advance(ring);
 
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret)
 		return ret;
 
@@ -289,7 +288,7 @@  gen6_render_ring_flush(struct drm_i915_gem_request *req,
 		flags |= PIPE_CONTROL_QW_WRITE | PIPE_CONTROL_CS_STALL;
 	}
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
@@ -308,7 +307,7 @@  gen7_render_ring_cs_stall_wa(struct drm_i915_gem_request *req)
 	struct intel_engine_cs *ring = req->ring;
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
@@ -371,7 +370,7 @@  gen7_render_ring_flush(struct drm_i915_gem_request *req,
 		gen7_render_ring_cs_stall_wa(req);
 	}
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
@@ -391,7 +390,7 @@  gen8_emit_pipe_control(struct drm_i915_gem_request *req,
 	struct intel_engine_cs *ring = req->ring;
 	int ret;
 
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret)
 		return ret;
 
@@ -726,7 +725,7 @@  static int intel_ring_workarounds_emit(struct drm_i915_gem_request *req)
 	if (ret)
 		return ret;
 
-	ret = intel_ring_begin(ring, (w->count * 2 + 2));
+	ret = intel_ring_begin(req, (w->count * 2 + 2));
 	if (ret)
 		return ret;
 
@@ -1175,7 +1174,7 @@  static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
 	num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS;
 #undef MBOX_UPDATE_DWORDS
 
-	ret = intel_ring_begin(signaller, num_dwords);
+	ret = intel_ring_begin(signaller_req, num_dwords);
 	if (ret)
 		return ret;
 
@@ -1216,7 +1215,7 @@  static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
 	num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS;
 #undef MBOX_UPDATE_DWORDS
 
-	ret = intel_ring_begin(signaller, num_dwords);
+	ret = intel_ring_begin(signaller_req, num_dwords);
 	if (ret)
 		return ret;
 
@@ -1255,7 +1254,7 @@  static int gen6_signal(struct drm_i915_gem_request *signaller_req,
 	num_dwords += round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2);
 #undef MBOX_UPDATE_DWORDS
 
-	ret = intel_ring_begin(signaller, num_dwords);
+	ret = intel_ring_begin(signaller_req, num_dwords);
 	if (ret)
 		return ret;
 
@@ -1293,7 +1292,7 @@  gen6_add_request(struct drm_i915_gem_request *req)
 	if (ring->semaphore.signal)
 		ret = ring->semaphore.signal(req, 4);
 	else
-		ret = intel_ring_begin(ring, 4);
+		ret = intel_ring_begin(req, 4);
 
 	if (ret)
 		return ret;
@@ -1331,7 +1330,7 @@  gen8_ring_sync(struct drm_i915_gem_request *waiter_req,
 	struct drm_i915_private *dev_priv = waiter->dev->dev_private;
 	int ret;
 
-	ret = intel_ring_begin(waiter, 4);
+	ret = intel_ring_begin(waiter_req, 4);
 	if (ret)
 		return ret;
 
@@ -1368,7 +1367,7 @@  gen6_ring_sync(struct drm_i915_gem_request *waiter_req,
 
 	WARN_ON(wait_mbox == MI_SEMAPHORE_SYNC_INVALID);
 
-	ret = intel_ring_begin(waiter, 4);
+	ret = intel_ring_begin(waiter_req, 4);
 	if (ret)
 		return ret;
 
@@ -1413,7 +1412,7 @@  pc_render_add_request(struct drm_i915_gem_request *req)
 	 * incoherence by flushing the 6 PIPE_NOTIFY buffers out to
 	 * memory before requesting an interrupt.
 	 */
-	ret = intel_ring_begin(ring, 32);
+	ret = intel_ring_begin(req, 32);
 	if (ret)
 		return ret;
 
@@ -1598,7 +1597,7 @@  bsd_ring_flush(struct drm_i915_gem_request *req,
 	struct intel_engine_cs *ring = req->ring;
 	int ret;
 
-	ret = intel_ring_begin(ring, 2);
+	ret = intel_ring_begin(req, 2);
 	if (ret)
 		return ret;
 
@@ -1614,7 +1613,7 @@  i9xx_add_request(struct drm_i915_gem_request *req)
 	struct intel_engine_cs *ring = req->ring;
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
@@ -1759,7 +1758,7 @@  i965_dispatch_execbuffer(struct drm_i915_gem_request *req,
 	struct intel_engine_cs *ring = req->ring;
 	int ret;
 
-	ret = intel_ring_begin(ring, 2);
+	ret = intel_ring_begin(req, 2);
 	if (ret)
 		return ret;
 
@@ -1787,7 +1786,7 @@  i830_dispatch_execbuffer(struct drm_i915_gem_request *req,
 	u32 cs_offset = ring->scratch.gtt_offset;
 	int ret;
 
-	ret = intel_ring_begin(ring, 6);
+	ret = intel_ring_begin(req, 6);
 	if (ret)
 		return ret;
 
@@ -1804,7 +1803,7 @@  i830_dispatch_execbuffer(struct drm_i915_gem_request *req,
 		if (len > I830_BATCH_LIMIT)
 			return -ENOSPC;
 
-		ret = intel_ring_begin(ring, 6 + 2);
+		ret = intel_ring_begin(req, 6 + 2);
 		if (ret)
 			return ret;
 
@@ -1827,7 +1826,7 @@  i830_dispatch_execbuffer(struct drm_i915_gem_request *req,
 		offset = cs_offset;
 	}
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
@@ -1849,7 +1848,7 @@  i915_dispatch_execbuffer(struct drm_i915_gem_request *req,
 	struct intel_engine_cs *ring = req->ring;
 	int ret;
 
-	ret = intel_ring_begin(ring, 2);
+	ret = intel_ring_begin(req, 2);
 	if (ret)
 		return ret;
 
@@ -2272,13 +2271,17 @@  static int __intel_ring_prepare(struct intel_engine_cs *ring, int bytes)
 	return 0;
 }
 
-int intel_ring_begin(struct intel_engine_cs *ring,
+int intel_ring_begin(struct drm_i915_gem_request *req,
 		     int num_dwords)
 {
-	struct drm_i915_gem_request *req;
-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct intel_engine_cs *ring;
+	struct drm_i915_private *dev_priv;
 	int ret;
 
+	WARN_ON(req == NULL);
+	ring = req->ring;
+	dev_priv = ring->dev->dev_private;
+
 	ret = i915_gem_check_wedge(&dev_priv->gpu_error,
 				   dev_priv->mm.interruptible);
 	if (ret)
@@ -2288,11 +2291,6 @@  int intel_ring_begin(struct intel_engine_cs *ring,
 	if (ret)
 		return ret;
 
-	/* Preallocate the olr before touching the ring */
-	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
-	if (ret)
-		return ret;
-
 	ring->buffer->space -= num_dwords * sizeof(uint32_t);
 	return 0;
 }
@@ -2308,7 +2306,7 @@  int intel_ring_cacheline_align(struct drm_i915_gem_request *req)
 		return 0;
 
 	num_dwords = CACHELINE_BYTES / sizeof(uint32_t) - num_dwords;
-	ret = intel_ring_begin(ring, num_dwords);
+	ret = intel_ring_begin(req, num_dwords);
 	if (ret)
 		return ret;
 
@@ -2378,7 +2376,7 @@  static int gen6_bsd_ring_flush(struct drm_i915_gem_request *req,
 	uint32_t cmd;
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
@@ -2425,7 +2423,7 @@  gen8_ring_dispatch_execbuffer(struct drm_i915_gem_request *req,
 			!(dispatch_flags & I915_DISPATCH_SECURE);
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
@@ -2447,7 +2445,7 @@  hsw_ring_dispatch_execbuffer(struct drm_i915_gem_request *req,
 	struct intel_engine_cs *ring = req->ring;
 	int ret;
 
-	ret = intel_ring_begin(ring, 2);
+	ret = intel_ring_begin(req, 2);
 	if (ret)
 		return ret;
 
@@ -2470,7 +2468,7 @@  gen6_ring_dispatch_execbuffer(struct drm_i915_gem_request *req,
 	struct intel_engine_cs *ring = req->ring;
 	int ret;
 
-	ret = intel_ring_begin(ring, 2);
+	ret = intel_ring_begin(req, 2);
 	if (ret)
 		return ret;
 
@@ -2495,7 +2493,7 @@  static int gen6_ring_flush(struct drm_i915_gem_request *req,
 	uint32_t cmd;
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(req, 4);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index bfeca53..16fd9ba 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -399,7 +399,7 @@  void intel_cleanup_ring_buffer(struct intel_engine_cs *ring);
 
 int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request);
 
-int __must_check intel_ring_begin(struct intel_engine_cs *ring, int n);
+int __must_check intel_ring_begin(struct drm_i915_gem_request *req, int n);
 int __must_check intel_ring_cacheline_align(struct drm_i915_gem_request *req);
 static inline void intel_ring_emit(struct intel_engine_cs *ring,
 				   u32 data)