[PATCH v4 1/4] drm: move debugfs functionality from drm_drv.c to drm_debugfs.c
Khatri, Sunil
sukhatri at amd.com
Thu Jun 19 05:07:23 UTC 2025
On 6/18/2025 7:38 PM, Christian König wrote:
> On 6/18/25 15:47, Sunil Khatri wrote:
>> move the functions from drm_drv.c which uses the static
>> drm_debugfs_root as parent node in the debugfs by drm.
>>
>> move this root node to the debugfs for easily handling
>> of future requirements to add more information in the
>> root directory and one of which is planned to have
>> directories for each client in the root directory
>> which is dri.
>>
>> Suggested-by: Christian König <christian.koenig at amd.com>
>> Signed-off-by: Sunil Khatri <sunil.khatri at amd.com>
>> ---
>> drivers/gpu/drm/drm_debugfs.c | 22 ++++++++++++++++++----
>> drivers/gpu/drm/drm_drv.c | 11 ++++-------
>> drivers/gpu/drm/drm_internal.h | 6 ++----
>> include/drm/drm_drv.h | 10 ++++++++++
>> 4 files changed, 34 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 2d43bda82887..5a33ec299c04 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -44,6 +44,8 @@
>> #include "drm_crtc_internal.h"
>> #include "drm_internal.h"
>>
>> +static struct dentry *drm_debugfs_root;
>> +
>> /***************************************************
>> * Initialization, etc.
>> **************************************************/
>> @@ -286,6 +288,16 @@ int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
>> }
>> EXPORT_SYMBOL(drm_debugfs_remove_files);
>>
>> +void drm_debugfs_create_dir(void)
> I think we need a better name for this. drm_debugfs_init_root maybe? Ideas welcome.
Sounds good to me.
Regards
Sunil Khatri
>
>> +{
>> + drm_debugfs_root = debugfs_create_dir("dri", NULL);
>> +}
>> +
>> +void drm_debugfs_remove_dir(void)
>> +{
>> + debugfs_remove(drm_debugfs_root);
>> +}
>> +
>> /**
>> * drm_debugfs_dev_init - create debugfs directory for the device
>> * @dev: the device which we want to create the directory for
>> @@ -295,7 +307,10 @@ 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);
>> + if (!root)
>> + dev->debugfs_root = debugfs_create_dir(dev->unique, drm_debugfs_root);
>> + else
>> + dev->debugfs_root = debugfs_create_dir(dev->unique, root);
> Ah! That is also used from the accel subsystem, isn't it?
Yes
>
> Probably best to move accel_debugfs_root into this here as well and distinct based on drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) where to create the debugfs directory.
>
> We probably need the same distinction for other use cases as well, so probably best to create a helper function for that.
I did explore that accel_debugfs_init in file drivers/accel/drm_accel.c
and i dont think of a clear and acceptable design. It is a different
module all together and we dont want to create debugfs directory for it
in drm code. If the module fail for some reason and also removing the
debugfs directory is another problem. We do not know when we want to
init/exit the module.
The approach that i used is that drm_debugfs_dev_init is used by other
module(right now there is one but we never know in future) as well as
drm itself. The parent node is passed by the caller and for drm itself
the parent node is defined statically. So with the approach i took is
if parent node is NULL its an internal call within drm and we can
directly use the the static dentry i.e drm_debugfs_root.
Can we move below call from accel_core_init to drm_debugfs.c although
its a different module all together for accel ??
accel_debugfs_root = debugfs_create_dir("accel", NULL);
>
> Apart from that looks really good to me.
>
> Regards,
> Christian.
>
>> }
>>
>> /**
>> @@ -322,14 +337,13 @@ void drm_debugfs_dev_register(struct drm_device *dev)
>> drm_atomic_debugfs_init(dev);
>> }
>>
>> -int drm_debugfs_register(struct drm_minor *minor, int minor_id,
>> - struct dentry *root)
>> +int drm_debugfs_register(struct drm_minor *minor, int minor_id)
>> {
>> struct drm_device *dev = minor->dev;
>> char name[64];
>>
>> sprintf(name, "%d", minor_id);
>> - minor->debugfs_symlink = debugfs_create_symlink(name, root,
>> + minor->debugfs_symlink = debugfs_create_symlink(name, drm_debugfs_root,
>> dev->unique);
>>
>> /* TODO: Only for compatibility with drivers */
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 17fc5dc708f4..8abc52eac8f3 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -69,8 +69,6 @@ DEFINE_XARRAY_ALLOC(drm_minors_xa);
>> */
>> static bool drm_core_init_complete;
>>
>> -static struct dentry *drm_debugfs_root;
>> -
>> DEFINE_STATIC_SRCU(drm_unplug_srcu);
>>
>> /*
>> @@ -183,8 +181,7 @@ static int drm_minor_register(struct drm_device *dev, enum drm_minor_type type)
>> return 0;
>>
>> if (minor->type != DRM_MINOR_ACCEL) {
>> - ret = drm_debugfs_register(minor, minor->index,
>> - drm_debugfs_root);
>> + ret = drm_debugfs_register(minor, minor->index);
>> if (ret) {
>> DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
>> goto err_debugfs;
>> @@ -754,7 +751,7 @@ static int drm_dev_init(struct drm_device *dev,
>> if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL))
>> accel_debugfs_init(dev);
>> else
>> - drm_debugfs_dev_init(dev, drm_debugfs_root);
>> + drm_debugfs_dev_init(dev, NULL);
>>
>> return 0;
>>
>> @@ -1168,7 +1165,7 @@ static void drm_core_exit(void)
>> drm_panic_exit();
>> accel_core_exit();
>> unregister_chrdev(DRM_MAJOR, "drm");
>> - debugfs_remove(drm_debugfs_root);
>> + drm_debugfs_remove_dir();
>> drm_sysfs_destroy();
>> WARN_ON(!xa_empty(&drm_minors_xa));
>> drm_connector_ida_destroy();
>> @@ -1187,7 +1184,7 @@ static int __init drm_core_init(void)
>> goto error;
>> }
>>
>> - drm_debugfs_root = debugfs_create_dir("dri", NULL);
>> + drm_debugfs_create_dir();
>>
>> ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
>> if (ret < 0)
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index b2b6a8e49dda..d2d8e72f32d9 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -186,8 +186,7 @@ void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
>> #if defined(CONFIG_DEBUG_FS)
>> void drm_debugfs_dev_fini(struct drm_device *dev);
>> void drm_debugfs_dev_register(struct drm_device *dev);
>> -int drm_debugfs_register(struct drm_minor *minor, int minor_id,
>> - struct dentry *root);
>> +int drm_debugfs_register(struct drm_minor *minor, int minor_id);
>> void drm_debugfs_unregister(struct drm_minor *minor);
>> void drm_debugfs_connector_add(struct drm_connector *connector);
>> void drm_debugfs_connector_remove(struct drm_connector *connector);
>> @@ -205,8 +204,7 @@ static inline void drm_debugfs_dev_register(struct drm_device *dev)
>> {
>> }
>>
>> -static inline int drm_debugfs_register(struct drm_minor *minor, int minor_id,
>> - struct dentry *root)
>> +static inline int drm_debugfs_register(struct drm_minor *minor, int minor_id)
>> {
>> return 0;
>> }
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index a43d707b5f36..4e77a0c4a7f9 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -567,10 +567,20 @@ static inline bool drm_firmware_drivers_only(void)
>>
>> #if defined(CONFIG_DEBUG_FS)
>> void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root);
>> +void drm_debugfs_create_dir(void);
>> +void drm_debugfs_remove_dir(void);
>> #else
>> static inline void drm_debugfs_dev_init(struct drm_device *dev, struct dentry *root)
>> {
>> }
>> +
>> +static inline void drm_debugfs_create_dir(void)
>> +{
>> +}
>> +
>> +static inline void drm_debugfs_remove_dir(void)
>> +{
>> +}
>> #endif
>>
>> #endif
More information about the amd-gfx
mailing list