[PATCH v4 1/4] drm: move debugfs functionality from drm_drv.c to drm_debugfs.c

[PATCH v4 1/4] drm: move debugfs functionality from drm_drv.c to drm_debugfs.c

[Sunil Khatri]

move the functions from drm_drv.c which uses the static
drm_debugfs_root as parent node in the debugfs by drm.

move this root node to the debugfs for easily handling
of future requirements to add more information in the
root directory and one of which is planned to have
directories for each client in the root directory
which is dri.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/drm_debugfs.c  | 22 ++++++++++++++++++----
 drivers/gpu/drm/drm_drv.c      | 11 ++++-------
 drivers/gpu/drm/drm_internal.h |  6 ++----
 include/drm/drm_drv.h          | 10 ++++++++++
 4 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 2d43bda82887..5a33ec299c04 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -44,6 +44,8 @@
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
 
+static struct dentry *drm_debugfs_root;
+
 /***************************************************
  * Initialization, etc.
  **************************************************/
@@ -286,6 +288,16 @@ int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
 }
 EXPORT_SYMBOL(drm_debugfs_remove_files);
 
+void drm_debugfs_create_dir(void)
+{
+	drm_debugfs_root = debugfs_create_dir("dri", NULL);
+}
+
+void drm_debugfs_remove_dir(void)
+{
+	debugfs_remove(drm_debugfs_root);
+}
+
 /**
  * drm_debugfs_dev_init - create debugfs directory for the device
  * @dev: the device which we want to create the directory for
@@ -295,7 +307,10 @@ EXPORT_SYMBOL(drm_debugfs_remove_files);
  */
 void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root)
 {
-	dev->debugfs_root = debugfs_create_dir(dev->unique, root);
+	if (!root)
+		dev->debugfs_root = debugfs_create_dir(dev->unique, drm_debugfs_root);
+	else
+		dev->debugfs_root = debugfs_create_dir(dev->unique, root);
 }
 
 /**
@@ -322,14 +337,13 @@ void drm_debugfs_dev_register(struct drm_device *dev)
 		drm_atomic_debugfs_init(dev);
 }
 
-int drm_debugfs_register(struct drm_minor *minor, int minor_id,
-			 struct dentry *root)
+int drm_debugfs_register(struct drm_minor *minor, int minor_id)
 {
 	struct drm_device *dev = minor->dev;
 	char name[64];
 
 	sprintf(name, "%d", minor_id);
-	minor->debugfs_symlink = debugfs_create_symlink(name, root,
+	minor->debugfs_symlink = debugfs_create_symlink(name, drm_debugfs_root,
 							dev->unique);
 
 	/* TODO: Only for compatibility with drivers */
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 17fc5dc708f4..8abc52eac8f3 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -69,8 +69,6 @@ DEFINE_XARRAY_ALLOC(drm_minors_xa);
  */
 static bool drm_core_init_complete;
 
-static struct dentry *drm_debugfs_root;
-
 DEFINE_STATIC_SRCU(drm_unplug_srcu);
 
 /*
@@ -183,8 +181,7 @@ static int drm_minor_register(struct drm_device *dev, enum drm_minor_type type)
 		return 0;
 
 	if (minor->type != DRM_MINOR_ACCEL) {
-		ret = drm_debugfs_register(minor, minor->index,
-					   drm_debugfs_root);
+		ret = drm_debugfs_register(minor, minor->index);
 		if (ret) {
 			DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
 			goto err_debugfs;
@@ -754,7 +751,7 @@ static int drm_dev_init(struct drm_device *dev,
 	if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL))
 		accel_debugfs_init(dev);
 	else
-		drm_debugfs_dev_init(dev, drm_debugfs_root);
+		drm_debugfs_dev_init(dev, NULL);
 
 	return 0;
 
@@ -1168,7 +1165,7 @@ static void drm_core_exit(void)
 	drm_panic_exit();
 	accel_core_exit();
 	unregister_chrdev(DRM_MAJOR, "drm");
-	debugfs_remove(drm_debugfs_root);
+	drm_debugfs_remove_dir();
 	drm_sysfs_destroy();
 	WARN_ON(!xa_empty(&drm_minors_xa));
 	drm_connector_ida_destroy();
@@ -1187,7 +1184,7 @@ static int __init drm_core_init(void)
 		goto error;
 	}
 
-	drm_debugfs_root = debugfs_create_dir("dri", NULL);
+	drm_debugfs_create_dir();
 
 	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
 	if (ret < 0)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index b2b6a8e49dda..d2d8e72f32d9 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -186,8 +186,7 @@ void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
 #if defined(CONFIG_DEBUG_FS)
 void drm_debugfs_dev_fini(struct drm_device *dev);
 void drm_debugfs_dev_register(struct drm_device *dev);
-int drm_debugfs_register(struct drm_minor *minor, int minor_id,
-			 struct dentry *root);
+int drm_debugfs_register(struct drm_minor *minor, int minor_id);
 void drm_debugfs_unregister(struct drm_minor *minor);
 void drm_debugfs_connector_add(struct drm_connector *connector);
 void drm_debugfs_connector_remove(struct drm_connector *connector);
@@ -205,8 +204,7 @@ static inline void drm_debugfs_dev_register(struct drm_device *dev)
 {
 }
 
-static inline int drm_debugfs_register(struct drm_minor *minor, int minor_id,
-				       struct dentry *root)
+static inline int drm_debugfs_register(struct drm_minor *minor, int minor_id)
 {
 	return 0;
 }
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index a43d707b5f36..4e77a0c4a7f9 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -567,10 +567,20 @@ static inline bool drm_firmware_drivers_only(void)
 
 #if defined(CONFIG_DEBUG_FS)
 void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root);
+void drm_debugfs_create_dir(void);
+void drm_debugfs_remove_dir(void);
 #else
 static inline void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root)
 {
 }
+
+static inline void drm_debugfs_create_dir(void)
+{
+}
+
+static inline void drm_debugfs_remove_dir(void)
+{
+}
 #endif
 
 #endif
-- 
2.34.1

[PATCH v4 2/4] drm: add debugfs support on per client-id basis

[Sunil Khatri]

add support to add a directory for each client-id
with root at the dri level. Since the clients are
unique and not just related to one single drm device,
so it makes more sense to add all the client based
nodes with root as dri.

Also create a symlink back to the parent drm device
from each client.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/drm_debugfs.c | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_file.c    | 10 ++++++++++
 include/drm/drm_debugfs.h     | 12 ++++++++++++
 include/drm/drm_file.h        |  7 +++++++
 4 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 5a33ec299c04..875276d5fb9f 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -298,6 +298,38 @@ void drm_debugfs_remove_dir(void)
 	debugfs_remove(drm_debugfs_root);
 }
 
+int drm_debugfs_clients_add(struct drm_file *file)
+{
+	struct drm_device *dev;
+	char *client_dir, *symlink;
+
+	dev = file->minor->dev;
+
+	client_dir = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
+	if (!client_dir)
+		return -ENOMEM;
+
+	/* Create a debugfs directory for the client in root on drm debugfs */
+	file->debugfs_client = debugfs_create_dir(client_dir, drm_debugfs_root);
+	kfree(client_dir);
+
+	symlink = kasprintf(GFP_KERNEL, "../%s", dev->unique);
+	if (!symlink)
+		return -ENOMEM;
+
+	/* Create a link from client_id to the drm device this client id belongs to */
+	debugfs_create_symlink("device", file->debugfs_client, symlink);
+	kfree(symlink);
+
+	return 0;
+}
+
+void drm_debugfs_clients_remove(struct drm_file *file)
+{
+	debugfs_remove_recursive(file->debugfs_client);
+	file->debugfs_client = NULL;
+}
+
 /**
  * drm_debugfs_dev_init - create debugfs directory for the device
  * @dev: the device which we want to create the directory for
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 06ba6dcbf5ae..8502c5a630b1 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -39,12 +39,14 @@
 #include <linux/poll.h>
 #include <linux/slab.h>
 #include <linux/vga_switcheroo.h>
+#include <linux/debugfs.h>
 
 #include <drm/drm_client_event.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_print.h>
+#include <drm/drm_debugfs.h>
 
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
@@ -143,6 +145,13 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
 	rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
 	file->minor = minor;
 
+	ret = drm_debugfs_clients_add(file);
+	if (ret) {
+		put_pid(rcu_access_pointer(file->pid));
+		kfree(file);
+		return ERR_PTR(ret);
+	}
+
 	/* for compatibility root is always authenticated */
 	file->authenticated = capable(CAP_SYS_ADMIN);
 
@@ -236,6 +245,7 @@ void drm_file_free(struct drm_file *file)
 		     atomic_read(&dev->open_count));
 
 	drm_events_release(file);
+	drm_debugfs_clients_remove(file);
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		drm_fb_release(file);
diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
index cf06cee4343f..4bd6cc1d0900 100644
--- a/include/drm/drm_debugfs.h
+++ b/include/drm/drm_debugfs.h
@@ -153,6 +153,9 @@ void drm_debugfs_add_files(struct drm_device *dev,
 
 int drm_debugfs_gpuva_info(struct seq_file *m,
 			   struct drm_gpuvm *gpuvm);
+
+int drm_debugfs_clients_add(struct drm_file *file);
+void drm_debugfs_clients_remove(struct drm_file *file);
 #else
 static inline void drm_debugfs_create_files(const struct drm_info_list *files,
 					    int count, struct dentry *root,
@@ -181,6 +184,15 @@ static inline int drm_debugfs_gpuva_info(struct seq_file *m,
 {
 	return 0;
 }
+
+int drm_debugfs_clients_add(struct drm_file *file)
+{
+	return 0;
+}
+
+void drm_debugfs_clients_remove(struct drm_file *file)
+{
+}
 #endif
 
 #endif /* _DRM_DEBUGFS_H_ */
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 5c3b2aa3e69d..eab7546aad79 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -400,6 +400,13 @@ struct drm_file {
 	 * @client_name_lock: Protects @client_name.
 	 */
 	struct mutex client_name_lock;
+
+	/**
+	 * @debugfs_client:
+	 *
+	 * debugfs directory for each client under a drm node.
+	 */
+	struct dentry *debugfs_client;
 };
 
 /**
-- 
2.34.1

[PATCH v4 3/4] drm/amdgpu: add debugfs support for VM pagetable per client

[Sunil Khatri]

Each drm node is associated with a unique client-id.
Create a directory for each drm-file in the dri root
directory. This directory is unique to hold information
related to a client id which is unique in the system
irrespective of how many drm devices are on the system.

Adding root page table base address of the VM under
the client-id node along with the process information
in debugfs.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index d2ce7d86dbc8..aa912168fd68 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1395,7 +1395,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
 	if (r)
 		goto error_pasid;
 
-	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id);
+	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id, file_priv);
 	if (r)
 		goto error_pasid;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 3911c78f8282..9e3dd187b597 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2520,12 +2520,67 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
 	get_task_comm(vm->task_info->process_name, current->group_leader);
 }
 
+#if defined(CONFIG_DEBUG_FS)
+static int amdgpu_pt_info_read(struct seq_file *m, void *unused)
+{
+	struct drm_file *file;
+	struct amdgpu_fpriv *fpriv;
+	struct pid *pid;
+	struct task_struct *task;
+	struct amdgpu_bo *root_bo;
+	int r;
+
+	file = (struct drm_file *)m->private;
+	if (!file || !file->driver_priv)
+		return -EINVAL;
+
+	fpriv = file->driver_priv;
+	if (!fpriv || !fpriv->vm.root.bo)
+		return -ENODEV;
+
+	root_bo = amdgpu_bo_ref(fpriv->vm.root.bo);
+	r = amdgpu_bo_reserve(root_bo, true);
+	if (r) {
+		amdgpu_bo_unref(&root_bo);
+		return 0;
+	}
+
+	rcu_read_lock();
+	pid = rcu_dereference(file->pid);
+	task = pid_task(pid, PIDTYPE_TGID);
+
+	seq_printf(m, "pid: %d\n", task ? task->pid : 0);
+	seq_printf(m, "comm: %s\n", task ? task->comm : "Unset");
+	seq_printf(m, "pt_base: 0x%llx\n", amdgpu_bo_gpu_offset(fpriv->vm.root.bo));
+
+	rcu_read_unlock();
+	amdgpu_bo_unreserve(root_bo);
+	amdgpu_bo_unref(&root_bo);
+
+	return 0;
+}
+
+static int amdgpu_pt_info_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, amdgpu_pt_info_read, inode->i_private);
+}
+
+static const struct file_operations amdgpu_pt_info_fops = {
+	.owner = THIS_MODULE,
+	.open = amdgpu_pt_info_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+#endif
+
 /**
  * amdgpu_vm_init - initialize a vm instance
  *
  * @adev: amdgpu_device pointer
  * @vm: requested vm
  * @xcp_id: GPU partition selection id
+ * @file: drm_file
  *
  * Init @vm fields.
  *
@@ -2533,7 +2588,7 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
  * 0 for success, error for failure.
  */
 int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
-		   int32_t xcp_id)
+		   int32_t xcp_id, struct drm_file *file)
 {
 	struct amdgpu_bo *root_bo;
 	struct amdgpu_bo_vm *root;
@@ -2609,6 +2664,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	if (r)
 		DRM_DEBUG("Failed to create task info for VM\n");
 
+	debugfs_create_file("pt_info", 0444, file->debugfs_client, file, &amdgpu_pt_info_fops);
 	amdgpu_bo_unreserve(vm->root.bo);
 	amdgpu_bo_unref(&root_bo);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index f3ad687125ad..555afaf867c4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -487,7 +487,9 @@ int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			u32 pasid);
 
 long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
-int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp_id);
+int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t xcp_id,
+		   struct drm_file *file);
+
 int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 int amdgpu_vm_lock_pd(struct amdgpu_vm *vm, struct drm_exec *exec,
-- 
2.34.1

[PATCH v4 4/4] drm/amdgpu: add support of debugfs for mqd information

[Sunil Khatri]

add mqd support based on queue of for each client-id
so the gpu address could be used to dump the mqd.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 295e7186e156..50b9df9caf00 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -343,6 +343,46 @@ static int amdgpu_userq_priority_permit(struct drm_file *filp,
 	return -EACCES;
 }
 
+#if defined(CONFIG_DEBUG_FS)
+static int amdgpu_mqd_info_read(struct seq_file *m, void *unused)
+{
+	struct amdgpu_usermode_queue *queue = (struct amdgpu_usermode_queue *)m->private;
+	struct amdgpu_bo *bo;
+	int r;
+
+	if (!queue || !queue->mqd.obj)
+		return -EINVAL;
+
+	bo = amdgpu_bo_ref(queue->mqd.obj);
+	r = amdgpu_bo_reserve(bo, true);
+	if (r) {
+		amdgpu_bo_unref(&bo);
+		return 0;
+	}
+
+	seq_printf(m, "queue_type %d\n", queue->queue_type);
+	seq_printf(m, "mqd_gpu_address: 0x%llx\n", amdgpu_bo_gpu_offset(queue->mqd.obj));
+
+	amdgpu_bo_unreserve(bo);
+	amdgpu_bo_unref(&bo);
+
+	return 0;
+}
+
+static int amdgpu_mqd_info_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, amdgpu_mqd_info_read, inode->i_private);
+}
+
+static const struct file_operations amdgpu_mqd_info_fops = {
+	.owner = THIS_MODULE,
+	.open = amdgpu_mqd_info_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+#endif
+
 static int
 amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 {
@@ -352,6 +392,8 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	const struct amdgpu_userq_funcs *uq_funcs;
 	struct amdgpu_usermode_queue *queue;
 	struct amdgpu_db_info db_info;
+	char *queue_name;
+	struct dentry *debugfs_queue;
 	bool skip_map_queue;
 	uint64_t index;
 	int qid, r = 0;
@@ -475,6 +517,14 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 		}
 	}
 
+	queue_name = kasprintf(GFP_KERNEL, "queue-%d", qid);
+	if (!queue_name)
+		return -ENOMEM;
+
+	/* Create a debugfs directory for the client in root on drm debugfs */
+	debugfs_queue = debugfs_create_dir(queue_name, filp->debugfs_client);
+	debugfs_create_file("mqd_info", 0444, debugfs_queue, queue, &amdgpu_mqd_info_fops);
+	kfree(queue_name);
 
 	args->out.queue_id = qid;
 
-- 
2.34.1

Re: [PATCH v4 1/4] drm: move debugfs functionality from drm_drv.c to drm_debugfs.c

[Christian König]

On 6/18/25 15:47, Sunil Khatri wrote:
> move the functions from drm_drv.c which uses the static
> drm_debugfs_root as parent node in the debugfs by drm.
> 
> move this root node to the debugfs for easily handling
> of future requirements to add more information in the
> root directory and one of which is planned to have
> directories for each client in the root directory
> which is dri.
> 
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c  | 22 ++++++++++++++++++----
>  drivers/gpu/drm/drm_drv.c      | 11 ++++-------
>  drivers/gpu/drm/drm_internal.h |  6 ++----
>  include/drm/drm_drv.h          | 10 ++++++++++
>  4 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 2d43bda82887..5a33ec299c04 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -44,6 +44,8 @@
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
>  
> +static struct dentry *drm_debugfs_root;
> +
>  /***************************************************
>   * Initialization, etc.
>   **************************************************/
> @@ -286,6 +288,16 @@ int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
>  }
>  EXPORT_SYMBOL(drm_debugfs_remove_files);
>  
> +void drm_debugfs_create_dir(void)

I think we need a better name for this. drm_debugfs_init_root maybe? Ideas welcome.

> +{
> +	drm_debugfs_root = debugfs_create_dir("dri", NULL);
> +}
> +
> +void drm_debugfs_remove_dir(void)
> +{
> +	debugfs_remove(drm_debugfs_root);
> +}
> +
>  /**
>   * drm_debugfs_dev_init - create debugfs directory for the device
>   * @dev: the device which we want to create the directory for
> @@ -295,7 +307,10 @@ EXPORT_SYMBOL(drm_debugfs_remove_files);
>   */
>  void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root)
>  {
> -	dev->debugfs_root = debugfs_create_dir(dev->unique, root);
> +	if (!root)
> +		dev->debugfs_root = debugfs_create_dir(dev->unique, drm_debugfs_root);
> +	else
> +		dev->debugfs_root = debugfs_create_dir(dev->unique, root);

Ah! That is also used from the accel subsystem, isn't it?

Probably best to move accel_debugfs_root into this here as well and distinct based on drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) where to create the debugfs directory.

We probably need the same distinction for other use cases as well, so probably best to create a helper function for that.

Apart from that looks really good to me.

Regards,
Christian.

>  }
>  
>  /**
> @@ -322,14 +337,13 @@ void drm_debugfs_dev_register(struct drm_device *dev)
>  		drm_atomic_debugfs_init(dev);
>  }
>  
> -int drm_debugfs_register(struct drm_minor *minor, int minor_id,
> -			 struct dentry *root)
> +int drm_debugfs_register(struct drm_minor *minor, int minor_id)
>  {
>  	struct drm_device *dev = minor->dev;
>  	char name[64];
>  
>  	sprintf(name, "%d", minor_id);
> -	minor->debugfs_symlink = debugfs_create_symlink(name, root,
> +	minor->debugfs_symlink = debugfs_create_symlink(name, drm_debugfs_root,
>  							dev->unique);
>  
>  	/* TODO: Only for compatibility with drivers */
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 17fc5dc708f4..8abc52eac8f3 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -69,8 +69,6 @@ DEFINE_XARRAY_ALLOC(drm_minors_xa);
>   */
>  static bool drm_core_init_complete;
>  
> -static struct dentry *drm_debugfs_root;
> -
>  DEFINE_STATIC_SRCU(drm_unplug_srcu);
>  
>  /*
> @@ -183,8 +181,7 @@ static int drm_minor_register(struct drm_device *dev, enum drm_minor_type type)
>  		return 0;
>  
>  	if (minor->type != DRM_MINOR_ACCEL) {
> -		ret = drm_debugfs_register(minor, minor->index,
> -					   drm_debugfs_root);
> +		ret = drm_debugfs_register(minor, minor->index);
>  		if (ret) {
>  			DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
>  			goto err_debugfs;
> @@ -754,7 +751,7 @@ static int drm_dev_init(struct drm_device *dev,
>  	if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL))
>  		accel_debugfs_init(dev);
>  	else
> -		drm_debugfs_dev_init(dev, drm_debugfs_root);
> +		drm_debugfs_dev_init(dev, NULL);
>  
>  	return 0;
>  
> @@ -1168,7 +1165,7 @@ static void drm_core_exit(void)
>  	drm_panic_exit();
>  	accel_core_exit();
>  	unregister_chrdev(DRM_MAJOR, "drm");
> -	debugfs_remove(drm_debugfs_root);
> +	drm_debugfs_remove_dir();
>  	drm_sysfs_destroy();
>  	WARN_ON(!xa_empty(&drm_minors_xa));
>  	drm_connector_ida_destroy();
> @@ -1187,7 +1184,7 @@ static int __init drm_core_init(void)
>  		goto error;
>  	}
>  
> -	drm_debugfs_root = debugfs_create_dir("dri", NULL);
> +	drm_debugfs_create_dir();
>  
>  	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index b2b6a8e49dda..d2d8e72f32d9 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -186,8 +186,7 @@ void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
>  #if defined(CONFIG_DEBUG_FS)
>  void drm_debugfs_dev_fini(struct drm_device *dev);
>  void drm_debugfs_dev_register(struct drm_device *dev);
> -int drm_debugfs_register(struct drm_minor *minor, int minor_id,
> -			 struct dentry *root);
> +int drm_debugfs_register(struct drm_minor *minor, int minor_id);
>  void drm_debugfs_unregister(struct drm_minor *minor);
>  void drm_debugfs_connector_add(struct drm_connector *connector);
>  void drm_debugfs_connector_remove(struct drm_connector *connector);
> @@ -205,8 +204,7 @@ static inline void drm_debugfs_dev_register(struct drm_device *dev)
>  {
>  }
>  
> -static inline int drm_debugfs_register(struct drm_minor *minor, int minor_id,
> -				       struct dentry *root)
> +static inline int drm_debugfs_register(struct drm_minor *minor, int minor_id)
>  {
>  	return 0;
>  }
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index a43d707b5f36..4e77a0c4a7f9 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -567,10 +567,20 @@ static inline bool drm_firmware_drivers_only(void)
>  
>  #if defined(CONFIG_DEBUG_FS)
>  void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root);
> +void drm_debugfs_create_dir(void);
> +void drm_debugfs_remove_dir(void);
>  #else
>  static inline void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root)
>  {
>  }
> +
> +static inline void drm_debugfs_create_dir(void)
> +{
> +}
> +
> +static inline void drm_debugfs_remove_dir(void)
> +{
> +}
>  #endif
>  
>  #endif

Re: [PATCH v4 2/4] drm: add debugfs support on per client-id basis

[Christian König]

On 6/18/25 15:47, Sunil Khatri wrote:
> add support to add a directory for each client-id
> with root at the dri level. Since the clients are
> unique and not just related to one single drm device,
> so it makes more sense to add all the client based
> nodes with root as dri.
> 
> Also create a symlink back to the parent drm device
> from each client.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c | 32 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_file.c    | 10 ++++++++++
>  include/drm/drm_debugfs.h     | 12 ++++++++++++
>  include/drm/drm_file.h        |  7 +++++++
>  4 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 5a33ec299c04..875276d5fb9f 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -298,6 +298,38 @@ void drm_debugfs_remove_dir(void)
>  	debugfs_remove(drm_debugfs_root);
>  }
>  
> +int drm_debugfs_clients_add(struct drm_file *file)
> +{
> +	struct drm_device *dev;
> +	char *client_dir, *symlink;
> +
> +	dev = file->minor->dev;
> +
> +	client_dir = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
> +	if (!client_dir)
> +		return -ENOMEM;
> +
> +	/* Create a debugfs directory for the client in root on drm debugfs */

That comments on what is done, so probably not a good idea.

But we should certainly have some explanation why we do this! Probably best to add some overview under Documentation/gpu.

Apart from that this works technically for me.

Regards,
Christian.


> +	file->debugfs_client = debugfs_create_dir(client_dir, drm_debugfs_root);
> +	kfree(client_dir);
> +
> +	symlink = kasprintf(GFP_KERNEL, "../%s", dev->unique);
> +	if (!symlink)
> +		return -ENOMEM;
> +
> +	/* Create a link from client_id to the drm device this client id belongs to */
> +	debugfs_create_symlink("device", file->debugfs_client, symlink);
> +	kfree(symlink);
> +
> +	return 0;
> +}
> +
> +void drm_debugfs_clients_remove(struct drm_file *file)
> +{
> +	debugfs_remove_recursive(file->debugfs_client);
> +	file->debugfs_client = NULL;
> +}
> +
>  /**
>   * drm_debugfs_dev_init - create debugfs directory for the device
>   * @dev: the device which we want to create the directory for
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 06ba6dcbf5ae..8502c5a630b1 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -39,12 +39,14 @@
>  #include <linux/poll.h>
>  #include <linux/slab.h>
>  #include <linux/vga_switcheroo.h>
> +#include <linux/debugfs.h>
>  
>  #include <drm/drm_client_event.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_print.h>
> +#include <drm/drm_debugfs.h>
>  
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
> @@ -143,6 +145,13 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>  	rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>  	file->minor = minor;
>  
> +	ret = drm_debugfs_clients_add(file);
> +	if (ret) {
> +		put_pid(rcu_access_pointer(file->pid));
> +		kfree(file);
> +		return ERR_PTR(ret);
> +	}
> +
>  	/* for compatibility root is always authenticated */
>  	file->authenticated = capable(CAP_SYS_ADMIN);
>  
> @@ -236,6 +245,7 @@ void drm_file_free(struct drm_file *file)
>  		     atomic_read(&dev->open_count));
>  
>  	drm_events_release(file);
> +	drm_debugfs_clients_remove(file);
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		drm_fb_release(file);
> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> index cf06cee4343f..4bd6cc1d0900 100644
> --- a/include/drm/drm_debugfs.h
> +++ b/include/drm/drm_debugfs.h
> @@ -153,6 +153,9 @@ void drm_debugfs_add_files(struct drm_device *dev,
>  
>  int drm_debugfs_gpuva_info(struct seq_file *m,
>  			   struct drm_gpuvm *gpuvm);
> +
> +int drm_debugfs_clients_add(struct drm_file *file);
> +void drm_debugfs_clients_remove(struct drm_file *file);
>  #else
>  static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>  					    int count, struct dentry *root,
> @@ -181,6 +184,15 @@ static inline int drm_debugfs_gpuva_info(struct seq_file *m,
>  {
>  	return 0;
>  }
> +
> +int drm_debugfs_clients_add(struct drm_file *file)
> +{
> +	return 0;
> +}
> +
> +void drm_debugfs_clients_remove(struct drm_file *file)
> +{
> +}
>  #endif
>  
>  #endif /* _DRM_DEBUGFS_H_ */
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 5c3b2aa3e69d..eab7546aad79 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -400,6 +400,13 @@ struct drm_file {
>  	 * @client_name_lock: Protects @client_name.
>  	 */
>  	struct mutex client_name_lock;
> +
> +	/**
> +	 * @debugfs_client:
> +	 *
> +	 * debugfs directory for each client under a drm node.
> +	 */
> +	struct dentry *debugfs_client;
>  };
>  
>  /**

Re: [PATCH v4 1/4] drm: move debugfs functionality from drm_drv.c to drm_debugfs.c

[Khatri, Sunil]

On 6/18/2025 7:38 PM, Christian König wrote:
> On 6/18/25 15:47, Sunil Khatri wrote:
>> move the functions from drm_drv.c which uses the static
>> drm_debugfs_root as parent node in the debugfs by drm.
>>
>> move this root node to the debugfs for easily handling
>> of future requirements to add more information in the
>> root directory and one of which is planned to have
>> directories for each client in the root directory
>> which is dri.
>>
>> Suggested-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/drm_debugfs.c  | 22 ++++++++++++++++++----
>>   drivers/gpu/drm/drm_drv.c      | 11 ++++-------
>>   drivers/gpu/drm/drm_internal.h |  6 ++----
>>   include/drm/drm_drv.h          | 10 ++++++++++
>>   4 files changed, 34 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 2d43bda82887..5a33ec299c04 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -44,6 +44,8 @@
>>   #include "drm_crtc_internal.h"
>>   #include "drm_internal.h"
>>   
>> +static struct dentry *drm_debugfs_root;
>> +
>>   /***************************************************
>>    * Initialization, etc.
>>    **************************************************/
>> @@ -286,6 +288,16 @@ int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
>>   }
>>   EXPORT_SYMBOL(drm_debugfs_remove_files);
>>   
>> +void drm_debugfs_create_dir(void)
> I think we need a better name for this. drm_debugfs_init_root maybe? Ideas welcome.

Sounds good to me.

Regards
Sunil Khatri

>
>> +{
>> +	drm_debugfs_root = debugfs_create_dir("dri", NULL);
>> +}
>> +
>> +void drm_debugfs_remove_dir(void)
>> +{
>> +	debugfs_remove(drm_debugfs_root);
>> +}
>> +
>>   /**
>>    * drm_debugfs_dev_init - create debugfs directory for the device
>>    * @dev: the device which we want to create the directory for
>> @@ -295,7 +307,10 @@ EXPORT_SYMBOL(drm_debugfs_remove_files);
>>    */
>>   void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root)
>>   {
>> -	dev->debugfs_root = debugfs_create_dir(dev->unique, root);
>> +	if (!root)
>> +		dev->debugfs_root = debugfs_create_dir(dev->unique, drm_debugfs_root);
>> +	else
>> +		dev->debugfs_root = debugfs_create_dir(dev->unique, root);
> Ah! That is also used from the accel subsystem, isn't it?
Yes
>
> Probably best to move accel_debugfs_root into this here as well and distinct based on drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) where to create the debugfs directory.
>
> We probably need the same distinction for other use cases as well, so probably best to create a helper function for that.
I did explore that accel_debugfs_init in file drivers/accel/drm_accel.c 
and i dont think of a clear and acceptable design. It is a different 
module all together and we dont want to create debugfs directory for it 
in drm code. If the module fail for some reason and also removing the 
debugfs directory is another problem. We do not know when we want to 
init/exit the module.

The approach that i used is that drm_debugfs_dev_init is used by other 
module(right now there is one but we never know in future) as well as 
drm itself. The parent node is passed by the caller and for drm itself 
the parent node is defined statically.  So with the approach i took is 
if parent node is NULL its an internal call within drm and we can 
directly use the the static dentry i.e drm_debugfs_root.


Can we move below call from accel_core_init to drm_debugfs.c although 
its a different module all together for accel ??

accel_debugfs_root = debugfs_create_dir("accel", NULL);

>
> Apart from that looks really good to me.
>
> Regards,
> Christian.
>
>>   }
>>   
>>   /**
>> @@ -322,14 +337,13 @@ void drm_debugfs_dev_register(struct drm_device *dev)
>>   		drm_atomic_debugfs_init(dev);
>>   }
>>   
>> -int drm_debugfs_register(struct drm_minor *minor, int minor_id,
>> -			 struct dentry *root)
>> +int drm_debugfs_register(struct drm_minor *minor, int minor_id)
>>   {
>>   	struct drm_device *dev = minor->dev;
>>   	char name[64];
>>   
>>   	sprintf(name, "%d", minor_id);
>> -	minor->debugfs_symlink = debugfs_create_symlink(name, root,
>> +	minor->debugfs_symlink = debugfs_create_symlink(name, drm_debugfs_root,
>>   							dev->unique);
>>   
>>   	/* TODO: Only for compatibility with drivers */
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 17fc5dc708f4..8abc52eac8f3 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -69,8 +69,6 @@ DEFINE_XARRAY_ALLOC(drm_minors_xa);
>>    */
>>   static bool drm_core_init_complete;
>>   
>> -static struct dentry *drm_debugfs_root;
>> -
>>   DEFINE_STATIC_SRCU(drm_unplug_srcu);
>>   
>>   /*
>> @@ -183,8 +181,7 @@ static int drm_minor_register(struct drm_device *dev, enum drm_minor_type type)
>>   		return 0;
>>   
>>   	if (minor->type != DRM_MINOR_ACCEL) {
>> -		ret = drm_debugfs_register(minor, minor->index,
>> -					   drm_debugfs_root);
>> +		ret = drm_debugfs_register(minor, minor->index);
>>   		if (ret) {
>>   			DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
>>   			goto err_debugfs;
>> @@ -754,7 +751,7 @@ static int drm_dev_init(struct drm_device *dev,
>>   	if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL))
>>   		accel_debugfs_init(dev);
>>   	else
>> -		drm_debugfs_dev_init(dev, drm_debugfs_root);
>> +		drm_debugfs_dev_init(dev, NULL);
>>   
>>   	return 0;
>>   
>> @@ -1168,7 +1165,7 @@ static void drm_core_exit(void)
>>   	drm_panic_exit();
>>   	accel_core_exit();
>>   	unregister_chrdev(DRM_MAJOR, "drm");
>> -	debugfs_remove(drm_debugfs_root);
>> +	drm_debugfs_remove_dir();
>>   	drm_sysfs_destroy();
>>   	WARN_ON(!xa_empty(&drm_minors_xa));
>>   	drm_connector_ida_destroy();
>> @@ -1187,7 +1184,7 @@ static int __init drm_core_init(void)
>>   		goto error;
>>   	}
>>   
>> -	drm_debugfs_root = debugfs_create_dir("dri", NULL);
>> +	drm_debugfs_create_dir();
>>   
>>   	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
>>   	if (ret < 0)
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index b2b6a8e49dda..d2d8e72f32d9 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -186,8 +186,7 @@ void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
>>   #if defined(CONFIG_DEBUG_FS)
>>   void drm_debugfs_dev_fini(struct drm_device *dev);
>>   void drm_debugfs_dev_register(struct drm_device *dev);
>> -int drm_debugfs_register(struct drm_minor *minor, int minor_id,
>> -			 struct dentry *root);
>> +int drm_debugfs_register(struct drm_minor *minor, int minor_id);
>>   void drm_debugfs_unregister(struct drm_minor *minor);
>>   void drm_debugfs_connector_add(struct drm_connector *connector);
>>   void drm_debugfs_connector_remove(struct drm_connector *connector);
>> @@ -205,8 +204,7 @@ static inline void drm_debugfs_dev_register(struct drm_device *dev)
>>   {
>>   }
>>   
>> -static inline int drm_debugfs_register(struct drm_minor *minor, int minor_id,
>> -				       struct dentry *root)
>> +static inline int drm_debugfs_register(struct drm_minor *minor, int minor_id)
>>   {
>>   	return 0;
>>   }
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index a43d707b5f36..4e77a0c4a7f9 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -567,10 +567,20 @@ static inline bool drm_firmware_drivers_only(void)
>>   
>>   #if defined(CONFIG_DEBUG_FS)
>>   void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root);
>> +void drm_debugfs_create_dir(void);
>> +void drm_debugfs_remove_dir(void);
>>   #else
>>   static inline void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root)
>>   {
>>   }
>> +
>> +static inline void drm_debugfs_create_dir(void)
>> +{
>> +}
>> +
>> +static inline void drm_debugfs_remove_dir(void)
>> +{
>> +}
>>   #endif
>>   
>>   #endif

Re: [PATCH v4 1/4] drm: move debugfs functionality from drm_drv.c to drm_debugfs.c

[Khatri, Sunil]

On 6/19/2025 10:37 AM, Khatri, Sunil wrote:
>
> On 6/18/2025 7:38 PM, Christian König wrote:
>> On 6/18/25 15:47, Sunil Khatri wrote:
>>> move the functions from drm_drv.c which uses the static
>>> drm_debugfs_root as parent node in the debugfs by drm.
>>>
>>> move this root node to the debugfs for easily handling
>>> of future requirements to add more information in the
>>> root directory and one of which is planned to have
>>> directories for each client in the root directory
>>> which is dri.
>>>
>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_debugfs.c  | 22 ++++++++++++++++++----
>>>   drivers/gpu/drm/drm_drv.c      | 11 ++++-------
>>>   drivers/gpu/drm/drm_internal.h |  6 ++----
>>>   include/drm/drm_drv.h          | 10 ++++++++++
>>>   4 files changed, 34 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_debugfs.c 
>>> b/drivers/gpu/drm/drm_debugfs.c
>>> index 2d43bda82887..5a33ec299c04 100644
>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>> @@ -44,6 +44,8 @@
>>>   #include "drm_crtc_internal.h"
>>>   #include "drm_internal.h"
>>>   +static struct dentry *drm_debugfs_root;
>>> +
>>>   /***************************************************
>>>    * Initialization, etc.
>>>    **************************************************/
>>> @@ -286,6 +288,16 @@ int drm_debugfs_remove_files(const struct 
>>> drm_info_list *files, int count,
>>>   }
>>>   EXPORT_SYMBOL(drm_debugfs_remove_files);
>>>   +void drm_debugfs_create_dir(void)
>> I think we need a better name for this. drm_debugfs_init_root maybe? 
>> Ideas welcome.
You mean the function name drm_debugfs_create_dir ??
>
> Sounds good to me.
>
> Regards
> Sunil Khatri
>
>>
>>> +{
>>> +    drm_debugfs_root = debugfs_create_dir("dri", NULL);
>>> +}
>>> +
>>> +void drm_debugfs_remove_dir(void)
>>> +{
>>> +    debugfs_remove(drm_debugfs_root);
>>> +}
>>> +
>>>   /**
>>>    * drm_debugfs_dev_init - create debugfs directory for the device
>>>    * @dev: the device which we want to create the directory for
>>> @@ -295,7 +307,10 @@ EXPORT_SYMBOL(drm_debugfs_remove_files);
>>>    */
>>>   void drm_debugfs_dev_init(struct drm_device *dev, struct dentry 
>>> *root)
>>>   {
>>> -    dev->debugfs_root = debugfs_create_dir(dev->unique, root);
>>> +    if (!root)
>>> +        dev->debugfs_root = debugfs_create_dir(dev->unique, 
>>> drm_debugfs_root);
>>> +    else
>>> +        dev->debugfs_root = debugfs_create_dir(dev->unique, root);
>> Ah! That is also used from the accel subsystem, isn't it?
> Yes
>>
>> Probably best to move accel_debugfs_root into this here as well and 
>> distinct based on drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) 
>> where to create the debugfs directory.
I checked this code again and we could not use 
drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) as at that time as 
there is no drm_device as its done too early and done before any 
drm_device is created.

I have something in mind as i found that accel_core_init is called from 
drm_drv and we can do something there itself. I will push another patch 
set and lets see how you find that.

Regards
Sunil khatri

>>
>> We probably need the same distinction for other use cases as well, so 
>> probably best to create a helper function for that.
> I did explore that accel_debugfs_init in file 
> drivers/accel/drm_accel.c and i dont think of a clear and acceptable 
> design. It is a different module all together and we dont want to 
> create debugfs directory for it in drm code. If the module fail for 
> some reason and also removing the debugfs directory is another 
> problem. We do not know when we want to init/exit the module.
>
> The approach that i used is that drm_debugfs_dev_init is used by other 
> module(right now there is one but we never know in future) as well as 
> drm itself. The parent node is passed by the caller and for drm itself 
> the parent node is defined statically.  So with the approach i took is 
> if parent node is NULL its an internal call within drm and we can 
> directly use the the static dentry i.e drm_debugfs_root.
>
>
> Can we move below call from accel_core_init to drm_debugfs.c although 
> its a different module all together for accel ??
>
> accel_debugfs_root = debugfs_create_dir("accel", NULL);
>
>>
>> Apart from that looks really good to me.
>>
>> Regards,
>> Christian.
>>
>>>   }
>>>     /**
>>> @@ -322,14 +337,13 @@ void drm_debugfs_dev_register(struct 
>>> drm_device *dev)
>>>           drm_atomic_debugfs_init(dev);
>>>   }
>>>   -int drm_debugfs_register(struct drm_minor *minor, int minor_id,
>>> -             struct dentry *root)
>>> +int drm_debugfs_register(struct drm_minor *minor, int minor_id)
>>>   {
>>>       struct drm_device *dev = minor->dev;
>>>       char name[64];
>>>         sprintf(name, "%d", minor_id);
>>> -    minor->debugfs_symlink = debugfs_create_symlink(name, root,
>>> +    minor->debugfs_symlink = debugfs_create_symlink(name, 
>>> drm_debugfs_root,
>>>                               dev->unique);
>>>         /* TODO: Only for compatibility with drivers */
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 17fc5dc708f4..8abc52eac8f3 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -69,8 +69,6 @@ DEFINE_XARRAY_ALLOC(drm_minors_xa);
>>>    */
>>>   static bool drm_core_init_complete;
>>>   -static struct dentry *drm_debugfs_root;
>>> -
>>>   DEFINE_STATIC_SRCU(drm_unplug_srcu);
>>>     /*
>>> @@ -183,8 +181,7 @@ static int drm_minor_register(struct drm_device 
>>> *dev, enum drm_minor_type type)
>>>           return 0;
>>>         if (minor->type != DRM_MINOR_ACCEL) {
>>> -        ret = drm_debugfs_register(minor, minor->index,
>>> -                       drm_debugfs_root);
>>> +        ret = drm_debugfs_register(minor, minor->index);
>>>           if (ret) {
>>>               DRM_ERROR("DRM: Failed to initialize 
>>> /sys/kernel/debug/dri.\n");
>>>               goto err_debugfs;
>>> @@ -754,7 +751,7 @@ static int drm_dev_init(struct drm_device *dev,
>>>       if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL))
>>>           accel_debugfs_init(dev);
>>>       else
>>> -        drm_debugfs_dev_init(dev, drm_debugfs_root);
>>> +        drm_debugfs_dev_init(dev, NULL);
>>>         return 0;
>>>   @@ -1168,7 +1165,7 @@ static void drm_core_exit(void)
>>>       drm_panic_exit();
>>>       accel_core_exit();
>>>       unregister_chrdev(DRM_MAJOR, "drm");
>>> -    debugfs_remove(drm_debugfs_root);
>>> +    drm_debugfs_remove_dir();
>>>       drm_sysfs_destroy();
>>>       WARN_ON(!xa_empty(&drm_minors_xa));
>>>       drm_connector_ida_destroy();
>>> @@ -1187,7 +1184,7 @@ static int __init drm_core_init(void)
>>>           goto error;
>>>       }
>>>   -    drm_debugfs_root = debugfs_create_dir("dri", NULL);
>>> +    drm_debugfs_create_dir();
>>>         ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
>>>       if (ret < 0)
>>> diff --git a/drivers/gpu/drm/drm_internal.h 
>>> b/drivers/gpu/drm/drm_internal.h
>>> index b2b6a8e49dda..d2d8e72f32d9 100644
>>> --- a/drivers/gpu/drm/drm_internal.h
>>> +++ b/drivers/gpu/drm/drm_internal.h
>>> @@ -186,8 +186,7 @@ void drm_gem_vunmap(struct drm_gem_object *obj, 
>>> struct iosys_map *map);
>>>   #if defined(CONFIG_DEBUG_FS)
>>>   void drm_debugfs_dev_fini(struct drm_device *dev);
>>>   void drm_debugfs_dev_register(struct drm_device *dev);
>>> -int drm_debugfs_register(struct drm_minor *minor, int minor_id,
>>> -             struct dentry *root);
>>> +int drm_debugfs_register(struct drm_minor *minor, int minor_id);
>>>   void drm_debugfs_unregister(struct drm_minor *minor);
>>>   void drm_debugfs_connector_add(struct drm_connector *connector);
>>>   void drm_debugfs_connector_remove(struct drm_connector *connector);
>>> @@ -205,8 +204,7 @@ static inline void 
>>> drm_debugfs_dev_register(struct drm_device *dev)
>>>   {
>>>   }
>>>   -static inline int drm_debugfs_register(struct drm_minor *minor, 
>>> int minor_id,
>>> -                       struct dentry *root)
>>> +static inline int drm_debugfs_register(struct drm_minor *minor, int 
>>> minor_id)
>>>   {
>>>       return 0;
>>>   }
>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>> index a43d707b5f36..4e77a0c4a7f9 100644
>>> --- a/include/drm/drm_drv.h
>>> +++ b/include/drm/drm_drv.h
>>> @@ -567,10 +567,20 @@ static inline bool 
>>> drm_firmware_drivers_only(void)
>>>     #if defined(CONFIG_DEBUG_FS)
>>>   void drm_debugfs_dev_init(struct drm_device *dev, struct dentry 
>>> *root);
>>> +void drm_debugfs_create_dir(void);
>>> +void drm_debugfs_remove_dir(void);
>>>   #else
>>>   static inline void drm_debugfs_dev_init(struct drm_device *dev, 
>>> struct dentry *root)
>>>   {
>>>   }
>>> +
>>> +static inline void drm_debugfs_create_dir(void)
>>> +{
>>> +}
>>> +
>>> +static inline void drm_debugfs_remove_dir(void)
>>> +{
>>> +}
>>>   #endif
>>>     #endif

Re: [PATCH v4 2/4] drm: add debugfs support on per client-id basis

[Tvrtko Ursulin]

On 18/06/2025 14:47, Sunil Khatri wrote:
> add support to add a directory for each client-id
> with root at the dri level. Since the clients are
> unique and not just related to one single drm device,
> so it makes more sense to add all the client based
> nodes with root as dri.
> 
> Also create a symlink back to the parent drm device
> from each client.

TBH I can see an use case for both clients at DRI level and clients 
under DRM devices. I guess you have an use case for global and per 
device can be added later if it becomes needed.

> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>   drivers/gpu/drm/drm_debugfs.c | 32 ++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/drm_file.c    | 10 ++++++++++
>   include/drm/drm_debugfs.h     | 12 ++++++++++++
>   include/drm/drm_file.h        |  7 +++++++
>   4 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 5a33ec299c04..875276d5fb9f 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -298,6 +298,38 @@ void drm_debugfs_remove_dir(void)
>   	debugfs_remove(drm_debugfs_root);
>   }
>   
> +int drm_debugfs_clients_add(struct drm_file *file)
> +{
> +	struct drm_device *dev;
> +	char *client_dir, *symlink;
> +
> +	dev = file->minor->dev;

FWIW, as dev is only used once and string locals are not overlapping, 
you could reduce to a single local variable like char *name and re-use 
it. Up to you.

> +
> +	client_dir = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
> +	if (!client_dir)
> +		return -ENOMEM;

It is a bit more work, but I think a clients/ directory with numerical 
client id subdirs would be nicer.

> +
> +	/* Create a debugfs directory for the client in root on drm debugfs */
> +	file->debugfs_client = debugfs_create_dir(client_dir, drm_debugfs_root);
> +	kfree(client_dir);
> +
> +	symlink = kasprintf(GFP_KERNEL, "../%s", dev->unique);
> +	if (!symlink)
> +		return -ENOMEM;

Worth removing the partial construction?

> +
> +	/* Create a link from client_id to the drm device this client id belongs to */
> +	debugfs_create_symlink("device", file->debugfs_client, symlink);

This can also fail.

> +	kfree(symlink);
> +
> +	return 0;
> +}
> +
> +void drm_debugfs_clients_remove(struct drm_file *file)
> +{
> +	debugfs_remove_recursive(file->debugfs_client);
> +	file->debugfs_client = NULL;
> +}
> +
>   /**
>    * drm_debugfs_dev_init - create debugfs directory for the device
>    * @dev: the device which we want to create the directory for
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 06ba6dcbf5ae..8502c5a630b1 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -39,12 +39,14 @@
>   #include <linux/poll.h>
>   #include <linux/slab.h>
>   #include <linux/vga_switcheroo.h>
> +#include <linux/debugfs.h>
>   
>   #include <drm/drm_client_event.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_gem.h>
>   #include <drm/drm_print.h>
> +#include <drm/drm_debugfs.h>
>   
>   #include "drm_crtc_internal.h"
>   #include "drm_internal.h"
> @@ -143,6 +145,13 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>   	rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>   	file->minor = minor;
>   
> +	ret = drm_debugfs_clients_add(file);

Slightly tricky part is that as soon as this runs userspace can enter 
debugfs. If in the future any debufs clients file is added which can 
dereference any of the drm_file fields not yet initialized it has the 
potential to explode and/or be exploited.

Hence I think to be safe the usual pattern of exposing drm_file to 
userspace at the end, only _after_ drm_file has been *fully* initialized.

Slightly annoying part with that might be undoing dev->driver->open() 
but maybe it is not that bad.

> +	if (ret) {
> +		put_pid(rcu_access_pointer(file->pid));
> +		kfree(file);
> +		return ERR_PTR(ret);

Onion unwind already exists in the function so could have used it. (Add 
a new label and here simply "goto out_put_pid".) But as above we discuss 
tweaking the order lets see how that goes first.

> +	}
> +
>   	/* for compatibility root is always authenticated */
>   	file->authenticated = capable(CAP_SYS_ADMIN);
>   
> @@ -236,6 +245,7 @@ void drm_file_free(struct drm_file *file)
>   		     atomic_read(&dev->open_count));
>   
>   	drm_events_release(file);
> +	drm_debugfs_clients_remove(file);
>   
>   	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>   		drm_fb_release(file);
> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> index cf06cee4343f..4bd6cc1d0900 100644
> --- a/include/drm/drm_debugfs.h
> +++ b/include/drm/drm_debugfs.h
> @@ -153,6 +153,9 @@ void drm_debugfs_add_files(struct drm_device *dev,
>   
>   int drm_debugfs_gpuva_info(struct seq_file *m,
>   			   struct drm_gpuvm *gpuvm);
> +
> +int drm_debugfs_clients_add(struct drm_file *file);
> +void drm_debugfs_clients_remove(struct drm_file *file);
>   #else
>   static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>   					    int count, struct dentry *root,
> @@ -181,6 +184,15 @@ static inline int drm_debugfs_gpuva_info(struct seq_file *m,
>   {
>   	return 0;
>   }
> +
> +int drm_debugfs_clients_add(struct drm_file *file)
> +{
> +	return 0;
> +}
> +
> +void drm_debugfs_clients_remove(struct drm_file *file)
> +{
> +}

Static inline for the two above.

>   #endif
>   
>   #endif /* _DRM_DEBUGFS_H_ */
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 5c3b2aa3e69d..eab7546aad79 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -400,6 +400,13 @@ struct drm_file {
>   	 * @client_name_lock: Protects @client_name.
>   	 */
>   	struct mutex client_name_lock;
> +
> +	/**
> +	 * @debugfs_client:
> +	 *
> +	 * debugfs directory for each client under a drm node.
> +	 */
> +	struct dentry *debugfs_client;

Is it worth idefing this out if !CONFIG_DEBUG_FS?

Regards,

Tvrtko

>   };
>   
>   /**

Re: [PATCH v4 2/4] drm: add debugfs support on per client-id basis

[Khatri, Sunil]

On 6/23/2025 2:58 PM, Tvrtko Ursulin wrote:
>
>
> On 18/06/2025 14:47, Sunil Khatri wrote:
>> add support to add a directory for each client-id
>> with root at the dri level. Since the clients are
>> unique and not just related to one single drm device,
>> so it makes more sense to add all the client based
>> nodes with root as dri.
>>
>> Also create a symlink back to the parent drm device
>> from each client.
>
> TBH I can see an use case for both clients at DRI level and clients 
> under DRM devices. I guess you have an use case for global and per 
> device can be added later if it becomes needed.
>
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/drm_debugfs.c | 32 ++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_file.c    | 10 ++++++++++
>>   include/drm/drm_debugfs.h     | 12 ++++++++++++
>>   include/drm/drm_file.h        |  7 +++++++
>>   4 files changed, 61 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs.c 
>> b/drivers/gpu/drm/drm_debugfs.c
>> index 5a33ec299c04..875276d5fb9f 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -298,6 +298,38 @@ void drm_debugfs_remove_dir(void)
>>       debugfs_remove(drm_debugfs_root);
>>   }
>>   +int drm_debugfs_clients_add(struct drm_file *file)
>> +{
>> +    struct drm_device *dev;
>> +    char *client_dir, *symlink;
>> +
>> +    dev = file->minor->dev;
>
> FWIW, as dev is only used once and string locals are not overlapping, 
> you could reduce to a single local variable like char *name and re-use 
> it. Up to you.
>
Let me see what i could do with that. But yes can reduce locals.

regards

Sunil

>> +
>> +    client_dir = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
>> +    if (!client_dir)
>> +        return -ENOMEM;
>
> It is a bit more work, but I think a clients/ directory with numerical 
> client id subdirs would be nicer.

It was with the id only first but with feedback from Christian i moved 
it with client-$. Also since we want it in main root directory along 
with nodes like 0 and 128, it makes sense to differentiate and make a clear

representation of clients.

>
>> +
>> +    /* Create a debugfs directory for the client in root on drm 
>> debugfs */
>> +    file->debugfs_client = debugfs_create_dir(client_dir, 
>> drm_debugfs_root);
>> +    kfree(client_dir);
>> +
>> +    symlink = kasprintf(GFP_KERNEL, "../%s", dev->unique);
>> +    if (!symlink)
>> +        return -ENOMEM;
>
> Worth removing the partial construction?
Ideally it should never fail and but yes makes sense to clean up.
>
>> +
>> +    /* Create a link from client_id to the drm device this client id 
>> belongs to */
>> +    debugfs_create_symlink("device", file->debugfs_client, symlink);
>
> This can also fail.
sure. Noted
>
>> +    kfree(symlink);
>> +
>> +    return 0;
>> +}
>> +
>> +void drm_debugfs_clients_remove(struct drm_file *file)
>> +{
>> +    debugfs_remove_recursive(file->debugfs_client);
>> +    file->debugfs_client = NULL;
>> +}
>> +
>>   /**
>>    * drm_debugfs_dev_init - create debugfs directory for the device
>>    * @dev: the device which we want to create the directory for
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 06ba6dcbf5ae..8502c5a630b1 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -39,12 +39,14 @@
>>   #include <linux/poll.h>
>>   #include <linux/slab.h>
>>   #include <linux/vga_switcheroo.h>
>> +#include <linux/debugfs.h>
>>     #include <drm/drm_client_event.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_file.h>
>>   #include <drm/drm_gem.h>
>>   #include <drm/drm_print.h>
>> +#include <drm/drm_debugfs.h>
>>     #include "drm_crtc_internal.h"
>>   #include "drm_internal.h"
>> @@ -143,6 +145,13 @@ struct drm_file *drm_file_alloc(struct drm_minor 
>> *minor)
>>       rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>>       file->minor = minor;
>>   +    ret = drm_debugfs_clients_add(file);
>
> Slightly tricky part is that as soon as this runs userspace can enter 
> debugfs. If in the future any debufs clients file is added which can 
> dereference any of the drm_file fields not yet initialized it has the 
> potential to explode and/or be exploited.
>
> Hence I think to be safe the usual pattern of exposing drm_file to 
> userspace at the end, only _after_ drm_file has been *fully* initialized.
>
> Slightly annoying part with that might be undoing dev->driver->open() 
> but maybe it is not that bad.

I need this before driver open as the entry is accessed in driver->open 
in amdgpu to add files to the directory.

So, i could see to move it just before the open but not after. Anyways 
if we reach till driver open surely file is fully initialized. Nothing 
else is done in that function after that.

>
>> +    if (ret) {
>> +        put_pid(rcu_access_pointer(file->pid));
>> +        kfree(file);
>> +        return ERR_PTR(ret);
>
> Onion unwind already exists in the function so could have used it. 
> (Add a new label and here simply "goto out_put_pid".) But as above we 
> discuss tweaking the order lets see how that goes first.
Sure.
>
>> +    }
>> +
>>       /* for compatibility root is always authenticated */
>>       file->authenticated = capable(CAP_SYS_ADMIN);
>>   @@ -236,6 +245,7 @@ void drm_file_free(struct drm_file *file)
>>                atomic_read(&dev->open_count));
>>         drm_events_release(file);
>> +    drm_debugfs_clients_remove(file);
>>         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>           drm_fb_release(file);
>> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
>> index cf06cee4343f..4bd6cc1d0900 100644
>> --- a/include/drm/drm_debugfs.h
>> +++ b/include/drm/drm_debugfs.h
>> @@ -153,6 +153,9 @@ void drm_debugfs_add_files(struct drm_device *dev,
>>     int drm_debugfs_gpuva_info(struct seq_file *m,
>>                  struct drm_gpuvm *gpuvm);
>> +
>> +int drm_debugfs_clients_add(struct drm_file *file);
>> +void drm_debugfs_clients_remove(struct drm_file *file);
>>   #else
>>   static inline void drm_debugfs_create_files(const struct 
>> drm_info_list *files,
>>                           int count, struct dentry *root,
>> @@ -181,6 +184,15 @@ static inline int drm_debugfs_gpuva_info(struct 
>> seq_file *m,
>>   {
>>       return 0;
>>   }
>> +
>> +int drm_debugfs_clients_add(struct drm_file *file)
>> +{
>> +    return 0;
>> +}
>> +
>> +void drm_debugfs_clients_remove(struct drm_file *file)
>> +{
>> +}
>
> Static inline for the two above.
Noted
>
>>   #endif
>>     #endif /* _DRM_DEBUGFS_H_ */
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 5c3b2aa3e69d..eab7546aad79 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -400,6 +400,13 @@ struct drm_file {
>>        * @client_name_lock: Protects @client_name.
>>        */
>>       struct mutex client_name_lock;
>> +
>> +    /**
>> +     * @debugfs_client:
>> +     *
>> +     * debugfs directory for each client under a drm node.
>> +     */
>> +    struct dentry *debugfs_client;
>
> Is it worth idefing this out if !CONFIG_DEBUG_FS?

Surprisingly i dont see CONFIG_DEBUG_FS used in drm much.  So keeping it 
same for this one variable too. Need a whole new change to keep debugfs 
related things under the if.

Regards
Sunil Khatri

>
> Regards,
>
> Tvrtko
>
>>   };
>>     /**
>

Re: [PATCH v4 2/4] drm: add debugfs support on per client-id basis

[Tvrtko Ursulin]

On 23/06/2025 11:24, Khatri, Sunil wrote:
> 
> On 6/23/2025 2:58 PM, Tvrtko Ursulin wrote:
>>
>>
>> On 18/06/2025 14:47, Sunil Khatri wrote:
>>> add support to add a directory for each client-id
>>> with root at the dri level. Since the clients are
>>> unique and not just related to one single drm device,
>>> so it makes more sense to add all the client based
>>> nodes with root as dri.
>>>
>>> Also create a symlink back to the parent drm device
>>> from each client.
>>
>> TBH I can see an use case for both clients at DRI level and clients 
>> under DRM devices. I guess you have an use case for global and per 
>> device can be added later if it becomes needed.
>>
>>>
>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_debugfs.c | 32 ++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/drm_file.c    | 10 ++++++++++
>>>   include/drm/drm_debugfs.h     | 12 ++++++++++++
>>>   include/drm/drm_file.h        |  7 +++++++
>>>   4 files changed, 61 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/ 
>>> drm_debugfs.c
>>> index 5a33ec299c04..875276d5fb9f 100644
>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>> @@ -298,6 +298,38 @@ void drm_debugfs_remove_dir(void)
>>>       debugfs_remove(drm_debugfs_root);
>>>   }
>>>   +int drm_debugfs_clients_add(struct drm_file *file)
>>> +{
>>> +    struct drm_device *dev;
>>> +    char *client_dir, *symlink;
>>> +
>>> +    dev = file->minor->dev;
>>
>> FWIW, as dev is only used once and string locals are not overlapping, 
>> you could reduce to a single local variable like char *name and re-use 
>> it. Up to you.
>>
> Let me see what i could do with that. But yes can reduce locals.

Ok.

> regards
> 
> Sunil

Usually when you sign people stop reading. In this case I accidentaly 
spotted there is more below.

> 
>>> +
>>> +    client_dir = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
>>> +    if (!client_dir)
>>> +        return -ENOMEM;
>>
>> It is a bit more work, but I think a clients/ directory with numerical 
>> client id subdirs would be nicer.
> 
> It was with the id only first but with feedback from Christian i moved 
> it with client-$. Also since we want it in main root directory along 
> with nodes like 0 and 128, it makes sense to differentiate and make a clear
> 
> representation of clients.

I don't mean id only in the root dir, but add a clients subdir in the 
root, where clients subdir contains more subdirs for individual clients. 
Maybe it is personal but for me $dri_root/clients/1/something feels 
nicer, less cluttered and potentially easier to handle in scripts and/or 
code that $dri_root/client-1/something.

> 
>>
>>> +
>>> +    /* Create a debugfs directory for the client in root on drm 
>>> debugfs */
>>> +    file->debugfs_client = debugfs_create_dir(client_dir, 
>>> drm_debugfs_root);
>>> +    kfree(client_dir);
>>> +
>>> +    symlink = kasprintf(GFP_KERNEL, "../%s", dev->unique);
>>> +    if (!symlink)
>>> +        return -ENOMEM;
>>
>> Worth removing the partial construction?
> Ideally it should never fail and but yes makes sense to clean up.
>>
>>> +
>>> +    /* Create a link from client_id to the drm device this client id 
>>> belongs to */
>>> +    debugfs_create_symlink("device", file->debugfs_client, symlink);
>>
>> This can also fail.
> sure. Noted
>>
>>> +    kfree(symlink);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +void drm_debugfs_clients_remove(struct drm_file *file)
>>> +{
>>> +    debugfs_remove_recursive(file->debugfs_client);
>>> +    file->debugfs_client = NULL;
>>> +}
>>> +
>>>   /**
>>>    * drm_debugfs_dev_init - create debugfs directory for the device
>>>    * @dev: the device which we want to create the directory for
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index 06ba6dcbf5ae..8502c5a630b1 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -39,12 +39,14 @@
>>>   #include <linux/poll.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/vga_switcheroo.h>
>>> +#include <linux/debugfs.h>
>>>     #include <drm/drm_client_event.h>
>>>   #include <drm/drm_drv.h>
>>>   #include <drm/drm_file.h>
>>>   #include <drm/drm_gem.h>
>>>   #include <drm/drm_print.h>
>>> +#include <drm/drm_debugfs.h>
>>>     #include "drm_crtc_internal.h"
>>>   #include "drm_internal.h"
>>> @@ -143,6 +145,13 @@ struct drm_file *drm_file_alloc(struct drm_minor 
>>> *minor)
>>>       rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>>>       file->minor = minor;
>>>   +    ret = drm_debugfs_clients_add(file);
>>
>> Slightly tricky part is that as soon as this runs userspace can enter 
>> debugfs. If in the future any debufs clients file is added which can 
>> dereference any of the drm_file fields not yet initialized it has the 
>> potential to explode and/or be exploited.
>>
>> Hence I think to be safe the usual pattern of exposing drm_file to 
>> userspace at the end, only _after_ drm_file has been *fully* initialized.
>>
>> Slightly annoying part with that might be undoing dev->driver->open() 
>> but maybe it is not that bad.
> 
> I need this before driver open as the entry is accessed in driver->open 
> in amdgpu to add files to the directory.
> 
> So, i could see to move it just before the open but not after. Anyways 
> if we reach till driver open surely file is fully initialized. Nothing 
> else is done in that function after that.

I guess it is fine as long as dev->driver->open() will be the only place 
which will be adding files. If one day DRM core decides to add some 
common file it will need to make things it can dereference are fully 
initialized.

Perhaps what makes sense today to make this more robust, and it is not 
hard, is to simply move drm_debugfs_clients_add to just before 
dev->driver->open()?

> 
>>
>>> +    if (ret) {
>>> +        put_pid(rcu_access_pointer(file->pid));
>>> +        kfree(file);
>>> +        return ERR_PTR(ret);
>>
>> Onion unwind already exists in the function so could have used it. 
>> (Add a new label and here simply "goto out_put_pid".) But as above we 
>> discuss tweaking the order lets see how that goes first.
> Sure.
>>
>>> +    }
>>> +
>>>       /* for compatibility root is always authenticated */
>>>       file->authenticated = capable(CAP_SYS_ADMIN);
>>>   @@ -236,6 +245,7 @@ void drm_file_free(struct drm_file *file)
>>>                atomic_read(&dev->open_count));
>>>         drm_events_release(file);
>>> +    drm_debugfs_clients_remove(file);
>>>         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>>           drm_fb_release(file);
>>> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
>>> index cf06cee4343f..4bd6cc1d0900 100644
>>> --- a/include/drm/drm_debugfs.h
>>> +++ b/include/drm/drm_debugfs.h
>>> @@ -153,6 +153,9 @@ void drm_debugfs_add_files(struct drm_device *dev,
>>>     int drm_debugfs_gpuva_info(struct seq_file *m,
>>>                  struct drm_gpuvm *gpuvm);
>>> +
>>> +int drm_debugfs_clients_add(struct drm_file *file);
>>> +void drm_debugfs_clients_remove(struct drm_file *file);
>>>   #else
>>>   static inline void drm_debugfs_create_files(const struct 
>>> drm_info_list *files,
>>>                           int count, struct dentry *root,
>>> @@ -181,6 +184,15 @@ static inline int drm_debugfs_gpuva_info(struct 
>>> seq_file *m,
>>>   {
>>>       return 0;
>>>   }
>>> +
>>> +int drm_debugfs_clients_add(struct drm_file *file)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +void drm_debugfs_clients_remove(struct drm_file *file)
>>> +{
>>> +}
>>
>> Static inline for the two above.
> Noted
>>
>>>   #endif
>>>     #endif /* _DRM_DEBUGFS_H_ */
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index 5c3b2aa3e69d..eab7546aad79 100644
>>> --- a/include/drm/drm_file.h
>>> +++ b/include/drm/drm_file.h
>>> @@ -400,6 +400,13 @@ struct drm_file {
>>>        * @client_name_lock: Protects @client_name.
>>>        */
>>>       struct mutex client_name_lock;
>>> +
>>> +    /**
>>> +     * @debugfs_client:
>>> +     *
>>> +     * debugfs directory for each client under a drm node.
>>> +     */
>>> +    struct dentry *debugfs_client;
>>
>> Is it worth idefing this out if !CONFIG_DEBUG_FS?
> 
> Surprisingly i dont see CONFIG_DEBUG_FS used in drm much.  So keeping it 
> same for this one variable too. Need a whole new change to keep debugfs 
> related things under the if.

Ah struct drm_device.. I see what you mean. I guess the waste if 
progressively worse as the unused fields move from structs with fewer 
instances to ones which can be a lot more.

Regards,

Tvrtko

> 
> Regards
> Sunil Khatri
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>>   };
>>>     /**
>>

Re: [PATCH v4 2/4] drm: add debugfs support on per client-id basis

[Christian König]

On 23.06.25 15:07, Tvrtko Ursulin wrote:
> 
> On 23/06/2025 11:24, Khatri, Sunil wrote:
>>
>> On 6/23/2025 2:58 PM, Tvrtko Ursulin wrote:
>>>
>>>
>>> On 18/06/2025 14:47, Sunil Khatri wrote:
>>>> add support to add a directory for each client-id
>>>> with root at the dri level. Since the clients are
>>>> unique and not just related to one single drm device,
>>>> so it makes more sense to add all the client based
>>>> nodes with root as dri.
>>>>
>>>> Also create a symlink back to the parent drm device
>>>> from each client.
>>>
>>> TBH I can see an use case for both clients at DRI level and clients under DRM devices. I guess you have an use case for global and per device can be added later if it becomes needed.

We already have a "clients" file in the driver directory giving you a list of clients who use this driver instance.

>>>
>>>>
>>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_debugfs.c | 32 ++++++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/drm_file.c    | 10 ++++++++++
>>>>   include/drm/drm_debugfs.h     | 12 ++++++++++++
>>>>   include/drm/drm_file.h        |  7 +++++++
>>>>   4 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/ drm_debugfs.c
>>>> index 5a33ec299c04..875276d5fb9f 100644
>>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>>> @@ -298,6 +298,38 @@ void drm_debugfs_remove_dir(void)
>>>>       debugfs_remove(drm_debugfs_root);
>>>>   }
>>>>   +int drm_debugfs_clients_add(struct drm_file *file)
>>>> +{
>>>> +    struct drm_device *dev;
>>>> +    char *client_dir, *symlink;
>>>> +
>>>> +    dev = file->minor->dev;
>>>
>>> FWIW, as dev is only used once and string locals are not overlapping, you could reduce to a single local variable like char *name and re-use it. Up to you.
>>>
>> Let me see what i could do with that. But yes can reduce locals.
> 
> Ok.
> 
>> regards
>>
>> Sunil
> 
> Usually when you sign people stop reading. In this case I accidentaly spotted there is more below.
> 
>>
>>>> +
>>>> +    client_dir = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
>>>> +    if (!client_dir)
>>>> +        return -ENOMEM;
>>>
>>> It is a bit more work, but I think a clients/ directory with numerical client id subdirs would be nicer.
>>
>> It was with the id only first but with feedback from Christian i moved it with client-$. Also since we want it in main root directory along with nodes like 0 and 128, it makes sense to differentiate and make a clear
>>
>> representation of clients.
> 
> I don't mean id only in the root dir, but add a clients subdir in the root, where clients subdir contains more subdirs for individual clients. Maybe it is personal but for me $dri_root/clients/1/something feels nicer, less cluttered and potentially easier to handle in scripts and/or code that $dri_root/client-1/something.

I've played around with that idea as well, but then abandoned it as only an extra step. But it might indeed be nicer.

>>
>>>
>>>> +
>>>> +    /* Create a debugfs directory for the client in root on drm debugfs */
>>>> +    file->debugfs_client = debugfs_create_dir(client_dir, drm_debugfs_root);
>>>> +    kfree(client_dir);
>>>> +
>>>> +    symlink = kasprintf(GFP_KERNEL, "../%s", dev->unique);
>>>> +    if (!symlink)
>>>> +        return -ENOMEM;
>>>
>>> Worth removing the partial construction?
>> Ideally it should never fail and but yes makes sense to clean up.
>>>
>>>> +
>>>> +    /* Create a link from client_id to the drm device this client id belongs to */
>>>> +    debugfs_create_symlink("device", file->debugfs_client, symlink);
>>>
>>> This can also fail.
>> sure. Noted

Keep in mind that the results of debugfs functions should *not* be checked and failures should *not* be fatal.

Otherwise Greg comes out and beats your code into shape :)

>>>
>>>> +    kfree(symlink);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +void drm_debugfs_clients_remove(struct drm_file *file)
>>>> +{
>>>> +    debugfs_remove_recursive(file->debugfs_client);
>>>> +    file->debugfs_client = NULL;
>>>> +}
>>>> +
>>>>   /**
>>>>    * drm_debugfs_dev_init - create debugfs directory for the device
>>>>    * @dev: the device which we want to create the directory for
>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>> index 06ba6dcbf5ae..8502c5a630b1 100644
>>>> --- a/drivers/gpu/drm/drm_file.c
>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>> @@ -39,12 +39,14 @@
>>>>   #include <linux/poll.h>
>>>>   #include <linux/slab.h>
>>>>   #include <linux/vga_switcheroo.h>
>>>> +#include <linux/debugfs.h>
>>>>     #include <drm/drm_client_event.h>
>>>>   #include <drm/drm_drv.h>
>>>>   #include <drm/drm_file.h>
>>>>   #include <drm/drm_gem.h>
>>>>   #include <drm/drm_print.h>
>>>> +#include <drm/drm_debugfs.h>
>>>>     #include "drm_crtc_internal.h"
>>>>   #include "drm_internal.h"
>>>> @@ -143,6 +145,13 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>>>       rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>>>>       file->minor = minor;
>>>>   +    ret = drm_debugfs_clients_add(file);

No error handling for debugfs functions please!

>>>
>>> Slightly tricky part is that as soon as this runs userspace can enter debugfs. If in the future any debufs clients file is added which can dereference any of the drm_file fields not yet initialized it has the potential to explode and/or be exploited.
>>>
>>> Hence I think to be safe the usual pattern of exposing drm_file to userspace at the end, only _after_ drm_file has been *fully* initialized.
>>>
>>> Slightly annoying part with that might be undoing dev->driver->open() but maybe it is not that bad.
>>
>> I need this before driver open as the entry is accessed in driver->open in amdgpu to add files to the directory.
>>
>> So, i could see to move it just before the open but not after. Anyways if we reach till driver open surely file is fully initialized. Nothing else is done in that function after that.
> 
> I guess it is fine as long as dev->driver->open() will be the only place which will be adding files. If one day DRM core decides to add some common file it will need to make things it can dereference are fully initialized.
> 
> Perhaps what makes sense today to make this more robust, and it is not hard, is to simply move drm_debugfs_clients_add to just before dev->driver->open()?

Well the common files should always work and never crash on read, don't they? So it should be save to have them created as soon as the drm_file exists.

The question is rather how do we handle teardown? Removing the directory as soon as possible?

Regards,
Christian.

> 
>>
>>>
>>>> +    if (ret) {
>>>> +        put_pid(rcu_access_pointer(file->pid));
>>>> +        kfree(file);
>>>> +        return ERR_PTR(ret);
>>>
>>> Onion unwind already exists in the function so could have used it. (Add a new label and here simply "goto out_put_pid".) But as above we discuss tweaking the order lets see how that goes first.
>> Sure.
>>>
>>>> +    }
>>>> +
>>>>       /* for compatibility root is always authenticated */
>>>>       file->authenticated = capable(CAP_SYS_ADMIN);
>>>>   @@ -236,6 +245,7 @@ void drm_file_free(struct drm_file *file)
>>>>                atomic_read(&dev->open_count));
>>>>         drm_events_release(file);
>>>> +    drm_debugfs_clients_remove(file);
>>>>         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>>>           drm_fb_release(file);
>>>> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
>>>> index cf06cee4343f..4bd6cc1d0900 100644
>>>> --- a/include/drm/drm_debugfs.h
>>>> +++ b/include/drm/drm_debugfs.h
>>>> @@ -153,6 +153,9 @@ void drm_debugfs_add_files(struct drm_device *dev,
>>>>     int drm_debugfs_gpuva_info(struct seq_file *m,
>>>>                  struct drm_gpuvm *gpuvm);
>>>> +
>>>> +int drm_debugfs_clients_add(struct drm_file *file);
>>>> +void drm_debugfs_clients_remove(struct drm_file *file);
>>>>   #else
>>>>   static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>>>>                           int count, struct dentry *root,
>>>> @@ -181,6 +184,15 @@ static inline int drm_debugfs_gpuva_info(struct seq_file *m,
>>>>   {
>>>>       return 0;
>>>>   }
>>>> +
>>>> +int drm_debugfs_clients_add(struct drm_file *file)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +void drm_debugfs_clients_remove(struct drm_file *file)
>>>> +{
>>>> +}
>>>
>>> Static inline for the two above.
>> Noted
>>>
>>>>   #endif
>>>>     #endif /* _DRM_DEBUGFS_H_ */
>>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>>> index 5c3b2aa3e69d..eab7546aad79 100644
>>>> --- a/include/drm/drm_file.h
>>>> +++ b/include/drm/drm_file.h
>>>> @@ -400,6 +400,13 @@ struct drm_file {
>>>>        * @client_name_lock: Protects @client_name.
>>>>        */
>>>>       struct mutex client_name_lock;
>>>> +
>>>> +    /**
>>>> +     * @debugfs_client:
>>>> +     *
>>>> +     * debugfs directory for each client under a drm node.
>>>> +     */
>>>> +    struct dentry *debugfs_client;
>>>
>>> Is it worth idefing this out if !CONFIG_DEBUG_FS?
>>
>> Surprisingly i dont see CONFIG_DEBUG_FS used in drm much.  So keeping it same for this one variable too. Need a whole new change to keep debugfs related things under the if.
> 
> Ah struct drm_device.. I see what you mean. I guess the waste if progressively worse as the unused fields move from structs with fewer instances to ones which can be a lot more.
> 
> Regards,
> 
> Tvrtko
> 
>>
>> Regards
>> Sunil Khatri
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>   };
>>>>     /**
>>>
> 

Re: [PATCH v4 2/4] drm: add debugfs support on per client-id basis

[Tvrtko Ursulin]

On 23/06/2025 15:55, Christian König wrote:
> On 23.06.25 15:07, Tvrtko Ursulin wrote:
>>
>> On 23/06/2025 11:24, Khatri, Sunil wrote:
>>>
>>> On 6/23/2025 2:58 PM, Tvrtko Ursulin wrote:
>>>>
>>>>
>>>> On 18/06/2025 14:47, Sunil Khatri wrote:
>>>>> add support to add a directory for each client-id
>>>>> with root at the dri level. Since the clients are
>>>>> unique and not just related to one single drm device,
>>>>> so it makes more sense to add all the client based
>>>>> nodes with root as dri.
>>>>>
>>>>> Also create a symlink back to the parent drm device
>>>>> from each client.
>>>>
>>>> TBH I can see an use case for both clients at DRI level and clients under DRM devices. I guess you have an use case for global and per device can be added later if it becomes needed.
> 
> We already have a "clients" file in the driver directory giving you a list of clients who use this driver instance.

Okay, that won't work then.

>>>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/drm_debugfs.c | 32 ++++++++++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/drm_file.c    | 10 ++++++++++
>>>>>    include/drm/drm_debugfs.h     | 12 ++++++++++++
>>>>>    include/drm/drm_file.h        |  7 +++++++
>>>>>    4 files changed, 61 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/ drm_debugfs.c
>>>>> index 5a33ec299c04..875276d5fb9f 100644
>>>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>>>> @@ -298,6 +298,38 @@ void drm_debugfs_remove_dir(void)
>>>>>        debugfs_remove(drm_debugfs_root);
>>>>>    }
>>>>>    +int drm_debugfs_clients_add(struct drm_file *file)
>>>>> +{
>>>>> +    struct drm_device *dev;
>>>>> +    char *client_dir, *symlink;
>>>>> +
>>>>> +    dev = file->minor->dev;
>>>>
>>>> FWIW, as dev is only used once and string locals are not overlapping, you could reduce to a single local variable like char *name and re-use it. Up to you.
>>>>
>>> Let me see what i could do with that. But yes can reduce locals.
>>
>> Ok.
>>
>>> regards
>>>
>>> Sunil
>>
>> Usually when you sign people stop reading. In this case I accidentaly spotted there is more below.
>>
>>>
>>>>> +
>>>>> +    client_dir = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
>>>>> +    if (!client_dir)
>>>>> +        return -ENOMEM;
>>>>
>>>> It is a bit more work, but I think a clients/ directory with numerical client id subdirs would be nicer.
>>>
>>> It was with the id only first but with feedback from Christian i moved it with client-$. Also since we want it in main root directory along with nodes like 0 and 128, it makes sense to differentiate and make a clear
>>>
>>> representation of clients.
>>
>> I don't mean id only in the root dir, but add a clients subdir in the root, where clients subdir contains more subdirs for individual clients. Maybe it is personal but for me $dri_root/clients/1/something feels nicer, less cluttered and potentially easier to handle in scripts and/or code that $dri_root/client-1/something.
> 
> I've played around with that idea as well, but then abandoned it as only an extra step. But it might indeed be nicer.
> 
>>>
>>>>
>>>>> +
>>>>> +    /* Create a debugfs directory for the client in root on drm debugfs */
>>>>> +    file->debugfs_client = debugfs_create_dir(client_dir, drm_debugfs_root);
>>>>> +    kfree(client_dir);
>>>>> +
>>>>> +    symlink = kasprintf(GFP_KERNEL, "../%s", dev->unique);
>>>>> +    if (!symlink)
>>>>> +        return -ENOMEM;
>>>>
>>>> Worth removing the partial construction?
>>> Ideally it should never fail and but yes makes sense to clean up.
>>>>
>>>>> +
>>>>> +    /* Create a link from client_id to the drm device this client id belongs to */
>>>>> +    debugfs_create_symlink("device", file->debugfs_client, symlink);
>>>>
>>>> This can also fail.
>>> sure. Noted
> 
> Keep in mind that the results of debugfs functions should *not* be checked and failures should *not* be fatal.
> 
> Otherwise Greg comes out and beats your code into shape :)

Oops, my bad!

>>>>> +    kfree(symlink);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +void drm_debugfs_clients_remove(struct drm_file *file)
>>>>> +{
>>>>> +    debugfs_remove_recursive(file->debugfs_client);
>>>>> +    file->debugfs_client = NULL;
>>>>> +}
>>>>> +
>>>>>    /**
>>>>>     * drm_debugfs_dev_init - create debugfs directory for the device
>>>>>     * @dev: the device which we want to create the directory for
>>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>>> index 06ba6dcbf5ae..8502c5a630b1 100644
>>>>> --- a/drivers/gpu/drm/drm_file.c
>>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>>> @@ -39,12 +39,14 @@
>>>>>    #include <linux/poll.h>
>>>>>    #include <linux/slab.h>
>>>>>    #include <linux/vga_switcheroo.h>
>>>>> +#include <linux/debugfs.h>
>>>>>      #include <drm/drm_client_event.h>
>>>>>    #include <drm/drm_drv.h>
>>>>>    #include <drm/drm_file.h>
>>>>>    #include <drm/drm_gem.h>
>>>>>    #include <drm/drm_print.h>
>>>>> +#include <drm/drm_debugfs.h>
>>>>>      #include "drm_crtc_internal.h"
>>>>>    #include "drm_internal.h"
>>>>> @@ -143,6 +145,13 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>>>>        rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>>>>>        file->minor = minor;
>>>>>    +    ret = drm_debugfs_clients_add(file);
> 
> No error handling for debugfs functions please!

Hmm but then any userspace will have a hard time figuring out what 
clients exist.

> 
>>>>
>>>> Slightly tricky part is that as soon as this runs userspace can enter debugfs. If in the future any debufs clients file is added which can dereference any of the drm_file fields not yet initialized it has the potential to explode and/or be exploited.
>>>>
>>>> Hence I think to be safe the usual pattern of exposing drm_file to userspace at the end, only _after_ drm_file has been *fully* initialized.
>>>>
>>>> Slightly annoying part with that might be undoing dev->driver->open() but maybe it is not that bad.
>>>
>>> I need this before driver open as the entry is accessed in driver->open in amdgpu to add files to the directory.
>>>
>>> So, i could see to move it just before the open but not after. Anyways if we reach till driver open surely file is fully initialized. Nothing else is done in that function after that.
>>
>> I guess it is fine as long as dev->driver->open() will be the only place which will be adding files. If one day DRM core decides to add some common file it will need to make things it can dereference are fully initialized.
>>
>> Perhaps what makes sense today to make this more robust, and it is not hard, is to simply move drm_debugfs_clients_add to just before dev->driver->open()?
> 
> Well the common files should always work and never crash on read, don't they? So it should be save to have them created as soon as the drm_file exists.

Yes they should not. :)

My concern is one day someone adds a common DRM file in here which maybe 
accesses file->client_name_lock. So the debugfs register must not be 
before the mutex_init.

Second option is a common file which would call some DRM helper, which 
in turn could dereference into driver ops, where those driver ops assume 
dev->driver->open() has already set up the relevant state.

For now it is theoretical but if any of the two happens it would be a 
timing sensitive null ptr deref, or worse.

Whether or not it is worth protecting against that at this stage I am 
not sure. One way would be two stage setup. First set up the client dir, 
then after dev->driver->open call a new hook into the driver to set up 
the files under it.

> The question is rather how do we handle teardown? Removing the directory as soon as possible?

You think the placent in the patch is too late?

Regards,

Tvrtko

>>>>
>>>>> +    if (ret) {
>>>>> +        put_pid(rcu_access_pointer(file->pid));
>>>>> +        kfree(file);
>>>>> +        return ERR_PTR(ret);
>>>>
>>>> Onion unwind already exists in the function so could have used it. (Add a new label and here simply "goto out_put_pid".) But as above we discuss tweaking the order lets see how that goes first.
>>> Sure.
>>>>
>>>>> +    }
>>>>> +
>>>>>        /* for compatibility root is always authenticated */
>>>>>        file->authenticated = capable(CAP_SYS_ADMIN);
>>>>>    @@ -236,6 +245,7 @@ void drm_file_free(struct drm_file *file)
>>>>>                 atomic_read(&dev->open_count));
>>>>>          drm_events_release(file);
>>>>> +    drm_debugfs_clients_remove(file);
>>>>>          if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>>>>            drm_fb_release(file);
>>>>> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
>>>>> index cf06cee4343f..4bd6cc1d0900 100644
>>>>> --- a/include/drm/drm_debugfs.h
>>>>> +++ b/include/drm/drm_debugfs.h
>>>>> @@ -153,6 +153,9 @@ void drm_debugfs_add_files(struct drm_device *dev,
>>>>>      int drm_debugfs_gpuva_info(struct seq_file *m,
>>>>>                   struct drm_gpuvm *gpuvm);
>>>>> +
>>>>> +int drm_debugfs_clients_add(struct drm_file *file);
>>>>> +void drm_debugfs_clients_remove(struct drm_file *file);
>>>>>    #else
>>>>>    static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>>>>>                            int count, struct dentry *root,
>>>>> @@ -181,6 +184,15 @@ static inline int drm_debugfs_gpuva_info(struct seq_file *m,
>>>>>    {
>>>>>        return 0;
>>>>>    }
>>>>> +
>>>>> +int drm_debugfs_clients_add(struct drm_file *file)
>>>>> +{
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +void drm_debugfs_clients_remove(struct drm_file *file)
>>>>> +{
>>>>> +}
>>>>
>>>> Static inline for the two above.
>>> Noted
>>>>
>>>>>    #endif
>>>>>      #endif /* _DRM_DEBUGFS_H_ */
>>>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>>>> index 5c3b2aa3e69d..eab7546aad79 100644
>>>>> --- a/include/drm/drm_file.h
>>>>> +++ b/include/drm/drm_file.h
>>>>> @@ -400,6 +400,13 @@ struct drm_file {
>>>>>         * @client_name_lock: Protects @client_name.
>>>>>         */
>>>>>        struct mutex client_name_lock;
>>>>> +
>>>>> +    /**
>>>>> +     * @debugfs_client:
>>>>> +     *
>>>>> +     * debugfs directory for each client under a drm node.
>>>>> +     */
>>>>> +    struct dentry *debugfs_client;
>>>>
>>>> Is it worth idefing this out if !CONFIG_DEBUG_FS?
>>>
>>> Surprisingly i dont see CONFIG_DEBUG_FS used in drm much.  So keeping it same for this one variable too. Need a whole new change to keep debugfs related things under the if.
>>
>> Ah struct drm_device.. I see what you mean. I guess the waste if progressively worse as the unused fields move from structs with fewer instances to ones which can be a lot more.
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Regards
>>> Sunil Khatri
>>>
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>>    };
>>>>>      /**
>>>>
>>
>