Re: [PATCH 03/11] drm/amdgpu: nuke amdgpu_userq_fence_free

["Khatri, Sunil" <sukhatri@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 8467 bytes --]


On 21-04-2026 06:25 pm, Christian König wrote:
> As preparation for independent fences remove the function and do all of
> it's cleanup directly after signaling.
>
> 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   | 79 +++++++------------
>   .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.h   |  3 -
>   3 files changed, 31 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index c6546a858597..1b15b51dc3f4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -3162,11 +3162,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;
>   
>   	amdgpu_register_atpx_handler();
>   	amdgpu_acpi_detect();
> @@ -3182,12 +3178,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)
> @@ -3197,7 +3187,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 a58342c2ac44..909bdccc2a92 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -32,29 +32,9 @@
>   #include "amdgpu.h"
>   #include "amdgpu_userq_fence.h"
>   
> -static const struct dma_fence_ops amdgpu_userq_fence_ops;
> -static struct kmem_cache *amdgpu_userq_fence_slab;
> -
>   #define AMDGPU_USERQ_MAX_HANDLES	(1U << 16)
>   
> -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);
Are we not having benefit enough to continue create a cache here ? If 
that is fine that LGTM,

Acked-by: Sunil Khatri <sunil.khatri@amd.com>

Regards
Sunil



> -	if (!amdgpu_userq_fence_slab)
> -		return -ENOMEM;
> -
> -	return 0;
> -}
> -
> -void amdgpu_userq_fence_slab_fini(void)
> -{
> -	rcu_barrier();
> -	kmem_cache_destroy(amdgpu_userq_fence_slab);
> -}
> +static const struct dma_fence_ops amdgpu_userq_fence_ops;
>   
>   static inline struct amdgpu_userq_fence *to_amdgpu_userq_fence(struct dma_fence *f)
>   {
> @@ -146,12 +126,18 @@ amdgpu_userq_fence_driver_free(struct amdgpu_usermode_queue *userq)
>   }
>   
>   static void
> -amdgpu_userq_fence_put_fence_drv_array(struct amdgpu_userq_fence *userq_fence)
> +amdgpu_userq_fence_put_fence_drv_refs(struct amdgpu_userq_fence *userq_fence)
>   {
>   	unsigned long i;
> +
>   	for (i = 0; i < userq_fence->fence_drv_array_count; i++)
>   		amdgpu_userq_fence_driver_put(userq_fence->fence_drv_array[i]);
>   	userq_fence->fence_drv_array_count = 0;
> +	kfree(userq_fence->fence_drv_array);
> +	userq_fence->fence_drv_array = NULL;
> +
> +	amdgpu_userq_fence_driver_put(userq_fence->fence_drv);
> +	userq_fence->fence_drv = NULL;
>   }
>   
>   void amdgpu_userq_fence_driver_process(struct amdgpu_userq_fence_driver *fence_drv)
> @@ -181,10 +167,11 @@ void amdgpu_userq_fence_driver_process(struct amdgpu_userq_fence_driver *fence_d
>   		fence = &userq_fence->base;
>   		list_del_init(&userq_fence->link);
>   		dma_fence_signal(fence);
> -		/* Drop fence_drv_array outside fence_list_lock
> +		/*
> +		 * Drop fence_drv_array outside fence_list_lock
>   		 * to avoid the recursion lock.
>   		 */
> -		amdgpu_userq_fence_put_fence_drv_array(userq_fence);
> +		amdgpu_userq_fence_put_fence_drv_refs(userq_fence);
>   		dma_fence_put(fence);
>   	}
>   
> @@ -231,7 +218,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);
>   	return *userq_fence ? 0 : -ENOMEM;
>   }
>   
> @@ -299,7 +286,7 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq,
>   	spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags);
>   
>   	if (signaled)
> -		amdgpu_userq_fence_put_fence_drv_array(userq_fence);
> +		amdgpu_userq_fence_put_fence_drv_refs(userq_fence);
>   
>   	*f = fence;
>   
> @@ -333,29 +320,10 @@ static bool amdgpu_userq_fence_signaled(struct dma_fence *f)
>   	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,
>   };
>   
>   /**
> @@ -546,7 +514,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;
>   	}
>   
> @@ -871,6 +839,7 @@ amdgpu_userq_wait_return_fence_info(struct drm_file *filp,
>   	for (i = 0, cnt = 0; i < num_fences; i++) {
>   		struct amdgpu_userq_fence_driver *fence_drv;
>   		struct amdgpu_userq_fence *userq_fence;
> +		unsigned long flags;
>   		u32 index;
>   
>   		userq_fence = to_amdgpu_userq_fence(fences[i]);
> @@ -886,7 +855,19 @@ amdgpu_userq_wait_return_fence_info(struct drm_file *filp,
>   			continue;
>   		}
>   
> +		spin_lock_irqsave(userq_fence->base.lock, flags);
> +		if (dma_fence_is_signaled_locked(&userq_fence->base)) {
> +			/*
> +			 * It is possible that fence is already signaled and the
> +			 * fence_drv now NULL, just skip over such fences.
> +			*/
> +			spin_unlock_irqrestore(userq_fence->base.lock, flags);
> +			continue;
> +		}
>   		fence_drv = userq_fence->fence_drv;
> +		amdgpu_userq_fence_driver_get(fence_drv);
> +		spin_unlock_irqrestore(userq_fence->base.lock, flags);
> +
>   		/*
>   		 * We need to make sure the user queue release their reference
>   		 * to the fence drivers at some point before queue destruction.
> @@ -895,10 +876,10 @@ amdgpu_userq_wait_return_fence_info(struct drm_file *filp,
>   		 */
>   		r = xa_alloc(&waitq->fence_drv_xa, &index, fence_drv,
>   			     xa_limit_32b, GFP_KERNEL);
> -		if (r)
> +		if (r) {
> +			amdgpu_userq_fence_driver_put(fence_drv);
>   			goto put_waitq;
> -
> -		amdgpu_userq_fence_driver_get(fence_drv);
> +		}
>   
>   		/* Store drm syncobj's gpu va address and value */
>   		fence_info[cnt].va = fence_drv->va;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h
> index d56246ad8c26..d355a0eecc07 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h
> @@ -58,9 +58,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,

[-- Attachment #2: Type: text/html, Size: 8894 bytes --]