Re: [PATCH v7 1/5] drm: move the debugfs accel driver code to drm layer

["Khatri, Sunil" <sukhatri@amd.com>]

On 6/30/2025 8:49 PM, Jeff Hugo wrote:
> On 6/30/2025 8:36 AM, Sunil Khatri wrote:
>
> I don't see a cover letter on list. Surely there should be one?
Yes there is one with the first version of patches.
>
> Looks like you didn't send this to the Accel maintainer. Did you 
> forget to run the get_maintainers/pl script?
Let me check and add more. Christian is already there who did make 
changes to code around that area where i made but surely let me add more.
>> Move the debugfs accel driver code to the drm layer
>> and it is an intermediate step to move all debugfs
>> related handling into drm_debugfs.c
>
> This is missing the answer to the most important question - why?
Updated in V8.
>
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/accel/drm_accel.c | 16 ----------------
>>   drivers/gpu/drm/drm_drv.c |  6 +++++-
>>   include/drm/drm_accel.h   |  5 -----
>>   3 files changed, 5 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
>> index aa826033b0ce..ca3357acd127 100644
>> --- a/drivers/accel/drm_accel.c
>> +++ b/drivers/accel/drm_accel.c
>> @@ -20,8 +20,6 @@
>>     DEFINE_XARRAY_ALLOC(accel_minors_xa);
>>   -static struct dentry *accel_debugfs_root;
>> -
>>   static const struct device_type accel_sysfs_device_minor = {
>>       .name = "accel_minor"
>>   };
>> @@ -73,17 +71,6 @@ static const struct drm_info_list 
>> accel_debugfs_list[] = {
>>   };
>>   #define ACCEL_DEBUGFS_ENTRIES ARRAY_SIZE(accel_debugfs_list)
>>   -/**
>> - * accel_debugfs_init() - Initialize debugfs for device
>> - * @dev: Pointer to the device instance.
>> - *
>> - * This function creates a root directory for the device in debugfs.
>> - */
>> -void accel_debugfs_init(struct drm_device *dev)
>> -{
>> -    drm_debugfs_dev_init(dev, accel_debugfs_root);
>> -}
>> -
>>   /**
>>    * accel_debugfs_register() - Register debugfs for device
>>    * @dev: Pointer to the device instance.
>> @@ -194,7 +181,6 @@ static const struct file_operations 
>> accel_stub_fops = {
>>   void accel_core_exit(void)
>>   {
>>       unregister_chrdev(ACCEL_MAJOR, "accel");
>> -    debugfs_remove(accel_debugfs_root);
>>       accel_sysfs_destroy();
>>       WARN_ON(!xa_empty(&accel_minors_xa));
>>   }
>> @@ -209,8 +195,6 @@ int __init accel_core_init(void)
>>           goto error;
>>       }
>>   -    accel_debugfs_root = debugfs_create_dir("accel", NULL);
>> -
>>       ret = register_chrdev(ACCEL_MAJOR, "accel", &accel_stub_fops);
>>       if (ret < 0)
>>           DRM_ERROR("Cannot register ACCEL major: %d\n", ret);
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 17fc5dc708f4..5d57b622f9aa 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -70,6 +70,7 @@ DEFINE_XARRAY_ALLOC(drm_minors_xa);
>>   static bool drm_core_init_complete;
>>     static struct dentry *drm_debugfs_root;
>> +static struct dentry *accel_debugfs_root;
>>     DEFINE_STATIC_SRCU(drm_unplug_srcu);
>>   @@ -752,7 +753,7 @@ static int drm_dev_init(struct drm_device *dev,
>>       }
>>         if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL))
>> -        accel_debugfs_init(dev);
>> +        drm_debugfs_dev_init(dev, accel_debugfs_root);
>>       else
>>           drm_debugfs_dev_init(dev, drm_debugfs_root);
>>   @@ -1166,6 +1167,7 @@ static void drm_core_exit(void)
>>   {
>>       drm_privacy_screen_lookup_exit();
>>       drm_panic_exit();
>> +    debugfs_remove(accel_debugfs_root);
>>       accel_core_exit();
>>       unregister_chrdev(DRM_MAJOR, "drm");
>>       debugfs_remove(drm_debugfs_root);
>> @@ -1193,6 +1195,8 @@ static int __init drm_core_init(void)
>>       if (ret < 0)
>>           goto error;
>>   +    accel_debugfs_root = debugfs_create_dir("accel", NULL);
>
> Did you test this with CONFIG_DRM_ACCEL=n?
> It looks like you change the behavior with this change in that we'll 
> have an accel debugfs directory created, even when ACCEL is not built 
> into DRM.

Thanks for catching that out. Added support based on the configuration now.

Regards
Sunil Khatri