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

Submitted by John.C.Harrison@Intel.com on May 29, 2015, 4:44 p.m.

Details

Message ID 1432917856-12261-47-git-send-email-John.C.Harrison@Intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

John.C.Harrison@Intel.com May 29, 2015, 4:44 p.m.
From: John Harrison <John.C.Harrison@Intel.com>

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 <John.C.Harrison@Intel.com>
Reviewed-by: Tomas Elf <tomas.elf@intel.com>
---
 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(-)

Patch hide | download patch | download mbox

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)

Comments

On Fri, May 29, 2015 at 05:44:07PM +0100, John.C.Harrison@Intel.com 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
On 23/06/2015 11:24, Chris Wilson wrote:
> On Fri, May 29, 2015 at 05:44:07PM +0100, John.C.Harrison@Intel.com 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.
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, John.C.Harrison@Intel.com 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
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, John.C.Harrison@Intel.com 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.
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, John.C.Harrison@Intel.com 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