Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Dave Gordon <david.s.gordon@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

On 02/12/15 14:58, Chris Wilson wrote:
> On Wed, Dec 02, 2015 at 02:51:17PM +0000, Dave Gordon wrote:
>> Or, put the active ones on a linked list, or keep a bitmask of which
>> ones have been initialised inside the dev_priv structure, so you
>> don't have to even dereference the engine[] array to work out
>> whether a particular engine is initialised. Apropos which, wouldn't
>> it be much more efficient to do that, because
>> intel_ring_initialized() is quite heavyweight and the results surely
>> don't change often, if at all, during normal operation. So we should
>> only evaluate it when something has changed, and cache the bool
>> result for use in all those for_each() loops!
>
> commit b53cd50c19f9d3c6f3308165b3e26c47b19dd041
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Mar 31 00:25:20 2015 +0100
>
>      drm/i915: intel_ring_initialized() must be simple and inline
>
>      Fixes regression from
>      commit 48d823878d64f93163f5a949623346748bbce1b4
>      Author: Oscar Mateo <oscar.mateo@intel.com>
>      Date:   Thu Jul 24 17:04:23 2014 +0100
>
>          drm/i915/bdw: Generic logical ring init and cleanup
>
>      Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> ...
>
> -bool intel_ring_initialized(struct intel_engine_cs *ring);
> +static inline bool
> +intel_ring_initialized(struct intel_engine_cs *ring)
> +{
> +       return ring->dev != NULL;
> +}
>
> Just waiting to clear a massive backlog.
>
> If we can use an array rather than a list (and a static assignment
> certainly qualifies), use an array.
> -Chris

Aha! That looks useful, but it didn't apply cleanly, so I've reworked it 
somewhat. Here's the patch (UNTESTED) for your comments ...

.Dave.

[-- Attachment #2: 0001-drm-i915-intel_ring_initialized-must-be-simple-and-i.patch --]
[-- Type: text/x-patch, Size: 6019 bytes --]

>From e2cc1e8f65c9b4bc465a3e1097de91d4bb6c13cd Mon Sep 17 00:00:00 2001
From: Dave Gordon <david.s.gordon@intel.com>
Date: Wed, 2 Dec 2015 17:58:33 +0000
Subject: [RFC] drm/i915: intel_ring_initialized() must be simple and inline
Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ

Based on Chris Wilson's patch from 6 months ago, rebased and adapted.

The idea is to use ring->dev as an indicator showing which engines have
been initialised and are therefore to be included in iterations that use
for_each_ring(). This allows us to avoid multiple memory references and
a (non-inlined) function call on each iteration of each such loop.

This version differs from Chris' primarily in the error cleanup paths,
where initialisation has failed and we therefore want to mark an engine
as NOT initialised. I have made the ring_cleanup() functions callable from
the failure path of the ring_init() code, rather than duplicating all the
steps to tear down a partially-constructed state. This also increases
symmetry; ring->dev is set at the start of ring_init, and cleared at the
end of ring_cleanup, in both the normal and error cases.

	Fixes regression from
	commit 48d823878d64f93163f5a949623346748bbce1b4
	Author: Oscar Mateo <oscar.mateo@intel.com>
	Date:   Thu Jul 24 17:04:23 2014 +0100

	    drm/i915/bdw: Generic logical ring init and cleanup

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 17 +++++++++-----
 drivers/gpu/drm/i915/intel_ringbuffer.c | 39 +++++++++++----------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  6 ++++-
 3 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 4ebafab..7644c48 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1894,8 +1894,10 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 
 	dev_priv = ring->dev->dev_private;
 
-	intel_logical_ring_stop(ring);
-	WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
+	if (ring->buffer) {
+		intel_logical_ring_stop(ring);
+		WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
+	}
 
 	if (ring->cleanup)
 		ring->cleanup(ring);
@@ -1909,6 +1911,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 	}
 
 	lrc_destroy_wa_ctx_obj(ring);
+	ring->dev = NULL;
 }
 
 static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
@@ -1931,11 +1934,11 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 
 	ret = i915_cmd_parser_init_ring(ring);
 	if (ret)
-		return ret;
+		goto error;
 
 	ret = intel_lr_context_deferred_alloc(ring->default_context, ring);
 	if (ret)
-		return ret;
+		goto error;
 
 	/* As this is the default context, always pin it */
 	ret = intel_lr_context_do_pin(
@@ -1946,9 +1949,13 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 		DRM_ERROR(
 			"Failed to pin and map ringbuffer %s: %d\n",
 			ring->name, ret);
-		return ret;
+		goto error;
 	}
 
+	return 0;
+
+error:
+	intel_logical_ring_cleanup(ring);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 57d78f2..921c8a6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -33,23 +33,6 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-bool
-intel_ring_initialized(struct intel_engine_cs *ring)
-{
-	struct drm_device *dev = ring->dev;
-
-	if (!dev)
-		return false;
-
-	if (i915.enable_execlists) {
-		struct intel_context *dctx = ring->default_context;
-		struct intel_ringbuffer *ringbuf = dctx->engine[ring->id].ringbuf;
-
-		return ringbuf->obj;
-	} else
-		return ring->buffer && ring->buffer->obj;
-}
-
 int __intel_ring_space(int head, int tail, int size)
 {
 	int space = head - tail;
@@ -2167,8 +2150,10 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	init_waitqueue_head(&ring->irq_queue);
 
 	ringbuf = intel_engine_create_ringbuffer(ring, 32 * PAGE_SIZE);
-	if (IS_ERR(ringbuf))
-		return PTR_ERR(ringbuf);
+	if (IS_ERR(ringbuf)) {
+		ret = PTR_ERR(ringbuf);
+		goto error;
+	}
 	ring->buffer = ringbuf;
 
 	if (I915_NEED_GFX_HWS(dev)) {
@@ -2197,8 +2182,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	return 0;
 
 error:
-	intel_ringbuffer_free(ringbuf);
-	ring->buffer = NULL;
+	intel_cleanup_ring_buffer(ring);
 	return ret;
 }
 
@@ -2211,12 +2195,14 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 
 	dev_priv = to_i915(ring->dev);
 
-	intel_stop_ring_buffer(ring);
-	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
+	if (ring->buffer) {
+		intel_stop_ring_buffer(ring);
+		WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
-	intel_unpin_ringbuffer_obj(ring->buffer);
-	intel_ringbuffer_free(ring->buffer);
-	ring->buffer = NULL;
+		intel_unpin_ringbuffer_obj(ring->buffer);
+		intel_ringbuffer_free(ring->buffer);
+		ring->buffer = NULL;
+	}
 
 	if (ring->cleanup)
 		ring->cleanup(ring);
@@ -2225,6 +2211,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 
 	i915_cmd_parser_fini_ring(ring);
 	i915_gem_batch_pool_fini(&ring->batch_pool);
+	ring->dev = NULL;
 }
 
 static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 5d1eb20..49574ff 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -350,7 +350,11 @@ struct  intel_engine_cs {
 	u32 (*get_cmd_length_mask)(u32 cmd_header);
 };
 
-bool intel_ring_initialized(struct intel_engine_cs *ring);
+static inline bool
+intel_ring_initialized(struct intel_engine_cs *ring)
+{
+	return ring->dev != NULL;
+}
 
 static inline unsigned
 intel_ring_flag(struct intel_engine_cs *ring)
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx