The V3D block exposes a single set of performance counters, so at most one perfmon can be programmed into the hardware at a time. Until now the active perfmon was tracked by a bare pointer whose access was serialized by job_lock, which was sufficient because the legacy job queue started and stopped perfmons from a single serialized context.
Once job execution moves to the DRM GPU scheduler, a perfmon is started from the job's run_job() callback but stopped from the IRQ handler and the reset path, so the shared perfmon state can be touched concurrently and job_lock no longer covers those accesses. Introduce a dedicated spinlock next to the active perfmon pointer and take it in vc4_perfmon_start() and vc4_perfmon_stop(). Snapshot the counter values under the same lock in vc4_perfmon_get_values_ioctl() so that a concurrent stop cannot expose a half-updated array to userspace. Signed-off-by: Maíra Canal <[email protected]> --- drivers/gpu/drm/vc4/vc4_drv.h | 13 ++++++++++--- drivers/gpu/drm/vc4/vc4_gem.c | 3 ++- drivers/gpu/drm/vc4/vc4_perfmon.c | 34 ++++++++++++++++++++++------------ 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index fe313027a91e..91c9c7aa8039 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -181,10 +181,17 @@ struct vc4_dev { wait_queue_head_t job_wait_queue; struct work_struct job_done_work; - /* Used to track the active perfmon if any. Access to this field is - * protected by job_lock. + /* Tracks the performance monitor state. The V3D block exposes a single + * set of performance counters, so at most one perfmon can be active at + * any moment. */ - struct vc4_perfmon *active_perfmon; + struct { + /* Protects @active. */ + spinlock_t lock; + + /* Perfmon currently programmed in HW (or NULL if none). */ + struct vc4_perfmon *active; + } perfmon_state; /* The memory used for storing binner tile alloc, tile state, * and overflow memory allocations. This is freed when V3D diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c index f0c711f114cf..f84d82b3bfbb 100644 --- a/drivers/gpu/drm/vc4/vc4_gem.c +++ b/drivers/gpu/drm/vc4/vc4_gem.c @@ -491,7 +491,7 @@ vc4_submit_next_bin_job(struct drm_device *dev) /* Only start the perfmon if it was not already started by a previous * job. */ - if (exec->perfmon && vc4->active_perfmon != exec->perfmon) + if (exec->perfmon && vc4->perfmon_state.active != exec->perfmon) vc4_perfmon_start(vc4, exec->perfmon); /* Either put the job in the binner if it uses the binner, or @@ -1138,6 +1138,7 @@ int vc4_gem_init(struct drm_device *dev) INIT_LIST_HEAD(&vc4->render_job_list); INIT_LIST_HEAD(&vc4->job_done_list); spin_lock_init(&vc4->job_lock); + spin_lock_init(&vc4->perfmon_state.lock); INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work); timer_setup(&vc4->hangcheck.timer, vc4_hangcheck_elapsed, 0); diff --git a/drivers/gpu/drm/vc4/vc4_perfmon.c b/drivers/gpu/drm/vc4/vc4_perfmon.c index f75dfd156756..2bcf6a7d538f 100644 --- a/drivers/gpu/drm/vc4/vc4_perfmon.c +++ b/drivers/gpu/drm/vc4/vc4_perfmon.c @@ -51,16 +51,18 @@ void vc4_perfmon_start(struct vc4_dev *vc4, struct vc4_perfmon *perfmon) if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4)) return; - if (WARN_ON_ONCE(!perfmon || vc4->active_perfmon)) + if (!perfmon) return; + guard(spinlock_irqsave)(&vc4->perfmon_state.lock); + for (i = 0; i < perfmon->ncounters; i++) V3D_WRITE(V3D_PCTRS(i), perfmon->events[i]); mask = GENMASK(perfmon->ncounters - 1, 0); V3D_WRITE(V3D_PCTRC, mask); V3D_WRITE(V3D_PCTRE, V3D_PCTRE_EN | mask); - vc4->active_perfmon = perfmon; + vc4->perfmon_state.active = perfmon; } void vc4_perfmon_stop(struct vc4_dev *vc4, struct vc4_perfmon *perfmon, @@ -71,8 +73,12 @@ void vc4_perfmon_stop(struct vc4_dev *vc4, struct vc4_perfmon *perfmon, if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4)) return; - if (WARN_ON_ONCE(!vc4->active_perfmon || - perfmon != vc4->active_perfmon)) + if (!perfmon) + return; + + guard(spinlock_irqsave)(&vc4->perfmon_state.lock); + + if (perfmon != vc4->perfmon_state.active) return; if (capture) { @@ -81,7 +87,7 @@ void vc4_perfmon_stop(struct vc4_dev *vc4, struct vc4_perfmon *perfmon, } V3D_WRITE(V3D_PCTRE, 0); - vc4->active_perfmon = NULL; + vc4->perfmon_state.active = NULL; } struct vc4_perfmon *vc4_perfmon_find(struct vc4_file *vc4file, int id) @@ -116,8 +122,7 @@ static void vc4_perfmon_delete(struct vc4_file *vc4file, struct vc4_dev *vc4 = vc4file->dev; /* If the active perfmon is being destroyed, stop it first */ - if (perfmon == vc4->active_perfmon) - vc4_perfmon_stop(vc4, perfmon, false); + vc4_perfmon_stop(vc4, perfmon, false); vc4_perfmon_put(perfmon); } @@ -222,8 +227,10 @@ int vc4_perfmon_get_values_ioctl(struct drm_device *dev, void *data, struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_file *vc4file = file_priv->driver_priv; struct drm_vc4_perfmon_get_values *req = data; + u64 values[DRM_VC4_MAX_PERF_COUNTERS]; struct vc4_perfmon *perfmon; - int ret; + size_t size; + int ret = 0; if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4)) return -ENODEV; @@ -237,11 +244,14 @@ int vc4_perfmon_get_values_ioctl(struct drm_device *dev, void *data, if (!perfmon) return -EINVAL; - if (copy_to_user(u64_to_user_ptr(req->values_ptr), perfmon->counters, - perfmon->ncounters * sizeof(u64))) + size = perfmon->ncounters * sizeof(u64); + + /* Snapshot the counters under the perfmon lock */ + scoped_guard(spinlock_irqsave, &vc4->perfmon_state.lock) + memcpy(values, perfmon->counters, size); + + if (copy_to_user(u64_to_user_ptr(req->values_ptr), values, size)) ret = -EFAULT; - else - ret = 0; vc4_perfmon_put(perfmon); return ret; -- 2.54.0
