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

[PATCH v3 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 v3:
- Drop new wait_event macro patch as the existing ones have a memory
  barrier that makes the current use valid
- Rewrite fix patch to use scoped guards, and just get rid of the IRQ
  disabling
- Add separate fix for the outside-of-lock pending_reqs clearing in soft
  reset path
- Move tracepoint patch to before fixes for easier before/after testing
- Link to v2: https://patch.msgid.link/20260730-panthor-cache-flush-fix-v2-0-28790478bfff@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):
      drm/panthor: Add tracepoints for cache flushing
      drm/panthor: Revisit reqs_lock handling in flush/reset paths
      drm/panthor: Take reqs_lock in soft_reset for clearing pending_reqs

 drivers/gpu/drm/panthor/panthor_gpu.c   | 70 +++++++++++++++++----------------
 drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++++++
 2 files changed, 85 insertions(+), 34 deletions(-)
---
base-commit: dc462ab791b686c48545c160ecc81be64f77a846
change-id: 20260728-panthor-cache-flush-fix-b36cb15f92c3

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

[PATCH v3 1/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.

Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_gpu.c   |  7 ++++-
 drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index c013d6bf9a59..68e2dd2527df 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -337,6 +337,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 	guard(mutex)(&ptdev->gpu->cache_flush_lock);
 
 	spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
+	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,8 +346,10 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 	}
 	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 
-	if (ret)
+	if (ret) {
+		trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
 		return ret;
+	}
 
 	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
 				!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED),
@@ -360,6 +363,8 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
 	}
 
+	trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
+
 	if (ret) {
 		panthor_device_schedule_reset(ptdev);
 		drm_err(&ptdev->base, "Flush caches timeout");
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 v3 1/3] drm/panthor: Add tracepoints for cache flushing

[Boris Brezillon]

On Tue, 11 Aug 2026 16:08:31 +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.
> 
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/panthor/panthor_gpu.c   |  7 ++++-
>  drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index c013d6bf9a59..68e2dd2527df 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -337,6 +337,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
>  	guard(mutex)(&ptdev->gpu->cache_flush_lock);
>  
>  	spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> +	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,8 +346,10 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
>  	}
>  	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
>  
> -	if (ret)
> +	if (ret) {
> +		trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);

I don't mind having start/end traces, but I still think it'd be
valuable to report failure cases.

>  		return ret;
> +	}
>  
>  	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
>  				!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED),
> @@ -360,6 +363,8 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
>  		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
>  	}
>  
> +	trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
> +
>  	if (ret) {
>  		panthor_device_schedule_reset(ptdev);
>  		drm_err(&ptdev->base, "Flush caches timeout");
> 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 v3 1/3] drm/panthor: Add tracepoints for cache flushing

[Nicolas Frattaroli]

On Tuesday, 11 August 2026 16:29:34 Central European Summer Time Boris Brezillon wrote:
> On Tue, 11 Aug 2026 16:08:31 +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.
> > 
> > Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > Reviewed-by: Steven Price <steven.price@arm.com>
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_gpu.c   |  7 ++++-
> >  drivers/gpu/drm/panthor/panthor_trace.h | 49 +++++++++++++++++++++++++++++++++
> >  2 files changed, 55 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> > index c013d6bf9a59..68e2dd2527df 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> > @@ -337,6 +337,7 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
> >  	guard(mutex)(&ptdev->gpu->cache_flush_lock);
> >  
> >  	spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> > +	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,8 +346,10 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
> >  	}
> >  	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
> >  
> > -	if (ret)
> > +	if (ret) {
> > +		trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
> 
> I don't mind having start/end traces, but I still think it'd be
> valuable to report failure cases.

Alright, I think in that case I will get rid of the start/end ones
(since they now need to have different args) and just do one on exit
with a duration arg and an ret arg.

> 
> >  		return ret;
> > +	}
> >  
> >  	if (!wait_event_timeout(ptdev->gpu->reqs_acked,
> >  				!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED),
> > @@ -360,6 +363,8 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev,
> >  		spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
> >  	}
> >  
> > +	trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
> > +
> >  	if (ret) {
> >  		panthor_device_schedule_reset(ptdev);
> >  		drm_err(&ptdev->base, "Flush caches timeout");
> > 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
> > 
> 
> 

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

[Nicolas Frattaroli]

panthor_gpu_flush_caches() and panthor_gpu_soft_reset() acquire their
reqs_lock spinlock with the IRQ-disabling variants of the spinlocking
functions. This isn't necessary, as the lock is never taken from an
atomic context, as Panthor uses threaded interrupt handlers. The result
of this overly strict locking is that IRQs may be disabled more
frequently and for longer than they should be, resulting in increased
system latency.

Switch the locking to use non-IRQ-disabling scoped_guard statements for
locking. The wait_event_timeout read of pending_reqs outside of the
spinlock is fine as wait_event_timeout is a memory barrier according to
the Linux Memory Model.

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 | 66 ++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index 68e2dd2527df..cb5319d1c5de 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -330,37 +330,32 @@ 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);
-	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));
-	} else {
-		ret = -EIO;
-	}
-	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
-
-	if (ret) {
-		trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
-		return ret;
+	scoped_guard(spinlock, &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));
+		} else {
+			trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
+			return -EIO;
+		}
 	}
 
 	if (!wait_event_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);
-		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);
+		scoped_guard(spinlock, &ptdev->gpu->reqs_lock) {
+			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;
+		}
 	}
 
 	trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
@@ -383,27 +378,26 @@ 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);
-	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);
+	scoped_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,
 				!(ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED),
 				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);
+		scoped_guard(spinlock, &ptdev->gpu->reqs_lock) {
+			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;
+		}
 	}
 
 	if (timedout) {

-- 
2.55.0

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

[Boris Brezillon]

On Tue, 11 Aug 2026 16:08:32 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> panthor_gpu_flush_caches() and panthor_gpu_soft_reset() acquire their
> reqs_lock spinlock with the IRQ-disabling variants of the spinlocking
> functions. This isn't necessary, as the lock is never taken from an
> atomic context, as Panthor uses threaded interrupt handlers. The result
> of this overly strict locking is that IRQs may be disabled more
> frequently and for longer than they should be, resulting in increased
> system latency.
> 
> Switch the locking to use non-IRQ-disabling scoped_guard statements for
> locking. The wait_event_timeout read of pending_reqs outside of the
> spinlock is fine as wait_event_timeout is a memory barrier according to
> the Linux Memory Model.
> 
> Fixes: 5cd894e258c4 ("drm/panthor: Add the GPU logical block")
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panthor/panthor_gpu.c | 66 ++++++++++++++++-------------------
>  1 file changed, 30 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index 68e2dd2527df..cb5319d1c5de 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -330,37 +330,32 @@ 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);
> -	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));
> -	} else {
> -		ret = -EIO;
> -	}
> -	spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags);
> -
> -	if (ret) {
> -		trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
> -		return ret;
> +	scoped_guard(spinlock, &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));
> +		} else {
> +			trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
> +			return -EIO;
> +		}
>  	}
>  
>  	if (!wait_event_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);
> -		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);
> +		scoped_guard(spinlock, &ptdev->gpu->reqs_lock) {
> +			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;
> +		}
>  	}
>  
>  	trace_gpu_cache_flush_end(ptdev->base.dev, l2, lsc, other);
> @@ -383,27 +378,26 @@ 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);
> -	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);
> +	scoped_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,
>  				!(ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED),
>  				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);
> +		scoped_guard(spinlock, &ptdev->gpu->reqs_lock) {
> +			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;
> +		}
>  	}
>  
>  	if (timedout) {
> 

[PATCH v3 3/3] drm/panthor: Take reqs_lock in soft_reset for clearing pending_reqs

[Nicolas Frattaroli]

panthor_gpu_soft_reset() sets pending_reqs to 0 without taking the
requisite reqs_lock.

Fix this by taking the lock for the duration of the modification.

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index cb5319d1c5de..12e7e29b35b5 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -405,7 +405,10 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev)
 		return -ETIMEDOUT;
 	}
 
-	ptdev->gpu->pending_reqs = 0;
+	scoped_guard(spinlock, &ptdev->gpu->reqs_lock) {
+		ptdev->gpu->pending_reqs = 0;
+	}
+
 	return 0;
 }
 

-- 
2.55.0

Re: [PATCH v3 3/3] drm/panthor: Take reqs_lock in soft_reset for clearing pending_reqs

[Boris Brezillon]

On Tue, 11 Aug 2026 16:08:33 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> panthor_gpu_soft_reset() sets pending_reqs to 0 without taking the
> requisite reqs_lock.
> 
> Fix this by taking the lock for the duration of the modification.
> 
> 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index cb5319d1c5de..12e7e29b35b5 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -405,7 +405,10 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev)
>  		return -ETIMEDOUT;
>  	}
>  
> -	ptdev->gpu->pending_reqs = 0;
> +	scoped_guard(spinlock, &ptdev->gpu->reqs_lock) {
> +		ptdev->gpu->pending_reqs = 0;
> +	}

In practice, this happens when nothing else interacts with the GPU
(IRQs have been suspended, and any access to the HW should be filtered
out/delayed until the reset is effective), but I agree it's safer and
more consistent to have this assigned done under the reqs_lock.

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>