[Intel-gfx] [PATCH] drm/i915: Delegate our irq handler to a thread

[Chris Wilson]

Quoting Sebastian Andrzej Siewior (2019-09-26 16:13:08)
> On 2019-09-26 15:25:38 [+0100], Chris Wilson wrote:
> > Moving our primary irq handler to a RT thread incurs an extra 1us delay
> > in process interrupts. This is most notice in waking up client threads,
> > where it adds about 20% of extra latency. It also imposes a delay in
> > feeding the GPU, an extra 1us before signaling secondary engines and
> > extra latency in resubmitting work to keep the GPU busy. The latter case
> > is insignificant as the latency hidden by the active GPU, and
> > preempt-to-busy ensures that no extra latency is incurred for
> > preemption.
> > 
> > The benefit is that we reduced the impact on the rest of the system, the
> > cycletest shows a reduction from 5us mean latency to 2us, with the
> > maximum observed latency (in a 2 minute window) reduced by over 160us.
> 
> cycletest is the name of the tool I don't know about, right? It is not
> cyclictest with a typo?

It's a tool like cyclictest that measures the timer latency in one
thread while flooding the system with GPU work, filling the page cache,
grabbing as many THP as possible etc. (It's actually called
gem_syslatency.)

> I don't know how many interrupts you have in a system but if this is the
> only one for the card then you could remove _irqsave() from locking if
> the lock is never observed in IRQ context (but then you have irqwork
> which requires this irqsave).

True, pushing the irq into a thread should allow us to re-evaluate a lot
of our spin_lock_irq.

> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Clark Williams <williams at redhat.com>
> > Cc: Sebastian Andrzej Siewior <bigeasy at linutronix.de>
> > ---
> > Note this should need the fixes in
> > 20190926105644.16703-2-bigeasy at linutronix.de, by itself this should be a
> > test vehicle to exercise that patch!
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index bc83f094065a..f3df7714a3f3 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -4491,8 +4491,8 @@ int intel_irq_install(struct drm_i915_private *dev_priv)
> >  
> >       intel_irq_reset(dev_priv);
> >  
> > -     ret = request_irq(irq, intel_irq_handler(dev_priv),
> > -                       IRQF_SHARED, DRIVER_NAME, dev_priv);
> > +     ret = request_threaded_irq(irq, NULL, intel_irq_handler(dev_priv),
> > +                                IRQF_SHARED, DRIVER_NAME, dev_priv);
> 
> I think you should add IRQF_ONESHOT. Otherwise a LEVEL interrupt will
> keep interrupting the CPU and you never manage to switch to the thread.

The interrupts only keep coming if we feed the GPU, and we only feed the
GPU if we service the interrupt. That should provide a natural
quiescence :)
-Chris