From b136b1953f201542ae5cdfee4b35e9a2e4aa16fa Mon Sep 17 00:00:00 2001 From: Maíra Canal Date: Sat, 20 Apr 2024 18:32:10 -0300 Subject: drm/v3d: Create a struct to store the GPU stats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Tvrtko Ursulin Reviewed-by: Jose Maria Casanova Crespo Link: https://patchwork.freedesktop.org/patch/msgid/20240420213632.339941-4-mcanal@igalia.com --- drivers/gpu/drm/v3d/v3d_gem.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/gpu/drm/v3d/v3d_gem.c') diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index afc565078c78..0086081a9261 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -247,10 +247,10 @@ 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); - v3d->queue[i].start_ns = 0; - v3d->queue[i].enabled_ns = 0; - v3d->queue[i].jobs_sent = 0; + struct v3d_queue_state *queue = &v3d->queue[i]; + + queue->fence_context = dma_fence_context_alloc(1); + memset(&queue->stats, 0, sizeof(queue->stats)); } spin_lock_init(&v3d->mm_lock); -- cgit v1.2.3-59-g8ed1b From 6abe93b621ab12e93cf0eb7e42a609b36be32da1 Mon Sep 17 00:00:00 2001 From: Maíra Canal Date: Sat, 20 Apr 2024 18:32:13 -0300 Subject: drm/v3d: Fix race-condition between sysfs/fdinfo and interrupt handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 interrupt 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 match 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 Signed-off-by: Maíra Canal Reviewed-by: Tvrtko Ursulin Link: https://patchwork.freedesktop.org/patch/msgid/20240420213632.339941-7-mcanal@igalia.com --- drivers/gpu/drm/v3d/v3d_drv.c | 14 ++++++++++---- drivers/gpu/drm/v3d/v3d_drv.h | 7 +++++++ drivers/gpu/drm/v3d/v3d_gem.c | 1 + drivers/gpu/drm/v3d/v3d_sched.c | 7 +++++++ 4 files changed, 25 insertions(+), 4 deletions(-) (limited to 'drivers/gpu/drm/v3d/v3d_gem.c') diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index 2ec359ed2def..28b7ddce7747 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); @@ -145,10 +146,15 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file) void v3d_get_stats(const struct v3d_stats *stats, u64 timestamp, u64 *active_runtime, u64 *jobs_completed) { - *active_runtime = stats->enabled_ns; - if (stats->start_ns) - *active_runtime += timestamp - stats->start_ns; - *jobs_completed = stats->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)); } static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file) diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index ff06dc1cc078..a2c516fe6d79 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -40,6 +40,13 @@ 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; }; struct v3d_queue_state { diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 0086081a9261..da8faf3b9011 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -251,6 +251,7 @@ v3d_gem_init(struct drm_device *dev) 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 -- cgit v1.2.3-59-g8ed1b