[Intel-gfx] [PATCH] drm/i915/gt: execlists->active is serialised by the tasklet

[Tvrtko Ursulin]

On 09/10/2019 11:26, Chris Wilson wrote:
> The active/pending execlists is no longer protected by the
> engine->active.lock, but is serialised by the tasklet instead. Update
> the locking around the debug and stats to follow suit.
> 
> Fixes: df403069029d ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> Wrap the tasklet_lock inside execlists_active_lock() to help clarify
> what it is meant to be protecting.
> ---
>   drivers/gpu/drm/i915/gt/intel_engine.h    | 12 ++++++++++++
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c | 16 +++++++---------
>   drivers/gpu/drm/i915/i915_gem.h           |  6 ++++++
>   3 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index d624752f2a92..c99910c4eeb9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -136,6 +136,18 @@ execlists_active(const struct intel_engine_execlists *execlists)
>   	return READ_ONCE(*execlists->active);
>   }
>   
> +static inline void
> +execlists_active_lock(struct intel_engine_execlists *execlists)
> +{
> +	tasklet_lock(&execlists->tasklet);
> +}
> +
> +static inline void
> +execlists_active_unlock(struct intel_engine_execlists *execlists)
> +{
> +	tasklet_unlock(&execlists->tasklet);
> +}

After we stop preventing the tasklet from running should we maybe kick 
ksoftirqd? I am thinking if a tasklet gets scheduled and ran during us 
holding the lock here, it won't lose the "scheduled" status, but not 
sure at what next opportunity it would get re-run.

> +
>   struct i915_request *
>   execlists_unwind_incomplete_requests(struct intel_engine_execlists *execlists);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 5aa1371f6a0f..45b708fc4b52 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1245,9 +1245,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine,
>   					 struct drm_printer *m)
>   {
>   	struct drm_i915_private *dev_priv = engine->i915;
> -	const struct intel_engine_execlists * const execlists =
> -		&engine->execlists;
> -	unsigned long flags;
> +	struct intel_engine_execlists * const execlists = &engine->execlists;
>   	u64 addr;
>   
>   	if (engine->id == RENDER_CLASS && IS_GEN_RANGE(dev_priv, 4, 7))
> @@ -1329,7 +1327,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine,
>   				   idx, hws[idx * 2], hws[idx * 2 + 1]);
>   		}
>   
> -		spin_lock_irqsave(&engine->active.lock, flags);
> +		execlists_active_lock(execlists);
>   		for (port = execlists->active; (rq = *port); port++) {
>   			char hdr[80];
>   			int len;
> @@ -1367,7 +1365,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine,
>   			if (tl)
>   				intel_timeline_put(tl);
>   		}
> -		spin_unlock_irqrestore(&engine->active.lock, flags);
> +		execlists_active_unlock(execlists);
>   	} else if (INTEL_GEN(dev_priv) > 6) {
>   		drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
>   			   ENGINE_READ(engine, RING_PP_DIR_BASE));
> @@ -1509,8 +1507,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
>   	if (!intel_engine_supports_stats(engine))
>   		return -ENODEV;
>   
> -	spin_lock_irqsave(&engine->active.lock, flags);
> -	write_seqlock(&engine->stats.lock);
> +	execlists_active_lock(execlists);
> +	write_seqlock_irqsave(&engine->stats.lock, flags);
>   
>   	if (unlikely(engine->stats.enabled == ~0)) {
>   		err = -EBUSY;
> @@ -1538,8 +1536,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
>   	}
>   
>   unlock:
> -	write_sequnlock(&engine->stats.lock);
> -	spin_unlock_irqrestore(&engine->active.lock, flags);
> +	write_sequnlock_irqrestore(&engine->stats.lock, flags);
> +	execlists_active_unlock(execlists);
>   
>   	return err;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> index 167a7b56ed5b..6795f1daa3d5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.h
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -77,6 +77,12 @@ struct drm_i915_private;
>   
>   #define I915_GEM_IDLE_TIMEOUT (HZ / 5)
>   
> +static inline void tasklet_lock(struct tasklet_struct *t)
> +{
> +	while (!tasklet_trylock(t))
> +		cpu_relax();
> +}
> +
>   static inline void __tasklet_disable_sync_once(struct tasklet_struct *t)
>   {
>   	if (!atomic_fetch_inc(&t->count))
> 

Regards,

Tvrtko