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

[PATCH v2 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>
---
Changes in v2:
- Use trace event template for the two tracepoints (ty Steven Rostedt)
- Link to v1: https://patch.msgid.link/20260729-panthor-cache-flush-fix-v1-0-205921ed3c81@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 | 49 +++++++++++++++++++
 include/linux/wait.h                    | 84 +++++++++++++++++++++++++++++++++
 3 files changed, 147 insertions(+), 14 deletions(-)
---
base-commit: 5e24f68d311764bf343f9def49b752b509dfd5fd
change-id: 20260728-panthor-cache-flush-fix-b36cb15f92c3

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

[PATCH v2 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 v2 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 v2 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 | 49 +++++++++++++++++++++++++++++++++
 2 files changed, 52 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..6951b95b1de7 100644
--- a/drivers/gpu/drm/panthor/panthor_trace.h
+++ b/drivers/gpu/drm/panthor/panthor_trace.h
@@ -76,6 +76,55 @@ TRACE_EVENT(gpu_job_irq,
 		  __entry->events, __entry->duration_ns)
 );
 
+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)
+);
+
+/**
+ * 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.
+ */
+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)
+);
+
+/**
+ * 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.
+ */
+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)
+);
+
 #endif /* __PANTHOR_TRACE_H__ */
 
 #undef TRACE_INCLUDE_PATH

-- 
2.55.0

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

[Steven Rostedt]

On Thu, 30 Jul 2026 13:45:16 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> 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 | 49 +++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)

From the tracing point of view:

Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

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

[Liviu Dudau]

On Thu, Jul 30, 2026 at 01:45:15PM +0200, Nicolas Frattaroli wrote:
> 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>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Liviu Dudau]

On Thu, Jul 30, 2026 at 01:45:14PM +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>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Liviu Dudau]

On Thu, Jul 30, 2026 at 01:45:16PM +0200, Nicolas Frattaroli wrote:
> 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>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_gpu.c   |  3 ++
>  drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++++++++++++++++
>  2 files changed, 52 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..6951b95b1de7 100644
> --- a/drivers/gpu/drm/panthor/panthor_trace.h
> +++ b/drivers/gpu/drm/panthor/panthor_trace.h
> @@ -76,6 +76,55 @@ TRACE_EVENT(gpu_job_irq,
>  		  __entry->events, __entry->duration_ns)
>  );
>  
> +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)
> +);
> +
> +/**
> + * 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.
> + */
> +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)
> +);
> +
> +/**
> + * 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.
> + */
> +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)
> +);
> +
>  #endif /* __PANTHOR_TRACE_H__ */
>  
>  #undef TRACE_INCLUDE_PATH
> 
> -- 
> 2.55.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Steven Price]

On 30/07/2026 12:45, 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>

There's a documentation bug below...

> ---
>  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
                                   ^^^^^^^^^^^^^^^^^^^^^
UNINTERRUPTIBLE means that a signal doesn't wake it - that text should
be removed.

With that fixed:

Reviewed-by: Steven Price <steven.price@arm.com>

Thanks,
Steve

> + * 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
>   */
> 

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

[Steven Price]

On 30/07/2026 12:45, Nicolas Frattaroli wrote:
> 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>

Reviewed-by: Steven Price <steven.price@arm.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) {
> 

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

[Steven Price]

On 30/07/2026 12:45, Nicolas Frattaroli wrote:
> 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>

Reviewed-by: Steven Price <steven.price@arm.com>

Thanks,
Steve

> ---
>  drivers/gpu/drm/panthor/panthor_gpu.c   |  3 ++
>  drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++++++++++++++++
>  2 files changed, 52 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..6951b95b1de7 100644
> --- a/drivers/gpu/drm/panthor/panthor_trace.h
> +++ b/drivers/gpu/drm/panthor/panthor_trace.h
> @@ -76,6 +76,55 @@ TRACE_EVENT(gpu_job_irq,
>  		  __entry->events, __entry->duration_ns)
>  );
>  
> +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)
> +);
> +
> +/**
> + * 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.
> + */
> +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)
> +);
> +
> +/**
> + * 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.
> + */
> +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)
> +);
> +
>  #endif /* __PANTHOR_TRACE_H__ */
>  
>  #undef TRACE_INCLUDE_PATH
> 

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

[Boris Brezillon]

Hello Nicolas,

On Thu, 30 Jul 2026 13:45:15 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

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

Can you elaborate a bit on the race being fixed here? If pending_reqs bits
are truly cleared before the wake_up_all() call (which would require a
WRITE_ONCE() to be enforced, admittedly), there's no risk for the
wait_event() call to do a test before the bits have been updated,
and this holds even if the test is done without the lock held.

The other race I could think of is two threads calling
panthor_gpu_flush_caches() concurrently, and the second one stealing
the FLUSH_COMPLETED event the first thread waits on and re-issuing a
second flush on top, thus delaying the completion for the first thread.
But that should be covered by the cache_flush_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);

Can we make the _irq{save,restore}-drop its own patch?

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

Assuming we really need to do the test with the lock held, could we add
a patch at the beginning of the series that fixes the race without depending
on the new wait macro, so that we have a version that can easily be backported?

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

I think a scoped_guard() could make things a bit cleaner, and given you
already turn the regular lock/unlock sequence into a guard in
panthor_gpu_soft_reset(), I'd do that here as well.

Regards,

Boris

[1]https://elixir.bootlin.com/linux/v7.2-rc5/source/drivers/gpu/drm/panthor/panthor_gpu.c#L114

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

[Boris Brezillon]

On Thu, 30 Jul 2026 13:45:16 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> 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 | 49 +++++++++++++++++++++++++++++++++
>  2 files changed, 52 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);

Should we add a status to the end event, so that faulty flushes can
be filtered out (those can be immediate, or timeout=100ms depending on
where the failure happens, but they are not reflecting anything useful,
and would pollute the stats)? Actually, if what we care about is the
time it takes to do a flush, do we even need those start/end events,
can't we pass the time as an argument and do the math in the function
like we do in the job IRQ handler?

>  		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..6951b95b1de7 100644
> --- a/drivers/gpu/drm/panthor/panthor_trace.h
> +++ b/drivers/gpu/drm/panthor/panthor_trace.h
> @@ -76,6 +76,55 @@ TRACE_EVENT(gpu_job_irq,
>  		  __entry->events, __entry->duration_ns)
>  );
>  
> +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)
> +);
> +
> +/**
> + * 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.
> + */
> +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)
> +);
> +
> +/**
> + * 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.
> + */
> +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)
> +);
> +
>  #endif /* __PANTHOR_TRACE_H__ */
>  
>  #undef TRACE_INCLUDE_PATH
> 

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

[Nicolas Frattaroli]

On Monday, 3 August 2026 10:53:24 Central European Summer Time Boris Brezillon wrote:
> Hello Nicolas,
> 
> On Thu, 30 Jul 2026 13:45:15 +0200
> Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> 
> > 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.
> 
> Can you elaborate a bit on the race being fixed here? If pending_reqs bits
> are truly cleared before the wake_up_all() call (which would require a
> WRITE_ONCE() to be enforced, admittedly), there's no risk for the
> wait_event() call to do a test before the bits have been updated,
> and this holds even if the test is done without the lock held.
> 
> The other race I could think of is two threads calling
> panthor_gpu_flush_caches() concurrently, and the second one stealing
> the FLUSH_COMPLETED event the first thread waits on and re-issuing a
> second flush on top, thus delaying the completion for the first thread.
> But that should be covered by the cache_flush_lock.

panthor_gpu_flush_caches() is not the only thing that sets/gets
pending_reqs. Notably, the threaded interrupt handler does, as
well as any other functionality using the same member for reqs
tracking (e.g. the soft reset).

Consider the following serialisation of events:
1. T1 asks to flush caches by writing GPU_CMD and setting pending_reqs
2. T1 drops reqs_lock.
3. T2 enters IRQ handler for flush complete, spins lock waiting for
   reqs_lock
4. T1 sleeps at wait_event_timeout
5. T2 updates pending_reqs and wakes up the waiter in any order, since
   the effects of those two can't consistently be observed as sequential
   logic without the outer reqs_lock being held by the observer
6. T1 wakes up, checks pending_reqs, but since pending_reqs is checked
   without holding any lock, so we implictly depend on the synchronisation
   point that is the waitqueue's lock rather than the reqs_lock spinlock,
   which says nothing about whether the pending_reqs change materialised
   on T1's side yet as far as I can tell?
7. T1 sees that pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED is still != 0,
   so goes back to sleep for some future wake-up of reqs_acked or a timeout.

I'm not 100% sure, but I think 6. means that the memory model would permit
T1 to re-use the pending_reqs it previously set, rather than the updated
one set by T2, since there's nothing stopping us from being woken up before
the pending_reqs change has made itself known to observers not serialising
with the reqs_lock being released by T2 in panthor_gpu_irq_handler after.

If you check lock_stat before the change, you see that the reqs_lock is
actually never contended. This isn't a good sign because it means
whatever situation it exists to protect against never occurs, so either
the lock is pointless or the lock is non-functional.

> > 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);
> 
> Can we make the _irq{save,restore}-drop its own patch?

I'm not sure it's fine to drop the IRQ disabling without fixing the read of
pending_reqs outside its 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),
> 
> Assuming we really need to do the test with the lock held, could we add
> a patch at the beginning of the series that fixes the race without depending
> on the new wait macro, so that we have a version that can easily be backported?

It's either backporting the prerequisite new macro or still doing this all
with IRQs disabled using the pre-existing wait_event_lock_irq_timeout macro,
and the IRQ disabled thing is what caused problems.

> 
> > -				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);
> 
> I think a scoped_guard() could make things a bit cleaner, and given you
> already turn the regular lock/unlock sequence into a guard in
> panthor_gpu_soft_reset(), I'd do that here as well.

That would add an additional layer of indentation, which I'm wary of.
We can't use a non-scoped guard due to the reset at the end of the
function.

I'll see if I can reshuffle the code to make it not as ugly to use a
scoped_guard here. Will also slightly change the semantics of the
_end tracepoint, since it'll then fire it before dropping the lock,
but that's not much of a change.

Kind regards,
Nicolas Frattaroli

> 
> Regards,
> 
> Boris
> 
> [1]https://elixir.bootlin.com/linux/v7.2-rc5/source/drivers/gpu/drm/panthor/panthor_gpu.c#L114
> 

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

[Nicolas Frattaroli]

On Monday, 3 August 2026 15:13:25 Central European Summer Time Nicolas Frattaroli wrote:
> On Monday, 3 August 2026 10:53:24 Central European Summer Time Boris Brezillon wrote:
> > Hello Nicolas,
> > 
> > On Thu, 30 Jul 2026 13:45:15 +0200
> > Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> > 
> > > 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.
> > 
> > Can you elaborate a bit on the race being fixed here? If pending_reqs bits
> > are truly cleared before the wake_up_all() call (which would require a
> > WRITE_ONCE() to be enforced, admittedly), there's no risk for the
> > wait_event() call to do a test before the bits have been updated,
> > and this holds even if the test is done without the lock held.
> > 
> > The other race I could think of is two threads calling
> > panthor_gpu_flush_caches() concurrently, and the second one stealing
> > the FLUSH_COMPLETED event the first thread waits on and re-issuing a
> > second flush on top, thus delaying the completion for the first thread.
> > But that should be covered by the cache_flush_lock.
> 
> panthor_gpu_flush_caches() is not the only thing that sets/gets
> pending_reqs. Notably, the threaded interrupt handler does, as
> well as any other functionality using the same member for reqs
> tracking (e.g. the soft reset).
> 
> Consider the following serialisation of events:
> 1. T1 asks to flush caches by writing GPU_CMD and setting pending_reqs
> 2. T1 drops reqs_lock.
> 3. T2 enters IRQ handler for flush complete, spins lock waiting for
>    reqs_lock

Minor correction: Imagine 2 and 3 reversed here in a non-IRQ-disabling,
variant, otherwise "spins lock" does not make sense. With an IRQ-disabling
variant, ignore the "spins lock waiting for reqs_lock" part. 

> 4. T1 sleeps at wait_event_timeout
> 5. T2 updates pending_reqs and wakes up the waiter in any order, since
>    the effects of those two can't consistently be observed as sequential
>    logic without the outer reqs_lock being held by the observer
> 6. T1 wakes up, checks pending_reqs, but since pending_reqs is checked
>    without holding any lock, so we implictly depend on the synchronisation
>    point that is the waitqueue's lock rather than the reqs_lock spinlock,
>    which says nothing about whether the pending_reqs change materialised
>    on T1's side yet as far as I can tell?
> 7. T1 sees that pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED is still != 0,
>    so goes back to sleep for some future wake-up of reqs_acked or a timeout.
> 

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

[Boris Brezillon]

On Mon, 03 Aug 2026 15:13:25 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> On Monday, 3 August 2026 10:53:24 Central European Summer Time Boris Brezillon wrote:
> > Hello Nicolas,
> > 
> > On Thu, 30 Jul 2026 13:45:15 +0200
> > Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> >   
> > > 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.  
> > 
> > Can you elaborate a bit on the race being fixed here? If pending_reqs bits
> > are truly cleared before the wake_up_all() call (which would require a
> > WRITE_ONCE() to be enforced, admittedly), there's no risk for the
> > wait_event() call to do a test before the bits have been updated,
> > and this holds even if the test is done without the lock held.
> > 
> > The other race I could think of is two threads calling
> > panthor_gpu_flush_caches() concurrently, and the second one stealing
> > the FLUSH_COMPLETED event the first thread waits on and re-issuing a
> > second flush on top, thus delaying the completion for the first thread.
> > But that should be covered by the cache_flush_lock.  
> 
> panthor_gpu_flush_caches() is not the only thing that sets/gets
> pending_reqs. Notably, the threaded interrupt handler does, as
> well as any other functionality using the same member for reqs
> tracking (e.g. the soft reset).
> 
> Consider the following serialisation of events:
> 1. T1 asks to flush caches by writing GPU_CMD and setting pending_reqs
> 2. T1 drops reqs_lock.
> 3. T2 enters IRQ handler for flush complete, spins lock waiting for
>    reqs_lock
> 4. T1 sleeps at wait_event_timeout
> 5. T2 updates pending_reqs and wakes up the waiter in any order, since
>    the effects of those two can't consistently be observed as sequential
>    logic without the outer reqs_lock being held by the observer
> 6. T1 wakes up, checks pending_reqs, but since pending_reqs is checked
>    without holding any lock, so we implictly depend on the synchronisation
>    point that is the waitqueue's lock rather than the reqs_lock spinlock,
>    which says nothing about whether the pending_reqs change materialised
>    on T1's side yet as far as I can tell?
> 7. T1 sees that pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED is still != 0,
>    so goes back to sleep for some future wake-up of reqs_acked or a timeout.
> 
> I'm not 100% sure, but I think 6. means that the memory model would permit
> T1 to re-use the pending_reqs it previously set, rather than the updated
> one set by T2, since there's nothing stopping us from being woken up before
> the pending_reqs change has made itself known to observers not serialising
> with the reqs_lock being released by T2 in panthor_gpu_irq_handler after.

If I read the "SLEEP AND WAKE-UP FUNCTIONS" section in [1] correctly, I
think we're covered by the "general memory barrier" here.

> 
> If you check lock_stat before the change, you see that the reqs_lock is
> actually never contended. This isn't a good sign because it means
> whatever situation it exists to protect against never occurs, so either
> the lock is pointless or the lock is non-functional.

Or we're just in a situation where there's almost never two threads updating
the pending_reqs concurrently, which is not the same as saying this can't
happen at all.

> 
> > > 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);  
> > 
> > Can we make the _irq{save,restore}-drop its own patch?  
> 
> I'm not sure it's fine to drop the IRQ disabling without fixing the read of
> pending_reqs outside its lock.

Can be done after fixing the race, if it's really making the race more
likely. Although, as I said above, I'm not yet convinced the race we're
chasing is the one we've been discussing here.

> 
> >   
> > >  	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),  
> > 
> > Assuming we really need to do the test with the lock held, could we add
> > a patch at the beginning of the series that fixes the race without depending
> > on the new wait macro, so that we have a version that can easily be backported?  
> 
> It's either backporting the prerequisite new macro or still doing this all
> with IRQs disabled using the pre-existing wait_event_lock_irq_timeout macro,
> and the IRQ disabled thing is what caused problems.

Can't we have the following helper function?

static bool cache_flush_done(struct panthor_device *ptdev)
{
	guard(spinlock_irqsave)(&ptdev->gpu->reqs_lock);

	return ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED;
}

and then:

	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
			        cache_flush_done(ptdev),
				msecs_to_jiffies(100))) {
....

[1]https://docs.kernel.org/core-api/wrappers/memory-barriers.html#

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

[Nicolas Frattaroli]

On Monday, 3 August 2026 11:03:58 Central European Summer Time Boris Brezillon wrote:
> On Thu, 30 Jul 2026 13:45:16 +0200
> Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> 
> > 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 | 49 +++++++++++++++++++++++++++++++++
> >  2 files changed, 52 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);
> 
> Should we add a status to the end event, so that faulty flushes can
> be filtered out (those can be immediate, or timeout=100ms depending on
> where the failure happens, but they are not reflecting anything useful,
> and would pollute the stats)? Actually, if what we care about is the
> time it takes to do a flush, do we even need those start/end events,
> can't we pass the time as an argument and do the math in the function
> like we do in the job IRQ handler?

The start/end pattern seems more common across the kernel. Since there
is no doubt as to which start correlates with which end due to the
mutex, I chose to go with that.

> 
> >  		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..6951b95b1de7 100644
> > --- a/drivers/gpu/drm/panthor/panthor_trace.h
> > +++ b/drivers/gpu/drm/panthor/panthor_trace.h
> > @@ -76,6 +76,55 @@ TRACE_EVENT(gpu_job_irq,
> >  		  __entry->events, __entry->duration_ns)
> >  );
> >  
> > +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)
> > +);
> > +
> > +/**
> > + * 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.
> > + */
> > +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)
> > +);
> > +
> > +/**
> > + * 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.
> > + */
> > +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)
> > +);
> > +
> >  #endif /* __PANTHOR_TRACE_H__ */
> >  
> >  #undef TRACE_INCLUDE_PATH
> > 
> 
>