| Message ID | 20191009102622.32681-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: execlists->active is serialised by the tasklet | expand |
On 09/10/2019 11:26, Chris Wilson wrote: > The active/pending execlists is no longer protected by the > engine->active.lock, but is serialised by the tasklet instead. Update > the locking around the debug and stats to follow suit. > > Fixes: df403069029d ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > Wrap the tasklet_lock inside execlists_active_lock() to help clarify > what it is meant to be protecting. > --- > drivers/gpu/drm/i915/gt/intel_engine.h | 12 ++++++++++++ > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 16 +++++++--------- > drivers/gpu/drm/i915/i915_gem.h | 6 ++++++ > 3 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h > index d624752f2a92..c99910c4eeb9 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h > @@ -136,6 +136,18 @@ execlists_active(const struct intel_engine_execlists *execlists) > return READ_ONCE(*execlists->active); > } > > +static inline void > +execlists_active_lock(struct intel_engine_execlists *execlists) > +{ > + tasklet_lock(&execlists->tasklet); > +} > + > +static inline void > +execlists_active_unlock(struct intel_engine_execlists *execlists) > +{ > + tasklet_unlock(&execlists->tasklet); > +} After we stop preventing the tasklet from running should we maybe kick ksoftirqd? I am thinking if a tasklet gets scheduled and ran during us holding the lock here, it won't lose the "scheduled" status, but not sure at what next opportunity it would get re-run. > + > struct i915_request * > execlists_unwind_incomplete_requests(struct intel_engine_execlists *execlists); > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > index 5aa1371f6a0f..45b708fc4b52 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c > @@ -1245,9 +1245,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, > struct drm_printer *m) > { > struct drm_i915_private *dev_priv = engine->i915; > - const struct intel_engine_execlists * const execlists = > - &engine->execlists; > - unsigned long flags; > + struct intel_engine_execlists * const execlists = &engine->execlists; > u64 addr; > > if (engine->id == RENDER_CLASS && IS_GEN_RANGE(dev_priv, 4, 7)) > @@ -1329,7 +1327,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, > idx, hws[idx * 2], hws[idx * 2 + 1]); > } > > - spin_lock_irqsave(&engine->active.lock, flags); > + execlists_active_lock(execlists); > for (port = execlists->active; (rq = *port); port++) { > char hdr[80]; > int len; > @@ -1367,7 +1365,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, > if (tl) > intel_timeline_put(tl); > } > - spin_unlock_irqrestore(&engine->active.lock, flags); > + execlists_active_unlock(execlists); > } else if (INTEL_GEN(dev_priv) > 6) { > drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n", > ENGINE_READ(engine, RING_PP_DIR_BASE)); > @@ -1509,8 +1507,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) > if (!intel_engine_supports_stats(engine)) > return -ENODEV; > > - spin_lock_irqsave(&engine->active.lock, flags); > - write_seqlock(&engine->stats.lock); > + execlists_active_lock(execlists); > + write_seqlock_irqsave(&engine->stats.lock, flags); > > if (unlikely(engine->stats.enabled == ~0)) { > err = -EBUSY; > @@ -1538,8 +1536,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) > } > > unlock: > - write_sequnlock(&engine->stats.lock); > - spin_unlock_irqrestore(&engine->active.lock, flags); > + write_sequnlock_irqrestore(&engine->stats.lock, flags); > + execlists_active_unlock(execlists); > > return err; > } > diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h > index 167a7b56ed5b..6795f1daa3d5 100644 > --- a/drivers/gpu/drm/i915/i915_gem.h > +++ b/drivers/gpu/drm/i915/i915_gem.h > @@ -77,6 +77,12 @@ struct drm_i915_private; > > #define I915_GEM_IDLE_TIMEOUT (HZ / 5) > > +static inline void tasklet_lock(struct tasklet_struct *t) > +{ > + while (!tasklet_trylock(t)) > + cpu_relax(); > +} > + > static inline void __tasklet_disable_sync_once(struct tasklet_struct *t) > { > if (!atomic_fetch_inc(&t->count)) > Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-10-09 16:53:53) > > On 09/10/2019 11:26, Chris Wilson wrote: > > +static inline void > > +execlists_active_lock(struct intel_engine_execlists *execlists) > > +{ > > + tasklet_lock(&execlists->tasklet); > > +} > > + > > +static inline void > > +execlists_active_unlock(struct intel_engine_execlists *execlists) > > +{ > > + tasklet_unlock(&execlists->tasklet); > > +} > > After we stop preventing the tasklet from running should we maybe kick > ksoftirqd? I am thinking if a tasklet gets scheduled and ran during us > holding the lock here, it won't lose the "scheduled" status, but not > sure at what next opportunity it would get re-run. If we call tasklet_schedule() while we hold tasklet_lock, ksoftirqd (on another thread, hmm, we need local_bh_disable() to stop ourselves entering the softirq processing), then that tasklet_action will spin on tasklet_trylock. -Chris
On 09/10/2019 16:59, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-10-09 16:53:53) >> >> On 09/10/2019 11:26, Chris Wilson wrote: >>> +static inline void >>> +execlists_active_lock(struct intel_engine_execlists *execlists) >>> +{ >>> + tasklet_lock(&execlists->tasklet); >>> +} >>> + >>> +static inline void >>> +execlists_active_unlock(struct intel_engine_execlists *execlists) >>> +{ >>> + tasklet_unlock(&execlists->tasklet); >>> +} >> >> After we stop preventing the tasklet from running should we maybe kick >> ksoftirqd? I am thinking if a tasklet gets scheduled and ran during us >> holding the lock here, it won't lose the "scheduled" status, but not >> sure at what next opportunity it would get re-run. > > If we call tasklet_schedule() while we hold tasklet_lock, ksoftirqd (on > another thread, hmm, we need local_bh_disable() to stop ourselves > entering the softirq processing), then that tasklet_action will spin on > tasklet_trylock. I don't see it spinning, I see it unlinking the tasklet is trylock fails and going onto the next one. So even what I implied before seems wrong - it doesn't look like it would get re-run on next tasklet processing run. Where do you see the retry? Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-10-09 17:37:42) > > On 09/10/2019 16:59, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-10-09 16:53:53) > >> > >> On 09/10/2019 11:26, Chris Wilson wrote: > >>> +static inline void > >>> +execlists_active_lock(struct intel_engine_execlists *execlists) > >>> +{ > >>> + tasklet_lock(&execlists->tasklet); > >>> +} > >>> + > >>> +static inline void > >>> +execlists_active_unlock(struct intel_engine_execlists *execlists) > >>> +{ > >>> + tasklet_unlock(&execlists->tasklet); > >>> +} > >> > >> After we stop preventing the tasklet from running should we maybe kick > >> ksoftirqd? I am thinking if a tasklet gets scheduled and ran during us > >> holding the lock here, it won't lose the "scheduled" status, but not > >> sure at what next opportunity it would get re-run. > > > > If we call tasklet_schedule() while we hold tasklet_lock, ksoftirqd (on > > another thread, hmm, we need local_bh_disable() to stop ourselves > > entering the softirq processing), then that tasklet_action will spin on > > tasklet_trylock. > > I don't see it spinning, I see it unlinking the tasklet is trylock fails > and going onto the next one. So even what I implied before seems wrong - > it doesn't look like it would get re-run on next tasklet processing run. > Where do you see the retry? It puts it back onto the list of taskets to run. Then sees it still has tasklets to run and reschedules itself. tasklet_action_common: 1. sets list of pending tasklets to NULL 2. iterates over current list 3. if not executed (trylock failed, or disabled) then appends the tasklet to the list of pending for next time, and re-raises the softirq, thereby rescheduling the softirq/ksoftirqd. -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index d624752f2a92..c99910c4eeb9 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -136,6 +136,18 @@ execlists_active(const struct intel_engine_execlists *execlists) return READ_ONCE(*execlists->active); } +static inline void +execlists_active_lock(struct intel_engine_execlists *execlists) +{ + tasklet_lock(&execlists->tasklet); +} + +static inline void +execlists_active_unlock(struct intel_engine_execlists *execlists) +{ + tasklet_unlock(&execlists->tasklet); +} + struct i915_request * execlists_unwind_incomplete_requests(struct intel_engine_execlists *execlists); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 5aa1371f6a0f..45b708fc4b52 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -1245,9 +1245,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, struct drm_printer *m) { struct drm_i915_private *dev_priv = engine->i915; - const struct intel_engine_execlists * const execlists = - &engine->execlists; - unsigned long flags; + struct intel_engine_execlists * const execlists = &engine->execlists; u64 addr; if (engine->id == RENDER_CLASS && IS_GEN_RANGE(dev_priv, 4, 7)) @@ -1329,7 +1327,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, idx, hws[idx * 2], hws[idx * 2 + 1]); } - spin_lock_irqsave(&engine->active.lock, flags); + execlists_active_lock(execlists); for (port = execlists->active; (rq = *port); port++) { char hdr[80]; int len; @@ -1367,7 +1365,7 @@ static void intel_engine_print_registers(struct intel_engine_cs *engine, if (tl) intel_timeline_put(tl); } - spin_unlock_irqrestore(&engine->active.lock, flags); + execlists_active_unlock(execlists); } else if (INTEL_GEN(dev_priv) > 6) { drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n", ENGINE_READ(engine, RING_PP_DIR_BASE)); @@ -1509,8 +1507,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) if (!intel_engine_supports_stats(engine)) return -ENODEV; - spin_lock_irqsave(&engine->active.lock, flags); - write_seqlock(&engine->stats.lock); + execlists_active_lock(execlists); + write_seqlock_irqsave(&engine->stats.lock, flags); if (unlikely(engine->stats.enabled == ~0)) { err = -EBUSY; @@ -1538,8 +1536,8 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) } unlock: - write_sequnlock(&engine->stats.lock); - spin_unlock_irqrestore(&engine->active.lock, flags); + write_sequnlock_irqrestore(&engine->stats.lock, flags); + execlists_active_unlock(execlists); return err; } diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h index 167a7b56ed5b..6795f1daa3d5 100644 --- a/drivers/gpu/drm/i915/i915_gem.h +++ b/drivers/gpu/drm/i915/i915_gem.h @@ -77,6 +77,12 @@ struct drm_i915_private; #define I915_GEM_IDLE_TIMEOUT (HZ / 5) +static inline void tasklet_lock(struct tasklet_struct *t) +{ + while (!tasklet_trylock(t)) + cpu_relax(); +} + static inline void __tasklet_disable_sync_once(struct tasklet_struct *t) { if (!atomic_fetch_inc(&t->count))
The active/pending execlists is no longer protected by the engine->active.lock, but is serialised by the tasklet instead. Update the locking around the debug and stats to follow suit. Fixes: df403069029d ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- Wrap the tasklet_lock inside execlists_active_lock() to help clarify what it is meant to be protecting. --- drivers/gpu/drm/i915/gt/intel_engine.h | 12 ++++++++++++ drivers/gpu/drm/i915/gt/intel_engine_cs.c | 16 +++++++--------- drivers/gpu/drm/i915/i915_gem.h | 6 ++++++ 3 files changed, 25 insertions(+), 9 deletions(-)