[PATCH] drm/v3d: Using spin_lock_irqsave instead of spin_lock in the RT kernel

[PATCH] drm/v3d: Using spin_lock_irqsave instead of spin_lock in the RT kernel

[Xiaolei Wang]

Disabling interrupts before calling 'spin_lock()' is unnecessary;
its implementation is only for satisfying lockdep's requirements.
Since preemption is disabled by default when calling 'spin_lock()'
in the standard kernel, both 'local_irq_save()' and 'preempt_disable()'
become redundant. Therefore, we can replace 'spin_lock()' with
'spin_lock_irqsave()' to ensure compatibility between the standard
kernel and the rt kernel. Avoid the following warning:

Hardware name: Raspberry Pi 5 Model B Rev 1.0 (DT)
Call trace:
dump_backtrace.part.0+0xe0/0x100
show_stack+0x20/0x40
dump_stack_lvl+0x60/0x80 dump_stack+0x18/0x28
__might_resched+0x134/0x168
rt_spin_lock+0x5c/0xe0
v3d_job_update_stats+0x64/0x1d0 [v3d]
v3d_irq+0xf0/0x410 [v3d]
irq_forced_thread_fn+0x44/0xd8
irq_thread+0x1ac/0x2d8
kthread+0x124/0x138
ret_from_fork+0x10/0x20

Fixes: fa6a20c87470 ("drm/v3d: Address race-condition between per-fd GPU stats and fd release")
Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
---
 drivers/gpu/drm/v3d/v3d_sched.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 6dc871fc9a62..5eaacb1d1480 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -203,24 +203,13 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q)
 	u64 now = local_clock();
 	unsigned long flags;
 
-	/* See comment in v3d_job_start_stats() */
-	if (IS_ENABLED(CONFIG_LOCKDEP))
-		local_irq_save(flags);
-	else
-		preempt_disable();
-
 	/* Don't update the local stats if the file context has already closed */
-	spin_lock(&queue->queue_lock);
+	spin_lock_irqsave(&queue->queue_lock, flags);
 	if (job->file_priv)
 		v3d_stats_update(&job->file_priv->stats[q], now);
-	spin_unlock(&queue->queue_lock);
+	spin_unlock_irqrestore(&queue->queue_lock, flags);
 
 	v3d_stats_update(global_stats, now);
-
-	if (IS_ENABLED(CONFIG_LOCKDEP))
-		local_irq_restore(flags);
-	else
-		preempt_enable();
 }
 
 static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
-- 
2.43.0

Re: [PATCH] drm/v3d: Using spin_lock_irqsave instead of spin_lock in the RT kernel

[Maíra Canal]

Hi Xiaolei,


On 04/03/26 00:17, Xiaolei Wang wrote:
> Disabling interrupts before calling 'spin_lock()' is unnecessary;
> its implementation is only for satisfying lockdep's requirements.

Yeah, the idea is really to satisfy lockdep's requirements. See comment
in `v3d_job_start_stats()`.

However, I sent a series recently to address this workaround [1]. I
appreciate reviews and comments about the series.

[1] 
https://lore.kernel.org/dri-devel/20260217-v3d-reset-locking-improv-v1-0-0db848016869@igalia.com/T/

Best regards,
- Maíra

> Since preemption is disabled by default when calling 'spin_lock()'
> in the standard kernel, both 'local_irq_save()' and 'preempt_disable()'
> become redundant. Therefore, we can replace 'spin_lock()' with
> 'spin_lock_irqsave()' to ensure compatibility between the standard
> kernel and the rt kernel. Avoid the following warning:
> 
> Hardware name: Raspberry Pi 5 Model B Rev 1.0 (DT)
> Call trace:
> dump_backtrace.part.0+0xe0/0x100
> show_stack+0x20/0x40
> dump_stack_lvl+0x60/0x80 dump_stack+0x18/0x28
> __might_resched+0x134/0x168
> rt_spin_lock+0x5c/0xe0
> v3d_job_update_stats+0x64/0x1d0 [v3d]
> v3d_irq+0xf0/0x410 [v3d]
> irq_forced_thread_fn+0x44/0xd8
> irq_thread+0x1ac/0x2d8
> kthread+0x124/0x138
> ret_from_fork+0x10/0x20
> 
> Fixes: fa6a20c87470 ("drm/v3d: Address race-condition between per-fd GPU stats and fd release")
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
>   drivers/gpu/drm/v3d/v3d_sched.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index 6dc871fc9a62..5eaacb1d1480 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -203,24 +203,13 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q)
>   	u64 now = local_clock();
>   	unsigned long flags;
>   
> -	/* See comment in v3d_job_start_stats() */
> -	if (IS_ENABLED(CONFIG_LOCKDEP))
> -		local_irq_save(flags);
> -	else
> -		preempt_disable();
> -
>   	/* Don't update the local stats if the file context has already closed */
> -	spin_lock(&queue->queue_lock);
> +	spin_lock_irqsave(&queue->queue_lock, flags);
>   	if (job->file_priv)
>   		v3d_stats_update(&job->file_priv->stats[q], now);
> -	spin_unlock(&queue->queue_lock);
> +	spin_unlock_irqrestore(&queue->queue_lock, flags);
>   
>   	v3d_stats_update(global_stats, now);
> -
> -	if (IS_ENABLED(CONFIG_LOCKDEP))
> -		local_irq_restore(flags);
> -	else
> -		preempt_enable();
>   }
>   
>   static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)

Claude review: drm/v3d: Using spin_lock_irqsave instead of spin_lock in the RT kernel

[Claude Code Review Bot]

Overall Series Review

Subject: drm/v3d: Using spin_lock_irqsave instead of spin_lock in the RT kernel
Author: Xiaolei Wang <xiaolei.wang@windriver.com>
Patches: 2
Reviewed: 2026-03-05T13:49:50.617735

---

This is a single patch attempting to fix an RT kernel warning in v3d's `v3d_job_update_stats()` by replacing the `spin_lock()` + manual `local_irq_save()`/`preempt_disable()` pattern with `spin_lock_irqsave()`. While the motivation is valid (the current code causes a sleeping-in-atomic warning on PREEMPT_RT), the patch has a **correctness bug** and is **incomplete**.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Using spin_lock_irqsave instead of spin_lock in the RT kernel

[Claude Code Review Bot]

Patch Review

**Correctness bug: `v3d_stats_update(global_stats, now)` left unprotected**

The original code wraps *both* the `spin_lock`/`spin_unlock` region *and* the subsequent `v3d_stats_update(global_stats, now)` call inside the `preempt_disable()`/`local_irq_save()` scope:

```c
// original
preempt_disable();            // or local_irq_save
spin_lock(&queue->queue_lock);
  v3d_stats_update(&job->file_priv->stats[q], now);  // per-fd stats
spin_unlock(&queue->queue_lock);
v3d_stats_update(global_stats, now);                  // global stats -- still under preempt_disable
preempt_enable();             // or local_irq_restore
```

After the patch:

```c
spin_lock_irqsave(&queue->queue_lock, flags);
  v3d_stats_update(&job->file_priv->stats[q], now);
spin_unlock_irqrestore(&queue->queue_lock, flags);
v3d_stats_update(global_stats, now);   // <-- NO LONGER under any preemption/irq protection!
```

`v3d_stats_update()` calls `write_seqcount_begin()` which internally asserts that preemption is disabled (`lockdep_assert_preemption_disabled()`). After this patch, the `global_stats` update runs with preemption enabled and interrupts enabled, which will trigger a lockdep splat on non-RT kernels and is a correctness issue. The `global_stats` update must also be protected.

To fix this, the `v3d_stats_update(global_stats, now)` call should be moved *inside* the spin_lock_irqsave/spin_unlock_irqrestore region.

**Incomplete: `v3d_job_start_stats()` has the same pattern**

The function `v3d_job_start_stats()` at `v3d_sched.c:139` uses the identical `IS_ENABLED(CONFIG_LOCKDEP)` + `local_irq_save`/`preempt_disable` pattern to protect its `write_seqcount_begin/end` calls. On PREEMPT_RT, `v3d_job_start_stats()` will have the same class of issues. The commit message and patch only address `v3d_job_update_stats()`, leaving the sibling function unfixed.

**Commit message issues**

The commit message says:
> Disabling interrupts before calling 'spin_lock()' is unnecessary;
> its implementation is only for satisfying lockdep's requirements.

This is partially correct — the `local_irq_save` *is* for lockdep — but the `preempt_disable` is not just for lockdep. It's needed to satisfy `write_seqcount_begin()`'s requirement that preemption is disabled. The commit message conflates the two purposes.

**The `seqcount_t` type itself may be the real problem**

The lock is declared as a bare `seqcount_t` (`v3d_drv.h:50`). On PREEMPT_RT, using `seqcount_spinlock_t` (associated with the `queue_lock` spinlock) would be the more idiomatic fix. This would let lockdep understand the association between the seqcount and its protecting lock, eliminating the need for the `IS_ENABLED(CONFIG_LOCKDEP)` hack entirely, and would be RT-safe by design. The patch should consider whether converting to `seqcount_spinlock_t` is the right approach rather than just shuffling lock calls.

**Summary:** The patch introduces a regression by leaving `v3d_stats_update(global_stats, now)` outside any preemption-disabled region, is incomplete (doesn't fix `v3d_job_start_stats`), and doesn't address the underlying `seqcount_t` vs `seqcount_spinlock_t` design question. Needs a v2.

---
Generated by Claude Code Patch Reviewer