[Intel-gfx] [PATCH 1/4] drm/i915: Fix PIPESTATE irq ack on i965/g4x

[Chris Wilson]

Quoting Ville Syrjala (2018-06-11 21:02:55)
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> On i965/g4x IIR is edge triggered. So in order for IIR to notice that
> there is still a pending interrupt we have to force and edge in ISR.
> For the ISR/IIR pipe event bits we can do that by temporarily
> clearing all the PIPESTAT enable bits when we ack the status bits.
> This will force the ISR pipe event bit low, and it can then go back
> high when we restore the PIPESTAT enable bits.
> 
> This avoids the following race:
> 1. stat = read(PIPESTAT)
> 2. an enabled PIPESTAT status bit goes high
> 3. write(PIPESTAT, enable|stat);
> 4. write(IIR, PIPE_EVENT)
> 
> The end result is IIR==0 and ISR!=0. This can lead to nasty
> vblank wait/flip_done timeouts if another interrupt source
> doesn't trick us into looking at the PIPESTAT status bits despite
> the IIR PIPE_EVENT bit being low.
> 
> Before i965 IIR was level triggered so this problem can't actually
> happen there. And curiously VLV/CHV went back to the level triggered
> scheme as well. But for simplicity we'll use the same i965/g4x
> compatible code for all platforms.
> 
> Cc: stable at vger.kernel.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106033
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105225
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106030
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2fd92a886789..364e1c85315e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1893,9 +1893,17 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
>  
>                 /*
>                  * Clear the PIPE*STAT regs before the IIR
> +                *
> +                * Toggle the enable bits to make sure we get an
> +                * edge in the ISR pipe event bit if we don't clear
> +                * all the enabled status bits. Otherwise the edge
> +                * triggered IIR on i965/g4x wouldn't notice that
> +                * an interrupt is still pending.
>                  */
> -               if (pipe_stats[pipe])
> -                       I915_WRITE(reg, enable_mask | pipe_stats[pipe]);
> +               if (pipe_stats[pipe]) {
> +                       I915_WRITE(reg, pipe_stats[pipe]);

Ack status, disable all.

> +                       I915_WRITE(reg, enable_mask);

Enable, an asserted bit should trigger a new edge.

It certainly does as you explain, now I just hope the hw feels the same!

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris