| Message ID | 20180509142801.28130-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Chris Wilson (2018-05-09 15:27:58) > In the next few patches, we want to abuse tasklet to avoid ksoftirqd > latency along critical paths. To make that abuse easily to swallow, > first coat the tasklet in a little syntactic sugar. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 4 +- > drivers/gpu/drm/i915/i915_irq.c | 2 +- > drivers/gpu/drm/i915/i915_tasklet.h | 78 +++++++++++++++++++++ > drivers/gpu/drm/i915/intel_engine_cs.c | 11 ++- > drivers/gpu/drm/i915/intel_guc_submission.c | 8 +-- > drivers/gpu/drm/i915/intel_lrc.c | 18 ++--- > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 +- > 7 files changed, 102 insertions(+), 22 deletions(-) > create mode 100644 drivers/gpu/drm/i915/i915_tasklet.h > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 63c96c2b8fcf..59e04387a27c 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); > + i915_tasklet_lock(&engine->execlists.tasklet); Hmm, probably sensible to stick to disable/enable: - better match to tasklet_interface (no arbitrary impedance mismatch) - they are counting locks rather than mutex which we commonly think of for lock/unlock (more like a semaphore). After dropping the custom flush+disable wrapper, there's no good reason to have a custom name. Thoughts? -Chris
On 09/05/2018 15:27, Chris Wilson wrote: > In the next few patches, we want to abuse tasklet to avoid ksoftirqd > latency along critical paths. To make that abuse easily to swallow, > first coat the tasklet in a little syntactic sugar. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 4 +- > drivers/gpu/drm/i915/i915_irq.c | 2 +- > drivers/gpu/drm/i915/i915_tasklet.h | 78 +++++++++++++++++++++ > drivers/gpu/drm/i915/intel_engine_cs.c | 11 ++- > drivers/gpu/drm/i915/intel_guc_submission.c | 8 +-- > drivers/gpu/drm/i915/intel_lrc.c | 18 ++--- > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 +- > 7 files changed, 102 insertions(+), 22 deletions(-) > create mode 100644 drivers/gpu/drm/i915/i915_tasklet.h > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 63c96c2b8fcf..59e04387a27c 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); > + i915_tasklet_lock(&engine->execlists.tasklet); > > /* > * We're using worker to queue preemption requests from the tasklet in > @@ -3256,7 +3256,7 @@ void i915_gem_reset(struct drm_i915_private *dev_priv, > > void i915_gem_reset_finish_engine(struct intel_engine_cs *engine) > { > - tasklet_enable(&engine->execlists.tasklet); > + i915_tasklet_unlock(&engine->execlists.tasklet); > kthread_unpark(engine->breadcrumbs.signaler); > > intel_uncore_forcewake_put(engine->i915, FORCEWAKE_ALL); > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index f9bc3aaa90d0..f8aff5a5aa83 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1477,7 +1477,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir) > } > > if (tasklet) > - tasklet_hi_schedule(&execlists->tasklet); > + i915_tasklet_schedule(&execlists->tasklet); > } > > static void gen8_gt_irq_ack(struct drm_i915_private *i915, > diff --git a/drivers/gpu/drm/i915/i915_tasklet.h b/drivers/gpu/drm/i915/i915_tasklet.h > new file mode 100644 > index 000000000000..42b002b88edb > --- /dev/null > +++ b/drivers/gpu/drm/i915/i915_tasklet.h > @@ -0,0 +1,78 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0 > + * > + * Copyright © 2018 Intel Corporation > + */ > + > +#ifndef _I915_TASKLET_H_ > +#define _I915_TASKLET_H_ > + > +#include <linux/atomic.h> > +#include <linux/interrupt.h> > + > +/** > + * struct i915_tasklet - wrapper around tasklet_struct > + * > + * We want to abuse tasklets slightly, such as calling them directly. In some > + * cases, this requires some auxiliary tracking so subclass the tasklet_struct > + * so that we have a central place and helpers. > + */ > +struct i915_tasklet { > + struct tasklet_struct base; > +}; > + > +static inline void i915_tasklet_init(struct i915_tasklet *t, > + void (*func)(unsigned long), > + unsigned long data) > +{ > + tasklet_init(&t->base, func, data); > +} > + > +static inline bool i915_tasklet_is_scheduled(const struct i915_tasklet *t) > +{ > + return test_bit(TASKLET_STATE_SCHED, &t->base.state); > +} > + > +static inline bool i915_tasklet_is_running(const struct i915_tasklet *t) > +{ > + return test_bit(TASKLET_STATE_RUN, &t->base.state); > +} > + > +static inline bool i915_tasklet_is_enabled(const struct i915_tasklet *t) > +{ > + return likely(!atomic_read(&t->base.count)); > +} > + > +static inline void i915_tasklet_schedule(struct i915_tasklet *t) > +{ > + tasklet_hi_schedule(&t->base); > +} > + > +static inline void i915_tasklet_flush(struct i915_tasklet *t) > +{ > + tasklet_kill(&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); > +} I agree keeping the original naming for kill/enable/disable would be better. > + > +static inline void i915_tasklet_set_func(struct i915_tasklet *t, > + void (*func)(unsigned long data), > + unsigned long data) > +{ > + i915_tasklet_lock(t); > + > + t->base.func = func; > + t->base.data = data; > + > + i915_tasklet_unlock(t); I kind of remember something about issues we had with placing tasklet_disable placed in some contexts. Hm.. if only I could recall the details. It's probably fine. I cannot come up with ideas on how to protect against that. GEM_BUG_ON(in_irq() || in_softirq())? Too far fetched probably. > +} > + > +#endif /* _I915_TASKLET_H_ */ > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 70325e0824e3..3c8a0c3245f3 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -1032,7 +1032,7 @@ void intel_engines_park(struct drm_i915_private *i915) > for_each_engine(engine, i915, id) { > /* Flush the residual irq tasklets first. */ > intel_engine_disarm_breadcrumbs(engine); > - tasklet_kill(&engine->execlists.tasklet); > + i915_tasklet_flush(&engine->execlists.tasklet); > > /* > * We are committed now to parking the engines, make sure there > @@ -1249,9 +1249,8 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine, > intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)), > yesno(test_bit(ENGINE_IRQ_EXECLIST, > &engine->irq_posted)), > - yesno(test_bit(TASKLET_STATE_SCHED, > - &engine->execlists.tasklet.state)), > - enableddisabled(!atomic_read(&engine->execlists.tasklet.count))); > + yesno(i915_tasklet_is_scheduled(&engine->execlists.tasklet)), > + enableddisabled(i915_tasklet_is_enabled(&engine->execlists.tasklet))); > if (read >= GEN8_CSB_ENTRIES) > read = 0; > if (write >= GEN8_CSB_ENTRIES) > @@ -1479,7 +1478,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) > if (!intel_engine_supports_stats(engine)) > return -ENODEV; > > - tasklet_disable(&execlists->tasklet); > + i915_tasklet_lock(&execlists->tasklet); > write_seqlock_irqsave(&engine->stats.lock, flags); > > if (unlikely(engine->stats.enabled == ~0)) { > @@ -1505,7 +1504,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) > > unlock: > write_sequnlock_irqrestore(&engine->stats.lock, flags); > - tasklet_enable(&execlists->tasklet); > + i915_tasklet_unlock(&execlists->tasklet); > > return err; > } > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c > index 2feb65096966..a7afc976c3b9 100644 > --- a/drivers/gpu/drm/i915/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c > @@ -591,7 +591,7 @@ static void inject_preempt_context(struct work_struct *work) > if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) { > execlists_clear_active(&engine->execlists, > EXECLISTS_ACTIVE_PREEMPT); > - tasklet_schedule(&engine->execlists.tasklet); > + i915_tasklet_schedule(&engine->execlists.tasklet); > } > } > > @@ -1263,10 +1263,10 @@ int intel_guc_submission_enable(struct intel_guc *guc) > guc_interrupts_capture(dev_priv); > > for_each_engine(engine, dev_priv, id) { > - struct intel_engine_execlists * const execlists = > - &engine->execlists; > + i915_tasklet_set_func(&engine->execlists.tasklet, > + guc_submission_tasklet, > + (unsigned long)engine); > > - execlists->tasklet.func = guc_submission_tasklet; > engine->park = guc_submission_park; > engine->unpark = guc_submission_unpark; > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 7f98dda3c929..539fa03d7600 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1165,7 +1165,7 @@ static void queue_request(struct intel_engine_cs *engine, > static void __submit_queue(struct intel_engine_cs *engine, int prio) > { > engine->execlists.queue_priority = prio; > - tasklet_hi_schedule(&engine->execlists.tasklet); > + i915_tasklet_schedule(&engine->execlists.tasklet); > } > > static void submit_queue(struct intel_engine_cs *engine, int prio) > @@ -1778,7 +1778,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) > > /* After a GPU reset, we may have requests to replay */ > if (execlists->first) > - tasklet_schedule(&execlists->tasklet); > + i915_tasklet_schedule(&execlists->tasklet); > > return 0; > } > @@ -2182,9 +2182,8 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine) > * Tasklet cannot be active at this point due intel_mark_active/idle > * so this is just for documentation. > */ > - if (WARN_ON(test_bit(TASKLET_STATE_SCHED, > - &engine->execlists.tasklet.state))) > - tasklet_kill(&engine->execlists.tasklet); > + if (WARN_ON(i915_tasklet_is_scheduled(&engine->execlists.tasklet))) > + i915_tasklet_flush(&engine->execlists.tasklet); > > dev_priv = engine->i915; > > @@ -2209,7 +2208,10 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine) > engine->submit_request = execlists_submit_request; > engine->cancel_requests = execlists_cancel_requests; > engine->schedule = execlists_schedule; > - engine->execlists.tasklet.func = execlists_submission_tasklet; > + > + i915_tasklet_set_func(&engine->execlists.tasklet, > + execlists_submission_tasklet, > + (unsigned long)engine); Or eliminate the above dilemma by removing the data parameter from i915_tasklet_set_func since it looks it is not needed at the moment? Regards, Tvrtko > > engine->park = NULL; > engine->unpark = NULL; > @@ -2303,8 +2305,8 @@ logical_ring_setup(struct intel_engine_cs *engine) > > engine->execlists.fw_domains = fw_domains; > > - tasklet_init(&engine->execlists.tasklet, > - execlists_submission_tasklet, (unsigned long)engine); > + i915_tasklet_init(&engine->execlists.tasklet, > + execlists_submission_tasklet, (unsigned long)engine); > > logical_ring_default_vfuncs(engine); > logical_ring_default_irqs(engine); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 010750e8ee44..f6ba354faf89 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -11,6 +11,7 @@ > #include "i915_pmu.h" > #include "i915_request.h" > #include "i915_selftest.h" > +#include "i915_tasklet.h" > #include "i915_timeline.h" > #include "intel_gpu_commands.h" > > @@ -202,7 +203,7 @@ struct intel_engine_execlists { > /** > * @tasklet: softirq tasklet for bottom handler > */ > - struct tasklet_struct tasklet; > + struct i915_tasklet tasklet; > > /** > * @default_priolist: priority list for I915_PRIORITY_NORMAL >
Quoting Tvrtko Ursulin (2018-05-10 16:49:03) > > On 09/05/2018 15:27, Chris Wilson wrote: > > In the next few patches, we want to abuse tasklet to avoid ksoftirqd > > latency along critical paths. To make that abuse easily to swallow, > > first coat the tasklet in a little syntactic sugar. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > +static inline void i915_tasklet_unlock(struct i915_tasklet *t) > > +{ > > + tasklet_enable(&t->base); > > +} > > I agree keeping the original naming for kill/enable/disable would be better. > > > + > > +static inline void i915_tasklet_set_func(struct i915_tasklet *t, > > + void (*func)(unsigned long data), > > + unsigned long data) > > +{ > > + i915_tasklet_lock(t); > > + > > + t->base.func = func; > > + t->base.data = data; > > + > > + i915_tasklet_unlock(t); > > I kind of remember something about issues we had with placing > tasklet_disable placed in some contexts. Hm.. if only I could recall the > details. It's probably fine. I cannot come up with ideas on how to > protect against that. GEM_BUG_ON(in_irq() || in_softirq())? Too far > fetched probably. When we get it wrong, CI complains with a lock up. It cannot be used in atomic context as it uses yield() to kick the tasklet (as it may be on the local cpu). > > @@ -2209,7 +2208,10 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine) > > engine->submit_request = execlists_submit_request; > > engine->cancel_requests = execlists_cancel_requests; > > engine->schedule = execlists_schedule; > > - engine->execlists.tasklet.func = execlists_submission_tasklet; > > + > > + i915_tasklet_set_func(&engine->execlists.tasklet, > > + execlists_submission_tasklet, > > + (unsigned long)engine); > > Or eliminate the above dilemma by removing the data parameter from > i915_tasklet_set_func since it looks it is not needed at the moment? It doesn't eliminate the dilemma, updating the func itself raises the question of what if the tasklet is running at that time, what was the caller thinking would happen? I expect tasklets will start passing themselves to the func and eliminate the data parameters themselves at some point in the mid term. -Chris
On 10/05/2018 17:03, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-05-10 16:49:03) >> >> On 09/05/2018 15:27, Chris Wilson wrote: >>> In the next few patches, we want to abuse tasklet to avoid ksoftirqd >>> latency along critical paths. To make that abuse easily to swallow, >>> first coat the tasklet in a little syntactic sugar. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> +static inline void i915_tasklet_unlock(struct i915_tasklet *t) >>> +{ >>> + tasklet_enable(&t->base); >>> +} >> >> I agree keeping the original naming for kill/enable/disable would be better. >> >>> + >>> +static inline void i915_tasklet_set_func(struct i915_tasklet *t, >>> + void (*func)(unsigned long data), >>> + unsigned long data) >>> +{ >>> + i915_tasklet_lock(t); >>> + >>> + t->base.func = func; >>> + t->base.data = data; >>> + >>> + i915_tasklet_unlock(t); >> >> I kind of remember something about issues we had with placing >> tasklet_disable placed in some contexts. Hm.. if only I could recall the >> details. It's probably fine. I cannot come up with ideas on how to >> protect against that. GEM_BUG_ON(in_irq() || in_softirq())? Too far >> fetched probably. > > When we get it wrong, CI complains with a lock up. It cannot be used in > atomic context as it uses yield() to kick the tasklet (as it may be on > the local cpu). > >>> @@ -2209,7 +2208,10 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine) >>> engine->submit_request = execlists_submit_request; >>> engine->cancel_requests = execlists_cancel_requests; >>> engine->schedule = execlists_schedule; >>> - engine->execlists.tasklet.func = execlists_submission_tasklet; >>> + >>> + i915_tasklet_set_func(&engine->execlists.tasklet, >>> + execlists_submission_tasklet, >>> + (unsigned long)engine); >> >> Or eliminate the above dilemma by removing the data parameter from >> i915_tasklet_set_func since it looks it is not needed at the moment? > > It doesn't eliminate the dilemma, updating the func itself raises the > question of what if the tasklet is running at that time, what was the > caller thinking would happen? Well same as it can do now so you could drop tasklet_disable/enable then. At least I assumed the purpose is for atomicity of func/data updates. > I expect tasklets will start passing themselves to the func and > eliminate the data parameters themselves at some point in the mid term. Needs to be tasklet_disable_nosync then, or it will hang. But then it loses its purpose anyway. So I am confused. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-05-10 17:15:46) > > On 10/05/2018 17:03, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-05-10 16:49:03) > >> > >> On 09/05/2018 15:27, Chris Wilson wrote: > >>> In the next few patches, we want to abuse tasklet to avoid ksoftirqd > >>> latency along critical paths. To make that abuse easily to swallow, > >>> first coat the tasklet in a little syntactic sugar. > >>> > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>> --- > >>> +static inline void i915_tasklet_unlock(struct i915_tasklet *t) > >>> +{ > >>> + tasklet_enable(&t->base); > >>> +} > >> > >> I agree keeping the original naming for kill/enable/disable would be better. > >> > >>> + > >>> +static inline void i915_tasklet_set_func(struct i915_tasklet *t, > >>> + void (*func)(unsigned long data), > >>> + unsigned long data) > >>> +{ > >>> + i915_tasklet_lock(t); > >>> + > >>> + t->base.func = func; > >>> + t->base.data = data; > >>> + > >>> + i915_tasklet_unlock(t); > >> > >> I kind of remember something about issues we had with placing > >> tasklet_disable placed in some contexts. Hm.. if only I could recall the > >> details. It's probably fine. I cannot come up with ideas on how to > >> protect against that. GEM_BUG_ON(in_irq() || in_softirq())? Too far > >> fetched probably. > > > > When we get it wrong, CI complains with a lock up. It cannot be used in > > atomic context as it uses yield() to kick the tasklet (as it may be on > > the local cpu). > > > >>> @@ -2209,7 +2208,10 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine) > >>> engine->submit_request = execlists_submit_request; > >>> engine->cancel_requests = execlists_cancel_requests; > >>> engine->schedule = execlists_schedule; > >>> - engine->execlists.tasklet.func = execlists_submission_tasklet; > >>> + > >>> + i915_tasklet_set_func(&engine->execlists.tasklet, > >>> + execlists_submission_tasklet, > >>> + (unsigned long)engine); > >> > >> Or eliminate the above dilemma by removing the data parameter from > >> i915_tasklet_set_func since it looks it is not needed at the moment? > > > > It doesn't eliminate the dilemma, updating the func itself raises the > > question of what if the tasklet is running at that time, what was the > > caller thinking would happen? > > Well same as it can do now so you could drop tasklet_disable/enable > then. At least I assumed the purpose is for atomicity of func/data updates. Sure, it mostly there to catch using it from silly circumstances where the tasklet may be being called as the change occurs. Should never happen, so I bet it does. > > I expect tasklets will start passing themselves to the func and > > eliminate the data parameters themselves at some point in the mid term. > > Needs to be tasklet_disable_nosync then, or it will hang. But then it > loses its purpose anyway. So I am confused. I was just referring to my hope that the unsigned long data parameter will be moved to the great dustbin of history. If timers can make the leap, so can tasklets! -Chris
On 10/05/2018 17:19, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-05-10 17:15:46) >> >> On 10/05/2018 17:03, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2018-05-10 16:49:03) >>>> >>>> On 09/05/2018 15:27, Chris Wilson wrote: >>>>> In the next few patches, we want to abuse tasklet to avoid ksoftirqd >>>>> latency along critical paths. To make that abuse easily to swallow, >>>>> first coat the tasklet in a little syntactic sugar. >>>>> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> --- >>>>> +static inline void i915_tasklet_unlock(struct i915_tasklet *t) >>>>> +{ >>>>> + tasklet_enable(&t->base); >>>>> +} >>>> >>>> I agree keeping the original naming for kill/enable/disable would be better. >>>> >>>>> + >>>>> +static inline void i915_tasklet_set_func(struct i915_tasklet *t, >>>>> + void (*func)(unsigned long data), >>>>> + unsigned long data) >>>>> +{ >>>>> + i915_tasklet_lock(t); >>>>> + >>>>> + t->base.func = func; >>>>> + t->base.data = data; >>>>> + >>>>> + i915_tasklet_unlock(t); >>>> >>>> I kind of remember something about issues we had with placing >>>> tasklet_disable placed in some contexts. Hm.. if only I could recall the >>>> details. It's probably fine. I cannot come up with ideas on how to >>>> protect against that. GEM_BUG_ON(in_irq() || in_softirq())? Too far >>>> fetched probably. >>> >>> When we get it wrong, CI complains with a lock up. It cannot be used in >>> atomic context as it uses yield() to kick the tasklet (as it may be on >>> the local cpu). >>> >>>>> @@ -2209,7 +2208,10 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine) >>>>> engine->submit_request = execlists_submit_request; >>>>> engine->cancel_requests = execlists_cancel_requests; >>>>> engine->schedule = execlists_schedule; >>>>> - engine->execlists.tasklet.func = execlists_submission_tasklet; >>>>> + >>>>> + i915_tasklet_set_func(&engine->execlists.tasklet, >>>>> + execlists_submission_tasklet, >>>>> + (unsigned long)engine); >>>> >>>> Or eliminate the above dilemma by removing the data parameter from >>>> i915_tasklet_set_func since it looks it is not needed at the moment? >>> >>> It doesn't eliminate the dilemma, updating the func itself raises the >>> question of what if the tasklet is running at that time, what was the >>> caller thinking would happen? >> >> Well same as it can do now so you could drop tasklet_disable/enable >> then. At least I assumed the purpose is for atomicity of func/data updates. > > Sure, it mostly there to catch using it from silly circumstances where > the tasklet may be being called as the change occurs. Should never > happen, so I bet it does. You could do similar by asserting tasklet is either disabled, or at least not running. >>> I expect tasklets will start passing themselves to the func and >>> eliminate the data parameters themselves at some point in the mid term. >> >> Needs to be tasklet_disable_nosync then, or it will hang. But then it >> loses its purpose anyway. So I am confused. > > I was just referring to my hope that the unsigned long data parameter > will be moved to the great dustbin of history. If timers can make the > leap, so can tasklets! I just don't see the point in adding the second parameter now. I mean it's not a big deal, but I don't see it useful. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 63c96c2b8fcf..59e04387a27c 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); + i915_tasklet_lock(&engine->execlists.tasklet); /* * We're using worker to queue preemption requests from the tasklet in @@ -3256,7 +3256,7 @@ void i915_gem_reset(struct drm_i915_private *dev_priv, void i915_gem_reset_finish_engine(struct intel_engine_cs *engine) { - tasklet_enable(&engine->execlists.tasklet); + i915_tasklet_unlock(&engine->execlists.tasklet); kthread_unpark(engine->breadcrumbs.signaler); intel_uncore_forcewake_put(engine->i915, FORCEWAKE_ALL); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index f9bc3aaa90d0..f8aff5a5aa83 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1477,7 +1477,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir) } if (tasklet) - tasklet_hi_schedule(&execlists->tasklet); + i915_tasklet_schedule(&execlists->tasklet); } static void gen8_gt_irq_ack(struct drm_i915_private *i915, diff --git a/drivers/gpu/drm/i915/i915_tasklet.h b/drivers/gpu/drm/i915/i915_tasklet.h new file mode 100644 index 000000000000..42b002b88edb --- /dev/null +++ b/drivers/gpu/drm/i915/i915_tasklet.h @@ -0,0 +1,78 @@ +/* + * SPDX-License-Identifier: GPL-2.0 + * + * Copyright © 2018 Intel Corporation + */ + +#ifndef _I915_TASKLET_H_ +#define _I915_TASKLET_H_ + +#include <linux/atomic.h> +#include <linux/interrupt.h> + +/** + * struct i915_tasklet - wrapper around tasklet_struct + * + * We want to abuse tasklets slightly, such as calling them directly. In some + * cases, this requires some auxiliary tracking so subclass the tasklet_struct + * so that we have a central place and helpers. + */ +struct i915_tasklet { + struct tasklet_struct base; +}; + +static inline void i915_tasklet_init(struct i915_tasklet *t, + void (*func)(unsigned long), + unsigned long data) +{ + tasklet_init(&t->base, func, data); +} + +static inline bool i915_tasklet_is_scheduled(const struct i915_tasklet *t) +{ + return test_bit(TASKLET_STATE_SCHED, &t->base.state); +} + +static inline bool i915_tasklet_is_running(const struct i915_tasklet *t) +{ + return test_bit(TASKLET_STATE_RUN, &t->base.state); +} + +static inline bool i915_tasklet_is_enabled(const struct i915_tasklet *t) +{ + return likely(!atomic_read(&t->base.count)); +} + +static inline void i915_tasklet_schedule(struct i915_tasklet *t) +{ + tasklet_hi_schedule(&t->base); +} + +static inline void i915_tasklet_flush(struct i915_tasklet *t) +{ + tasklet_kill(&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) +{ + i915_tasklet_lock(t); + + t->base.func = func; + t->base.data = data; + + i915_tasklet_unlock(t); +} + +#endif /* _I915_TASKLET_H_ */ diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 70325e0824e3..3c8a0c3245f3 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -1032,7 +1032,7 @@ void intel_engines_park(struct drm_i915_private *i915) for_each_engine(engine, i915, id) { /* Flush the residual irq tasklets first. */ intel_engine_disarm_breadcrumbs(engine); - tasklet_kill(&engine->execlists.tasklet); + i915_tasklet_flush(&engine->execlists.tasklet); /* * We are committed now to parking the engines, make sure there @@ -1249,9 +1249,8 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine, intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)), yesno(test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted)), - yesno(test_bit(TASKLET_STATE_SCHED, - &engine->execlists.tasklet.state)), - enableddisabled(!atomic_read(&engine->execlists.tasklet.count))); + yesno(i915_tasklet_is_scheduled(&engine->execlists.tasklet)), + enableddisabled(i915_tasklet_is_enabled(&engine->execlists.tasklet))); if (read >= GEN8_CSB_ENTRIES) read = 0; if (write >= GEN8_CSB_ENTRIES) @@ -1479,7 +1478,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) if (!intel_engine_supports_stats(engine)) return -ENODEV; - tasklet_disable(&execlists->tasklet); + i915_tasklet_lock(&execlists->tasklet); write_seqlock_irqsave(&engine->stats.lock, flags); if (unlikely(engine->stats.enabled == ~0)) { @@ -1505,7 +1504,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine) unlock: write_sequnlock_irqrestore(&engine->stats.lock, flags); - tasklet_enable(&execlists->tasklet); + i915_tasklet_unlock(&execlists->tasklet); return err; } diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c index 2feb65096966..a7afc976c3b9 100644 --- a/drivers/gpu/drm/i915/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/intel_guc_submission.c @@ -591,7 +591,7 @@ static void inject_preempt_context(struct work_struct *work) if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) { execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT); - tasklet_schedule(&engine->execlists.tasklet); + i915_tasklet_schedule(&engine->execlists.tasklet); } } @@ -1263,10 +1263,10 @@ int intel_guc_submission_enable(struct intel_guc *guc) guc_interrupts_capture(dev_priv); for_each_engine(engine, dev_priv, id) { - struct intel_engine_execlists * const execlists = - &engine->execlists; + i915_tasklet_set_func(&engine->execlists.tasklet, + guc_submission_tasklet, + (unsigned long)engine); - execlists->tasklet.func = guc_submission_tasklet; engine->park = guc_submission_park; engine->unpark = guc_submission_unpark; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 7f98dda3c929..539fa03d7600 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1165,7 +1165,7 @@ static void queue_request(struct intel_engine_cs *engine, static void __submit_queue(struct intel_engine_cs *engine, int prio) { engine->execlists.queue_priority = prio; - tasklet_hi_schedule(&engine->execlists.tasklet); + i915_tasklet_schedule(&engine->execlists.tasklet); } static void submit_queue(struct intel_engine_cs *engine, int prio) @@ -1778,7 +1778,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine) /* After a GPU reset, we may have requests to replay */ if (execlists->first) - tasklet_schedule(&execlists->tasklet); + i915_tasklet_schedule(&execlists->tasklet); return 0; } @@ -2182,9 +2182,8 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine) * Tasklet cannot be active at this point due intel_mark_active/idle * so this is just for documentation. */ - if (WARN_ON(test_bit(TASKLET_STATE_SCHED, - &engine->execlists.tasklet.state))) - tasklet_kill(&engine->execlists.tasklet); + if (WARN_ON(i915_tasklet_is_scheduled(&engine->execlists.tasklet))) + i915_tasklet_flush(&engine->execlists.tasklet); dev_priv = engine->i915; @@ -2209,7 +2208,10 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine) engine->submit_request = execlists_submit_request; engine->cancel_requests = execlists_cancel_requests; engine->schedule = execlists_schedule; - engine->execlists.tasklet.func = execlists_submission_tasklet; + + i915_tasklet_set_func(&engine->execlists.tasklet, + execlists_submission_tasklet, + (unsigned long)engine); engine->park = NULL; engine->unpark = NULL; @@ -2303,8 +2305,8 @@ logical_ring_setup(struct intel_engine_cs *engine) engine->execlists.fw_domains = fw_domains; - tasklet_init(&engine->execlists.tasklet, - execlists_submission_tasklet, (unsigned long)engine); + i915_tasklet_init(&engine->execlists.tasklet, + execlists_submission_tasklet, (unsigned long)engine); logical_ring_default_vfuncs(engine); logical_ring_default_irqs(engine); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 010750e8ee44..f6ba354faf89 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -11,6 +11,7 @@ #include "i915_pmu.h" #include "i915_request.h" #include "i915_selftest.h" +#include "i915_tasklet.h" #include "i915_timeline.h" #include "intel_gpu_commands.h" @@ -202,7 +203,7 @@ struct intel_engine_execlists { /** * @tasklet: softirq tasklet for bottom handler */ - struct tasklet_struct tasklet; + struct i915_tasklet tasklet; /** * @default_priolist: priority list for I915_PRIORITY_NORMAL
In the next few patches, we want to abuse tasklet to avoid ksoftirqd latency along critical paths. To make that abuse easily to swallow, first coat the tasklet in a little syntactic sugar. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 4 +- drivers/gpu/drm/i915/i915_irq.c | 2 +- drivers/gpu/drm/i915/i915_tasklet.h | 78 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_engine_cs.c | 11 ++- drivers/gpu/drm/i915/intel_guc_submission.c | 8 +-- drivers/gpu/drm/i915/intel_lrc.c | 18 ++--- drivers/gpu/drm/i915/intel_ringbuffer.h | 3 +- 7 files changed, 102 insertions(+), 22 deletions(-) create mode 100644 drivers/gpu/drm/i915/i915_tasklet.h