Re: [PATCH v2 04/11] drm/panthor: Extend the IRQ logic to allow fast/hard IRQ handlers

[Chia-I Wu]

On Wed, May 13, 2026 at 10:30 AM Boris Brezillon
<boris.brezillon@xxxxxxxxxxxxx> wrote:
>
> On Wed, 13 May 2026 10:06:14 -0700
> Chia-I Wu <olvaffe@xxxxxxxxx> wrote:
>
> > On Wed, May 13, 2026 at 1:09 AM Boris Brezillon
> > <boris.brezillon@xxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, 12 May 2026 12:11:08 -0700
> > > Chia-I Wu <olvaffe@xxxxxxxxx> wrote:
> > >
> > > > On Tue, May 12, 2026 at 4:54 AM Boris Brezillon
> > > > <boris.brezillon@xxxxxxxxxxxxx> wrote:
> > > > >
> > > > > All drivers except panthor signal their fences from their interrupt
> > > > > handler to minimize latency. We could do the same from the threaded
> > > > > handler, but the latency is still quite high in that case, so let's
> > > > > allow components to choose the context they want their IRQ handler
> > > > > to run in by exposing support for custom hard handlers.
> > > > >
> > > > > Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
> > > > > Reviewed-by: Steven Price <steven.price@xxxxxxx>
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > > > > ---
> > > > >  drivers/gpu/drm/panthor/panthor_device.h | 11 ++++++++---
> > > > >  drivers/gpu/drm/panthor/panthor_fw.c     |  1 +
> > > > >  drivers/gpu/drm/panthor/panthor_gpu.c    |  1 +
> > > > >  drivers/gpu/drm/panthor/panthor_mmu.c    |  1 +
> > > > >  drivers/gpu/drm/panthor/panthor_pwr.c    |  1 +
> > > > >  5 files changed, 12 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > > > > index 393fcda73d88..1aaf06df875b 100644
> > > > > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > > > > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > > > > @@ -672,6 +672,7 @@ static inline void panthor_irq_disable_events(struct panthor_irq *pirq, u32 mask
> > > > >  static inline int
> > > > >  panthor_irq_request(struct panthor_device *ptdev, struct panthor_irq *pirq,
> > > > >                     int irq, u32 mask, void __iomem *iomem, const char *name,
> > > > > +                   irqreturn_t (*raw_handler)(int, void *data),
> > > > >                     irqreturn_t (*threaded_handler)(int, void *data))
> > > > >  {
> > > > >         const char *full_name;
> > > > > @@ -687,9 +688,13 @@ panthor_irq_request(struct panthor_device *ptdev, struct panthor_irq *pirq,
> > > > >                 return -ENOMEM;
> > > > >
> > > > >         panthor_irq_resume(pirq);
> > > > > -       return devm_request_threaded_irq(ptdev->base.dev, irq,
> > > > > -                                        panthor_irq_default_raw_handler,
> > > > > -                                        threaded_handler,
> > > > > +
> > > > > +       if (!threaded_handler) {
> > > > > +               return devm_request_irq(ptdev->base.dev, irq, raw_handler,
> > > > > +                                       IRQF_SHARED, full_name, pirq);
> > > > > +       }
> > > > devm_request_irq expands to devm_request_threaded_irq plus
> > > > IRQF_COND_ONESHOT. This appears redundant.
> > >
> > > I considered going for devm_request_threaded_irq(COND_ONESHOT), but I
> > > thought it was easier to reason about with a regular devm_request_irq()
> > > and an extra conditional since request_irq() is what people tend
> > > to use when they just have a hard handler (see [1], there's just one
> > > driver using it, and it's not even needed, because it's calling
> > > devm_request_irq() which adds this flag already)
> > It is unclear to me why the current version wants IRQF_COND_ONESHOT in
> > one case but not in another. Can't we call devm_request_threaded_irq
> > without IRQF_COND_ONESHOT for both cases?
>
> Hm, I thought this had to do with the automatic hard -> threaded
> downgrade happening when RT is enabled, but I fail to see why it
> matters, since all irqaction end up with IRQF_ONESHOT in that case
> anyway. Honestly, I'm tempted to stay on the safe side, and have
> devm_request_irq() called when we just have a hard handler, because I'm
> sure there's a reason for this COND_ONESHOT flag.
Sounds good. Feel free to add Reviewed-by: Chia-I Wu <olvaffe@xxxxxxxxx>.

FWIW, I think this commit explains the motivation

  commit c37927a203fa283950f6045602b9f71328ad786c
  Author: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
  Date:   Thu Jul 11 12:20:04 2024 +0200

      genirq: Set IRQF_COND_ONESHOT in request_irq()

      The callers of request_irq() don't care about IRQF_ONESHOT because they
      don't provide threaded handlers, but if they happen to share the IRQ with
      the ACPI SCI, which has a threaded handler and sets IRQF_ONESHOT,
      request_irq() will fail for them due to a flags mismatch.

For panthor, my takeaway is we either care about flag mismatch (then
we should set IRQF_COND_ONESHOT for the threaded case) or we don't
(then we don't need to use devm_request_irq).