From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/4] drm/i915: also disable south interrupts when handling them Date: Tue, 5 Mar 2013 20:08:07 +0100 Message-ID: <20130305190806.GK9021@phenom.ffwll.local> References: <1361563531-4653-1-git-send-email-przanoni@gmail.com> <1361563531-4653-2-git-send-email-przanoni@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f52.google.com (mail-wg0-f52.google.com [74.125.82.52]) by gabe.freedesktop.org (Postfix) with ESMTP id 819F9E5D18 for ; Tue, 5 Mar 2013 11:05:35 -0800 (PST) Received: by mail-wg0-f52.google.com with SMTP id 12so6124878wgh.31 for ; Tue, 05 Mar 2013 11:05:34 -0800 (PST) Content-Disposition: inline In-Reply-To: <1361563531-4653-2-git-send-email-przanoni@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Fri, Feb 22, 2013 at 05:05:28PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > 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. > > Signed-off-by: Paulo Zanoni Since this seems to fix the dp aux irq timeout regression I've merged this to -fixes. Also volunteered Imre for a review, I'll add that if it pops up in the next few days. Big thanks to Paulo&Imre for tracking this down. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch