* [PATCH v3 1/6] drm/v3d: Handle error from drm_sched_entity_init()
2026-03-06 11:30 [PATCH v3 0/6] drm/v3d: Improve v3d_stats lifetime and locking Maíra Canal
@ 2026-03-06 11:30 ` Maíra Canal
2026-03-08 22:56 ` Claude review: " Claude Code Review Bot
2026-03-06 11:30 ` [PATCH v3 2/6] drm/v3d: Use raw seqcount helpers instead of fighting with lockdep Maíra Canal
` (5 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Maíra Canal @ 2026-03-06 11:30 UTC (permalink / raw)
To: Melissa Wen, Tvrtko Ursulin, Maxime Ripard
Cc: kernel-dev, dri-devel, Maíra Canal, Iago Toral Quiroga
drm_sched_entity_init() can fail but its return value is currently being
ignored in v3d_open(). Check the return value and properly unwind
on failure by destroying any already-initialized scheduler entities.
Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
drivers/gpu/drm/v3d/v3d_drv.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index dd60acdf52c2b2e44c6150f11678b291ab1f6df4..86e05fcf6cf65638c4bf97ab3511ccac40f21e2f 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -131,7 +131,7 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
struct v3d_dev *v3d = to_v3d_dev(dev);
struct v3d_file_priv *v3d_priv;
struct drm_gpu_scheduler *sched;
- int i;
+ int i, ret;
v3d_priv = kzalloc_obj(*v3d_priv);
if (!v3d_priv)
@@ -141,9 +141,11 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
for (i = 0; i < V3D_MAX_QUEUES; i++) {
sched = &v3d->queue[i].sched;
- drm_sched_entity_init(&v3d_priv->sched_entity[i],
- DRM_SCHED_PRIORITY_NORMAL, &sched,
- 1, NULL);
+ ret = drm_sched_entity_init(&v3d_priv->sched_entity[i],
+ DRM_SCHED_PRIORITY_NORMAL, &sched,
+ 1, NULL);
+ if (ret)
+ goto err_sched;
memset(&v3d_priv->stats[i], 0, sizeof(v3d_priv->stats[i]));
seqcount_init(&v3d_priv->stats[i].lock);
@@ -153,6 +155,12 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
file->driver_priv = v3d_priv;
return 0;
+
+err_sched:
+ for (i--; i >= 0; i--)
+ drm_sched_entity_destroy(&v3d_priv->sched_entity[i]);
+ kfree(v3d_priv);
+ return ret;
}
static void
--
2.53.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 2/6] drm/v3d: Use raw seqcount helpers instead of fighting with lockdep
2026-03-06 11:30 [PATCH v3 0/6] drm/v3d: Improve v3d_stats lifetime and locking Maíra Canal
2026-03-06 11:30 ` [PATCH v3 1/6] drm/v3d: Handle error from drm_sched_entity_init() Maíra Canal
@ 2026-03-06 11:30 ` Maíra Canal
2026-03-08 22:56 ` Claude review: " Claude Code Review Bot
2026-03-06 11:30 ` [PATCH v3 3/6] drm/v3d: Refcount v3d_stats Maíra Canal
` (4 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Maíra Canal @ 2026-03-06 11:30 UTC (permalink / raw)
To: Melissa Wen, Tvrtko Ursulin, Maxime Ripard
Cc: kernel-dev, dri-devel, Maíra Canal, Iago Toral Quiroga
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
The `v3d_stats` sequence counter uses regular seqcount helpers, which
carry lockdep annotations that expect a consistent IRQ context between
all writers. However, lockdep is unable to detect that v3d's readers
are never in IRQ or softirq context, and that for CPU job queues, even
the write side never is. This led to false positive that were previously
worked around by conditionally disabling local IRQs under
IS_ENABLED(CONFIG_LOCKDEP).
Switch to the raw seqcount helpers which skip lockdep tracking entirely.
This is safe because jobs are fully serialized per queue: the next job
can only be queued after the previous one has been signaled, so there is
no scope for the start and update paths to race on the same seqcount.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Co-developed-by: Maíra Canal <mcanal@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
drivers/gpu/drm/v3d/v3d_drv.c | 2 +-
drivers/gpu/drm/v3d/v3d_drv.h | 5 ++++
drivers/gpu/drm/v3d/v3d_sched.c | 54 ++++++++---------------------------------
3 files changed, 16 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 86e05fcf6cf65638c4bf97ab3511ccac40f21e2f..6086c04629adbe8611b1f7297879feacec454b43 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -194,7 +194,7 @@ void v3d_get_stats(const struct v3d_stats *stats, u64 timestamp,
unsigned int seq;
do {
- seq = read_seqcount_begin(&stats->lock);
+ seq = raw_read_seqcount_begin(&stats->lock);
*active_runtime = stats->enabled_ns;
if (stats->start_ns)
*active_runtime += timestamp - stats->start_ns;
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 314213c2671003862c486a1a7237af5480afa9e4..2e5520015e08c47fef4bfbf185eda15027992032 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -46,6 +46,11 @@ struct v3d_stats {
* This seqcount is used to protect the access to the GPU stats
* variables. It must be used as, while we are reading the stats,
* IRQs can happen and the stats can be updated.
+ *
+ * However, we use the raw seqcount helpers to interact with this lock
+ * to avoid false positives from lockdep, which is unable to detect that
+ * our readers are never from irq or softirq context, and that, for CPU
+ * job queues, even the write side never is.
*/
seqcount_t lock;
};
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 6dc871fc9a62303da4fbc62b612c3a797fe762de..18265721c1d32158fa6f7e68fa3e70a77d265b9d 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -144,54 +144,28 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
struct v3d_stats *global_stats = &v3d->queue[queue].stats;
struct v3d_stats *local_stats = &file->stats[queue];
u64 now = local_clock();
- unsigned long flags;
- /*
- * We only need to disable local interrupts to appease lockdep who
- * otherwise would think v3d_job_start_stats vs v3d_stats_update has an
- * unsafe in-irq vs no-irq-off usage problem. This is a false positive
- * because all the locks are per queue and stats type, and all jobs are
- * completely one at a time serialised. More specifically:
- *
- * 1. Locks for GPU queues are updated from interrupt handlers under a
- * spin lock and started here with preemption disabled.
- *
- * 2. Locks for CPU queues are updated from the worker with preemption
- * disabled and equally started here with preemption disabled.
- *
- * Therefore both are consistent.
- *
- * 3. Because next job can only be queued after the previous one has
- * been signaled, and locks are per queue, there is also no scope for
- * the start part to race with the update part.
- */
- if (IS_ENABLED(CONFIG_LOCKDEP))
- local_irq_save(flags);
- else
- preempt_disable();
+ preempt_disable();
- write_seqcount_begin(&local_stats->lock);
+ raw_write_seqcount_begin(&local_stats->lock);
local_stats->start_ns = now;
- write_seqcount_end(&local_stats->lock);
+ raw_write_seqcount_end(&local_stats->lock);
- write_seqcount_begin(&global_stats->lock);
+ raw_write_seqcount_begin(&global_stats->lock);
global_stats->start_ns = now;
- write_seqcount_end(&global_stats->lock);
+ raw_write_seqcount_end(&global_stats->lock);
- if (IS_ENABLED(CONFIG_LOCKDEP))
- local_irq_restore(flags);
- else
- preempt_enable();
+ preempt_enable();
}
static void
v3d_stats_update(struct v3d_stats *stats, u64 now)
{
- write_seqcount_begin(&stats->lock);
+ raw_write_seqcount_begin(&stats->lock);
stats->enabled_ns += now - stats->start_ns;
stats->jobs_completed++;
stats->start_ns = 0;
- write_seqcount_end(&stats->lock);
+ raw_write_seqcount_end(&stats->lock);
}
void
@@ -201,13 +175,8 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q)
struct v3d_queue_state *queue = &v3d->queue[q];
struct v3d_stats *global_stats = &queue->stats;
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();
+ preempt_disable();
/* Don't update the local stats if the file context has already closed */
spin_lock(&queue->queue_lock);
@@ -217,10 +186,7 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q)
v3d_stats_update(global_stats, now);
- if (IS_ENABLED(CONFIG_LOCKDEP))
- local_irq_restore(flags);
- else
- preempt_enable();
+ preempt_enable();
}
static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
--
2.53.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Claude review: drm/v3d: Use raw seqcount helpers instead of fighting with lockdep
2026-03-06 11:30 ` [PATCH v3 2/6] drm/v3d: Use raw seqcount helpers instead of fighting with lockdep Maíra Canal
@ 2026-03-08 22:56 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 22:56 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Good cleanup that removes the ugly `IS_ENABLED(CONFIG_LOCKDEP)` conditional IRQ disabling. The rationale for using raw seqcount helpers is well-explained: jobs are serialized per-queue so writers can't race on the same seqcount.
**Bug: Mismatched raw/non-raw read-side helper.** The write side is switched to `raw_write_seqcount_begin/end` and the read begin is switched to `raw_read_seqcount_begin`, but `v3d_get_stats()` at `v3d_drv.c:197` still uses the non-raw `read_seqcount_retry()`:
```c
do {
seq = raw_read_seqcount_begin(&stats->lock);
...
} while (read_seqcount_retry(&stats->lock, seq));
```
This should be `raw_read_seqcount_retry()` to match. While `read_seqcount_retry` will functionally work (it just adds a lockdep check that won't fire since the reader context itself is fine), it's inconsistent and the lockdep annotation on the retry side could theoretically trigger if lockdep's view of the seqcount changes in future kernels. For correctness and consistency, both read helpers should be raw.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 3/6] drm/v3d: Refcount v3d_stats
2026-03-06 11:30 [PATCH v3 0/6] drm/v3d: Improve v3d_stats lifetime and locking Maíra Canal
2026-03-06 11:30 ` [PATCH v3 1/6] drm/v3d: Handle error from drm_sched_entity_init() Maíra Canal
2026-03-06 11:30 ` [PATCH v3 2/6] drm/v3d: Use raw seqcount helpers instead of fighting with lockdep Maíra Canal
@ 2026-03-06 11:30 ` Maíra Canal
2026-03-08 22:56 ` Claude review: " Claude Code Review Bot
2026-03-06 11:30 ` [PATCH v3 4/6] drm/v3d: Hold v3d_stats references in each job Maíra Canal
` (3 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Maíra Canal @ 2026-03-06 11:30 UTC (permalink / raw)
To: Melissa Wen, Tvrtko Ursulin, Maxime Ripard
Cc: kernel-dev, dri-devel, Maíra Canal, Iago Toral Quiroga
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Convert `v3d_stats` from embedded structs to heap-allocated, refcounted
objects. This decouples the stats lifetime from the containing
structures (this is, `v3d_queue_state` and `v3d_file_priv`), allowing
jobs to safely hold their own references to stats objects even after the
file descriptor is closed.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Co-developed-by: Maíra Canal <mcanal@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
drivers/gpu/drm/v3d/v3d_drv.c | 19 ++++++++++++++-----
drivers/gpu/drm/v3d/v3d_drv.h | 19 +++++++++++++++++--
drivers/gpu/drm/v3d/v3d_gem.c | 42 ++++++++++++++++++++++++++---------------
drivers/gpu/drm/v3d/v3d_sched.c | 29 ++++++++++++++++++++++++----
drivers/gpu/drm/v3d/v3d_sysfs.c | 2 +-
5 files changed, 84 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 6086c04629adbe8611b1f7297879feacec454b43..0f5e29f57fa59bfa2890de51b9ca78cda33d1edc 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -140,15 +140,18 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
v3d_priv->v3d = v3d;
for (i = 0; i < V3D_MAX_QUEUES; i++) {
+ v3d_priv->stats[i] = v3d_stats_alloc();
+ if (!v3d_priv->stats[i]) {
+ ret = -ENOMEM;
+ goto err_stats;
+ }
+
sched = &v3d->queue[i].sched;
ret = drm_sched_entity_init(&v3d_priv->sched_entity[i],
DRM_SCHED_PRIORITY_NORMAL, &sched,
1, NULL);
if (ret)
goto err_sched;
-
- memset(&v3d_priv->stats[i], 0, sizeof(v3d_priv->stats[i]));
- seqcount_init(&v3d_priv->stats[i].lock);
}
v3d_perfmon_open_file(v3d_priv);
@@ -157,8 +160,12 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
return 0;
err_sched:
- for (i--; i >= 0; i--)
+ v3d_stats_put(v3d_priv->stats[i]);
+err_stats:
+ for (i--; i >= 0; i--) {
drm_sched_entity_destroy(&v3d_priv->sched_entity[i]);
+ v3d_stats_put(v3d_priv->stats[i]);
+ }
kfree(v3d_priv);
return ret;
}
@@ -182,6 +189,8 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file)
job->file_priv = NULL;
spin_unlock_irqrestore(&queue->queue_lock, irqflags);
}
+
+ v3d_stats_put(v3d_priv->stats[q]);
}
v3d_perfmon_close_file(v3d_priv);
@@ -209,7 +218,7 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
enum v3d_queue queue;
for (queue = 0; queue < V3D_MAX_QUEUES; queue++) {
- struct v3d_stats *stats = &file_priv->stats[queue];
+ struct v3d_stats *stats = file_priv->stats[queue];
u64 active_runtime, jobs_completed;
v3d_get_stats(stats, timestamp, &active_runtime, &jobs_completed);
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 2e5520015e08c47fef4bfbf185eda15027992032..03fa2d174b1ca8b5a98a72c4addaa1f977d11174 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -38,6 +38,8 @@ static inline char *v3d_queue_to_string(enum v3d_queue queue)
}
struct v3d_stats {
+ struct kref refcount;
+
u64 start_ns;
u64 enabled_ns;
u64 jobs_completed;
@@ -62,7 +64,7 @@ struct v3d_queue_state {
u64 emit_seqno;
/* Stores the GPU stats for this queue in the global context. */
- struct v3d_stats stats;
+ struct v3d_stats *stats;
/* Currently active job for this queue */
struct v3d_job *active_job;
@@ -230,7 +232,7 @@ struct v3d_file_priv {
struct drm_sched_entity sched_entity[V3D_MAX_QUEUES];
/* Stores the GPU stats for a specific queue for this fd. */
- struct v3d_stats stats[V3D_MAX_QUEUES];
+ struct v3d_stats *stats[V3D_MAX_QUEUES];
/* Per-fd reset counter, must be incremented when a job submitted
* by this fd causes a GPU reset. It must be protected by
@@ -603,10 +605,23 @@ void v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *query_info,
unsigned int count);
void v3d_performance_query_info_free(struct v3d_performance_query_info *query_info,
unsigned int count);
+struct v3d_stats *v3d_stats_alloc(void);
+void v3d_stats_release(struct kref *refcount);
void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q);
int v3d_sched_init(struct v3d_dev *v3d);
void v3d_sched_fini(struct v3d_dev *v3d);
+static inline struct v3d_stats *v3d_stats_get(struct v3d_stats *stats)
+{
+ kref_get(&stats->refcount);
+ return stats;
+}
+
+static inline void v3d_stats_put(struct v3d_stats *stats)
+{
+ kref_put(&stats->refcount, v3d_stats_release);
+}
+
/* v3d_perfmon.c */
void v3d_perfmon_init(struct v3d_dev *v3d);
void v3d_perfmon_get(struct v3d_perfmon *perfmon);
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 57965c0d6f6efea0019fb0b1a47addf2f586d138..859e63dd7e9738e3a3702edfb857ec3e844b052b 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -287,9 +287,13 @@ v3d_gem_init(struct drm_device *dev)
for (i = 0; i < V3D_MAX_QUEUES; i++) {
struct v3d_queue_state *queue = &v3d->queue[i];
+ queue->stats = v3d_stats_alloc();
+ if (!queue->stats) {
+ ret = -ENOMEM;
+ goto err_stats;
+ }
+
queue->fence_context = dma_fence_context_alloc(1);
- memset(&queue->stats, 0, sizeof(queue->stats));
- seqcount_init(&queue->stats.lock);
spin_lock_init(&queue->queue_lock);
spin_lock_init(&queue->fence_lock);
@@ -298,16 +302,16 @@ v3d_gem_init(struct drm_device *dev)
spin_lock_init(&v3d->mm_lock);
ret = drmm_mutex_init(dev, &v3d->bo_lock);
if (ret)
- return ret;
+ goto err_stats;
ret = drmm_mutex_init(dev, &v3d->reset_lock);
if (ret)
- return ret;
+ goto err_stats;
ret = drmm_mutex_init(dev, &v3d->sched_lock);
if (ret)
- return ret;
+ goto err_stats;
ret = drmm_mutex_init(dev, &v3d->cache_clean_lock);
if (ret)
- return ret;
+ goto err_stats;
/* Note: We don't allocate address 0. Various bits of HW
* treat 0 as special, such as the occlusion query counters
@@ -319,10 +323,10 @@ v3d_gem_init(struct drm_device *dev)
&v3d->pt_paddr,
GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO);
if (!v3d->pt) {
- drm_mm_takedown(&v3d->mm);
dev_err(v3d->drm.dev,
"Failed to allocate page tables. Please ensure you have DMA enabled.\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_dma_alloc;
}
v3d_init_hw_state(v3d);
@@ -331,14 +335,20 @@ v3d_gem_init(struct drm_device *dev)
v3d_huge_mnt_init(v3d);
ret = v3d_sched_init(v3d);
- if (ret) {
- drm_mm_takedown(&v3d->mm);
- dma_free_coherent(v3d->drm.dev, pt_size, (void *)v3d->pt,
- v3d->pt_paddr);
- return ret;
- }
+ if (ret)
+ goto err_sched;
return 0;
+
+err_sched:
+ dma_free_coherent(v3d->drm.dev, pt_size, (void *)v3d->pt, v3d->pt_paddr);
+err_dma_alloc:
+ drm_mm_takedown(&v3d->mm);
+err_stats:
+ for (i--; i >= 0; i--)
+ v3d_stats_put(v3d->queue[i].stats);
+
+ return ret;
}
void
@@ -352,8 +362,10 @@ v3d_gem_destroy(struct drm_device *dev)
/* Waiting for jobs to finish would need to be done before
* unregistering V3D.
*/
- for (q = 0; q < V3D_MAX_QUEUES; q++)
+ for (q = 0; q < V3D_MAX_QUEUES; q++) {
WARN_ON(v3d->queue[q].active_job);
+ v3d_stats_put(v3d->queue[q].stats);
+ }
drm_mm_takedown(&v3d->mm);
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 18265721c1d32158fa6f7e68fa3e70a77d265b9d..787f21337b2a03a923342fe4f5f927e9214082c4 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -66,6 +66,27 @@ to_cpu_job(struct drm_sched_job *sched_job)
return container_of(sched_job, struct v3d_cpu_job, base.base);
}
+void v3d_stats_release(struct kref *refcount)
+{
+ struct v3d_stats *stats = container_of(refcount, typeof(*stats), refcount);
+
+ kfree(stats);
+}
+
+struct v3d_stats *v3d_stats_alloc(void)
+{
+ struct v3d_stats *stats;
+
+ stats = kzalloc_obj(*stats);
+ if (!stats)
+ return NULL;
+
+ kref_init(&stats->refcount);
+ seqcount_init(&stats->lock);
+
+ return stats;
+}
+
static void
v3d_sched_job_free(struct drm_sched_job *sched_job)
{
@@ -141,8 +162,8 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
{
struct v3d_dev *v3d = job->v3d;
struct v3d_file_priv *file = job->file_priv;
- struct v3d_stats *global_stats = &v3d->queue[queue].stats;
- struct v3d_stats *local_stats = &file->stats[queue];
+ struct v3d_stats *global_stats = v3d->queue[queue].stats;
+ struct v3d_stats *local_stats = file->stats[queue];
u64 now = local_clock();
preempt_disable();
@@ -173,7 +194,7 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q)
{
struct v3d_dev *v3d = job->v3d;
struct v3d_queue_state *queue = &v3d->queue[q];
- struct v3d_stats *global_stats = &queue->stats;
+ struct v3d_stats *global_stats = queue->stats;
u64 now = local_clock();
preempt_disable();
@@ -181,7 +202,7 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q)
/* Don't update the local stats if the file context has already closed */
spin_lock(&queue->queue_lock);
if (job->file_priv)
- v3d_stats_update(&job->file_priv->stats[q], now);
+ v3d_stats_update(job->file_priv->stats[q], now);
spin_unlock(&queue->queue_lock);
v3d_stats_update(global_stats, now);
diff --git a/drivers/gpu/drm/v3d/v3d_sysfs.c b/drivers/gpu/drm/v3d/v3d_sysfs.c
index d610e355964ffaf45f7d44e5c667369cedc205dc..b45a9b3db42d698856873145e4948aad27eeb28c 100644
--- a/drivers/gpu/drm/v3d/v3d_sysfs.c
+++ b/drivers/gpu/drm/v3d/v3d_sysfs.c
@@ -20,7 +20,7 @@ gpu_stats_show(struct device *dev, struct device_attribute *attr, char *buf)
len += sysfs_emit(buf, "queue\ttimestamp\tjobs\truntime\n");
for (queue = 0; queue < V3D_MAX_QUEUES; queue++) {
- struct v3d_stats *stats = &v3d->queue[queue].stats;
+ struct v3d_stats *stats = v3d->queue[queue].stats;
u64 active_runtime, jobs_completed;
v3d_get_stats(stats, timestamp, &active_runtime, &jobs_completed);
--
2.53.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Claude review: drm/v3d: Refcount v3d_stats
2026-03-06 11:30 ` [PATCH v3 3/6] drm/v3d: Refcount v3d_stats Maíra Canal
@ 2026-03-08 22:56 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 22:56 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Well-implemented refcounting conversion. The `v3d_stats_alloc()` / `v3d_stats_get()` / `v3d_stats_put()` API is clean.
The error unwinding in `v3d_open()` is correct: when `drm_sched_entity_init` fails, `err_sched` puts the stats[i] that was allocated in the current iteration (before the entity init), then `err_stats` walks backwards to clean up both entities and stats from prior iterations.
The `v3d_gem_init()` error path refactoring is a nice improvement over the previous inline cleanup code. One minor observation: when `drmm_mutex_init` calls fail after the loop completes (`i == V3D_MAX_QUEUES`), the `err_stats` label does `for (i--; i >= 0; i--)` which correctly walks all queues. Good.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 4/6] drm/v3d: Hold v3d_stats references in each job
2026-03-06 11:30 [PATCH v3 0/6] drm/v3d: Improve v3d_stats lifetime and locking Maíra Canal
` (2 preceding siblings ...)
2026-03-06 11:30 ` [PATCH v3 3/6] drm/v3d: Refcount v3d_stats Maíra Canal
@ 2026-03-06 11:30 ` Maíra Canal
2026-03-08 22:56 ` Claude review: " Claude Code Review Bot
2026-03-06 11:30 ` [PATCH v3 5/6] drm/v3d: Attach per-fd reset counters to v3d_stats Maíra Canal
` (2 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Maíra Canal @ 2026-03-06 11:30 UTC (permalink / raw)
To: Melissa Wen, Tvrtko Ursulin, Maxime Ripard
Cc: kernel-dev, dri-devel, Maíra Canal, Iago Toral Quiroga
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Have each job hold its own references to the per-fd and global stats
objects. This eliminates the need for `queue_lock` protection in the
stats update path, since the job's stats pointers are guaranteed to
remain valid for the job's entire lifetime regardless of file descriptor
closure.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Co-developed-by: Maíra Canal <mcanal@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
drivers/gpu/drm/v3d/v3d_drv.h | 6 ++++-
drivers/gpu/drm/v3d/v3d_irq.c | 2 +-
drivers/gpu/drm/v3d/v3d_sched.c | 57 ++++++++++++++++------------------------
drivers/gpu/drm/v3d/v3d_submit.c | 6 +++++
4 files changed, 34 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 03fa2d174b1ca8b5a98a72c4addaa1f977d11174..72c3f40715dae6e86e0c8356cb997cdf1cf03fae 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -326,6 +326,10 @@ struct v3d_job {
*/
struct v3d_file_priv *file_priv;
+ /* Pointers to this job's per-fd and global queue stats. */
+ struct v3d_stats *client_stats;
+ struct v3d_stats *global_stats;
+
/* Callback for the freeing of the job on refcount going to 0. */
void (*free)(struct kref *ref);
};
@@ -607,7 +611,7 @@ void v3d_performance_query_info_free(struct v3d_performance_query_info *query_in
unsigned int count);
struct v3d_stats *v3d_stats_alloc(void);
void v3d_stats_release(struct kref *refcount);
-void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q);
+void v3d_job_update_stats(struct v3d_job *job);
int v3d_sched_init(struct v3d_dev *v3d);
void v3d_sched_fini(struct v3d_dev *v3d);
diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 1249f6e64b979fe29cf2b9bfc43b39aa755f71ce..c28e74ab5442857031b48bcbd4e43eb48c1e0f07 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -92,7 +92,7 @@ v3d_irq_signal_fence(struct v3d_dev *v3d, enum v3d_queue q,
struct v3d_queue_state *queue = &v3d->queue[q];
struct v3d_fence *fence = to_v3d_fence(queue->active_job->irq_fence);
- v3d_job_update_stats(queue->active_job, q);
+ v3d_job_update_stats(queue->active_job);
trace_irq(&v3d->drm, fence->seqno);
queue->active_job = NULL;
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 787f21337b2a03a923342fe4f5f927e9214082c4..5c387a152e33f5ccbca6a9af97675f050a2a701f 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -158,24 +158,21 @@ v3d_switch_perfmon(struct v3d_dev *v3d, struct v3d_job *job)
}
static void
-v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
+v3d_stats_start(struct v3d_stats *stats, u64 now)
+{
+ raw_write_seqcount_begin(&stats->lock);
+ stats->start_ns = now;
+ raw_write_seqcount_end(&stats->lock);
+}
+
+static void
+v3d_job_start_stats(struct v3d_job *job)
{
- struct v3d_dev *v3d = job->v3d;
- struct v3d_file_priv *file = job->file_priv;
- struct v3d_stats *global_stats = v3d->queue[queue].stats;
- struct v3d_stats *local_stats = file->stats[queue];
u64 now = local_clock();
preempt_disable();
-
- raw_write_seqcount_begin(&local_stats->lock);
- local_stats->start_ns = now;
- raw_write_seqcount_end(&local_stats->lock);
-
- raw_write_seqcount_begin(&global_stats->lock);
- global_stats->start_ns = now;
- raw_write_seqcount_end(&global_stats->lock);
-
+ v3d_stats_start(job->client_stats, now);
+ v3d_stats_start(job->global_stats, now);
preempt_enable();
}
@@ -190,23 +187,13 @@ v3d_stats_update(struct v3d_stats *stats, u64 now)
}
void
-v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q)
+v3d_job_update_stats(struct v3d_job *job)
{
- struct v3d_dev *v3d = job->v3d;
- struct v3d_queue_state *queue = &v3d->queue[q];
- struct v3d_stats *global_stats = queue->stats;
u64 now = local_clock();
preempt_disable();
-
- /* Don't update the local stats if the file context has already closed */
- spin_lock(&queue->queue_lock);
- if (job->file_priv)
- v3d_stats_update(job->file_priv->stats[q], now);
- spin_unlock(&queue->queue_lock);
-
- v3d_stats_update(global_stats, now);
-
+ v3d_stats_update(job->client_stats, now);
+ v3d_stats_update(job->global_stats, now);
preempt_enable();
}
@@ -250,7 +237,7 @@ static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
trace_v3d_submit_cl(dev, false, to_v3d_fence(fence)->seqno,
job->start, job->end);
- v3d_job_start_stats(&job->base, V3D_BIN);
+ v3d_job_start_stats(&job->base);
v3d_switch_perfmon(v3d, &job->base);
/* Set the current and end address of the control list.
@@ -304,7 +291,7 @@ static struct dma_fence *v3d_render_job_run(struct drm_sched_job *sched_job)
trace_v3d_submit_cl(dev, true, to_v3d_fence(fence)->seqno,
job->start, job->end);
- v3d_job_start_stats(&job->base, V3D_RENDER);
+ v3d_job_start_stats(&job->base);
v3d_switch_perfmon(v3d, &job->base);
/* XXX: Set the QCFG */
@@ -343,7 +330,7 @@ v3d_tfu_job_run(struct drm_sched_job *sched_job)
trace_v3d_submit_tfu(dev, to_v3d_fence(fence)->seqno);
- v3d_job_start_stats(&job->base, V3D_TFU);
+ v3d_job_start_stats(&job->base);
V3D_WRITE(V3D_TFU_IIA(v3d->ver), job->args.iia);
V3D_WRITE(V3D_TFU_IIS(v3d->ver), job->args.iis);
@@ -393,7 +380,7 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
trace_v3d_submit_csd(dev, to_v3d_fence(fence)->seqno);
- v3d_job_start_stats(&job->base, V3D_CSD);
+ v3d_job_start_stats(&job->base);
v3d_switch_perfmon(v3d, &job->base);
csd_cfg0_reg = V3D_CSD_QUEUED_CFG0(v3d->ver);
@@ -681,13 +668,13 @@ v3d_cpu_job_run(struct drm_sched_job *sched_job)
return NULL;
}
- v3d_job_start_stats(&job->base, V3D_CPU);
+ v3d_job_start_stats(&job->base);
trace_v3d_cpu_job_begin(&v3d->drm, job->job_type);
cpu_job_function[job->job_type](job);
trace_v3d_cpu_job_end(&v3d->drm, job->job_type);
- v3d_job_update_stats(&job->base, V3D_CPU);
+ v3d_job_update_stats(&job->base);
/* Synchronous operation, so no fence to wait on. */
return NULL;
@@ -699,11 +686,11 @@ v3d_cache_clean_job_run(struct drm_sched_job *sched_job)
struct v3d_job *job = to_v3d_job(sched_job);
struct v3d_dev *v3d = job->v3d;
- v3d_job_start_stats(job, V3D_CACHE_CLEAN);
+ v3d_job_start_stats(job);
v3d_clean_caches(v3d);
- v3d_job_update_stats(job, V3D_CACHE_CLEAN);
+ v3d_job_update_stats(job);
/* Synchronous operation, so no fence to wait on. */
return NULL;
diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index 18f2bf1fe89face6ede3de465c80b63a6635511e..8f061b6a05c6aa76ea5513407ebf3c0ce80b8256 100644
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -103,6 +103,9 @@ v3d_job_free(struct kref *ref)
if (job->perfmon)
v3d_perfmon_put(job->perfmon);
+ v3d_stats_put(job->client_stats);
+ v3d_stats_put(job->global_stats);
+
kfree(job);
}
@@ -203,6 +206,9 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
kref_init(&job->refcount);
+ job->client_stats = v3d_stats_get(v3d_priv->stats[queue]);
+ job->global_stats = v3d_stats_get(v3d->queue[queue].stats);
+
return 0;
fail_deps:
--
2.53.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Claude review: drm/v3d: Hold v3d_stats references in each job
2026-03-06 11:30 ` [PATCH v3 4/6] drm/v3d: Hold v3d_stats references in each job Maíra Canal
@ 2026-03-08 22:56 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 22:56 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the key patch that eliminates the race. By having each job hold its own kref'd references to both `client_stats` and `global_stats`, the stats pointers in the job are guaranteed valid for the job's entire lifetime, regardless of fd closure.
The refactoring of `v3d_job_start_stats` and `v3d_job_update_stats` to no longer take a queue parameter is clean - the job already knows its stats pointers.
The removal of the `queue_lock` / `file_priv` NULL check dance in `v3d_job_update_stats` is the main payoff:
```c
- /* Don't update the local stats if the file context has already closed */
- spin_lock(&queue->queue_lock);
- if (job->file_priv)
- v3d_stats_update(job->file_priv->stats[q], now);
- spin_unlock(&queue->queue_lock);
```
becomes simply:
```c
v3d_stats_update(job->client_stats, now);
```
This is safe because the job's kref keeps the stats object alive.
The stats refs are taken in `v3d_job_init()` after `kref_init(&job->refcount)` and released in `v3d_job_free()`. The placement looks correct.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 5/6] drm/v3d: Attach per-fd reset counters to v3d_stats
2026-03-06 11:30 [PATCH v3 0/6] drm/v3d: Improve v3d_stats lifetime and locking Maíra Canal
` (3 preceding siblings ...)
2026-03-06 11:30 ` [PATCH v3 4/6] drm/v3d: Hold v3d_stats references in each job Maíra Canal
@ 2026-03-06 11:30 ` Maíra Canal
2026-03-08 22:56 ` Claude review: " Claude Code Review Bot
2026-03-06 11:30 ` [PATCH v3 6/6] drm/v3d: Remove dedicated fence_lock Maíra Canal
2026-03-08 22:56 ` Claude review: drm/v3d: Improve v3d_stats lifetime and locking Claude Code Review Bot
6 siblings, 1 reply; 15+ messages in thread
From: Maíra Canal @ 2026-03-06 11:30 UTC (permalink / raw)
To: Melissa Wen, Tvrtko Ursulin, Maxime Ripard
Cc: kernel-dev, dri-devel, Maíra Canal, Iago Toral Quiroga
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
To remove the file_priv NULL-ing dance needed to check if the file
descriptor is open, move the per-fd reset counter into v3d_stats, which
is heap-allocated and refcounted, outliving the fd as long as jobs
reference it.
This change allows the removal of the last `queue_lock` usage to protect
`job->file_priv` and avoids possible NULL ptr dereference issues due to
lifetime mismatches.
Also, to simplify locking, replace both the global and per-fd locked
reset counters with atomics.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Co-developed-by: Maíra Canal <mcanal@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
drivers/gpu/drm/v3d/v3d_drv.c | 22 ++++------------------
drivers/gpu/drm/v3d/v3d_drv.h | 14 ++++----------
drivers/gpu/drm/v3d/v3d_sched.c | 9 ++-------
3 files changed, 10 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 0f5e29f57fa59bfa2890de51b9ca78cda33d1edc..4b441afcb602de08bd193d57649121e44ab31f2a 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -110,14 +110,12 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void *data,
args->value = !!drm_gem_get_huge_mnt(dev);
return 0;
case DRM_V3D_PARAM_GLOBAL_RESET_COUNTER:
- mutex_lock(&v3d->reset_lock);
- args->value = v3d->reset_counter;
- mutex_unlock(&v3d->reset_lock);
+ args->value = atomic_read(&v3d->reset_counter);
return 0;
case DRM_V3D_PARAM_CONTEXT_RESET_COUNTER:
- mutex_lock(&v3d->reset_lock);
- args->value = v3d_priv->reset_counter;
- mutex_unlock(&v3d->reset_lock);
+ args->value = 0;
+ for (enum v3d_queue q = 0; q < V3D_MAX_QUEUES; q++)
+ args->value += atomic_read(&v3d_priv->stats[q]->reset_counter);
return 0;
default:
drm_dbg(dev, "Unknown parameter %d\n", args->param);
@@ -173,23 +171,11 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
static void
v3d_postclose(struct drm_device *dev, struct drm_file *file)
{
- struct v3d_dev *v3d = to_v3d_dev(dev);
struct v3d_file_priv *v3d_priv = file->driver_priv;
- unsigned long irqflags;
enum v3d_queue q;
for (q = 0; q < V3D_MAX_QUEUES; q++) {
- struct v3d_queue_state *queue = &v3d->queue[q];
- struct v3d_job *job = queue->active_job;
-
drm_sched_entity_destroy(&v3d_priv->sched_entity[q]);
-
- if (job && job->base.entity == &v3d_priv->sched_entity[q]) {
- spin_lock_irqsave(&queue->queue_lock, irqflags);
- job->file_priv = NULL;
- spin_unlock_irqrestore(&queue->queue_lock, irqflags);
- }
-
v3d_stats_put(v3d_priv->stats[q]);
}
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 72c3f40715dae6e86e0c8356cb997cdf1cf03fae..3de485abd8fc274b361cd17a00cab189d8e69643 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -55,6 +55,8 @@ struct v3d_stats {
* job queues, even the write side never is.
*/
seqcount_t lock;
+
+ atomic_t reset_counter;
};
struct v3d_queue_state {
@@ -203,10 +205,8 @@ struct v3d_dev {
*/
struct v3d_perfmon *global_perfmon;
- /* Global reset counter. The counter must be incremented when
- * a GPU reset happens. It must be protected by @reset_lock.
- */
- unsigned int reset_counter;
+ /* Global reset counter incremented on each GPU reset. */
+ atomic_t reset_counter;
};
static inline struct v3d_dev *
@@ -233,12 +233,6 @@ struct v3d_file_priv {
/* Stores the GPU stats for a specific queue for this fd. */
struct v3d_stats *stats[V3D_MAX_QUEUES];
-
- /* Per-fd reset counter, must be incremented when a job submitted
- * by this fd causes a GPU reset. It must be protected by
- * &struct v3d_dev->reset_lock.
- */
- unsigned int reset_counter;
};
struct v3d_bo {
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 5c387a152e33f5ccbca6a9af97675f050a2a701f..1855ef5b3b5fe4e2de1cf0b77bced3735c23ab15 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -701,8 +701,6 @@ v3d_gpu_reset_for_timeout(struct v3d_dev *v3d, struct drm_sched_job *sched_job,
enum v3d_queue q)
{
struct v3d_job *job = to_v3d_job(sched_job);
- struct v3d_file_priv *v3d_priv = job->file_priv;
- unsigned long irqflags;
enum v3d_queue i;
mutex_lock(&v3d->reset_lock);
@@ -717,11 +715,8 @@ v3d_gpu_reset_for_timeout(struct v3d_dev *v3d, struct drm_sched_job *sched_job,
/* get the GPU back into the init state */
v3d_reset(v3d);
- v3d->reset_counter++;
- spin_lock_irqsave(&v3d->queue[q].queue_lock, irqflags);
- if (v3d_priv)
- v3d_priv->reset_counter++;
- spin_unlock_irqrestore(&v3d->queue[q].queue_lock, irqflags);
+ atomic_inc(&v3d->reset_counter);
+ atomic_inc(&job->client_stats->reset_counter);
for (i = 0; i < V3D_MAX_QUEUES; i++)
drm_sched_resubmit_jobs(&v3d->queue[i].sched);
--
2.53.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Claude review: drm/v3d: Attach per-fd reset counters to v3d_stats
2026-03-06 11:30 ` [PATCH v3 5/6] drm/v3d: Attach per-fd reset counters to v3d_stats Maíra Canal
@ 2026-03-08 22:56 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 22:56 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Moving the per-fd reset counter into `v3d_stats` is the natural follow-up. Since `v3d_stats` now outlives the fd (via job references), the reset can safely increment the counter through `job->client_stats->reset_counter` without needing the `file_priv` NULL check.
Using `atomic_t` for both global and per-fd reset counters is appropriate - they're incremented under `reset_lock` but read without it, so atomics provide the necessary visibility.
The `CONTEXT_RESET_COUNTER` ioctl now sums across all queues:
```c
args->value = 0;
for (enum v3d_queue q = 0; q < V3D_MAX_QUEUES; q++)
args->value += atomic_read(&v3d_priv->stats[q]->reset_counter);
```
This is a slight semantic change from the old per-fd single counter: a single GPU reset previously incremented `v3d_priv->reset_counter` once, but now it increments only the specific queue's stats `reset_counter`. Since `v3d_gpu_reset_for_timeout` increments `job->client_stats->reset_counter` (one specific queue), and the ioctl sums all queues, the total should still reflect one increment per reset caused by this fd. This looks correct.
The big cleanup in `v3d_postclose` removing the `file_priv` NULL-ing dance is the main simplification goal of the series, and it's well-justified at this point.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 6/6] drm/v3d: Remove dedicated fence_lock
2026-03-06 11:30 [PATCH v3 0/6] drm/v3d: Improve v3d_stats lifetime and locking Maíra Canal
` (4 preceding siblings ...)
2026-03-06 11:30 ` [PATCH v3 5/6] drm/v3d: Attach per-fd reset counters to v3d_stats Maíra Canal
@ 2026-03-06 11:30 ` Maíra Canal
2026-03-08 22:56 ` Claude review: " Claude Code Review Bot
2026-03-08 22:56 ` Claude review: drm/v3d: Improve v3d_stats lifetime and locking Claude Code Review Bot
6 siblings, 1 reply; 15+ messages in thread
From: Maíra Canal @ 2026-03-06 11:30 UTC (permalink / raw)
To: Melissa Wen, Tvrtko Ursulin, Maxime Ripard
Cc: kernel-dev, dri-devel, Maíra Canal, Iago Toral Quiroga
Commit adefb2ccea1e ("drm/v3d: create a dedicated lock for dma fence")
split `fence_lock` from `queue_lock` because v3d_job_update_stats() was
taking `queue_lock` to protect `job->file_priv` during stats collection
in the IRQ handler. Using the same lock for both DMA fence signaling and
stats protection in a IRQ context caused issues on PREEMPT_RT.
Since then, the stats infrastructure has been reworked: v3d_stats is now
refcounted and jobs hold their own references to stats objects, so
v3d_job_update_stats() no longer takes `queue_lock` at all.
With the original reason for the split gone, merge `fence_lock` back
into `queue_lock` to simplify the locking scheme.
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
drivers/gpu/drm/v3d/v3d_drv.h | 2 --
drivers/gpu/drm/v3d/v3d_fence.c | 2 +-
drivers/gpu/drm/v3d/v3d_gem.c | 1 -
3 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 3de485abd8fc274b361cd17a00cab189d8e69643..6a3cad933439812d78da5797749c020a9bf46402 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -71,8 +71,6 @@ struct v3d_queue_state {
/* Currently active job for this queue */
struct v3d_job *active_job;
spinlock_t queue_lock;
- /* Protect dma fence for signalling job completion */
- spinlock_t fence_lock;
};
/* Performance monitor object. The perform lifetime is controlled by userspace
diff --git a/drivers/gpu/drm/v3d/v3d_fence.c b/drivers/gpu/drm/v3d/v3d_fence.c
index 3c999f60003c8746fba3c635efcaa7ac8c9ecb41..c500136d045572fbaa1e69877b21255c9091dcb1 100644
--- a/drivers/gpu/drm/v3d/v3d_fence.c
+++ b/drivers/gpu/drm/v3d/v3d_fence.c
@@ -15,7 +15,7 @@ struct dma_fence *v3d_fence_create(struct v3d_dev *v3d, enum v3d_queue q)
fence->dev = &v3d->drm;
fence->queue = q;
fence->seqno = ++queue->emit_seqno;
- dma_fence_init(&fence->base, &v3d_fence_ops, &queue->fence_lock,
+ dma_fence_init(&fence->base, &v3d_fence_ops, &queue->queue_lock,
queue->fence_context, fence->seqno);
return &fence->base;
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 859e63dd7e9738e3a3702edfb857ec3e844b052b..75d9eccd796664e67277c1f83ad59063f164d1da 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -296,7 +296,6 @@ v3d_gem_init(struct drm_device *dev)
queue->fence_context = dma_fence_context_alloc(1);
spin_lock_init(&queue->queue_lock);
- spin_lock_init(&queue->fence_lock);
}
spin_lock_init(&v3d->mm_lock);
--
2.53.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Claude review: drm/v3d: Remove dedicated fence_lock
2026-03-06 11:30 ` [PATCH v3 6/6] drm/v3d: Remove dedicated fence_lock Maíra Canal
@ 2026-03-08 22:56 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 22:56 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Straightforward cleanup. Since `v3d_job_update_stats()` no longer takes `queue_lock`, there's no longer a conflict between DMA fence signaling (which needs a spinlock in IRQ context) and stats updates. The `fence_lock` can be folded back into `queue_lock`.
This is safe because `queue_lock` is still taken with `spin_lock_irqsave` in all its remaining call sites (bin job run, overflow mem work, IRQ handler), so it's compatible with the IRQ context requirements of `dma_fence_init`.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Claude review: drm/v3d: Improve v3d_stats lifetime and locking
2026-03-06 11:30 [PATCH v3 0/6] drm/v3d: Improve v3d_stats lifetime and locking Maíra Canal
` (5 preceding siblings ...)
2026-03-06 11:30 ` [PATCH v3 6/6] drm/v3d: Remove dedicated fence_lock Maíra Canal
@ 2026-03-08 22:56 ` Claude Code Review Bot
6 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 22:56 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/v3d: Improve v3d_stats lifetime and locking
Author: =?utf-8?q?Ma=C3=ADra_Canal?= <mcanal@igalia.com>
Patches: 7
Reviewed: 2026-03-09T08:56:08.355925
---
This is a well-structured series that improves the lifetime management of `v3d_stats` by converting them from embedded structs to heap-allocated, refcounted objects. The core motivation is sound: the previous approach used a fragile `file_priv` NULL-ing dance under `queue_lock` to handle the race between fd closure and stats updates in the IRQ handler. By giving each job its own kref'd reference to the stats, the race is eliminated cleanly.
The series is logically ordered with good incremental steps: error handling fix, lockdep cleanup, refcounting introduction, per-job references, reset counter migration, and finally lock removal. Each patch has a clear purpose and the overall result simplifies both locking and lifetime management.
One bug was spotted in the read-side seqcount usage. The series is otherwise solid.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/6] drm/v3d: Refcount v3d_stats
2026-03-05 23:25 [PATCH v2 0/6] " Maíra Canal
@ 2026-03-05 23:25 ` Maíra Canal
2026-03-08 23:20 ` Claude review: " Claude Code Review Bot
0 siblings, 1 reply; 15+ messages in thread
From: Maíra Canal @ 2026-03-05 23:25 UTC (permalink / raw)
To: Melissa Wen, Tvrtko Ursulin, Maxime Ripard
Cc: kernel-dev, dri-devel, Maíra Canal, Iago Toral Quiroga
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Convert `v3d_stats` from embedded structs to heap-allocated, refcounted
objects. This decouples the stats lifetime from the containing
structures (this is, `v3d_queue_state` and `v3d_file_priv`), allowing
jobs to safely hold their own references to stats objects even after the
file descriptor is closed.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Co-developed-by: Maíra Canal <mcanal@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
---
drivers/gpu/drm/v3d/v3d_drv.c | 19 ++++++++++++++-----
drivers/gpu/drm/v3d/v3d_drv.h | 19 +++++++++++++++++--
drivers/gpu/drm/v3d/v3d_gem.c | 42 ++++++++++++++++++++++++++---------------
drivers/gpu/drm/v3d/v3d_sched.c | 29 ++++++++++++++++++++++++----
drivers/gpu/drm/v3d/v3d_sysfs.c | 2 +-
5 files changed, 84 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 6086c04629adbe8611b1f7297879feacec454b43..0f5e29f57fa59bfa2890de51b9ca78cda33d1edc 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -140,15 +140,18 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
v3d_priv->v3d = v3d;
for (i = 0; i < V3D_MAX_QUEUES; i++) {
+ v3d_priv->stats[i] = v3d_stats_alloc();
+ if (!v3d_priv->stats[i]) {
+ ret = -ENOMEM;
+ goto err_stats;
+ }
+
sched = &v3d->queue[i].sched;
ret = drm_sched_entity_init(&v3d_priv->sched_entity[i],
DRM_SCHED_PRIORITY_NORMAL, &sched,
1, NULL);
if (ret)
goto err_sched;
-
- memset(&v3d_priv->stats[i], 0, sizeof(v3d_priv->stats[i]));
- seqcount_init(&v3d_priv->stats[i].lock);
}
v3d_perfmon_open_file(v3d_priv);
@@ -157,8 +160,12 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
return 0;
err_sched:
- for (i--; i >= 0; i--)
+ v3d_stats_put(v3d_priv->stats[i]);
+err_stats:
+ for (i--; i >= 0; i--) {
drm_sched_entity_destroy(&v3d_priv->sched_entity[i]);
+ v3d_stats_put(v3d_priv->stats[i]);
+ }
kfree(v3d_priv);
return ret;
}
@@ -182,6 +189,8 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file)
job->file_priv = NULL;
spin_unlock_irqrestore(&queue->queue_lock, irqflags);
}
+
+ v3d_stats_put(v3d_priv->stats[q]);
}
v3d_perfmon_close_file(v3d_priv);
@@ -209,7 +218,7 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
enum v3d_queue queue;
for (queue = 0; queue < V3D_MAX_QUEUES; queue++) {
- struct v3d_stats *stats = &file_priv->stats[queue];
+ struct v3d_stats *stats = file_priv->stats[queue];
u64 active_runtime, jobs_completed;
v3d_get_stats(stats, timestamp, &active_runtime, &jobs_completed);
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 2e5520015e08c47fef4bfbf185eda15027992032..03fa2d174b1ca8b5a98a72c4addaa1f977d11174 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -38,6 +38,8 @@ static inline char *v3d_queue_to_string(enum v3d_queue queue)
}
struct v3d_stats {
+ struct kref refcount;
+
u64 start_ns;
u64 enabled_ns;
u64 jobs_completed;
@@ -62,7 +64,7 @@ struct v3d_queue_state {
u64 emit_seqno;
/* Stores the GPU stats for this queue in the global context. */
- struct v3d_stats stats;
+ struct v3d_stats *stats;
/* Currently active job for this queue */
struct v3d_job *active_job;
@@ -230,7 +232,7 @@ struct v3d_file_priv {
struct drm_sched_entity sched_entity[V3D_MAX_QUEUES];
/* Stores the GPU stats for a specific queue for this fd. */
- struct v3d_stats stats[V3D_MAX_QUEUES];
+ struct v3d_stats *stats[V3D_MAX_QUEUES];
/* Per-fd reset counter, must be incremented when a job submitted
* by this fd causes a GPU reset. It must be protected by
@@ -603,10 +605,23 @@ void v3d_timestamp_query_info_free(struct v3d_timestamp_query_info *query_info,
unsigned int count);
void v3d_performance_query_info_free(struct v3d_performance_query_info *query_info,
unsigned int count);
+struct v3d_stats *v3d_stats_alloc(void);
+void v3d_stats_release(struct kref *refcount);
void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q);
int v3d_sched_init(struct v3d_dev *v3d);
void v3d_sched_fini(struct v3d_dev *v3d);
+static inline struct v3d_stats *v3d_stats_get(struct v3d_stats *stats)
+{
+ kref_get(&stats->refcount);
+ return stats;
+}
+
+static inline void v3d_stats_put(struct v3d_stats *stats)
+{
+ kref_put(&stats->refcount, v3d_stats_release);
+}
+
/* v3d_perfmon.c */
void v3d_perfmon_init(struct v3d_dev *v3d);
void v3d_perfmon_get(struct v3d_perfmon *perfmon);
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 57965c0d6f6efea0019fb0b1a47addf2f586d138..859e63dd7e9738e3a3702edfb857ec3e844b052b 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -287,9 +287,13 @@ v3d_gem_init(struct drm_device *dev)
for (i = 0; i < V3D_MAX_QUEUES; i++) {
struct v3d_queue_state *queue = &v3d->queue[i];
+ queue->stats = v3d_stats_alloc();
+ if (!queue->stats) {
+ ret = -ENOMEM;
+ goto err_stats;
+ }
+
queue->fence_context = dma_fence_context_alloc(1);
- memset(&queue->stats, 0, sizeof(queue->stats));
- seqcount_init(&queue->stats.lock);
spin_lock_init(&queue->queue_lock);
spin_lock_init(&queue->fence_lock);
@@ -298,16 +302,16 @@ v3d_gem_init(struct drm_device *dev)
spin_lock_init(&v3d->mm_lock);
ret = drmm_mutex_init(dev, &v3d->bo_lock);
if (ret)
- return ret;
+ goto err_stats;
ret = drmm_mutex_init(dev, &v3d->reset_lock);
if (ret)
- return ret;
+ goto err_stats;
ret = drmm_mutex_init(dev, &v3d->sched_lock);
if (ret)
- return ret;
+ goto err_stats;
ret = drmm_mutex_init(dev, &v3d->cache_clean_lock);
if (ret)
- return ret;
+ goto err_stats;
/* Note: We don't allocate address 0. Various bits of HW
* treat 0 as special, such as the occlusion query counters
@@ -319,10 +323,10 @@ v3d_gem_init(struct drm_device *dev)
&v3d->pt_paddr,
GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO);
if (!v3d->pt) {
- drm_mm_takedown(&v3d->mm);
dev_err(v3d->drm.dev,
"Failed to allocate page tables. Please ensure you have DMA enabled.\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_dma_alloc;
}
v3d_init_hw_state(v3d);
@@ -331,14 +335,20 @@ v3d_gem_init(struct drm_device *dev)
v3d_huge_mnt_init(v3d);
ret = v3d_sched_init(v3d);
- if (ret) {
- drm_mm_takedown(&v3d->mm);
- dma_free_coherent(v3d->drm.dev, pt_size, (void *)v3d->pt,
- v3d->pt_paddr);
- return ret;
- }
+ if (ret)
+ goto err_sched;
return 0;
+
+err_sched:
+ dma_free_coherent(v3d->drm.dev, pt_size, (void *)v3d->pt, v3d->pt_paddr);
+err_dma_alloc:
+ drm_mm_takedown(&v3d->mm);
+err_stats:
+ for (i--; i >= 0; i--)
+ v3d_stats_put(v3d->queue[i].stats);
+
+ return ret;
}
void
@@ -352,8 +362,10 @@ v3d_gem_destroy(struct drm_device *dev)
/* Waiting for jobs to finish would need to be done before
* unregistering V3D.
*/
- for (q = 0; q < V3D_MAX_QUEUES; q++)
+ for (q = 0; q < V3D_MAX_QUEUES; q++) {
WARN_ON(v3d->queue[q].active_job);
+ v3d_stats_put(v3d->queue[q].stats);
+ }
drm_mm_takedown(&v3d->mm);
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 18265721c1d32158fa6f7e68fa3e70a77d265b9d..787f21337b2a03a923342fe4f5f927e9214082c4 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -66,6 +66,27 @@ to_cpu_job(struct drm_sched_job *sched_job)
return container_of(sched_job, struct v3d_cpu_job, base.base);
}
+void v3d_stats_release(struct kref *refcount)
+{
+ struct v3d_stats *stats = container_of(refcount, typeof(*stats), refcount);
+
+ kfree(stats);
+}
+
+struct v3d_stats *v3d_stats_alloc(void)
+{
+ struct v3d_stats *stats;
+
+ stats = kzalloc_obj(*stats);
+ if (!stats)
+ return NULL;
+
+ kref_init(&stats->refcount);
+ seqcount_init(&stats->lock);
+
+ return stats;
+}
+
static void
v3d_sched_job_free(struct drm_sched_job *sched_job)
{
@@ -141,8 +162,8 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
{
struct v3d_dev *v3d = job->v3d;
struct v3d_file_priv *file = job->file_priv;
- struct v3d_stats *global_stats = &v3d->queue[queue].stats;
- struct v3d_stats *local_stats = &file->stats[queue];
+ struct v3d_stats *global_stats = v3d->queue[queue].stats;
+ struct v3d_stats *local_stats = file->stats[queue];
u64 now = local_clock();
preempt_disable();
@@ -173,7 +194,7 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q)
{
struct v3d_dev *v3d = job->v3d;
struct v3d_queue_state *queue = &v3d->queue[q];
- struct v3d_stats *global_stats = &queue->stats;
+ struct v3d_stats *global_stats = queue->stats;
u64 now = local_clock();
preempt_disable();
@@ -181,7 +202,7 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue q)
/* Don't update the local stats if the file context has already closed */
spin_lock(&queue->queue_lock);
if (job->file_priv)
- v3d_stats_update(&job->file_priv->stats[q], now);
+ v3d_stats_update(job->file_priv->stats[q], now);
spin_unlock(&queue->queue_lock);
v3d_stats_update(global_stats, now);
diff --git a/drivers/gpu/drm/v3d/v3d_sysfs.c b/drivers/gpu/drm/v3d/v3d_sysfs.c
index d610e355964ffaf45f7d44e5c667369cedc205dc..b45a9b3db42d698856873145e4948aad27eeb28c 100644
--- a/drivers/gpu/drm/v3d/v3d_sysfs.c
+++ b/drivers/gpu/drm/v3d/v3d_sysfs.c
@@ -20,7 +20,7 @@ gpu_stats_show(struct device *dev, struct device_attribute *attr, char *buf)
len += sysfs_emit(buf, "queue\ttimestamp\tjobs\truntime\n");
for (queue = 0; queue < V3D_MAX_QUEUES; queue++) {
- struct v3d_stats *stats = &v3d->queue[queue].stats;
+ struct v3d_stats *stats = v3d->queue[queue].stats;
u64 active_runtime, jobs_completed;
v3d_get_stats(stats, timestamp, &active_runtime, &jobs_completed);
--
2.53.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Claude review: drm/v3d: Refcount v3d_stats
2026-03-05 23:25 ` [PATCH v2 3/6] drm/v3d: Refcount v3d_stats Maíra Canal
@ 2026-03-08 23:20 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 23:20 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the core change of the series. The design is sound: `v3d_stats` gets a `kref` and is heap-allocated via `v3d_stats_alloc()`.
The error handling in `v3d_open` is correct — the two-label approach (`err_sched` / `err_stats`) properly handles the case where stats were allocated but `drm_sched_entity_init` failed:
```c
err_sched:
v3d_stats_put(v3d_priv->stats[i]);
err_stats:
for (i--; i >= 0; i--) {
drm_sched_entity_destroy(&v3d_priv->sched_entity[i]);
v3d_stats_put(v3d_priv->stats[i]);
}
```
The `v3d_gem_init` error handling is also correctly reworked — previous bare `return -ENOMEM` / `return ret` paths that leaked stats are now routed through proper cleanup labels. The reordering that separates `drm_mm_takedown` from the DMA alloc failure path (`err_dma_alloc` vs `err_sched`) is correct.
One observation: `v3d_stats_release` and `v3d_stats_alloc` are placed in `v3d_sched.c` rather than, say, `v3d_drv.c` where `v3d_get_stats` lives. This is fine since `v3d_sched.c` is the primary consumer, but worth noting for future reference.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread