On Wed, May 13, 2026 at 1:54 AM Boris Brezillon
<[email protected]> wrote:
>
> On Tue, 12 May 2026 15:40:41 -0700
> Chia-I Wu <[email protected]> wrote:
>
> > On Tue, May 12, 2026 at 5:09 AM Boris Brezillon
> > <[email protected]> wrote:
> > >
> > > On Tue, 12 May 2026 13:37:41 +0200
> > > Boris Brezillon <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.
The difference comes from the default raw handler clears INT_MASK
while the custom raw handler does not. That's how one interacts with
{enable,disable}_events and the other does not.
I think it makes sense to move work from hot raw handlers to cold
{enable,disable}_events in the event. It can certainly be a follow-up
series because the benefit is insignificant compared to this series :)
>
> > 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.
