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

[Sebastian Andrzej Siewior]

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?
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).

> 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.

>  	if (ret < 0) {
>  		dev_priv->drm.irq_enabled = false;
>  		return ret;

Sebastian