[Patch v1 0/4] Bunch of patches to do trivial clean up and

[Patch v1 0/4] Bunch of patches to do trivial clean up and

[Sunil Khatri]

Sunil Khatri (4):
  drm/amdgpu/userq: no need to use local variable ret
  drm/amdgpu/userq: no need to use local variable here for return
  drm/amdgpu/userq: no need to use local variable here for return
  drm/amdgpu/userq: Fix the code alignment for readability

 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 31 +++++++++--------------
 1 file changed, 12 insertions(+), 19 deletions(-)

-- 
2.34.1

[Patch v1 1/4] drm/amdgpu/userq: no need to use local variable ret

[Sunil Khatri]

In function amdgpu_userq_evict use the function return
value in the if condition instead.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index aa0e6eea9436..2a1832fce6d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -1308,17 +1308,13 @@ void
 amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr)
 {
 	struct amdgpu_device *adev = uq_mgr->adev;
-	int ret;
 
 	/* Wait for any pending userqueue fence work to finish */
-	ret = amdgpu_userq_wait_for_signal(uq_mgr);
-	if (ret)
+	if (amdgpu_userq_wait_for_signal(uq_mgr))
 		dev_err(adev->dev, "Not evicting userqueue, timeout waiting for work\n");
 
-	ret = amdgpu_userq_evict_all(uq_mgr);
-	if (ret)
+	if (amdgpu_userq_evict_all(uq_mgr))
 		dev_err(adev->dev, "Failed to evict userqueue\n");
-
 }
 
 int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr, struct drm_file *file_priv,
-- 
2.34.1

[Patch v1 2/4] drm/amdgpu/userq: no need to use local variable here for return

[Sunil Khatri]

In function amdgpu_userq_restore_worker use directly
the function's return value in the if condition instead
of local variable ret.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 2a1832fce6d2..2b07c3941927 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -1222,20 +1222,17 @@ static void amdgpu_userq_restore_worker(struct work_struct *work)
 	struct amdgpu_userq_mgr *uq_mgr = work_to_uq_mgr(work, resume_work.work);
 	struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr);
 	struct dma_fence *ev_fence;
-	int ret;
 
 	ev_fence = amdgpu_evf_mgr_get_fence(&fpriv->evf_mgr);
 	if (!dma_fence_is_signaled(ev_fence))
 		goto put_fence;
 
-	ret = amdgpu_userq_vm_validate(uq_mgr);
-	if (ret) {
+	if (amdgpu_userq_vm_validate(uq_mgr)) {
 		drm_file_err(uq_mgr->file, "Failed to validate BOs to restore\n");
 		goto put_fence;
 	}
 
-	ret = amdgpu_userq_restore_all(uq_mgr);
-	if (ret)
+	if (amdgpu_userq_restore_all(uq_mgr))
 		drm_file_err(uq_mgr->file, "Failed to restore all queues\n");
 
 put_fence:
-- 
2.34.1

[Patch v1 3/4] drm/amdgpu/userq: no need to use local variable here for return

[Sunil Khatri]

In function amdgpu_userq_gem_va_unmap_validate use function
return value directly in the if condition instead
of local variable ret.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 2b07c3941927..48cb2e21ce56 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -1478,7 +1478,6 @@ int amdgpu_userq_gem_va_unmap_validate(struct amdgpu_device *adev,
 	u32 ip_mask = amdgpu_userq_get_supported_ip_mask(adev);
 	struct amdgpu_bo_va *bo_va = mapping->bo_va;
 	struct dma_resv *resv = bo_va->base.bo->tbo.base.resv;
-	int ret = 0;
 
 	if (!ip_mask)
 		return 0;
@@ -1493,9 +1492,8 @@ int amdgpu_userq_gem_va_unmap_validate(struct amdgpu_device *adev,
 	 * the eviction fence is always unsignaled.
 	 */
 	if (!dma_resv_test_signaled(resv, DMA_RESV_USAGE_BOOKKEEP)) {
-		ret = dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP, true,
-					    MAX_SCHEDULE_TIMEOUT);
-		if (ret <= 0)
+		if (dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP, true,
+					    MAX_SCHEDULE_TIMEOUT) <= 0)
 			return -EBUSY;
 	}
 
-- 
2.34.1

[Patch v1 4/4] drm/amdgpu/userq: Fix the code alignment for readability

[Sunil Khatri]

Fix the code alignment for if condition and also provide
a line space between multiline if condition and next
statement.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 48cb2e21ce56..7cdfe3adcbae 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -1454,17 +1454,19 @@ int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
 
 	if (!adev->userq_halt_for_enforce_isolation)
 		dev_warn(adev->dev, "userq scheduling already started!\n");
+
 	adev->userq_halt_for_enforce_isolation = false;
+
 	xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {
 		uqm = queue->userq_mgr;
 		mutex_lock(&uqm->userq_mutex);
-			if (((queue->queue_type == AMDGPU_HW_IP_GFX) ||
-			     (queue->queue_type == AMDGPU_HW_IP_COMPUTE)) &&
-			    (queue->xcp_id == idx)) {
+		if (((queue->queue_type == AMDGPU_HW_IP_GFX) ||
+		     (queue->queue_type == AMDGPU_HW_IP_COMPUTE)) &&
+		    (queue->xcp_id == idx)) {
 			r = amdgpu_userq_restore_helper(queue);
 			if (r)
 				ret = r;
-			}
+		}
 		mutex_unlock(&uqm->userq_mutex);
 	}
 
-- 
2.34.1

Re: [Patch v1 1/4] drm/amdgpu/userq: no need to use local variable ret

[Christian König]

On 3/26/26 09:55, Sunil Khatri wrote:
> In function amdgpu_userq_evict use the function return
> value in the if condition instead.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index aa0e6eea9436..2a1832fce6d2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -1308,17 +1308,13 @@ void
>  amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr)
>  {
>  	struct amdgpu_device *adev = uq_mgr->adev;
> -	int ret;
>  
>  	/* Wait for any pending userqueue fence work to finish */
> -	ret = amdgpu_userq_wait_for_signal(uq_mgr);
> -	if (ret)
> +	if (amdgpu_userq_wait_for_signal(uq_mgr))
>  		dev_err(adev->dev, "Not evicting userqueue, timeout waiting for work\n");

That actually looks like a pretty bad idea. Instead we should start printing the error code.

But before we do that I would rather like to know why amdgpu_userq_wait_for_signal() can fail?

That should never happen in the first place.

Regards,
Christian.

>  
> -	ret = amdgpu_userq_evict_all(uq_mgr);
> -	if (ret)
> +	if (amdgpu_userq_evict_all(uq_mgr))
>  		dev_err(adev->dev, "Failed to evict userqueue\n");
> -
>  }
>  
>  int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr, struct drm_file *file_priv,

Re: [Patch v1 2/4] drm/amdgpu/userq: no need to use local variable here for return

[Christian König]

On 3/26/26 09:55, Sunil Khatri wrote:
> In function amdgpu_userq_restore_worker use directly
> the function's return value in the if condition instead
> of local variable ret.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 2a1832fce6d2..2b07c3941927 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -1222,20 +1222,17 @@ static void amdgpu_userq_restore_worker(struct work_struct *work)
>  	struct amdgpu_userq_mgr *uq_mgr = work_to_uq_mgr(work, resume_work.work);
>  	struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr);
>  	struct dma_fence *ev_fence;
> -	int ret;
>  
>  	ev_fence = amdgpu_evf_mgr_get_fence(&fpriv->evf_mgr);
>  	if (!dma_fence_is_signaled(ev_fence))
>  		goto put_fence;
>  
> -	ret = amdgpu_userq_vm_validate(uq_mgr);
> -	if (ret) {
> +	if (amdgpu_userq_vm_validate(uq_mgr)) {
>  		drm_file_err(uq_mgr->file, "Failed to validate BOs to restore\n");

Again, probably a good idea to print the error code here.

Regards,
Christian.

>  		goto put_fence;
>  	}
>  
> -	ret = amdgpu_userq_restore_all(uq_mgr);
> -	if (ret)
> +	if (amdgpu_userq_restore_all(uq_mgr))
>  		drm_file_err(uq_mgr->file, "Failed to restore all queues\n");
>  
>  put_fence:

Re: [Patch v1 3/4] drm/amdgpu/userq: no need to use local variable here for return

[Christian König]

On 3/26/26 09:56, Sunil Khatri wrote:
> In function amdgpu_userq_gem_va_unmap_validate use function
> return value directly in the if condition instead
> of local variable ret.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 2b07c3941927..48cb2e21ce56 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -1478,7 +1478,6 @@ int amdgpu_userq_gem_va_unmap_validate(struct amdgpu_device *adev,
>  	u32 ip_mask = amdgpu_userq_get_supported_ip_mask(adev);
>  	struct amdgpu_bo_va *bo_va = mapping->bo_va;
>  	struct dma_resv *resv = bo_va->base.bo->tbo.base.resv;
> -	int ret = 0;
>  
>  	if (!ip_mask)
>  		return 0;
> @@ -1493,9 +1492,8 @@ int amdgpu_userq_gem_va_unmap_validate(struct amdgpu_device *adev,
>  	 * the eviction fence is always unsignaled.
>  	 */
>  	if (!dma_resv_test_signaled(resv, DMA_RESV_USAGE_BOOKKEEP)) {

That test is just nonsense, call dma_resv_wait_timeout() directly here.

> -		ret = dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP, true,
> -					    MAX_SCHEDULE_TIMEOUT);
> -		if (ret <= 0)
> +		if (dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP, true,
> +					    MAX_SCHEDULE_TIMEOUT) <= 0)
>  			return -EBUSY;

That is wrong as well. We need to return ret here and not -EBUSY!

Regards,
Christian.

>  	}
>  

Re: [Patch v1 4/4] drm/amdgpu/userq: Fix the code alignment for readability

[Christian König]

On 3/26/26 09:56, Sunil Khatri wrote:
> Fix the code alignment for if condition and also provide
> a line space between multiline if condition and next
> statement.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 48cb2e21ce56..7cdfe3adcbae 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -1454,17 +1454,19 @@ int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
>  
>  	if (!adev->userq_halt_for_enforce_isolation)
>  		dev_warn(adev->dev, "userq scheduling already started!\n");
> +
>  	adev->userq_halt_for_enforce_isolation = false;
> +
>  	xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {
>  		uqm = queue->userq_mgr;
>  		mutex_lock(&uqm->userq_mutex);
> -			if (((queue->queue_type == AMDGPU_HW_IP_GFX) ||
> -			     (queue->queue_type == AMDGPU_HW_IP_COMPUTE)) &&
> -			    (queue->xcp_id == idx)) {
> +		if (((queue->queue_type == AMDGPU_HW_IP_GFX) ||
> +		     (queue->queue_type == AMDGPU_HW_IP_COMPUTE)) &&
> +		    (queue->xcp_id == idx)) {
>  			r = amdgpu_userq_restore_helper(queue);
>  			if (r)
>  				ret = r;
> -			}
> +		}
>  		mutex_unlock(&uqm->userq_mutex);
>  	}
>  

Re: [Patch v1 1/4] drm/amdgpu/userq: no need to use local variable ret

[Khatri, Sunil]

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


On 26-03-2026 05:38 pm, Christian König wrote:
> On 3/26/26 09:55, Sunil Khatri wrote:
>> In function amdgpu_userq_evict use the function return
>> value in the if condition instead.
>>
>> Signed-off-by: Sunil Khatri<sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> index aa0e6eea9436..2a1832fce6d2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> @@ -1308,17 +1308,13 @@ void
>>   amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr)
>>   {
>>   	struct amdgpu_device *adev = uq_mgr->adev;
>> -	int ret;
>>   
>>   	/* Wait for any pending userqueue fence work to finish */
>> -	ret = amdgpu_userq_wait_for_signal(uq_mgr);
>> -	if (ret)
>> +	if (amdgpu_userq_wait_for_signal(uq_mgr))
>>   		dev_err(adev->dev, "Not evicting userqueue, timeout waiting for work\n");
> That actually looks like a pretty bad idea. Instead we should start printing the error code.
Sure could add an error code in the logging.
>
> But before we do that I would rather like to know why amdgpu_userq_wait_for_signal() can fail?
dma_fence_wait_timeout is what could fail and we are returning 
-ETIMEDOUT. We could totally avoid checking for the error here 
completely as we are already printing the error in
the called function below.
ret=dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
if(ret<=0) {
drm_file_err(uq_mgr->file, "Timed out waiting for fence=%llu:%llu\n",
f->context, f->seqno);
return-ETIMEDOUT;
                 }
> That should never happen in the first place.
>
> Regards,
> Christian.
>
>>   
>> -	ret = amdgpu_userq_evict_all(uq_mgr);
>> -	if (ret)
>> +	if (amdgpu_userq_evict_all(uq_mgr))
>>   		dev_err(adev->dev, "Failed to evict userqueue\n");
Here also the below function returns error and printing error too. We 
could avoid the return value here too as we are already printing the error.
amdgpu_userq_preempt_helper(queue);
if(r)
ret=r;


Regards
Sunil Khatri

>> -
>>   }
>>   
>>   int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr, struct drm_file *file_priv,

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

Re: [Patch v1 2/4] drm/amdgpu/userq: no need to use local variable here for return

[Khatri, Sunil]

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


On 26-03-2026 05:39 pm, Christian König wrote:
>
> On 3/26/26 09:55, Sunil Khatri wrote:
>> In function amdgpu_userq_restore_worker use directly
>> the function's return value in the if condition instead
>> of local variable ret.
>>
>> Signed-off-by: Sunil Khatri<sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> index 2a1832fce6d2..2b07c3941927 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> @@ -1222,20 +1222,17 @@ static void amdgpu_userq_restore_worker(struct work_struct *work)
>>   	struct amdgpu_userq_mgr *uq_mgr = work_to_uq_mgr(work, resume_work.work);
>>   	struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr);
>>   	struct dma_fence *ev_fence;
>> -	int ret;
>>   
>>   	ev_fence = amdgpu_evf_mgr_get_fence(&fpriv->evf_mgr);
>>   	if (!dma_fence_is_signaled(ev_fence))
>>   		goto put_fence;
>>   
>> -	ret = amdgpu_userq_vm_validate(uq_mgr);
>> -	if (ret) {
>> +	if (amdgpu_userq_vm_validate(uq_mgr)) {
>>   		drm_file_err(uq_mgr->file, "Failed to validate BOs to restore\n");
> Again, probably a good idea to print the error code here.
since this is a void function and all the functions called here are 
already printing the failures and we could just remove the if and let 
the called function print error.
>
> Regards,
> Christian.
>
>>   		goto put_fence;
>>   	}
>>   
>> -	ret = amdgpu_userq_restore_all(uq_mgr);
>> -	if (ret)
>> +	if (amdgpu_userq_restore_all(uq_mgr))
>>   		drm_file_err(uq_mgr->file, "Failed to restore all queues\n");
Could avoid printing this error all together as we are printing the 
error in the function. We could update the error message.
if(ret)
drm_file_err(uq_mgr->file, "Failed to map all the queues\n");
returnret;


>>   
>>   put_fence:

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

Re: [Patch v1 3/4] drm/amdgpu/userq: no need to use local variable here for return

[Khatri, Sunil]

On 26-03-2026 05:41 pm, Christian König wrote:
>
> On 3/26/26 09:56, Sunil Khatri wrote:
>> In function amdgpu_userq_gem_va_unmap_validate use function
>> return value directly in the if condition instead
>> of local variable ret.
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> index 2b07c3941927..48cb2e21ce56 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> @@ -1478,7 +1478,6 @@ int amdgpu_userq_gem_va_unmap_validate(struct amdgpu_device *adev,
>>   	u32 ip_mask = amdgpu_userq_get_supported_ip_mask(adev);
>>   	struct amdgpu_bo_va *bo_va = mapping->bo_va;
>>   	struct dma_resv *resv = bo_va->base.bo->tbo.base.resv;
>> -	int ret = 0;
>>   
>>   	if (!ip_mask)
>>   		return 0;
>> @@ -1493,9 +1492,8 @@ int amdgpu_userq_gem_va_unmap_validate(struct amdgpu_device *adev,
>>   	 * the eviction fence is always unsignaled.
>>   	 */
>>   	if (!dma_resv_test_signaled(resv, DMA_RESV_USAGE_BOOKKEEP)) {
> That test is just nonsense, call dma_resv_wait_timeout() directly here.
Make sense.
>
>> -		ret = dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP, true,
>> -					    MAX_SCHEDULE_TIMEOUT);
>> -		if (ret <= 0)
>> +		if (dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP, true,
>> +					    MAX_SCHEDULE_TIMEOUT) <= 0)
>>   			return -EBUSY;
> That is wrong as well. We need to return ret here and not -EBUSY!

sure

Regards
Sunil Khatri

>
> Regards,
> Christian.
>
>>   	}
>>   

Re: [Patch v1 1/4] drm/amdgpu/userq: no need to use local variable ret

[Christian König]

On 3/26/26 13:26, Khatri, Sunil wrote:
> 
> On 26-03-2026 05:38 pm, Christian König wrote:
>> On 3/26/26 09:55, Sunil Khatri wrote:
>>> In function amdgpu_userq_evict use the function return
>>> value in the if condition instead.
>>>
>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index aa0e6eea9436..2a1832fce6d2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -1308,17 +1308,13 @@ void
>>>  amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr)
>>>  {
>>>  	struct amdgpu_device *adev = uq_mgr->adev;
>>> -	int ret;
>>>  
>>>  	/* Wait for any pending userqueue fence work to finish */
>>> -	ret = amdgpu_userq_wait_for_signal(uq_mgr);
>>> -	if (ret)
>>> +	if (amdgpu_userq_wait_for_signal(uq_mgr))
>>>  		dev_err(adev->dev, "Not evicting userqueue, timeout waiting for work\n");
>> That actually looks like a pretty bad idea. Instead we should start printing the error code.
> Sure could add an error code in the logging.
>> But before we do that I would rather like to know why amdgpu_userq_wait_for_signal() can fail?
> dma_fence_wait_timeout is what could fail and we are returning -ETIMEDOUT. We could totally avoid checking for the error here completely as we are already printing the error in
> the called function below.
> ret=dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));

*sigh* I explicitly NAKed that timeout before. Please change that to the maximum.

Additional to that please drop the extra call to dma_fence_is_signaled() before the wait, such stuff is completely superfluous.

Then the second parameter to dma_fence_wait_timeout should be false, this way we can't be interrupted any more and don't need to check the return value for errors.

Thanks for take a look at that,
Christian.

>                 if(ret<=0) {
>                         drm_file_err(uq_mgr->file, "Timed out waiting for fence=%llu:%llu\n",
>                                      f->context, f->seqno);
>         return-ETIMEDOUT;
>                 }
>> That should never happen in the first place.
>>
>> Regards,
>> Christian.
>>
>>>  
>>> -	ret = amdgpu_userq_evict_all(uq_mgr);
>>> -	if (ret)
>>> +	if (amdgpu_userq_evict_all(uq_mgr))
>>>  		dev_err(adev->dev, "Failed to evict userqueue\n");
> Here also the below function returns error and printing error too. We could avoid the return value here too as we are already printing the error.
> amdgpu_userq_preempt_helper(queue);
>                 if(r)
>                         ret=r;
> 
> 
> Regards
> Sunil Khatri
> 
>>> -
>>>  }
>>>  
>>>  int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr, struct drm_file *file_priv,

Re: [Patch v1 2/4] drm/amdgpu/userq: no need to use local variable here for return

[Christian König]

On 3/26/26 13:30, Khatri, Sunil wrote:
> 
> On 26-03-2026 05:39 pm, Christian König wrote:
>> On 3/26/26 09:55, Sunil Khatri wrote:
>>> In function amdgpu_userq_restore_worker use directly
>>> the function's return value in the if condition instead
>>> of local variable ret.
>>>
>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 7 ++-----
>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index 2a1832fce6d2..2b07c3941927 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -1222,20 +1222,17 @@ static void amdgpu_userq_restore_worker(struct work_struct *work)
>>>  	struct amdgpu_userq_mgr *uq_mgr = work_to_uq_mgr(work, resume_work.work);
>>>  	struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr);
>>>  	struct dma_fence *ev_fence;
>>> -	int ret;
>>>  
>>>  	ev_fence = amdgpu_evf_mgr_get_fence(&fpriv->evf_mgr);
>>>  	if (!dma_fence_is_signaled(ev_fence))
>>>  		goto put_fence;
>>>  
>>> -	ret = amdgpu_userq_vm_validate(uq_mgr);
>>> -	if (ret) {
>>> +	if (amdgpu_userq_vm_validate(uq_mgr)) {
>>>  		drm_file_err(uq_mgr->file, "Failed to validate BOs to restore\n");
>> Again, probably a good idea to print the error code here.
> since this is a void function and all the functions called here are already printing the failures and we could just remove the if and let the called function print error.
>> Regards,
>> Christian.
>>
>>>  		goto put_fence;
>>>  	}
>>>  
>>> -	ret = amdgpu_userq_restore_all(uq_mgr);
>>> -	if (ret)
>>> +	if (amdgpu_userq_restore_all(uq_mgr))
>>>  		drm_file_err(uq_mgr->file, "Failed to restore all queues\n");
> Could avoid printing this error all together as we are printing the error in the function. We could update the error message.

Yeah that works for me as well.

Thanks,
Christian.

> if(ret)
>                 drm_file_err(uq_mgr->file, "Failed to map all the queues\n");
>         returnret;
> 
> 
>>>  
>>>  put_fence: