* [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs
@ 2021-09-02 11:44 Nirmoy Das
2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Nirmoy Das @ 2021-09-02 11:44 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian.Koenig, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-02 11:44 [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Nirmoy Das
@ 2021-09-02 11:44 ` Nirmoy Das
2021-09-03 6:36 ` Sharma, Shashank
2021-09-02 15:23 ` [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Sharma, Shashank
2021-09-03 7:53 ` Christian König
2 siblings, 1 reply; 16+ messages in thread
From: Nirmoy Das @ 2021-09-02 11:44 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian.Koenig, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs
2021-09-02 11:44 [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Nirmoy Das
2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das
@ 2021-09-02 15:23 ` Sharma, Shashank
2021-09-03 7:53 ` Christian König
2 siblings, 0 replies; 16+ messages in thread
From: Sharma, Shashank @ 2021-09-02 15:23 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Christian.Koenig
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das
@ 2021-09-03 6:36 ` Sharma, Shashank
2021-09-03 8:09 ` Das, Nirmoy
0 siblings, 1 reply; 16+ messages in thread
From: Sharma, Shashank @ 2021-09-03 6:36 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Christian.Koenig
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs
2021-09-02 11:44 [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Nirmoy Das
2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das
2021-09-02 15:23 ` [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Sharma, Shashank
@ 2021-09-03 7:53 ` Christian König
2021-09-03 8:13 ` Das, Nirmoy
2 siblings, 1 reply; 16+ messages in thread
From: Christian König @ 2021-09-03 7:53 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx
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);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-03 6:36 ` Sharma, Shashank
@ 2021-09-03 8:09 ` Das, Nirmoy
2021-09-03 15:26 ` Sharma, Shashank
0 siblings, 1 reply; 16+ messages in thread
From: Das, Nirmoy @ 2021-09-03 8:09 UTC (permalink / raw)
To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig
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
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs
2021-09-03 7:53 ` Christian König
@ 2021-09-03 8:13 ` Das, Nirmoy
2021-09-03 8:17 ` Christian König
0 siblings, 1 reply; 16+ messages in thread
From: Das, Nirmoy @ 2021-09-03 8:13 UTC (permalink / raw)
To: Christian König, amd-gfx; +Cc: Sharma, Shashank
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);
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs
2021-09-03 8:13 ` Das, Nirmoy
@ 2021-09-03 8:17 ` Christian König
2021-09-03 15:28 ` Sharma, Shashank
0 siblings, 1 reply; 16+ messages in thread
From: Christian König @ 2021-09-03 8:17 UTC (permalink / raw)
To: Das, Nirmoy, amd-gfx; +Cc: Sharma, Shashank
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);
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-03 8:09 ` Das, Nirmoy
@ 2021-09-03 15:26 ` Sharma, Shashank
2021-09-03 15:51 ` Das, Nirmoy
0 siblings, 1 reply; 16+ messages in thread
From: Sharma, Shashank @ 2021-09-03 15:26 UTC (permalink / raw)
To: Das, Nirmoy, amd-gfx; +Cc: Christian.Koenig
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
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs
2021-09-03 8:17 ` Christian König
@ 2021-09-03 15:28 ` Sharma, Shashank
0 siblings, 0 replies; 16+ messages in thread
From: Sharma, Shashank @ 2021-09-03 15:28 UTC (permalink / raw)
To: Christian König, Das, Nirmoy, amd-gfx
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);
>>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-03 15:26 ` Sharma, Shashank
@ 2021-09-03 15:51 ` Das, Nirmoy
2021-09-03 16:14 ` Das, Nirmoy
0 siblings, 1 reply; 16+ messages in thread
From: Das, Nirmoy @ 2021-09-03 15:51 UTC (permalink / raw)
To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig
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
>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-03 15:51 ` Das, Nirmoy
@ 2021-09-03 16:14 ` Das, Nirmoy
2021-09-05 8:03 ` Sharma, Shashank
0 siblings, 1 reply; 16+ messages in thread
From: Das, Nirmoy @ 2021-09-03 16:14 UTC (permalink / raw)
To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig
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
>>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-03 16:14 ` Das, Nirmoy
@ 2021-09-05 8:03 ` Sharma, Shashank
2021-09-05 11:31 ` Das, Nirmoy
0 siblings, 1 reply; 16+ messages in thread
From: Sharma, Shashank @ 2021-09-05 8:03 UTC (permalink / raw)
To: Das, Nirmoy, amd-gfx; +Cc: Christian.Koenig
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
>>>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-05 8:03 ` Sharma, Shashank
@ 2021-09-05 11:31 ` Das, Nirmoy
2021-09-06 16:45 ` Sharma, Shashank
0 siblings, 1 reply; 16+ messages in thread
From: Das, Nirmoy @ 2021-09-05 11:31 UTC (permalink / raw)
To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig
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
>>>>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-05 11:31 ` Das, Nirmoy
@ 2021-09-06 16:45 ` Sharma, Shashank
2021-09-08 8:54 ` Das, Nirmoy
0 siblings, 1 reply; 16+ messages in thread
From: Sharma, Shashank @ 2021-09-06 16:45 UTC (permalink / raw)
To: Das, Nirmoy, amd-gfx; +Cc: Christian.Koenig
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
>>>>>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-06 16:45 ` Sharma, Shashank
@ 2021-09-08 8:54 ` Das, Nirmoy
0 siblings, 0 replies; 16+ messages in thread
From: Das, Nirmoy @ 2021-09-08 8:54 UTC (permalink / raw)
To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig
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
>>>>>>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-09-08 8:54 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 11:44 [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Nirmoy Das
2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das
2021-09-03 6:36 ` Sharma, Shashank
2021-09-03 8:09 ` Das, Nirmoy
2021-09-03 15:26 ` Sharma, Shashank
2021-09-03 15:51 ` Das, Nirmoy
2021-09-03 16:14 ` Das, Nirmoy
2021-09-05 8:03 ` Sharma, Shashank
2021-09-05 11:31 ` Das, Nirmoy
2021-09-06 16:45 ` Sharma, Shashank
2021-09-08 8:54 ` Das, Nirmoy
2021-09-02 15:23 ` [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Sharma, Shashank
2021-09-03 7:53 ` Christian König
2021-09-03 8:13 ` Das, Nirmoy
2021-09-03 8:17 ` Christian König
2021-09-03 15:28 ` Sharma, Shashank
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.