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.kha...@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
>>>
>>>> };
>>>> /**
>>>
>
