[PATCH 1/1] drm/amdgpu: init debugfs drm driver callback

[PATCH 1/1] drm/amdgpu: init debugfs drm driver callback

[Nirmoy Das]

drm_dev_register() will try to init driver's debugfs using
drm_driver.debugfs_init call back function. Use that callback
also for amdgpu to intialize debugfs.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 +++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |  7 +++----
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 6611b3c7c149..3076742f8f85 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1611,8 +1611,9 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
 DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
 			amdgpu_debugfs_sclk_set, "%llu\n");
 
-int amdgpu_debugfs_init(struct amdgpu_device *adev)
+void amdgpu_debugfs_init(struct drm_minor *minor)
 {
+	struct amdgpu_device *adev = drm_to_adev(minor->dev);
 	struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
 	struct dentry *ent;
 	int r, i;
@@ -1621,14 +1622,14 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
 				  &fops_ib_preempt);
 	if (IS_ERR(ent)) {
 		DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n");
-		return PTR_ERR(ent);
+		return;
 	}
 
 	ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
 				  &fops_sclk_set);
 	if (IS_ERR(ent)) {
 		DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
-		return PTR_ERR(ent);
+		return;
 	}
 
 	/* Register debugfs entries for amdgpu_ttm */
@@ -1682,11 +1683,10 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
 	debugfs_create_blob("amdgpu_discovery", 0444, root,
 			    &adev->debugfs_discovery_blob);
 
-	return 0;
 }
 
 #else
-int amdgpu_debugfs_init(struct amdgpu_device *adev)
+void amdgpu_debugfs_init(struct drm_minor *minor)
 {
 	return 0;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
index 371a6f0deb29..06b68e16e35d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
@@ -27,7 +27,7 @@
  */
 
 int amdgpu_debugfs_regs_init(struct amdgpu_device *adev);
-int amdgpu_debugfs_init(struct amdgpu_device *adev);
+void amdgpu_debugfs_init(struct drm_minor *minor);
 void amdgpu_debugfs_fini(struct amdgpu_device *adev);
 void amdgpu_debugfs_fence_init(struct amdgpu_device *adev);
 void amdgpu_debugfs_firmware_init(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index df83b1f438b6..ceda650895db 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2012,10 +2012,6 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
 			drm_fbdev_generic_setup(adev_to_drm(adev), 32);
 	}
 
-	ret = amdgpu_debugfs_init(adev);
-	if (ret)
-		DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
-
 	return 0;
 
 err_pci:
@@ -2479,6 +2475,9 @@ static const struct drm_driver amdgpu_kms_driver = {
 	.dumb_map_offset = amdgpu_mode_dumb_mmap,
 	.fops = &amdgpu_driver_kms_fops,
 	.release = &amdgpu_driver_release_kms,
+#if defined(CONFIG_DEBUG_FS)
+	.debugfs_init = amdgpu_debugfs_init,
+#endif
 
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
-- 
2.32.0

Re: [PATCH 1/1] drm/amdgpu: init debugfs drm driver callback

[Christian König]

Am 05.10.21 um 13:58 schrieb Nirmoy Das:
> drm_dev_register() will try to init driver's debugfs using
> drm_driver.debugfs_init call back function. Use that callback
> also for amdgpu to intialize debugfs.

Mhm, why is that useful? We rather wanted to get rid of all this DRM 
midlayering.

Christian.

>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 +++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |  7 +++----
>   3 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index 6611b3c7c149..3076742f8f85 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -1611,8 +1611,9 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
>   DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
>   			amdgpu_debugfs_sclk_set, "%llu\n");
>   
> -int amdgpu_debugfs_init(struct amdgpu_device *adev)
> +void amdgpu_debugfs_init(struct drm_minor *minor)
>   {
> +	struct amdgpu_device *adev = drm_to_adev(minor->dev);
>   	struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
>   	struct dentry *ent;
>   	int r, i;
> @@ -1621,14 +1622,14 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   				  &fops_ib_preempt);
>   	if (IS_ERR(ent)) {
>   		DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n");
> -		return PTR_ERR(ent);
> +		return;
>   	}
>   
>   	ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
>   				  &fops_sclk_set);
>   	if (IS_ERR(ent)) {
>   		DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
> -		return PTR_ERR(ent);
> +		return;
>   	}
>   
>   	/* Register debugfs entries for amdgpu_ttm */
> @@ -1682,11 +1683,10 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   	debugfs_create_blob("amdgpu_discovery", 0444, root,
>   			    &adev->debugfs_discovery_blob);
>   
> -	return 0;
>   }
>   
>   #else
> -int amdgpu_debugfs_init(struct amdgpu_device *adev)
> +void amdgpu_debugfs_init(struct drm_minor *minor)
>   {
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
> index 371a6f0deb29..06b68e16e35d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
> @@ -27,7 +27,7 @@
>    */
>   
>   int amdgpu_debugfs_regs_init(struct amdgpu_device *adev);
> -int amdgpu_debugfs_init(struct amdgpu_device *adev);
> +void amdgpu_debugfs_init(struct drm_minor *minor);
>   void amdgpu_debugfs_fini(struct amdgpu_device *adev);
>   void amdgpu_debugfs_fence_init(struct amdgpu_device *adev);
>   void amdgpu_debugfs_firmware_init(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index df83b1f438b6..ceda650895db 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2012,10 +2012,6 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>   			drm_fbdev_generic_setup(adev_to_drm(adev), 32);
>   	}
>   
> -	ret = amdgpu_debugfs_init(adev);
> -	if (ret)
> -		DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
> -
>   	return 0;
>   
>   err_pci:
> @@ -2479,6 +2475,9 @@ static const struct drm_driver amdgpu_kms_driver = {
>   	.dumb_map_offset = amdgpu_mode_dumb_mmap,
>   	.fops = &amdgpu_driver_kms_fops,
>   	.release = &amdgpu_driver_release_kms,
> +#if defined(CONFIG_DEBUG_FS)
> +	.debugfs_init = amdgpu_debugfs_init,
> +#endif
>   
>   	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,

Re: [PATCH 1/1] drm/amdgpu: init debugfs drm driver callback

[Das, Nirmoy]

On 10/5/2021 1:58 PM, Nirmoy Das wrote:
> drm_dev_register() will try to init driver's debugfs using
> drm_driver.debugfs_init call back function. Use that callback
> also for amdgpu to intialize debugfs.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 +++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |  7 +++----
>   3 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index 6611b3c7c149..3076742f8f85 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -1611,8 +1611,9 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
>   DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
>   			amdgpu_debugfs_sclk_set, "%llu\n");
>   
> -int amdgpu_debugfs_init(struct amdgpu_device *adev)
> +void amdgpu_debugfs_init(struct drm_minor *minor)
>   {
> +	struct amdgpu_device *adev = drm_to_adev(minor->dev);
>   	struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
>   	struct dentry *ent;
>   	int r, i;
> @@ -1621,14 +1622,14 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   				  &fops_ib_preempt);
>   	if (IS_ERR(ent)) {
>   		DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n");
> -		return PTR_ERR(ent);
> +		return;
>   	}
>   
>   	ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
>   				  &fops_sclk_set);
>   	if (IS_ERR(ent)) {
>   		DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
> -		return PTR_ERR(ent);
> +		return;
>   	}
>   
>   	/* Register debugfs entries for amdgpu_ttm */
> @@ -1682,11 +1683,10 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   	debugfs_create_blob("amdgpu_discovery", 0444, root,
>   			    &adev->debugfs_discovery_blob);
>   
> -	return 0;
>   }
>   
>   #else
> -int amdgpu_debugfs_init(struct amdgpu_device *adev)
> +void amdgpu_debugfs_init(struct drm_minor *minor)
>   {
>   	return 0;


Ah, this should be just "return".

>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
> index 371a6f0deb29..06b68e16e35d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
> @@ -27,7 +27,7 @@
>    */
>   
>   int amdgpu_debugfs_regs_init(struct amdgpu_device *adev);
> -int amdgpu_debugfs_init(struct amdgpu_device *adev);
> +void amdgpu_debugfs_init(struct drm_minor *minor);
>   void amdgpu_debugfs_fini(struct amdgpu_device *adev);
>   void amdgpu_debugfs_fence_init(struct amdgpu_device *adev);
>   void amdgpu_debugfs_firmware_init(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index df83b1f438b6..ceda650895db 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2012,10 +2012,6 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>   			drm_fbdev_generic_setup(adev_to_drm(adev), 32);
>   	}
>   
> -	ret = amdgpu_debugfs_init(adev);
> -	if (ret)
> -		DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
> -
>   	return 0;
>   
>   err_pci:
> @@ -2479,6 +2475,9 @@ static const struct drm_driver amdgpu_kms_driver = {
>   	.dumb_map_offset = amdgpu_mode_dumb_mmap,
>   	.fops = &amdgpu_driver_kms_fops,
>   	.release = &amdgpu_driver_release_kms,
> +#if defined(CONFIG_DEBUG_FS)
> +	.debugfs_init = amdgpu_debugfs_init,
> +#endif
>   
>   	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,

Re: [PATCH 1/1] drm/amdgpu: init debugfs drm driver callback

[Das, Nirmoy]

On 10/5/2021 2:01 PM, Christian König wrote:
> Am 05.10.21 um 13:58 schrieb Nirmoy Das:
>> drm_dev_register() will try to init driver's debugfs using
>> drm_driver.debugfs_init call back function. Use that callback
>> also for amdgpu to intialize debugfs.
>
> Mhm, why is that useful? We rather wanted to get rid of all this DRM 
> midlayering.


I was thinking of not calling further debugfs  APIs if we are unable to 
create the root dentry itself by adding another

patch in drm_debugfs_init(). But I agree with removing DRM midlayering, 
I  will then add a  IS_ERR(root) check in amdgpu_debugfs_init()


Nirmoy

>
> Christian.
>
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 +++++-----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |  7 +++----
>>   3 files changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> index 6611b3c7c149..3076742f8f85 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> @@ -1611,8 +1611,9 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
>>   DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
>>               amdgpu_debugfs_sclk_set, "%llu\n");
>>   -int amdgpu_debugfs_init(struct amdgpu_device *adev)
>> +void amdgpu_debugfs_init(struct drm_minor *minor)
>>   {
>> +    struct amdgpu_device *adev = drm_to_adev(minor->dev);
>>       struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
>>       struct dentry *ent;
>>       int r, i;
>> @@ -1621,14 +1622,14 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>> *adev)
>>                     &fops_ib_preempt);
>>       if (IS_ERR(ent)) {
>>           DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs 
>> file\n");
>> -        return PTR_ERR(ent);
>> +        return;
>>       }
>>         ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
>>                     &fops_sclk_set);
>>       if (IS_ERR(ent)) {
>>           DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
>> -        return PTR_ERR(ent);
>> +        return;
>>       }
>>         /* Register debugfs entries for amdgpu_ttm */
>> @@ -1682,11 +1683,10 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>> *adev)
>>       debugfs_create_blob("amdgpu_discovery", 0444, root,
>>                   &adev->debugfs_discovery_blob);
>>   -    return 0;
>>   }
>>     #else
>> -int amdgpu_debugfs_init(struct amdgpu_device *adev)
>> +void amdgpu_debugfs_init(struct drm_minor *minor)
>>   {
>>       return 0;
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>> index 371a6f0deb29..06b68e16e35d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>> @@ -27,7 +27,7 @@
>>    */
>>     int amdgpu_debugfs_regs_init(struct amdgpu_device *adev);
>> -int amdgpu_debugfs_init(struct amdgpu_device *adev);
>> +void amdgpu_debugfs_init(struct drm_minor *minor);
>>   void amdgpu_debugfs_fini(struct amdgpu_device *adev);
>>   void amdgpu_debugfs_fence_init(struct amdgpu_device *adev);
>>   void amdgpu_debugfs_firmware_init(struct amdgpu_device *adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index df83b1f438b6..ceda650895db 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -2012,10 +2012,6 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>>               drm_fbdev_generic_setup(adev_to_drm(adev), 32);
>>       }
>>   -    ret = amdgpu_debugfs_init(adev);
>> -    if (ret)
>> -        DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
>> -
>>       return 0;
>>     err_pci:
>> @@ -2479,6 +2475,9 @@ static const struct drm_driver 
>> amdgpu_kms_driver = {
>>       .dumb_map_offset = amdgpu_mode_dumb_mmap,
>>       .fops = &amdgpu_driver_kms_fops,
>>       .release = &amdgpu_driver_release_kms,
>> +#if defined(CONFIG_DEBUG_FS)
>> +    .debugfs_init = amdgpu_debugfs_init,
>> +#endif
>>         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>

Re: [PATCH 1/1] drm/amdgpu: init debugfs drm driver callback

[Das, Nirmoy]

Hi Christian,

On 10/5/2021 2:01 PM, Christian König wrote:
> Am 05.10.21 um 13:58 schrieb Nirmoy Das:
>> drm_dev_register() will try to init driver's debugfs using
>> drm_driver.debugfs_init call back function. Use that callback
>> also for amdgpu to intialize debugfs.
>
> Mhm, why is that useful? We rather wanted to get rid of all this DRM 
> midlayering.


Actually main issue I am trying to solve is:

When user disables debugfs with CONFIG_DEBUG_FS_ALLOW_NONE, amdgpu gets 
EPERM and throws a DRM_ERROR even though it is not an error as this is 
user controllable.

Shall I just make all debugfs error logs to DRM_WARN ?

ref: https://gitlab.freedesktop.org/drm/amd/-/issues/1686#note_1052168

Regards,

Nirmoy

>
> Christian.
>
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 +++++-----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |  7 +++----
>>   3 files changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> index 6611b3c7c149..3076742f8f85 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> @@ -1611,8 +1611,9 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
>>   DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
>>               amdgpu_debugfs_sclk_set, "%llu\n");
>>   -int amdgpu_debugfs_init(struct amdgpu_device *adev)
>> +void amdgpu_debugfs_init(struct drm_minor *minor)
>>   {
>> +    struct amdgpu_device *adev = drm_to_adev(minor->dev);
>>       struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
>>       struct dentry *ent;
>>       int r, i;
>> @@ -1621,14 +1622,14 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>> *adev)
>>                     &fops_ib_preempt);
>>       if (IS_ERR(ent)) {
>>           DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs 
>> file\n");
>> -        return PTR_ERR(ent);
>> +        return;
>>       }
>>         ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
>>                     &fops_sclk_set);
>>       if (IS_ERR(ent)) {
>>           DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
>> -        return PTR_ERR(ent);
>> +        return;
>>       }
>>         /* Register debugfs entries for amdgpu_ttm */
>> @@ -1682,11 +1683,10 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>> *adev)
>>       debugfs_create_blob("amdgpu_discovery", 0444, root,
>>                   &adev->debugfs_discovery_blob);
>>   -    return 0;
>>   }
>>     #else
>> -int amdgpu_debugfs_init(struct amdgpu_device *adev)
>> +void amdgpu_debugfs_init(struct drm_minor *minor)
>>   {
>>       return 0;
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>> index 371a6f0deb29..06b68e16e35d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>> @@ -27,7 +27,7 @@
>>    */
>>     int amdgpu_debugfs_regs_init(struct amdgpu_device *adev);
>> -int amdgpu_debugfs_init(struct amdgpu_device *adev);
>> +void amdgpu_debugfs_init(struct drm_minor *minor);
>>   void amdgpu_debugfs_fini(struct amdgpu_device *adev);
>>   void amdgpu_debugfs_fence_init(struct amdgpu_device *adev);
>>   void amdgpu_debugfs_firmware_init(struct amdgpu_device *adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index df83b1f438b6..ceda650895db 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -2012,10 +2012,6 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>>               drm_fbdev_generic_setup(adev_to_drm(adev), 32);
>>       }
>>   -    ret = amdgpu_debugfs_init(adev);
>> -    if (ret)
>> -        DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
>> -
>>       return 0;
>>     err_pci:
>> @@ -2479,6 +2475,9 @@ static const struct drm_driver 
>> amdgpu_kms_driver = {
>>       .dumb_map_offset = amdgpu_mode_dumb_mmap,
>>       .fops = &amdgpu_driver_kms_fops,
>>       .release = &amdgpu_driver_release_kms,
>> +#if defined(CONFIG_DEBUG_FS)
>> +    .debugfs_init = amdgpu_debugfs_init,
>> +#endif
>>         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>

Re: [PATCH 1/1] drm/amdgpu: init debugfs drm driver callback

[Christian König]

Am 05.10.21 um 14:20 schrieb Das, Nirmoy:
> Hi Christian,
>
> On 10/5/2021 2:01 PM, Christian König wrote:
>> Am 05.10.21 um 13:58 schrieb Nirmoy Das:
>>> drm_dev_register() will try to init driver's debugfs using
>>> drm_driver.debugfs_init call back function. Use that callback
>>> also for amdgpu to intialize debugfs.
>>
>> Mhm, why is that useful? We rather wanted to get rid of all this DRM 
>> midlayering.
>
>
> Actually main issue I am trying to solve is:
>
> When user disables debugfs with CONFIG_DEBUG_FS_ALLOW_NONE, amdgpu 
> gets EPERM and throws a DRM_ERROR even though it is not an error as 
> this is user controllable.
>
> Shall I just make all debugfs error logs to DRM_WARN ?
>
> ref: https://gitlab.freedesktop.org/drm/amd/-/issues/1686#note_1052168

Why not just add an "if (!root) return" at the beginning of 
amdgpu_debugfs_init() ?

Regards,
Christian.

>
> Regards,
>
> Nirmoy
>
>>
>> Christian.
>>
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 +++++-----
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h |  2 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |  7 +++----
>>>   3 files changed, 9 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> index 6611b3c7c149..3076742f8f85 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> @@ -1611,8 +1611,9 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
>>>   DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
>>>               amdgpu_debugfs_sclk_set, "%llu\n");
>>>   -int amdgpu_debugfs_init(struct amdgpu_device *adev)
>>> +void amdgpu_debugfs_init(struct drm_minor *minor)
>>>   {
>>> +    struct amdgpu_device *adev = drm_to_adev(minor->dev);
>>>       struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
>>>       struct dentry *ent;
>>>       int r, i;
>>> @@ -1621,14 +1622,14 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>> *adev)
>>>                     &fops_ib_preempt);
>>>       if (IS_ERR(ent)) {
>>>           DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs 
>>> file\n");
>>> -        return PTR_ERR(ent);
>>> +        return;
>>>       }
>>>         ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, 
>>> adev,
>>>                     &fops_sclk_set);
>>>       if (IS_ERR(ent)) {
>>>           DRM_ERROR("unable to create amdgpu_set_sclk debugsfs 
>>> file\n");
>>> -        return PTR_ERR(ent);
>>> +        return;
>>>       }
>>>         /* Register debugfs entries for amdgpu_ttm */
>>> @@ -1682,11 +1683,10 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>> *adev)
>>>       debugfs_create_blob("amdgpu_discovery", 0444, root,
>>>                   &adev->debugfs_discovery_blob);
>>>   -    return 0;
>>>   }
>>>     #else
>>> -int amdgpu_debugfs_init(struct amdgpu_device *adev)
>>> +void amdgpu_debugfs_init(struct drm_minor *minor)
>>>   {
>>>       return 0;
>>>   }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>>> index 371a6f0deb29..06b68e16e35d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>>> @@ -27,7 +27,7 @@
>>>    */
>>>     int amdgpu_debugfs_regs_init(struct amdgpu_device *adev);
>>> -int amdgpu_debugfs_init(struct amdgpu_device *adev);
>>> +void amdgpu_debugfs_init(struct drm_minor *minor);
>>>   void amdgpu_debugfs_fini(struct amdgpu_device *adev);
>>>   void amdgpu_debugfs_fence_init(struct amdgpu_device *adev);
>>>   void amdgpu_debugfs_firmware_init(struct amdgpu_device *adev);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index df83b1f438b6..ceda650895db 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -2012,10 +2012,6 @@ static int amdgpu_pci_probe(struct pci_dev 
>>> *pdev,
>>>               drm_fbdev_generic_setup(adev_to_drm(adev), 32);
>>>       }
>>>   -    ret = amdgpu_debugfs_init(adev);
>>> -    if (ret)
>>> -        DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
>>> -
>>>       return 0;
>>>     err_pci:
>>> @@ -2479,6 +2475,9 @@ static const struct drm_driver 
>>> amdgpu_kms_driver = {
>>>       .dumb_map_offset = amdgpu_mode_dumb_mmap,
>>>       .fops = &amdgpu_driver_kms_fops,
>>>       .release = &amdgpu_driver_release_kms,
>>> +#if defined(CONFIG_DEBUG_FS)
>>> +    .debugfs_init = amdgpu_debugfs_init,
>>> +#endif
>>>         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>>       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>>

Re: [PATCH 1/1] drm/amdgpu: init debugfs drm driver callback

[Das, Nirmoy]

On 10/5/2021 2:41 PM, Christian König wrote:
>
>
> Am 05.10.21 um 14:20 schrieb Das, Nirmoy:
>> Hi Christian,
>>
>> On 10/5/2021 2:01 PM, Christian König wrote:
>>> Am 05.10.21 um 13:58 schrieb Nirmoy Das:
>>>> drm_dev_register() will try to init driver's debugfs using
>>>> drm_driver.debugfs_init call back function. Use that callback
>>>> also for amdgpu to intialize debugfs.
>>>
>>> Mhm, why is that useful? We rather wanted to get rid of all this DRM 
>>> midlayering.
>>
>>
>> Actually main issue I am trying to solve is:
>>
>> When user disables debugfs with CONFIG_DEBUG_FS_ALLOW_NONE, amdgpu 
>> gets EPERM and throws a DRM_ERROR even though it is not an error as 
>> this is user controllable.
>>
>> Shall I just make all debugfs error logs to DRM_WARN ?
>>
>> ref: 
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686%23note_1052168&data=04%7C01%7Cnirmoy.das%40amd.com%7C63032bdca1394c92e88808d987fd867b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637690345246933980%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=eXyj6wlxD0YGiNsjB4smmRAm2KKGRtq%2FWiDSMzEWTo8%3D&reserved=0
>
> Why not just add an "if (!root) return" at the beginning of 
> amdgpu_debugfs_init() ?


This is fine too, I will resend.


Nirmoy

>
> Regards,
> Christian.
>
>>
>> Regards,
>>
>> Nirmoy
>>
>>>
>>> Christian.
>>>
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 +++++-----
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h |  2 +-
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c     |  7 +++----
>>>>   3 files changed, 9 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> index 6611b3c7c149..3076742f8f85 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> @@ -1611,8 +1611,9 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
>>>>   DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
>>>>               amdgpu_debugfs_sclk_set, "%llu\n");
>>>>   -int amdgpu_debugfs_init(struct amdgpu_device *adev)
>>>> +void amdgpu_debugfs_init(struct drm_minor *minor)
>>>>   {
>>>> +    struct amdgpu_device *adev = drm_to_adev(minor->dev);
>>>>       struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
>>>>       struct dentry *ent;
>>>>       int r, i;
>>>> @@ -1621,14 +1622,14 @@ int amdgpu_debugfs_init(struct 
>>>> amdgpu_device *adev)
>>>>                     &fops_ib_preempt);
>>>>       if (IS_ERR(ent)) {
>>>>           DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs 
>>>> file\n");
>>>> -        return PTR_ERR(ent);
>>>> +        return;
>>>>       }
>>>>         ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, 
>>>> adev,
>>>>                     &fops_sclk_set);
>>>>       if (IS_ERR(ent)) {
>>>>           DRM_ERROR("unable to create amdgpu_set_sclk debugsfs 
>>>> file\n");
>>>> -        return PTR_ERR(ent);
>>>> +        return;
>>>>       }
>>>>         /* Register debugfs entries for amdgpu_ttm */
>>>> @@ -1682,11 +1683,10 @@ int amdgpu_debugfs_init(struct 
>>>> amdgpu_device *adev)
>>>>       debugfs_create_blob("amdgpu_discovery", 0444, root,
>>>>                   &adev->debugfs_discovery_blob);
>>>>   -    return 0;
>>>>   }
>>>>     #else
>>>> -int amdgpu_debugfs_init(struct amdgpu_device *adev)
>>>> +void amdgpu_debugfs_init(struct drm_minor *minor)
>>>>   {
>>>>       return 0;
>>>>   }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>>>> index 371a6f0deb29..06b68e16e35d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>>>> @@ -27,7 +27,7 @@
>>>>    */
>>>>     int amdgpu_debugfs_regs_init(struct amdgpu_device *adev);
>>>> -int amdgpu_debugfs_init(struct amdgpu_device *adev);
>>>> +void amdgpu_debugfs_init(struct drm_minor *minor);
>>>>   void amdgpu_debugfs_fini(struct amdgpu_device *adev);
>>>>   void amdgpu_debugfs_fence_init(struct amdgpu_device *adev);
>>>>   void amdgpu_debugfs_firmware_init(struct amdgpu_device *adev);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> index df83b1f438b6..ceda650895db 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> @@ -2012,10 +2012,6 @@ static int amdgpu_pci_probe(struct pci_dev 
>>>> *pdev,
>>>>               drm_fbdev_generic_setup(adev_to_drm(adev), 32);
>>>>       }
>>>>   -    ret = amdgpu_debugfs_init(adev);
>>>> -    if (ret)
>>>> -        DRM_ERROR("Creating debugfs files failed (%d).\n", ret);
>>>> -
>>>>       return 0;
>>>>     err_pci:
>>>> @@ -2479,6 +2475,9 @@ static const struct drm_driver 
>>>> amdgpu_kms_driver = {
>>>>       .dumb_map_offset = amdgpu_mode_dumb_mmap,
>>>>       .fops = &amdgpu_driver_kms_fops,
>>>>       .release = &amdgpu_driver_release_kms,
>>>> +#if defined(CONFIG_DEBUG_FS)
>>>> +    .debugfs_init = amdgpu_debugfs_init,
>>>> +#endif
>>>>         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>>>       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>>>
>