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@xxxxxxxxxxxxxxxxxx>
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@xxxxxxxxx>
Date: Thu Jul 24 17:04:23 2014 +0100
drm/i915/bdw: Generic logical ring init and cleanup
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
...
-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.
>From e2cc1e8f65c9b4bc465a3e1097de91d4bb6c13cd Mon Sep 17 00:00:00 2001
From: Dave Gordon <david.s.gordon@xxxxxxxxx>
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@xxxxxxxxx>
Date: Thu Jul 24 17:04:23 2014 +0100
drm/i915/bdw: Generic logical ring init and cleanup
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
---
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
