On Wed, May 13, 2026 at 10:30 AM Boris Brezillon <[email protected]> wrote: > > On Wed, 13 May 2026 10:06:14 -0700 > Chia-I Wu <[email protected]> wrote: > > > On Wed, May 13, 2026 at 1:09 AM Boris Brezillon > > <[email protected]> wrote: > > > > > > On Tue, 12 May 2026 12:11:08 -0700 > > > Chia-I Wu <[email protected]> wrote: > > > > > > > On Tue, May 12, 2026 at 4:54 AM Boris Brezillon > > > > <[email protected]> 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 <[email protected]> > > > > > Reviewed-by: Steven Price <[email protected]> > > > > > Signed-off-by: Boris Brezillon <[email protected]> > > > > > --- > > > > > 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 <[email protected]>.
FWIW, I think this commit explains the motivation commit c37927a203fa283950f6045602b9f71328ad786c Author: Rafael J. Wysocki <[email protected]> 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).
