Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

Chris Wilson
2016-08-16T16:19:00Z

On Tue, Aug 16, 2016 at 05:04:21PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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.ursulin@intel.com>
> Reviewed-by: Chris Wilson <chris@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)
-Chris

--
Chris Wilson, Intel Open Source Technology Centre