[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 at 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;
>>>   };
>>>     /**