[PATCH v2 0/6] drm/v3d: Improve v3d_stats lifetime and locking

[PATCH v2 0/6] drm/v3d: Improve v3d_stats lifetime and locking

[Maíra Canal]

This series aims to improve the `struct v3d_stats` lifetime management.
The commit fa6a20c87470 ("drm/v3d: Address race-condition between per-fd
GPU stats and fd release") addressed a race-condition between the per-fd
GPU stats and the fd release by clearing `job->file_priv` before the V3D
per-fd structure is freed and assuring that `job->file_priv` exists
during the per-fd GPU stats updates.

However, this approach introduced a tricky file_priv NULL-ing dance with
the `queue_lock` spinlock. This series aims to address this issue in a
simpler way: converting `v3d_stats` from embedded structs to
heap-allocated, refcounted objects. This simplifies the code and leaves
`queue_lock` exclusively for job management.

The series also switches to raw seqcount helpers to resolve a lockdep
annotation issue, fixes missing error handling in drm_sched_entity_init(),
and removes the now-unnecessary dedicated fence_lock.

Best regards,
- Maíra

---
v1 -> v2:

- [All patches] Added Iago's R-b (Iago Toral)
- [3/6] s/kzalloc/kzalloc_obj
- [5/6] Add a comment explaining while the mutex is used in
        DRM_V3D_PARAM_CONTEXT_RESET_COUNTER (Iago Toral)
- Link to v1: https://lore.kernel.org/r/20260217-v3d-reset-locking-improv-v1-0-0db848016869@igalia.com

To: Melissa Wen <mwen@igalia.com>
To: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
To: Maxime Ripard <mripard@kernel.org>
Cc: kernel-dev@igalia.com
Cc: dri-devel@lists.freedesktop.org

---
Maíra Canal (2):
      drm/v3d: Handle error from drm_sched_entity_init()
      drm/v3d: Remove dedicated fence_lock

Tvrtko Ursulin (4):
      drm/v3d: Use raw seqcount helpers instead of fighting with lockdep
      drm/v3d: Refcount v3d_stats
      drm/v3d: Hold v3d_stats references in each job
      drm/v3d: Attach per-fd reset counters to v3d_stats

 drivers/gpu/drm/v3d/v3d_drv.c    |  57 ++++++++++-------
 drivers/gpu/drm/v3d/v3d_drv.h    |  46 +++++++++-----
 drivers/gpu/drm/v3d/v3d_fence.c  |   2 +-
 drivers/gpu/drm/v3d/v3d_gem.c    |  43 ++++++++-----
 drivers/gpu/drm/v3d/v3d_irq.c    |   2 +-
 drivers/gpu/drm/v3d/v3d_sched.c  | 133 +++++++++++++++------------------------
 drivers/gpu/drm/v3d/v3d_submit.c |   6 ++
 drivers/gpu/drm/v3d/v3d_sysfs.c  |   2 +-
 8 files changed, 151 insertions(+), 140 deletions(-)
---
base-commit: 21613f67ede11e495281b4a6dde72cd7db3ada4e
change-id: 20260215-v3d-reset-locking-improv-ffe16c35569b

[PATCH v2 1/6] drm/v3d: Handle error from drm_sched_entity_init()

[Maíra Canal]

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

Claude review: drm/v3d: Handle error from drm_sched_entity_init()

[Claude Code Review Bot]

Patch Review

Straightforward bugfix. The error unwinding correctly walks backwards destroying only the already-initialized entities:

```c
err_sched:
	for (i--; i >= 0; i--)
		drm_sched_entity_destroy(&v3d_priv->sched_entity[i]);
	kfree(v3d_priv);
	return ret;
```

Correct and clean. The `Fixes:` tag referencing the original driver commit is appropriate.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 2/6] drm/v3d: Use raw seqcount helpers instead of fighting with lockdep

[Maíra Canal]

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

Claude review: drm/v3d: Use raw seqcount helpers instead of fighting with lockdep

[Claude Code Review Bot]

Patch Review

The switch to `raw_write_seqcount_begin/end` and `raw_read_seqcount_begin` is justified by the commit message — jobs are serialized per queue, so there's no real race, and the lockdep annotations were producing false positives.

Minor nit: The read side changes `read_seqcount_begin` to `raw_read_seqcount_begin` but leaves `read_seqcount_retry` unchanged:

```c
	do {
		seq = raw_read_seqcount_begin(&stats->lock);
		...
	} while (read_seqcount_retry(&stats->lock, seq));
```

This is functionally correct since the lockdep annotation is on the `begin` side, not the `retry` side (which only has KCSAN annotations). But for consistency, you might consider whether pairing with `raw_read_seqcount_retry` (if available) would be cleaner.

The removal of the `IS_ENABLED(CONFIG_LOCKDEP)` conditional IRQ save/restore significantly simplifies the code. The comment in `v3d_drv.h` explaining why raw helpers are used is helpful.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 3/6] drm/v3d: Refcount v3d_stats

[Maíra Canal]

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

Claude review: drm/v3d: Refcount v3d_stats

[Claude Code Review Bot]

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

[PATCH v2 4/6] drm/v3d: Hold v3d_stats references in each job

[Maíra Canal]

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

Claude review: drm/v3d: Hold v3d_stats references in each job

[Claude Code Review Bot]

Patch Review

This is the key patch that eliminates the need for `queue_lock` in the stats update path. Each job now holds its own refcounted references to both client and global stats:

```c
job->client_stats = v3d_stats_get(v3d_priv->stats[queue]);
job->global_stats = v3d_stats_get(v3d->queue[queue].stats);
```

The critical simplification in `v3d_job_update_stats` is correct — the old code needed `queue_lock` and the `if (job->file_priv)` NULL check because stats were embedded in `v3d_file_priv` which could be freed. With refcounting, the stats outlive the fd:

```c
void
v3d_job_update_stats(struct v3d_job *job)
{
	u64 now = local_clock();

	preempt_disable();
	v3d_stats_update(job->client_stats, now);
	v3d_stats_update(job->global_stats, now);
	preempt_enable();
}
```

The refactoring of `v3d_job_start_stats` into `v3d_stats_start` + `v3d_job_start_stats` is clean and makes both functions independent of queue type, which is why the `enum v3d_queue` parameter can be dropped. All callers are updated consistently.

The `v3d_job_free` cleanup correctly puts both stats references before freeing the job. The placement after `v3d_perfmon_put` and before `kfree` is appropriate.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 5/6] drm/v3d: Attach per-fd reset counters to v3d_stats

[Maíra Canal]

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   | 24 ++++++++----------------
 drivers/gpu/drm/v3d/v3d_drv.h   | 14 ++++----------
 drivers/gpu/drm/v3d/v3d_sched.c |  9 ++-------
 3 files changed, 14 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 0f5e29f57fa59bfa2890de51b9ca78cda33d1edc..e5708b71738debaf3dd16010d50fb9e910a3918d 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -110,13 +110,17 @@ 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:
+		args->value = 0;
+		/* The mutex ensures the user gets a reliable snapshot from all
+		 * queues. Without it, a queue could trigger a reset while the
+		 * loop is iterating.
+		 */
 		mutex_lock(&v3d->reset_lock);
-		args->value = v3d_priv->reset_counter;
+		for (enum v3d_queue q = 0; q < V3D_MAX_QUEUES; q++)
+			args->value += atomic_read(&v3d_priv->stats[q]->reset_counter);
 		mutex_unlock(&v3d->reset_lock);
 		return 0;
 	default:
@@ -173,23 +177,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

Claude review: drm/v3d: Attach per-fd reset counters to v3d_stats

[Claude Code Review Bot]

Patch Review

This patch completes the refcounting story by moving the per-fd reset counter into `v3d_stats`. The use of `atomic_t` for both global and per-client reset counters is appropriate since they're now incremented from the reset path without the `reset_lock` held for the per-client counter:

```c
atomic_inc(&v3d->reset_counter);
atomic_inc(&job->client_stats->reset_counter);
```

The `DRM_V3D_PARAM_CONTEXT_RESET_COUNTER` implementation correctly sums across all queues under `reset_lock` to get a consistent snapshot:

```c
mutex_lock(&v3d->reset_lock);
for (enum v3d_queue q = 0; q < V3D_MAX_QUEUES; q++)
    args->value += atomic_read(&v3d_priv->stats[q]->reset_counter);
mutex_unlock(&v3d->reset_lock);
```

The comment explaining why the mutex is still needed here is good.

The removal of the `file_priv` NULL-ing dance in `v3d_postclose` is the culmination of the series and is safe because: (1) `drm_sched_entity_destroy` waits for all entity jobs to complete, so no active job can reference this fd's entity afterward; and (2) the timeout/reset path now uses `job->client_stats` instead of `job->file_priv`.

Note: `DRM_V3D_PARAM_GLOBAL_RESET_COUNTER` no longer takes `reset_lock` since it's a single atomic read. This is correct.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 6/6] drm/v3d: Remove dedicated fence_lock

[Maíra Canal]

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

Claude review: drm/v3d: Remove dedicated fence_lock

[Claude Code Review Bot]

Patch Review

With `queue_lock` no longer taken from the IRQ handler for stats updates (the original reason for the split), merging `fence_lock` back is a reasonable simplification:

```c
dma_fence_init(&fence->base, &v3d_fence_ops, &queue->queue_lock,
	       queue->fence_context, fence->seqno);
```

The reasoning in the commit message is sound. The `fence_lock` was introduced specifically because `v3d_job_update_stats` was taking `queue_lock` in the IRQ handler alongside DMA fence signaling, which caused PREEMPT_RT issues. That usage is now gone.

One thing worth verifying: after this merge, `queue_lock` is used from process context with `spin_lock_irqsave` (e.g., in `v3d_bin_job_run`) and from IRQ context via `dma_fence_signal`. Since both paths use IRQ-safe locking, this is correct for non-RT kernels. On PREEMPT_RT, the situation is the same as the original `fence_lock` (both were `spinlock_t`), so no regression there.

Clean and correct patch.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/v3d: Improve v3d_stats lifetime and locking

[Claude Code Review Bot]

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-09T09:20:14.843131

---

This is a well-structured 6-patch series that improves `v3d_stats` lifetime management by converting embedded stats structs to heap-allocated, refcounted objects. The core motivation is sound: eliminating the tricky `file_priv` NULL-ing dance that was needed to handle the race between per-fd GPU stats updates and fd release. The refcounting approach is clean, the error handling paths are correct, and the series progressively simplifies the locking scheme. The patches are logically ordered and each builds on the previous one.

No blocking issues found. A few minor observations below.

---
Generated by Claude Code Patch Reviewer

[PATCH v3 0/6] drm/v3d: Improve v3d_stats lifetime and locking

[Maíra Canal]

This series aims to improve the `struct v3d_stats` lifetime management.
The commit fa6a20c87470 ("drm/v3d: Address race-condition between per-fd
GPU stats and fd release") addressed a race-condition between the per-fd
GPU stats and the fd release by clearing `job->file_priv` before the V3D
per-fd structure is freed and assuring that `job->file_priv` exists
during the per-fd GPU stats updates.

However, this approach introduced a tricky file_priv NULL-ing dance with
the `queue_lock` spinlock. This series aims to address this issue in a
simpler way: converting `v3d_stats` from embedded structs to
heap-allocated, refcounted objects. This simplifies the code and leaves
`queue_lock` exclusively for job management.

The series also switches to raw seqcount helpers to resolve a lockdep
annotation issue, fixes missing error handling in drm_sched_entity_init(),
and removes the now-unnecessary dedicated fence_lock.

Best regards,
- Maíra

---
v1 -> v2:

- [All patches] Added Iago's R-b (Iago Toral)
- [3/6] s/kzalloc/kzalloc_obj
- [5/6] Add a comment explaining while the mutex is used in
        DRM_V3D_PARAM_CONTEXT_RESET_COUNTER (Iago Toral)
- Link to v1: https://lore.kernel.org/r/20260217-v3d-reset-locking-improv-v1-0-0db848016869@igalia.com

v2 -> v3:

- [5/6] Remove mutex from DRM_V3D_PARAM_CONTEXT_RESET_COUNTER (Tvrtko Ursulin)
- Link to v2: https://lore.kernel.org/r/20260305-v3d-reset-locking-improv-v2-0-fd53c91f7726@igalia.com

---
Maíra Canal (2):
      drm/v3d: Handle error from drm_sched_entity_init()
      drm/v3d: Remove dedicated fence_lock

Tvrtko Ursulin (4):
      drm/v3d: Use raw seqcount helpers instead of fighting with lockdep
      drm/v3d: Refcount v3d_stats
      drm/v3d: Hold v3d_stats references in each job
      drm/v3d: Attach per-fd reset counters to v3d_stats

 drivers/gpu/drm/v3d/v3d_drv.c    |  55 ++++++++--------
 drivers/gpu/drm/v3d/v3d_drv.h    |  46 +++++++++-----
 drivers/gpu/drm/v3d/v3d_fence.c  |   2 +-
 drivers/gpu/drm/v3d/v3d_gem.c    |  43 ++++++++-----
 drivers/gpu/drm/v3d/v3d_irq.c    |   2 +-
 drivers/gpu/drm/v3d/v3d_sched.c  | 133 +++++++++++++++------------------------
 drivers/gpu/drm/v3d/v3d_submit.c |   6 ++
 drivers/gpu/drm/v3d/v3d_sysfs.c  |   2 +-
 8 files changed, 147 insertions(+), 142 deletions(-)
---
base-commit: e597a809a2b97e927060ba182f58eb3e6101bc70
change-id: 20260215-v3d-reset-locking-improv-ffe16c35569b

[PATCH v3 3/6] drm/v3d: Refcount v3d_stats

[Maíra Canal]

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

Claude review: drm/v3d: Refcount v3d_stats

[Claude Code Review Bot]

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