| 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 |
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
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 --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)); }
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(-)