Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

drm/i915: also disable south interrupts when handling them

From the docs:

"IIR can queue up to two interrupt events. When the IIR is cleared,
it will set itself again after one clock if a second event was
stored."

"Only the rising edge of the PCH Display interrupt will cause the
North Display IIR (DEIIR) PCH Display Interrupt even bit to be set,
so all PCH Display Interrupts, including back to back interrupts,
must be cleared before a new PCH Display interrupt can cause DEIIR
to be set".

The current code works fine because we don't get many interrupts, but
if we enable the PCH FIFO underrun interrupts we'll start getting so
many interrupts that at some point new PCH interrupts won't cause
DEIIR to be set.

The initial implementation I tried was to turn the code that checks
SDEIIR into a loop, but we can still get interrupts even after the
loop is done (and before the irq handler finishes), so we have to
either disable the interrupts or mask them. In the end I concluded
that just disabling the PCH interrupts is enough, you don't even need
the loop, so this is what this patch implements. I've tested it and it
passes the 2 "PCH FIFO underrun interrupt storms" I can reproduce:
the "ironlake_crtc_disable" case and the "wrong watermarks" case.

In other words, here's how to reproduce the problem fixed by this
patch:
1 - Enable PCH FIFO underrun interrupts (SERR_INT on SNB+)
2 - Boot the machine
3 - While booting we'll get tons of PCH FIFO underrun interrupts
4 - Plug a new monitor
5 - Run xrandr, notice it won't detect the new monitor
6 - Read SDEIIR and notice it's not 0 while DEIIR is 0

Q: Can't we just clear DEIIR before SDEIIR?
A: It doesn't work. SDEIIR has to be completely cleared (including the
interrupts stored on its back queue) before it can flip DEIIR's bit to
1 again, and even while you're clearing it you'll be getting more and
more interrupts.

Q: Why does it work by just disabling+enabling the south interrupts?
A: Because when we re-enable them, if there's something on the SDEIIR
register (maybe an interrupt stored on the queue), the re-enabling
will make DEIIR's bit flip to 1, and since we'll already have
interrupts enabled we'll get another interrupt, then run our irq
handler again to process the "back" interrupts.

v2: Even bigger commit message, added code comments.

Note that this fixes missed dp aux irqs which have been reported for
3.9-rc1. This regression has been introduced by switching to
irq-driven dp aux transactions with

commit 9ee32fea5fe810ec06af3a15e4c65478de56d4f5
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Sat Dec 1 13:53:48 2012 +0100

drm/i915: irq-drive the dp aux communication

References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18588.html
References: https://lkml.org/lkml/2013/2/26/769
Tested-by: Imre Deak <imre.deak@intel.com>
Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
[danvet: Pimp commit message with references for the dp aux irq
timeout regression this fixes.]
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

authored by

Paulo Zanoni and committed by
Daniel Vetter
44498aea 15239099

+24 -2
+24 -2
drivers/gpu/drm/i915/i915_irq.c
··· 701 701 { 702 702 struct drm_device *dev = (struct drm_device *) arg; 703 703 drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; 704 - u32 de_iir, gt_iir, de_ier, pm_iir; 704 + u32 de_iir, gt_iir, de_ier, pm_iir, sde_ier; 705 705 irqreturn_t ret = IRQ_NONE; 706 706 int i; 707 707 ··· 710 710 /* disable master interrupt before clearing iir */ 711 711 de_ier = I915_READ(DEIER); 712 712 I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL); 713 + 714 + /* Disable south interrupts. We'll only write to SDEIIR once, so further 715 + * interrupts will will be stored on its back queue, and then we'll be 716 + * able to process them after we restore SDEIER (as soon as we restore 717 + * it, we'll get an interrupt if SDEIIR still has something to process 718 + * due to its back queue). */ 719 + sde_ier = I915_READ(SDEIER); 720 + I915_WRITE(SDEIER, 0); 721 + POSTING_READ(SDEIER); 713 722 714 723 gt_iir = I915_READ(GTIIR); 715 724 if (gt_iir) { ··· 768 759 769 760 I915_WRITE(DEIER, de_ier); 770 761 POSTING_READ(DEIER); 762 + I915_WRITE(SDEIER, sde_ier); 763 + POSTING_READ(SDEIER); 771 764 772 765 return ret; 773 766 } ··· 789 778 struct drm_device *dev = (struct drm_device *) arg; 790 779 drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; 791 780 int ret = IRQ_NONE; 792 - u32 de_iir, gt_iir, de_ier, pm_iir; 781 + u32 de_iir, gt_iir, de_ier, pm_iir, sde_ier; 793 782 794 783 atomic_inc(&dev_priv->irq_received); 795 784 ··· 797 786 de_ier = I915_READ(DEIER); 798 787 I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL); 799 788 POSTING_READ(DEIER); 789 + 790 + /* Disable south interrupts. We'll only write to SDEIIR once, so further 791 + * interrupts will will be stored on its back queue, and then we'll be 792 + * able to process them after we restore SDEIER (as soon as we restore 793 + * it, we'll get an interrupt if SDEIIR still has something to process 794 + * due to its back queue). */ 795 + sde_ier = I915_READ(SDEIER); 796 + I915_WRITE(SDEIER, 0); 797 + POSTING_READ(SDEIER); 800 798 801 799 de_iir = I915_READ(DEIIR); 802 800 gt_iir = I915_READ(GTIIR); ··· 869 849 done: 870 850 I915_WRITE(DEIER, de_ier); 871 851 POSTING_READ(DEIER); 852 + I915_WRITE(SDEIER, sde_ier); 853 + POSTING_READ(SDEIER); 872 854 873 855 return ret; 874 856 }