[PATCH v2 0/2] Support for debugfs directory for each client-id in /dri directory

[PATCH v2 0/2] Support for debugfs directory for each client-id in /dri directory

[Sunil Khatri]

Add support of debugfs for client-id so information specific to each
client could be added by corresponding driver such as page table and
queues associted etc.

For amdgpu below is the information and data stucture added:

----------------------------------------------------------------------------------------------------------------------------------
root@amd-X570-AORUS-ELITE:/sys/kernel/debug/dri# cat 128/clients
             command  tgid dev master a   uid      magic                                                             name client-id
      systemd-logind  1049   0   y    y     0          0                                                          <unset>     5
            Xwayland  1706 128   n    n   120          0                                                          <unset>     8
     mutter-x11-fram  2019 128   n    n   120          0                                                          <unset>     9
            ibus-x11  2046 128   n    n   120          0                                                          <unset>    10
root@amd-X570-AORUS-ELITE:/sys/kernel/debug/dri# cat client-5/
0/            pt_base_info
root@amd-X570-AORUS-ELITE:/sys/kernel/debug/dri# cat client-5/pt_base_info
pid: 1049
comm: systemd-logind
pt_base: 0x81febe9000
root@amd-X570-AORUS-ELITE:/sys/kernel/debug/dri# cat client-8/pt_base_info
pid: 1706
comm: Xwayland
pt_base: 0x81febdc000
-------------------------------------------------------------------------------------------------------------------------------------

Sunil Khatri (2):
  drm/debugfs: add client-id to the debugfs entry
  drm: add debugfs support on per client-id basis

 drivers/gpu/drm/drm_debugfs.c | 10 ++++++----
 drivers/gpu/drm/drm_file.c    | 26 ++++++++++++++++++++++++++
 include/drm/drm_device.h      |  4 ++++
 include/drm/drm_file.h        |  7 +++++++
 4 files changed, 43 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] drm/debugfs: add client-id to the debugfs entry

[Sunil Khatri]

pid is not always the right choice for fd to track
the caller and hence adding drm client-id to the
print which is unique for a drm client and can be
used by driver in debugging

One of the use is to further enhance debugging for
amdgpu driver based on client-id.

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

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 6b2178864c7e..2d43bda82887 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -77,14 +77,15 @@ static int drm_clients_info(struct seq_file *m, void *data)
 	kuid_t uid;
 
 	seq_printf(m,
-		   "%20s %5s %3s master a %5s %10s %*s\n",
+		   "%20s %5s %3s master a %5s %10s %*s %5s\n",
 		   "command",
 		   "tgid",
 		   "dev",
 		   "uid",
 		   "magic",
 		   DRM_CLIENT_NAME_MAX_LEN,
-		   "name");
+		   "name",
+		   "client-id");
 
 	/* dev->filelist is sorted youngest first, but we want to present
 	 * oldest first (i.e. kernel, servers, clients), so walk backwardss.
@@ -100,7 +101,7 @@ static int drm_clients_info(struct seq_file *m, void *data)
 		pid = rcu_dereference(priv->pid);
 		task = pid_task(pid, PIDTYPE_TGID);
 		uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
-		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %*s\n",
+		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %*s %5llu\n",
 			   task ? task->comm : "<unknown>",
 			   pid_vnr(pid),
 			   priv->minor->index,
@@ -109,7 +110,7 @@ static int drm_clients_info(struct seq_file *m, void *data)
 			   from_kuid_munged(seq_user_ns(m), uid),
 			   priv->magic,
 			   DRM_CLIENT_NAME_MAX_LEN,
-			   priv->client_name ? priv->client_name : "<unset>");
+			   priv->client_name ? priv->client_name : "<unset>", priv->client_id);
 		rcu_read_unlock();
 		mutex_unlock(&priv->client_name_lock);
 	}
-- 
2.34.1

[PATCH v2 2/2] 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 |  1 +
 drivers/gpu/drm/drm_file.c    | 26 ++++++++++++++++++++++++++
 include/drm/drm_device.h      |  4 ++++
 include/drm/drm_file.h        |  7 +++++++
 4 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 2d43bda82887..b4956960ae76 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -296,6 +296,7 @@ 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);
+	dev->drm_debugfs_root = root;
 }
 
 /**
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 06ba6dcbf5ae..32012e39dcb4 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -39,6 +39,7 @@
 #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>
@@ -133,6 +134,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
 	struct drm_device *dev = minor->dev;
 	struct drm_file *file;
 	int ret;
+	char *dir_name, *drm_name, *symlink;
 
 	file = kzalloc(sizeof(*file), GFP_KERNEL);
 	if (!file)
@@ -143,6 +145,27 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
 	rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
 	file->minor = minor;
 
+	dir_name = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
+	if (!dir_name)
+		return ERR_PTR(-ENOMEM);
+
+	/* Create a debugfs directory for the client in root on drm debugfs */
+	file->debugfs_client = debugfs_create_dir(dir_name, dev->drm_debugfs_root);
+	kfree(dir_name);
+
+	drm_name = kasprintf(GFP_KERNEL, "%d", minor->index);
+	if (!drm_name)
+		return ERR_PTR(-ENOMEM);
+
+	symlink = kasprintf(GFP_KERNEL, "../%d", minor->index);
+	if (!symlink)
+		return ERR_PTR(-ENOMEM);
+
+	/* Create a link from client_id to the drm device this client id belongs to */
+	debugfs_create_symlink(drm_name, file->debugfs_client, symlink);
+	kfree(drm_name);
+	kfree(symlink);
+
 	/* for compatibility root is always authenticated */
 	file->authenticated = capable(CAP_SYS_ADMIN);
 
@@ -237,6 +260,9 @@ void drm_file_free(struct drm_file *file)
 
 	drm_events_release(file);
 
+	debugfs_remove_recursive(file->debugfs_client);
+	file->debugfs_client = NULL;
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		drm_fb_release(file);
 		drm_property_destroy_user_blobs(dev, file);
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 6ea54a578cda..ec20b777b3cc 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -325,6 +325,10 @@ struct drm_device {
 	 * Root directory for debugfs files.
 	 */
 	struct dentry *debugfs_root;
+	/**
+	 * @drm_debugfs_root;
+	 */
+	struct dentry *drm_debugfs_root;
 };
 
 #endif
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

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

[Christian König]

On 6/16/25 12:05, 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 |  1 +
>  drivers/gpu/drm/drm_file.c    | 26 ++++++++++++++++++++++++++
>  include/drm/drm_device.h      |  4 ++++
>  include/drm/drm_file.h        |  7 +++++++
>  4 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 2d43bda82887..b4956960ae76 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -296,6 +296,7 @@ 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);
> +	dev->drm_debugfs_root = root;

We should probably just move drm_debugfs_root into drm_debugfs.c instead of keeping that around per device.

>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 06ba6dcbf5ae..32012e39dcb4 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -39,6 +39,7 @@
>  #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>
> @@ -133,6 +134,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>  	struct drm_device *dev = minor->dev;
>  	struct drm_file *file;
>  	int ret;
> +	char *dir_name, *drm_name, *symlink;
>  
>  	file = kzalloc(sizeof(*file), GFP_KERNEL);
>  	if (!file)
> @@ -143,6 +145,27 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>  	rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>  	file->minor = minor;
>  

> +	dir_name = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
> +	if (!dir_name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Create a debugfs directory for the client in root on drm debugfs */
> +	file->debugfs_client = debugfs_create_dir(dir_name, dev->drm_debugfs_root);
> +	kfree(dir_name);
> +
> +	drm_name = kasprintf(GFP_KERNEL, "%d", minor->index);
> +	if (!drm_name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	symlink = kasprintf(GFP_KERNEL, "../%d", minor->index);

Better use dev->unique here, minor->index is also only a symlink.

> +	if (!symlink)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Create a link from client_id to the drm device this client id belongs to */
> +	debugfs_create_symlink(drm_name, file->debugfs_client, symlink);
> +	kfree(drm_name);
> +	kfree(symlink);
> +

Move all that debugfs handling into a function in drm_debugfs.c

>  	/* for compatibility root is always authenticated */
>  	file->authenticated = capable(CAP_SYS_ADMIN);
>  
> @@ -237,6 +260,9 @@ void drm_file_free(struct drm_file *file)
>  
>  	drm_events_release(file);
>  
> +	debugfs_remove_recursive(file->debugfs_client);
> +	file->debugfs_client = NULL;
> +

Same here, move to drm_debugfs.c

Apart from that looks solid to me.

Regards,
Christian.


>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		drm_fb_release(file);
>  		drm_property_destroy_user_blobs(dev, file);
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 6ea54a578cda..ec20b777b3cc 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -325,6 +325,10 @@ struct drm_device {
>  	 * Root directory for debugfs files.
>  	 */
>  	struct dentry *debugfs_root;
> +	/**
> +	 * @drm_debugfs_root;
> +	 */
> +	struct dentry *drm_debugfs_root;
>  };
>  
>  #endif
> 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 v2 2/2] drm: add debugfs support on per client-id basis

[Khatri, Sunil]

On 6/16/2025 5:41 PM, Christian König wrote:
> On 6/16/25 12:05, 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 |  1 +
>>   drivers/gpu/drm/drm_file.c    | 26 ++++++++++++++++++++++++++
>>   include/drm/drm_device.h      |  4 ++++
>>   include/drm/drm_file.h        |  7 +++++++
>>   4 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 2d43bda82887..b4956960ae76 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -296,6 +296,7 @@ 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);
>> +	dev->drm_debugfs_root = root;
> We should probably just move drm_debugfs_root into drm_debugfs.c instead of keeping that around per device.
root node above is needed in the drm_file.c function drm_alloc and there 
is nothing above drm_device where i can get the root information. that 
is the reason i mentioned it as drm_debugfs_root as root node of the drm 
subsystem itself.
~Sunil
>
>>   }
>>   
>>   /**
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 06ba6dcbf5ae..32012e39dcb4 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -39,6 +39,7 @@
>>   #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>
>> @@ -133,6 +134,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>   	struct drm_device *dev = minor->dev;
>>   	struct drm_file *file;
>>   	int ret;
>> +	char *dir_name, *drm_name, *symlink;
>>   
>>   	file = kzalloc(sizeof(*file), GFP_KERNEL);
>>   	if (!file)
>> @@ -143,6 +145,27 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>   	rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>>   	file->minor = minor;
>>   
>> +	dir_name = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
>> +	if (!dir_name)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	/* Create a debugfs directory for the client in root on drm debugfs */
>> +	file->debugfs_client = debugfs_create_dir(dir_name, dev->drm_debugfs_root);
>> +	kfree(dir_name);
>> +
>> +	drm_name = kasprintf(GFP_KERNEL, "%d", minor->index);
>> +	if (!drm_name)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	symlink = kasprintf(GFP_KERNEL, "../%d", minor->index);
> Better use dev->unique here, minor->index is also only a symlink.

Thats a good suggestion and doable. Will update in next version

~Sunil

>
>> +	if (!symlink)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	/* Create a link from client_id to the drm device this client id belongs to */
>> +	debugfs_create_symlink(drm_name, file->debugfs_client, symlink);
>> +	kfree(drm_name);
>> +	kfree(symlink);
>> +
> Move all that debugfs handling into a function in drm_debugfs.c
Sure, let me try that and push another patch.
>
>>   	/* for compatibility root is always authenticated */
>>   	file->authenticated = capable(CAP_SYS_ADMIN);
>>   
>> @@ -237,6 +260,9 @@ void drm_file_free(struct drm_file *file)
>>   
>>   	drm_events_release(file);
>>   
>> +	debugfs_remove_recursive(file->debugfs_client);
>> +	file->debugfs_client = NULL;
>> +
> Same here, move to drm_debugfs.c

Sure, let me try that if there are not challenges.

~sunil

>
> Apart from that looks solid to me.
>
> Regards,
> Christian.
>
>
>>   	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>   		drm_fb_release(file);
>>   		drm_property_destroy_user_blobs(dev, file);
>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>> index 6ea54a578cda..ec20b777b3cc 100644
>> --- a/include/drm/drm_device.h
>> +++ b/include/drm/drm_device.h
>> @@ -325,6 +325,10 @@ struct drm_device {
>>   	 * Root directory for debugfs files.
>>   	 */
>>   	struct dentry *debugfs_root;
>> +	/**
>> +	 * @drm_debugfs_root;
>> +	 */
>> +	struct dentry *drm_debugfs_root;
>>   };
>>   
>>   #endif
>> 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 v2 2/2] drm: add debugfs support on per client-id basis

[Christian König]

On 6/16/25 14:25, Khatri, Sunil wrote:
> 
> On 6/16/2025 5:41 PM, Christian König wrote:
>> On 6/16/25 12:05, 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 |  1 +
>>>   drivers/gpu/drm/drm_file.c    | 26 ++++++++++++++++++++++++++
>>>   include/drm/drm_device.h      |  4 ++++
>>>   include/drm/drm_file.h        |  7 +++++++
>>>   4 files changed, 38 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>>> index 2d43bda82887..b4956960ae76 100644
>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>> @@ -296,6 +296,7 @@ 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);
>>> +    dev->drm_debugfs_root = root;
>> We should probably just move drm_debugfs_root into drm_debugfs.c instead of keeping that around per device.
> root node above is needed in the drm_file.c function drm_alloc and there is nothing above drm_device where i can get the root information. that is the reason i mentioned it as drm_debugfs_root as root node of the drm subsystem itself.

drm_debugfs_root is currently a global variable in drm_drv.c, but if we move it to drm_debugfs.c all functions in that file could use it.

Including the new functions for creating the per-client debugfs directory.

Regards,
Christian.


> ~Sunil
>>
>>>   }
>>>     /**
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index 06ba6dcbf5ae..32012e39dcb4 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -39,6 +39,7 @@
>>>   #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>
>>> @@ -133,6 +134,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>>       struct drm_device *dev = minor->dev;
>>>       struct drm_file *file;
>>>       int ret;
>>> +    char *dir_name, *drm_name, *symlink;
>>>         file = kzalloc(sizeof(*file), GFP_KERNEL);
>>>       if (!file)
>>> @@ -143,6 +145,27 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>>       rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>>>       file->minor = minor;
>>>   +    dir_name = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
>>> +    if (!dir_name)
>>> +        return ERR_PTR(-ENOMEM);
>>> +
>>> +    /* Create a debugfs directory for the client in root on drm debugfs */
>>> +    file->debugfs_client = debugfs_create_dir(dir_name, dev->drm_debugfs_root);
>>> +    kfree(dir_name);
>>> +
>>> +    drm_name = kasprintf(GFP_KERNEL, "%d", minor->index);
>>> +    if (!drm_name)
>>> +        return ERR_PTR(-ENOMEM);
>>> +
>>> +    symlink = kasprintf(GFP_KERNEL, "../%d", minor->index);
>> Better use dev->unique here, minor->index is also only a symlink.
> 
> Thats a good suggestion and doable. Will update in next version
> 
> ~Sunil
> 
>>
>>> +    if (!symlink)
>>> +        return ERR_PTR(-ENOMEM);
>>> +
>>> +    /* Create a link from client_id to the drm device this client id belongs to */
>>> +    debugfs_create_symlink(drm_name, file->debugfs_client, symlink);
>>> +    kfree(drm_name);
>>> +    kfree(symlink);
>>> +
>> Move all that debugfs handling into a function in drm_debugfs.c
> Sure, let me try that and push another patch.
>>
>>>       /* for compatibility root is always authenticated */
>>>       file->authenticated = capable(CAP_SYS_ADMIN);
>>>   @@ -237,6 +260,9 @@ void drm_file_free(struct drm_file *file)
>>>         drm_events_release(file);
>>>   +    debugfs_remove_recursive(file->debugfs_client);
>>> +    file->debugfs_client = NULL;
>>> +
>> Same here, move to drm_debugfs.c
> 
> Sure, let me try that if there are not challenges.
> 
> ~sunil
> 
>>
>> Apart from that looks solid to me.
>>
>> Regards,
>> Christian.
>>
>>
>>>       if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>>           drm_fb_release(file);
>>>           drm_property_destroy_user_blobs(dev, file);
>>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>>> index 6ea54a578cda..ec20b777b3cc 100644
>>> --- a/include/drm/drm_device.h
>>> +++ b/include/drm/drm_device.h
>>> @@ -325,6 +325,10 @@ struct drm_device {
>>>        * Root directory for debugfs files.
>>>        */
>>>       struct dentry *debugfs_root;
>>> +    /**
>>> +     * @drm_debugfs_root;
>>> +     */
>>> +    struct dentry *drm_debugfs_root;
>>>   };
>>>     #endif
>>> 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 v2 2/2] drm: add debugfs support on per client-id basis

[Khatri, Sunil]

On 6/16/2025 6:26 PM, Christian König wrote:
> On 6/16/25 14:25, Khatri, Sunil wrote:
>> On 6/16/2025 5:41 PM, Christian König wrote:
>>> On 6/16/25 12:05, 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 |  1 +
>>>>    drivers/gpu/drm/drm_file.c    | 26 ++++++++++++++++++++++++++
>>>>    include/drm/drm_device.h      |  4 ++++
>>>>    include/drm/drm_file.h        |  7 +++++++
>>>>    4 files changed, 38 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>>>> index 2d43bda82887..b4956960ae76 100644
>>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>>> @@ -296,6 +296,7 @@ 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);
>>>> +    dev->drm_debugfs_root = root;
>>> We should probably just move drm_debugfs_root into drm_debugfs.c instead of keeping that around per device.
>> root node above is needed in the drm_file.c function drm_alloc and there is nothing above drm_device where i can get the root information. that is the reason i mentioned it as drm_debugfs_root as root node of the drm subsystem itself.
> drm_debugfs_root is currently a global variable in drm_drv.c, but if we move it to drm_debugfs.c all functions in that file could use it.

The global variable drm_debugfs_root is global variable in drm_drv.c and 
a lot of function there are dependent on it. So no matter where we move 
the variable we need to have a reference of dentry in drm_drv.c too and 
drm_device is the only thing that is being used in drm_drv.c.

eg:
drm_core_init is using it and there is no structure there to use to have 
a reference to this node.
drm_minor_register also uses the same root to create the device nodes 
and we cant move all the code from drm_drv.c so we are stuck again how 
to get the reference in drm_debugfs.c

I am trying to explore if its possible but if there is anything you 
could suggest appreciate that. What is in the current patch is we have a 
reference of root in drm_device itself like drm core is parent to every 
drm device, could use a different name like parent_debugfs or something 
like that.


Regards
Sunil Khatri
>
> Including the new functions for creating the per-client debugfs directory.
>
> Regards,
> Christian.
>
>
>> ~Sunil
>>>>    }
>>>>      /**
>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>> index 06ba6dcbf5ae..32012e39dcb4 100644
>>>> --- a/drivers/gpu/drm/drm_file.c
>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>> @@ -39,6 +39,7 @@
>>>>    #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>
>>>> @@ -133,6 +134,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>>>        struct drm_device *dev = minor->dev;
>>>>        struct drm_file *file;
>>>>        int ret;
>>>> +    char *dir_name, *drm_name, *symlink;
>>>>          file = kzalloc(sizeof(*file), GFP_KERNEL);
>>>>        if (!file)
>>>> @@ -143,6 +145,27 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>>>        rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>>>>        file->minor = minor;
>>>>    +    dir_name = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
>>>> +    if (!dir_name)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    /* Create a debugfs directory for the client in root on drm debugfs */
>>>> +    file->debugfs_client = debugfs_create_dir(dir_name, dev->drm_debugfs_root);
>>>> +    kfree(dir_name);
>>>> +
>>>> +    drm_name = kasprintf(GFP_KERNEL, "%d", minor->index);
>>>> +    if (!drm_name)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    symlink = kasprintf(GFP_KERNEL, "../%d", minor->index);
>>> Better use dev->unique here, minor->index is also only a symlink.
>> Thats a good suggestion and doable. Will update in next version
>>
>> ~Sunil
>>
>>>> +    if (!symlink)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    /* Create a link from client_id to the drm device this client id belongs to */
>>>> +    debugfs_create_symlink(drm_name, file->debugfs_client, symlink);
>>>> +    kfree(drm_name);
>>>> +    kfree(symlink);
>>>> +
>>> Move all that debugfs handling into a function in drm_debugfs.c
>> Sure, let me try that and push another patch.
>>>>        /* for compatibility root is always authenticated */
>>>>        file->authenticated = capable(CAP_SYS_ADMIN);
>>>>    @@ -237,6 +260,9 @@ void drm_file_free(struct drm_file *file)
>>>>          drm_events_release(file);
>>>>    +    debugfs_remove_recursive(file->debugfs_client);
>>>> +    file->debugfs_client = NULL;
>>>> +
>>> Same here, move to drm_debugfs.c
>> Sure, let me try that if there are not challenges.
>>
>> ~sunil
>>
>>> Apart from that looks solid to me.
>>>
>>> Regards,
>>> Christian.
>>>
>>>
>>>>        if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>>>            drm_fb_release(file);
>>>>            drm_property_destroy_user_blobs(dev, file);
>>>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>>>> index 6ea54a578cda..ec20b777b3cc 100644
>>>> --- a/include/drm/drm_device.h
>>>> +++ b/include/drm/drm_device.h
>>>> @@ -325,6 +325,10 @@ struct drm_device {
>>>>         * Root directory for debugfs files.
>>>>         */
>>>>        struct dentry *debugfs_root;
>>>> +    /**
>>>> +     * @drm_debugfs_root;
>>>> +     */
>>>> +    struct dentry *drm_debugfs_root;
>>>>    };
>>>>      #endif
>>>> 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 v2 2/2] drm: add debugfs support on per client-id basis

[Christian König]

On 6/16/25 16:32, Khatri, Sunil wrote:
> 
> On 6/16/2025 6:26 PM, Christian König wrote:
>> On 6/16/25 14:25, Khatri, Sunil wrote:
>>> On 6/16/2025 5:41 PM, Christian König wrote:
>>>> On 6/16/25 12:05, 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 |  1 +
>>>>>    drivers/gpu/drm/drm_file.c    | 26 ++++++++++++++++++++++++++
>>>>>    include/drm/drm_device.h      |  4 ++++
>>>>>    include/drm/drm_file.h        |  7 +++++++
>>>>>    4 files changed, 38 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>>>>> index 2d43bda82887..b4956960ae76 100644
>>>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>>>> @@ -296,6 +296,7 @@ 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);
>>>>> +    dev->drm_debugfs_root = root;
>>>> We should probably just move drm_debugfs_root into drm_debugfs.c instead of keeping that around per device.
>>> root node above is needed in the drm_file.c function drm_alloc and there is nothing above drm_device where i can get the root information. that is the reason i mentioned it as drm_debugfs_root as root node of the drm subsystem itself.
>> drm_debugfs_root is currently a global variable in drm_drv.c, but if we move it to drm_debugfs.c all functions in that file could use it.
> 
> The global variable drm_debugfs_root is global variable in drm_drv.c and a lot of function there are dependent on it. So no matter where we move the variable we need to have a reference of dentry in drm_drv.c too and drm_device is the only thing that is being used in drm_drv.c.
> 
> eg:
> drm_core_init is using it and there is no structure there to use to have a reference to this node.
> drm_minor_register also uses the same root to create the device nodes and we cant move all the code from drm_drv.c so we are stuck again how to get the reference in drm_debugfs.c
> 
> I am trying to explore if its possible but if there is anything you could suggest appreciate that. What is in the current patch is we have a reference of root in drm_device itself like drm core is parent to every drm device, could use a different name like parent_debugfs or something like that.

drm_debugfs_root is only used in a few places in drm_drv.c:
1. To create and destroy it.
	That should potentially be moved to drm_debugfs.c

2. As parameter to drm_debugfs_register() and drm_debugfs_dev_init()
	Those functions are already in drm_debugfs.c, so you can just drop the parameter.

3. As parameter for drm_bridge_debugfs_params().
	That function is in drm_bridge.c, but it should be easy to call it from drm_debugfs.c after creating drm_debugfs_root instead of drm_drv.c

So moving drm_debugfs_root to drm_debugfs.c should be trivial, you basically just need a create and destroy function.

Regards,
Christian.

> 
> 
> Regards
> Sunil Khatri
>>
>> Including the new functions for creating the per-client debugfs directory.
>>
>> Regards,
>> Christian.
>>
>>
>>> ~Sunil
>>>>>    }
>>>>>      /**
>>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>>> index 06ba6dcbf5ae..32012e39dcb4 100644
>>>>> --- a/drivers/gpu/drm/drm_file.c
>>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>>> @@ -39,6 +39,7 @@
>>>>>    #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>
>>>>> @@ -133,6 +134,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>>>>        struct drm_device *dev = minor->dev;
>>>>>        struct drm_file *file;
>>>>>        int ret;
>>>>> +    char *dir_name, *drm_name, *symlink;
>>>>>          file = kzalloc(sizeof(*file), GFP_KERNEL);
>>>>>        if (!file)
>>>>> @@ -143,6 +145,27 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>>>>        rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>>>>>        file->minor = minor;
>>>>>    +    dir_name = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
>>>>> +    if (!dir_name)
>>>>> +        return ERR_PTR(-ENOMEM);
>>>>> +
>>>>> +    /* Create a debugfs directory for the client in root on drm debugfs */
>>>>> +    file->debugfs_client = debugfs_create_dir(dir_name, dev->drm_debugfs_root);
>>>>> +    kfree(dir_name);
>>>>> +
>>>>> +    drm_name = kasprintf(GFP_KERNEL, "%d", minor->index);
>>>>> +    if (!drm_name)
>>>>> +        return ERR_PTR(-ENOMEM);
>>>>> +
>>>>> +    symlink = kasprintf(GFP_KERNEL, "../%d", minor->index);
>>>> Better use dev->unique here, minor->index is also only a symlink.
>>> Thats a good suggestion and doable. Will update in next version
>>>
>>> ~Sunil
>>>
>>>>> +    if (!symlink)
>>>>> +        return ERR_PTR(-ENOMEM);
>>>>> +
>>>>> +    /* Create a link from client_id to the drm device this client id belongs to */
>>>>> +    debugfs_create_symlink(drm_name, file->debugfs_client, symlink);
>>>>> +    kfree(drm_name);
>>>>> +    kfree(symlink);
>>>>> +
>>>> Move all that debugfs handling into a function in drm_debugfs.c
>>> Sure, let me try that and push another patch.
>>>>>        /* for compatibility root is always authenticated */
>>>>>        file->authenticated = capable(CAP_SYS_ADMIN);
>>>>>    @@ -237,6 +260,9 @@ void drm_file_free(struct drm_file *file)
>>>>>          drm_events_release(file);
>>>>>    +    debugfs_remove_recursive(file->debugfs_client);
>>>>> +    file->debugfs_client = NULL;
>>>>> +
>>>> Same here, move to drm_debugfs.c
>>> Sure, let me try that if there are not challenges.
>>>
>>> ~sunil
>>>
>>>> Apart from that looks solid to me.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>
>>>>>        if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>>>>>            drm_fb_release(file);
>>>>>            drm_property_destroy_user_blobs(dev, file);
>>>>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>>>>> index 6ea54a578cda..ec20b777b3cc 100644
>>>>> --- a/include/drm/drm_device.h
>>>>> +++ b/include/drm/drm_device.h
>>>>> @@ -325,6 +325,10 @@ struct drm_device {
>>>>>         * Root directory for debugfs files.
>>>>>         */
>>>>>        struct dentry *debugfs_root;
>>>>> +    /**
>>>>> +     * @drm_debugfs_root;
>>>>> +     */
>>>>> +    struct dentry *drm_debugfs_root;
>>>>>    };
>>>>>      #endif
>>>>> 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;
>>>>>    };
>>>>>      /**