[PATCH v2 2/2] drm: add debugfs support on per client-id basis
Christian König
christian.koenig at amd.com
Mon Jun 16 14:59:12 UTC 2025
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 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.
>
> 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;
>>>>> };
>>>>> /**
More information about the amd-gfx
mailing list