On Wed, 13 May 2026 11:17:26 -0700 Chia-I Wu <[email protected]> wrote:
> 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). 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.
