On Mon, Jul 08, 2013 at 05:20:21PM -0300, Paulo Zanoni wrote:
> Hi
>
> Due to the current features that I'm implementing I started looking at
> our interrupt code so I can reuse it. I noticed quite a few details
> and potential problems, and I can write patches for them, but first I
> think I should ask some questions to make sure I won't spend a big
> time writing patches that get NACKed because of a small detail I
> missed.
>
> Here's a list of questions/problems:
>
> 1 - IRQ preinstall/uninstall
>
> One of Daniel's comments implied that runing irq_uninstall should
> leave the hardware in the same state as runing irq_preinstall. I also
> read Documentation/DocBook/drm.tmpl and the text suggests the same
> thing. On our driver, we have the following differences between
> irq_uninstall and irq_preinstall:
>
> - irq_uninstall clears the IIR registers
> - irq_preinstall calls posting_read on the IER registers
> - HSWSTAM is set differently
> - some registers are only present in one of the functions (e.g,
> GEN6_PMI* is reset at preinstall, but it's completely forgotten at
> uninstall)
>
> Why do we have these differences? We could save a lot of source code
> lines by merging chunks of the irq_uninstall and irq_preinstall
> functions. We could make both functions clear the IIR register, and
> also do the posting_read if needed. If I propose such patches, is
> anybody going to reject them?
I think the right sequence in preinstall for the usul interrupts register
with 4 regs (IIR, ISR, IER and IMR) is:
1) mask everything in IMR, disable all interrupts in IER.
2) posting read on IER
3) clear IIR interrupts, posting read
4) repeat step 3)
With some C tricker that could be made to look rather tidy:
#define I915_DISABLE_AND_CLEAR_INTERRUPTS(prefix) \
do { \
I915_WRITE(prefix # IMR, 0xffffffff); \
I915_WRITE(prefix # IER, 0); \
POSTING_READ(prefix # IER); \
I915_WRITE(prefix # IIR, 0xffffffff); \
POSTING_READ(prefix # IIR); \
I915_WRITE(prefix # IIR, 0xffffffff); \
POSTING_READ(prefix # IIR); \
} while (0)
and then in the code
I915_DISABLE_AND_CLEAR_INTERRUPTS(GT);
I915_DISABLE_AND_CLEAR_INTERRUPTS(DE);
I915_DISABLE_AND_CLEAR_INTERRUPTS(GEN6_PM);
(totally untested, but I think you get the idea).
Sharing the code more between preinstall and uninstall on top of that
sounds nice.
> 2 - IIR register initialization
>
> 2.1- Writing twice vs writing once
>
> As documented on our specification, the IIR registers are capable of
> storing 2 interrupts, so if we get two interrupts and then write
> 0xFFFFFFFF to IIR, its value won't become 0, the bits that received 2
> interrupts will go back to 1. In most of our code, when we want to
> clear IIR we only do something like "I915_WRITE(XXX_IIR,
> I915_READ(XXX_IIR))", but this does not guarantee us that the XXX_IIR
> register will become 0. Shouldn't we write twice? Notice that GTIIR on
> valleyview_irq_preinstall is an exception.
>
> If I write a patch adding a macro/inline-function
> intel_init_iir(uint32_t register) that writes twice to make sure the
> value is 0, is this okay? This may even solve bugs since when we
> initialize the HW there's no guarantee that all the IIR registers are
> zero.
See above my suggestion for solving the IER, IMR, IIR initial setup
sequence in preinstall and clearing them in the uninstall hook. Note that
we need the C trickery to have flexibilty in case the relative register
offsets of IIR, IMR IER ever changes.
> 2.2 - How we clear them
>
> In most of the cases we do "I915_WRITE(XXX_IIR, I915_READ(XXX_IIR))".
> I understand this is what we should do after interrupts are processed,
> but when they are disabled, why not do I915_WRITE(XXX_IIR, 0xFFFFFFF)?
> That seems faster. We do this with the VLV_IIR register.
>
> Anybody against changing the code to use 0xFFFFFFFF when interrupts
> are disabled?
I think it's a good idea, so I've used it in my example code above
already.
> 2.3 - The interrupt handler
>
> On some interrupt handlers (e.g., ironlake_irq_handler) we only read
> the IIR register once and then clear it by rewriting the value we
> read. If we have 2 of the same interrupt stored in the queue, we will
> leave the IRQ handler with the IIR register with a non-zero value.
> Shouldn't we process the IIR registers on the IRQ handlers until they
> become zero? I patched my HSW machine to do this and it's working
> fine. I also expect this could make things a little bit faster or
> solve random bugs. Again, the valleyview_irq_handler function makes
> sure all IIR registers are zero.
>
> Anybody against this?
I'm against looping in the interrupt handler since it's a really good way
to paper over bugs. E.g. I expect that the south interrupt race you've
fixed could be paper over this way. But it'll obviously not fix the
underlying issue correctly.
So I prefer our interrupt handling code to not have such loops if
possible.
> All these patches could be quite big, so an upfront ACK/NACK on each
> of the 4 proposals would be good. Anyway, if we get a NACK for
> something we should probably write patches changing VLV code that uses
> it
I think if you split up the patches into interrupt register
blocks/platforms we should be able to get things in pretty quickly. At
least we'll have minimal blockage if something suspicious pops up.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
