| Message ID | 20180516064741.30912-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris Wilson <chris@chris-wilson.co.uk> writes: > When setting up reset, we may need to recursively prepare an engine. In > which case we should only synchronously flush the tasklets on the outer > most call, the inner calls will then be inside an atomic section where > the tasklet will never be run (and so the sync will never complete). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 +- > drivers/gpu/drm/i915/i915_gem.h | 7 +++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 0dc369a9ec4d..aab1f5e77ae9 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3036,7 +3036,7 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine) > * Turning off the execlists->tasklet until the reset is over > * prevents the race. > */ > - tasklet_disable(&engine->execlists.tasklet); > + __tasklet_disable_once(&engine->execlists.tasklet); It is debatable that could the naming be improved as the 'once' is tied to the disable now. __tasklet_disable_wait_[once|first]. I was going to insist on adding a comment tho, but there it is already above, explaining the recursion from set wedged. Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > /* > * We're using worker to queue preemption requests from the tasklet in > diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h > index 525920404ede..af033e7c1c56 100644 > --- a/drivers/gpu/drm/i915/i915_gem.h > +++ b/drivers/gpu/drm/i915/i915_gem.h > @@ -26,6 +26,7 @@ > #define __I915_GEM_H__ > > #include <linux/bug.h> > +#include <linux/interrupt.h> > > struct drm_i915_private; > > @@ -72,4 +73,10 @@ struct drm_i915_private; > void i915_gem_park(struct drm_i915_private *i915); > void i915_gem_unpark(struct drm_i915_private *i915); > > +static inline void __tasklet_disable_once(struct tasklet_struct *t) > +{ > + if (atomic_inc_return(&t->count) == 1) > + tasklet_unlock_wait(t); > +} > + > #endif /* __I915_GEM_H__ */ > -- > 2.17.0
Quoting Mika Kuoppala (2018-05-16 15:24:56) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > When setting up reset, we may need to recursively prepare an engine. In > > which case we should only synchronously flush the tasklets on the outer > > most call, the inner calls will then be inside an atomic section where > > the tasklet will never be run (and so the sync will never complete). > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 2 +- > > drivers/gpu/drm/i915/i915_gem.h | 7 +++++++ > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 0dc369a9ec4d..aab1f5e77ae9 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3036,7 +3036,7 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine) > > * Turning off the execlists->tasklet until the reset is over > > * prevents the race. > > */ > > - tasklet_disable(&engine->execlists.tasklet); > > + __tasklet_disable_once(&engine->execlists.tasklet); > > It is debatable that could the naming be improved as > the 'once' is tied to the disable now. > > __tasklet_disable_wait_[once|first]. wait_once is a good suggestion tasklet_disable tasklet_disable_nosync tasklet_disable_sync_once ? -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 0dc369a9ec4d..aab1f5e77ae9 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3036,7 +3036,7 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine) * Turning off the execlists->tasklet until the reset is over * prevents the race. */ - tasklet_disable(&engine->execlists.tasklet); + __tasklet_disable_once(&engine->execlists.tasklet); /* * We're using worker to queue preemption requests from the tasklet in diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h index 525920404ede..af033e7c1c56 100644 --- a/drivers/gpu/drm/i915/i915_gem.h +++ b/drivers/gpu/drm/i915/i915_gem.h @@ -26,6 +26,7 @@ #define __I915_GEM_H__ #include <linux/bug.h> +#include <linux/interrupt.h> struct drm_i915_private; @@ -72,4 +73,10 @@ struct drm_i915_private; void i915_gem_park(struct drm_i915_private *i915); void i915_gem_unpark(struct drm_i915_private *i915); +static inline void __tasklet_disable_once(struct tasklet_struct *t) +{ + if (atomic_inc_return(&t->count) == 1) + tasklet_unlock_wait(t); +} + #endif /* __I915_GEM_H__ */
When setting up reset, we may need to recursively prepare an engine. In which case we should only synchronously flush the tasklets on the outer most call, the inner calls will then be inside an atomic section where the tasklet will never be run (and so the sync will never complete). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/i915_gem.h | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-)