Re: [PATCH v2 11/11] drm/panthor: Process GPU events in IRQ context

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

On Tue, 12 May 2026 15:40:41 -0700
Chia-I Wu <olvaffe@gmail.com> wrote:

> On Tue, May 12, 2026 at 5:09 AM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Tue, 12 May 2026 13:37:41 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >  
> > > The current panthor_gpu_irq_handler() logic is already IRQ-safe
> > > (no sleep or sleeping locks, spinlocks taken with irqsave in other
> > > contexts, etc), so let's toggle the switch and make it an hard IRQ
> > > handler.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  drivers/gpu/drm/panthor/panthor_gpu.c | 15 ++++++++-------
> > >  1 file changed, 8 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> > > index b9c51f8a051d..04c8f23baf3f 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> > > @@ -86,10 +86,15 @@ static void panthor_gpu_l2_config_set(struct panthor_device *ptdev)
> > >       gpu_write(gpu->iomem, GPU_L2_CONFIG, l2_config);
> > >  }
> > >
> > > -static void panthor_gpu_irq_handler(struct panthor_irq *pirq, u32 status)
> > > +static irqreturn_t panthor_gpu_irq_raw_handler(int irq, void *data)
> > >  {
> > > +     struct panthor_irq *pirq = data;
> > >       struct panthor_device *ptdev = pirq->ptdev;
> > >       struct panthor_gpu *gpu = ptdev->gpu;
> > > +     u32 status = gpu_read(gpu->irq.iomem, INT_STAT);
> > > +
> > > +     if (!status)
> > > +             return IRQ_NONE;
> > >  
> >
> > Forgot to add the pirq state transition here:
> >
> >        scoped_guard(spinlock_irqsave, &pirq->mask_lock) {
> >                if (pirq->state != PANTHOR_IRQ_STATE_ACTIVE)
> >                        return IRQ_NONE;
> >
> >                pirq->state = PANTHOR_IRQ_STATE_PROCESSING;
> >        }
> >  
> > >       gpu_write(gpu->irq.iomem, INT_CLEAR, status);
> > >
> > > @@ -115,11 +120,8 @@ static void panthor_gpu_irq_handler(struct panthor_irq *pirq, u32 status)
> > >               ptdev->gpu->pending_reqs &= ~status;
> > >               wake_up_all(&ptdev->gpu->reqs_acked);
> > >       }
> > > -}
> > >
> > > -static irqreturn_t panthor_gpu_irq_threaded_handler(int irq, void *data)
> > > -{
> > > -     return panthor_irq_default_threaded_handler(data, panthor_gpu_irq_handler);  
> >
> > and restore it here:
> >
> >        scoped_guard(spinlock_irqsave, &pirq->mask_lock) {
> >                if (pirq->state == PANTHOR_IRQ_STATE_PROCESSING)
> >                        pirq->state = PANTHOR_IRQ_STATE_ACTIVE;
> >        }
> >  
> It looks like we can get rid of state transitions if
> panthor_irq_{enable,disable}_events updates INT_MASK directly when the
> handler is not threaded.

Hm, this would add some conditionals to
panthor_irq_{enable,disable}_events() and it makes the whole thing even
harder to reason about, because now it's different depending on whether
this is a threaded handler or not.

> Hm, we can even make pirq->state atomic again
> to get rid of locking.

I'd say, if we really want to optimize that, we do it in a follow-up
series. And I'd rather have an attempt at turning the MMU handler into a
hard handler (which implies selecting what we process immediately and
what we defer) than adding conditionals to irq_enabled/disable_events.