Re: [PATCH 15/19] drm/amdgpu: independence for the amdgpu_userq_fence! - Linaro-mm-sig

12 Dec 2025

On 12/11/25 17:12, Tvrtko Ursulin wrote:
...
On 11/12/2025 13:16, Christian König wrote:
...
This allows amdgpu_userq_fences to outlive the amdgpu module.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       | 13 +----
  .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 54 ++++---------------
  .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.h   |  8 ---
  3 files changed, 11 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2dfbddcef9ab..f206297aae8b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -3155,11 +3155,7 @@ static int __init amdgpu_init(void)
        r = amdgpu_sync_init();
      if (r)
-        goto error_sync;



-    r = amdgpu_userq_fence_slab_init();
-    if (r)
-        goto error_fence;
+        return r;
        DRM_INFO("amdgpu kernel modesetting enabled.\n");
      amdgpu_register_atpx_handler();
@@ -3176,12 +3172,6 @@ static int __init amdgpu_init(void)
        /* let modprobe override vga console setting */
      return pci_register_driver(&amdgpu_kms_pci_driver);



-error_fence:
-    amdgpu_sync_fini();



-error_sync:
-    return r;
  }
    static void __exit amdgpu_exit(void)
@@ -3191,7 +3181,6 @@ static void __exit amdgpu_exit(void)
      amdgpu_unregister_atpx_handler();
      amdgpu_acpi_release();
      amdgpu_sync_fini();
-    amdgpu_userq_fence_slab_fini();
      mmu_notifier_synchronize();
      amdgpu_xcp_drv_release();
  }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index eba9fb359047..bb19f72770b0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -33,26 +33,6 @@
  #include "amdgpu_userq_fence.h"
    static const struct dma_fence_ops amdgpu_userq_fence_ops;
-static struct kmem_cache *amdgpu_userq_fence_slab;



-int amdgpu_userq_fence_slab_init(void)
-{
-    amdgpu_userq_fence_slab = kmem_cache_create("amdgpu_userq_fence",
-                            sizeof(struct amdgpu_userq_fence),
-                            0,
-                            SLAB_HWCACHE_ALIGN,
-                            NULL);
-    if (!amdgpu_userq_fence_slab)
-        return -ENOMEM;



-    return 0;
-}



-void amdgpu_userq_fence_slab_fini(void)
-{
-    rcu_barrier();
What was this rcu_barrier() for? Cargo culted or more to it?
All dma_fences are RCU protected. When they are backed by a kmem_cache you need to make sure to wait for an RCU grace period to pass before destroying the kmem_cache.
Since the dma_fence framework now uses kfree_rcu that shouldn't be problematic any more.
...
...
-    kmem_cache_destroy(amdgpu_userq_fence_slab);
-}
    static inline struct amdgpu_userq_fence *to_amdgpu_userq_fence(struct dma_fence *f)
  {
@@ -227,7 +207,7 @@ void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv)
    static int amdgpu_userq_fence_alloc(struct amdgpu_userq_fence **userq_fence)
  {
-    *userq_fence = kmem_cache_alloc(amdgpu_userq_fence_slab, GFP_ATOMIC);
+    *userq_fence = kmalloc(sizeof(**userq_fence), GFP_ATOMIC);
This GFP_ATOMIC is suboptimal for sure being on the ioctl path. It is outside of the scope for this patch, but once my userq cleanup patches get reviewed next on my list was to try and understand this.
...
return *userq_fence ? 0 : -ENOMEM;
  }
  @@ -243,12 +223,11 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq,
      if (!fence_drv)
          return -EINVAL;
  -    spin_lock_init(&userq_fence->lock);
      INIT_LIST_HEAD(&userq_fence->link);
      fence = &userq_fence->base;
      userq_fence->fence_drv = fence_drv;
  -    dma_fence_init64(fence, &amdgpu_userq_fence_ops, &userq_fence->lock,
+    dma_fence_init64(fence, &amdgpu_userq_fence_ops, NULL,
               fence_drv->context, seq);
        amdgpu_userq_fence_driver_get(fence_drv);
@@ -318,35 +297,22 @@ static bool amdgpu_userq_fence_signaled(struct dma_fence *f)
      rptr = amdgpu_userq_fence_read(fence_drv);
      wptr = fence->base.seqno;
  -    if (rptr >= wptr)
+    if (rptr >= wptr) {
+        amdgpu_userq_fence_driver_put(fence->fence_drv);
fence_drv is in a local already.
...
+        fence->fence_drv = NULL;
amdgpu_userq_fence_get_timeline_name could now oops somehow?
...



+        kvfree(fence->fence_drv_array);
+        fence->fence_drv_array = NULL;
Not sure if this is safe either. amdgpu_userq_fence_driver_process() drops its reference before it unlinks the fence from the list. Can someone external trigger the fence_is_signaled check, before the interrupt processing kicks in, which will clear fence_drv_array, and so amdgpu_userq_fence_driver_process() would oops?
Oh, good question. I need to double check that.
Thanks,
Christian.
...
Regards,
Tvrtko
...
return true;
+    }
        return false;
  }
  -static void amdgpu_userq_fence_free(struct rcu_head *rcu)
-{
-    struct dma_fence *fence = container_of(rcu, struct dma_fence, rcu);
-    struct amdgpu_userq_fence *userq_fence = to_amdgpu_userq_fence(fence);
-    struct amdgpu_userq_fence_driver *fence_drv = userq_fence->fence_drv;



-    /* Release the fence driver reference */
-    amdgpu_userq_fence_driver_put(fence_drv);



-    kvfree(userq_fence->fence_drv_array);
-    kmem_cache_free(amdgpu_userq_fence_slab, userq_fence);
-}



-static void amdgpu_userq_fence_release(struct dma_fence *f)
-{
-    call_rcu(&f->rcu, amdgpu_userq_fence_free);
-}



static const struct dma_fence_ops amdgpu_userq_fence_ops = {
      .get_driver_name = amdgpu_userq_fence_get_driver_name,
      .get_timeline_name = amdgpu_userq_fence_get_timeline_name,
      .signaled = amdgpu_userq_fence_signaled,
-    .release = amdgpu_userq_fence_release,
  };
    /**
@@ -560,7 +526,7 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
      r = amdgpu_userq_fence_create(queue, userq_fence, wptr, &fence);
      if (r) {
          mutex_unlock(&userq_mgr->userq_mutex);
-        kmem_cache_free(amdgpu_userq_fence_slab, userq_fence);
+        kfree(userq_fence);
          goto put_gobj_write;
      }
  diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h
index d76add2afc77..6f04782f3ea9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h
@@ -31,11 +31,6 @@
    struct amdgpu_userq_fence {
      struct dma_fence base;
-    /*
-     * This lock is necessary to synchronize the
-     * userqueue dma fence operations.
-     */
-    spinlock_t lock;
      struct list_head link;
      unsigned long fence_drv_array_count;
      struct amdgpu_userq_fence_driver *fence_drv;
@@ -58,9 +53,6 @@ struct amdgpu_userq_fence_driver {
      char timeline_name[TASK_COMM_LEN];
  };
  -int amdgpu_userq_fence_slab_init(void);
-void amdgpu_userq_fence_slab_fini(void);



void amdgpu_userq_fence_driver_get(struct amdgpu_userq_fence_driver *fence_drv);
  void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv);
  int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,

    