diff mbox series

drm/i915/execlists: Flush tasklet directly from reset-finish

Message ID 20180828152702.27536-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series drm/i915/execlists: Flush tasklet directly from reset-finish | expand

Commit Message

Chris Wilson Aug. 28, 2018, 3:27 p.m. UTC
On finishing the reset, the intention is to restart the GPU before we
relinquish the forcewake taken to handle the reset - the goal being the
GPU reloads a context before it is allowed to sleep. For this purpose,
we used tasklet_flush() which although it accomplished the goal of
restarting the GPU, carried with it a sting in its tail: it cleared the
TASKLET_STATE_SCHED bit. This meant that if another CPU queued a new
request to this engine, we would clear the flag and later attempt to
requeue the tasklet on the local CPU, breaking the per-cpu softirq
lists.

Remove the dangerous tasklet_kill() and just run the tasklet func
directly as we know it is safe to do so (the tasklets are internally
locked to allow mixed usage from direct submission).

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107710
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.h  |  6 ------
 drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++-----------
 2 files changed, 6 insertions(+), 17 deletions(-)

Comments

Mika Kuoppala Aug. 29, 2018, 12:38 p.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> On finishing the reset, the intention is to restart the GPU before we
> relinquish the forcewake taken to handle the reset - the goal being the
> GPU reloads a context before it is allowed to sleep. For this purpose,
> we used tasklet_flush() which although it accomplished the goal of
> restarting the GPU, carried with it a sting in its tail: it cleared the
> TASKLET_STATE_SCHED bit. This meant that if another CPU queued a new
> request to this engine, we would clear the flag and later attempt to
> requeue the tasklet on the local CPU, breaking the per-cpu softirq
> lists.
>
> Remove the dangerous tasklet_kill() and just run the tasklet func
> directly as we know it is safe to do so (the tasklets are internally
> locked to allow mixed usage from direct submission).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107710
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.h  |  6 ------
>  drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++-----------
>  2 files changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> index e46592956872..599c4f6eb1ea 100644
> --- a/drivers/gpu/drm/i915/i915_gem.h
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -82,12 +82,6 @@ static inline void __tasklet_disable_sync_once(struct tasklet_struct *t)
>  		tasklet_unlock_wait(t);
>  }
>  
> -static inline void __tasklet_enable_sync_once(struct tasklet_struct *t)
> -{
> -	if (atomic_dec_return(&t->count) == 0)
> -		tasklet_kill(t);
> -}
> -
>  static inline bool __tasklet_is_enabled(const struct tasklet_struct *t)
>  {
>  	return !atomic_read(&t->count);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 36050f085071..f8ceb9c99dd6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1962,21 +1962,16 @@ static void execlists_reset_finish(struct intel_engine_cs *engine)
>  {
>  	struct intel_engine_execlists * const execlists = &engine->execlists;
>  
> -	/* After a GPU reset, we may have requests to replay */
> -	if (!RB_EMPTY_ROOT(&execlists->queue.rb_root))
> -		tasklet_schedule(&execlists->tasklet);
> -
>  	/*
> -	 * Flush the tasklet while we still have the forcewake to be sure
> -	 * that it is not allowed to sleep before we restart and reload a
> -	 * context.
> +	 * After a GPU reset, we may have requests to replay. Do so now while
> +	 * we still have the forcewake to be sure that the GPU is not allowed
> +	 * to sleep before we restart and reload a context.
>  	 *
> -	 * As before (with execlists_reset_prepare) we rely on the caller
> -	 * serialising multiple attempts to reset so that we know that we
> -	 * are the only one manipulating tasklet state.
>  	 */
> -	__tasklet_enable_sync_once(&execlists->tasklet);
> +	if (!RB_EMPTY_ROOT(&execlists->queue.rb_root))
> +		execlists->tasklet.func(execlists->tasklet.data);

The enabling right after should be enough of a hint that
we can do this.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>  
> +	tasklet_enable(&execlists->tasklet);
>  	GEM_TRACE("%s: depth->%d\n", engine->name,
>  		  atomic_read(&execlists->tasklet.count));
>  }
> -- 
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Aug. 29, 2018, 12:57 p.m. UTC | #2
Quoting Mika Kuoppala (2018-08-29 13:38:27)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On finishing the reset, the intention is to restart the GPU before we
> > relinquish the forcewake taken to handle the reset - the goal being the
> > GPU reloads a context before it is allowed to sleep. For this purpose,
> > we used tasklet_flush() which although it accomplished the goal of
> > restarting the GPU, carried with it a sting in its tail: it cleared the
> > TASKLET_STATE_SCHED bit. This meant that if another CPU queued a new
> > request to this engine, we would clear the flag and later attempt to
> > requeue the tasklet on the local CPU, breaking the per-cpu softirq
> > lists.
> >
> > Remove the dangerous tasklet_kill() and just run the tasklet func
> > directly as we know it is safe to do so (the tasklets are internally
> > locked to allow mixed usage from direct submission).
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107710
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.h  |  6 ------
> >  drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++-----------
> >  2 files changed, 6 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> > index e46592956872..599c4f6eb1ea 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.h
> > +++ b/drivers/gpu/drm/i915/i915_gem.h
> > @@ -82,12 +82,6 @@ static inline void __tasklet_disable_sync_once(struct tasklet_struct *t)
> >               tasklet_unlock_wait(t);
> >  }
> >  
> > -static inline void __tasklet_enable_sync_once(struct tasklet_struct *t)
> > -{
> > -     if (atomic_dec_return(&t->count) == 0)
> > -             tasklet_kill(t);
> > -}
> > -
> >  static inline bool __tasklet_is_enabled(const struct tasklet_struct *t)
> >  {
> >       return !atomic_read(&t->count);
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 36050f085071..f8ceb9c99dd6 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1962,21 +1962,16 @@ static void execlists_reset_finish(struct intel_engine_cs *engine)
> >  {
> >       struct intel_engine_execlists * const execlists = &engine->execlists;
> >  
> > -     /* After a GPU reset, we may have requests to replay */
> > -     if (!RB_EMPTY_ROOT(&execlists->queue.rb_root))
> > -             tasklet_schedule(&execlists->tasklet);
> > -
> >       /*
> > -      * Flush the tasklet while we still have the forcewake to be sure
> > -      * that it is not allowed to sleep before we restart and reload a
> > -      * context.
> > +      * After a GPU reset, we may have requests to replay. Do so now while
> > +      * we still have the forcewake to be sure that the GPU is not allowed
> > +      * to sleep before we restart and reload a context.
> >        *
> > -      * As before (with execlists_reset_prepare) we rely on the caller
> > -      * serialising multiple attempts to reset so that we know that we
> > -      * are the only one manipulating tasklet state.
> >        */
> > -     __tasklet_enable_sync_once(&execlists->tasklet);
> > +     if (!RB_EMPTY_ROOT(&execlists->queue.rb_root))
> > +             execlists->tasklet.func(execlists->tasklet.data);
> 
> The enabling right after should be enough of a hint that
> we can do this.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

And that I think is the last outstanding reset bug. But there are a few
capture bugs if you are interested...
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index e46592956872..599c4f6eb1ea 100644
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -82,12 +82,6 @@  static inline void __tasklet_disable_sync_once(struct tasklet_struct *t)
 		tasklet_unlock_wait(t);
 }
 
-static inline void __tasklet_enable_sync_once(struct tasklet_struct *t)
-{
-	if (atomic_dec_return(&t->count) == 0)
-		tasklet_kill(t);
-}
-
 static inline bool __tasklet_is_enabled(const struct tasklet_struct *t)
 {
 	return !atomic_read(&t->count);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 36050f085071..f8ceb9c99dd6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1962,21 +1962,16 @@  static void execlists_reset_finish(struct intel_engine_cs *engine)
 {
 	struct intel_engine_execlists * const execlists = &engine->execlists;
 
-	/* After a GPU reset, we may have requests to replay */
-	if (!RB_EMPTY_ROOT(&execlists->queue.rb_root))
-		tasklet_schedule(&execlists->tasklet);
-
 	/*
-	 * Flush the tasklet while we still have the forcewake to be sure
-	 * that it is not allowed to sleep before we restart and reload a
-	 * context.
+	 * After a GPU reset, we may have requests to replay. Do so now while
+	 * we still have the forcewake to be sure that the GPU is not allowed
+	 * to sleep before we restart and reload a context.
 	 *
-	 * As before (with execlists_reset_prepare) we rely on the caller
-	 * serialising multiple attempts to reset so that we know that we
-	 * are the only one manipulating tasklet state.
 	 */
-	__tasklet_enable_sync_once(&execlists->tasklet);
+	if (!RB_EMPTY_ROOT(&execlists->queue.rb_root))
+		execlists->tasklet.func(execlists->tasklet.data);
 
+	tasklet_enable(&execlists->tasklet);
 	GEM_TRACE("%s: depth->%d\n", engine->name,
 		  atomic_read(&execlists->tasklet.count));
 }