[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 07:33:30 UTC 2025
On 6/19/2025 10:37 AM, Khatri, Sunil wrote:
>
> 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.
You mean the function name drm_debugfs_create_dir ??
>
> 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.
I checked this code again and we could not use
drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) as at that time as
there is no drm_device as its done too early and done before any
drm_device is created.
I have something in mind as i found that accel_core_init is called from
drm_drv and we can do something there itself. I will push another patch
set and lets see how you find that.
Regards
Sunil khatri
>>
>> 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