| Message ID | 1371037046-3732-11-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > > 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
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
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));
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. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 2 -- 1 file changed, 2 deletions(-)