[PATCH 0/3] Rework panthor's cache flush and soft reset locking

[PATCH 0/3] Rework panthor's cache flush and soft reset locking

[Nicolas Frattaroli]

There are two problems in panthor's cache flushing/soft reset code
related to locking.

The first problem is that the spinlocks are taken with _irqsave, even
though the contended lock is never acquired in a raw interrupt handler.
Only a threaded handler locks it.

The second problem is that the wait_event condition references
pending_reqs without holding the required lock. This means there's a
data race.

While resolving the first problem is easy, the second problem requires
adding some more macro variants to wait.h, which is done in the first
patch.

New tracepoints to debug cache flushing duration without initial locking
waits is thrown in for good measure as well, to complement what's in
lock_stat and what the function tracer can already do.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Nicolas Frattaroli (3):
      wait: Introduce non-irq variants of wait_event_lock_timeout
      drm/panthor: Revisit reqs_lock handling in flush/reset paths
      drm/panthor: Add tracepoints for cache flushing

 drivers/gpu/drm/panthor/panthor_gpu.c   | 28 +++++------
 drivers/gpu/drm/panthor/panthor_trace.h | 58 +++++++++++++++++++++++
 include/linux/wait.h                    | 84 +++++++++++++++++++++++++++++++++
 3 files changed, 156 insertions(+), 14 deletions(-)
---
base-commit: f57436380d5de1c1ded898e21378b0b4159c8790
change-id: 20260728-panthor-cache-flush-fix-b36cb15f92c3

Best regards,
--  
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH 1/3] wait: Introduce non-irq variants of wait_event_lock_timeout

[Nicolas Frattaroli]

For locks that are never acquired in an interrupt context, the non-irq
variants for spin_lock()/spin_unlock() are more efficient. However, so
far, wait.h only provided wait_event_lock_irq_timeout and
wait_event_interruptible_lock_irq_timeout, both of which use
spin_lock_irq()/spin_unlock_irq().

Rectify this by adding new non-irq variants of these two macros.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 include/linux/wait.h | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index dce055e6add3..cd9b989c120e 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -1174,6 +1174,13 @@ do {										\
 		      __ret = schedule_timeout(__ret);				\
 		      spin_lock_irq(&lock));
 
+#define __wait_event_lock_timeout(wq_head, condition, lock, timeout, state)	\
+	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
+		      state, 0, timeout,					\
+		      spin_unlock(&lock);					\
+		      __ret = schedule_timeout(__ret);				\
+		      spin_lock(&lock))
+
 /**
  * wait_event_interruptible_lock_irq_timeout - sleep until a condition gets
  *		true or a timeout elapses. The condition is checked under
@@ -1219,6 +1226,83 @@ do {										\
 	__ret;									\
 })
 
+/**
+ * wait_event_interruptible_lock_timeout - sleep until a condition gets
+ *		true or a timeout elapses. The condition is checked under
+ *		the lock with IRQs enabled. This is expected to be called
+ *		with the lock taken.
+ * @wq_head: the waitqueue to wait on
+ * @condition: a C expression for the event to wait for
+ * @lock: a spinlock_t that has been locked with spin_lock(), which will be
+ *	  released before schedule() and reacquired afterwards.
+ * @timeout: timeout, in jiffies
+ *
+ * The process is put to sleep (TASK_INTERRUPTIBLE) until the
+ * @condition evaluates to true or signal is received. The @condition is
+ * checked each time the waitqueue @wq_head is woken up.
+ *
+ * wake_up() has to be called after changing any variable that could
+ * change the result of the wait condition.
+ *
+ * This is supposed to be called while holding the lock. The lock is
+ * dropped before going to sleep using spin_unlock() and is reacquired
+ * using spin_lock() afterwards, but without globally disabling IRQs.
+ * This is in contrast to wait_event_interruptible_lock_irq_timeout(),
+ * which uses the spin_lock_irq() and spin_unlock_irq() functions instead.
+ *
+ * The function returns 0 if the @timeout elapsed, -ERESTARTSYS if it
+ * was interrupted by a signal, and the remaining jiffies otherwise
+ * if the condition evaluated to true before the timeout elapsed.
+ */
+#define wait_event_interruptible_lock_timeout(wq_head, condition, lock,		\
+					      timeout)				\
+({										\
+	long __ret = timeout;							\
+	if (!___wait_cond_timeout(condition))					\
+		__ret = __wait_event_lock_timeout(				\
+					wq_head, condition, lock, timeout,	\
+					TASK_INTERRUPTIBLE);			\
+	__ret;									\
+})
+
+/**
+ * wait_event_lock_timeout - sleep until a condition gets true or a timeout
+ *		elapses. The condition is checked under the lock with IRQs
+ *		enabled. This is expected to be called with the lock taken.
+ * @wq_head: the waitqueue to wait on
+ * @condition: a C expression for the event to wait for
+ * @lock: a spinlock_t that has been locked with spin_lock(), which will be
+ *	  released before schedule() and reacquired afterwards.
+ * @timeout: timeout, in jiffies
+ *
+ * The process is put to sleep (TASK_UNINTERRUPTIBLE) until the
+ * @condition evaluates to true or signal is received. The @condition is
+ * checked each time the waitqueue @wq_head is woken up.
+ *
+ * wake_up() has to be called after changing any variable that could
+ * change the result of the wait condition.
+ *
+ * This is supposed to be called while holding the lock. The lock is
+ * dropped before going to sleep using spin_unlock() and is reacquired
+ * using spin_lock() afterwards, but without globally disabling IRQs.
+ * This is in contrast to wait_event_lock_irq_timeout(), which uses the
+ * spin_lock_irq() and spin_unlock_irq() functions instead. Compared to
+ * wait_event_interruptible_lock_timeout(), it uses the %TASK_UNINTERRUPTIBLE
+ * instead of allowing the task to be interrupted.
+ *
+ * The function returns 0 if the @timeout elapsed, or the remaining jiffies
+ * otherwise if the condition evaluated to true before the timeout elapsed.
+ */
+#define wait_event_lock_timeout(wq_head, condition, lock, timeout)		\
+({										\
+	long __ret = timeout;							\
+	if (!___wait_cond_timeout(condition))					\
+		__ret = __wait_event_lock_timeout(				\
+					wq_head, condition, lock, timeout,	\
+					TASK_UNINTERRUPTIBLE);			\
+	__ret;									\
+})
+
 /*
  * Waitqueues which are removed from the waitqueue_head at wakeup time
  */

-- 
2.55.0

[PATCH 2/3] drm/panthor: Revisit reqs_lock handling in flush/reset paths

[Nicolas Frattaroli]

panthor_gpu_flush_caches() and panthor_gpu_soft_reset() would read (and
even reset) the contents of the pending_reqs register outside of holding
the reqs_lock. Additionally, when it did hold the lock, it did so with
the irqsave/irqrestore variants, even though the spinlock was never
acquired in an atomic context, just the threaded handler.

Use the new wait_event_lock_timeout() macro to check pending_reqs under
the lock, and only do so without disabling interrupts.

Fixes: 5cd894e258c4 ("drm/panthor: Add the GPU logical block")
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_gpu.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index c013d6bf9a59..f015bde80abf 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -330,35 +330,34 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 			     u32 l2, u32 lsc, u32 other)
 {
 	struct panthor_gpu *gpu = ptdev->gpu;
-	unsigned long flags;
 	int ret = 0;
 
 	/* Serialize cache flush operations. */
 	guard(mutex)(&ptdev->gpu->cache_flush_lock);
 
-	spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
+	spin_lock(&ptdev->gpu->reqs_lock);
 	if (!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED)) {
 		ptdev->gpu->pending_reqs |= GPU_IRQ_CLEAN_CACHES_COMPLETED;
 		gpu_write(gpu->iomem, GPU_CMD, GPU_FLUSH_CACHES(l2, lsc, other));
 	} else {
 		ret = -EIO;
 	}
-	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 
-	if (ret)
+	if (ret) {
+		spin_unlock(&ptdev->gpu->reqs_lock);
 		return ret;
+	}
 
-	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
+	if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
 				!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED),
-				msecs_to_jiffies(100))) {
-		spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
+				ptdev->gpu->reqs_lock, msecs_to_jiffies(100))) {
 		if ((ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED) != 0 &&
 		    !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & GPU_IRQ_CLEAN_CACHES_COMPLETED))
 			ret = -ETIMEDOUT;
 		else
 			ptdev->gpu->pending_reqs &= ~GPU_IRQ_CLEAN_CACHES_COMPLETED;
-		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 	}
+	spin_unlock(&ptdev->gpu->reqs_lock);
 
 	if (ret) {
 		panthor_device_schedule_reset(ptdev);
@@ -378,27 +377,25 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev)
 {
 	struct panthor_gpu *gpu = ptdev->gpu;
 	bool timedout = false;
-	unsigned long flags;
 
-	spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
+	guard(spinlock)(&ptdev->gpu->reqs_lock);
+
 	if (!drm_WARN_ON(&ptdev->base,
 			 ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED)) {
 		ptdev->gpu->pending_reqs |= GPU_IRQ_RESET_COMPLETED;
 		gpu_write(gpu->irq.iomem, INT_CLEAR, GPU_IRQ_RESET_COMPLETED);
 		gpu_write(gpu->iomem, GPU_CMD, GPU_SOFT_RESET);
 	}
-	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 
-	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
+	if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked,
 				!(ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED),
+				ptdev->gpu->reqs_lock,
 				msecs_to_jiffies(100))) {
-		spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
 		if ((ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED) != 0 &&
 		    !(gpu_read(gpu->irq.iomem, INT_RAWSTAT) & GPU_IRQ_RESET_COMPLETED))
 			timedout = true;
 		else
 			ptdev->gpu->pending_reqs &= ~GPU_IRQ_RESET_COMPLETED;
-		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 	}
 
 	if (timedout) {

-- 
2.55.0

[PATCH 3/3] drm/panthor: Add tracepoints for cache flushing

[Nicolas Frattaroli]

Add two new event tracepoints: gpu_cache_flush_start to be emitted after
acquiring the flush mutex and reqs spinlock, and gpu_cache_flush_end to
be emitted when leaving the function.

This allows debugging the duration a flush takes irrespective of initial
function entry lock contention by subtracting the start tracepoint's
timestamp from the end tracepoint timestamp, and additionally contains
information such as which caches were flushed.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_gpu.c   |  3 ++
 drivers/gpu/drm/panthor/panthor_trace.h | 58 +++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index f015bde80abf..a25955ad668b 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -336,6 +336,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 	guard(mutex)(&ptdev->gpu->cache_flush_lock);
 
 	spin_lock(&ptdev->gpu->reqs_lock);
+	trace_gpu_cache_flush_start(ptdev->base.dev, l2, lsc, other);
 	if (!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED)) {
 		ptdev->gpu->pending_reqs |= GPU_IRQ_CLEAN_CACHES_COMPLETED;
 		gpu_write(gpu->iomem, GPU_CMD, GPU_FLUSH_CACHES(l2, lsc, other));
@@ -345,6 +346,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 
 	if (ret) {
 		spin_unlock(&ptdev->gpu->reqs_lock);
+		trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
 		return ret;
 	}
 
@@ -358,6 +360,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 			ptdev->gpu->pending_reqs &= ~GPU_IRQ_CLEAN_CACHES_COMPLETED;
 	}
 	spin_unlock(&ptdev->gpu->reqs_lock);
+	trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
 
 	if (ret) {
 		panthor_device_schedule_reset(ptdev);
diff --git a/drivers/gpu/drm/panthor/panthor_trace.h b/drivers/gpu/drm/panthor/panthor_trace.h
index 6ffeb4fe6599..5e2a9b8d0481 100644
--- a/drivers/gpu/drm/panthor/panthor_trace.h
+++ b/drivers/gpu/drm/panthor/panthor_trace.h
@@ -76,6 +76,64 @@ TRACE_EVENT(gpu_job_irq,
 		  __entry->events, __entry->duration_ns)
 );
 
+/**
+ * gpu_cache_flush_start - called after cache flush locks taken, before flush
+ * @dev: pointer to the &struct device, for printing the device name
+ * @l2: "l2" flush flags
+ * @lsc: "lsc" flush flags
+ * @other: "other" flush flags
+ *
+ * Fires after any initial lock contention around the locks needed for flushing
+ * caches, but before the actual cache flush is requested.
+ */
+TRACE_EVENT(gpu_cache_flush_start,
+	TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
+	TP_ARGS(dev, l2, lsc, other),
+	TP_STRUCT__entry(
+		__string(dev_name, dev_name(dev))
+		__field(u32, l2)
+		__field(u32, lsc)
+		__field(u32, other)
+	),
+	TP_fast_assign(
+		__assign_str(dev_name);
+		__entry->l2	= l2;
+		__entry->lsc	= lsc;
+		__entry->other	= other;
+	),
+	TP_printk("%s: l2=0x%x lsc=0x%x other=0x%x", __get_str(dev_name),
+		  __entry->l2, __entry->lsc, __entry->other)
+);
+
+/**
+ * gpu_cache_flush_end - called after cache flush
+ * @dev: pointer to the &struct device, for printing the device name
+ * @l2: "l2" flush flags
+ * @lsc: "lsc" flush flags
+ * @other: "other" flush flags
+ *
+ * Fires after either the cache flush is complete, or has failed. Can be used
+ * together with gpu_cache_flush_start to get how long the flush has taken.
+ */
+TRACE_EVENT(gpu_cache_flush_end,
+	TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
+	TP_ARGS(dev, l2, lsc, other),
+	TP_STRUCT__entry(
+		__string(dev_name, dev_name(dev))
+		__field(u32, l2)
+		__field(u32, lsc)
+		__field(u32, other)
+	),
+	TP_fast_assign(
+		__assign_str(dev_name);
+		__entry->l2	= l2;
+		__entry->lsc	= lsc;
+		__entry->other	= other;
+	),
+	TP_printk("%s: l2=0x%x lsc=0x%x other=0x%x", __get_str(dev_name),
+		  __entry->l2, __entry->lsc, __entry->other)
+);
+
 #endif /* __PANTHOR_TRACE_H__ */
 
 #undef TRACE_INCLUDE_PATH

-- 
2.55.0

Re: [PATCH 1/3] wait: Introduce non-irq variants of wait_event_lock_timeout

[Peter Zijlstra]

On Wed, Jul 29, 2026 at 08:19:49AM +0200, Nicolas Frattaroli wrote:
> For locks that are never acquired in an interrupt context, the non-irq
> variants for spin_lock()/spin_unlock() are more efficient. However, so
> far, wait.h only provided wait_event_lock_irq_timeout and
> wait_event_interruptible_lock_irq_timeout, both of which use
> spin_lock_irq()/spin_unlock_irq().
> 
> Rectify this by adding new non-irq variants of these two macros.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH 3/3] drm/panthor: Add tracepoints for cache flushing

[Steven Rostedt]

On Wed, 29 Jul 2026 08:19:51 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> diff --git a/drivers/gpu/drm/panthor/panthor_trace.h b/drivers/gpu/drm/panthor/panthor_trace.h
> index 6ffeb4fe6599..5e2a9b8d0481 100644
> --- a/drivers/gpu/drm/panthor/panthor_trace.h
> +++ b/drivers/gpu/drm/panthor/panthor_trace.h
> @@ -76,6 +76,64 @@ TRACE_EVENT(gpu_job_irq,
>  		  __entry->events, __entry->duration_ns)
>  );
>  
> +/**
> + * gpu_cache_flush_start - called after cache flush locks taken, before flush
> + * @dev: pointer to the &struct device, for printing the device name
> + * @l2: "l2" flush flags
> + * @lsc: "lsc" flush flags
> + * @other: "other" flush flags
> + *
> + * Fires after any initial lock contention around the locks needed for flushing
> + * caches, but before the actual cache flush is requested.
> + */
> +TRACE_EVENT(gpu_cache_flush_start,
> +	TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
> +	TP_ARGS(dev, l2, lsc, other),
> +	TP_STRUCT__entry(
> +		__string(dev_name, dev_name(dev))
> +		__field(u32, l2)
> +		__field(u32, lsc)
> +		__field(u32, other)
> +	),
> +	TP_fast_assign(
> +		__assign_str(dev_name);
> +		__entry->l2	= l2;
> +		__entry->lsc	= lsc;
> +		__entry->other	= other;
> +	),
> +	TP_printk("%s: l2=0x%x lsc=0x%x other=0x%x", __get_str(dev_name),
> +		  __entry->l2, __entry->lsc, __entry->other)
> +);
> +
> +/**
> + * gpu_cache_flush_end - called after cache flush
> + * @dev: pointer to the &struct device, for printing the device name
> + * @l2: "l2" flush flags
> + * @lsc: "lsc" flush flags
> + * @other: "other" flush flags
> + *
> + * Fires after either the cache flush is complete, or has failed. Can be used
> + * together with gpu_cache_flush_start to get how long the flush has taken.
> + */
> +TRACE_EVENT(gpu_cache_flush_end,
> +	TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
> +	TP_ARGS(dev, l2, lsc, other),
> +	TP_STRUCT__entry(
> +		__string(dev_name, dev_name(dev))
> +		__field(u32, l2)
> +		__field(u32, lsc)
> +		__field(u32, other)
> +	),
> +	TP_fast_assign(
> +		__assign_str(dev_name);
> +		__entry->l2	= l2;
> +		__entry->lsc	= lsc;
> +		__entry->other	= other;
> +	),
> +	TP_printk("%s: l2=0x%x lsc=0x%x other=0x%x", __get_str(dev_name),
> +		  __entry->l2, __entry->lsc, __entry->other)
> +);
> +
>  #endif /* __PANTHOR_TRACE_H__ */

The above two are identical. You'll save memory by using a template instead:

DECLARE_EVENT_CLASS(gpu_cache_flush_template,
	TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
	TP_ARGS(dev, l2, lsc, other),
	TP_STRUCT__entry(
		__string(dev_name, dev_name(dev))
		__field(u32, l2)
		__field(u32, lsc)
		__field(u32, other)
	),
	TP_fast_assign(
		__assign_str(dev_name);
		__entry->l2	= l2;
		__entry->lsc	= lsc;
		__entry->other	= other;
	),
	TP_printk("%s: l2=0x%x lsc=0x%x other=0x%x", __get_str(dev_name),
		  __entry->l2, __entry->lsc, __entry->other)
);

DEFINE_EVENT(gpu_cache_flush_template, gpu_cache_flush_start,
	TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
	TP_ARGS(dev, l2, lsc, other)
);

DEFINE_EVENT(gpu_cache_flush_template, gpu_cache_flush_end,
	TP_PROTO(const struct device *dev, u32 l2, u32 lsc, u32 other),
	TP_ARGS(dev, l2, lsc, other)
);

-- Steve