Re: [PATCH 2/5] drm/i915: Wrap tasklet_struct for abuse

[Chris Wilson <chris@chris-wilson.co.uk>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx