Re: [PATCH 2/2] drm/vc4: Replace IDR with XArray for perfmon tracking

[Tvrtko Ursulin]

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 at it, introduce the vc4_perfmon_delete() helper to consolidate
the perfmon cleanup logic used by both vc4_perfmon_close_file() and
vc4_perfmon_destroy_ioctl(). Also, remove the redundant assignment of
vc4file->dev to itself in vc4_perfmon_open_file().

Signed-off-by: Maíra Canal <mca...@igalia.com>
---
  drivers/gpu/drm/vc4/vc4_drv.h     |  5 +---
  drivers/gpu/drm/vc4/vc4_perfmon.c | 49 ++++++++++++++-----------------
  2 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 221d8e01d539..dbcc83b7df00 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -791,10 +791,7 @@ struct vc4_exec_info {
  struct vc4_file {
        struct vc4_dev *dev;
  
-	struct {
-               struct idr idr;
-               struct mutex lock;
-       } perfmon;
+       struct xarray perfmons;
  
  	bool bin_bo_used;
  };
diff --git a/drivers/gpu/drm/vc4/vc4_perfmon.c 
b/drivers/gpu/drm/vc4/vc4_perfmon.c
index 1ac80c0b258f..29e549342852 100644
--- a/drivers/gpu/drm/vc4/vc4_perfmon.c
+++ b/drivers/gpu/drm/vc4/vc4_perfmon.c
@@ -95,10 +95,10 @@ struct vc4_perfmon *vc4_perfmon_find(struct vc4_file 
*vc4file, int id)
        if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
                return NULL;
  
-	mutex_lock(&vc4file->perfmon.lock);
-       perfmon = idr_find(&vc4file->perfmon.idr, id);
+       xa_lock(&vc4file->perfmons);
+       perfmon = xa_load(&vc4file->perfmons, id);
        vc4_perfmon_get(perfmon);
-       mutex_unlock(&vc4file->perfmon.lock);
+       xa_unlock(&vc4file->perfmons);
  
  	return perfmon;
  }
@@ -110,37 +110,34 @@ void vc4_perfmon_open_file(struct vc4_file *vc4file)
        if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
                return;
  
-	mutex_init(&vc4file->perfmon.lock);
-       idr_init_base(&vc4file->perfmon.idr, VC4_PERFMONID_MIN);
-       vc4file->dev = vc4;
+       xa_init_flags(&vc4file->perfmons, XA_FLAGS_ALLOC1);
  }
  
-static int vc4_perfmon_idr_del(int id, void *elem, void *data)
+static void vc4_perfmon_delete(struct vc4_file *vc4file,
+                              struct vc4_perfmon *perfmon)
  {
-       struct vc4_perfmon *perfmon = elem;
-       struct vc4_dev *vc4 = (struct vc4_dev *)data;
+       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_put(perfmon);
-
-       return 0;
  }
  
  void vc4_perfmon_close_file(struct vc4_file *vc4file)
  {
        struct vc4_dev *vc4 = vc4file->dev;
+       struct vc4_perfmon *perfmon;
+       unsigned long id;
  
  	if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
                return;
  
-	mutex_lock(&vc4file->perfmon.lock);
-       idr_for_each(&vc4file->perfmon.idr, vc4_perfmon_idr_del, vc4);
-       idr_destroy(&vc4file->perfmon.idr);
-       mutex_unlock(&vc4file->perfmon.lock);
-       mutex_destroy(&vc4file->perfmon.lock);
+       xa_for_each(&vc4file->perfmons, id, perfmon)
+               vc4_perfmon_delete(vc4file, perfmon);
+
+       xa_destroy(&vc4file->perfmons);
  }
  
  int vc4_perfmon_create_ioctl(struct drm_device *dev, void *data,
@@ -152,6 +149,7 @@ int vc4_perfmon_create_ioctl(struct drm_device *dev, void 
*data,
        struct vc4_perfmon *perfmon;
        unsigned int i;
        int ret;
+       u32 id;
  
  	if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
                return -ENODEV;
@@ -185,17 +183,16 @@ int vc4_perfmon_create_ioctl(struct drm_device *dev, void 
*data,
  
  	refcount_set(&perfmon->refcnt, 1);
  
-	mutex_lock(&vc4file->perfmon.lock);
-       ret = idr_alloc(&vc4file->perfmon.idr, perfmon, VC4_PERFMONID_MIN,
-                       VC4_PERFMONID_MAX, GFP_KERNEL);
-       mutex_unlock(&vc4file->perfmon.lock);
-
+       ret = xa_alloc(&vc4file->perfmons, &id, perfmon,
+                      XA_LIMIT(VC4_PERFMONID_MIN, VC4_PERFMONID_MAX),

Appears the same further cleanup potential with the min/max defines as 
in v3d. Anyway, conversion looks good to me:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursu...@igalia.com>

Regards,

Tvrtko

+                      GFP_KERNEL);
        if (ret < 0) {
                kfree(perfmon);
                return ret;
        }
  
-	req->id = ret;
+       req->id = id;
+
        return 0;
  }
  
@@ -215,14 +212,12 @@ int vc4_perfmon_destroy_ioctl(struct drm_device *dev, void *data,
                return -ENODEV;
        }
  
-	mutex_lock(&vc4file->perfmon.lock);
-       perfmon = idr_remove(&vc4file->perfmon.idr, req->id);
-       mutex_unlock(&vc4file->perfmon.lock);
-
+       perfmon = xa_erase(&vc4file->perfmons, req->id);
        if (!perfmon)
                return -EINVAL;
  
-	vc4_perfmon_put(perfmon);
+       vc4_perfmon_delete(vc4file, perfmon);
+
        return 0;
  }