[PATCH v3 1/4] drm: add function drm_file_err to print proc information too

[PATCH v3 1/4] drm: add function drm_file_err to print proc information too

[Sunil Khatri]

Add a drm helper function which get the process information for
the drm_file and append the process information using the existing
drm_err.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 include/drm/drm_file.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 94d365b22505..e329299a2b2c 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -37,6 +37,7 @@
 #include <uapi/drm/drm.h>
 
 #include <drm/drm_prime.h>
+#include <drm/drm_print.h>
 
 struct dma_fence;
 struct drm_file;
@@ -446,6 +447,45 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
 	return file_priv->minor->type == DRM_MINOR_ACCEL;
 }
 
+/**
+ * drm_file_err - Fill info string with process name and pid
+ * @file_priv: context of interest for process name and pid
+ * @fmt: prinf() like format string
+ *
+ * This update the user provided buffer with process
+ * name and pid information for @file_priv
+ */
+__printf(2, 3)
+static inline void drm_file_err(struct drm_file *file_priv, const char *fmt, ...)
+{
+	struct task_struct *task;
+	struct pid *pid;
+	struct drm_device *dev = file_priv->minor->dev;
+	char new_fmt[256];
+	char final_fmt[512];
+	va_list args;
+
+	mutex_lock(&file_priv->client_name_lock);
+	rcu_read_lock();
+	pid = rcu_dereference(file_priv->pid);
+	task = pid_task(pid, PIDTYPE_TGID);
+
+	if (drm_WARN_ON_ONCE(dev, !task))
+		return;
+
+	snprintf(new_fmt, sizeof(new_fmt), "proc:%s pid:%d client_name:%s %s",
+		task->comm, task->pid, file_priv->client_name ?: "Unset", fmt);
+
+	va_start(args, fmt);
+	vsnprintf(final_fmt, sizeof(final_fmt), new_fmt, args);
+
+	drm_err(dev, "%s", final_fmt);
+	va_end(args);
+
+	rcu_read_unlock();
+	mutex_unlock(&file_priv->client_name_lock);
+}
+
 void drm_file_update_pid(struct drm_file *);
 
 struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
-- 
2.34.1

[PATCH v3 2/4] drm/amdgpu: add drm_file reference in userq_mgr

[Sunil Khatri]

drm_file will be used in usermode queues code to
enable better process information in logging and hence
add drm_file part of the userq_mgr struct.

update the drm_file pointer in userq_mgr for each
amdgpu_driver_open_kms.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c       | 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 3d319687c1c9..3de3071d66ee 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1436,6 +1436,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 
 	amdgpu_ctx_mgr_init(&fpriv->ctx_mgr, adev);
 
+	fpriv->userq_mgr.file = file_priv;
 	r = amdgpu_userq_mgr_init(&fpriv->userq_mgr, adev);
 	if (r)
 		DRM_WARN("Can't setup usermode queues, use legacy workload submission only\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
index 381b9c6f0573..fe51a45f7ee4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
@@ -77,6 +77,7 @@ struct amdgpu_userq_mgr {
 	struct amdgpu_device		*adev;
 	struct delayed_work		resume_work;
 	struct list_head		list;
+	struct drm_file			*file;
 };
 
 struct amdgpu_db_info {
-- 
2.34.1

Re: [PATCH v3 2/4] drm/amdgpu: add drm_file reference in userq_mgr

[Tvrtko Ursulin]

On 15/04/2025 19:43, Sunil Khatri wrote:
> drm_file will be used in usermode queues code to
> enable better process information in logging and hence
> add drm_file part of the userq_mgr struct.
> 
> update the drm_file pointer in userq_mgr for each
> amdgpu_driver_open_kms.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c       | 1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 3d319687c1c9..3de3071d66ee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1436,6 +1436,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>   
>   	amdgpu_ctx_mgr_init(&fpriv->ctx_mgr, adev);
>   
> +	fpriv->userq_mgr.file = file_priv;
>   	r = amdgpu_userq_mgr_init(&fpriv->userq_mgr, adev);

It's a bit of a layering violation since amdgpu_userq_mgr_init() is the 
place which otherwise initialises fpriv->user_mgr. One day someome might 
put a memset in there for example. Anyway, I think it would be nicer if 
you passed fpriv to that function. Potentially instead of adev. Looks 
like that would be cleaner "design".

Regards,

Tvrtko

>   	if (r)
>   		DRM_WARN("Can't setup usermode queues, use legacy workload submission only\n");
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
> index 381b9c6f0573..fe51a45f7ee4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
> @@ -77,6 +77,7 @@ struct amdgpu_userq_mgr {
>   	struct amdgpu_device		*adev;
>   	struct delayed_work		resume_work;
>   	struct list_head		list;
> +	struct drm_file			*file;
>   };
>   
>   struct amdgpu_db_info {

Re: [PATCH v3 2/4] drm/amdgpu: add drm_file reference in userq_mgr

[Khatri, Sunil]

On 4/16/2025 12:59 PM, Tvrtko Ursulin wrote:
>
> On 15/04/2025 19:43, Sunil Khatri wrote:
>> drm_file will be used in usermode queues code to
>> enable better process information in logging and hence
>> add drm_file part of the userq_mgr struct.
>>
>> update the drm_file pointer in userq_mgr for each
>> amdgpu_driver_open_kms.
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c       | 1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 3d319687c1c9..3de3071d66ee 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -1436,6 +1436,7 @@ int amdgpu_driver_open_kms(struct drm_device 
>> *dev, struct drm_file *file_priv)
>>         amdgpu_ctx_mgr_init(&fpriv->ctx_mgr, adev);
>>   +    fpriv->userq_mgr.file = file_priv;
>>       r = amdgpu_userq_mgr_init(&fpriv->userq_mgr, adev);
>
> It's a bit of a layering violation since amdgpu_userq_mgr_init() is 
> the place which otherwise initialises fpriv->user_mgr. One day someome 
> might put a memset in there for example. Anyway, I think it would be 
> nicer if you passed fpriv to that function. Potentially instead of 
> adev. Looks like that would be cleaner "design".
>
I agree totally this should be inside amdgpu_userq_mgr_init with fpriv 
passed to function. But i guess whoever wrote it in first place thought 
to make it same as done in a line above fot ctx_mgr. Once we have these 
patches merge i will push these fixes separately.

Regards
Sunil Khatri

> Regards,
>
> Tvrtko
>
>>       if (r)
>>           DRM_WARN("Can't setup usermode queues, use legacy workload 
>> submission only\n");
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
>> index 381b9c6f0573..fe51a45f7ee4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
>> @@ -77,6 +77,7 @@ struct amdgpu_userq_mgr {
>>       struct amdgpu_device        *adev;
>>       struct delayed_work        resume_work;
>>       struct list_head        list;
>> +    struct drm_file            *file;
>>   };
>>     struct amdgpu_db_info {
>

[PATCH v3 3/4] drm/amdgpu: use drm_file_err in logging to also dump process information

[Sunil Khatri]

add process and pid information in the userqueue error
logging to make it more useful in resolving the error
by logs.

Sample log:
[   42.444297] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] *ERROR* Timed out waiting for fence f=000000001c74d978 for comm:Xwayland pid:3427
[   42.444669] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not suspending userqueue, timeout waiting for comm:Xwayland pid:3427
[   42.824729] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] *ERROR* Timed out waiting for fence f=0000000074407d3e for comm:systemd-logind pid:1058
[   42.825082] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not suspending userqueue, timeout waiting for comm:systemd-logind pid:1058

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
index 1867520ba258..05c1ee27a319 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -43,7 +43,7 @@ amdgpu_userqueue_cleanup(struct amdgpu_userq_mgr *uq_mgr,
 	if (f && !dma_fence_is_signaled(f)) {
 		ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
 		if (ret <= 0) {
-			DRM_ERROR("Timed out waiting for fence f=%p\n", f);
+			drm_file_err(uq_mgr->file, "Timed out waiting for fence f=%p\n", f);
 			return;
 		}
 	}
@@ -440,7 +440,8 @@ amdgpu_userqueue_resume_all(struct amdgpu_userq_mgr *uq_mgr)
 	}
 
 	if (ret)
-		DRM_ERROR("Failed to map all the queues\n");
+		drm_file_err(uq_mgr->file, "Failed to map all the queue\n");
+
 	return ret;
 }
 
@@ -598,7 +599,8 @@ amdgpu_userqueue_suspend_all(struct amdgpu_userq_mgr *uq_mgr)
 	}
 
 	if (ret)
-		DRM_ERROR("Couldn't unmap all the queues\n");
+		drm_file_err(uq_mgr->file, "Couldn't unmap all the queues\n");
+
 	return ret;
 }
 
@@ -615,7 +617,7 @@ amdgpu_userqueue_wait_for_signal(struct amdgpu_userq_mgr *uq_mgr)
 			continue;
 		ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
 		if (ret <= 0) {
-			DRM_ERROR("Timed out waiting for fence f=%p\n", f);
+			drm_file_err(uq_mgr->file, "Timed out waiting for fence f=%p\n", f);
 			return -ETIMEDOUT;
 		}
 	}
@@ -634,13 +636,13 @@ amdgpu_userqueue_suspend(struct amdgpu_userq_mgr *uq_mgr,
 	/* Wait for any pending userqueue fence work to finish */
 	ret = amdgpu_userqueue_wait_for_signal(uq_mgr);
 	if (ret) {
-		DRM_ERROR("Not suspending userqueue, timeout waiting for work\n");
+		drm_file_err(uq_mgr->file, "Not suspending userqueue, timeout waiting\n");
 		return;
 	}
 
 	ret = amdgpu_userqueue_suspend_all(uq_mgr);
 	if (ret) {
-		DRM_ERROR("Failed to evict userqueue\n");
+		drm_file_err(uq_mgr->file, "Failed to evict userqueue\n");
 		return;
 	}
 
-- 
2.34.1

Re: [PATCH v3 3/4] drm/amdgpu: use drm_file_err in logging to also dump process information

[Tvrtko Ursulin]

On 15/04/2025 19:43, Sunil Khatri wrote:
> add process and pid information in the userqueue error
> logging to make it more useful in resolving the error
> by logs.
> 
> Sample log:
> [   42.444297] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] *ERROR* Timed out waiting for fence f=000000001c74d978 for comm:Xwayland pid:3427
> [   42.444669] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not suspending userqueue, timeout waiting for comm:Xwayland pid:3427
> [   42.824729] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] *ERROR* Timed out waiting for fence f=0000000074407d3e for comm:systemd-logind pid:1058
> [   42.825082] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not suspending userqueue, timeout waiting for comm:systemd-logind pid:1058
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> index 1867520ba258..05c1ee27a319 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> @@ -43,7 +43,7 @@ amdgpu_userqueue_cleanup(struct amdgpu_userq_mgr *uq_mgr,
>   	if (f && !dma_fence_is_signaled(f)) {
>   		ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
>   		if (ret <= 0) {
> -			DRM_ERROR("Timed out waiting for fence f=%p\n", f);
> +			drm_file_err(uq_mgr->file, "Timed out waiting for fence f=%p\n", f);

You decided to leave %p after all?

>   			return;
>   		}
>   	}
> @@ -440,7 +440,8 @@ amdgpu_userqueue_resume_all(struct amdgpu_userq_mgr *uq_mgr)
>   	}
>   
>   	if (ret)
> -		DRM_ERROR("Failed to map all the queues\n");
> +		drm_file_err(uq_mgr->file, "Failed to map all the queue\n");

You lost the plural by accident.

I am also not sure "all the queues" makes sense in this context versus 
"all queues" but it's inconsequential really.

> +
>   	return ret;
>   }
>   
> @@ -598,7 +599,8 @@ amdgpu_userqueue_suspend_all(struct amdgpu_userq_mgr *uq_mgr)
>   	}
>   
>   	if (ret)
> -		DRM_ERROR("Couldn't unmap all the queues\n");
> +		drm_file_err(uq_mgr->file, "Couldn't unmap all the queues\n");
> +
>   	return ret;
>   }
>   
> @@ -615,7 +617,7 @@ amdgpu_userqueue_wait_for_signal(struct amdgpu_userq_mgr *uq_mgr)
>   			continue;
>   		ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
>   		if (ret <= 0) {
> -			DRM_ERROR("Timed out waiting for fence f=%p\n", f);
> +			drm_file_err(uq_mgr->file, "Timed out waiting for fence f=%p\n", f);
>   			return -ETIMEDOUT;
>   		}
>   	}
> @@ -634,13 +636,13 @@ amdgpu_userqueue_suspend(struct amdgpu_userq_mgr *uq_mgr,
>   	/* Wait for any pending userqueue fence work to finish */
>   	ret = amdgpu_userqueue_wait_for_signal(uq_mgr);
>   	if (ret) {
> -		DRM_ERROR("Not suspending userqueue, timeout waiting for work\n");
> +		drm_file_err(uq_mgr->file, "Not suspending userqueue, timeout waiting\n");
>   		return;
>   	}
>   
>   	ret = amdgpu_userqueue_suspend_all(uq_mgr);
>   	if (ret) {
> -		DRM_ERROR("Failed to evict userqueue\n");
> +		drm_file_err(uq_mgr->file, "Failed to evict userqueue\n");
>   		return;

It is pre-existing but strikes me as odd that failure to 
amdgpu_userqueue_suspend_all() logs a failure to *evict* instead of 
suspend (as the previous log does). Anyway, I did not look at the 
surrounding code so just thinking out loud.

Regards,

Tvrtko

>   	}
>   

Re: [PATCH v3 3/4] drm/amdgpu: use drm_file_err in logging to also dump process information

[Khatri, Sunil]

On 4/16/2025 12:56 PM, Tvrtko Ursulin wrote:
>
> On 15/04/2025 19:43, Sunil Khatri wrote:
>> add process and pid information in the userqueue error
>> logging to make it more useful in resolving the error
>> by logs.
>>
>> Sample log:
>> [   42.444297] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] 
>> *ERROR* Timed out waiting for fence f=000000001c74d978 for 
>> comm:Xwayland pid:3427
>> [   42.444669] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not 
>> suspending userqueue, timeout waiting for comm:Xwayland pid:3427
>> [   42.824729] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] 
>> *ERROR* Timed out waiting for fence f=0000000074407d3e for 
>> comm:systemd-logind pid:1058
>> [   42.825082] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not 
>> suspending userqueue, timeout waiting for comm:systemd-logind pid:1058
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>> index 1867520ba258..05c1ee27a319 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>> @@ -43,7 +43,7 @@ amdgpu_userqueue_cleanup(struct amdgpu_userq_mgr 
>> *uq_mgr,
>>       if (f && !dma_fence_is_signaled(f)) {
>>           ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
>>           if (ret <= 0) {
>> -            DRM_ERROR("Timed out waiting for fence f=%p\n", f);
>> +            drm_file_err(uq_mgr->file, "Timed out waiting for fence 
>> f=%p\n", f);
>
> You decided to leave %p after all?

Yes we are printing the fence ptr here to see which fence is timing out. 
Anyways right now intention of this patch is to add additional process 
information along with existing information like fence here.

regards
Sunil

>
>>               return;
>>           }
>>       }
>> @@ -440,7 +440,8 @@ amdgpu_userqueue_resume_all(struct 
>> amdgpu_userq_mgr *uq_mgr)
>>       }
>>         if (ret)
>> -        DRM_ERROR("Failed to map all the queues\n");
>> +        drm_file_err(uq_mgr->file, "Failed to map all the queue\n");
>
> You lost the plural by accident.
Yes i will add 's'. Noted.
>
I am also not sure "all the queues" makes sense in this context versus 
"all queues" but it's inconsequential really.
Regards
Sunil
> Yes it all queues from a uq_mgr.
>> +
>>       return ret;
>>   }
>>   @@ -598,7 +599,8 @@ amdgpu_userqueue_suspend_all(struct 
>> amdgpu_userq_mgr *uq_mgr)
>>       }
>>         if (ret)
>> -        DRM_ERROR("Couldn't unmap all the queues\n");
>> +        drm_file_err(uq_mgr->file, "Couldn't unmap all the queues\n");
>> +
>>       return ret;
>>   }
>>   @@ -615,7 +617,7 @@ amdgpu_userqueue_wait_for_signal(struct 
>> amdgpu_userq_mgr *uq_mgr)
>>               continue;
>>           ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
>>           if (ret <= 0) {
>> -            DRM_ERROR("Timed out waiting for fence f=%p\n", f);
>> +            drm_file_err(uq_mgr->file, "Timed out waiting for fence 
>> f=%p\n", f);
>>               return -ETIMEDOUT;
>>           }
>>       }
>> @@ -634,13 +636,13 @@ amdgpu_userqueue_suspend(struct 
>> amdgpu_userq_mgr *uq_mgr,
>>       /* Wait for any pending userqueue fence work to finish */
>>       ret = amdgpu_userqueue_wait_for_signal(uq_mgr);
>>       if (ret) {
>> -        DRM_ERROR("Not suspending userqueue, timeout waiting for 
>> work\n");
>> +        drm_file_err(uq_mgr->file, "Not suspending userqueue, 
>> timeout waiting\n");
>>           return;
>>       }
>>         ret = amdgpu_userqueue_suspend_all(uq_mgr);
>>       if (ret) {
>> -        DRM_ERROR("Failed to evict userqueue\n");
>> +        drm_file_err(uq_mgr->file, "Failed to evict userqueue\n");
>>           return;
>
> It is pre-existing but strikes me as odd that failure to 
> amdgpu_userqueue_suspend_all() logs a failure to *evict* instead of 
> suspend (as the previous log does). Anyway, I did not look at the 
> surrounding code so just thinking out loud.

Yes suspend failed as all the fences were not evicted and thats why 
suspend failed. Anyways there are already alex patches which will change 
this to unmap as a code reorganisation for suspend/resume is in pipeline.

regards

Sunil

>
> Regards,
>
> Tvrtko
>
>>       }
>

Re: [PATCH v3 3/4] drm/amdgpu: use drm_file_err in logging to also dump process information

[Pierre-Eric Pelloux-Prayer]

Hi,

Le 16/04/2025 à 12:01, Khatri, Sunil a écrit :
> 
> On 4/16/2025 12:56 PM, Tvrtko Ursulin wrote:
>>
>> On 15/04/2025 19:43, Sunil Khatri wrote:
>>> add process and pid information in the userqueue error
>>> logging to make it more useful in resolving the error
>>> by logs.
>>>
>>> Sample log:
>>> [   42.444297] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] *ERROR* Timed out waiting for 
>>> fence f=000000001c74d978 for comm:Xwayland pid:3427
>>> [   42.444669] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not suspending userqueue, timeout 
>>> waiting for comm:Xwayland pid:3427
>>> [   42.824729] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] *ERROR* Timed out waiting for 
>>> fence f=0000000074407d3e for comm:systemd-logind pid:1058
>>> [   42.825082] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not suspending userqueue, timeout 
>>> waiting for comm:systemd-logind pid:1058
>>>
>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 14 ++++++++------
>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/ 
>>> amdgpu_userqueue.c
>>> index 1867520ba258..05c1ee27a319 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>> @@ -43,7 +43,7 @@ amdgpu_userqueue_cleanup(struct amdgpu_userq_mgr *uq_mgr,
>>>       if (f && !dma_fence_is_signaled(f)) {
>>>           ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
>>>           if (ret <= 0) {
>>> -            DRM_ERROR("Timed out waiting for fence f=%p\n", f);
>>> +            drm_file_err(uq_mgr->file, "Timed out waiting for fence f=%p\n", f);
>>
>> You decided to leave %p after all?
> 
> Yes we are printing the fence ptr here to see which fence is timing out. Anyways right now intention 
> of this patch is to add additional process information along with existing information like fence here.
> 

I agree with Tvrtko, "fence=%llu:%llu" would be better to identify "which fence is timing out".


Pierre-Eric


> regards
> Sunil
> 
>>
>>>               return;
>>>           }
>>>       }
>>> @@ -440,7 +440,8 @@ amdgpu_userqueue_resume_all(struct amdgpu_userq_mgr *uq_mgr)
>>>       }
>>>         if (ret)
>>> -        DRM_ERROR("Failed to map all the queues\n");
>>> +        drm_file_err(uq_mgr->file, "Failed to map all the queue\n");
>>
>> You lost the plural by accident.
> Yes i will add 's'. Noted.
>>
> I am also not sure "all the queues" makes sense in this context versus "all queues" but it's 
> inconsequential really.
> Regards
> Sunil
>> Yes it all queues from a uq_mgr.
>>> +
>>>       return ret;
>>>   }
>>>   @@ -598,7 +599,8 @@ amdgpu_userqueue_suspend_all(struct amdgpu_userq_mgr *uq_mgr)
>>>       }
>>>         if (ret)
>>> -        DRM_ERROR("Couldn't unmap all the queues\n");
>>> +        drm_file_err(uq_mgr->file, "Couldn't unmap all the queues\n");
>>> +
>>>       return ret;
>>>   }
>>>   @@ -615,7 +617,7 @@ amdgpu_userqueue_wait_for_signal(struct amdgpu_userq_mgr *uq_mgr)
>>>               continue;
>>>           ret = dma_fence_wait_timeout(f, true, msecs_to_jiffies(100));
>>>           if (ret <= 0) {
>>> -            DRM_ERROR("Timed out waiting for fence f=%p\n", f);
>>> +            drm_file_err(uq_mgr->file, "Timed out waiting for fence f=%p\n", f);
>>>               return -ETIMEDOUT;
>>>           }
>>>       }
>>> @@ -634,13 +636,13 @@ amdgpu_userqueue_suspend(struct amdgpu_userq_mgr *uq_mgr,
>>>       /* Wait for any pending userqueue fence work to finish */
>>>       ret = amdgpu_userqueue_wait_for_signal(uq_mgr);
>>>       if (ret) {
>>> -        DRM_ERROR("Not suspending userqueue, timeout waiting for work\n");
>>> +        drm_file_err(uq_mgr->file, "Not suspending userqueue, timeout waiting\n");
>>>           return;
>>>       }
>>>         ret = amdgpu_userqueue_suspend_all(uq_mgr);
>>>       if (ret) {
>>> -        DRM_ERROR("Failed to evict userqueue\n");
>>> +        drm_file_err(uq_mgr->file, "Failed to evict userqueue\n");
>>>           return;
>>
>> It is pre-existing but strikes me as odd that failure to amdgpu_userqueue_suspend_all() logs a 
>> failure to *evict* instead of suspend (as the previous log does). Anyway, I did not look at the 
>> surrounding code so just thinking out loud.
> 
> Yes suspend failed as all the fences were not evicted and thats why suspend failed. Anyways there 
> are already alex patches which will change this to unmap as a code reorganisation for suspend/resume 
> is in pipeline.
> 
> regards
> 
> Sunil
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>>       }
>>

Re: [PATCH v3 3/4] drm/amdgpu: use drm_file_err in logging to also dump process information

[Khatri, Sunil]

On 4/16/2025 5:37 PM, Pierre-Eric Pelloux-Prayer wrote:
> Hi,
>
> Le 16/04/2025 à 12:01, Khatri, Sunil a écrit :
>>
>> On 4/16/2025 12:56 PM, Tvrtko Ursulin wrote:
>>>
>>> On 15/04/2025 19:43, Sunil Khatri wrote:
>>>> add process and pid information in the userqueue error
>>>> logging to make it more useful in resolving the error
>>>> by logs.
>>>>
>>>> Sample log:
>>>> [   42.444297] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] 
>>>> *ERROR* Timed out waiting for fence f=000000001c74d978 for 
>>>> comm:Xwayland pid:3427
>>>> [   42.444669] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not 
>>>> suspending userqueue, timeout waiting for comm:Xwayland pid:3427
>>>> [   42.824729] [drm:amdgpu_userqueue_wait_for_signal [amdgpu]] 
>>>> *ERROR* Timed out waiting for fence f=0000000074407d3e for 
>>>> comm:systemd-logind pid:1058
>>>> [   42.825082] [drm:amdgpu_userqueue_suspend [amdgpu]] *ERROR* Not 
>>>> suspending userqueue, timeout waiting for comm:systemd-logind pid:1058
>>>>
>>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 14 ++++++++------
>>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/ amdgpu_userqueue.c
>>>> index 1867520ba258..05c1ee27a319 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>>> @@ -43,7 +43,7 @@ amdgpu_userqueue_cleanup(struct amdgpu_userq_mgr 
>>>> *uq_mgr,
>>>>       if (f && !dma_fence_is_signaled(f)) {
>>>>           ret = dma_fence_wait_timeout(f, true, 
>>>> msecs_to_jiffies(100));
>>>>           if (ret <= 0) {
>>>> -            DRM_ERROR("Timed out waiting for fence f=%p\n", f);
>>>> +            drm_file_err(uq_mgr->file, "Timed out waiting for 
>>>> fence f=%p\n", f);
>>>
>>> You decided to leave %p after all?
>>
>> Yes we are printing the fence ptr here to see which fence is timing 
>> out. Anyways right now intention of this patch is to add additional 
>> process information along with existing information like fence here.
>>
>
> I agree with Tvrtko, "fence=%llu:%llu" would be better to identify 
> "which fence is timing out".

I agree to it for sure, just that there are other places also where we 
are printing fence ptr and will take that up in another patch.

Regards
Sunil Khatri

>
>
> Pierre-Eric
>
>
>> regards
>> Sunil
>>
>>>
>>>>               return;
>>>>           }
>>>>       }
>>>> @@ -440,7 +440,8 @@ amdgpu_userqueue_resume_all(struct 
>>>> amdgpu_userq_mgr *uq_mgr)
>>>>       }
>>>>         if (ret)
>>>> -        DRM_ERROR("Failed to map all the queues\n");
>>>> +        drm_file_err(uq_mgr->file, "Failed to map all the queue\n");
>>>
>>> You lost the plural by accident.
>> Yes i will add 's'. Noted.
>>>
>> I am also not sure "all the queues" makes sense in this context 
>> versus "all queues" but it's inconsequential really.
>> Regards
>> Sunil
>>> Yes it all queues from a uq_mgr.
>>>> +
>>>>       return ret;
>>>>   }
>>>>   @@ -598,7 +599,8 @@ amdgpu_userqueue_suspend_all(struct 
>>>> amdgpu_userq_mgr *uq_mgr)
>>>>       }
>>>>         if (ret)
>>>> -        DRM_ERROR("Couldn't unmap all the queues\n");
>>>> +        drm_file_err(uq_mgr->file, "Couldn't unmap all the 
>>>> queues\n");
>>>> +
>>>>       return ret;
>>>>   }
>>>>   @@ -615,7 +617,7 @@ amdgpu_userqueue_wait_for_signal(struct 
>>>> amdgpu_userq_mgr *uq_mgr)
>>>>               continue;
>>>>           ret = dma_fence_wait_timeout(f, true, 
>>>> msecs_to_jiffies(100));
>>>>           if (ret <= 0) {
>>>> -            DRM_ERROR("Timed out waiting for fence f=%p\n", f);
>>>> +            drm_file_err(uq_mgr->file, "Timed out waiting for 
>>>> fence f=%p\n", f);
>>>>               return -ETIMEDOUT;
>>>>           }
>>>>       }
>>>> @@ -634,13 +636,13 @@ amdgpu_userqueue_suspend(struct 
>>>> amdgpu_userq_mgr *uq_mgr,
>>>>       /* Wait for any pending userqueue fence work to finish */
>>>>       ret = amdgpu_userqueue_wait_for_signal(uq_mgr);
>>>>       if (ret) {
>>>> -        DRM_ERROR("Not suspending userqueue, timeout waiting for 
>>>> work\n");
>>>> +        drm_file_err(uq_mgr->file, "Not suspending userqueue, 
>>>> timeout waiting\n");
>>>>           return;
>>>>       }
>>>>         ret = amdgpu_userqueue_suspend_all(uq_mgr);
>>>>       if (ret) {
>>>> -        DRM_ERROR("Failed to evict userqueue\n");
>>>> +        drm_file_err(uq_mgr->file, "Failed to evict userqueue\n");
>>>>           return;
>>>
>>> It is pre-existing but strikes me as odd that failure to 
>>> amdgpu_userqueue_suspend_all() logs a failure to *evict* instead of 
>>> suspend (as the previous log does). Anyway, I did not look at the 
>>> surrounding code so just thinking out loud.
>>
>> Yes suspend failed as all the fences were not evicted and thats why 
>> suspend failed. Anyways there are already alex patches which will 
>> change this to unmap as a code reorganisation for suspend/resume is 
>> in pipeline.
>>
>> regards
>>
>> Sunil
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>       }
>>>

[PATCH v3 4/4] drm/amdgpu: change DRM_ERROR to drm_file_err in amdgpu_userqueue.c

[Sunil Khatri]

change the DRM_ERROR to drm_file_err which gives the drm device
information too which is useful in case of multiple GPU's and also
add process information.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 59 +++++++++++--------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
index 05c1ee27a319..e07dff14256c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -123,25 +123,25 @@ int amdgpu_userqueue_create_object(struct amdgpu_userq_mgr *uq_mgr,
 
 	r = amdgpu_bo_create(adev, &bp, &userq_obj->obj);
 	if (r) {
-		DRM_ERROR("Failed to allocate BO for userqueue (%d)", r);
+		drm_file_err(uq_mgr->file, "Failed to allocate BO for userqueue (%d)", r);
 		return r;
 	}
 
 	r = amdgpu_bo_reserve(userq_obj->obj, true);
 	if (r) {
-		DRM_ERROR("Failed to reserve BO to map (%d)", r);
+		drm_file_err(uq_mgr->file, "Failed to reserve BO to map (%d)", r);
 		goto free_obj;
 	}
 
 	r = amdgpu_ttm_alloc_gart(&(userq_obj->obj)->tbo);
 	if (r) {
-		DRM_ERROR("Failed to alloc GART for userqueue object (%d)", r);
+		drm_file_err(uq_mgr->file, "Failed to alloc GART for userqueue object (%d)", r);
 		goto unresv;
 	}
 
 	r = amdgpu_bo_kmap(userq_obj->obj, &userq_obj->cpu_ptr);
 	if (r) {
-		DRM_ERROR("Failed to map BO for userqueue (%d)", r);
+		drm_file_err(uq_mgr->file, "Failed to map BO for userqueue (%d)", r);
 		goto unresv;
 	}
 
@@ -177,7 +177,7 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,
 
 	gobj = drm_gem_object_lookup(filp, db_info->doorbell_handle);
 	if (gobj == NULL) {
-		DRM_ERROR("Can't find GEM object for doorbell\n");
+		drm_file_err(uq_mgr->file, "Can't find GEM object for doorbell\n");
 		return -EINVAL;
 	}
 
@@ -187,13 +187,15 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,
 	/* Pin the BO before generating the index, unpin in queue destroy */
 	r = amdgpu_bo_pin(db_obj->obj, AMDGPU_GEM_DOMAIN_DOORBELL);
 	if (r) {
-		DRM_ERROR("[Usermode queues] Failed to pin doorbell object\n");
+		drm_file_err(uq_mgr->file,
+			"[Usermode queues] Failed to pin doorbell object\n");
 		goto unref_bo;
 	}
 
 	r = amdgpu_bo_reserve(db_obj->obj, true);
 	if (r) {
-		DRM_ERROR("[Usermode queues] Failed to pin doorbell object\n");
+		drm_file_err(uq_mgr->file,
+			"[Usermode queues] Failed to pin doorbell object\n");
 		goto unpin_bo;
 	}
 
@@ -215,14 +217,16 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,
 		break;
 
 	default:
-		DRM_ERROR("[Usermode queues] IP %d not support\n", db_info->queue_type);
+	drm_file_err(uq_mgr->file,
+			"[Usermode queues] IP %d not support\n", db_info->queue_type);
 		r = -EINVAL;
 		goto unpin_bo;
 	}
 
 	index = amdgpu_doorbell_index_on_bar(uq_mgr->adev, db_obj->obj,
 					     db_info->doorbell_offset, db_size);
-	DRM_DEBUG_DRIVER("[Usermode queues] doorbell index=%lld\n", index);
+	drm_dbg_driver(adev_to_drm(uq_mgr->adev),
+		       "[Usermode queues] doorbell index=%lld\n", index);
 	amdgpu_bo_unreserve(db_obj->obj);
 	return index;
 
@@ -249,7 +253,7 @@ amdgpu_userqueue_destroy(struct drm_file *filp, int queue_id)
 
 	queue = amdgpu_userqueue_find(uq_mgr, queue_id);
 	if (!queue) {
-		DRM_DEBUG_DRIVER("Invalid queue id to destroy\n");
+		drm_dbg_driver(adev_to_drm(uq_mgr->adev), "Invalid queue id to destroy\n");
 		mutex_unlock(&uq_mgr->userq_mutex);
 		return -EINVAL;
 	}
@@ -282,7 +286,8 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	if (args->in.ip_type != AMDGPU_HW_IP_GFX &&
 	    args->in.ip_type != AMDGPU_HW_IP_DMA &&
 	    args->in.ip_type != AMDGPU_HW_IP_COMPUTE) {
-		DRM_ERROR("Usermode queue doesn't support IP type %u\n", args->in.ip_type);
+		drm_file_err(uq_mgr->file,
+			"Usermode queue doesn't support IP type %u\n", args->in.ip_type);
 		return -EINVAL;
 	}
 
@@ -304,14 +309,16 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 
 	uq_funcs = adev->userq_funcs[args->in.ip_type];
 	if (!uq_funcs) {
-		DRM_ERROR("Usermode queue is not supported for this IP (%u)\n", args->in.ip_type);
+		drm_file_err(uq_mgr->file,
+			"Usermode queue is not supported for this IP (%u)\n",
+			args->in.ip_type);
 		r = -EINVAL;
 		goto unlock;
 	}
 
 	queue = kzalloc(sizeof(struct amdgpu_usermode_queue), GFP_KERNEL);
 	if (!queue) {
-		DRM_ERROR("Failed to allocate memory for queue\n");
+		drm_file_err(uq_mgr->file, "Failed to allocate memory for queue\n");
 		r = -ENOMEM;
 		goto unlock;
 	}
@@ -327,7 +334,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	/* Convert relative doorbell offset into absolute doorbell index */
 	index = amdgpu_userqueue_get_doorbell_index(uq_mgr, &db_info, filp);
 	if (index == (uint64_t)-EINVAL) {
-		DRM_ERROR("Failed to get doorbell for queue\n");
+		drm_file_err(uq_mgr->file, "Failed to get doorbell for queue\n");
 		kfree(queue);
 		goto unlock;
 	}
@@ -336,13 +343,13 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	xa_init_flags(&queue->fence_drv_xa, XA_FLAGS_ALLOC);
 	r = amdgpu_userq_fence_driver_alloc(adev, queue);
 	if (r) {
-		DRM_ERROR("Failed to alloc fence driver\n");
+		drm_file_err(uq_mgr->file, "Failed to alloc fence driver\n");
 		goto unlock;
 	}
 
 	r = uq_funcs->mqd_create(uq_mgr, &args->in, queue);
 	if (r) {
-		DRM_ERROR("Failed to create Queue\n");
+		drm_file_err(uq_mgr->file, "Failed to create Queue\n");
 		amdgpu_userq_fence_driver_free(queue);
 		kfree(queue);
 		goto unlock;
@@ -350,7 +357,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 
 	qid = idr_alloc(&uq_mgr->userq_idr, queue, 1, AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
 	if (qid < 0) {
-		DRM_ERROR("Failed to allocate a queue id\n");
+		drm_file_err(uq_mgr->file, "Failed to allocate a queue id\n");
 		amdgpu_userq_fence_driver_free(queue);
 		uq_funcs->mqd_destroy(uq_mgr, queue);
 		kfree(queue);
@@ -360,7 +367,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 
 	r = uq_funcs->map(uq_mgr, queue);
 	if (r) {
-		DRM_ERROR("Failed to map Queue\n");
+		drm_file_err(uq_mgr->file, "Failed to map Queue\n");
 		idr_remove(&uq_mgr->userq_idr, qid);
 		amdgpu_userq_fence_driver_free(queue);
 		uq_funcs->mqd_destroy(uq_mgr, queue);
@@ -388,7 +395,7 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
 			return -EINVAL;
 		r = amdgpu_userqueue_create(filp, args);
 		if (r)
-			DRM_ERROR("Failed to create usermode queue\n");
+			drm_file_err(filp, "Failed to create usermode queue\n");
 		break;
 
 	case AMDGPU_USERQ_OP_FREE:
@@ -406,11 +413,11 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
 			return -EINVAL;
 		r = amdgpu_userqueue_destroy(filp, args->in.queue_id);
 		if (r)
-			DRM_ERROR("Failed to destroy usermode queue\n");
+			drm_file_err(filp, "Failed to destroy usermode queue\n");
 		break;
 
 	default:
-		DRM_DEBUG_DRIVER("Invalid user queue op specified: %d\n", args->in.op);
+		drm_dbg_driver(dev, "Invalid user queue op specified: %d\n", args->in.op);
 		return -EINVAL;
 	}
 
@@ -479,7 +486,7 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr)
 		ret = amdgpu_vm_lock_pd(vm, &exec, 2);
 		drm_exec_retry_on_contention(&exec);
 		if (unlikely(ret)) {
-			DRM_ERROR("Failed to lock PD\n");
+			drm_file_err(uq_mgr->file, "Failed to lock PD\n");
 			goto unlock_all;
 		}
 
@@ -519,7 +526,7 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr)
 		bo = bo_va->base.bo;
 		ret = amdgpu_userqueue_validate_vm_bo(NULL, bo);
 		if (ret) {
-			DRM_ERROR("Failed to validate BO\n");
+			drm_file_err(uq_mgr->file, "Failed to validate BO\n");
 			goto unlock_all;
 		}
 
@@ -550,7 +557,7 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr)
 
 	ret = amdgpu_eviction_fence_replace_fence(&fpriv->evf_mgr, &exec);
 	if (ret)
-		DRM_ERROR("Failed to replace eviction fence\n");
+		drm_file_err(uq_mgr->file, "Failed to replace eviction fence\n");
 
 unlock_all:
 	drm_exec_fini(&exec);
@@ -569,13 +576,13 @@ static void amdgpu_userqueue_resume_worker(struct work_struct *work)
 
 	ret = amdgpu_userqueue_validate_bos(uq_mgr);
 	if (ret) {
-		DRM_ERROR("Failed to validate BOs to restore\n");
+		drm_file_err(uq_mgr->file, "Failed to validate BOs to restore\n");
 		goto unlock;
 	}
 
 	ret = amdgpu_userqueue_resume_all(uq_mgr);
 	if (ret) {
-		DRM_ERROR("Failed to resume all queues\n");
+		drm_file_err(uq_mgr->file, "Failed to resume all queues\n");
 		goto unlock;
 	}
 
-- 
2.34.1

Re: [PATCH v3 4/4] drm/amdgpu: change DRM_ERROR to drm_file_err in amdgpu_userqueue.c

[Tvrtko Ursulin]

On 15/04/2025 19:43, Sunil Khatri wrote:
> change the DRM_ERROR to drm_file_err which gives the drm device
> information too which is useful in case of multiple GPU's and also
> add process information.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 59 +++++++++++--------
>   1 file changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> index 05c1ee27a319..e07dff14256c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> @@ -123,25 +123,25 @@ int amdgpu_userqueue_create_object(struct amdgpu_userq_mgr *uq_mgr,
>   
>   	r = amdgpu_bo_create(adev, &bp, &userq_obj->obj);
>   	if (r) {
> -		DRM_ERROR("Failed to allocate BO for userqueue (%d)", r);
> +		drm_file_err(uq_mgr->file, "Failed to allocate BO for userqueue (%d)", r);
>   		return r;
>   	}
>   
>   	r = amdgpu_bo_reserve(userq_obj->obj, true);
>   	if (r) {
> -		DRM_ERROR("Failed to reserve BO to map (%d)", r);
> +		drm_file_err(uq_mgr->file, "Failed to reserve BO to map (%d)", r);
>   		goto free_obj;
>   	}
>   
>   	r = amdgpu_ttm_alloc_gart(&(userq_obj->obj)->tbo);
>   	if (r) {
> -		DRM_ERROR("Failed to alloc GART for userqueue object (%d)", r);
> +		drm_file_err(uq_mgr->file, "Failed to alloc GART for userqueue object (%d)", r);
>   		goto unresv;
>   	}
>   
>   	r = amdgpu_bo_kmap(userq_obj->obj, &userq_obj->cpu_ptr);
>   	if (r) {
> -		DRM_ERROR("Failed to map BO for userqueue (%d)", r);
> +		drm_file_err(uq_mgr->file, "Failed to map BO for userqueue (%d)", r);
>   		goto unresv;
>   	}
>   
> @@ -177,7 +177,7 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,
>   
>   	gobj = drm_gem_object_lookup(filp, db_info->doorbell_handle);
>   	if (gobj == NULL) {
> -		DRM_ERROR("Can't find GEM object for doorbell\n");
> +		drm_file_err(uq_mgr->file, "Can't find GEM object for doorbell\n");
>   		return -EINVAL;
>   	}
>   
> @@ -187,13 +187,15 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,
>   	/* Pin the BO before generating the index, unpin in queue destroy */
>   	r = amdgpu_bo_pin(db_obj->obj, AMDGPU_GEM_DOMAIN_DOORBELL);
>   	if (r) {
> -		DRM_ERROR("[Usermode queues] Failed to pin doorbell object\n");
> +		drm_file_err(uq_mgr->file,
> +			"[Usermode queues] Failed to pin doorbell object\n");

Indentation could be off here (and a few more below), if it isn't my 
email client not displaying it properly.

>   		goto unref_bo;
>   	}
>   
>   	r = amdgpu_bo_reserve(db_obj->obj, true);
>   	if (r) {
> -		DRM_ERROR("[Usermode queues] Failed to pin doorbell object\n");
> +		drm_file_err(uq_mgr->file,
> +			"[Usermode queues] Failed to pin doorbell object\n");
>   		goto unpin_bo;
>   	}
>   
> @@ -215,14 +217,16 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,
>   		break;
>   
>   	default:
> -		DRM_ERROR("[Usermode queues] IP %d not support\n", db_info->queue_type);
> +	drm_file_err(uq_mgr->file,
> +			"[Usermode queues] IP %d not support\n", db_info->queue_type);
>   		r = -EINVAL;
>   		goto unpin_bo;
>   	}
>   
>   	index = amdgpu_doorbell_index_on_bar(uq_mgr->adev, db_obj->obj,
>   					     db_info->doorbell_offset, db_size);
> -	DRM_DEBUG_DRIVER("[Usermode queues] doorbell index=%lld\n", index);
> +	drm_dbg_driver(adev_to_drm(uq_mgr->adev),
> +		       "[Usermode queues] doorbell index=%lld\n", index);

This and others are technically okay but not what the commit message 
says. I'd say either split them into a separate patch or change the 
commit message to just say something like "Add device and client 
information to userq logging" so you give patch a wider mandate. ;)

>   	amdgpu_bo_unreserve(db_obj->obj);
>   	return index;
>   
> @@ -249,7 +253,7 @@ amdgpu_userqueue_destroy(struct drm_file *filp, int queue_id)
>   
>   	queue = amdgpu_userqueue_find(uq_mgr, queue_id);
>   	if (!queue) {
> -		DRM_DEBUG_DRIVER("Invalid queue id to destroy\n");
> +		drm_dbg_driver(adev_to_drm(uq_mgr->adev), "Invalid queue id to destroy\n");
>   		mutex_unlock(&uq_mgr->userq_mutex);
>   		return -EINVAL;
>   	}
> @@ -282,7 +286,8 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>   	if (args->in.ip_type != AMDGPU_HW_IP_GFX &&
>   	    args->in.ip_type != AMDGPU_HW_IP_DMA &&
>   	    args->in.ip_type != AMDGPU_HW_IP_COMPUTE) {
> -		DRM_ERROR("Usermode queue doesn't support IP type %u\n", args->in.ip_type);
> +		drm_file_err(uq_mgr->file,
> +			"Usermode queue doesn't support IP type %u\n", args->in.ip_type);
>   		return -EINVAL;
>   	}
>   
> @@ -304,14 +309,16 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>   
>   	uq_funcs = adev->userq_funcs[args->in.ip_type];
>   	if (!uq_funcs) {
> -		DRM_ERROR("Usermode queue is not supported for this IP (%u)\n", args->in.ip_type);
> +		drm_file_err(uq_mgr->file,
> +			"Usermode queue is not supported for this IP (%u)\n",
> +			args->in.ip_type);
>   		r = -EINVAL;
>   		goto unlock;
>   	}
>   
>   	queue = kzalloc(sizeof(struct amdgpu_usermode_queue), GFP_KERNEL);
>   	if (!queue) {
> -		DRM_ERROR("Failed to allocate memory for queue\n");
> +		drm_file_err(uq_mgr->file, "Failed to allocate memory for queue\n");
>   		r = -ENOMEM;
>   		goto unlock;
>   	}
> @@ -327,7 +334,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>   	/* Convert relative doorbell offset into absolute doorbell index */
>   	index = amdgpu_userqueue_get_doorbell_index(uq_mgr, &db_info, filp);
>   	if (index == (uint64_t)-EINVAL) {
> -		DRM_ERROR("Failed to get doorbell for queue\n");
> +		drm_file_err(uq_mgr->file, "Failed to get doorbell for queue\n");
>   		kfree(queue);
>   		goto unlock;
>   	}
> @@ -336,13 +343,13 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>   	xa_init_flags(&queue->fence_drv_xa, XA_FLAGS_ALLOC);
>   	r = amdgpu_userq_fence_driver_alloc(adev, queue);
>   	if (r) {
> -		DRM_ERROR("Failed to alloc fence driver\n");
> +		drm_file_err(uq_mgr->file, "Failed to alloc fence driver\n");
>   		goto unlock;
>   	}
>   
>   	r = uq_funcs->mqd_create(uq_mgr, &args->in, queue);
>   	if (r) {
> -		DRM_ERROR("Failed to create Queue\n");
> +		drm_file_err(uq_mgr->file, "Failed to create Queue\n");

My OCD is upset by inconsistencies of queue vs Queue and queue vs 
usermode queue vs user queue. Looks like a good opportunity to tidy 
things up while touching the lines.

>   		amdgpu_userq_fence_driver_free(queue);
>   		kfree(queue);
>   		goto unlock;
> @@ -350,7 +357,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>   
>   	qid = idr_alloc(&uq_mgr->userq_idr, queue, 1, AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
>   	if (qid < 0) {
> -		DRM_ERROR("Failed to allocate a queue id\n");
> +		drm_file_err(uq_mgr->file, "Failed to allocate a queue id\n");
>   		amdgpu_userq_fence_driver_free(queue);
>   		uq_funcs->mqd_destroy(uq_mgr, queue);
>   		kfree(queue);
> @@ -360,7 +367,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>   
>   	r = uq_funcs->map(uq_mgr, queue);
>   	if (r) {
> -		DRM_ERROR("Failed to map Queue\n");
> +		drm_file_err(uq_mgr->file, "Failed to map Queue\n");
>   		idr_remove(&uq_mgr->userq_idr, qid);
>   		amdgpu_userq_fence_driver_free(queue);
>   		uq_funcs->mqd_destroy(uq_mgr, queue);
> @@ -388,7 +395,7 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
>   			return -EINVAL;
>   		r = amdgpu_userqueue_create(filp, args);
>   		if (r)
> -			DRM_ERROR("Failed to create usermode queue\n");
> +			drm_file_err(filp, "Failed to create usermode queue\n");

Not really a kernel wide error if userspace passed invalid arguements to 
the ioctl. Usually it is good to avoid allowing userspace at will log 
spamming.

Regards,

Tvrtko

>   		break;
>   
>   	case AMDGPU_USERQ_OP_FREE:
> @@ -406,11 +413,11 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
>   			return -EINVAL;
>   		r = amdgpu_userqueue_destroy(filp, args->in.queue_id);
>   		if (r)
> -			DRM_ERROR("Failed to destroy usermode queue\n");
> +			drm_file_err(filp, "Failed to destroy usermode queue\n");
>   		break;
>   
>   	default:
> -		DRM_DEBUG_DRIVER("Invalid user queue op specified: %d\n", args->in.op);
> +		drm_dbg_driver(dev, "Invalid user queue op specified: %d\n", args->in.op);
>   		return -EINVAL;
>   	}
>   
> @@ -479,7 +486,7 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr)
>   		ret = amdgpu_vm_lock_pd(vm, &exec, 2);
>   		drm_exec_retry_on_contention(&exec);
>   		if (unlikely(ret)) {
> -			DRM_ERROR("Failed to lock PD\n");
> +			drm_file_err(uq_mgr->file, "Failed to lock PD\n");
>   			goto unlock_all;
>   		}
>   
> @@ -519,7 +526,7 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr)
>   		bo = bo_va->base.bo;
>   		ret = amdgpu_userqueue_validate_vm_bo(NULL, bo);
>   		if (ret) {
> -			DRM_ERROR("Failed to validate BO\n");
> +			drm_file_err(uq_mgr->file, "Failed to validate BO\n");
>   			goto unlock_all;
>   		}
>   
> @@ -550,7 +557,7 @@ amdgpu_userqueue_validate_bos(struct amdgpu_userq_mgr *uq_mgr)
>   
>   	ret = amdgpu_eviction_fence_replace_fence(&fpriv->evf_mgr, &exec);
>   	if (ret)
> -		DRM_ERROR("Failed to replace eviction fence\n");
> +		drm_file_err(uq_mgr->file, "Failed to replace eviction fence\n");
>   
>   unlock_all:
>   	drm_exec_fini(&exec);
> @@ -569,13 +576,13 @@ static void amdgpu_userqueue_resume_worker(struct work_struct *work)
>   
>   	ret = amdgpu_userqueue_validate_bos(uq_mgr);
>   	if (ret) {
> -		DRM_ERROR("Failed to validate BOs to restore\n");
> +		drm_file_err(uq_mgr->file, "Failed to validate BOs to restore\n");
>   		goto unlock;
>   	}
>   
>   	ret = amdgpu_userqueue_resume_all(uq_mgr);
>   	if (ret) {
> -		DRM_ERROR("Failed to resume all queues\n");
> +		drm_file_err(uq_mgr->file, "Failed to resume all queues\n");
>   		goto unlock;
>   	}
>   

Re: [PATCH v3 4/4] drm/amdgpu: change DRM_ERROR to drm_file_err in amdgpu_userqueue.c

[Khatri, Sunil]

On 4/16/2025 12:48 PM, Tvrtko Ursulin wrote:
>
> On 15/04/2025 19:43, Sunil Khatri wrote:
>> change the DRM_ERROR to drm_file_err which gives the drm device
>> information too which is useful in case of multiple GPU's and also
>> add process information.
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 59 +++++++++++--------
>>   1 file changed, 33 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>> index 05c1ee27a319..e07dff14256c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>> @@ -123,25 +123,25 @@ int amdgpu_userqueue_create_object(struct 
>> amdgpu_userq_mgr *uq_mgr,
>>         r = amdgpu_bo_create(adev, &bp, &userq_obj->obj);
>>       if (r) {
>> -        DRM_ERROR("Failed to allocate BO for userqueue (%d)", r);
>> +        drm_file_err(uq_mgr->file, "Failed to allocate BO for 
>> userqueue (%d)", r);
>>           return r;
>>       }
>>         r = amdgpu_bo_reserve(userq_obj->obj, true);
>>       if (r) {
>> -        DRM_ERROR("Failed to reserve BO to map (%d)", r);
>> +        drm_file_err(uq_mgr->file, "Failed to reserve BO to map 
>> (%d)", r);
>>           goto free_obj;
>>       }
>>         r = amdgpu_ttm_alloc_gart(&(userq_obj->obj)->tbo);
>>       if (r) {
>> -        DRM_ERROR("Failed to alloc GART for userqueue object (%d)", r);
>> +        drm_file_err(uq_mgr->file, "Failed to alloc GART for 
>> userqueue object (%d)", r);
>>           goto unresv;
>>       }
>>         r = amdgpu_bo_kmap(userq_obj->obj, &userq_obj->cpu_ptr);
>>       if (r) {
>> -        DRM_ERROR("Failed to map BO for userqueue (%d)", r);
>> +        drm_file_err(uq_mgr->file, "Failed to map BO for userqueue 
>> (%d)", r);
>>           goto unresv;
>>       }
>>   @@ -177,7 +177,7 @@ amdgpu_userqueue_get_doorbell_index(struct 
>> amdgpu_userq_mgr *uq_mgr,
>>         gobj = drm_gem_object_lookup(filp, db_info->doorbell_handle);
>>       if (gobj == NULL) {
>> -        DRM_ERROR("Can't find GEM object for doorbell\n");
>> +        drm_file_err(uq_mgr->file, "Can't find GEM object for 
>> doorbell\n");
>>           return -EINVAL;
>>       }
>>   @@ -187,13 +187,15 @@ amdgpu_userqueue_get_doorbell_index(struct 
>> amdgpu_userq_mgr *uq_mgr,
>>       /* Pin the BO before generating the index, unpin in queue 
>> destroy */
>>       r = amdgpu_bo_pin(db_obj->obj, AMDGPU_GEM_DOMAIN_DOORBELL);
>>       if (r) {
>> -        DRM_ERROR("[Usermode queues] Failed to pin doorbell object\n");
>> +        drm_file_err(uq_mgr->file,
>> +            "[Usermode queues] Failed to pin doorbell object\n");
>
> Indentation could be off here (and a few more below), if it isn't my 
> email client not displaying it properly.

Noted, will check again for indentation.

regards
Sunil

>
>>           goto unref_bo;
>>       }
>>         r = amdgpu_bo_reserve(db_obj->obj, true);
>>       if (r) {
>> -        DRM_ERROR("[Usermode queues] Failed to pin doorbell object\n");
>> +        drm_file_err(uq_mgr->file,
>> +            "[Usermode queues] Failed to pin doorbell object\n");
>>           goto unpin_bo;
>>       }
>>   @@ -215,14 +217,16 @@ amdgpu_userqueue_get_doorbell_index(struct 
>> amdgpu_userq_mgr *uq_mgr,
>>           break;
>>         default:
>> -        DRM_ERROR("[Usermode queues] IP %d not support\n", 
>> db_info->queue_type);
>> +    drm_file_err(uq_mgr->file,
>> +            "[Usermode queues] IP %d not support\n", 
>> db_info->queue_type);
>>           r = -EINVAL;
>>           goto unpin_bo;
>>       }
>>         index = amdgpu_doorbell_index_on_bar(uq_mgr->adev, db_obj->obj,
>>                            db_info->doorbell_offset, db_size);
>> -    DRM_DEBUG_DRIVER("[Usermode queues] doorbell index=%lld\n", index);
>> +    drm_dbg_driver(adev_to_drm(uq_mgr->adev),
>> +               "[Usermode queues] doorbell index=%lld\n", index);
>
> This and others are technically okay but not what the commit message 
> says. I'd say either split them into a separate patch or change the 
> commit message to just say something like "Add device and client 
> information to userq logging" so you give patch a wider mandate. ;)
>
Sure will split the patch and update commit message to precisely say 
what is being done.

Regards
Sunil Khatri


>
>>       amdgpu_bo_unreserve(db_obj->obj);
>>       return index;
>>   @@ -249,7 +253,7 @@ amdgpu_userqueue_destroy(struct drm_file *filp, 
>> int queue_id)
>>         queue = amdgpu_userqueue_find(uq_mgr, queue_id);
>>       if (!queue) {
>> -        DRM_DEBUG_DRIVER("Invalid queue id to destroy\n");
>> +        drm_dbg_driver(adev_to_drm(uq_mgr->adev), "Invalid queue id 
>> to destroy\n");
>>           mutex_unlock(&uq_mgr->userq_mutex);
>>           return -EINVAL;
>>       }
>> @@ -282,7 +286,8 @@ amdgpu_userqueue_create(struct drm_file *filp, 
>> union drm_amdgpu_userq *args)
>>       if (args->in.ip_type != AMDGPU_HW_IP_GFX &&
>>           args->in.ip_type != AMDGPU_HW_IP_DMA &&
>>           args->in.ip_type != AMDGPU_HW_IP_COMPUTE) {
>> -        DRM_ERROR("Usermode queue doesn't support IP type %u\n", 
>> args->in.ip_type);
>> +        drm_file_err(uq_mgr->file,
>> +            "Usermode queue doesn't support IP type %u\n", 
>> args->in.ip_type);
>>           return -EINVAL;
>>       }
>>   @@ -304,14 +309,16 @@ amdgpu_userqueue_create(struct drm_file 
>> *filp, union drm_amdgpu_userq *args)
>>         uq_funcs = adev->userq_funcs[args->in.ip_type];
>>       if (!uq_funcs) {
>> -        DRM_ERROR("Usermode queue is not supported for this IP 
>> (%u)\n", args->in.ip_type);
>> +        drm_file_err(uq_mgr->file,
>> +            "Usermode queue is not supported for this IP (%u)\n",
>> +            args->in.ip_type);
>>           r = -EINVAL;
>>           goto unlock;
>>       }
>>         queue = kzalloc(sizeof(struct amdgpu_usermode_queue), 
>> GFP_KERNEL);
>>       if (!queue) {
>> -        DRM_ERROR("Failed to allocate memory for queue\n");
>> +        drm_file_err(uq_mgr->file, "Failed to allocate memory for 
>> queue\n");
>>           r = -ENOMEM;
>>           goto unlock;
>>       }
>> @@ -327,7 +334,7 @@ amdgpu_userqueue_create(struct drm_file *filp, 
>> union drm_amdgpu_userq *args)
>>       /* Convert relative doorbell offset into absolute doorbell 
>> index */
>>       index = amdgpu_userqueue_get_doorbell_index(uq_mgr, &db_info, 
>> filp);
>>       if (index == (uint64_t)-EINVAL) {
>> -        DRM_ERROR("Failed to get doorbell for queue\n");
>> +        drm_file_err(uq_mgr->file, "Failed to get doorbell for 
>> queue\n");
>>           kfree(queue);
>>           goto unlock;
>>       }
>> @@ -336,13 +343,13 @@ amdgpu_userqueue_create(struct drm_file *filp, 
>> union drm_amdgpu_userq *args)
>>       xa_init_flags(&queue->fence_drv_xa, XA_FLAGS_ALLOC);
>>       r = amdgpu_userq_fence_driver_alloc(adev, queue);
>>       if (r) {
>> -        DRM_ERROR("Failed to alloc fence driver\n");
>> +        drm_file_err(uq_mgr->file, "Failed to alloc fence driver\n");
>>           goto unlock;
>>       }
>>         r = uq_funcs->mqd_create(uq_mgr, &args->in, queue);
>>       if (r) {
>> -        DRM_ERROR("Failed to create Queue\n");
>> +        drm_file_err(uq_mgr->file, "Failed to create Queue\n");
>
> My OCD is upset by inconsistencies of queue vs Queue and queue vs 
> usermode queue vs user queue. Looks like a good opportunity to tidy 
> things up while touching the lines.
I see the frustration. let me update those to all say the same thing.
>
>> amdgpu_userq_fence_driver_free(queue);
>>           kfree(queue);
>>           goto unlock;
>> @@ -350,7 +357,7 @@ amdgpu_userqueue_create(struct drm_file *filp, 
>> union drm_amdgpu_userq *args)
>>         qid = idr_alloc(&uq_mgr->userq_idr, queue, 1, 
>> AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
>>       if (qid < 0) {
>> -        DRM_ERROR("Failed to allocate a queue id\n");
>> +        drm_file_err(uq_mgr->file, "Failed to allocate a queue id\n");
>>           amdgpu_userq_fence_driver_free(queue);
>>           uq_funcs->mqd_destroy(uq_mgr, queue);
>>           kfree(queue);
>> @@ -360,7 +367,7 @@ amdgpu_userqueue_create(struct drm_file *filp, 
>> union drm_amdgpu_userq *args)
>>         r = uq_funcs->map(uq_mgr, queue);
>>       if (r) {
>> -        DRM_ERROR("Failed to map Queue\n");
>> +        drm_file_err(uq_mgr->file, "Failed to map Queue\n");
>>           idr_remove(&uq_mgr->userq_idr, qid);
>>           amdgpu_userq_fence_driver_free(queue);
>>           uq_funcs->mqd_destroy(uq_mgr, queue);
>> @@ -388,7 +395,7 @@ int amdgpu_userq_ioctl(struct drm_device *dev, 
>> void *data,
>>               return -EINVAL;
>>           r = amdgpu_userqueue_create(filp, args);
>>           if (r)
>> -            DRM_ERROR("Failed to create usermode queue\n");
>> +            drm_file_err(filp, "Failed to create usermode queue\n");
>
> Not really a kernel wide error if userspace passed invalid arguements 
> to the ioctl. Usually it is good to avoid allowing userspace at will 
> log spamming.

I would prefer to handle all such things separately. Might be touching a 
lot other places too, hope thats fine.

regards

Sunil

>
> Regards,
>
> Tvrtko
>
>>           break;
>>         case AMDGPU_USERQ_OP_FREE:
>> @@ -406,11 +413,11 @@ int amdgpu_userq_ioctl(struct drm_device *dev, 
>> void *data,
>>               return -EINVAL;
>>           r = amdgpu_userqueue_destroy(filp, args->in.queue_id);
>>           if (r)
>> -            DRM_ERROR("Failed to destroy usermode queue\n");
>> +            drm_file_err(filp, "Failed to destroy usermode queue\n");
>>           break;
>>         default:
>> -        DRM_DEBUG_DRIVER("Invalid user queue op specified: %d\n", 
>> args->in.op);
>> +        drm_dbg_driver(dev, "Invalid user queue op specified: %d\n", 
>> args->in.op);
>>           return -EINVAL;
>>       }
>>   @@ -479,7 +486,7 @@ amdgpu_userqueue_validate_bos(struct 
>> amdgpu_userq_mgr *uq_mgr)
>>           ret = amdgpu_vm_lock_pd(vm, &exec, 2);
>>           drm_exec_retry_on_contention(&exec);
>>           if (unlikely(ret)) {
>> -            DRM_ERROR("Failed to lock PD\n");
>> +            drm_file_err(uq_mgr->file, "Failed to lock PD\n");
>>               goto unlock_all;
>>           }
>>   @@ -519,7 +526,7 @@ amdgpu_userqueue_validate_bos(struct 
>> amdgpu_userq_mgr *uq_mgr)
>>           bo = bo_va->base.bo;
>>           ret = amdgpu_userqueue_validate_vm_bo(NULL, bo);
>>           if (ret) {
>> -            DRM_ERROR("Failed to validate BO\n");
>> +            drm_file_err(uq_mgr->file, "Failed to validate BO\n");
>>               goto unlock_all;
>>           }
>>   @@ -550,7 +557,7 @@ amdgpu_userqueue_validate_bos(struct 
>> amdgpu_userq_mgr *uq_mgr)
>>         ret = amdgpu_eviction_fence_replace_fence(&fpriv->evf_mgr, 
>> &exec);
>>       if (ret)
>> -        DRM_ERROR("Failed to replace eviction fence\n");
>> +        drm_file_err(uq_mgr->file, "Failed to replace eviction 
>> fence\n");
>>     unlock_all:
>>       drm_exec_fini(&exec);
>> @@ -569,13 +576,13 @@ static void 
>> amdgpu_userqueue_resume_worker(struct work_struct *work)
>>         ret = amdgpu_userqueue_validate_bos(uq_mgr);
>>       if (ret) {
>> -        DRM_ERROR("Failed to validate BOs to restore\n");
>> +        drm_file_err(uq_mgr->file, "Failed to validate BOs to 
>> restore\n");
>>           goto unlock;
>>       }
>>         ret = amdgpu_userqueue_resume_all(uq_mgr);
>>       if (ret) {
>> -        DRM_ERROR("Failed to resume all queues\n");
>> +        drm_file_err(uq_mgr->file, "Failed to resume all queues\n");
>>           goto unlock;
>>       }
>

Re: [PATCH v3 1/4] drm: add function drm_file_err to print proc information too

[Tvrtko Ursulin]

On 15/04/2025 19:43, Sunil Khatri wrote:
> Add a drm helper function which get the process information for
> the drm_file and append the process information using the existing
> drm_err.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>   include/drm/drm_file.h | 40 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 94d365b22505..e329299a2b2c 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -37,6 +37,7 @@
>   #include <uapi/drm/drm.h>
>   
>   #include <drm/drm_prime.h>
> +#include <drm/drm_print.h>
>   
>   struct dma_fence;
>   struct drm_file;
> @@ -446,6 +447,45 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
>   	return file_priv->minor->type == DRM_MINOR_ACCEL;
>   }
>   
> +/**
> + * drm_file_err - Fill info string with process name and pid
> + * @file_priv: context of interest for process name and pid
> + * @fmt: prinf() like format string
> + *
> + * This update the user provided buffer with process
> + * name and pid information for @file_priv
> + */
> +__printf(2, 3)
> +static inline void drm_file_err(struct drm_file *file_priv, const char *fmt, ...)
> +{
> +	struct task_struct *task;
> +	struct pid *pid;
> +	struct drm_device *dev = file_priv->minor->dev;
> +	char new_fmt[256];
> +	char final_fmt[512];
> +	va_list args;
> +
> +	mutex_lock(&file_priv->client_name_lock);
> +	rcu_read_lock();
> +	pid = rcu_dereference(file_priv->pid);
> +	task = pid_task(pid, PIDTYPE_TGID);
> +
> +	if (drm_WARN_ON_ONCE(dev, !task))
> +		return;
> +
> +	snprintf(new_fmt, sizeof(new_fmt), "proc:%s pid:%d client_name:%s %s",
> +		task->comm, task->pid, file_priv->client_name ?: "Unset", fmt);
> +
> +	va_start(args, fmt);
> +	vsnprintf(final_fmt, sizeof(final_fmt), new_fmt, args);
> +
> +	drm_err(dev, "%s", final_fmt);
> +	va_end(args);
> +
> +	rcu_read_unlock();
> +	mutex_unlock(&file_priv->client_name_lock);
> +}
> +

I was hoping something primitive could be enough. With no temporary 
stack space required. Primitive on the level of (but simplified for 
illustration purpose):

#define some_err(_file, _fmt, ...) \
	drm_err(dev, "client-%s: " _fmt, (_this)->client_name, ##__VA_ARGS__)

Am I missing something or that would work?

Regards,

Tvrtko

>   void drm_file_update_pid(struct drm_file *);
>   
>   struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);

Re: [PATCH v3 1/4] drm: add function drm_file_err to print proc information too

[Khatri, Sunil]

On 4/16/2025 12:37 PM, Tvrtko Ursulin wrote:
>
> On 15/04/2025 19:43, Sunil Khatri wrote:
>> Add a drm helper function which get the process information for
>> the drm_file and append the process information using the existing
>> drm_err.
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   include/drm/drm_file.h | 40 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 94d365b22505..e329299a2b2c 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -37,6 +37,7 @@
>>   #include <uapi/drm/drm.h>
>>     #include <drm/drm_prime.h>
>> +#include <drm/drm_print.h>
>>     struct dma_fence;
>>   struct drm_file;
>> @@ -446,6 +447,45 @@ static inline bool drm_is_accel_client(const 
>> struct drm_file *file_priv)
>>       return file_priv->minor->type == DRM_MINOR_ACCEL;
>>   }
>>   +/**
>> + * drm_file_err - Fill info string with process name and pid
>> + * @file_priv: context of interest for process name and pid
>> + * @fmt: prinf() like format string
>> + *
>> + * This update the user provided buffer with process
>> + * name and pid information for @file_priv
>> + */
>> +__printf(2, 3)
>> +static inline void drm_file_err(struct drm_file *file_priv, const 
>> char *fmt, ...)
>> +{
>> +    struct task_struct *task;
>> +    struct pid *pid;
>> +    struct drm_device *dev = file_priv->minor->dev;
>> +    char new_fmt[256];
>> +    char final_fmt[512];
>> +    va_list args;
>> +
>> +    mutex_lock(&file_priv->client_name_lock);
>> +    rcu_read_lock();
>> +    pid = rcu_dereference(file_priv->pid);
>> +    task = pid_task(pid, PIDTYPE_TGID);
>> +
>> +    if (drm_WARN_ON_ONCE(dev, !task))
>> +        return;
>> +
>> +    snprintf(new_fmt, sizeof(new_fmt), "proc:%s pid:%d 
>> client_name:%s %s",
>> +        task->comm, task->pid, file_priv->client_name ?: "Unset", fmt);
>> +
>> +    va_start(args, fmt);
>> +    vsnprintf(final_fmt, sizeof(final_fmt), new_fmt, args);
>> +
>> +    drm_err(dev, "%s", final_fmt);
>> +    va_end(args);
>> +
>> +    rcu_read_unlock();
>> +    mutex_unlock(&file_priv->client_name_lock);
>> +}
>> +
>
> I was hoping something primitive could be enough. With no temporary 
> stack space required. Primitive on the level of (but simplified for 
> illustration purpose):
>
> #define some_err(_file, _fmt, ...) \
>     drm_err(dev, "client-%s: " _fmt, (_this)->client_name, ##__VA_ARGS__)
I also thought of doing it similarly but that dint work. There was lot 
of code to get the process name and pid along with client_name too. So 
##__VA_ARGS__ dont work as soon as its a function and not macro.
Also drm_err gave me errors and this is the way i find it not 
complaining. new_fmt is a string directly anymore and hence need to %s 
to pass but then the drm_err complain too many args for args to pass. So 
i have to combine new_fmt and args in one to get final_fmt and atleast 
functionally it worked.

Yesterday even i though that i would be as simple as adding a macro.
>
> Am I missing something or that would work?
>
> Regards,
>
> Tvrtko
>
>>   void drm_file_update_pid(struct drm_file *);
>>     struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, 
>> unsigned int minor_id);
>

Re: [PATCH v3 1/4] drm: add function drm_file_err to print proc information too

[Christian König]

Am 16.04.25 um 10:39 schrieb Khatri, Sunil:
>
> On 4/16/2025 12:37 PM, Tvrtko Ursulin wrote:
>>
>> On 15/04/2025 19:43, Sunil Khatri wrote:
>>> [SNIP]
>>> +
>>
>> I was hoping something primitive could be enough. With no temporary stack space required. Primitive on the level of (but simplified for illustration purpose):
>>
>> #define some_err(_file, _fmt, ...) \
>>     drm_err(dev, "client-%s: " _fmt, (_this)->client_name, ##__VA_ARGS__)
> I also thought of doing it similarly but that dint work. There was lot of code to get the process name and pid along with client_name too. So ##__VA_ARGS__ dont work as soon as its a function and not macro.
> Also drm_err gave me errors and this is the way i find it not complaining. new_fmt is a string directly anymore and hence need to %s to pass but then the drm_err complain too many args for args to pass. So i have to combine new_fmt and args in one to get final_fmt and atleast functionally it worked.
>
> Yesterday even i though that i would be as simple as adding a macro.

It's a bit tricky, but I think that is doable.

You need something like this here:

#define drm_file_err(file, fmt, ...)
    do {
        struct task_struct *task = drm_file_lock_pid(file);

        drm_err(file->dev, "task: %s pid: %d client: %s" fmt, task, file->pid, ##__VA_ARGS_);
        drm_file_unlock_pid(file);
    } while (0);

You then just need to implement drm_file_lock_pid (maybe come up with a better name) to grab the mutex and take the RCU read lock.

Christian.

>>
>> Am I missing something or that would work?
>>
>> Regards,
>>
>> Tvrtko
>>
>>>   void drm_file_update_pid(struct drm_file *);
>>>     struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
>>