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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 18 May 2026 10:16:56 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 14 May 2026 15:25:07 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
> > On 12/05/2026 12:37, 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>    
> > 
> > As mentioned in the other thread[1] it turns out this won't work with
> > the current firmware.
> > 
> > The firmware checks the interrupt mask before signalling the ACK - so
> > enabling the bit in the mask just before waiting for it is problematic -
> > the firmware may not see the addition in the mask and will not trigger
> > the interrupt.  
> 
> Is it a problem though? wait_event_timeout() will evaluate the
> condition before going to sleep, so, if the FW raced with the
> input->ack_irq_mask update, I assume the condition will evaluate to
> true and wait_event_timeout() would return immediately. The only issue
> is if the FW updates the output->ack register after reading
> input->ack_irq_mask, but that would be weird, since the output->ack
> update doesn't depend on input->ack_irq_mask, and raising an interrupt
> before updating output->ack would be racy anyway.
> 
> Am I missing something?
> 

Quoting your reply to v1, for the context, because I missed it when
replying here:

> So I've looked at the firmware implementation and I can say that there's
> a race condition on the firmware side if we change the mask after
> sending the START/RESUME request. The firmware currently samples the
> mask before triggering the update to CSG_ACK and then uses the sampled
> value to decide whether to trigger an interrupt.

So, the race is:

CPU						MCU

CSG_REQ.STATE = START
atomic_poll(CSG_ACK & STATE_MASK)
						process_state_change()
							...
 							mask = CSG_ACK_IRQ_MASK
							...
							// done
CSG_ACK_IRQ_MASK |= STATE_MASK
wait(CSG_ACK & STATE_MASK)
							CSG_ACK = (CSG_ACK & STATE_MASK) | CSG_REQ.STATE
							if (mask & STATE_MASK) // evaluates to false
								raise_int()


It's a bit unfortunate, because conditionally enabling IRQs
only after the polling period expires reduces the number of
interrupts directly caused by requests initiated by the CPU
since it's assumed those requests will complete relatively
quickly and avoid a sleep+interrupt+wake-up round-trip. We
could of course update CSG_ACK_IRQ_MASK at the time we update
CSG_REQ, but that means we'll still get an interrupt, so I
guess we should stick to what exists now if there's no other
option. Oh well.

>
> This is all fine if the mask is set before the request (so nothing
> broken with the current code), but we'd need a firmware change before we
> can safely do what this patch was proposing. And of course we'd have to
> get our heads round the barriers needed! ;)

:-(