[PATCH] drm/amdgpu: drop userq fence driver refs out of fence process()

[PATCH] drm/amdgpu: drop userq fence driver refs out of fence process()

[Prike Liang]

amdgpu_userq_wait_ioctl() takes extra references on waited-on fence
drivers and stores them in waitq->fence_drv_xa. When a new userq fence is
created, those references are transferred into userq_fence->fence_drv_array
so they can be released when the fence completes.

However, those inherited references are currently only dropped from
amdgpu_userq_fence_driver_process(). If a fence never reaches that path,
such as it is already signaled when created, so we need to explicitly release
those fences in that case.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 54 +++++++++++++------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
index 3be80a82788a..f132295890c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
@@ -145,35 +145,52 @@ amdgpu_userq_fence_driver_free(struct amdgpu_usermode_queue *userq)
 	amdgpu_userq_fence_driver_put(userq->fence_drv);
 }
 
+static void
+amdgpu_userq_fence_put_fence_drv_array(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;
+}
+
 void amdgpu_userq_fence_driver_process(struct amdgpu_userq_fence_driver *fence_drv)
 {
 	struct amdgpu_userq_fence *userq_fence, *tmp;
 	struct dma_fence *fence;
 	unsigned long flags;
 	u64 rptr;
-	int i;
 
 	if (!fence_drv)
 		return;
 
-	spin_lock_irqsave(&fence_drv->fence_list_lock, flags);
-	rptr = amdgpu_userq_fence_read(fence_drv);
-
-	list_for_each_entry_safe(userq_fence, tmp, &fence_drv->fences, link) {
-		fence = &userq_fence->base;
+	for (;;) {
+		spin_lock_irqsave(&fence_drv->fence_list_lock, flags);
+		rptr = amdgpu_userq_fence_read(fence_drv);
+		userq_fence = NULL;
+		fence = NULL;
 
-		if (rptr < fence->seqno)
-			break;
+		list_for_each_entry_safe(userq_fence, tmp, &fence_drv->fences, link) {
+			fence = &userq_fence->base;
 
-		dma_fence_signal(fence);
+			if (rptr < fence->seqno) {
+				userq_fence = NULL;
+				fence = NULL;
+				break;
+			}
 
-		for (i = 0; i < userq_fence->fence_drv_array_count; i++)
-			amdgpu_userq_fence_driver_put(userq_fence->fence_drv_array[i]);
+			dma_fence_signal(fence);
+			list_del(&userq_fence->link);
+			break;
+		}
+		spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags);
 
-		list_del(&userq_fence->link);
+		if (!fence)
+			break;
+		/* Drop fence_drv_array outside fence_list_lock to avoid the recursion lock. */
+		amdgpu_userq_fence_put_fence_drv_array(userq_fence);
 		dma_fence_put(fence);
 	}
-	spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags);
 }
 
 void amdgpu_userq_fence_driver_destroy(struct kref *ref)
@@ -228,6 +245,7 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq,
 	struct amdgpu_userq_fence_driver *fence_drv;
 	struct dma_fence *fence;
 	unsigned long flags;
+	bool signaled = false;
 
 	fence_drv = userq->fence_drv;
 	if (!fence_drv)
@@ -275,13 +293,17 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq,
 
 	/* Check if hardware has already processed the job */
 	spin_lock_irqsave(&fence_drv->fence_list_lock, flags);
-	if (!dma_fence_is_signaled(fence))
+	if (!dma_fence_is_signaled(fence)) {
 		list_add_tail(&userq_fence->link, &fence_drv->fences);
-	else
+	} else {
+		signaled = true;
 		dma_fence_put(fence);
-
+	}
 	spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags);
 
+	if (signaled)
+		amdgpu_userq_fence_put_fence_drv_array(userq_fence);
+
 	*f = fence;
 
 	return 0;
-- 
2.34.1

Re: [PATCH] drm/amdgpu: drop userq fence driver refs out of fence process()

[Christian König]

On 4/13/26 08:17, Prike Liang wrote:
> amdgpu_userq_wait_ioctl() takes extra references on waited-on fence
> drivers and stores them in waitq->fence_drv_xa. When a new userq fence is
> created, those references are transferred into userq_fence->fence_drv_array
> so they can be released when the fence completes.
> 
> However, those inherited references are currently only dropped from
> amdgpu_userq_fence_driver_process(). If a fence never reaches that path,
> such as it is already signaled when created, so we need to explicitly release
> those fences in that case.
> 
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 54 +++++++++++++------
>  1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index 3be80a82788a..f132295890c0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -145,35 +145,52 @@ amdgpu_userq_fence_driver_free(struct amdgpu_usermode_queue *userq)
>  	amdgpu_userq_fence_driver_put(userq->fence_drv);
>  }
>  
> +static void
> +amdgpu_userq_fence_put_fence_drv_array(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;
> +}
> +
>  void amdgpu_userq_fence_driver_process(struct amdgpu_userq_fence_driver *fence_drv)
>  {
>  	struct amdgpu_userq_fence *userq_fence, *tmp;
>  	struct dma_fence *fence;
>  	unsigned long flags;
>  	u64 rptr;
> -	int i;
>  
>  	if (!fence_drv)
>  		return;
>  
> -	spin_lock_irqsave(&fence_drv->fence_list_lock, flags);
> -	rptr = amdgpu_userq_fence_read(fence_drv);
> -
> -	list_for_each_entry_safe(userq_fence, tmp, &fence_drv->fences, link) {
> -		fence = &userq_fence->base;
> +	for (;;) {
> +		spin_lock_irqsave(&fence_drv->fence_list_lock, flags);
> +		rptr = amdgpu_userq_fence_read(fence_drv);
> +		userq_fence = NULL;
> +		fence = NULL;
>  
> -		if (rptr < fence->seqno)
> -			break;
> +		list_for_each_entry_safe(userq_fence, tmp, &fence_drv->fences, link) {
> +			fence = &userq_fence->base;
>  
> -		dma_fence_signal(fence);
> +			if (rptr < fence->seqno) {
> +				userq_fence = NULL;
> +				fence = NULL;
> +				break;
> +			}
>  
> -		for (i = 0; i < userq_fence->fence_drv_array_count; i++)
> -			amdgpu_userq_fence_driver_put(userq_fence->fence_drv_array[i]);
> +			dma_fence_signal(fence);
> +			list_del(&userq_fence->link);
> +			break;
> +		}
> +		spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags);
>  
> -		list_del(&userq_fence->link);
> +		if (!fence)
> +			break;
> +		/* Drop fence_drv_array outside fence_list_lock to avoid the recursion lock. */

That is a reasonable, but the resulting code looks ugly like hell and very inefficient.

Instead do it this way:

list_head *to_be_signaled;

rptr = amdgpu_userq_fence_read(fence_drv);
spin_lock_irqsave(&fence_drv->fence_list_lock, flags);

list_for_each_entry(userq_fence, tmp, &fence_drv->fences, link) {
	if (reptr < userq_fence->base.seq)
		break;
}

/* Move everything which needs to be signaled to the to_be_signaled list */
list_cut_before(&to_be_signaled, &fence_drv->fences, &userq_fence->link);

spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags);

list_for_each_entry_safe(userq_fence, tmp, &to_be_signaled, link) {
	....
}

regards,
Christian.


> +		amdgpu_userq_fence_put_fence_drv_array(userq_fence);
>  		dma_fence_put(fence);
>  	}
> -	spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags);
>  }
>  
>  void amdgpu_userq_fence_driver_destroy(struct kref *ref)
> @@ -228,6 +245,7 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq,
>  	struct amdgpu_userq_fence_driver *fence_drv;
>  	struct dma_fence *fence;
>  	unsigned long flags;
> +	bool signaled = false;
>  
>  	fence_drv = userq->fence_drv;
>  	if (!fence_drv)
> @@ -275,13 +293,17 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq,
>  
>  	/* Check if hardware has already processed the job */
>  	spin_lock_irqsave(&fence_drv->fence_list_lock, flags);
> -	if (!dma_fence_is_signaled(fence))
> +	if (!dma_fence_is_signaled(fence)) {
>  		list_add_tail(&userq_fence->link, &fence_drv->fences);
> -	else
> +	} else {
> +		signaled = true;
>  		dma_fence_put(fence);
> -
> +	}
>  	spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags);
>  
> +	if (signaled)
> +		amdgpu_userq_fence_put_fence_drv_array(userq_fence);
> +
>  	*f = fence;
>  
>  	return 0;