* [PATCH v6 1/3] drm/panthor: Extend IRQ helpers for mask modification/restoration
2025-12-23 16:24 [PATCH v6 0/3] Add a few tracepoints to panthor Nicolas Frattaroli
@ 2025-12-23 16:24 ` Nicolas Frattaroli
2026-01-05 11:12 ` Boris Brezillon
2025-12-23 16:24 ` [PATCH v6 2/3] drm/panthor: Add tracepoint for hardware utilisation changes Nicolas Frattaroli
2025-12-23 16:25 ` [PATCH v6 3/3] drm/panthor: Add gpu_job_irq tracepoint Nicolas Frattaroli
2 siblings, 1 reply; 7+ messages in thread
From: Nicolas Frattaroli @ 2025-12-23 16:24 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Chia-I Wu, Karunika Choo
Cc: kernel, linux-kernel, dri-devel, Nicolas Frattaroli
The current IRQ helpers do not guarantee mutual exclusion that covers
the entire transaction from accessing the mask member and modifying the
mask register.
This makes it hard, if not impossible, to implement mask modification
helpers that may change one of these outside the normal
suspend/resume/isr code paths.
Add a spinlock to struct panthor_irq that protects both the mask member
and register. Acquire it in all code paths that access these, but drop
it before processing the threaded handler function. Then, add the
aforementioned new helpers: mask_enable, mask_disable, and
resume_restore. The first two work by ORing and NANDing the mask bits,
and the latter relies on the new behaviour that panthor_irq::mask is not
set to 0 on suspend.
panthor_irq::suspended remains an atomic, as it's necessarily written to
outside the mask_lock in the suspend path.
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
drivers/gpu/drm/panthor/panthor_device.h | 68 +++++++++++++++++++++++++++-----
1 file changed, 58 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index f35e52b9546a..bf554cf376fb 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -73,11 +73,14 @@ struct panthor_irq {
/** @irq: IRQ number. */
int irq;
- /** @mask: Current mask being applied to xxx_INT_MASK. */
+ /** @mask: Values to write to xxx_INT_MASK if active. */
u32 mask;
/** @suspended: Set to true when the IRQ is suspended. */
atomic_t suspended;
+
+ /** @mask_lock: protects modifications to _INT_MASK and @mask */
+ spinlock_t mask_lock;
};
/**
@@ -410,6 +413,8 @@ static irqreturn_t panthor_ ## __name ## _irq_raw_handler(int irq, void *data)
struct panthor_irq *pirq = data; \
struct panthor_device *ptdev = pirq->ptdev; \
\
+ guard(spinlock_irqsave)(&pirq->mask_lock); \
+ \
if (atomic_read(&pirq->suspended)) \
return IRQ_NONE; \
if (!gpu_read(ptdev, __reg_prefix ## _INT_STAT)) \
@@ -424,9 +429,14 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
struct panthor_irq *pirq = data; \
struct panthor_device *ptdev = pirq->ptdev; \
irqreturn_t ret = IRQ_NONE; \
+ u32 mask; \
+ \
+ scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
+ mask = pirq->mask; \
+ } \
\
while (true) { \
- u32 status = gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & pirq->mask; \
+ u32 status = (gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & mask); \
\
if (!status) \
break; \
@@ -435,26 +445,44 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
ret = IRQ_HANDLED; \
} \
\
- if (!atomic_read(&pirq->suspended)) \
- gpu_write(ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
+ scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
+ if (!atomic_read(&pirq->suspended)) { \
+ /* Only restore the bits that were used and are still enabled */ \
+ gpu_write(ptdev, __reg_prefix ## _INT_MASK, \
+ gpu_read(ptdev, __reg_prefix ## _INT_MASK) | \
+ (mask & pirq->mask)); \
+ } \
+ } \
\
return ret; \
} \
\
static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
{ \
- pirq->mask = 0; \
- gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
+ scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
+ gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
+ } \
synchronize_irq(pirq->irq); \
atomic_set(&pirq->suspended, true); \
} \
\
static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u32 mask) \
{ \
- atomic_set(&pirq->suspended, false); \
+ guard(spinlock_irqsave)(&pirq->mask_lock); \
+ \
pirq->mask = mask; \
- gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask); \
- gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask); \
+ atomic_set(&pirq->suspended, false); \
+ gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, pirq->mask); \
+ gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
+} \
+ \
+static inline void panthor_ ## __name ## _irq_resume_restore(struct panthor_irq *pirq) \
+{ \
+ guard(spinlock_irqsave)(&pirq->mask_lock); \
+ \
+ atomic_set(&pirq->suspended, false); \
+ gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, pirq->mask); \
+ gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
} \
\
static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
@@ -463,13 +491,33 @@ static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
{ \
pirq->ptdev = ptdev; \
pirq->irq = irq; \
- panthor_ ## __name ## _irq_resume(pirq, mask); \
+ pirq->mask = mask; \
+ spin_lock_init(&pirq->mask_lock); \
+ panthor_ ## __name ## _irq_resume_restore(pirq); \
\
return devm_request_threaded_irq(ptdev->base.dev, irq, \
panthor_ ## __name ## _irq_raw_handler, \
panthor_ ## __name ## _irq_threaded_handler, \
IRQF_SHARED, KBUILD_MODNAME "-" # __name, \
pirq); \
+} \
+ \
+static inline void panthor_ ## __name ## _irq_mask_enable(struct panthor_irq *pirq, u32 mask) \
+{ \
+ guard(spinlock_irqsave)(&pirq->mask_lock); \
+ \
+ pirq->mask |= mask; \
+ if (!atomic_read(&pirq->suspended)) \
+ gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
+} \
+ \
+static inline void panthor_ ## __name ## _irq_mask_disable(struct panthor_irq *pirq, u32 mask) \
+{ \
+ guard(spinlock_irqsave)(&pirq->mask_lock); \
+ \
+ pirq->mask &= ~mask; \
+ if (!atomic_read(&pirq->suspended)) \
+ gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
}
extern struct workqueue_struct *panthor_cleanup_wq;
--
2.52.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v6 1/3] drm/panthor: Extend IRQ helpers for mask modification/restoration
2025-12-23 16:24 ` [PATCH v6 1/3] drm/panthor: Extend IRQ helpers for mask modification/restoration Nicolas Frattaroli
@ 2026-01-05 11:12 ` Boris Brezillon
2026-01-05 13:17 ` Nicolas Frattaroli
0 siblings, 1 reply; 7+ messages in thread
From: Boris Brezillon @ 2026-01-05 11:12 UTC (permalink / raw)
To: Nicolas Frattaroli
Cc: Steven Price, Liviu Dudau, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Chia-I Wu,
Karunika Choo, kernel, linux-kernel, dri-devel
On Tue, 23 Dec 2025 17:24:58 +0100
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> The current IRQ helpers do not guarantee mutual exclusion that covers
> the entire transaction from accessing the mask member and modifying the
> mask register.
>
> This makes it hard, if not impossible, to implement mask modification
> helpers that may change one of these outside the normal
> suspend/resume/isr code paths.
>
> Add a spinlock to struct panthor_irq that protects both the mask member
> and register. Acquire it in all code paths that access these, but drop
> it before processing the threaded handler function. Then, add the
> aforementioned new helpers: mask_enable, mask_disable, and
> resume_restore. The first two work by ORing and NANDing the mask bits,
> and the latter relies on the new behaviour that panthor_irq::mask is not
> set to 0 on suspend.
>
> panthor_irq::suspended remains an atomic, as it's necessarily written to
> outside the mask_lock in the suspend path.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> drivers/gpu/drm/panthor/panthor_device.h | 68 +++++++++++++++++++++++++++-----
> 1 file changed, 58 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index f35e52b9546a..bf554cf376fb 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -73,11 +73,14 @@ struct panthor_irq {
> /** @irq: IRQ number. */
> int irq;
>
> - /** @mask: Current mask being applied to xxx_INT_MASK. */
> + /** @mask: Values to write to xxx_INT_MASK if active. */
> u32 mask;
>
> /** @suspended: Set to true when the IRQ is suspended. */
> atomic_t suspended;
> +
> + /** @mask_lock: protects modifications to _INT_MASK and @mask */
> + spinlock_t mask_lock;
> };
>
> /**
> @@ -410,6 +413,8 @@ static irqreturn_t panthor_ ## __name ## _irq_raw_handler(int irq, void *data)
> struct panthor_irq *pirq = data; \
> struct panthor_device *ptdev = pirq->ptdev; \
> \
> + guard(spinlock_irqsave)(&pirq->mask_lock); \
> + \
> if (atomic_read(&pirq->suspended)) \
> return IRQ_NONE; \
> if (!gpu_read(ptdev, __reg_prefix ## _INT_STAT)) \
> @@ -424,9 +429,14 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> struct panthor_irq *pirq = data; \
> struct panthor_device *ptdev = pirq->ptdev; \
> irqreturn_t ret = IRQ_NONE; \
> + u32 mask; \
> + \
> + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
> + mask = pirq->mask; \
> + } \
> \
> while (true) { \
> - u32 status = gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & pirq->mask; \
> + u32 status = (gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & mask); \
> \
> if (!status) \
> break; \
> @@ -435,26 +445,44 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> ret = IRQ_HANDLED; \
> } \
> \
> - if (!atomic_read(&pirq->suspended)) \
> - gpu_write(ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
> + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
> + if (!atomic_read(&pirq->suspended)) { \
> + /* Only restore the bits that were used and are still enabled */ \
> + gpu_write(ptdev, __reg_prefix ## _INT_MASK, \
> + gpu_read(ptdev, __reg_prefix ## _INT_MASK) | \
> + (mask & pirq->mask)); \
> + } \
> + } \
> \
> return ret; \
> } \
> \
> static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
> { \
> - pirq->mask = 0; \
> - gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
> + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
> + } \
> synchronize_irq(pirq->irq); \
> atomic_set(&pirq->suspended, true); \
> } \
> \
> static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u32 mask) \
If pirq->mask is encoding the user-selected mask, there's no point
passing it as an argument to _irq_resume().
> { \
> - atomic_set(&pirq->suspended, false); \
> + guard(spinlock_irqsave)(&pirq->mask_lock); \
> + \
> pirq->mask = mask; \
> - gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask); \
> - gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask); \
> + atomic_set(&pirq->suspended, false); \
> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, pirq->mask); \
> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
> +} \
> + \
> +static inline void panthor_ ## __name ## _irq_resume_restore(struct panthor_irq *pirq) \
As mentioned above, I'd just change the semantics of _irq_resume() to
match _irq_resume_restore() and drop _irq_resume_restore().
> +{ \
> + guard(spinlock_irqsave)(&pirq->mask_lock); \
> + \
> + atomic_set(&pirq->suspended, false); \
> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, pirq->mask); \
> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
> } \
> \
> static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
> @@ -463,13 +491,33 @@ static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
> { \
> pirq->ptdev = ptdev; \
> pirq->irq = irq; \
> - panthor_ ## __name ## _irq_resume(pirq, mask); \
> + pirq->mask = mask; \
> + spin_lock_init(&pirq->mask_lock); \
> + panthor_ ## __name ## _irq_resume_restore(pirq); \
> \
> return devm_request_threaded_irq(ptdev->base.dev, irq, \
> panthor_ ## __name ## _irq_raw_handler, \
> panthor_ ## __name ## _irq_threaded_handler, \
> IRQF_SHARED, KBUILD_MODNAME "-" # __name, \
> pirq); \
> +} \
> + \
> +static inline void panthor_ ## __name ## _irq_mask_enable(struct panthor_irq *pirq, u32 mask) \
nit: I think I prefer _irq_{enable,disable}_events() as a name.
> +{ \
> + guard(spinlock_irqsave)(&pirq->mask_lock); \
> + \
> + pirq->mask |= mask; \
> + if (!atomic_read(&pirq->suspended)) \
> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
There's still a problem with this solution: you might be re-enabling
interrupts before the threaded handler is done, causing HW interrupts
to needlessly fire. You need to repurpose ::suspended into multi-state
field (ACTIVE, PROCESSING, SUSPENDING, SUSPENDED) to detect the case
where the interrupt is being processed and only write to _INT_MASK if
this ::state == ACTIVE.
> +} \
> + \
> +static inline void panthor_ ## __name ## _irq_mask_disable(struct panthor_irq *pirq, u32 mask) \
> +{ \
> + guard(spinlock_irqsave)(&pirq->mask_lock); \
> + \
> + pirq->mask &= ~mask; \
> + if (!atomic_read(&pirq->suspended)) \
> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
> }
>
> extern struct workqueue_struct *panthor_cleanup_wq;
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v6 1/3] drm/panthor: Extend IRQ helpers for mask modification/restoration
2026-01-05 11:12 ` Boris Brezillon
@ 2026-01-05 13:17 ` Nicolas Frattaroli
2026-01-05 13:29 ` Boris Brezillon
0 siblings, 1 reply; 7+ messages in thread
From: Nicolas Frattaroli @ 2026-01-05 13:17 UTC (permalink / raw)
To: Boris Brezillon
Cc: Steven Price, Liviu Dudau, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Chia-I Wu,
Karunika Choo, kernel, linux-kernel, dri-devel
On Monday, 5 January 2026 12:12:20 Central European Standard Time Boris Brezillon wrote:
> On Tue, 23 Dec 2025 17:24:58 +0100
> Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
>
> > The current IRQ helpers do not guarantee mutual exclusion that covers
> > the entire transaction from accessing the mask member and modifying the
> > mask register.
> >
> > This makes it hard, if not impossible, to implement mask modification
> > helpers that may change one of these outside the normal
> > suspend/resume/isr code paths.
> >
> > Add a spinlock to struct panthor_irq that protects both the mask member
> > and register. Acquire it in all code paths that access these, but drop
> > it before processing the threaded handler function. Then, add the
> > aforementioned new helpers: mask_enable, mask_disable, and
> > resume_restore. The first two work by ORing and NANDing the mask bits,
> > and the latter relies on the new behaviour that panthor_irq::mask is not
> > set to 0 on suspend.
> >
> > panthor_irq::suspended remains an atomic, as it's necessarily written to
> > outside the mask_lock in the suspend path.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> > drivers/gpu/drm/panthor/panthor_device.h | 68 +++++++++++++++++++++++++++-----
> > 1 file changed, 58 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > index f35e52b9546a..bf554cf376fb 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > @@ -73,11 +73,14 @@ struct panthor_irq {
> > /** @irq: IRQ number. */
> > int irq;
> >
> > - /** @mask: Current mask being applied to xxx_INT_MASK. */
> > + /** @mask: Values to write to xxx_INT_MASK if active. */
> > u32 mask;
> >
> > /** @suspended: Set to true when the IRQ is suspended. */
> > atomic_t suspended;
> > +
> > + /** @mask_lock: protects modifications to _INT_MASK and @mask */
> > + spinlock_t mask_lock;
> > };
> >
> > /**
> > @@ -410,6 +413,8 @@ static irqreturn_t panthor_ ## __name ## _irq_raw_handler(int irq, void *data)
> > struct panthor_irq *pirq = data; \
> > struct panthor_device *ptdev = pirq->ptdev; \
> > \
> > + guard(spinlock_irqsave)(&pirq->mask_lock); \
> > + \
> > if (atomic_read(&pirq->suspended)) \
> > return IRQ_NONE; \
> > if (!gpu_read(ptdev, __reg_prefix ## _INT_STAT)) \
> > @@ -424,9 +429,14 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> > struct panthor_irq *pirq = data; \
> > struct panthor_device *ptdev = pirq->ptdev; \
> > irqreturn_t ret = IRQ_NONE; \
> > + u32 mask; \
> > + \
> > + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
> > + mask = pirq->mask; \
> > + } \
> > \
> > while (true) { \
> > - u32 status = gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & pirq->mask; \
> > + u32 status = (gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & mask); \
> > \
> > if (!status) \
> > break; \
> > @@ -435,26 +445,44 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> > ret = IRQ_HANDLED; \
> > } \
> > \
> > - if (!atomic_read(&pirq->suspended)) \
> > - gpu_write(ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
> > + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
> > + if (!atomic_read(&pirq->suspended)) { \
> > + /* Only restore the bits that were used and are still enabled */ \
> > + gpu_write(ptdev, __reg_prefix ## _INT_MASK, \
> > + gpu_read(ptdev, __reg_prefix ## _INT_MASK) | \
> > + (mask & pirq->mask)); \
> > + } \
> > + } \
> > \
> > return ret; \
> > } \
> > \
> > static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
> > { \
> > - pirq->mask = 0; \
> > - gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
> > + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
> > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
> > + } \
> > synchronize_irq(pirq->irq); \
> > atomic_set(&pirq->suspended, true); \
> > } \
> > \
> > static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u32 mask) \
>
> If pirq->mask is encoding the user-selected mask, there's no point
> passing it as an argument to _irq_resume().
There is. I don't want to refactor all of panthor_mmu and the
stuff it does with the mask. It needs to set-mask-and-resume in a
race-free manner, and that's not possible unless we keep this API
around, or we do some heavy refactoring. Remember that locks in the
kernel aren't reentrant, so we can't just acquire the lock in
panthor_mmu, set the mask, and then resume the IRQ, and then drop
the lock, as we'd be re-acquiring the lock in resume.
>
> > { \
> > - atomic_set(&pirq->suspended, false); \
> > + guard(spinlock_irqsave)(&pirq->mask_lock); \
> > + \
> > pirq->mask = mask; \
> > - gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask); \
> > - gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask); \
> > + atomic_set(&pirq->suspended, false); \
> > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, pirq->mask); \
> > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
> > +} \
> > + \
> > +static inline void panthor_ ## __name ## _irq_resume_restore(struct panthor_irq *pirq) \
>
> As mentioned above, I'd just change the semantics of _irq_resume() to
> match _irq_resume_restore() and drop _irq_resume_restore().
>
> > +{ \
> > + guard(spinlock_irqsave)(&pirq->mask_lock); \
> > + \
> > + atomic_set(&pirq->suspended, false); \
> > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, pirq->mask); \
> > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
> > } \
> > \
> > static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
> > @@ -463,13 +491,33 @@ static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev, \
> > { \
> > pirq->ptdev = ptdev; \
> > pirq->irq = irq; \
> > - panthor_ ## __name ## _irq_resume(pirq, mask); \
> > + pirq->mask = mask; \
> > + spin_lock_init(&pirq->mask_lock); \
> > + panthor_ ## __name ## _irq_resume_restore(pirq); \
> > \
> > return devm_request_threaded_irq(ptdev->base.dev, irq, \
> > panthor_ ## __name ## _irq_raw_handler, \
> > panthor_ ## __name ## _irq_threaded_handler, \
> > IRQF_SHARED, KBUILD_MODNAME "-" # __name, \
> > pirq); \
> > +} \
> > + \
> > +static inline void panthor_ ## __name ## _irq_mask_enable(struct panthor_irq *pirq, u32 mask) \
>
> nit: I think I prefer _irq_{enable,disable}_events() as a name.
Agreed, that seems clearer.
>
> > +{ \
> > + guard(spinlock_irqsave)(&pirq->mask_lock); \
> > + \
> > + pirq->mask |= mask; \
> > + if (!atomic_read(&pirq->suspended)) \
> > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
>
> There's still a problem with this solution: you might be re-enabling
> interrupts before the threaded handler is done, causing HW interrupts
> to needlessly fire. You need to repurpose ::suspended into multi-state
> field (ACTIVE, PROCESSING, SUSPENDING, SUSPENDED) to detect the case
> where the interrupt is being processed and only write to _INT_MASK if
> this ::state == ACTIVE.
Hmmm, yeah, I think there's no getting around needing more state
here. I'll do that.
Kind regards,
Nicolas Frattaroli
> > +} \
> > + \
> > +static inline void panthor_ ## __name ## _irq_mask_disable(struct panthor_irq *pirq, u32 mask) \
> > +{ \
> > + guard(spinlock_irqsave)(&pirq->mask_lock); \
> > + \
> > + pirq->mask &= ~mask; \
> > + if (!atomic_read(&pirq->suspended)) \
> > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
> > }
> >
> > extern struct workqueue_struct *panthor_cleanup_wq;
> >
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v6 1/3] drm/panthor: Extend IRQ helpers for mask modification/restoration
2026-01-05 13:17 ` Nicolas Frattaroli
@ 2026-01-05 13:29 ` Boris Brezillon
0 siblings, 0 replies; 7+ messages in thread
From: Boris Brezillon @ 2026-01-05 13:29 UTC (permalink / raw)
To: Nicolas Frattaroli
Cc: Steven Price, Liviu Dudau, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Chia-I Wu,
Karunika Choo, kernel, linux-kernel, dri-devel
On Mon, 05 Jan 2026 14:17:55 +0100
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> On Monday, 5 January 2026 12:12:20 Central European Standard Time Boris Brezillon wrote:
> > On Tue, 23 Dec 2025 17:24:58 +0100
> > Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> >
> > > The current IRQ helpers do not guarantee mutual exclusion that covers
> > > the entire transaction from accessing the mask member and modifying the
> > > mask register.
> > >
> > > This makes it hard, if not impossible, to implement mask modification
> > > helpers that may change one of these outside the normal
> > > suspend/resume/isr code paths.
> > >
> > > Add a spinlock to struct panthor_irq that protects both the mask member
> > > and register. Acquire it in all code paths that access these, but drop
> > > it before processing the threaded handler function. Then, add the
> > > aforementioned new helpers: mask_enable, mask_disable, and
> > > resume_restore. The first two work by ORing and NANDing the mask bits,
> > > and the latter relies on the new behaviour that panthor_irq::mask is not
> > > set to 0 on suspend.
> > >
> > > panthor_irq::suspended remains an atomic, as it's necessarily written to
> > > outside the mask_lock in the suspend path.
> > >
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > > drivers/gpu/drm/panthor/panthor_device.h | 68 +++++++++++++++++++++++++++-----
> > > 1 file changed, 58 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > > index f35e52b9546a..bf554cf376fb 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > > @@ -73,11 +73,14 @@ struct panthor_irq {
> > > /** @irq: IRQ number. */
> > > int irq;
> > >
> > > - /** @mask: Current mask being applied to xxx_INT_MASK. */
> > > + /** @mask: Values to write to xxx_INT_MASK if active. */
> > > u32 mask;
> > >
> > > /** @suspended: Set to true when the IRQ is suspended. */
> > > atomic_t suspended;
> > > +
> > > + /** @mask_lock: protects modifications to _INT_MASK and @mask */
> > > + spinlock_t mask_lock;
> > > };
> > >
> > > /**
> > > @@ -410,6 +413,8 @@ static irqreturn_t panthor_ ## __name ## _irq_raw_handler(int irq, void *data)
> > > struct panthor_irq *pirq = data; \
> > > struct panthor_device *ptdev = pirq->ptdev; \
> > > \
> > > + guard(spinlock_irqsave)(&pirq->mask_lock); \
> > > + \
> > > if (atomic_read(&pirq->suspended)) \
> > > return IRQ_NONE; \
> > > if (!gpu_read(ptdev, __reg_prefix ## _INT_STAT)) \
> > > @@ -424,9 +429,14 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> > > struct panthor_irq *pirq = data; \
> > > struct panthor_device *ptdev = pirq->ptdev; \
> > > irqreturn_t ret = IRQ_NONE; \
> > > + u32 mask; \
> > > + \
> > > + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
> > > + mask = pirq->mask; \
> > > + } \
> > > \
> > > while (true) { \
> > > - u32 status = gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & pirq->mask; \
> > > + u32 status = (gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & mask); \
> > > \
> > > if (!status) \
> > > break; \
> > > @@ -435,26 +445,44 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> > > ret = IRQ_HANDLED; \
> > > } \
> > > \
> > > - if (!atomic_read(&pirq->suspended)) \
> > > - gpu_write(ptdev, __reg_prefix ## _INT_MASK, pirq->mask); \
> > > + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
> > > + if (!atomic_read(&pirq->suspended)) { \
> > > + /* Only restore the bits that were used and are still enabled */ \
> > > + gpu_write(ptdev, __reg_prefix ## _INT_MASK, \
> > > + gpu_read(ptdev, __reg_prefix ## _INT_MASK) | \
> > > + (mask & pirq->mask)); \
> > > + } \
> > > + } \
> > > \
> > > return ret; \
> > > } \
> > > \
> > > static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
> > > { \
> > > - pirq->mask = 0; \
> > > - gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
> > > + scoped_guard(spinlock_irqsave, &pirq->mask_lock) { \
> > > + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
> > > + } \
> > > synchronize_irq(pirq->irq); \
> > > atomic_set(&pirq->suspended, true); \
> > > } \
> > > \
> > > static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u32 mask) \
> >
> > If pirq->mask is encoding the user-selected mask, there's no point
> > passing it as an argument to _irq_resume().
>
> There is. I don't want to refactor all of panthor_mmu and the
> stuff it does with the mask. It needs to set-mask-and-resume in a
> race-free manner, and that's not possible unless we keep this API
> around, or we do some heavy refactoring.
That's problematic I think. It means we have two different semantics
for panthor_irq::mask now. One where it directly reflects the mask
wanted by its user (GPU, JOB, PWR) and one where it's not (MMU).
> Remember that locks in the
> kernel aren't reentrant, so we can't just acquire the lock in
> panthor_mmu, set the mask, and then resume the IRQ, and then drop
> the lock, as we'd be re-acquiring the lock in resume.
But you shouldn't have to, because panthor_irq::mask should always
reflect the user requested mask, so whatever is in panthor_irq::mask at
resume time is the thing we should push to INT_MASK, and that we do
with the ::mask_lock held to avoid races. What we need to do though, is
patch panthor_mmu.c to use _irq_{enable,disable}_events() instead of the
open-coded version we have at the moment.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v6 2/3] drm/panthor: Add tracepoint for hardware utilisation changes
2025-12-23 16:24 [PATCH v6 0/3] Add a few tracepoints to panthor Nicolas Frattaroli
2025-12-23 16:24 ` [PATCH v6 1/3] drm/panthor: Extend IRQ helpers for mask modification/restoration Nicolas Frattaroli
@ 2025-12-23 16:24 ` Nicolas Frattaroli
2025-12-23 16:25 ` [PATCH v6 3/3] drm/panthor: Add gpu_job_irq tracepoint Nicolas Frattaroli
2 siblings, 0 replies; 7+ messages in thread
From: Nicolas Frattaroli @ 2025-12-23 16:24 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Chia-I Wu, Karunika Choo
Cc: kernel, linux-kernel, dri-devel, Nicolas Frattaroli
Mali GPUs have three registers that indicate which parts of the hardware
are powered at any moment. These take the form of bitmaps. In the case
of SHADER_READY for example, a high bit indicates that the shader core
corresponding to that bit index is powered on. These bitmaps aren't
solely contiguous bits, as it's common to have holes in the sequence of
shader core indices, and the actual set of which cores are present is
defined by the "shader present" register.
When the GPU finishes a power state transition, it fires a
GPU_IRQ_POWER_CHANGED_ALL interrupt. After such an interrupt is
received, the _READY registers will contain new interesting data. During
power transitions, the GPU_IRQ_POWER_CHANGED interrupt will fire, and
the registers will likewise contain potentially changed data.
This is not to be confused with the PWR_IRQ_POWER_CHANGED_ALL interrupt,
which is something related to Mali v14+'s power control logic. The
_READY registers and corresponding interrupts are already available in
v9 and onwards.
Expose the data as a tracepoint to userspace. This allows users to debug
various scenarios and gather interesting information, such as: knowing
how much hardware is lit up at any given time, correlating graphics
corruption with a specific powered shader core, measuring when hardware
is allowed to go to a powered off state again, and so on.
The registration/unregistration functions for the tracepoint go through
a wrapper in panthor_hw.c, so that v14+ can implement the same
tracepoint by adding its hardware specific IRQ on/off callbacks to the
panthor_hw.ops member.
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
drivers/gpu/drm/panthor/panthor_gpu.c | 30 +++++++++++++++-
drivers/gpu/drm/panthor/panthor_gpu.h | 2 ++
drivers/gpu/drm/panthor/panthor_hw.c | 62 +++++++++++++++++++++++++++++++++
drivers/gpu/drm/panthor/panthor_hw.h | 8 +++++
drivers/gpu/drm/panthor/panthor_trace.h | 58 ++++++++++++++++++++++++++++++
5 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index 057e167468d0..fc0a8c2aa7c7 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -22,6 +22,9 @@
#include "panthor_hw.h"
#include "panthor_regs.h"
+#define CREATE_TRACE_POINTS
+#include "panthor_trace.h"
+
/**
* struct panthor_gpu - GPU block management data.
*/
@@ -48,6 +51,9 @@ struct panthor_gpu {
GPU_IRQ_RESET_COMPLETED | \
GPU_IRQ_CLEAN_CACHES_COMPLETED)
+#define GPU_POWER_INTERRUPTS_MASK \
+ (GPU_IRQ_POWER_CHANGED | GPU_IRQ_POWER_CHANGED_ALL)
+
static void panthor_gpu_coherency_set(struct panthor_device *ptdev)
{
gpu_write(ptdev, GPU_COHERENCY_PROTOCOL,
@@ -80,6 +86,12 @@ static void panthor_gpu_irq_handler(struct panthor_device *ptdev, u32 status)
{
gpu_write(ptdev, GPU_INT_CLEAR, status);
+ if (tracepoint_enabled(gpu_power_status) && (status & GPU_POWER_INTERRUPTS_MASK))
+ trace_gpu_power_status(ptdev->base.dev,
+ gpu_read64(ptdev, SHADER_READY),
+ gpu_read64(ptdev, TILER_READY),
+ gpu_read64(ptdev, L2_READY));
+
if (status & GPU_IRQ_FAULT) {
u32 fault_status = gpu_read(ptdev, GPU_FAULT_STATUS);
u64 address = gpu_read64(ptdev, GPU_FAULT_ADDR);
@@ -157,6 +169,22 @@ int panthor_gpu_init(struct panthor_device *ptdev)
return 0;
}
+int panthor_gpu_power_changed_on(struct panthor_device *ptdev)
+{
+ guard(pm_runtime_active)(ptdev->base.dev);
+
+ panthor_gpu_irq_mask_enable(&ptdev->gpu->irq, GPU_POWER_INTERRUPTS_MASK);
+
+ return 0;
+}
+
+void panthor_gpu_power_changed_off(struct panthor_device *ptdev)
+{
+ guard(pm_runtime_active)(ptdev->base.dev);
+
+ panthor_gpu_irq_mask_disable(&ptdev->gpu->irq, GPU_POWER_INTERRUPTS_MASK);
+}
+
/**
* panthor_gpu_block_power_off() - Power-off a specific block of the GPU
* @ptdev: Device.
@@ -395,7 +423,7 @@ void panthor_gpu_suspend(struct panthor_device *ptdev)
*/
void panthor_gpu_resume(struct panthor_device *ptdev)
{
- panthor_gpu_irq_resume(&ptdev->gpu->irq, GPU_INTERRUPTS_MASK);
+ panthor_gpu_irq_resume_restore(&ptdev->gpu->irq);
panthor_hw_l2_power_on(ptdev);
}
diff --git a/drivers/gpu/drm/panthor/panthor_gpu.h b/drivers/gpu/drm/panthor/panthor_gpu.h
index 12e66f48ced1..12c263a39928 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.h
+++ b/drivers/gpu/drm/panthor/panthor_gpu.h
@@ -51,5 +51,7 @@ int panthor_gpu_l2_power_on(struct panthor_device *ptdev);
int panthor_gpu_flush_caches(struct panthor_device *ptdev,
u32 l2, u32 lsc, u32 other);
int panthor_gpu_soft_reset(struct panthor_device *ptdev);
+void panthor_gpu_power_changed_off(struct panthor_device *ptdev);
+int panthor_gpu_power_changed_on(struct panthor_device *ptdev);
#endif
diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
index 87ebb7ae42c4..ae3320d0e251 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.c
+++ b/drivers/gpu/drm/panthor/panthor_hw.c
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: GPL-2.0 or MIT
/* Copyright 2025 ARM Limited. All rights reserved. */
+#include <linux/platform_device.h>
+
#include <drm/drm_print.h>
#include "panthor_device.h"
@@ -29,6 +31,8 @@ static struct panthor_hw panthor_hw_arch_v10 = {
.soft_reset = panthor_gpu_soft_reset,
.l2_power_off = panthor_gpu_l2_power_off,
.l2_power_on = panthor_gpu_l2_power_on,
+ .power_changed_off = panthor_gpu_power_changed_off,
+ .power_changed_on = panthor_gpu_power_changed_on,
},
};
@@ -53,6 +57,64 @@ static struct panthor_hw_entry panthor_hw_match[] = {
},
};
+static int panthor_hw_set_power_tracing(struct device *dev, void *data)
+{
+ struct panthor_device *ptdev = dev_get_drvdata(dev);
+
+ if (!ptdev)
+ return -ENODEV;
+
+ if (!ptdev->hw)
+ return 0;
+
+ if (data) {
+ if (ptdev->hw->ops.power_changed_on)
+ return ptdev->hw->ops.power_changed_on(ptdev);
+ } else {
+ if (ptdev->hw->ops.power_changed_off)
+ ptdev->hw->ops.power_changed_off(ptdev);
+ }
+
+ return 0;
+}
+
+int panthor_hw_power_status_register(void)
+{
+ struct device_driver *drv;
+ int ret;
+
+ drv = driver_find("panthor", &platform_bus_type);
+ if (!drv)
+ return -ENODEV;
+
+ ret = driver_for_each_device(drv, NULL, (void *)true,
+ panthor_hw_set_power_tracing);
+
+ return ret;
+}
+
+void panthor_hw_power_status_unregister(void)
+{
+ struct device_driver *drv;
+ int ret;
+
+ drv = driver_find("panthor", &platform_bus_type);
+ if (!drv)
+ return;
+
+ ret = driver_for_each_device(drv, NULL, NULL, panthor_hw_set_power_tracing);
+
+ /*
+ * Ideally, it'd be possible to ask driver_for_each_device to hand us
+ * another "start" to keep going after the failing device, but it
+ * doesn't do that. Minor inconvenience in what is probably a bad day
+ * on the computer already though.
+ */
+ if (ret)
+ pr_warn("Couldn't mask power IRQ for at least one device: %pe\n",
+ ERR_PTR(ret));
+}
+
static char *get_gpu_model_name(struct panthor_device *ptdev)
{
const u32 gpu_id = ptdev->gpu_info.gpu_id;
diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
index 56c68c1e9c26..2c28aea82841 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.h
+++ b/drivers/gpu/drm/panthor/panthor_hw.h
@@ -19,6 +19,12 @@ struct panthor_hw_ops {
/** @l2_power_on: L2 power on function pointer */
int (*l2_power_on)(struct panthor_device *ptdev);
+
+ /** @power_changed_on: Start listening to power change IRQs */
+ int (*power_changed_on)(struct panthor_device *ptdev);
+
+ /** @power_changed_off: Stop listening to power change IRQs */
+ void (*power_changed_off)(struct panthor_device *ptdev);
};
/**
@@ -32,6 +38,8 @@ struct panthor_hw {
};
int panthor_hw_init(struct panthor_device *ptdev);
+int panthor_hw_power_status_register(void);
+void panthor_hw_power_status_unregister(void);
static inline int panthor_hw_soft_reset(struct panthor_device *ptdev)
{
diff --git a/drivers/gpu/drm/panthor/panthor_trace.h b/drivers/gpu/drm/panthor/panthor_trace.h
new file mode 100644
index 000000000000..5bd420894745
--- /dev/null
+++ b/drivers/gpu/drm/panthor/panthor_trace.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+/* Copyright 2025 Collabora ltd. */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM panthor
+
+#if !defined(__PANTHOR_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
+#define __PANTHOR_TRACE_H__
+
+#include <linux/tracepoint.h>
+#include <linux/types.h>
+
+#include "panthor_hw.h"
+
+/**
+ * gpu_power_status - called whenever parts of GPU hardware are turned on or off
+ * @dev: pointer to the &struct device, for printing the device name
+ * @shader_bitmap: bitmap where a high bit indicates the shader core at a given
+ * bit index is on, and a low bit indicates a shader core is
+ * either powered off or absent
+ * @tiler_bitmap: bitmap where a high bit indicates the tiler unit at a given
+ * bit index is on, and a low bit indicates a tiler unit is
+ * either powered off or absent
+ * @l2_bitmap: bitmap where a high bit indicates the L2 cache at a given bit
+ * index is on, and a low bit indicates the L2 cache is either
+ * powered off or absent
+ */
+TRACE_EVENT_FN(gpu_power_status,
+ TP_PROTO(const struct device *dev, u64 shader_bitmap, u64 tiler_bitmap,
+ u64 l2_bitmap),
+ TP_ARGS(dev, shader_bitmap, tiler_bitmap, l2_bitmap),
+ TP_STRUCT__entry(
+ __string(dev_name, dev_name(dev))
+ __field(u64, shader_bitmap)
+ __field(u64, tiler_bitmap)
+ __field(u64, l2_bitmap)
+ ),
+ TP_fast_assign(
+ __assign_str(dev_name);
+ __entry->shader_bitmap = shader_bitmap;
+ __entry->tiler_bitmap = tiler_bitmap;
+ __entry->l2_bitmap = l2_bitmap;
+ ),
+ TP_printk("%s: shader_bitmap=0x%llx tiler_bitmap=0x%llx l2_bitmap=0x%llx",
+ __get_str(dev_name), __entry->shader_bitmap, __entry->tiler_bitmap,
+ __entry->l2_bitmap
+ ),
+ panthor_hw_power_status_register, panthor_hw_power_status_unregister
+);
+
+#endif /* __PANTHOR_TRACE_H__ */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE panthor_trace
+
+#include <trace/define_trace.h>
--
2.52.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v6 3/3] drm/panthor: Add gpu_job_irq tracepoint
2025-12-23 16:24 [PATCH v6 0/3] Add a few tracepoints to panthor Nicolas Frattaroli
2025-12-23 16:24 ` [PATCH v6 1/3] drm/panthor: Extend IRQ helpers for mask modification/restoration Nicolas Frattaroli
2025-12-23 16:24 ` [PATCH v6 2/3] drm/panthor: Add tracepoint for hardware utilisation changes Nicolas Frattaroli
@ 2025-12-23 16:25 ` Nicolas Frattaroli
2 siblings, 0 replies; 7+ messages in thread
From: Nicolas Frattaroli @ 2025-12-23 16:25 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Chia-I Wu, Karunika Choo
Cc: kernel, linux-kernel, dri-devel, Nicolas Frattaroli
Mali's CSF firmware triggers the job IRQ whenever there's new firmware
events for processing. While this can be a global event (BIT(31) of the
status register), it's usually an event relating to a command stream
group (the other bit indices).
Panthor throws these events onto a workqueue for processing outside the
IRQ handler. It's therefore useful to have an instrumented tracepoint
that goes beyond the generic IRQ tracepoint for this specific case, as
it can be augmented with additional data, namely the events bit mask.
This can then be used to debug problems relating to GPU jobs events not
being processed quickly enough. The duration_ns field can be used to
work backwards from when the tracepoint fires (at the end of the IRQ
handler) to figure out when the interrupt itself landed, providing not
just information on how long the work queueing took, but also when the
actual interrupt itself arrived.
With this information in hand, the IRQ handler itself being slow can be
excluded as a possible source of problems, and attention can be directed
to the workqueue processing instead.
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
drivers/gpu/drm/panthor/panthor_fw.c | 13 +++++++++++++
drivers/gpu/drm/panthor/panthor_trace.h | 28 ++++++++++++++++++++++++++++
2 files changed, 41 insertions(+)
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index a64ec8756bed..513ed42f7b41 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -26,6 +26,7 @@
#include "panthor_mmu.h"
#include "panthor_regs.h"
#include "panthor_sched.h"
+#include "panthor_trace.h"
#define CSF_FW_NAME "mali_csffw.bin"
@@ -1060,6 +1061,12 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
static void panthor_job_irq_handler(struct panthor_device *ptdev, u32 status)
{
+ u32 duration;
+ u64 start;
+
+ if (tracepoint_enabled(gpu_job_irq))
+ start = ktime_get_ns();
+
gpu_write(ptdev, JOB_INT_CLEAR, status);
if (!ptdev->fw->booted && (status & JOB_INT_GLOBAL_IF))
@@ -1072,6 +1079,12 @@ static void panthor_job_irq_handler(struct panthor_device *ptdev, u32 status)
return;
panthor_sched_report_fw_events(ptdev, status);
+
+ if (tracepoint_enabled(gpu_job_irq)) {
+ if (check_sub_overflow(ktime_get_ns(), start, &duration))
+ duration = U32_MAX;
+ trace_gpu_job_irq(ptdev->base.dev, status, duration);
+ }
}
PANTHOR_IRQ_HANDLER(job, JOB, panthor_job_irq_handler);
diff --git a/drivers/gpu/drm/panthor/panthor_trace.h b/drivers/gpu/drm/panthor/panthor_trace.h
index 5bd420894745..6ffeb4fe6599 100644
--- a/drivers/gpu/drm/panthor/panthor_trace.h
+++ b/drivers/gpu/drm/panthor/panthor_trace.h
@@ -48,6 +48,34 @@ TRACE_EVENT_FN(gpu_power_status,
panthor_hw_power_status_register, panthor_hw_power_status_unregister
);
+/**
+ * gpu_job_irq - called after a job interrupt from firmware completes
+ * @dev: pointer to the &struct device, for printing the device name
+ * @events: bitmask of BIT(CSG id) | BIT(31) for a global event
+ * @duration_ns: Nanoseconds between job IRQ handler entry and exit
+ *
+ * The panthor_job_irq_handler() function instrumented by this tracepoint exits
+ * once it has queued the firmware interrupts for processing, not when the
+ * firmware interrupts are fully processed. This tracepoint allows for debugging
+ * issues with delays in the workqueue's processing of events.
+ */
+TRACE_EVENT(gpu_job_irq,
+ TP_PROTO(const struct device *dev, u32 events, u32 duration_ns),
+ TP_ARGS(dev, events, duration_ns),
+ TP_STRUCT__entry(
+ __string(dev_name, dev_name(dev))
+ __field(u32, events)
+ __field(u32, duration_ns)
+ ),
+ TP_fast_assign(
+ __assign_str(dev_name);
+ __entry->events = events;
+ __entry->duration_ns = duration_ns;
+ ),
+ TP_printk("%s: events=0x%x duration_ns=%d", __get_str(dev_name),
+ __entry->events, __entry->duration_ns)
+);
+
#endif /* __PANTHOR_TRACE_H__ */
#undef TRACE_INCLUDE_PATH
--
2.52.0
^ permalink raw reply related [flat|nested] 7+ messages in thread