Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> CC: Michel Thierry <michel.thierry@xxxxxxxxx>
> Cc: Jeff McGee <jeff.mcgee@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
