On 17/08/16 10:41, Chris Wilson wrote:
On Wed, Aug 17, 2016 at 10:34:18AM +0100, Tvrtko Ursulin wrote:
On 16/08/16 17:18, Chris Wilson wrote:
On Tue, Aug 16, 2016 at 05:04:21PM +0100, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Build the legacy semaphore initialisation array using the engine
hardware ids instead of driver internal ones. This makes the
static array size dependent only on the number of gen6 semaphore
engines.
Also makes the per-engine semaphore wait and signal tables
hardware id indexed saving some more space.
v2: Refactor I915_GEN6_NUM_ENGINES to GEN6_SEMAPHORE_LAST. (Chris Wilson)
v3: More polish. (Chris Wilson)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++----------------
drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++--
2 files changed, 34 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index fa22bd87bab0..12703ea27259 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1337,8 +1337,7 @@ static int gen6_signal(struct drm_i915_gem_request *req)
{
struct intel_ring *ring = req->ring;
struct drm_i915_private *dev_priv = req->i915;
- struct intel_engine_cs *useless;
- enum intel_engine_id id;
+ struct intel_engine_cs *engine;
int ret, num_rings;
num_rings = INTEL_INFO(dev_priv)->num_rings;
@@ -1346,9 +1345,13 @@ static int gen6_signal(struct drm_i915_gem_request *req)
if (ret)
return ret;
- for_each_engine_id(useless, dev_priv, id) {
- i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
+ for_each_engine(engine, dev_priv) {
+ i915_reg_t mbox_reg;
+
+ if (!(BIT(engine->hw_id) & GEN6_SEMAPHORES_MASK))
+ continue;
Later on, it would be nice if this used
for_each_engine_masked()
instead (presupposing we have an efficient iterator for a sparse mask).
The issue is in defining GEN6_SEMAPHORES_MASK
#define GEN6_SEMAPHORES_MASK \
(RENDER_RING |
BSD_RING |
BSD2_RING |
BLT_RING |
VECS_RING)
Defnitely pencil that in for when otherwise we would iterate over 10
empty slots in dev_priv->engines[].
I'm thinking that we should make for_each_engine_mask() the default, say
for_each_engine(engine, dev_priv, ALL_ENGINES, id)
Or add an initialized engine array to dev_priv, in addition to the
existing map for best of both worlds.
We have the ring_mask which already tells us that mapping, so I think
the second array is overkill.
Yes, I said "in addition to the existing map". In addition we could have
an array of only initialized engines to avoid any skipping at runtime.
Since iterators end with intel_engine_initialized anyway.
That would prevent churn in for_each_engine users.
Or just give up and add something smarter, like a typdef
intel_engine_it, to each of them and be done with churn once for
all.
But we said there aren't any iterators on fast paths anyway so I
would rather we chose to do nothing. :)
gen6_semaphore_signal is called uncomfortably often, I'd be tempted to
keep it trim. We should also contemplate delayed semaphore signaling
again.
The list/array idea would solve that AFAICT.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
