Re: [PATCH v3 2/5] drm/i915: Combine tasklet_kill and tasklet_disable

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Ideally, we want to atomically flush and disable the tasklet before
> resetting the GPU. At present, we rely on being the only part to touch
> our tasklet and serialisation of the reset process to ensure that we can
> suspend the tasklet from the mix of reset/wedge pathways. In this patch,
> we move the tasklet abuse into its own function and tweak it such that
> we only do a synchronous operation the first time it is disabled around
> the reset. This allows us to avoid the sync inside a softirq context in
> subsequent patches.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> CC: Michel Thierry <michel.thierry@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c        |  2 --
>  drivers/gpu/drm/i915/i915_tasklet.h    | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_engine_cs.c |  4 ++--
>  3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 98481e150e61..8d27e78b052c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3043,8 +3043,6 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)
>  	 * common case of recursively being called from set-wedged from inside
>  	 * i915_reset.
>  	 */
> -	if (i915_tasklet_is_enabled(&engine->execlists.tasklet))
> -		i915_tasklet_flush(&engine->execlists.tasklet);
>  	i915_tasklet_disable(&engine->execlists.tasklet);
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/i915_tasklet.h b/drivers/gpu/drm/i915/i915_tasklet.h
> index c9f41a5ebb91..d8263892f385 100644
> --- a/drivers/gpu/drm/i915/i915_tasklet.h
> +++ b/drivers/gpu/drm/i915/i915_tasklet.h
> @@ -59,10 +59,24 @@ static inline void i915_tasklet_enable(struct i915_tasklet *t)
>  }
>  
>  static inline void i915_tasklet_disable(struct i915_tasklet *t)
> +{
> +	if (i915_tasklet_is_enabled(t))
> +		i915_tasklet_flush(t);

This needs a comment to explain how we can get away with
the race.

> +
> +	if (atomic_inc_return(&t->base.count) == 1)
> +		tasklet_unlock_wait(&t->base);

I would add comment in here too.
/* No need to sync on further disables */

-Mika

> +}
> +
> +static inline void i915_tasklet_lock(struct i915_tasklet *t)
>  {
>  	tasklet_disable(&t->base);
>  }
>  
> +static inline void i915_tasklet_unlock(struct i915_tasklet *t)
> +{
> +	tasklet_enable(&t->base);
> +}
> +
>  static inline void i915_tasklet_set_func(struct i915_tasklet *t,
>  					 void (*func)(unsigned long data),
>  					 unsigned long data)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 5f1118ea96d8..3c8a0c3245f3 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1478,7 +1478,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
>  	if (!intel_engine_supports_stats(engine))
>  		return -ENODEV;
>  
> -	i915_tasklet_disable(&execlists->tasklet);
> +	i915_tasklet_lock(&execlists->tasklet);

After the initial shock, the *lock starts to fit.
-Mika

>  	write_seqlock_irqsave(&engine->stats.lock, flags);
>  
>  	if (unlikely(engine->stats.enabled == ~0)) {
> @@ -1504,7 +1504,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
>  
>  unlock:
>  	write_sequnlock_irqrestore(&engine->stats.lock, flags);
> -	i915_tasklet_enable(&execlists->tasklet);
> +	i915_tasklet_unlock(&execlists->tasklet);
>  
>  	return err;
>  }
> -- 
> 2.17.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx