On Mon, 2014-06-16 at 16:10 +0100, oscar.mateo@xxxxxxxxx wrote:
> From: Oscar Mateo <oscar.mateo@xxxxxxxxx>
>
> Otherwise, we might receive a new interrupt before we have time to ack the first
> one, eventually missing it.
>
> According to BSPec, the right order should be:
>
> 1 - Disable Master Interrupt Control.
> 2 - Find the source(s) of the interrupt.
> 3 - Clear the Interrupt Identity bits (IIR).
> 4 - Process the interrupt(s) that had bits set in the IIRs.
> 5 - Re-enable Master Interrupt Control.
>
> Without an atomic XCHG operation with mmio space, the above merely reduces the window
> in which we can miss an interrupt (especially when you consider how heavyweight the
> I915_READ/I915_WRITE operations are).
I can see how we can miss a second, third etc. back-to-back interrupt,
but that's always a problem with edge triggered interrupts. But the
rearranging done in this patchset closes the race where we are left with
a pending interrupt flag without ever calling its handler.
> We maintain the "disable SDE interrupts when handling" hack since apparently it works.
>
> Spotted by Bob Beckett <robert.beckett@xxxxxxxxx>.
>
> v2: Add warning to commit message and comments to the code as per Chris Wilson's request.
> v3: Improve the source comments.
>
> Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
I couldn't spot any problems, so on all 4 patches:
Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5522cbf..a68f68c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2136,6 +2136,14 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> }
> }
>
> +/*
> + * To handle irqs with the minimum potential races with fresh interrupts, we:
> + * 1 - Disable Master Interrupt Control.
> + * 2 - Find the source(s) of the interrupt.
> + * 3 - Clear the Interrupt Identity bits (IIR).
> + * 4 - Process the interrupt(s) that had bits set in the IIRs.
> + * 5 - Re-enable Master Interrupt Control.
> + */
> static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> {
> struct drm_device *dev = arg;
> @@ -2163,32 +2171,34 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> POSTING_READ(SDEIER);
> }
>
> + /* Find, clear, then process each source of interrupt */
> +
> gt_iir = I915_READ(GTIIR);
> if (gt_iir) {
> + I915_WRITE(GTIIR, gt_iir);
> + ret = IRQ_HANDLED;
> if (INTEL_INFO(dev)->gen >= 6)
> snb_gt_irq_handler(dev, dev_priv, gt_iir);
> else
> ilk_gt_irq_handler(dev, dev_priv, gt_iir);
> - I915_WRITE(GTIIR, gt_iir);
> - ret = IRQ_HANDLED;
> }
>
> de_iir = I915_READ(DEIIR);
> if (de_iir) {
> + I915_WRITE(DEIIR, de_iir);
> + ret = IRQ_HANDLED;
> if (INTEL_INFO(dev)->gen >= 7)
> ivb_display_irq_handler(dev, de_iir);
> else
> ilk_display_irq_handler(dev, de_iir);
> - I915_WRITE(DEIIR, de_iir);
> - ret = IRQ_HANDLED;
> }
>
> if (INTEL_INFO(dev)->gen >= 6) {
> u32 pm_iir = I915_READ(GEN6_PMIIR);
> if (pm_iir) {
> - gen6_rps_irq_handler(dev_priv, pm_iir);
> I915_WRITE(GEN6_PMIIR, pm_iir);
> ret = IRQ_HANDLED;
> + gen6_rps_irq_handler(dev_priv, pm_iir);
> }
> }
>
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
