On Mon, Jun 16, 2014 at 03:09:24PM +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).
>
> 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.
>
> Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5522cbf..7e9fba0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2147,7 +2147,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> * do any I915_{READ,WRITE}. */
> intel_uncore_check_errors(dev);
>
> - /* disable master interrupt before clearing iir */
> + /* 1 - Disable master interrupt */
Sorry to be a nuisance, but I was thinking of more of theory of
operation block at the start of the function as well.
/* To handle irqs with the minimum of 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.
*/
Since we have a number of similar irq handlers this can be in a comment
block in just the first handler. And then we leave condensed reminders
in each function. Actually it may work best with this t.o.p. repeated
each time.
> de_ier = I915_READ(DEIER);
> I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> POSTING_READ(DEIER);
> @@ -2163,37 +2163,43 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
> POSTING_READ(SDEIER);
> }
>
> + /* 2 - Find the source(s) of the interrupt. */
/* Find, clear, then process each source of interrupt */
Then drop 3, 4 since we have the overview block and the ordering reminder
here.
> gt_iir = I915_READ(GTIIR);
> if (gt_iir) {
> + /* 3 - Clear the Interrupt Identity bits (IIR) before trying to
> + * handle the interrupt (we have the IIR safely stored) */
> + I915_WRITE(GTIIR, gt_iir);
> + ret = IRQ_HANDLED;
> + /* 4 - Process the interrupt(s) that had bits set in the IIRs. */
> 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);
> }
> }
>
> + /* 5 - Re-enable Master Interrupt Control */
/* Finally re-enable the master interrupt */
> I915_WRITE(DEIER, de_ier);
> POSTING_READ(DEIER);
> +
> if (!HAS_PCH_NOP(dev)) {
> I915_WRITE(SDEIER, sde_ier);
> POSTING_READ(SDEIER);
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
