[PATCH v2 0/4] drm/v3d: Fix GPU stats inconsistencies and race-condition

[PATCH v2 0/4] drm/v3d: Fix GPU stats inconsistencies and race-condition

[Maíra Canal]

The first version of this series had the intention to fix two major
issues with the GPU stats:

1. We were incrementing `enabled_ns` twice by the end of each job.
2. There is a race-condition between the IRQ handler and the users

The first of the issues was already addressed and the fix was applied to
drm-misc-fixes. Now, what is left, addresses the second issue.

Apart from addressing this issue, this series improved the GPU stats
code as a whole. We reduced code repetition as a whole, creating functions
to start and update the GPU stats. This will likely reduce the odds of
issue #1 happen again.

Best Regards,
- Maíra

v1 -> v2: https://lore.kernel.org/dri-devel/20240403203517.731876-1-mcanal@igalia.com/T/

	* As the first patch was a bugfix, it was pushed to drm-misc-fixes.
	* [1/4]: Add Chema Casanova's R-b
	* [2/4]: s/jobs_sent/jobs_completed and add the reasoning in the commit
		message (Chema Casanova)
	* [2/4]: Add Chema Casanova's and Tvrtko Ursulin's R-b
	* [3/4]: Call `local_clock()` only once, by adding a new parameter to the
		`v3d_stats_update` function (Chema Casanova)
	* [4/4]: Move new line to the correct patch (2/4) (Tvrtko Ursulin)
	* [4/4]: Use `seqcount_t` as locking primitive instead of a `rw_lock` (Tvrtko Ursulin)

Maíra Canal (4):
  drm/v3d: Create two functions to update all GPU stats variables
  drm/v3d: Create a struct to store the GPU stats
  drm/v3d: Create function to update a set of GPU stats
  drm/v3d: Fix race-condition between sysfs/fdinfo and interrupt handler

 drivers/gpu/drm/v3d/v3d_drv.c   | 19 +++----
 drivers/gpu/drm/v3d/v3d_drv.h   | 40 +++++++++++---
 drivers/gpu/drm/v3d/v3d_gem.c   |  9 ++--
 drivers/gpu/drm/v3d/v3d_irq.c   | 48 ++---------------
 drivers/gpu/drm/v3d/v3d_sched.c | 94 +++++++++++++++++----------------
 drivers/gpu/drm/v3d/v3d_sysfs.c | 13 ++---
 6 files changed, 105 insertions(+), 118 deletions(-)

-- 
2.44.0

[PATCH v2 1/4] drm/v3d: Create two functions to update all GPU stats variables

[Maíra Canal]

Currently, we manually perform all operations to update the GPU stats
variables. Apart from the code repetition, this is very prone to errors,
as we can see on commit 35f4f8c9fc97 ("drm/v3d: Don't increment
`enabled_ns` twice").

Therefore, create two functions to manage updating all GPU stats
variables. Now, the jobs only need to call for `v3d_job_update_stats()`
when the job is done and `v3d_job_start_stats()` when starting the job.

Co-developed-by: Tvrtko Ursulin <tursulin@igalia.com>
Signed-off-by: Tvrtko Ursulin <tursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.h   |  1 +
 drivers/gpu/drm/v3d/v3d_irq.c   | 48 ++------------------
 drivers/gpu/drm/v3d/v3d_sched.c | 80 +++++++++++++++------------------
 3 files changed, 40 insertions(+), 89 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 1950c723dde1..ee3545226d7f 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -543,6 +543,7 @@ void v3d_mmu_insert_ptes(struct v3d_bo *bo);
 void v3d_mmu_remove_ptes(struct v3d_bo *bo);
 
 /* v3d_sched.c */
+void v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue);
 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 ce6b2fb341d1..d469bda52c1a 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -102,18 +102,8 @@ v3d_irq(int irq, void *arg)
 	if (intsts & V3D_INT_FLDONE) {
 		struct v3d_fence *fence =
 			to_v3d_fence(v3d->bin_job->base.irq_fence);
-		struct v3d_file_priv *file = v3d->bin_job->base.file->driver_priv;
-		u64 runtime = local_clock() - file->start_ns[V3D_BIN];
-
-		file->jobs_sent[V3D_BIN]++;
-		v3d->queue[V3D_BIN].jobs_sent++;
-
-		file->start_ns[V3D_BIN] = 0;
-		v3d->queue[V3D_BIN].start_ns = 0;
-
-		file->enabled_ns[V3D_BIN] += runtime;
-		v3d->queue[V3D_BIN].enabled_ns += runtime;
 
+		v3d_job_update_stats(&v3d->bin_job->base, V3D_BIN);
 		trace_v3d_bcl_irq(&v3d->drm, fence->seqno);
 		dma_fence_signal(&fence->base);
 		status = IRQ_HANDLED;
@@ -122,18 +112,8 @@ v3d_irq(int irq, void *arg)
 	if (intsts & V3D_INT_FRDONE) {
 		struct v3d_fence *fence =
 			to_v3d_fence(v3d->render_job->base.irq_fence);
-		struct v3d_file_priv *file = v3d->render_job->base.file->driver_priv;
-		u64 runtime = local_clock() - file->start_ns[V3D_RENDER];
-
-		file->jobs_sent[V3D_RENDER]++;
-		v3d->queue[V3D_RENDER].jobs_sent++;
-
-		file->start_ns[V3D_RENDER] = 0;
-		v3d->queue[V3D_RENDER].start_ns = 0;
-
-		file->enabled_ns[V3D_RENDER] += runtime;
-		v3d->queue[V3D_RENDER].enabled_ns += runtime;
 
+		v3d_job_update_stats(&v3d->render_job->base, V3D_RENDER);
 		trace_v3d_rcl_irq(&v3d->drm, fence->seqno);
 		dma_fence_signal(&fence->base);
 		status = IRQ_HANDLED;
@@ -142,18 +122,8 @@ v3d_irq(int irq, void *arg)
 	if (intsts & V3D_INT_CSDDONE(v3d->ver)) {
 		struct v3d_fence *fence =
 			to_v3d_fence(v3d->csd_job->base.irq_fence);
-		struct v3d_file_priv *file = v3d->csd_job->base.file->driver_priv;
-		u64 runtime = local_clock() - file->start_ns[V3D_CSD];
-
-		file->jobs_sent[V3D_CSD]++;
-		v3d->queue[V3D_CSD].jobs_sent++;
-
-		file->start_ns[V3D_CSD] = 0;
-		v3d->queue[V3D_CSD].start_ns = 0;
-
-		file->enabled_ns[V3D_CSD] += runtime;
-		v3d->queue[V3D_CSD].enabled_ns += runtime;
 
+		v3d_job_update_stats(&v3d->csd_job->base, V3D_CSD);
 		trace_v3d_csd_irq(&v3d->drm, fence->seqno);
 		dma_fence_signal(&fence->base);
 		status = IRQ_HANDLED;
@@ -189,18 +159,8 @@ v3d_hub_irq(int irq, void *arg)
 	if (intsts & V3D_HUB_INT_TFUC) {
 		struct v3d_fence *fence =
 			to_v3d_fence(v3d->tfu_job->base.irq_fence);
-		struct v3d_file_priv *file = v3d->tfu_job->base.file->driver_priv;
-		u64 runtime = local_clock() - file->start_ns[V3D_TFU];
-
-		file->jobs_sent[V3D_TFU]++;
-		v3d->queue[V3D_TFU].jobs_sent++;
-
-		file->start_ns[V3D_TFU] = 0;
-		v3d->queue[V3D_TFU].start_ns = 0;
-
-		file->enabled_ns[V3D_TFU] += runtime;
-		v3d->queue[V3D_TFU].enabled_ns += runtime;
 
+		v3d_job_update_stats(&v3d->tfu_job->base, V3D_TFU);
 		trace_v3d_tfu_irq(&v3d->drm, fence->seqno);
 		dma_fence_signal(&fence->base);
 		status = IRQ_HANDLED;
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 54015ad765c7..8ca61bcd4b1c 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -105,11 +105,37 @@ v3d_switch_perfmon(struct v3d_dev *v3d, struct v3d_job *job)
 		v3d_perfmon_start(v3d, job->perfmon);
 }
 
+static void
+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->driver_priv;
+	u64 now = local_clock();
+
+	file->start_ns[queue] = now;
+	v3d->queue[queue].start_ns = now;
+}
+
+void
+v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue)
+{
+	struct v3d_dev *v3d = job->v3d;
+	struct v3d_file_priv *file = job->file->driver_priv;
+	u64 now = local_clock();
+
+	file->enabled_ns[queue] += now - file->start_ns[queue];
+	file->jobs_sent[queue]++;
+	file->start_ns[queue] = 0;
+
+	v3d->queue[queue].enabled_ns += now - v3d->queue[queue].start_ns;
+	v3d->queue[queue].jobs_sent++;
+	v3d->queue[queue].start_ns = 0;
+}
+
 static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
 {
 	struct v3d_bin_job *job = to_bin_job(sched_job);
 	struct v3d_dev *v3d = job->base.v3d;
-	struct v3d_file_priv *file = job->base.file->driver_priv;
 	struct drm_device *dev = &v3d->drm;
 	struct dma_fence *fence;
 	unsigned long irqflags;
@@ -141,9 +167,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);
 
-	file->start_ns[V3D_BIN] = local_clock();
-	v3d->queue[V3D_BIN].start_ns = file->start_ns[V3D_BIN];
-
+	v3d_job_start_stats(&job->base, V3D_BIN);
 	v3d_switch_perfmon(v3d, &job->base);
 
 	/* Set the current and end address of the control list.
@@ -168,7 +192,6 @@ static struct dma_fence *v3d_render_job_run(struct drm_sched_job *sched_job)
 {
 	struct v3d_render_job *job = to_render_job(sched_job);
 	struct v3d_dev *v3d = job->base.v3d;
-	struct v3d_file_priv *file = job->base.file->driver_priv;
 	struct drm_device *dev = &v3d->drm;
 	struct dma_fence *fence;
 
@@ -196,9 +219,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);
 
-	file->start_ns[V3D_RENDER] = local_clock();
-	v3d->queue[V3D_RENDER].start_ns = file->start_ns[V3D_RENDER];
-
+	v3d_job_start_stats(&job->base, V3D_RENDER);
 	v3d_switch_perfmon(v3d, &job->base);
 
 	/* XXX: Set the QCFG */
@@ -217,7 +238,6 @@ v3d_tfu_job_run(struct drm_sched_job *sched_job)
 {
 	struct v3d_tfu_job *job = to_tfu_job(sched_job);
 	struct v3d_dev *v3d = job->base.v3d;
-	struct v3d_file_priv *file = job->base.file->driver_priv;
 	struct drm_device *dev = &v3d->drm;
 	struct dma_fence *fence;
 
@@ -232,8 +252,7 @@ v3d_tfu_job_run(struct drm_sched_job *sched_job)
 
 	trace_v3d_submit_tfu(dev, to_v3d_fence(fence)->seqno);
 
-	file->start_ns[V3D_TFU] = local_clock();
-	v3d->queue[V3D_TFU].start_ns = file->start_ns[V3D_TFU];
+	v3d_job_start_stats(&job->base, V3D_TFU);
 
 	V3D_WRITE(V3D_TFU_IIA(v3d->ver), job->args.iia);
 	V3D_WRITE(V3D_TFU_IIS(v3d->ver), job->args.iis);
@@ -260,7 +279,6 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
 {
 	struct v3d_csd_job *job = to_csd_job(sched_job);
 	struct v3d_dev *v3d = job->base.v3d;
-	struct v3d_file_priv *file = job->base.file->driver_priv;
 	struct drm_device *dev = &v3d->drm;
 	struct dma_fence *fence;
 	int i, csd_cfg0_reg, csd_cfg_reg_count;
@@ -279,9 +297,7 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
 
 	trace_v3d_submit_csd(dev, to_v3d_fence(fence)->seqno);
 
-	file->start_ns[V3D_CSD] = local_clock();
-	v3d->queue[V3D_CSD].start_ns = file->start_ns[V3D_CSD];
-
+	v3d_job_start_stats(&job->base, V3D_CSD);
 	v3d_switch_perfmon(v3d, &job->base);
 
 	csd_cfg0_reg = V3D_CSD_QUEUED_CFG0(v3d->ver);
@@ -530,8 +546,6 @@ v3d_cpu_job_run(struct drm_sched_job *sched_job)
 {
 	struct v3d_cpu_job *job = to_cpu_job(sched_job);
 	struct v3d_dev *v3d = job->base.v3d;
-	struct v3d_file_priv *file = job->base.file->driver_priv;
-	u64 runtime;
 
 	v3d->cpu_job = job;
 
@@ -540,25 +554,13 @@ v3d_cpu_job_run(struct drm_sched_job *sched_job)
 		return NULL;
 	}
 
-	file->start_ns[V3D_CPU] = local_clock();
-	v3d->queue[V3D_CPU].start_ns = file->start_ns[V3D_CPU];
-
+	v3d_job_start_stats(&job->base, V3D_CPU);
 	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);
-
-	runtime = local_clock() - file->start_ns[V3D_CPU];
-
-	file->enabled_ns[V3D_CPU] += runtime;
-	v3d->queue[V3D_CPU].enabled_ns += runtime;
-
-	file->jobs_sent[V3D_CPU]++;
-	v3d->queue[V3D_CPU].jobs_sent++;
-
-	file->start_ns[V3D_CPU] = 0;
-	v3d->queue[V3D_CPU].start_ns = 0;
+	v3d_job_update_stats(&job->base, V3D_CPU);
 
 	return NULL;
 }
@@ -568,24 +570,12 @@ 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;
-	struct v3d_file_priv *file = job->file->driver_priv;
-	u64 runtime;
 
-	file->start_ns[V3D_CACHE_CLEAN] = local_clock();
-	v3d->queue[V3D_CACHE_CLEAN].start_ns = file->start_ns[V3D_CACHE_CLEAN];
+	v3d_job_start_stats(job, V3D_CACHE_CLEAN);
 
 	v3d_clean_caches(v3d);
 
-	runtime = local_clock() - file->start_ns[V3D_CACHE_CLEAN];
-
-	file->enabled_ns[V3D_CACHE_CLEAN] += runtime;
-	v3d->queue[V3D_CACHE_CLEAN].enabled_ns += runtime;
-
-	file->jobs_sent[V3D_CACHE_CLEAN]++;
-	v3d->queue[V3D_CACHE_CLEAN].jobs_sent++;
-
-	file->start_ns[V3D_CACHE_CLEAN] = 0;
-	v3d->queue[V3D_CACHE_CLEAN].start_ns = 0;
+	v3d_job_update_stats(job, V3D_CACHE_CLEAN);
 
 	return NULL;
 }
-- 
2.44.0

[PATCH v2 2/4] drm/v3d: Create a struct to store the GPU stats

[Maíra Canal]

This will make it easier to instantiate the GPU stats variables and it
will create a structure where we can store all the variables that refer
to GPU stats.

Note that, when we created the struct `v3d_stats`, we renamed
`jobs_sent` to `jobs_completed`. This better express the semantics of
the variable, as we are only accounting jobs that have been completed.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.c   | 15 +++++++--------
 drivers/gpu/drm/v3d/v3d_drv.h   | 18 ++++++++++--------
 drivers/gpu/drm/v3d/v3d_gem.c   |  4 +---
 drivers/gpu/drm/v3d/v3d_sched.c | 20 ++++++++++++--------
 drivers/gpu/drm/v3d/v3d_sysfs.c | 10 ++++++----
 5 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 3debf37e7d9b..52e3ba9df46f 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -115,14 +115,12 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
 	v3d_priv->v3d = v3d;
 
 	for (i = 0; i < V3D_MAX_QUEUES; i++) {
-		v3d_priv->enabled_ns[i] = 0;
-		v3d_priv->start_ns[i] = 0;
-		v3d_priv->jobs_sent[i] = 0;
-
 		sched = &v3d->queue[i].sched;
 		drm_sched_entity_init(&v3d_priv->sched_entity[i],
 				      DRM_SCHED_PRIORITY_NORMAL, &sched,
 				      1, NULL);
+
+		memset(&v3d_priv->stats[i], 0, sizeof(v3d_priv->stats[i]));
 	}
 
 	v3d_perfmon_open_file(v3d_priv);
@@ -151,20 +149,21 @@ 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];
+
 		/* Note that, in case of a GPU reset, the time spent during an
 		 * attempt of executing the job is not computed in the runtime.
 		 */
 		drm_printf(p, "drm-engine-%s: \t%llu ns\n",
 			   v3d_queue_to_string(queue),
-			   file_priv->start_ns[queue] ? file_priv->enabled_ns[queue]
-						      + timestamp - file_priv->start_ns[queue]
-						      : file_priv->enabled_ns[queue]);
+			   stats->start_ns ? stats->enabled_ns + timestamp - stats->start_ns
+					   : stats->enabled_ns);
 
 		/* Note that we only count jobs that completed. Therefore, jobs
 		 * that were resubmitted due to a GPU reset are not computed.
 		 */
 		drm_printf(p, "v3d-jobs-%s: \t%llu jobs\n",
-			   v3d_queue_to_string(queue), file_priv->jobs_sent[queue]);
+			   v3d_queue_to_string(queue), stats->jobs_completed);
 	}
 }
 
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index ee3545226d7f..5a198924d568 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -36,15 +36,20 @@ static inline char *v3d_queue_to_string(enum v3d_queue queue)
 	return "UNKNOWN";
 }
 
+struct v3d_stats {
+	u64 start_ns;
+	u64 enabled_ns;
+	u64 jobs_completed;
+};
+
 struct v3d_queue_state {
 	struct drm_gpu_scheduler sched;
 
 	u64 fence_context;
 	u64 emit_seqno;
 
-	u64 start_ns;
-	u64 enabled_ns;
-	u64 jobs_sent;
+	/* Stores the GPU stats for this queue in the global context. */
+	struct v3d_stats stats;
 };
 
 /* Performance monitor object. The perform lifetime is controlled by userspace
@@ -188,11 +193,8 @@ struct v3d_file_priv {
 
 	struct drm_sched_entity sched_entity[V3D_MAX_QUEUES];
 
-	u64 start_ns[V3D_MAX_QUEUES];
-
-	u64 enabled_ns[V3D_MAX_QUEUES];
-
-	u64 jobs_sent[V3D_MAX_QUEUES];
+	/* Stores the GPU stats for a specific queue for this fd. */
+	struct v3d_stats stats[V3D_MAX_QUEUES];
 };
 
 struct v3d_bo {
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index afc565078c78..d14589d3ae6c 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -248,9 +248,7 @@ v3d_gem_init(struct drm_device *dev)
 
 	for (i = 0; i < V3D_MAX_QUEUES; i++) {
 		v3d->queue[i].fence_context = dma_fence_context_alloc(1);
-		v3d->queue[i].start_ns = 0;
-		v3d->queue[i].enabled_ns = 0;
-		v3d->queue[i].jobs_sent = 0;
+		memset(&v3d->queue[i].stats, 0, sizeof(v3d->queue[i].stats));
 	}
 
 	spin_lock_init(&v3d->mm_lock);
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 8ca61bcd4b1c..b6b5542c3fcf 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -110,10 +110,12 @@ 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->driver_priv;
+	struct v3d_stats *global_stats = &v3d->queue[queue].stats;
+	struct v3d_stats *local_stats = &file->stats[queue];
 	u64 now = local_clock();
 
-	file->start_ns[queue] = now;
-	v3d->queue[queue].start_ns = now;
+	local_stats->start_ns = now;
+	global_stats->start_ns = now;
 }
 
 void
@@ -121,15 +123,17 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue)
 {
 	struct v3d_dev *v3d = job->v3d;
 	struct v3d_file_priv *file = job->file->driver_priv;
+	struct v3d_stats *global_stats = &v3d->queue[queue].stats;
+	struct v3d_stats *local_stats = &file->stats[queue];
 	u64 now = local_clock();
 
-	file->enabled_ns[queue] += now - file->start_ns[queue];
-	file->jobs_sent[queue]++;
-	file->start_ns[queue] = 0;
+	local_stats->enabled_ns += now - local_stats->start_ns;
+	local_stats->jobs_completed++;
+	local_stats->start_ns = 0;
 
-	v3d->queue[queue].enabled_ns += now - v3d->queue[queue].start_ns;
-	v3d->queue[queue].jobs_sent++;
-	v3d->queue[queue].start_ns = 0;
+	global_stats->enabled_ns += now - global_stats->start_ns;
+	global_stats->jobs_completed++;
+	global_stats->start_ns = 0;
 }
 
 static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
diff --git a/drivers/gpu/drm/v3d/v3d_sysfs.c b/drivers/gpu/drm/v3d/v3d_sysfs.c
index d106845ba890..6a8e7acc8b82 100644
--- a/drivers/gpu/drm/v3d/v3d_sysfs.c
+++ b/drivers/gpu/drm/v3d/v3d_sysfs.c
@@ -21,8 +21,10 @@ 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++) {
-		if (v3d->queue[queue].start_ns)
-			active_runtime = timestamp - v3d->queue[queue].start_ns;
+		struct v3d_stats *stats = &v3d->queue[queue].stats;
+
+		if (stats->start_ns)
+			active_runtime = timestamp - stats->start_ns;
 		else
 			active_runtime = 0;
 
@@ -39,8 +41,8 @@ gpu_stats_show(struct device *dev, struct device_attribute *attr, char *buf)
 		len += sysfs_emit_at(buf, len, "%s\t%llu\t%llu\t%llu\n",
 				     v3d_queue_to_string(queue),
 				     timestamp,
-				     v3d->queue[queue].jobs_sent,
-				     v3d->queue[queue].enabled_ns + active_runtime);
+				     stats->jobs_completed,
+				     stats->enabled_ns + active_runtime);
 	}
 
 	return len;
-- 
2.44.0

[PATCH v2 3/4] drm/v3d: Create function to update a set of GPU stats

[Maíra Canal]

Given a set of GPU stats, that is, a `struct v3d_stats` related to a
queue in a given context, create a function that can update this set
of GPU stats.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Reviewed-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_sched.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index b6b5542c3fcf..b9614944931c 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -118,6 +118,14 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
 	global_stats->start_ns = now;
 }
 
+static void
+v3d_stats_update(struct v3d_stats *stats, u64 now)
+{
+	stats->enabled_ns += now - stats->start_ns;
+	stats->jobs_completed++;
+	stats->start_ns = 0;
+}
+
 void
 v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue)
 {
@@ -127,13 +135,8 @@ v3d_job_update_stats(struct v3d_job *job, enum v3d_queue queue)
 	struct v3d_stats *local_stats = &file->stats[queue];
 	u64 now = local_clock();
 
-	local_stats->enabled_ns += now - local_stats->start_ns;
-	local_stats->jobs_completed++;
-	local_stats->start_ns = 0;
-
-	global_stats->enabled_ns += now - global_stats->start_ns;
-	global_stats->jobs_completed++;
-	global_stats->start_ns = 0;
+	v3d_stats_update(local_stats, now);
+	v3d_stats_update(global_stats, now);
 }
 
 static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
-- 
2.44.0

[PATCH v2 4/4] drm/v3d: Fix race-condition between sysfs/fdinfo and interrupt handler

[Maíra Canal]

In V3D, the conclusion of a job is indicated by a IRQ. When a job
finishes, then we update the local and the global GPU stats of that
queue. But, while the GPU stats are being updated, a user might be
reading the stats from sysfs or fdinfo.

For example, on `gpu_stats_show()`, we could think about a scenario where
`v3d->queue[queue].start_ns != 0`, then an interruption happens, we update
the value of `v3d->queue[queue].start_ns` to 0, we come back to
`gpu_stats_show()` to calculate `active_runtime` and now,
`active_runtime = timestamp`.

In this simple example, the user would see a spike in the queue usage,
that didn't matches reality.

In order to address this issue properly, use a seqcount to protect read
and write sections of the code.

Fixes: 09a93cc4f7d1 ("drm/v3d: Implement show_fdinfo() callback for GPU usage stats")
Reported-by: Tvrtko Ursulin <tursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_drv.c   | 10 ++++++----
 drivers/gpu/drm/v3d/v3d_drv.h   | 21 +++++++++++++++++++++
 drivers/gpu/drm/v3d/v3d_gem.c   |  7 +++++--
 drivers/gpu/drm/v3d/v3d_sched.c |  7 +++++++
 drivers/gpu/drm/v3d/v3d_sysfs.c | 11 +++--------
 5 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 52e3ba9df46f..cf15fa142968 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -121,6 +121,7 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
 				      1, NULL);
 
 		memset(&v3d_priv->stats[i], 0, sizeof(v3d_priv->stats[i]));
+		seqcount_init(&v3d_priv->stats[i].lock);
 	}
 
 	v3d_perfmon_open_file(v3d_priv);
@@ -150,20 +151,21 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 
 	for (queue = 0; queue < V3D_MAX_QUEUES; queue++) {
 		struct v3d_stats *stats = &file_priv->stats[queue];
+		u64 active_runtime, jobs_completed;
+
+		v3d_get_stats(stats, timestamp, &active_runtime, &jobs_completed);
 
 		/* Note that, in case of a GPU reset, the time spent during an
 		 * attempt of executing the job is not computed in the runtime.
 		 */
 		drm_printf(p, "drm-engine-%s: \t%llu ns\n",
-			   v3d_queue_to_string(queue),
-			   stats->start_ns ? stats->enabled_ns + timestamp - stats->start_ns
-					   : stats->enabled_ns);
+			   v3d_queue_to_string(queue), active_runtime);
 
 		/* Note that we only count jobs that completed. Therefore, jobs
 		 * that were resubmitted due to a GPU reset are not computed.
 		 */
 		drm_printf(p, "v3d-jobs-%s: \t%llu jobs\n",
-			   v3d_queue_to_string(queue), stats->jobs_completed);
+			   v3d_queue_to_string(queue), jobs_completed);
 	}
 }
 
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 5a198924d568..5211df7c7317 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -40,8 +40,29 @@ struct v3d_stats {
 	u64 start_ns;
 	u64 enabled_ns;
 	u64 jobs_completed;
+
+	/*
+	 * 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.
+	 */
+	seqcount_t lock;
 };
 
+static inline void v3d_get_stats(const struct v3d_stats *stats, u64 timestamp,
+				 u64 *active_runtime, u64 *jobs_completed)
+{
+	unsigned int seq;
+
+	do {
+		seq = read_seqcount_begin(&stats->lock);
+		*active_runtime = stats->enabled_ns;
+		if (stats->start_ns)
+			*active_runtime += timestamp - stats->start_ns;
+		*jobs_completed = stats->jobs_completed;
+	} while (read_seqcount_retry(&stats->lock, seq));
+}
+
 struct v3d_queue_state {
 	struct drm_gpu_scheduler sched;
 
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index d14589d3ae6c..da8faf3b9011 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -247,8 +247,11 @@ v3d_gem_init(struct drm_device *dev)
 	int ret, i;
 
 	for (i = 0; i < V3D_MAX_QUEUES; i++) {
-		v3d->queue[i].fence_context = dma_fence_context_alloc(1);
-		memset(&v3d->queue[i].stats, 0, sizeof(v3d->queue[i].stats));
+		struct v3d_queue_state *queue = &v3d->queue[i];
+
+		queue->fence_context = dma_fence_context_alloc(1);
+		memset(&queue->stats, 0, sizeof(queue->stats));
+		seqcount_init(&queue->stats.lock);
 	}
 
 	spin_lock_init(&v3d->mm_lock);
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index b9614944931c..7cd8c335cd9b 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -114,16 +114,23 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
 	struct v3d_stats *local_stats = &file->stats[queue];
 	u64 now = local_clock();
 
+	write_seqcount_begin(&local_stats->lock);
 	local_stats->start_ns = now;
+	write_seqcount_end(&local_stats->lock);
+
+	write_seqcount_begin(&global_stats->lock);
 	global_stats->start_ns = now;
+	write_seqcount_end(&global_stats->lock);
 }
 
 static void
 v3d_stats_update(struct v3d_stats *stats, u64 now)
 {
+	write_seqcount_begin(&stats->lock);
 	stats->enabled_ns += now - stats->start_ns;
 	stats->jobs_completed++;
 	stats->start_ns = 0;
+	write_seqcount_end(&stats->lock);
 }
 
 void
diff --git a/drivers/gpu/drm/v3d/v3d_sysfs.c b/drivers/gpu/drm/v3d/v3d_sysfs.c
index 6a8e7acc8b82..d610e355964f 100644
--- a/drivers/gpu/drm/v3d/v3d_sysfs.c
+++ b/drivers/gpu/drm/v3d/v3d_sysfs.c
@@ -15,18 +15,15 @@ gpu_stats_show(struct device *dev, struct device_attribute *attr, char *buf)
 	struct v3d_dev *v3d = to_v3d_dev(drm);
 	enum v3d_queue queue;
 	u64 timestamp = local_clock();
-	u64 active_runtime;
 	ssize_t len = 0;
 
 	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;
+		u64 active_runtime, jobs_completed;
 
-		if (stats->start_ns)
-			active_runtime = timestamp - stats->start_ns;
-		else
-			active_runtime = 0;
+		v3d_get_stats(stats, timestamp, &active_runtime, &jobs_completed);
 
 		/* Each line will display the queue name, timestamp, the number
 		 * of jobs sent to that queue and the runtime, as can be seem here:
@@ -40,9 +37,7 @@ gpu_stats_show(struct device *dev, struct device_attribute *attr, char *buf)
 		 */
 		len += sysfs_emit_at(buf, len, "%s\t%llu\t%llu\t%llu\n",
 				     v3d_queue_to_string(queue),
-				     timestamp,
-				     stats->jobs_completed,
-				     stats->enabled_ns + active_runtime);
+				     timestamp, jobs_completed, active_runtime);
 	}
 
 	return len;
-- 
2.44.0

Re: [PATCH v2 4/4] drm/v3d: Fix race-condition between sysfs/fdinfo and interrupt handler

[Tvrtko Ursulin]

On 17/04/2024 01:53, Maíra Canal wrote:
> In V3D, the conclusion of a job is indicated by a IRQ. When a job
> finishes, then we update the local and the global GPU stats of that
> queue. But, while the GPU stats are being updated, a user might be
> reading the stats from sysfs or fdinfo.
> 
> For example, on `gpu_stats_show()`, we could think about a scenario where
> `v3d->queue[queue].start_ns != 0`, then an interruption happens, we update

interrupt

> the value of `v3d->queue[queue].start_ns` to 0, we come back to
> `gpu_stats_show()` to calculate `active_runtime` and now,
> `active_runtime = timestamp`.
> 
> In this simple example, the user would see a spike in the queue usage,
> that didn't matches reality.

match

> In order to address this issue properly, use a seqcount to protect read
> and write sections of the code.
> 
> Fixes: 09a93cc4f7d1 ("drm/v3d: Implement show_fdinfo() callback for GPU usage stats")
> Reported-by: Tvrtko Ursulin <tursulin@igalia.com>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   drivers/gpu/drm/v3d/v3d_drv.c   | 10 ++++++----
>   drivers/gpu/drm/v3d/v3d_drv.h   | 21 +++++++++++++++++++++
>   drivers/gpu/drm/v3d/v3d_gem.c   |  7 +++++--
>   drivers/gpu/drm/v3d/v3d_sched.c |  7 +++++++
>   drivers/gpu/drm/v3d/v3d_sysfs.c | 11 +++--------
>   5 files changed, 42 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> index 52e3ba9df46f..cf15fa142968 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.c
> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> @@ -121,6 +121,7 @@ v3d_open(struct drm_device *dev, struct drm_file *file)
>   				      1, NULL);
>   
>   		memset(&v3d_priv->stats[i], 0, sizeof(v3d_priv->stats[i]));
> +		seqcount_init(&v3d_priv->stats[i].lock);
>   	}
>   
>   	v3d_perfmon_open_file(v3d_priv);
> @@ -150,20 +151,21 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>   
>   	for (queue = 0; queue < V3D_MAX_QUEUES; queue++) {
>   		struct v3d_stats *stats = &file_priv->stats[queue];
> +		u64 active_runtime, jobs_completed;
> +
> +		v3d_get_stats(stats, timestamp, &active_runtime, &jobs_completed);
>   
>   		/* Note that, in case of a GPU reset, the time spent during an
>   		 * attempt of executing the job is not computed in the runtime.
>   		 */
>   		drm_printf(p, "drm-engine-%s: \t%llu ns\n",
> -			   v3d_queue_to_string(queue),
> -			   stats->start_ns ? stats->enabled_ns + timestamp - stats->start_ns
> -					   : stats->enabled_ns);
> +			   v3d_queue_to_string(queue), active_runtime);
>   
>   		/* Note that we only count jobs that completed. Therefore, jobs
>   		 * that were resubmitted due to a GPU reset are not computed.
>   		 */
>   		drm_printf(p, "v3d-jobs-%s: \t%llu jobs\n",
> -			   v3d_queue_to_string(queue), stats->jobs_completed);
> +			   v3d_queue_to_string(queue), jobs_completed);
>   	}
>   }
>   
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> index 5a198924d568..5211df7c7317 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -40,8 +40,29 @@ struct v3d_stats {
>   	u64 start_ns;
>   	u64 enabled_ns;
>   	u64 jobs_completed;
> +
> +	/*
> +	 * 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.
> +	 */
> +	seqcount_t lock;
>   };
>   
> +static inline void v3d_get_stats(const struct v3d_stats *stats, u64 timestamp,
> +				 u64 *active_runtime, u64 *jobs_completed)
> +{
> +	unsigned int seq;
> +
> +	do {
> +		seq = read_seqcount_begin(&stats->lock);
> +		*active_runtime = stats->enabled_ns;
> +		if (stats->start_ns)
> +			*active_runtime += timestamp - stats->start_ns;
> +		*jobs_completed = stats->jobs_completed;
> +	} while (read_seqcount_retry(&stats->lock, seq));
> +}

Patch reads clean and obviously correct to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

The only possible discussion point I see is whether v3d_get_stats could 
have been introduced first to avoid mixing pure refactors with 
functionality, and whether it deserves to be in a header, or could be a 
function call in v3d_drv.c just as well. No strong opinion from me, 
since it is your driver your preference.

Regards,

Tvrtko

> +
>   struct v3d_queue_state {
>   	struct drm_gpu_scheduler sched;
>   
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index d14589d3ae6c..da8faf3b9011 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -247,8 +247,11 @@ v3d_gem_init(struct drm_device *dev)
>   	int ret, i;
>   
>   	for (i = 0; i < V3D_MAX_QUEUES; i++) {
> -		v3d->queue[i].fence_context = dma_fence_context_alloc(1);
> -		memset(&v3d->queue[i].stats, 0, sizeof(v3d->queue[i].stats));
> +		struct v3d_queue_state *queue = &v3d->queue[i];
> +
> +		queue->fence_context = dma_fence_context_alloc(1);
> +		memset(&queue->stats, 0, sizeof(queue->stats));
> +		seqcount_init(&queue->stats.lock);
>   	}
>   
>   	spin_lock_init(&v3d->mm_lock);
> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index b9614944931c..7cd8c335cd9b 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -114,16 +114,23 @@ v3d_job_start_stats(struct v3d_job *job, enum v3d_queue queue)
>   	struct v3d_stats *local_stats = &file->stats[queue];
>   	u64 now = local_clock();
>   
> +	write_seqcount_begin(&local_stats->lock);
>   	local_stats->start_ns = now;
> +	write_seqcount_end(&local_stats->lock);
> +
> +	write_seqcount_begin(&global_stats->lock);
>   	global_stats->start_ns = now;
> +	write_seqcount_end(&global_stats->lock);
>   }
>   
>   static void
>   v3d_stats_update(struct v3d_stats *stats, u64 now)
>   {
> +	write_seqcount_begin(&stats->lock);
>   	stats->enabled_ns += now - stats->start_ns;
>   	stats->jobs_completed++;
>   	stats->start_ns = 0;
> +	write_seqcount_end(&stats->lock);
>   }
>   
>   void
> diff --git a/drivers/gpu/drm/v3d/v3d_sysfs.c b/drivers/gpu/drm/v3d/v3d_sysfs.c
> index 6a8e7acc8b82..d610e355964f 100644
> --- a/drivers/gpu/drm/v3d/v3d_sysfs.c
> +++ b/drivers/gpu/drm/v3d/v3d_sysfs.c
> @@ -15,18 +15,15 @@ gpu_stats_show(struct device *dev, struct device_attribute *attr, char *buf)
>   	struct v3d_dev *v3d = to_v3d_dev(drm);
>   	enum v3d_queue queue;
>   	u64 timestamp = local_clock();
> -	u64 active_runtime;
>   	ssize_t len = 0;
>   
>   	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;
> +		u64 active_runtime, jobs_completed;
>   
> -		if (stats->start_ns)
> -			active_runtime = timestamp - stats->start_ns;
> -		else
> -			active_runtime = 0;
> +		v3d_get_stats(stats, timestamp, &active_runtime, &jobs_completed);
>   
>   		/* Each line will display the queue name, timestamp, the number
>   		 * of jobs sent to that queue and the runtime, as can be seem here:
> @@ -40,9 +37,7 @@ gpu_stats_show(struct device *dev, struct device_attribute *attr, char *buf)
>   		 */
>   		len += sysfs_emit_at(buf, len, "%s\t%llu\t%llu\t%llu\n",
>   				     v3d_queue_to_string(queue),
> -				     timestamp,
> -				     stats->jobs_completed,
> -				     stats->enabled_ns + active_runtime);
> +				     timestamp, jobs_completed, active_runtime);
>   	}
>   
>   	return len;