On 3/27/26 11:36, Prike Liang wrote:
> amdgpu_userq_fence_driver_free() is now responsible only for releasing
> per-queue ancillary state (last_fence, fence_drv_xa) and no longer
> touches the ownership reference, making each function's contract clear.
>
> Signed-off-by: Prike Liang <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 5 +++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 3 ---
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index c4841df80bf8..d676f2709a0c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -459,6 +459,9 @@ static void amdgpu_userq_cleanup(struct
> amdgpu_usermode_queue *queue)
> amdgpu_userq_buffer_vas_list_cleanup(adev, queue);
> uq_funcs->mqd_destroy(queue);
> amdgpu_userq_fence_driver_free(queue);
> + /* Drop the queue's ownership reference to fence_drv explicitly */
> + amdgpu_userq_fence_driver_put(queue->fence_drv);
> + queue->fence_drv = NULL;
> /* Use interrupt-safe locking since IRQ handlers may access these
> XArrays */
> xa_erase_irq(&adev->userq_doorbell_xa, queue->doorbell_index);
The xa_erase_irq() call must come first and then dropping the fence_drv
reference.
> queue->userq_mgr = NULL;
> @@ -866,6 +869,8 @@ amdgpu_userq_create(struct drm_file *filp, union
> drm_amdgpu_userq *args)
> up_read(&adev->reset_domain->sem);
> clean_fence_driver:
> amdgpu_userq_fence_driver_free(queue);
> + /* Pair with kref_init in amdgpu_userq_fence_driver_alloc */
It would be much cleaner if we would modify amdgpu_userq_fence_driver_alloc()
to get struct amdgpu_userq_fence_driver **out as parameter and then call it
like this:
ret = amdgpu_userq_fence_driver_alloc(adev, &userq->fence_drv);
This would make it absolutely clear what happens here and why we have to drop
the queue->fence_drv reference in case of an error.
> + amdgpu_userq_fence_driver_put(queue->fence_drv);
> free_queue:
> kfree(queue);
> unlock:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index 87560c1251d8..a392ef9ba5aa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -134,11 +134,8 @@ void
> amdgpu_userq_fence_driver_free(struct amdgpu_usermode_queue *userq)
> {
> dma_fence_put(userq->last_fence);
> -
> amdgpu_userq_walk_and_drop_fence_drv(&userq->fence_drv_xa);
> xa_destroy(&userq->fence_drv_xa);
> - /* Drop the fence_drv reference held by user queue */
> - amdgpu_userq_fence_driver_put(userq->fence_drv);
I would keep that inside here, it already looks like the right place to have it.
We just need to make sure that amdgpu_userq_fence_driver_free() is called
*after* xa_erase_irq().
Regards,
Christian.
> }
>
> void amdgpu_userq_fence_driver_process(struct amdgpu_userq_fence_driver
> *fence_drv)
