Re: [PATCH 08/10] drm/panthor: Automatically enable interrupts in panthor_fw_wait_acks()

Boris Brezillon
2026-05-04T11:02:22Z

On Fri, 1 May 2026 15:20:17 +0100
Steven Price <steven.price@arm.com> wrote:

> On 29/04/2026 10:38, Boris Brezillon wrote:
> > Rather than assuming an interrupt is always expected for request
> > acks, temporarily enable the relevant interrupts when the polling-wait
> > failed. This should hopefully reduce the number of interrupts the CPU
> > has to process.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>
> It seems to work, although I'm lightly uneasy about this because I'm not
> entirely sure whether the FW will immediately see the updates to
> ack_irq_mask and therefore whether there's a possibility to miss an
> event and be stuck waiting for the timeout.
>
> Memory models are not my strong point, OpenAI tells me the sequence
> should be something like:
>
> scoped_guard(spinlock_irqsave, lock) {
> u32 ack_irq_mask = READ_ONCE(*ack_irq_mask_ptr);
>
> WRITE_ONCE(*ack_irq_mask_ptr, ack_irq_mask | req_mask);
> }

Is this really needed? In which situation would the compiler/CPU decide
to re-order this read_update_modify sequence?

>
> /*
> * The FW interface can be mapped write-combine/Normal-NC.

I'm not too sure I see what the non-cached property has to do with it.
If it was cached we would still need this memory barrier, and in
addition, we'd need a cache flush if the FW is not IO-coherent.

>Make sure the
> * IRQ mask update is visible to the FW before sleeping waiting for
> the IRQ.
> */
> wmb();
>
> Which seems plausible. But I've long ago learnt that plausible doesn't
> mean much when dealing with memory models!

Yeah, I'm not too sure. I was honestly expecting the spinlock guard to
act as a memory barrier already, but maybe it's not enough.