| Message ID | 1374618835-28120-4-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 23, 2013 at 07:33:43PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > The problem here is that we have the PIPESTAT registers between IER > and IIR, so when we use intel_irq_reg_reset we flip the order used to > reset IIR and PIPESTAT. That should be safe since after we clear > IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the > change is on its own patch, so it should be easy to bisect and revert > if needed. This is wrong. PIPESTAT needs to be cleared before IIR. -Chris
2013/7/24 Chris Wilson <chris@chris-wilson.co.uk>: > On Tue, Jul 23, 2013 at 07:33:43PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> The problem here is that we have the PIPESTAT registers between IER >> and IIR, so when we use intel_irq_reg_reset we flip the order used to >> reset IIR and PIPESTAT. That should be safe since after we clear >> IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the >> change is on its own patch, so it should be easy to bisect and revert >> if needed. > > This is wrong. PIPESTAT needs to be cleared before IIR. Yeah, you're right. I see that if we want to make all the code touching pipestat consistent we could probably dedicate a full patch series to it. The fact that PIPESTAT needs to be disabled after IER/IMR but before IIR also kinda breaks my macros, I'll have to rethink them now. Thanks for the review! > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, Jul 24, 2013 at 11:14:57AM -0300, Paulo Zanoni wrote: > 2013/7/24 Chris Wilson <chris@chris-wilson.co.uk>: > > On Tue, Jul 23, 2013 at 07:33:43PM -0300, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> The problem here is that we have the PIPESTAT registers between IER > >> and IIR, so when we use intel_irq_reg_reset we flip the order used to > >> reset IIR and PIPESTAT. That should be safe since after we clear > >> IMR/IER we won't get any other IIR/PIPESTAT interrupts. Still, the > >> change is on its own patch, so it should be easy to bisect and revert > >> if needed. > > > > This is wrong. PIPESTAT needs to be cleared before IIR. > > Yeah, you're right. I see that if we want to make all the code > touching pipestat consistent we could probably dedicate a full patch > series to it. The fact that PIPESTAT needs to be disabled after > IER/IMR but before IIR also kinda breaks my macros, I'll have to > rethink them now. Why would it need to be cleared before IMR/IER? Clearing PIPESTAT first, them IER/IMR, and finally IIR should be OK.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 292337b..b1b6552 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2920,13 +2920,11 @@ static void i965_irq_uninstall(struct drm_device * dev) I915_WRITE(HWSTAM, 0xffffffff); for_each_pipe(pipe) I915_WRITE(PIPESTAT(pipe), 0); - I915_WRITE(IMR, 0xffffffff); - I915_WRITE(IER, 0x0); + INTEL_IRQ_REG_RESET(I, true); for_each_pipe(pipe) I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)) & 0x8000ffff); - I915_WRITE(IIR, I915_READ(IIR)); } static void i915_reenable_hotplug_timer_func(unsigned long data)