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.ursu...@intel.com>
> >>
> >>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.ursu...@intel.com>
> >>Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
> >>---
> >> 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.
> 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.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
