drm/v3d: Refactor perfmon locking

v3d exposes a single set of performance counters per core, so at any
moment at most one performance monitor can be programmed in HW. In
software, this singleton is represented by v3d_dev->active_perfmon, but
until now nothing actually serialized access to it: scheduler callbacks,
the GPU-reset path, and perfmon ioctls all read and wrote that field
lock-free.

The existence of v3d_perfmon->lock mutex did not close the gap. It
serialized start/stop of *one* perfmon object against itself, but the
invariant that needs protection is device-wide: there can be exactly one
active perfmon at any moment in HW. Two threads acting on different
perfmon objects could race through v3d_dev->active_perfmon and the
counter registers, leaving software and HW out of sync.

This commit moves the locking to where the invariant actually lives. Group
the active perfmon pointer with a device-wide spinlock and route every
state transition (job start, job completion, set global, reset,
suspend/resume, destruction) through a small set of locked entry points
that are the only mutators of the HW counters.

Some design improvements needed to be made for the refactor:

  1. Stop the perfmon from the IRQ handler at job-completion time (the
     natural boundary for "active perfmon follows the active job"). This
     required a change from a mutex to a spinlock. This solves another
     issue of the existing design: perfmon start/stop was exclusively
     attached to run_job() callbacks, which means that if nothing was
     further queued up, a perfmon would never actually be stopped.

  2. Pause/resume the HW counters across runtime-PM transitions without
     dropping the software reference. This preserves the perfmon state
     while the device is idle.

  3. Move the global perfmon lifecycle management to the set_global
     IOCTL. This simplifies the logic in v3d_perfmon_start() and
     v3d_perfmon_stop(), as there is no need to always check if the
     global perfmon is enabled.

  4. v3d_perfmon_get_values_ioctl() doesn't stop the perfmon when
     capturing the values. All lifecycle management is handled by the
     job (for per-job perfmons) or the set_global IOCTL (for global
     perfmons).

Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Link: https://patch.msgid.link/20260706-v3d-perfmon-lifetime-v4-1-d7b312ff2c83@igalia.com
Signed-off-by: Maíra Canal <mcanal@igalia.com>

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 2e298d7545c062c6e349cfb9ff6692e8c2189510..cf836c4f13ad56db8f9c22f16f1f99445a90f171 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -87,9 +87,6 @@ struct v3d_perfmon {
         */
        refcount_t refcnt;
 
-       /* Protects perfmon stop, as it can be invoked from multiple places. */
-       struct mutex lock;
-
        /* Number of counters activated in this perfmon instance
         * (should be less than DRM_V3D_MAX_PERF_COUNTERS).
         */
@@ -171,8 +168,14 @@ struct v3d_dev {
 
        struct v3d_queue_state queue[V3D_MAX_QUEUES];
 
-       /* Used to track the active perfmon if any. */
-       struct v3d_perfmon *active_perfmon;
+       /* Tracks the performance monitor state. */
+       struct {
+               /* Protects @active. */
+               spinlock_t lock;
+
+               /* Perfmon currently programmed in HW (or NULL if none). */
+               struct v3d_perfmon *active;
+       } perfmon_state;
 
        /* Protects bo_stats */
        struct mutex bo_lock;
@@ -667,6 +670,10 @@ void v3d_perfmon_put(struct v3d_perfmon *perfmon);
 void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon *perfmon);
 void v3d_perfmon_stop(struct v3d_dev *v3d, struct v3d_perfmon *perfmon,
                      bool capture);
+void v3d_perfmon_stop_locked(struct v3d_dev *v3d, struct v3d_perfmon *perfmon,
+                            bool capture);
+void v3d_perfmon_suspend(struct v3d_dev *v3d);
+void v3d_perfmon_resume(struct v3d_dev *v3d);
 struct v3d_perfmon *v3d_perfmon_find(struct v3d_file_priv *v3d_priv, int id);
 void v3d_perfmon_open_file(struct v3d_file_priv *v3d_priv);
 void v3d_perfmon_close_file(struct v3d_file_priv *v3d_priv);

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index c43d9af41374b7ac8d7842e14645067e3c9f652d..7ad81b146f02ed6171c19643b32151d1cd66e291 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -137,7 +137,8 @@ v3d_reset(struct v3d_dev *v3d)
        v3d_mmu_set_page_table(v3d);
        v3d_irq_reset(v3d);
 
-       v3d_perfmon_stop(v3d, v3d->active_perfmon, false);
+       /* Re-arm the global perfmon HW counters that the reset zeroed. */
+       v3d_perfmon_resume(v3d);
 
        trace_v3d_reset_end(dev);
 }
@@ -307,6 +308,7 @@ v3d_gem_init(struct drm_device *dev)
        }
 
        spin_lock_init(&v3d->mm_lock);
+       spin_lock_init(&v3d->perfmon_state.lock);
        ret = drmm_mutex_init(dev, &v3d->bo_lock);
        if (ret)
                goto err_stats;

diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 86efaef2722c3602346b037ba536228b2f368a81..963d711dc16ba553fb80c18017c92d0539dd0145 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -90,9 +90,12 @@ v3d_irq_signal_fence(struct v3d_dev *v3d, enum v3d_queue q,
                     void (*trace_irq)(struct drm_device *, uint64_t))
 {
        struct v3d_queue_state *queue = &v3d->queue[q];
-       struct v3d_fence *fence = to_v3d_fence(queue->active_job->irq_fence);
+       struct v3d_job *job = queue->active_job;
+       struct v3d_fence *fence = to_v3d_fence(job->irq_fence);
 
-       v3d_job_update_stats(queue->active_job);
+       v3d_perfmon_stop(v3d, job->perfmon, true);
+
+       v3d_job_update_stats(job);
        trace_irq(&v3d->drm, fence->seqno);
 
        queue->active_job = NULL;

diff --git a/drivers/gpu/drm/v3d/v3d_perfmon.c b/drivers/gpu/drm/v3d/v3d_perfmon.c
index 48ae748247be8fa140143bad1ed92905e5b2f6ff..3ad0f022753c844ce9985de2b9bbc0bee7d95353 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_perfmon.c
+++ b/drivers/gpu/drm/v3d/v3d_perfmon.c
@@ -217,26 +217,15 @@ void v3d_perfmon_get(struct v3d_perfmon *perfmon)
 
 void v3d_perfmon_put(struct v3d_perfmon *perfmon)
 {
-       if (perfmon && refcount_dec_and_test(&perfmon->refcnt)) {
-               mutex_destroy(&perfmon->lock);
+       if (perfmon && refcount_dec_and_test(&perfmon->refcnt))
                kfree(perfmon);
-       }
 }
 
-void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon *perfmon)
+static void v3d_perfmon_hw_start(struct v3d_dev *v3d, struct v3d_perfmon *perfmon)
 {
+       u8 ncounters = perfmon->ncounters;
+       u32 mask = GENMASK(ncounters - 1, 0);
        unsigned int i;
-       u32 mask;
-       u8 ncounters;
-
-       if (WARN_ON_ONCE(!perfmon || v3d->active_perfmon))
-               return;
-
-       if (!pm_runtime_get_if_active(v3d->drm.dev))
-               return;
-
-       ncounters = perfmon->ncounters;
-       mask = GENMASK(ncounters - 1, 0);
 
        for (i = 0; i < ncounters; i++) {
                u32 source = i / 4;
@@ -258,39 +247,106 @@ void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon *perfmon)
        V3D_CORE_WRITE(0, V3D_V4_PCTR_0_EN, mask);
        V3D_CORE_WRITE(0, V3D_V4_PCTR_0_CLR, mask);
        V3D_CORE_WRITE(0, V3D_PCTR_0_OVERFLOW, mask);
+}
 
-       v3d->active_perfmon = perfmon;
+static void v3d_perfmon_hw_capture(struct v3d_dev *v3d, struct v3d_perfmon *perfmon)
+{
+       u32 mask = GENMASK(perfmon->ncounters - 1, 0);
 
-       v3d_pm_runtime_put(v3d);
+       for (int i = 0; i < perfmon->ncounters; i++)
+               perfmon->values[i] += V3D_CORE_READ(0, V3D_PCTR_0_PCTRX(i));
+
+       V3D_CORE_WRITE(0, V3D_V4_PCTR_0_CLR, mask);
 }
 
-void v3d_perfmon_stop(struct v3d_dev *v3d, struct v3d_perfmon *perfmon,
-                     bool capture)
+static void v3d_perfmon_hw_stop(struct v3d_dev *v3d, struct v3d_perfmon *perfmon,
+                               bool capture)
 {
-       unsigned int i;
+       if (capture)
+               v3d_perfmon_hw_capture(v3d, perfmon);
+
+       V3D_CORE_WRITE(0, V3D_V4_PCTR_0_EN, 0);
+}
+
+void v3d_perfmon_start(struct v3d_dev *v3d, struct v3d_perfmon *perfmon)
+{
+       guard(spinlock_irqsave)(&v3d->perfmon_state.lock);
+
+       if (!perfmon || v3d->global_perfmon)
+               return;
 
-       if (!perfmon || !v3d->active_perfmon)
+       if (!pm_runtime_get_if_active(v3d->drm.dev))
                return;
 
-       mutex_lock(&perfmon->lock);
-       if (perfmon != v3d->active_perfmon)
-               goto out;
+       v3d_perfmon_hw_start(v3d, perfmon);
+       v3d->perfmon_state.active = perfmon;
+
+       v3d_pm_runtime_put(v3d);
+}
+
+static void v3d_perfmon_capture_locked(struct v3d_dev *v3d,
+                                      struct v3d_perfmon *perfmon)
+{
+       lockdep_assert_held(&v3d->perfmon_state.lock);
+
+       if (!perfmon || perfmon != v3d->perfmon_state.active)
+               return;
 
        if (!pm_runtime_get_if_active(v3d->drm.dev))
-               goto out_clear;
+               return;
 
-       if (capture)
-               for (i = 0; i < perfmon->ncounters; i++)
-                       perfmon->values[i] += V3D_CORE_READ(0, V3D_PCTR_0_PCTRX(i));
+       v3d_perfmon_hw_capture(v3d, perfmon);
+       v3d_pm_runtime_put(v3d);
+}
 
-       V3D_CORE_WRITE(0, V3D_V4_PCTR_0_EN, 0);
+void v3d_perfmon_stop_locked(struct v3d_dev *v3d, struct v3d_perfmon *perfmon,
+                            bool capture)
+{
+       lockdep_assert_held(&v3d->perfmon_state.lock);
+
+       if (!perfmon || perfmon != v3d->perfmon_state.active)
+               return;
+
+       v3d->perfmon_state.active = NULL;
+
+       /* If the device is suspended, the HW has already stopped counting. */
+       if (!pm_runtime_get_if_active(v3d->drm.dev))
+               return;
 
+       v3d_perfmon_hw_stop(v3d, perfmon, capture);
        v3d_pm_runtime_put(v3d);
+}
 
-out_clear:
-       v3d->active_perfmon = NULL;
-out:
-       mutex_unlock(&perfmon->lock);
+void v3d_perfmon_stop(struct v3d_dev *v3d, struct v3d_perfmon *perfmon,
+                     bool capture)
+{
+       if (!perfmon)
+               return;
+
+       guard(spinlock_irqsave)(&v3d->perfmon_state.lock);
+       v3d_perfmon_stop_locked(v3d, perfmon, capture);
+}
+
+void
+v3d_perfmon_suspend(struct v3d_dev *v3d)
+{
+       guard(spinlock_irqsave)(&v3d->perfmon_state.lock);
+
+       if (!v3d->perfmon_state.active)
+               return;
+
+       v3d_perfmon_hw_stop(v3d, v3d->perfmon_state.active, true);
+}
+
+void
+v3d_perfmon_resume(struct v3d_dev *v3d)
+{
+       guard(spinlock_irqsave)(&v3d->perfmon_state.lock);
+
+       if (!v3d->perfmon_state.active)
+               return;
+
+       v3d_perfmon_hw_start(v3d, v3d->perfmon_state.active);
 }
 
 struct v3d_perfmon *v3d_perfmon_find(struct v3d_file_priv *v3d_priv, int id)
@@ -316,14 +372,17 @@ static void v3d_perfmon_delete(struct v3d_file_priv *v3d_priv,
        struct v3d_dev *v3d = v3d_priv->v3d;
 
        /* If the active perfmon is being destroyed, stop it first */
-       if (perfmon == v3d->active_perfmon)
-               v3d_perfmon_stop(v3d, perfmon, false);
+       scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock) {
+               v3d_perfmon_stop_locked(v3d, perfmon, false);
 
-       /* If the global perfmon is being destroyed, clean it and release
-        * the reference stashed in v3d_perfmon_set_global_ioctl().
-        */
-       if (cmpxchg(&v3d->global_perfmon, perfmon, NULL) == perfmon)
-               v3d_perfmon_put(perfmon);
+               /* If the global perfmon is being destroyed, clean it and release
+                * the reference stashed in v3d_perfmon_set_global_ioctl().
+                */
+               if (v3d->global_perfmon == perfmon) {
+                       v3d_perfmon_put(v3d->global_perfmon);
+                       v3d->global_perfmon = NULL;
+               }
+       }
 
        v3d_perfmon_put(perfmon);
 }
@@ -371,12 +430,10 @@ int v3d_perfmon_create_ioctl(struct drm_device *dev, void *data,
        perfmon->ncounters = req->ncounters;
 
        refcount_set(&perfmon->refcnt, 1);
-       mutex_init(&perfmon->lock);
 
        ret = xa_alloc(&v3d_priv->perfmons, &id, perfmon, xa_limit_32b,
                       GFP_KERNEL);
        if (ret < 0) {
-               mutex_destroy(&perfmon->lock);
                kfree(perfmon);
                return ret;
        }
@@ -408,7 +465,9 @@ int v3d_perfmon_get_values_ioctl(struct drm_device *dev, void *data,
        struct v3d_dev *v3d = to_v3d_dev(dev);
        struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
        struct drm_v3d_perfmon_get_values *req = data;
+       u64 values[DRM_V3D_MAX_PERF_COUNTERS];
        struct v3d_perfmon *perfmon;
+       size_t size;
        int ret = 0;
 
        if (req->pad != 0)
@@ -418,10 +477,14 @@ int v3d_perfmon_get_values_ioctl(struct drm_device *dev, void *data,
        if (!perfmon)
                return -EINVAL;
 
-       v3d_perfmon_stop(v3d, perfmon, true);
+       size = perfmon->ncounters * sizeof(u64);
+
+       scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock) {
+               v3d_perfmon_capture_locked(v3d, perfmon);
+               memcpy(values, perfmon->values, size);
+       }
 
-       if (copy_to_user(u64_to_user_ptr(req->values_ptr), perfmon->values,
-                        perfmon->ncounters * sizeof(u64)))
+       if (copy_to_user(u64_to_user_ptr(req->values_ptr), values, size))
                ret = -EFAULT;
 
        v3d_perfmon_put(perfmon);
@@ -482,18 +545,36 @@ int v3d_perfmon_set_global_ioctl(struct drm_device *dev, void *data,
                 */
                v3d_perfmon_put(perfmon);
 
-               old = xchg(&v3d->global_perfmon, NULL);
-               if (!old)
-                       return -EINVAL;
+               scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock) {
+                       old = v3d->global_perfmon;
+                       if (!old)
+                               return -EINVAL;
+
+                       v3d_perfmon_stop_locked(v3d, old, true);
+                       v3d->global_perfmon = NULL;
+               }
 
                v3d_perfmon_put(old);
 
                return 0;
        }
 
-       if (cmpxchg(&v3d->global_perfmon, NULL, perfmon)) {
-               v3d_perfmon_put(perfmon);
-               return -EBUSY;
+       scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock) {
+               if (v3d->perfmon_state.active || v3d->global_perfmon) {
+                       v3d_perfmon_put(perfmon);
+                       return -EBUSY;
+               }
+
+               v3d->global_perfmon = perfmon;
+               v3d->perfmon_state.active = perfmon;
+
+               /* If the device is suspended, v3d_perfmon_resume() will
+                * program the HW on the next resume.
+                */
+               if (pm_runtime_get_if_active(v3d->drm.dev)) {
+                       v3d_perfmon_hw_start(v3d, perfmon);
+                       v3d_pm_runtime_put(v3d);
+               }
        }
 
        return 0;

diff --git a/drivers/gpu/drm/v3d/v3d_power.c b/drivers/gpu/drm/v3d/v3d_power.c
index f7df6393d38fe3a83a4aa3e517c6b7a6add3c481..ade8e932fb9c54452418eedbb1e56ffc6dd93cd2 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_power.c
+++ b/drivers/gpu/drm/v3d/v3d_power.c
@@ -50,6 +50,8 @@ int v3d_power_suspend(struct device *dev)
        struct v3d_dev *v3d = to_v3d_dev(drm);
        int ret;
 
+       v3d_perfmon_suspend(v3d);
+
        v3d_irq_disable(v3d);
 
        v3d_clean_caches(v3d);
@@ -85,5 +87,7 @@ int v3d_power_resume(struct device *dev)
        v3d_mmu_set_page_table(v3d);
        v3d_irq_enable(v3d);
 
+       v3d_perfmon_resume(v3d);
+
        return 0;
 }

diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index e950cee31bcba6cdd1aa46e14d5f91075727d2f9..5f33d379459855e50af120635c15b9dadccb0f72 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -125,24 +125,6 @@ v3d_performance_query_info_free(struct v3d_performance_query_info *query_info,
        }
 }
 
-static void
-v3d_switch_perfmon(struct v3d_dev *v3d, struct v3d_job *job)
-{
-       struct v3d_perfmon *perfmon = v3d->global_perfmon;
-
-       if (!perfmon)
-               perfmon = job->perfmon;
-
-       if (perfmon == v3d->active_perfmon)
-               return;
-
-       if (perfmon != v3d->active_perfmon)
-               v3d_perfmon_stop(v3d, v3d->active_perfmon, true);
-
-       if (perfmon && v3d->active_perfmon != perfmon)
-               v3d_perfmon_start(v3d, perfmon);
-}
-
 static void
 v3d_stats_start(struct v3d_stats *stats, u64 now)
 {
@@ -220,7 +202,7 @@ static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
                            job->start, job->end);
 
        v3d_job_start_stats(&job->base);
-       v3d_switch_perfmon(v3d, &job->base);
+       v3d_perfmon_start(v3d, job->base.perfmon);
 
        /* Set the current and end address of the control list.
         * Writing the end register is what starts the job.
@@ -278,7 +260,7 @@ static struct dma_fence *v3d_render_job_run(struct drm_sched_job *sched_job)
                            job->start, job->end);
 
        v3d_job_start_stats(&job->base);
-       v3d_switch_perfmon(v3d, &job->base);
+       v3d_perfmon_start(v3d, job->base.perfmon);
 
        /* XXX: Set the QCFG */
 
@@ -381,7 +363,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_switch_perfmon(v3d, &job->base);
+       v3d_perfmon_start(v3d, job->base.perfmon);
 
        csd_cfg0_reg = V3D_CSD_QUEUED_CFG0(v3d->ver);
        for (i = 1; i <= 6; i++)
@@ -723,6 +705,8 @@ v3d_gpu_reset_for_timeout(struct v3d_dev *v3d, struct drm_sched_job *sched_job,
        if (sched_job)
                drm_sched_increase_karma(sched_job);
 
+       v3d_perfmon_stop(v3d, job->perfmon, false);
+
        /* get the GPU back into the init state */
        v3d_reset(v3d);
 

diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
index ee2ac2540ed50c68b37ed71892d77dce387e368f..27cc98770b37e16c49571fea0ca62338cff9daa5 100644 (file)
--- a/drivers/gpu/drm/v3d/v3d_submit.c
+++ b/drivers/gpu/drm/v3d/v3d_submit.c
@@ -302,8 +302,10 @@ v3d_attach_perfmon_to_jobs(struct v3d_submit *submit, u32 perfmon_id)
        if (!perfmon_id)
                return 0;
 
-       if (v3d->global_perfmon)
-               return -EAGAIN;
+       scoped_guard(spinlock_irqsave, &v3d->perfmon_state.lock) {
+               if (v3d->global_perfmon)
+                       return -EAGAIN;
+       }
 
        perfmon = v3d_perfmon_find(v3d_priv, perfmon_id);
        if (!perfmon)