[PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs

[PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs

[Nirmoy Das]

debugfs APIs returns encoded error so use
IS_ERR for checking return value.

References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index d256215ab2c7..077f9baf74fe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
 	struct dentry *ent;
 	int r, i;
 
-
-
 	ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
 				  &fops_ib_preempt);
-	if (!ent) {
+	if (IS_ERR(ent)) {
 		DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n");
 		return -EIO;
 	}
 
 	ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
 				  &fops_sclk_set);
-	if (!ent) {
+	if (IS_ERR(ent)) {
 		DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
 		return -EIO;
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 7b634a1517f9..f40753e1a60d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
 	ent = debugfs_create_file(name,
 				  S_IFREG | S_IRUGO, root,
 				  ring, &amdgpu_debugfs_ring_fops);
-	if (!ent)
+	if (IS_ERR(ent))
 		return -ENOMEM;
 
 	i_size_write(ent->d_inode, ring->ring_size + 12);
-- 
2.32.0

[PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Nirmoy Das]

Use debugfs_create_file_size API for creating ring debugfs
file, also cleanup surrounding code.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 +++++-----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
 3 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 077f9baf74fe..dee56ab19a8f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
 		if (!ring)
 			continue;
 
-		if (amdgpu_debugfs_ring_init(adev, ring)) {
-			DRM_ERROR("Failed to register debugfs file for rings !\n");
-		}
+		amdgpu_debugfs_ring_init(adev, ring);
 	}
 
 	amdgpu_ras_debugfs_create_all(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index f40753e1a60d..968521d80514 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -415,26 +415,20 @@ static const struct file_operations amdgpu_debugfs_ring_fops = {
 
 #endif
 
-int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
+void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
 			     struct amdgpu_ring *ring)
 {
 #if defined(CONFIG_DEBUG_FS)
 	struct drm_minor *minor = adev_to_drm(adev)->primary;
-	struct dentry *ent, *root = minor->debugfs_root;
+	struct dentry *root = minor->debugfs_root;
 	char name[32];
 
 	sprintf(name, "amdgpu_ring_%s", ring->name);
 
-	ent = debugfs_create_file(name,
-				  S_IFREG | S_IRUGO, root,
-				  ring, &amdgpu_debugfs_ring_fops);
-	if (IS_ERR(ent))
-		return -ENOMEM;
-
-	i_size_write(ent->d_inode, ring->ring_size + 12);
-	ring->ent = ent;
+	debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
+				 &amdgpu_debugfs_ring_fops,
+				 ring->ring_size + 12);
 #endif
-	return 0;
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 88d80eb3fea1..c29fbce0a5b4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -253,10 +253,6 @@ struct amdgpu_ring {
 	bool			has_compute_vm_bug;
 	bool			no_scheduler;
 	int			hw_prio;
-
-#if defined(CONFIG_DEBUG_FS)
-	struct dentry *ent;
-#endif
 };
 
 #define amdgpu_ring_parse_cs(r, p, ib) ((r)->funcs->parse_cs((p), (ib)))
@@ -356,8 +352,6 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
 
 int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
 
-int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
+void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
 			     struct amdgpu_ring *ring);
-void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
-
 #endif
-- 
2.32.0

Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Sharma, Shashank]

On 9/2/2021 5:14 PM, Nirmoy Das wrote:
> Use debugfs_create_file_size API for creating ring debugfs
> file, also cleanup surrounding code.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 +++++-----------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
>   3 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index 077f9baf74fe..dee56ab19a8f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   		if (!ring)
>   			continue;
>   
> -		if (amdgpu_debugfs_ring_init(adev, ring)) {
> -			DRM_ERROR("Failed to register debugfs file for rings !\n");
> -		}
> +		amdgpu_debugfs_ring_init(adev, ring);
>   	}
>   
>   	amdgpu_ras_debugfs_create_all(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index f40753e1a60d..968521d80514 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -415,26 +415,20 @@ static const struct file_operations amdgpu_debugfs_ring_fops = {
>   
>   #endif
>   
> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>   			     struct amdgpu_ring *ring)
>   {
>   #if defined(CONFIG_DEBUG_FS)
>   	struct drm_minor *minor = adev_to_drm(adev)->primary;
> -	struct dentry *ent, *root = minor->debugfs_root;
> +	struct dentry *root = minor->debugfs_root;
>   	char name[32];
>   
>   	sprintf(name, "amdgpu_ring_%s", ring->name);
>   
> -	ent = debugfs_create_file(name,
> -				  S_IFREG | S_IRUGO, root,
> -				  ring, &amdgpu_debugfs_ring_fops);
> -	if (IS_ERR(ent))
> -		return -ENOMEM;

Why are we doing this ? Why to make it void from int ?

- Shashank


> -
> -	i_size_write(ent->d_inode, ring->ring_size + 12);
> -	ring->ent = ent;
> +	debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
> +				 &amdgpu_debugfs_ring_fops,
> +				 ring->ring_size + 12);
>   #endif
> -	return 0;
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 88d80eb3fea1..c29fbce0a5b4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -253,10 +253,6 @@ struct amdgpu_ring {
>   	bool			has_compute_vm_bug;
>   	bool			no_scheduler;
>   	int			hw_prio;
> -
> -#if defined(CONFIG_DEBUG_FS)
> -	struct dentry *ent;
> -#endif
>   };
>   
>   #define amdgpu_ring_parse_cs(r, p, ib) ((r)->funcs->parse_cs((p), (ib)))
> @@ -356,8 +352,6 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>   
>   int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>   
> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>   			     struct amdgpu_ring *ring);
> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
> -
>   #endif
> 

Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Das, Nirmoy]

On 9/3/2021 8:36 AM, Sharma, Shashank wrote:
>
>
> On 9/2/2021 5:14 PM, Nirmoy Das wrote:
>> Use debugfs_create_file_size API for creating ring debugfs
>> file, also cleanup surrounding code.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 +++++-----------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
>>   3 files changed, 7 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> index 077f9baf74fe..dee56ab19a8f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>> *adev)
>>           if (!ring)
>>               continue;
>>   -        if (amdgpu_debugfs_ring_init(adev, ring)) {
>> -            DRM_ERROR("Failed to register debugfs file for rings !\n");
>> -        }
>> +        amdgpu_debugfs_ring_init(adev, ring);
>>       }
>>         amdgpu_ras_debugfs_create_all(adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> index f40753e1a60d..968521d80514 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> @@ -415,26 +415,20 @@ static const struct file_operations 
>> amdgpu_debugfs_ring_fops = {
>>     #endif
>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>                    struct amdgpu_ring *ring)
>>   {
>>   #if defined(CONFIG_DEBUG_FS)
>>       struct drm_minor *minor = adev_to_drm(adev)->primary;
>> -    struct dentry *ent, *root = minor->debugfs_root;
>> +    struct dentry *root = minor->debugfs_root;
>>       char name[32];
>>         sprintf(name, "amdgpu_ring_%s", ring->name);
>>   -    ent = debugfs_create_file(name,
>> -                  S_IFREG | S_IRUGO, root,
>> -                  ring, &amdgpu_debugfs_ring_fops);
>> -    if (IS_ERR(ent))
>> -        return -ENOMEM;
>
> Why are we doing this ? Why to make it void from int ?


We tend to ignore debugfs return values as those are not serious errors. 
This to sync with rest of our

debugfs calls.


Regards,

Nirmoy

>
> - Shashank
>
>
>> -
>> -    i_size_write(ent->d_inode, ring->ring_size + 12);
>> -    ring->ent = ent;
>> +    debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
>> +                 &amdgpu_debugfs_ring_fops,
>> +                 ring->ring_size + 12);
>>   #endif
>> -    return 0;
>>   }
>>     /**
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index 88d80eb3fea1..c29fbce0a5b4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -253,10 +253,6 @@ struct amdgpu_ring {
>>       bool            has_compute_vm_bug;
>>       bool            no_scheduler;
>>       int            hw_prio;
>> -
>> -#if defined(CONFIG_DEBUG_FS)
>> -    struct dentry *ent;
>> -#endif
>>   };
>>     #define amdgpu_ring_parse_cs(r, p, ib) ((r)->funcs->parse_cs((p), 
>> (ib)))
>> @@ -356,8 +352,6 @@ static inline void 
>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>     int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>                    struct amdgpu_ring *ring);
>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>> -
>>   #endif
>>

Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Sharma, Shashank]

On 9/3/2021 1:39 PM, Das, Nirmoy wrote:
> 
> On 9/3/2021 8:36 AM, Sharma, Shashank wrote:
>>
>>
>> On 9/2/2021 5:14 PM, Nirmoy Das wrote:
>>> Use debugfs_create_file_size API for creating ring debugfs
>>> file, also cleanup surrounding code.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 +++++-----------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
>>>   3 files changed, 7 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> index 077f9baf74fe..dee56ab19a8f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>> *adev)
>>>           if (!ring)
>>>               continue;
>>>   -        if (amdgpu_debugfs_ring_init(adev, ring)) {
>>> -            DRM_ERROR("Failed to register debugfs file for rings !\n");
>>> -        }
>>> +        amdgpu_debugfs_ring_init(adev, ring);
>>>       }
>>>         amdgpu_ras_debugfs_create_all(adev);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> index f40753e1a60d..968521d80514 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> @@ -415,26 +415,20 @@ static const struct file_operations 
>>> amdgpu_debugfs_ring_fops = {
>>>     #endif
>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>                    struct amdgpu_ring *ring)
>>>   {
>>>   #if defined(CONFIG_DEBUG_FS)
>>>       struct drm_minor *minor = adev_to_drm(adev)->primary;
>>> -    struct dentry *ent, *root = minor->debugfs_root;
>>> +    struct dentry *root = minor->debugfs_root;
>>>       char name[32];
>>>         sprintf(name, "amdgpu_ring_%s", ring->name);
>>>   -    ent = debugfs_create_file(name,
>>> -                  S_IFREG | S_IRUGO, root,
>>> -                  ring, &amdgpu_debugfs_ring_fops);
>>> -    if (IS_ERR(ent))
>>> -        return -ENOMEM;
>>
>> Why are we doing this ? Why to make it void from int ?
> 
> 
> We tend to ignore debugfs return values as those are not serious errors. 
> This to sync with rest of our
> 
> debugfs calls.
> 
> 
> Regards,
> 
> Nirmoy
> 


I am not suere if completely removing the provision of return value is a 
good way of doing it, we can always ignore it at the caller side, isn't 
it ?

- Shashank

>>
>> - Shashank
>>
>>
>>> -
>>> -    i_size_write(ent->d_inode, ring->ring_size + 12);
>>> -    ring->ent = ent;
>>> +    debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
>>> +                 &amdgpu_debugfs_ring_fops,
>>> +                 ring->ring_size + 12);
>>>   #endif
>>> -    return 0;
>>>   }
>>>     /**
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> index 88d80eb3fea1..c29fbce0a5b4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> @@ -253,10 +253,6 @@ struct amdgpu_ring {
>>>       bool            has_compute_vm_bug;
>>>       bool            no_scheduler;
>>>       int            hw_prio;
>>> -
>>> -#if defined(CONFIG_DEBUG_FS)
>>> -    struct dentry *ent;
>>> -#endif
>>>   };
>>>     #define amdgpu_ring_parse_cs(r, p, ib) ((r)->funcs->parse_cs((p), 
>>> (ib)))
>>> @@ -356,8 +352,6 @@ static inline void 
>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>>     int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>                    struct amdgpu_ring *ring);
>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>> -
>>>   #endif
>>>

Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Das, Nirmoy]

On 9/3/2021 5:26 PM, Sharma, Shashank wrote:
>
>
> On 9/3/2021 1:39 PM, Das, Nirmoy wrote:
>>
>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote:
>>>
>>>
>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote:
>>>> Use debugfs_create_file_size API for creating ring debugfs
>>>> file, also cleanup surrounding code.
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 +++++-----------
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
>>>>   3 files changed, 7 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> index 077f9baf74fe..dee56ab19a8f 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>>> *adev)
>>>>           if (!ring)
>>>>               continue;
>>>>   -        if (amdgpu_debugfs_ring_init(adev, ring)) {
>>>> -            DRM_ERROR("Failed to register debugfs file for rings 
>>>> !\n");
>>>> -        }
>>>> +        amdgpu_debugfs_ring_init(adev, ring);
>>>>       }
>>>>         amdgpu_ras_debugfs_create_all(adev);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>> index f40753e1a60d..968521d80514 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>> @@ -415,26 +415,20 @@ static const struct file_operations 
>>>> amdgpu_debugfs_ring_fops = {
>>>>     #endif
>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>                    struct amdgpu_ring *ring)
>>>>   {
>>>>   #if defined(CONFIG_DEBUG_FS)
>>>>       struct drm_minor *minor = adev_to_drm(adev)->primary;
>>>> -    struct dentry *ent, *root = minor->debugfs_root;
>>>> +    struct dentry *root = minor->debugfs_root;
>>>>       char name[32];
>>>>         sprintf(name, "amdgpu_ring_%s", ring->name);
>>>>   -    ent = debugfs_create_file(name,
>>>> -                  S_IFREG | S_IRUGO, root,
>>>> -                  ring, &amdgpu_debugfs_ring_fops);
>>>> -    if (IS_ERR(ent))
>>>> -        return -ENOMEM;
>>>
>>> Why are we doing this ? Why to make it void from int ?
>>
>>
>> We tend to ignore debugfs return values as those are not serious 
>> errors. This to sync with rest of our
>>
>> debugfs calls.
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>
>
> I am not suere if completely removing the provision of return value is 
> a good way of doing it, we can always ignore it at the caller side, 
> isn't it ?


Yes, we are currently throwing an error msg and ignoring it. I don't 
have a strong opinion regarding this, I will send a v2 restoring  
previous behavior.


Thanks,

Nirmoy


>
> - Shashank
>
>>>
>>> - Shashank
>>>
>>>
>>>> -
>>>> -    i_size_write(ent->d_inode, ring->ring_size + 12);
>>>> -    ring->ent = ent;
>>>> +    debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
>>>> +                 &amdgpu_debugfs_ring_fops,
>>>> +                 ring->ring_size + 12);
>>>>   #endif
>>>> -    return 0;
>>>>   }
>>>>     /**
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>> index 88d80eb3fea1..c29fbce0a5b4 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring {
>>>>       bool            has_compute_vm_bug;
>>>>       bool            no_scheduler;
>>>>       int            hw_prio;
>>>> -
>>>> -#if defined(CONFIG_DEBUG_FS)
>>>> -    struct dentry *ent;
>>>> -#endif
>>>>   };
>>>>     #define amdgpu_ring_parse_cs(r, p, ib) 
>>>> ((r)->funcs->parse_cs((p), (ib)))
>>>> @@ -356,8 +352,6 @@ static inline void 
>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>>>     int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>                    struct amdgpu_ring *ring);
>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>>> -
>>>>   #endif
>>>>

Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Das, Nirmoy]

Hi Shashank,

On 9/3/2021 5:51 PM, Das, Nirmoy wrote:
>
> On 9/3/2021 5:26 PM, Sharma, Shashank wrote:
>>
>>
>> On 9/3/2021 1:39 PM, Das, Nirmoy wrote:
>>>
>>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote:
>>>>
>>>>
>>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote:
>>>>> Use debugfs_create_file_size API for creating ring debugfs
>>>>> file, also cleanup surrounding code.
>>>>>
>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 +++++-----------
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
>>>>>   3 files changed, 7 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> index 077f9baf74fe..dee56ab19a8f 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>>>> *adev)
>>>>>           if (!ring)
>>>>>               continue;
>>>>>   -        if (amdgpu_debugfs_ring_init(adev, ring)) {
>>>>> -            DRM_ERROR("Failed to register debugfs file for rings 
>>>>> !\n");
>>>>> -        }
>>>>> +        amdgpu_debugfs_ring_init(adev, ring);
>>>>>       }
>>>>>         amdgpu_ras_debugfs_create_all(adev);
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>> index f40753e1a60d..968521d80514 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>> @@ -415,26 +415,20 @@ static const struct file_operations 
>>>>> amdgpu_debugfs_ring_fops = {
>>>>>     #endif
>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>                    struct amdgpu_ring *ring)
>>>>>   {
>>>>>   #if defined(CONFIG_DEBUG_FS)
>>>>>       struct drm_minor *minor = adev_to_drm(adev)->primary;
>>>>> -    struct dentry *ent, *root = minor->debugfs_root;
>>>>> +    struct dentry *root = minor->debugfs_root;
>>>>>       char name[32];
>>>>>         sprintf(name, "amdgpu_ring_%s", ring->name);
>>>>>   -    ent = debugfs_create_file(name,
>>>>> -                  S_IFREG | S_IRUGO, root,
>>>>> -                  ring, &amdgpu_debugfs_ring_fops);
>>>>> -    if (IS_ERR(ent))
>>>>> -        return -ENOMEM;
>>>>
>>>> Why are we doing this ? Why to make it void from int ?
>>>
>>>
>>> We tend to ignore debugfs return values as those are not serious 
>>> errors. This to sync with rest of our
>>>
>>> debugfs calls.
>>>
>>>
>>> Regards,
>>>
>>> Nirmoy
>>>
>>
>>
>> I am not suere if completely removing the provision of return value 
>> is a good way of doing it, we can always ignore it at the caller 
>> side, isn't it ?



I just realized while making the change debugfs_create_file_size() is 
void return, so we don't have anything useful to return in 
amdgpu_debugfs_ring_init()


Regards,

Nirmoy


>
>
> Yes, we are currently throwing an error msg and ignoring it. I don't 
> have a strong opinion regarding this, I will send a v2 restoring  
> previous behavior.
>
>
> Thanks,
>
> Nirmoy
>
>
>>
>> - Shashank
>>
>>>>
>>>> - Shashank
>>>>
>>>>
>>>>> -
>>>>> -    i_size_write(ent->d_inode, ring->ring_size + 12);
>>>>> -    ring->ent = ent;
>>>>> +    debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
>>>>> +                 &amdgpu_debugfs_ring_fops,
>>>>> +                 ring->ring_size + 12);
>>>>>   #endif
>>>>> -    return 0;
>>>>>   }
>>>>>     /**
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>> index 88d80eb3fea1..c29fbce0a5b4 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring {
>>>>>       bool            has_compute_vm_bug;
>>>>>       bool            no_scheduler;
>>>>>       int            hw_prio;
>>>>> -
>>>>> -#if defined(CONFIG_DEBUG_FS)
>>>>> -    struct dentry *ent;
>>>>> -#endif
>>>>>   };
>>>>>     #define amdgpu_ring_parse_cs(r, p, ib) 
>>>>> ((r)->funcs->parse_cs((p), (ib)))
>>>>> @@ -356,8 +352,6 @@ static inline void 
>>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>>>>     int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>                    struct amdgpu_ring *ring);
>>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>>>> -
>>>>>   #endif
>>>>>

Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Sharma, Shashank]

On 9/3/2021 9:44 PM, Das, Nirmoy wrote:
> Hi Shashank,
> 
> On 9/3/2021 5:51 PM, Das, Nirmoy wrote:
>>
>> On 9/3/2021 5:26 PM, Sharma, Shashank wrote:
>>>
>>>
>>> On 9/3/2021 1:39 PM, Das, Nirmoy wrote:
>>>>
>>>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote:
>>>>>
>>>>>
>>>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote:
>>>>>> Use debugfs_create_file_size API for creating ring debugfs
>>>>>> file, also cleanup surrounding code.
>>>>>>
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 +++++-----------
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
>>>>>>   3 files changed, 7 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>> index 077f9baf74fe..dee56ab19a8f 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>>>>> *adev)
>>>>>>           if (!ring)
>>>>>>               continue;
>>>>>>   -        if (amdgpu_debugfs_ring_init(adev, ring)) {
>>>>>> -            DRM_ERROR("Failed to register debugfs file for rings 
>>>>>> !\n");
>>>>>> -        }
>>>>>> +        amdgpu_debugfs_ring_init(adev, ring);
>>>>>>       }
>>>>>>         amdgpu_ras_debugfs_create_all(adev);
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>> index f40753e1a60d..968521d80514 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>> @@ -415,26 +415,20 @@ static const struct file_operations 
>>>>>> amdgpu_debugfs_ring_fops = {
>>>>>>     #endif
>>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>                    struct amdgpu_ring *ring)
>>>>>>   {
>>>>>>   #if defined(CONFIG_DEBUG_FS)
>>>>>>       struct drm_minor *minor = adev_to_drm(adev)->primary;
>>>>>> -    struct dentry *ent, *root = minor->debugfs_root;
>>>>>> +    struct dentry *root = minor->debugfs_root;
>>>>>>       char name[32];
>>>>>>         sprintf(name, "amdgpu_ring_%s", ring->name);
>>>>>>   -    ent = debugfs_create_file(name,
>>>>>> -                  S_IFREG | S_IRUGO, root,
>>>>>> -                  ring, &amdgpu_debugfs_ring_fops);
>>>>>> -    if (IS_ERR(ent))
>>>>>> -        return -ENOMEM;
>>>>>
>>>>> Why are we doing this ? Why to make it void from int ?
>>>>
>>>>
>>>> We tend to ignore debugfs return values as those are not serious 
>>>> errors. This to sync with rest of our
>>>>
>>>> debugfs calls.
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Nirmoy
>>>>
>>>
>>>
>>> I am not suere if completely removing the provision of return value 
>>> is a good way of doing it, we can always ignore it at the caller 
>>> side, isn't it ?
> 
> 
> 
> I just realized while making the change debugfs_create_file_size() is 
> void return, so we don't have anything useful to return in 
> amdgpu_debugfs_ring_init()
> 
> 

Ah, it makes better sense now. Probably just a mention in the body of 
the message that we are moving from debugfs_create_file() to 
debugfs_create_file_size(), will make this change of return type more 
logical.

- Shashank

> Regards,
> 
> Nirmoy
> 
> 
>>
>>
>> Yes, we are currently throwing an error msg and ignoring it. I don't 
>> have a strong opinion regarding this, I will send a v2 restoring 
>> previous behavior.
>>
>>
>> Thanks,
>>
>> Nirmoy
>>
>>
>>>
>>> - Shashank
>>>
>>>>>
>>>>> - Shashank
>>>>>
>>>>>
>>>>>> -
>>>>>> -    i_size_write(ent->d_inode, ring->ring_size + 12);
>>>>>> -    ring->ent = ent;
>>>>>> +    debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
>>>>>> +                 &amdgpu_debugfs_ring_fops,
>>>>>> +                 ring->ring_size + 12);
>>>>>>   #endif
>>>>>> -    return 0;
>>>>>>   }
>>>>>>     /**
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>> index 88d80eb3fea1..c29fbce0a5b4 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring {
>>>>>>       bool            has_compute_vm_bug;
>>>>>>       bool            no_scheduler;
>>>>>>       int            hw_prio;
>>>>>> -
>>>>>> -#if defined(CONFIG_DEBUG_FS)
>>>>>> -    struct dentry *ent;
>>>>>> -#endif
>>>>>>   };
>>>>>>     #define amdgpu_ring_parse_cs(r, p, ib) 
>>>>>> ((r)->funcs->parse_cs((p), (ib)))
>>>>>> @@ -356,8 +352,6 @@ static inline void 
>>>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>>>>>     int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>                    struct amdgpu_ring *ring);
>>>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>>>>> -
>>>>>>   #endif
>>>>>>

Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Das, Nirmoy]

On 9/5/2021 10:03 AM, Sharma, Shashank wrote:
>
>
> On 9/3/2021 9:44 PM, Das, Nirmoy wrote:
>> Hi Shashank,
>>
>> On 9/3/2021 5:51 PM, Das, Nirmoy wrote:
>>>
>>> On 9/3/2021 5:26 PM, Sharma, Shashank wrote:
>>>>
>>>>
>>>> On 9/3/2021 1:39 PM, Das, Nirmoy wrote:
>>>>>
>>>>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote:
>>>>>>
>>>>>>
>>>>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote:
>>>>>>> Use debugfs_create_file_size API for creating ring debugfs
>>>>>>> file, also cleanup surrounding code.
>>>>>>>
>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 +++++-----------
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
>>>>>>>   3 files changed, 7 insertions(+), 21 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>> index 077f9baf74fe..dee56ab19a8f 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct 
>>>>>>> amdgpu_device *adev)
>>>>>>>           if (!ring)
>>>>>>>               continue;
>>>>>>>   -        if (amdgpu_debugfs_ring_init(adev, ring)) {
>>>>>>> -            DRM_ERROR("Failed to register debugfs file for 
>>>>>>> rings !\n");
>>>>>>> -        }
>>>>>>> +        amdgpu_debugfs_ring_init(adev, ring);
>>>>>>>       }
>>>>>>>         amdgpu_ras_debugfs_create_all(adev);
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>>> index f40753e1a60d..968521d80514 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>>> @@ -415,26 +415,20 @@ static const struct file_operations 
>>>>>>> amdgpu_debugfs_ring_fops = {
>>>>>>>     #endif
>>>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>                    struct amdgpu_ring *ring)
>>>>>>>   {
>>>>>>>   #if defined(CONFIG_DEBUG_FS)
>>>>>>>       struct drm_minor *minor = adev_to_drm(adev)->primary;
>>>>>>> -    struct dentry *ent, *root = minor->debugfs_root;
>>>>>>> +    struct dentry *root = minor->debugfs_root;
>>>>>>>       char name[32];
>>>>>>>         sprintf(name, "amdgpu_ring_%s", ring->name);
>>>>>>>   -    ent = debugfs_create_file(name,
>>>>>>> -                  S_IFREG | S_IRUGO, root,
>>>>>>> -                  ring, &amdgpu_debugfs_ring_fops);
>>>>>>> -    if (IS_ERR(ent))
>>>>>>> -        return -ENOMEM;
>>>>>>
>>>>>> Why are we doing this ? Why to make it void from int ?
>>>>>
>>>>>
>>>>> We tend to ignore debugfs return values as those are not serious 
>>>>> errors. This to sync with rest of our
>>>>>
>>>>> debugfs calls.
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Nirmoy
>>>>>
>>>>
>>>>
>>>> I am not suere if completely removing the provision of return value 
>>>> is a good way of doing it, we can always ignore it at the caller 
>>>> side, isn't it ?
>>
>>
>>
>> I just realized while making the change debugfs_create_file_size() is 
>> void return, so we don't have anything useful to return in 
>> amdgpu_debugfs_ring_init()
>>
>>
>
> Ah, it makes better sense now. Probably just a mention in the body of 
> the message that we are moving from debugfs_create_file() to 
> debugfs_create_file_size(), will make this change of return type more 
> logical.


Yes, I have that "Use debugfs_create_file_size API for creating ring 
debugfs file,..."


Nirmoy

>
> - Shashank
>
>> Regards,
>>
>> Nirmoy
>>
>>
>>>
>>>
>>> Yes, we are currently throwing an error msg and ignoring it. I don't 
>>> have a strong opinion regarding this, I will send a v2 restoring 
>>> previous behavior.
>>>
>>>
>>> Thanks,
>>>
>>> Nirmoy
>>>
>>>
>>>>
>>>> - Shashank
>>>>
>>>>>>
>>>>>> - Shashank
>>>>>>
>>>>>>
>>>>>>> -
>>>>>>> -    i_size_write(ent->d_inode, ring->ring_size + 12);
>>>>>>> -    ring->ent = ent;
>>>>>>> +    debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
>>>>>>> +                 &amdgpu_debugfs_ring_fops,
>>>>>>> +                 ring->ring_size + 12);
>>>>>>>   #endif
>>>>>>> -    return 0;
>>>>>>>   }
>>>>>>>     /**
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>>> index 88d80eb3fea1..c29fbce0a5b4 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring {
>>>>>>>       bool            has_compute_vm_bug;
>>>>>>>       bool            no_scheduler;
>>>>>>>       int            hw_prio;
>>>>>>> -
>>>>>>> -#if defined(CONFIG_DEBUG_FS)
>>>>>>> -    struct dentry *ent;
>>>>>>> -#endif
>>>>>>>   };
>>>>>>>     #define amdgpu_ring_parse_cs(r, p, ib) 
>>>>>>> ((r)->funcs->parse_cs((p), (ib)))
>>>>>>> @@ -356,8 +352,6 @@ static inline void 
>>>>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>>>>>>     int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>                    struct amdgpu_ring *ring);
>>>>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>>>>>> -
>>>>>>>   #endif
>>>>>>>

Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Sharma, Shashank]

On 9/5/2021 5:01 PM, Das, Nirmoy wrote:
> 
> On 9/5/2021 10:03 AM, Sharma, Shashank wrote:
>>
>>
>> On 9/3/2021 9:44 PM, Das, Nirmoy wrote:
>>> Hi Shashank,
>>>
>>> On 9/3/2021 5:51 PM, Das, Nirmoy wrote:
>>>>
>>>> On 9/3/2021 5:26 PM, Sharma, Shashank wrote:
>>>>>
>>>>>
>>>>> On 9/3/2021 1:39 PM, Das, Nirmoy wrote:
>>>>>>
>>>>>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote:
>>>>>>>> Use debugfs_create_file_size API for creating ring debugfs
>>>>>>>> file, also cleanup surrounding code.
>>>>>>>>
>>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 +++++-----------
>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
>>>>>>>>   3 files changed, 7 insertions(+), 21 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>> index 077f9baf74fe..dee56ab19a8f 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct 
>>>>>>>> amdgpu_device *adev)
>>>>>>>>           if (!ring)
>>>>>>>>               continue;
>>>>>>>>   -        if (amdgpu_debugfs_ring_init(adev, ring)) {
>>>>>>>> -            DRM_ERROR("Failed to register debugfs file for 
>>>>>>>> rings !\n");
>>>>>>>> -        }
>>>>>>>> +        amdgpu_debugfs_ring_init(adev, ring);
>>>>>>>>       }
>>>>>>>>         amdgpu_ras_debugfs_create_all(adev);
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>>>> index f40753e1a60d..968521d80514 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>>>> @@ -415,26 +415,20 @@ static const struct file_operations 
>>>>>>>> amdgpu_debugfs_ring_fops = {
>>>>>>>>     #endif
>>>>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>>                    struct amdgpu_ring *ring)
>>>>>>>>   {
>>>>>>>>   #if defined(CONFIG_DEBUG_FS)
>>>>>>>>       struct drm_minor *minor = adev_to_drm(adev)->primary;
>>>>>>>> -    struct dentry *ent, *root = minor->debugfs_root;
>>>>>>>> +    struct dentry *root = minor->debugfs_root;
>>>>>>>>       char name[32];
>>>>>>>>         sprintf(name, "amdgpu_ring_%s", ring->name);
>>>>>>>>   -    ent = debugfs_create_file(name,
>>>>>>>> -                  S_IFREG | S_IRUGO, root,
>>>>>>>> -                  ring, &amdgpu_debugfs_ring_fops);
>>>>>>>> -    if (IS_ERR(ent))
>>>>>>>> -        return -ENOMEM;
>>>>>>>
>>>>>>> Why are we doing this ? Why to make it void from int ?
>>>>>>
>>>>>>
>>>>>> We tend to ignore debugfs return values as those are not serious 
>>>>>> errors. This to sync with rest of our
>>>>>>
>>>>>> debugfs calls.
>>>>>>
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Nirmoy
>>>>>>
>>>>>
>>>>>
>>>>> I am not suere if completely removing the provision of return value 
>>>>> is a good way of doing it, we can always ignore it at the caller 
>>>>> side, isn't it ?
>>>
>>>
>>>
>>> I just realized while making the change debugfs_create_file_size() is 
>>> void return, so we don't have anything useful to return in 
>>> amdgpu_debugfs_ring_init()
>>>
>>>
>>
>> Ah, it makes better sense now. Probably just a mention in the body of 
>> the message that we are moving from debugfs_create_file() to 
>> debugfs_create_file_size(), will make this change of return type more 
>> logical.
> 
> 
> Yes, I have that "Use debugfs_create_file_size API for creating ring 
> debugfs file,..."
> 
> 

My bad, I was too focused (and a bit confused due to uasge of clean-up) 
around the code change.

Suggestion for message: Use debugfs_create_file_size API for creating 
ring debugfs, and as its a NULL returning API, change the return type 
for amdgpu_debugfs_ring_init API as well.

With (or even without) this change, please feel free to use:

Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>

- Shashank

> Nirmoy
> 
>>
>> - Shashank
>>
>>> Regards,
>>>
>>> Nirmoy
>>>
>>>
>>>>
>>>>
>>>> Yes, we are currently throwing an error msg and ignoring it. I don't 
>>>> have a strong opinion regarding this, I will send a v2 restoring 
>>>> previous behavior.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Nirmoy
>>>>
>>>>
>>>>>
>>>>> - Shashank
>>>>>
>>>>>>>
>>>>>>> - Shashank
>>>>>>>
>>>>>>>
>>>>>>>> -
>>>>>>>> -    i_size_write(ent->d_inode, ring->ring_size + 12);
>>>>>>>> -    ring->ent = ent;
>>>>>>>> +    debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
>>>>>>>> +                 &amdgpu_debugfs_ring_fops,
>>>>>>>> +                 ring->ring_size + 12);
>>>>>>>>   #endif
>>>>>>>> -    return 0;
>>>>>>>>   }
>>>>>>>>     /**
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>>>> index 88d80eb3fea1..c29fbce0a5b4 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring {
>>>>>>>>       bool            has_compute_vm_bug;
>>>>>>>>       bool            no_scheduler;
>>>>>>>>       int            hw_prio;
>>>>>>>> -
>>>>>>>> -#if defined(CONFIG_DEBUG_FS)
>>>>>>>> -    struct dentry *ent;
>>>>>>>> -#endif
>>>>>>>>   };
>>>>>>>>     #define amdgpu_ring_parse_cs(r, p, ib) 
>>>>>>>> ((r)->funcs->parse_cs((p), (ib)))
>>>>>>>> @@ -356,8 +352,6 @@ static inline void 
>>>>>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>>>>>>>     int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>>>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>>                    struct amdgpu_ring *ring);
>>>>>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>>>>>>> -
>>>>>>>>   #endif
>>>>>>>>

Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Das, Nirmoy]

On 9/6/2021 6:45 PM, Sharma, Shashank wrote:
>
>
> On 9/5/2021 5:01 PM, Das, Nirmoy wrote:
>>
>> On 9/5/2021 10:03 AM, Sharma, Shashank wrote:
>>>
>>>
>>> On 9/3/2021 9:44 PM, Das, Nirmoy wrote:
>>>> Hi Shashank,
>>>>
>>>> On 9/3/2021 5:51 PM, Das, Nirmoy wrote:
>>>>>
>>>>> On 9/3/2021 5:26 PM, Sharma, Shashank wrote:
>>>>>>
>>>>>>
>>>>>> On 9/3/2021 1:39 PM, Das, Nirmoy wrote:
>>>>>>>
>>>>>>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote:
>>>>>>>>> Use debugfs_create_file_size API for creating ring debugfs
>>>>>>>>> file, also cleanup surrounding code.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |  4 +---
>>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 16 
>>>>>>>>> +++++-----------
>>>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  8 +-------
>>>>>>>>>   3 files changed, 7 insertions(+), 21 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>>> index 077f9baf74fe..dee56ab19a8f 100644
>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>>>>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct 
>>>>>>>>> amdgpu_device *adev)
>>>>>>>>>           if (!ring)
>>>>>>>>>               continue;
>>>>>>>>>   -        if (amdgpu_debugfs_ring_init(adev, ring)) {
>>>>>>>>> -            DRM_ERROR("Failed to register debugfs file for 
>>>>>>>>> rings !\n");
>>>>>>>>> -        }
>>>>>>>>> +        amdgpu_debugfs_ring_init(adev, ring);
>>>>>>>>>       }
>>>>>>>>>         amdgpu_ras_debugfs_create_all(adev);
>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>>>>> index f40753e1a60d..968521d80514 100644
>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>>>>>>> @@ -415,26 +415,20 @@ static const struct file_operations 
>>>>>>>>> amdgpu_debugfs_ring_fops = {
>>>>>>>>>     #endif
>>>>>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>>>                    struct amdgpu_ring *ring)
>>>>>>>>>   {
>>>>>>>>>   #if defined(CONFIG_DEBUG_FS)
>>>>>>>>>       struct drm_minor *minor = adev_to_drm(adev)->primary;
>>>>>>>>> -    struct dentry *ent, *root = minor->debugfs_root;
>>>>>>>>> +    struct dentry *root = minor->debugfs_root;
>>>>>>>>>       char name[32];
>>>>>>>>>         sprintf(name, "amdgpu_ring_%s", ring->name);
>>>>>>>>>   -    ent = debugfs_create_file(name,
>>>>>>>>> -                  S_IFREG | S_IRUGO, root,
>>>>>>>>> -                  ring, &amdgpu_debugfs_ring_fops);
>>>>>>>>> -    if (IS_ERR(ent))
>>>>>>>>> -        return -ENOMEM;
>>>>>>>>
>>>>>>>> Why are we doing this ? Why to make it void from int ?
>>>>>>>
>>>>>>>
>>>>>>> We tend to ignore debugfs return values as those are not serious 
>>>>>>> errors. This to sync with rest of our
>>>>>>>
>>>>>>> debugfs calls.
>>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> Nirmoy
>>>>>>>
>>>>>>
>>>>>>
>>>>>> I am not suere if completely removing the provision of return 
>>>>>> value is a good way of doing it, we can always ignore it at the 
>>>>>> caller side, isn't it ?
>>>>
>>>>
>>>>
>>>> I just realized while making the change debugfs_create_file_size() 
>>>> is void return, so we don't have anything useful to return in 
>>>> amdgpu_debugfs_ring_init()
>>>>
>>>>
>>>
>>> Ah, it makes better sense now. Probably just a mention in the body 
>>> of the message that we are moving from debugfs_create_file() to 
>>> debugfs_create_file_size(), will make this change of return type 
>>> more logical.
>>
>>
>> Yes, I have that "Use debugfs_create_file_size API for creating ring 
>> debugfs file,..."
>>
>>
>
> My bad, I was too focused (and a bit confused due to uasge of 
> clean-up) around the code change.
>
> Suggestion for message: Use debugfs_create_file_size API for creating 
> ring debugfs, and as its a NULL returning API, change the return type 
> for amdgpu_debugfs_ring_init API as well.


Thanks Shashank, I pushed the change with your suggested commit message.


Nirmoy


>
> With (or even without) this change, please feel free to use:
>
> Reviewed-by: Shashank Sharma <shashank.sharma@amd.com>
>
> - Shashank
>
>> Nirmoy
>>
>>>
>>> - Shashank
>>>
>>>> Regards,
>>>>
>>>> Nirmoy
>>>>
>>>>
>>>>>
>>>>>
>>>>> Yes, we are currently throwing an error msg and ignoring it. I 
>>>>> don't have a strong opinion regarding this, I will send a v2 
>>>>> restoring previous behavior.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Nirmoy
>>>>>
>>>>>
>>>>>>
>>>>>> - Shashank
>>>>>>
>>>>>>>>
>>>>>>>> - Shashank
>>>>>>>>
>>>>>>>>
>>>>>>>>> -
>>>>>>>>> -    i_size_write(ent->d_inode, ring->ring_size + 12);
>>>>>>>>> -    ring->ent = ent;
>>>>>>>>> +    debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, 
>>>>>>>>> ring,
>>>>>>>>> +                 &amdgpu_debugfs_ring_fops,
>>>>>>>>> +                 ring->ring_size + 12);
>>>>>>>>>   #endif
>>>>>>>>> -    return 0;
>>>>>>>>>   }
>>>>>>>>>     /**
>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>>>>> index 88d80eb3fea1..c29fbce0a5b4 100644
>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>>>>>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring {
>>>>>>>>>       bool            has_compute_vm_bug;
>>>>>>>>>       bool            no_scheduler;
>>>>>>>>>       int            hw_prio;
>>>>>>>>> -
>>>>>>>>> -#if defined(CONFIG_DEBUG_FS)
>>>>>>>>> -    struct dentry *ent;
>>>>>>>>> -#endif
>>>>>>>>>   };
>>>>>>>>>     #define amdgpu_ring_parse_cs(r, p, ib) 
>>>>>>>>> ((r)->funcs->parse_cs((p), (ib)))
>>>>>>>>> @@ -356,8 +352,6 @@ static inline void 
>>>>>>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
>>>>>>>>>     int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
>>>>>>>>>   -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>>>>>>>>>                    struct amdgpu_ring *ring);
>>>>>>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
>>>>>>>>> -
>>>>>>>>>   #endif
>>>>>>>>>

Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs

[Sharma, Shashank]

On 9/2/2021 5:14 PM, Nirmoy Das wrote:
> debugfs APIs returns encoded error so use
> IS_ERR for checking return value.
> 
> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 2 +-
>   2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index d256215ab2c7..077f9baf74fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   	struct dentry *ent;
>   	int r, i;
>   
> -
> -
>   	ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>   				  &fops_ib_preempt);
> -	if (!ent) {
> +	if (IS_ERR(ent)) {
>   		DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n");
>   		return -EIO;
>   	}
>   
>   	ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
>   				  &fops_sclk_set);
> -	if (!ent) {
> +	if (IS_ERR(ent)) {
>   		DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
>   		return -EIO;
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index 7b634a1517f9..f40753e1a60d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>   	ent = debugfs_create_file(name,
>   				  S_IFREG | S_IRUGO, root,
>   				  ring, &amdgpu_debugfs_ring_fops);
> -	if (!ent)
> +	if (IS_ERR(ent))
>   		return -ENOMEM;
>   
>   	i_size_write(ent->d_inode, ring->ring_size + 12);
> 

ACK: Shashank Sharma <shashank.sharma@amd.com>

Regards
Shashank

Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs

[Christian König]

Am 02.09.21 um 13:44 schrieb Nirmoy Das:
> debugfs APIs returns encoded error so use
> IS_ERR for checking return value.
>
> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 2 +-
>   2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index d256215ab2c7..077f9baf74fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   	struct dentry *ent;
>   	int r, i;
>   
> -
> -
>   	ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>   				  &fops_ib_preempt);
> -	if (!ent) {
> +	if (IS_ERR(ent)) {
>   		DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n");
>   		return -EIO;
>   	}
>   
>   	ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
>   				  &fops_sclk_set);
> -	if (!ent) {
> +	if (IS_ERR(ent)) {
>   		DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
>   		return -EIO;
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index 7b634a1517f9..f40753e1a60d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
>   	ent = debugfs_create_file(name,
>   				  S_IFREG | S_IRUGO, root,
>   				  ring, &amdgpu_debugfs_ring_fops);
> -	if (!ent)
> +	if (IS_ERR(ent))
>   		return -ENOMEM;
>   
>   	i_size_write(ent->d_inode, ring->ring_size + 12);

Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs

[Das, Nirmoy]

Hi Christian and Shashank,


Please review the v2 : https://patchwork.freedesktop.org/patch/452175/

In v2, I am returning "PTR_ERR(ent)" instead of -EIO which I think makes more sense.

Regards,
Nirmoy

On 9/3/2021 9:53 AM, Christian König wrote:
> Am 02.09.21 um 13:44 schrieb Nirmoy Das:
>> debugfs APIs returns encoded error so use
>> IS_ERR for checking return value.
>>
>> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 2 +-
>>   2 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> index d256215ab2c7..077f9baf74fe 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>> *adev)
>>       struct dentry *ent;
>>       int r, i;
>>   -
>> -
>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>>                     &fops_ib_preempt);
>> -    if (!ent) {
>> +    if (IS_ERR(ent)) {
>>           DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs 
>> file\n");
>>           return -EIO;
>>       }
>>         ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
>>                     &fops_sclk_set);
>> -    if (!ent) {
>> +    if (IS_ERR(ent)) {
>>           DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
>>           return -EIO;
>>       }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> index 7b634a1517f9..f40753e1a60d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct amdgpu_device 
>> *adev,
>>       ent = debugfs_create_file(name,
>>                     S_IFREG | S_IRUGO, root,
>>                     ring, &amdgpu_debugfs_ring_fops);
>> -    if (!ent)
>> +    if (IS_ERR(ent))
>>           return -ENOMEM;
>>         i_size_write(ent->d_inode, ring->ring_size + 12);
>

Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs

[Christian König]

Yeah, sounds logical to me as well. Feel free to stick my rb on that as 
well.

Christian.

Am 03.09.21 um 10:13 schrieb Das, Nirmoy:
> Hi Christian and Shashank,
>
>
> Please review the v2 : https://patchwork.freedesktop.org/patch/452175/
>
> In v2, I am returning "PTR_ERR(ent)" instead of -EIO which I think 
> makes more sense.
>
> Regards,
> Nirmoy
>
> On 9/3/2021 9:53 AM, Christian König wrote:
>> Am 02.09.21 um 13:44 schrieb Nirmoy Das:
>>> debugfs APIs returns encoded error so use
>>> IS_ERR for checking return value.
>>>
>>> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++----
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 2 +-
>>>   2 files changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> index d256215ab2c7..077f9baf74fe 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>> @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>> *adev)
>>>       struct dentry *ent;
>>>       int r, i;
>>>   -
>>> -
>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>>>                     &fops_ib_preempt);
>>> -    if (!ent) {
>>> +    if (IS_ERR(ent)) {
>>>           DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs 
>>> file\n");
>>>           return -EIO;
>>>       }
>>>         ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, 
>>> adev,
>>>                     &fops_sclk_set);
>>> -    if (!ent) {
>>> +    if (IS_ERR(ent)) {
>>>           DRM_ERROR("unable to create amdgpu_set_sclk debugsfs 
>>> file\n");
>>>           return -EIO;
>>>       }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> index 7b634a1517f9..f40753e1a60d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct 
>>> amdgpu_device *adev,
>>>       ent = debugfs_create_file(name,
>>>                     S_IFREG | S_IRUGO, root,
>>>                     ring, &amdgpu_debugfs_ring_fops);
>>> -    if (!ent)
>>> +    if (IS_ERR(ent))
>>>           return -ENOMEM;
>>>         i_size_write(ent->d_inode, ring->ring_size + 12);
>>

Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs

[Sharma, Shashank]

On 9/3/2021 1:47 PM, Christian König wrote:
> Yeah, sounds logical to me as well. Feel free to stick my rb on that as 
> well.
> 
> Christian.
> 
> Am 03.09.21 um 10:13 schrieb Das, Nirmoy:
>> Hi Christian and Shashank,
>>
>>
>> Please review the v2 : https://patchwork.freedesktop.org/patch/452175/
>>
>> In v2, I am returning "PTR_ERR(ent)" instead of -EIO which I think 
>> makes more sense.

Agree, LGTM as well,
Reviewed-By: Shashank Sharma <shashank.sharma@amd.com>


Regards
Shashank

>>
>> Regards,
>> Nirmoy
>>
>> On 9/3/2021 9:53 AM, Christian König wrote:
>>> Am 02.09.21 um 13:44 schrieb Nirmoy Das:
>>>> debugfs APIs returns encoded error so use
>>>> IS_ERR for checking return value.
>>>>
>>>> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++----
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 2 +-
>>>>   2 files changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> index d256215ab2c7..077f9baf74fe 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>>> *adev)
>>>>       struct dentry *ent;
>>>>       int r, i;
>>>>   -
>>>> -
>>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>>>>                     &fops_ib_preempt);
>>>> -    if (!ent) {
>>>> +    if (IS_ERR(ent)) {
>>>>           DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs 
>>>> file\n");
>>>>           return -EIO;
>>>>       }
>>>>         ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, 
>>>> adev,
>>>>                     &fops_sclk_set);
>>>> -    if (!ent) {
>>>> +    if (IS_ERR(ent)) {
>>>>           DRM_ERROR("unable to create amdgpu_set_sclk debugsfs 
>>>> file\n");
>>>>           return -EIO;
>>>>       }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>> index 7b634a1517f9..f40753e1a60d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>>> @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct 
>>>> amdgpu_device *adev,
>>>>       ent = debugfs_create_file(name,
>>>>                     S_IFREG | S_IRUGO, root,
>>>>                     ring, &amdgpu_debugfs_ring_fops);
>>>> -    if (!ent)
>>>> +    if (IS_ERR(ent))
>>>>           return -ENOMEM;
>>>>         i_size_write(ent->d_inode, ring->ring_size + 12);
>>>
> 

[PATCH v2 1/2] drm/amdgpu: use IS_ERR for debugfs APIs

[Nirmoy Das]

debugfs APIs returns encoded error so use
IS_ERR for checking return value.

v2: return PTR_ERR(ent)

References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    |  4 ++--
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index d256215ab2c7..60f46a4b0144 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1696,20 +1696,18 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
 	struct dentry *ent;
 	int r, i;

-
-
 	ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
 				  &fops_ib_preempt);
-	if (!ent) {
+	if (IS_ERR(ent)) {
 		DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n");
-		return -EIO;
+		return PTR_ERR(ent);
 	}

 	ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
 				  &fops_sclk_set);
-	if (!ent) {
+	if (IS_ERR(ent)) {
 		DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
-		return -EIO;
+		return PTR_ERR(ent);
 	}

 	/* Register debugfs entries for amdgpu_ttm */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 7b634a1517f9..0554576d3695 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -428,8 +428,8 @@ int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
 	ent = debugfs_create_file(name,
 				  S_IFREG | S_IRUGO, root,
 				  ring, &amdgpu_debugfs_ring_fops);
-	if (!ent)
-		return -ENOMEM;
+	if (IS_ERR(ent))
+		return PTR_ERR(ent);

 	i_size_write(ent->d_inode, ring->ring_size + 12);
 	ring->ent = ent;
--
2.32.0

[PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings

[Nirmoy Das]

Use debugfs_create_file_size API for creating ring debugfs
file, also cleanup surrounding code.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 60f46a4b0144..97d88f3e1c4c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
 		if (!ring)
 			continue;
 
-		if (amdgpu_debugfs_ring_init(adev, ring)) {
-			DRM_ERROR("Failed to register debugfs file for rings !\n");
-		}
+		amdgpu_debugfs_ring_init(adev, ring);
 	}
 
 	amdgpu_ras_debugfs_create_all(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 0554576d3695..ab2351ba9574 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -415,26 +415,20 @@ static const struct file_operations amdgpu_debugfs_ring_fops = {
 
 #endif
 
-int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
-			     struct amdgpu_ring *ring)
+void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
+			      struct amdgpu_ring *ring)
 {
 #if defined(CONFIG_DEBUG_FS)
 	struct drm_minor *minor = adev_to_drm(adev)->primary;
-	struct dentry *ent, *root = minor->debugfs_root;
+	struct dentry *root = minor->debugfs_root;
 	char name[32];
 
 	sprintf(name, "amdgpu_ring_%s", ring->name);
+	debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring,
+				 &amdgpu_debugfs_ring_fops,
+				 ring->ring_size + 12);
 
-	ent = debugfs_create_file(name,
-				  S_IFREG | S_IRUGO, root,
-				  ring, &amdgpu_debugfs_ring_fops);
-	if (IS_ERR(ent))
-		return PTR_ERR(ent);
-
-	i_size_write(ent->d_inode, ring->ring_size + 12);
-	ring->ent = ent;
 #endif
-	return 0;
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 88d80eb3fea1..4d380e79752c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -253,10 +253,6 @@ struct amdgpu_ring {
 	bool			has_compute_vm_bug;
 	bool			no_scheduler;
 	int			hw_prio;
-
-#if defined(CONFIG_DEBUG_FS)
-	struct dentry *ent;
-#endif
 };
 
 #define amdgpu_ring_parse_cs(r, p, ib) ((r)->funcs->parse_cs((p), (ib)))
@@ -356,8 +352,6 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring,
 
 int amdgpu_ring_test_helper(struct amdgpu_ring *ring);
 
-int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
-			     struct amdgpu_ring *ring);
-void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring);
-
+void amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
+			      struct amdgpu_ring *ring);
 #endif
-- 
2.32.0