On 12/08/2026 15:07, Nicolas Frattaroli 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")
> Reviewed-by: Boris Brezillon <[email protected]>
> Signed-off-by: Nicolas Frattaroli <[email protected]>
Reviewed-by: Steven Price <[email protected]>
Although one minor formatting nit below.
> ---
> drivers/gpu/drm/panthor/panthor_gpu.c | 72
> ++++++++++++++++-------------------
> 1 file changed, 33 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c
> b/drivers/gpu/drm/panthor/panthor_gpu.c
> index 7088371c6d64..55e33f145b40 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -345,41 +345,36 @@ int panthor_gpu_flush_caches(struct panthor_device
> *ptdev,
> u32 l2, u32 lsc, u32 other)
> {
> struct panthor_gpu *gpu = ptdev->gpu;
> - unsigned long flags;
> u64 start = 0;
> int ret = 0;
>
> /* Serialize cache flush operations. */
> guard(mutex)(&ptdev->gpu->cache_flush_lock);
>
> - spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> -
> - if (tracepoint_enabled(gpu_cache_flush))
> - start = ktime_get_ns();
> -
> - 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) {
> - panthor_gpu_emit_flush_caches_tp(ptdev, start, l2, lsc, other,
> ret);
> - return ret;
> + scoped_guard(spinlock, &ptdev->gpu->reqs_lock) {
> + if (tracepoint_enabled(gpu_cache_flush))
> + start = ktime_get_ns();
> +
> + 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 {
> + panthor_gpu_emit_flush_caches_tp(ptdev, start, l2, lsc,
> + other, -EIO);
> + 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))
NIT: This isn't aligned correctly with the if() above any more.
To be honest what we really need here is a helper for this sequence as
there's basically the same code again in panthor_gpu_soft_reset() below.
> + ret = -ETIMEDOUT;
> + else
> + ptdev->gpu->pending_reqs &=
> ~GPU_IRQ_CLEAN_CACHES_COMPLETED;
> + }
> }
>
> panthor_gpu_emit_flush_caches_tp(ptdev, start, l2, lsc, other, ret);
> @@ -402,27 +397,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))
NIT: Same issue here.
Thanks,
Steve
> + timedout = true;
> + else
> + ptdev->gpu->pending_reqs &=
> ~GPU_IRQ_RESET_COMPLETED;
> + }
> }
>
> if (timedout) {
>
