On Fri, Jun 26, 2026 at 01:40:28PM +0200, Boris Brezillon wrote: > On Fri, 26 Jun 2026 10:29:54 +0100 > Liviu Dudau <[email protected]> wrote: > > > On Thu, Jun 25, 2026 at 02:40:31PM +0200, Boris Brezillon wrote: > > > In theory, our hardirq handler can be called while the device (and > > > thus the panthor_irq) is suspended, because the IRQ line is shared. > > > In practice though, in all the designs we've seen, the line is only > > > shared within the GPU, and because sub-component suspend state is > > > consistent (all-suspended or all-resumed), we shouldn't end up with > > > an interrupt triggered while we're suspended. > > > > > > Fix the problem anyway, if nothing else, for our sanity. > > > > > > Fixes: 0b2d86670a84 ("drm/panthor: Rework panthor_irq::suspended into > > > panthor_irq::state") > > > Reported-by: [email protected] > > > Closes: > > > https://sashiko.dev/#/patchset/[email protected]?part=1 > > > Signed-off-by: Boris Brezillon <[email protected]> > > > --- > > > drivers/gpu/drm/panthor/panthor_device.h | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/panthor/panthor_device.h > > > b/drivers/gpu/drm/panthor/panthor_device.h > > > index 35679bfa1f3a..a39386bd6382 100644 > > > --- a/drivers/gpu/drm/panthor/panthor_device.h > > > +++ b/drivers/gpu/drm/panthor/panthor_device.h > > > @@ -512,9 +512,6 @@ static irqreturn_t panthor_ ## __name ## > > > _irq_raw_handler(int irq, void *data) > > > struct panthor_irq *pirq = data; > > > \ > > > enum panthor_irq_state old_state; > > > \ > > > > > > \ > > > - if (!gpu_read(pirq->iomem, INT_STAT)) > > > \ > > > - return IRQ_NONE; > > > \ > > > - > > > \ > > > guard(spinlock_irqsave)(&pirq->mask_lock); > > > \ > > > old_state = atomic_cmpxchg(&pirq->state, > > > \ > > > PANTHOR_IRQ_STATE_ACTIVE, > > > \ > > > @@ -522,6 +519,13 @@ static irqreturn_t panthor_ ## __name ## > > > _irq_raw_handler(int irq, void *data) > > > if (old_state != PANTHOR_IRQ_STATE_ACTIVE) > > > \ > > > return IRQ_NONE; > > > \ > > > > > > \ > > > + if (!gpu_read(pirq->iomem, INT_STAT)) { > > > \ > > > + atomic_cmpxchg(&pirq->state, > > > \ > > > + PANTHOR_IRQ_STATE_PROCESSING, > > > \ > > > + PANTHOR_IRQ_STATE_ACTIVE); > > > \ > > > + return IRQ_NONE; > > > \ > > > + } > > > \ > > > > Hmm, > > > > I get it that you're trying to revert the effect of the previous > > atomic_cmpxchg() here but it feels > > like a better option would be to not do the swap at all if the state is not > > ACTIVE. > > That's what [1] does, but it's not possible until we've made this > atomic -> lock-based transition, and I want a fix that's not dependent > on non-Fixes patches so we can backport it. > > [1]https://lore.kernel.org/dri-devel/[email protected]/
I see. Reviewed-by: Liviu Dudau <[email protected]> Best regards, Liviu -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯
