On 15/01/2026 15:05, Maíra Canal wrote:
The IDR interface is deprecated and the XArray API is the recommended
replacement. Replace the per-file IDR used to track perfmons with an
XArray. This allows us to remove the external mutex that protects the
IDR.
While here, introduce the v3d_perfmon_delete() helper to consolidate
the perfmon cleanup logic used by both v3d_perfmon_close_file() and
v3d_perfmon_destroy_ioctl().
Signed-off-by: Maíra Canal <[email protected]>
---
drivers/gpu/drm/v3d/v3d_drv.h | 5 +--
drivers/gpu/drm/v3d/v3d_perfmon.c | 55 +++++++++++--------------------
2 files changed, 21 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 99a39329bb85..314213c26710 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -220,10 +220,7 @@ v3d_has_csd(struct v3d_dev *v3d)
struct v3d_file_priv {
struct v3d_dev *v3d;
- struct {
- struct idr idr;
- struct mutex lock;
- } perfmon;
+ struct xarray perfmons;
struct drm_sched_entity sched_entity[V3D_MAX_QUEUES];
diff --git a/drivers/gpu/drm/v3d/v3d_perfmon.c b/drivers/gpu/drm/v3d/v3d_perfmon.c
index 9a3fe5255874..41325ffc7f43 100644
--- a/drivers/gpu/drm/v3d/v3d_perfmon.c
+++ b/drivers/gpu/drm/v3d/v3d_perfmon.c
@@ -290,24 +290,23 @@ struct v3d_perfmon *v3d_perfmon_find(struct v3d_file_priv
*v3d_priv, int id)
{
struct v3d_perfmon *perfmon;
- mutex_lock(&v3d_priv->perfmon.lock);
- perfmon = idr_find(&v3d_priv->perfmon.idr, id);
+ xa_lock(&v3d_priv->perfmons);
+ perfmon = xa_load(&v3d_priv->perfmons, id);
v3d_perfmon_get(perfmon);
- mutex_unlock(&v3d_priv->perfmon.lock);
+ xa_unlock(&v3d_priv->perfmons);
return perfmon;
}
void v3d_perfmon_open_file(struct v3d_file_priv *v3d_priv)
{
- mutex_init(&v3d_priv->perfmon.lock);
- idr_init_base(&v3d_priv->perfmon.idr, 1);
+ xa_init_flags(&v3d_priv->perfmons, XA_FLAGS_ALLOC1);
}
-static int v3d_perfmon_idr_del(int id, void *elem, void *data)
+static void v3d_perfmon_delete(struct v3d_file_priv *v3d_priv,
+ struct v3d_perfmon *perfmon)
{
- struct v3d_perfmon *perfmon = elem;
- struct v3d_dev *v3d = (struct v3d_dev *)data;
+ struct v3d_dev *v3d = v3d_priv->v3d;
/* If the active perfmon is being destroyed, stop it first */
if (perfmon == v3d->active_perfmon)
@@ -317,19 +316,17 @@ static int v3d_perfmon_idr_del(int id, void *elem, void
*data)
cmpxchg(&v3d->global_perfmon, perfmon, NULL);
v3d_perfmon_put(perfmon);
-
- return 0;
}
void v3d_perfmon_close_file(struct v3d_file_priv *v3d_priv)
{
- struct v3d_dev *v3d = v3d_priv->v3d;
+ struct v3d_perfmon *perfmon;
+ unsigned long id;
- mutex_lock(&v3d_priv->perfmon.lock);
- idr_for_each(&v3d_priv->perfmon.idr, v3d_perfmon_idr_del, v3d);
- idr_destroy(&v3d_priv->perfmon.idr);
- mutex_unlock(&v3d_priv->perfmon.lock);
- mutex_destroy(&v3d_priv->perfmon.lock);
+ xa_for_each(&v3d_priv->perfmons, id, perfmon)
+ v3d_perfmon_delete(v3d_priv, perfmon);
Looking outside the XArray conversion the situation with
v3d->active_perfmon caught my eye. It doesn't look that driver global
field is protected by any lock, right? So if one client would be
exiting, is there enough serialisation between that and perhaps
v3d_switch_perfmon() (from run_job()) could see a stale
v3d->active_perfmon pointer and attempt to double put it? I am not sure,
but gain, that seem outside the scope of this conversion.
+
+ xa_destroy(&v3d_priv->perfmons);
}
int v3d_perfmon_create_ioctl(struct drm_device *dev, void *data,
@@ -341,6 +338,7 @@ int v3d_perfmon_create_ioctl(struct drm_device *dev, void
*data,
struct v3d_perfmon *perfmon;
unsigned int i;
int ret;
+ u32 id;
/* Number of monitored counters cannot exceed HW limits. */
if (req->ncounters > DRM_V3D_MAX_PERF_COUNTERS ||
@@ -366,18 +364,16 @@ int v3d_perfmon_create_ioctl(struct drm_device *dev, void
*data,
refcount_set(&perfmon->refcnt, 1);
mutex_init(&perfmon->lock);
- mutex_lock(&v3d_priv->perfmon.lock);
- ret = idr_alloc(&v3d_priv->perfmon.idr, perfmon, V3D_PERFMONID_MIN,
- V3D_PERFMONID_MAX, GFP_KERNEL);
Hm it seems V3D_PERFMONID_MAX being U32_MAX used to overflow the end
argument of idr_alloc.
- mutex_unlock(&v3d_priv->perfmon.lock);
-
+ ret = xa_alloc(&v3d_priv->perfmons, &id, perfmon,
+ XA_LIMIT(V3D_PERFMONID_MIN, V3D_PERFMONID_MAX),
Could perheps even lose the V3D_PERFMON_MIN/MAX defines and just use
xa_limit_32b.
+ GFP_KERNEL);
if (ret < 0) {
mutex_destroy(&perfmon->lock);
kfree(perfmon);
return ret;
}
- req->id = ret;
+ req->id = id;
return 0;
}
@@ -387,24 +383,13 @@ int v3d_perfmon_destroy_ioctl(struct drm_device *dev,
void *data,
{
struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
struct drm_v3d_perfmon_destroy *req = data;
- struct v3d_dev *v3d = v3d_priv->v3d;
struct v3d_perfmon *perfmon;
- mutex_lock(&v3d_priv->perfmon.lock);
- perfmon = idr_remove(&v3d_priv->perfmon.idr, req->id);
- mutex_unlock(&v3d_priv->perfmon.lock);
-
+ perfmon = xa_erase(&v3d_priv->perfmons, req->id);
if (!perfmon)
return -EINVAL;
- /* If the active perfmon is being destroyed, stop it first */
- if (perfmon == v3d->active_perfmon)
- v3d_perfmon_stop(v3d, perfmon, false);
-
- /* If the global perfmon is being destroyed, set it to NULL */
- cmpxchg(&v3d->global_perfmon, perfmon, NULL);
-
- v3d_perfmon_put(perfmon);
+ v3d_perfmon_delete(v3d_priv, perfmon);
Nice consolidation.
Reviewed-by: Tvrtko Ursulin <[email protected]>
Regards,
Tvrtko
return 0;
}
