* [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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ messages in thread
* [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings
2021-09-02 13:43 [PATCH v2 " Nirmoy Das
@ 2021-09-02 13:43 ` Nirmoy Das
0 siblings, 0 replies; 17+ messages in thread
From: Nirmoy Das @ 2021-09-02 13:43 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 | 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-09-08 8:54 UTC | newest]
Thread overview: 17+ 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
2021-09-02 13:43 [PATCH v2 " Nirmoy Das
2021-09-02 13:43 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das
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.