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