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: Fri, 23 May 2014 13:43:45 +0200 Message-ID: <20140523114345.GR14357@phenom.ffwll.local> References: <1361563531-4653-1-git-send-email-przanoni@gmail.com> <1361563531-4653-2-git-send-email-przanoni@gmail.com> <20130305190806.GK9021@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f43.google.com (mail-ee0-f43.google.com [74.125.83.43]) by gabe.freedesktop.org (Postfix) with ESMTP id 2F86B6E58C for ; Fri, 23 May 2014 04:43:50 -0700 (PDT) Received: by mail-ee0-f43.google.com with SMTP id d17so3575723eek.2 for ; Fri, 23 May 2014 04:43:49 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: intel-gfx , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Fri, May 23, 2014 at 08:25:59AM -0300, Paulo Zanoni wrote: > 2014-05-23 5:13 GMT-03:00 Daniel Vetter : > > On Tue, Mar 5, 2013 at 8:08 PM, Daniel Vetter wrote: > >> 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. > > > > Digging up zombies ... So reading through Oscar's execlist patches > > I've had a new idea for fixing our various missed irq issues. The new > > trick seems to work for ring interrupts, but I couldn't yet test the > > pch interrupts. > > > > Do you still remember on which platform we get tons of fifo underruns > > on the pch when doing modesets? I've tried hdmi on my ivb here for a > > start, but that didn't work out - no pch irq storm, not even a single > > underrun :( > > I think I tested all my patches on both SNB and HSW, could reproduce > the problem on both. And I usually have 2 monitors connected on my > machines. > > But if you want to get the interrupt storn, you have to revert all the > FIFO underrun detection code, revert the patch above, and just add > code that tries to enable/check/clear the underrun bits in case we get > it. Well I've hacked it to never disable fifo underruns and still didn't get a storm somehow. But then I've decided that my patch was bogus anyway, so I think I'll leave it at that. I've tried on hsw and ivb with multi-pipe configs. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch