From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 10/24] drm/i915: remove SERR_INT clearing in the postinstall hook Date: Thu, 4 Jul 2013 21:49:08 +0200 Message-ID: <20130704194908.GK18285@phenom.ffwll.local> References: <1371037046-3732-1-git-send-email-daniel.vetter@ffwll.ch> <1371037046-3732-11-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f48.google.com (mail-ee0-f48.google.com [74.125.83.48]) by gabe.freedesktop.org (Postfix) with ESMTP id 39AD7E5F38 for ; Thu, 4 Jul 2013 12:49:10 -0700 (PDT) Received: by mail-ee0-f48.google.com with SMTP id b47so944367eek.35 for ; Thu, 04 Jul 2013 12:49:09 -0700 (PDT) Content-Disposition: inline In-Reply-To: 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: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, Jun 27, 2013 at 04:34:25PM -0300, Paulo Zanoni wrote: > 2013/6/12 Daniel Vetter : > > The preinstallhook is supposed to clear all interrupts. Doing it > > again in the postinstall hook has the risk that we're eating > > an interrupt source from the handler. If that happens too often, > > the kernel will disable our interrupt handler. > > We do this with other registers too, why not remove those bits too > then? I mean, SERR_INT is just like one of the IIR interrupts, and we > always clear the IIR registers on postinstall functions. Can you > please explain a little bit more? You're right, we do clear the interrupt sticky bit clearing always in the postinstall hooks. I still think this is wrong, or at least a bit risky since the interrupt handler could fire as soon as we enable the master interrupt enable bit. And atm we do that too early. I think the right sequence is 1. Mask/disable interrupts in IMR/IER registers 2. Posting read 3. Clear IIR register, twice with a posting read (there's a little queue in the hw ...). ERR error registers (or HOTPLUG_STAT/PIPESTAT on vlv) only need one clear iirc. 4. Enable interrupt handler (i.e. 1.-3. above would be in the postinstall hook) 5. Write only IMR/IER registers in the postinstall hook since those aren't touched the interrupt handler (at least in general) There's currently one exception: SDEIER, since that's also touched in the interrupt handler. I think we need to handle DEIER similarly. Comments? The above is way more than just a simple patch though, so I think material for a different patch series. I'll drop this one here (since it's broken really, it only kills the clearing in postinstall but doesn't add it to preinstall). Cheers, Daniel > > > > > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/i915_irq.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index c2b4b09..685ad84 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2635,8 +2635,6 @@ static void ibx_irq_postinstall(struct drm_device *dev) > > SDE_TRANSA_FIFO_UNDER | SDE_POISON; > > } else { > > mask = SDE_GMBUS_CPT | SDE_AUX_MASK_CPT | SDE_ERROR_CPT; > > - > > - I915_WRITE(SERR_INT, I915_READ(SERR_INT)); > > } > > > > I915_WRITE(SDEIIR, I915_READ(SDEIIR)); > > -- > > 1.8.1.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch