[PATCH v4 1/5] drm: add macro drm_file_err to print process info

[PATCH v4 1/5] drm: add macro drm_file_err to print process info

[Sunil Khatri]

Add a drm helper macro which append the process information for
the drm_file over drm_err.

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

diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 94d365b22505..5ae5ad1048fb 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,46 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
 	return file_priv->minor->type == DRM_MINOR_ACCEL;
 }
 
+static struct task_struct *drm_task_lock(struct drm_file *file_priv)
+					__attribute__((__maybe_unused));
+static struct task_struct *drm_task_lock(struct drm_file *file_priv)
+{
+	struct task_struct *task;
+	struct pid *pid;
+
+	mutex_lock(&file_priv->client_name_lock);
+	rcu_read_lock();
+	pid = rcu_dereference(file_priv->pid);
+	task = pid_task(pid, PIDTYPE_TGID);
+	return task;
+}
+
+static void drm_task_unlock(struct drm_file *file_priv) __attribute__((__maybe_unused));
+static void drm_task_unlock(struct drm_file *file_priv)
+{
+	rcu_read_unlock();
+	mutex_unlock(&file_priv->client_name_lock);
+}
+/**
+ * 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
+ */
+#define drm_file_err(file_priv, fmt, ...)						\
+	do {										\
+		struct task_struct *task;						\
+		struct drm_device *dev = file_priv->minor->dev;				\
+											\
+		task = drm_task_lock(file_priv);					\
+		drm_err(dev, "comm: %s pid: %d client: %s " fmt,			\
+			task ? task->comm : "", task ? task->pid : 0,			\
+			file_priv->client_name ?: "Unset", ##__VA_ARGS__);		\
+		drm_task_unlock(file_priv);						\
+	} while (0)
+
 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 v4 2/5] 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

[PATCH v4 3/5] drm/amdgpu: use drm_file_err to add process info

[Sunil Khatri]

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

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..ea43bcd63feb 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 queues\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

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

[Sunil Khatri]

change the DRM_ERROR to drm_file_err to ad process name
and pid to the logging.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
index ea43bcd63feb..4957c7b04fe8 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,7 +217,8 @@ 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;
 	}
@@ -282,7 +285,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 +308,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 +333,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 +342,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 +356,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 +366,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 +394,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,7 +412,7 @@ 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:
@@ -479,7 +485,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 +525,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 +556,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 +575,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

[PATCH v4 5/5] drm/amdgpu: change DRM_DBG_DRIVER to drm_dbg_driver

[Sunil Khatri]

update the functions in amdgpu_userqueues.c from
DRM_DBG_DRIVER to drm_dbg_driver so multi gpu instance
can be logged in.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
index 4957c7b04fe8..f78cc0bc2d48 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -225,7 +225,7 @@ amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,
 
 	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_dev_dbg(adev_to_drm(adev), "[Usermode queues] doorbell index=%lld\n", index);
 	amdgpu_bo_unreserve(db_obj->obj);
 	return index;
 
@@ -252,7 +252,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(adev), "Invalid queue id to destroy\n");
 		mutex_unlock(&uq_mgr->userq_mutex);
 		return -EINVAL;
 	}
@@ -416,7 +416,8 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
 		break;
 
 	default:
-		DRM_DEBUG_DRIVER("Invalid user queue op specified: %d\n", args->in.op);
+		drm_dbg_driver(adev_to_drm(adev), "Invalid user queue op specified: %d\n",
+			       args->in.op);
 		return -EINVAL;
 	}
 
-- 
2.34.1

Re: [PATCH v4 1/5] drm: add macro drm_file_err to print process info

[Jani Nikula]

On Wed, 16 Apr 2025, Sunil Khatri <sunil.khatri@amd.com> wrote:
> Add a drm helper macro which append the process information for
> the drm_file over drm_err.
>
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>  include/drm/drm_file.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 94d365b22505..5ae5ad1048fb 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>

Not a fan of including drm_print.h everywhere that drm_file.h is
included. We've been trying to get rid of this, and go the other
way. It's really hard to manage dependencies when everything ends up
including everything.

>  
>  struct dma_fence;
>  struct drm_file;
> @@ -446,6 +447,46 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
>  	return file_priv->minor->type == DRM_MINOR_ACCEL;
>  }
>  
> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
> +					__attribute__((__maybe_unused));

inline is the keyword you're missing here, and that's why you've had to
add maybe unused...

> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
> +{
> +	struct task_struct *task;
> +	struct pid *pid;
> +
> +	mutex_lock(&file_priv->client_name_lock);
> +	rcu_read_lock();
> +	pid = rcu_dereference(file_priv->pid);
> +	task = pid_task(pid, PIDTYPE_TGID);
> +	return task;
> +}
> +
> +static void drm_task_unlock(struct drm_file *file_priv) __attribute__((__maybe_unused));
> +static void drm_task_unlock(struct drm_file *file_priv)
> +{
> +	rcu_read_unlock();
> +	mutex_unlock(&file_priv->client_name_lock);
> +}

...but *why* are you inlining these? To make this header self-contained,
I think you'd need to add maybe sched.h, pid.h, rcupdate.h, mutex.h, or
something. I consider static inlines actively harmful if they force you
to pull in a lot of other headers.

> +/**
> + * 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
> + */
> +#define drm_file_err(file_priv, fmt, ...)						\
> +	do {										\
> +		struct task_struct *task;						\
> +		struct drm_device *dev = file_priv->minor->dev;				\
> +											\
> +		task = drm_task_lock(file_priv);					\
> +		drm_err(dev, "comm: %s pid: %d client: %s " fmt,			\
> +			task ? task->comm : "", task ? task->pid : 0,			\
> +			file_priv->client_name ?: "Unset", ##__VA_ARGS__);		\
> +		drm_task_unlock(file_priv);						\
> +	} while (0)
> +

For that matter, why is *this* inline? For debugs it makes a little more
sense when it adds the function, but drm_err() doesn't.

Make all of these real functions, no need to include drm_print.h, and
everything is better.

BR,
Jani.

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

-- 
Jani Nikula, Intel

Re: [PATCH v4 1/5] drm: add macro drm_file_err to print process info

[Khatri, Sunil]

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


On 4/16/2025 7:55 PM, Jani Nikula wrote:
> On Wed, 16 Apr 2025, Sunil Khatri<sunil.khatri@amd.com> wrote:
>> Add a drm helper macro which append the process information for
>> the drm_file over drm_err.
>>
>> Signed-off-by: Sunil Khatri<sunil.khatri@amd.com>
>> ---
>>   include/drm/drm_file.h | 41 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 94d365b22505..5ae5ad1048fb 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>
> Not a fan of including drm_print.h everywhere that drm_file.h is
> included. We've been trying to get rid of this, and go the other
> way. It's really hard to manage dependencies when everything ends up
> including everything.

Sure, right thing to do, will update. Not sure why i added it, may be in 
initial versions needed that :)

regards Sunil

>>   
>>   struct dma_fence;
>>   struct drm_file;
>> @@ -446,6 +447,46 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
>>   	return file_priv->minor->type == DRM_MINOR_ACCEL;
>>   }
>>   
>> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
>> +					__attribute__((__maybe_unused));
> inline is the keyword you're missing here, and that's why you've had to
> add maybe unused...

I guess that's true. Let me try that and will make it inline if no warning.

Regards
Sunil

>
>> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
>> +{
>> +	struct task_struct *task;
>> +	struct pid *pid;
>> +
>> +	mutex_lock(&file_priv->client_name_lock);
>> +	rcu_read_lock();
>> +	pid = rcu_dereference(file_priv->pid);
>> +	task = pid_task(pid, PIDTYPE_TGID);
>> +	return task;
>> +}
>> +
>> +static void drm_task_unlock(struct drm_file *file_priv) __attribute__((__maybe_unused));
>> +static void drm_task_unlock(struct drm_file *file_priv)
>> +{
>> +	rcu_read_unlock();
>> +	mutex_unlock(&file_priv->client_name_lock);
>> +}
> ...but *why* are you inlining these? To make this header self-contained,
> I think you'd need to add maybe sched.h, pid.h, rcupdate.h, mutex.h, or
> something. I consider static inlines actively harmful if they force you
> to pull in a lot of other headers.

It builds and works well and i dont see the need to sched.h and etc. I 
dont see which function you mean by inlining here, mind to share which 
functions ?

Regards Sunil Khatri

>
>> +/**
>> + * 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
>> + */
>> +#define drm_file_err(file_priv, fmt, ...)						\
>> +	do {										\
>> +		struct task_struct *task;						\
>> +		struct drm_device *dev = file_priv->minor->dev;				\
>> +											\
>> +		task = drm_task_lock(file_priv);					\
>> +		drm_err(dev, "comm: %s pid: %d client: %s " fmt,			\
>> +			task ? task->comm : "", task ? task->pid : 0,			\
>> +			file_priv->client_name ?: "Unset", ##__VA_ARGS__);		\
>> +		drm_task_unlock(file_priv);						\
>> +	} while (0)
>> +
> For that matter, why is *this* inline? For debugs it makes a little more
> sense when it adds the function, but drm_err() doesn't.

Not getting to know which function again you mean inline here ?

Regards
Sunil Khatri

>
> Make all of these real functions, no need to include drm_print.h, and
> everything is better.
Sure i will do that and thanks a lot for those inputs.
>
> BR,
> Jani.
>
>>   void drm_file_update_pid(struct drm_file *);
>>   
>>   struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);

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

Re: [PATCH v4 1/5] drm: add macro drm_file_err to print process info

[Khatri, Sunil]

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


On 4/16/2025 7:55 PM, Jani Nikula wrote:
> On Wed, 16 Apr 2025, Sunil Khatri<sunil.khatri@amd.com> wrote:
>> Add a drm helper macro which append the process information for
>> the drm_file over drm_err.
>>
>> Signed-off-by: Sunil Khatri<sunil.khatri@amd.com>
>> ---
>>   include/drm/drm_file.h | 41 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 94d365b22505..5ae5ad1048fb 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>
> Not a fan of including drm_print.h everywhere that drm_file.h is
> included. We've been trying to get rid of this, and go the other
> way. It's really hard to manage dependencies when everything ends up
> including everything.
>
>>   
>>   struct dma_fence;
>>   struct drm_file;
>> @@ -446,6 +447,46 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
>>   	return file_priv->minor->type == DRM_MINOR_ACCEL;
>>   }
>>   
>> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
>> +					__attribute__((__maybe_unused));
> inline is the keyword you're missing here, and that's why you've had to
> add maybe unused...
>
>> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
>> +{
>> +	struct task_struct *task;
>> +	struct pid *pid;
>> +
>> +	mutex_lock(&file_priv->client_name_lock);
>> +	rcu_read_lock();
>> +	pid = rcu_dereference(file_priv->pid);
>> +	task = pid_task(pid, PIDTYPE_TGID);
>> +	return task;
>> +}
>> +
>> +static void drm_task_unlock(struct drm_file *file_priv) __attribute__((__maybe_unused));
>> +static void drm_task_unlock(struct drm_file *file_priv)
>> +{
>> +	rcu_read_unlock();
>> +	mutex_unlock(&file_priv->client_name_lock);
>> +}
> ...but *why* are you inlining these? To make this header self-contained,
> I think you'd need to add maybe sched.h, pid.h, rcupdate.h, mutex.h, or
> something. I consider static inlines actively harmful if they force you
> to pull in a lot of other headers.

Code readability and easy maintenance is the key to make these as 
inline. Also we are  keeping the logic function which gets the task with 
locks in separate function then actually

passing that in the drm_err as string.

>
>> +/**
>> + * 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
>> + */
>> +#define drm_file_err(file_priv, fmt, ...)						\
>> +	do {										\
>> +		struct task_struct *task;						\
>> +		struct drm_device *dev = file_priv->minor->dev;				\
>> +											\
>> +		task = drm_task_lock(file_priv);					\
>> +		drm_err(dev, "comm: %s pid: %d client: %s " fmt,			\
>> +			task ? task->comm : "", task ? task->pid : 0,			\
>> +			file_priv->client_name ?: "Unset", ##__VA_ARGS__);		\
>> +		drm_task_unlock(file_priv);						\
>> +	} while (0)
>> +
> For that matter, why is *this* inline? For debugs it makes a little more
> sense when it adds the function, but drm_err() doesn't.
>
> Make all of these real functions, no need to include drm_print.h, and
> everything is better.
>
Only reason of hacing drm_file_err as a macro as the variadic fmt and 
args are not possible to pass without using local variables to make 
strings of fmt and args separately and with macro to macro they Are 
passed cleanly and no local variables needed. you can check V3 i guess 
where this whole was an function only.

Regards Sunil Khatri

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

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

Re: [PATCH v4 1/5] drm: add macro drm_file_err to print process info

[Jani Nikula]

On Thu, 17 Apr 2025, "Khatri, Sunil" <sukhatri@amd.com> wrote:
> On 4/16/2025 7:55 PM, Jani Nikula wrote:
>> On Wed, 16 Apr 2025, Sunil Khatri<sunil.khatri@amd.com> wrote:
>>> Add a drm helper macro which append the process information for
>>> the drm_file over drm_err.
>>>
>>> Signed-off-by: Sunil Khatri<sunil.khatri@amd.com>
>>> ---
>>>   include/drm/drm_file.h | 41 +++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 41 insertions(+)
>>>
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index 94d365b22505..5ae5ad1048fb 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>
>> Not a fan of including drm_print.h everywhere that drm_file.h is
>> included. We've been trying to get rid of this, and go the other
>> way. It's really hard to manage dependencies when everything ends up
>> including everything.
>>
>>>   
>>>   struct dma_fence;
>>>   struct drm_file;
>>> @@ -446,6 +447,46 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
>>>   	return file_priv->minor->type == DRM_MINOR_ACCEL;
>>>   }
>>>   
>>> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
>>> +					__attribute__((__maybe_unused));
>> inline is the keyword you're missing here, and that's why you've had to
>> add maybe unused...
>>
>>> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
>>> +{
>>> +	struct task_struct *task;
>>> +	struct pid *pid;
>>> +
>>> +	mutex_lock(&file_priv->client_name_lock);
>>> +	rcu_read_lock();
>>> +	pid = rcu_dereference(file_priv->pid);
>>> +	task = pid_task(pid, PIDTYPE_TGID);
>>> +	return task;
>>> +}
>>> +
>>> +static void drm_task_unlock(struct drm_file *file_priv) __attribute__((__maybe_unused));
>>> +static void drm_task_unlock(struct drm_file *file_priv)
>>> +{
>>> +	rcu_read_unlock();
>>> +	mutex_unlock(&file_priv->client_name_lock);
>>> +}
>> ...but *why* are you inlining these? To make this header self-contained,
>> I think you'd need to add maybe sched.h, pid.h, rcupdate.h, mutex.h, or
>> something. I consider static inlines actively harmful if they force you
>> to pull in a lot of other headers.
>
> Code readability and easy maintenance is the key to make these as 
> inline.

Oh, quite the opposite. Static inlines are a maintenance nightmare.

BR,
Jani.


> Also we are  keeping the logic function which gets the task with 
> locks in separate function then actually
>
> passing that in the drm_err as string.
>
>>
>>> +/**
>>> + * 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
>>> + */
>>> +#define drm_file_err(file_priv, fmt, ...)						\
>>> +	do {										\
>>> +		struct task_struct *task;						\
>>> +		struct drm_device *dev = file_priv->minor->dev;				\
>>> +											\
>>> +		task = drm_task_lock(file_priv);					\
>>> +		drm_err(dev, "comm: %s pid: %d client: %s " fmt,			\
>>> +			task ? task->comm : "", task ? task->pid : 0,			\
>>> +			file_priv->client_name ?: "Unset", ##__VA_ARGS__);		\
>>> +		drm_task_unlock(file_priv);						\
>>> +	} while (0)
>>> +
>> For that matter, why is *this* inline? For debugs it makes a little more
>> sense when it adds the function, but drm_err() doesn't.
>>
>> Make all of these real functions, no need to include drm_print.h, and
>> everything is better.
>>
> Only reason of hacing drm_file_err as a macro as the variadic fmt and 
> args are not possible to pass without using local variables to make 
> strings of fmt and args separately and with macro to macro they Are 
> passed cleanly and no local variables needed. you can check V3 i guess 
> where this whole was an function only.
>
> Regards Sunil Khatri
>
>> BR,
>> Jani.
>>
>>>   void drm_file_update_pid(struct drm_file *);
>>>   
>>>   struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);

-- 
Jani Nikula, Intel