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.
Signed-off-by: Sunil Khatri <[email protected]>
---
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.
+
+ /* 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.
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()?
+ 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
};
/**
