Quoting Mika Kuoppala (2018-05-09 14:44:30)
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
>
> > 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@xxxxxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 8 +--
> > 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, 104 insertions(+), 24 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 89bf5d67cb74..98481e150e61 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3043,9 +3043,9 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)
> > * common case of recursively being called from set-wedged from inside
> > * i915_reset.
> > */
> > - if (!atomic_read(&engine->execlists.tasklet.count))
> > - tasklet_kill(&engine->execlists.tasklet);
> > - tasklet_disable(&engine->execlists.tasklet);
> > + if (i915_tasklet_is_enabled(&engine->execlists.tasklet))
> > + i915_tasklet_flush(&engine->execlists.tasklet);
> > + i915_tasklet_disable(&engine->execlists.tasklet);
> >
> > /*
> > * We're using worker to queue preemption requests from the tasklet in
> > @@ -3265,7 +3265,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_enable(&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..c9f41a5ebb91
> > --- /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_locked(const struct i915_tasklet *t)
>
> why not is_running() ?
Because I didn't want to resend the series immediately. It's confusing
because it's set by tasklet_trylock and cleared by tasklet_unlock.
> > + 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));
>
> I am concerned that we bury the sign of racy nature out of sight and
> then it comes and bites us.
Oh, I wouldn't worry about that, just wait until you see what comes
later :-p
> > +static inline void i915_tasklet_set_func(struct i915_tasklet *t,
> > + void (*func)(unsigned long data),
> > + unsigned long data)
> > +{
> > + tasklet_disable(&t->base);
>
> What does the disable/enable pair buy us here?
>
I was thinking about being clear about the ordering.
> It looks that you want trylock and unlock
> so that you can safely change the func/data.
As you point out, the lock is tasklet_disable, the trylock is just a
trylock. Embrace the insanity, let it flow through you.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
