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,
job_lock serialises the access to the active perfmon, but not every access
was being serialized. For example, vc4_perfmon_get_values_ioctl() copies
perfmon->counters out while vc4_perfmon_stop() accumulates into the same
array from the interrupt handler, so userspace can read a half-updated
array. Also, vc4_perfmon_delete() tests the pointer before taking any
lock.
Extending job_lock over those two paths would put a userspace read and the
destroy path on the lock the interrupt handler uses to drive the job
queues. The perfmon state has no invariant in common with those queues.
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.
Fixes: 65101d8c9108 ("drm/vc4: Expose performance counters 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 | 7 ++-----
drivers/gpu/drm/vc4/vc4_irq.c | 3 +--
drivers/gpu/drm/vc4/vc4_perfmon.c | 40 +++++++++++++++++++++++++++------------
4 files changed, 41 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index d18a0fece93c..2fad8a489558 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 b61f9b0a7c09..1f3cb3c2c7fc 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -488,11 +488,7 @@ vc4_submit_next_bin_job(struct drm_device *dev)
vc4_flush_caches(dev);
- /* Only start the perfmon if it was not already started by a previous
- * job.
- */
- if (exec->perfmon && vc4->active_perfmon != exec->perfmon)
- vc4_perfmon_start(vc4, exec->perfmon);
+ vc4_perfmon_start(vc4, exec->perfmon);
/* Either put the job in the binner if it uses the binner, or
* immediately move it to the to-be-rendered queue.
@@ -1181,6 +1177,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_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index dcee4a1e17ad..f8787d4169ef 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -145,8 +145,7 @@ vc4_cancel_bin_job(struct drm_device *dev)
return;
/* Stop the perfmon so that the next bin job can be started. */
- if (exec->perfmon)
- vc4_perfmon_stop(vc4, exec->perfmon, false);
+ vc4_perfmon_stop(vc4, exec->perfmon, false);
list_move_tail(&exec->head, &vc4->bin_job_list);
vc4_submit_next_bin_job(dev);
diff --git a/drivers/gpu/drm/vc4/vc4_perfmon.c
b/drivers/gpu/drm/vc4/vc4_perfmon.c
index f75dfd156756..dd3414157d61 100644
--- a/drivers/gpu/drm/vc4/vc4_perfmon.c
+++ b/drivers/gpu/drm/vc4/vc4_perfmon.c
@@ -51,7 +51,15 @@ 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);
+
+ /* Only start the perfmon if it was not already started by a
+ * previous job.
+ */
+ if (vc4->perfmon_state.active == perfmon)
return;
for (i = 0; i < perfmon->ncounters; i++)
@@ -60,7 +68,7 @@ void vc4_perfmon_start(struct vc4_dev *vc4, struct
vc4_perfmon *perfmon)
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 +79,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 +93,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 +128,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 +233,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 +250,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.55.0
