| Message ID | 20180509120419.6733-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Quoting Mika Kuoppala (2018-05-09 14:54:04) > 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. It doesn't, we rely on exclusivity that is provided by it is only us that controls the tasklet. The rationale comes from the caller. That we call flush here at all, is just that it helps avoid spinning, it is not required. If the race is too much, we just need the next line. > > + 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 */ Doesn't help much, that literally is what the code is doing but not why. Why we want that is the caller again. -Chris
Quoting Chris Wilson (2018-05-09 15:02:12) > Quoting Mika Kuoppala (2018-05-09 14:54:04) > > 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. > > It doesn't, we rely on exclusivity that is provided by it is only us > that controls the tasklet. The rationale comes from the caller. That we > call flush here at all, is just that it helps avoid spinning, it is not > required. If the race is too much, we just need the next line. Rather than ever worry, let's just kill it. -Chris
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); + + if (atomic_inc_return(&t->base.count) == 1) + tasklet_unlock_wait(&t->base); +} + +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); 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; }
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(-)