Re: [PATCH 10/24] drm/i915: remove SERR_INT clearing in the postinstall hook

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 27, 2013 at 04:34:25PM -0300, Paulo Zanoni wrote:
> 2013/6/12 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > 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 <daniel.vetter@ffwll.ch>
> > ---
> >  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