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

[Boris Brezillon]

On Wed, 13 May 2026 11:17:26 -0700
Chia-I Wu <olvaffe@xxxxxxxxx> wrote:

> 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).

Fair enough. I'll use devm_request_threaded_irq() without
IRQF_COND_ONESHOT since I don't think we ever had an SoC where the IRQ
line was shared with other non-GPU blocks, and we set the same flags
for all GPU irqs.