[Intel-gfx] proposals/questions about the IRQ registers

[Paulo Zanoni]

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