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? 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. 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? 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? 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 Thanks, Paulo -- Paulo Zanoni
