| Message ID | 20191013193115.16844-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/execlists: Assert tasklet is locked for process_csb() | expand |
On 13/10/2019 20:31, Chris Wilson wrote: > We rely on only the tasklet being allowed to call into process_csb(), so > assert that is locked when we do. As the tasklet uses a simple bitlock, > there is no strong lockdep checking so we must make do with a plain > assertion that the tasklet is running and assume that we are the > tasklet! > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_lrc.c | 1 + > drivers/gpu/drm/i915/i915_gem.h | 5 +++++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index 8be9e69d5718..ab20433182d1 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -1984,6 +1984,7 @@ static void process_csb(struct intel_engine_cs *engine) > u8 head, tail; > > GEM_BUG_ON(USES_GUC_SUBMISSION(engine->i915)); > + GEM_BUG_ON(!tasklet_is_locked(&execlists->tasklet)); I see some reset paths calling process_csb which looks like a problem for this assert. Regards, Tvrtko > > /* > * Note that csb_write, csb_status may be either in HWSP or mmio. > diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h > index db20d2b0842b..f6f9675848b8 100644 > --- a/drivers/gpu/drm/i915/i915_gem.h > +++ b/drivers/gpu/drm/i915/i915_gem.h > @@ -86,6 +86,11 @@ static inline void tasklet_lock(struct tasklet_struct *t) > cpu_relax(); > } > > +static inline bool tasklet_is_locked(const struct tasklet_struct *t) > +{ > + return test_bit(TASKLET_STATE_RUN, &t->state); > +} > + > static inline void __tasklet_disable_sync_once(struct tasklet_struct *t) > { > if (!atomic_fetch_inc(&t->count)) >
Quoting Tvrtko Ursulin (2019-10-14 10:25:04) > > On 13/10/2019 20:31, Chris Wilson wrote: > > We rely on only the tasklet being allowed to call into process_csb(), so > > assert that is locked when we do. As the tasklet uses a simple bitlock, > > there is no strong lockdep checking so we must make do with a plain > > assertion that the tasklet is running and assume that we are the > > tasklet! > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_lrc.c | 1 + > > drivers/gpu/drm/i915/i915_gem.h | 5 +++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > > index 8be9e69d5718..ab20433182d1 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > @@ -1984,6 +1984,7 @@ static void process_csb(struct intel_engine_cs *engine) > > u8 head, tail; > > > > GEM_BUG_ON(USES_GUC_SUBMISSION(engine->i915)); > > + GEM_BUG_ON(!tasklet_is_locked(&execlists->tasklet)); > > I see some reset paths calling process_csb which looks like a problem > for this assert. Oh, no it's not :) execlists_reset() is surrounded by reset_prepare and reset_finish who are responsible for disabling the tasklet and serialising with direct submission around the reset. -Chris
Quoting Chris Wilson (2019-10-14 10:27:59) > Quoting Tvrtko Ursulin (2019-10-14 10:25:04) > > > > On 13/10/2019 20:31, Chris Wilson wrote: > > > We rely on only the tasklet being allowed to call into process_csb(), so > > > assert that is locked when we do. As the tasklet uses a simple bitlock, > > > there is no strong lockdep checking so we must make do with a plain > > > assertion that the tasklet is running and assume that we are the > > > tasklet! > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > --- > > > drivers/gpu/drm/i915/gt/intel_lrc.c | 1 + > > > drivers/gpu/drm/i915/i915_gem.h | 5 +++++ > > > 2 files changed, 6 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > > > index 8be9e69d5718..ab20433182d1 100644 > > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > > @@ -1984,6 +1984,7 @@ static void process_csb(struct intel_engine_cs *engine) > > > u8 head, tail; > > > > > > GEM_BUG_ON(USES_GUC_SUBMISSION(engine->i915)); > > > + GEM_BUG_ON(!tasklet_is_locked(&execlists->tasklet)); > > > > I see some reset paths calling process_csb which looks like a problem > > for this assert. > > Oh, no it's not :) > > execlists_reset() is surrounded by reset_prepare and reset_finish who > are responsible for disabling the tasklet and serialising with direct > submission around the reset. Same being true for cancel_requests, on wedging we have to disable the tasklet before taking control of the state. -Chris
Quoting Chris Wilson (2019-10-14 10:28:53) > Quoting Chris Wilson (2019-10-14 10:27:59) > > Quoting Tvrtko Ursulin (2019-10-14 10:25:04) > > > > > > On 13/10/2019 20:31, Chris Wilson wrote: > > > > We rely on only the tasklet being allowed to call into process_csb(), so > > > > assert that is locked when we do. As the tasklet uses a simple bitlock, > > > > there is no strong lockdep checking so we must make do with a plain > > > > assertion that the tasklet is running and assume that we are the > > > > tasklet! > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/gt/intel_lrc.c | 1 + > > > > drivers/gpu/drm/i915/i915_gem.h | 5 +++++ > > > > 2 files changed, 6 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > > > > index 8be9e69d5718..ab20433182d1 100644 > > > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > > > @@ -1984,6 +1984,7 @@ static void process_csb(struct intel_engine_cs *engine) > > > > u8 head, tail; > > > > > > > > GEM_BUG_ON(USES_GUC_SUBMISSION(engine->i915)); > > > > + GEM_BUG_ON(!tasklet_is_locked(&execlists->tasklet)); > > > > > > I see some reset paths calling process_csb which looks like a problem > > > for this assert. > > > > Oh, no it's not :) > > > > execlists_reset() is surrounded by reset_prepare and reset_finish who > > are responsible for disabling the tasklet and serialising with direct > > submission around the reset. > > Same being true for cancel_requests, on wedging we have to disable the > tasklet before taking control of the state. Hmm. That being said we do have that sanitize path which cuts a few corners. -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 8be9e69d5718..ab20433182d1 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1984,6 +1984,7 @@ static void process_csb(struct intel_engine_cs *engine) u8 head, tail; GEM_BUG_ON(USES_GUC_SUBMISSION(engine->i915)); + GEM_BUG_ON(!tasklet_is_locked(&execlists->tasklet)); /* * Note that csb_write, csb_status may be either in HWSP or mmio. diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h index db20d2b0842b..f6f9675848b8 100644 --- a/drivers/gpu/drm/i915/i915_gem.h +++ b/drivers/gpu/drm/i915/i915_gem.h @@ -86,6 +86,11 @@ static inline void tasklet_lock(struct tasklet_struct *t) cpu_relax(); } +static inline bool tasklet_is_locked(const struct tasklet_struct *t) +{ + return test_bit(TASKLET_STATE_RUN, &t->state); +} + static inline void __tasklet_disable_sync_once(struct tasklet_struct *t) { if (!atomic_fetch_inc(&t->count))
We rely on only the tasklet being allowed to call into process_csb(), so assert that is locked when we do. As the tasklet uses a simple bitlock, there is no strong lockdep checking so we must make do with a plain assertion that the tasklet is running and assume that we are the tasklet! Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/gt/intel_lrc.c | 1 + drivers/gpu/drm/i915/i915_gem.h | 5 +++++ 2 files changed, 6 insertions(+)