* [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()
@ 2020-01-07 21:22 Yong Zhao
2020-01-07 23:01 ` Felix Kuehling
0 siblings, 1 reply; 7+ messages in thread
From: Yong Zhao @ 2020-01-07 21:22 UTC (permalink / raw)
To: amd-gfx; +Cc: Yong Zhao
The SOC15_REG_OFFSET() macro needs to dereference adev->reg_offset[IP]
pointer, which is NULL when there are fewer than 8 sdma engines. Avoid
that by not initializing the array regardless.
Change-Id: Iabae9bff7546b344720905d5d4a5cfc066a79d25
Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
---
.../drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 65 ++++++++++++-------
1 file changed, 43 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
index 3c119407dc34..2b26925623eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
@@ -71,32 +71,53 @@ static uint32_t get_sdma_rlc_reg_offset(struct amdgpu_device *adev,
unsigned int engine_id,
unsigned int queue_id)
{
- uint32_t sdma_engine_reg_base[8] = {
- SOC15_REG_OFFSET(SDMA0, 0,
- mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA1, 0,
- mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA2, 0,
- mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA3, 0,
- mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA4, 0,
- mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA5, 0,
- mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA6, 0,
- mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA7, 0,
- mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL
- };
-
- uint32_t retval = sdma_engine_reg_base[engine_id]
+ uint32_t sdma_engine_reg_base;
+ uint32_t sdma_rlc_reg_offset;
+
+ switch (engine_id) {
+ case 0:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA0, 0,
+ mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL;
+ break;
+ case 1:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA1, 0,
+ mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL;
+ break;
+ case 2:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA2, 0,
+ mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL;
+ break;
+ case 3:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA3, 0,
+ mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL;
+ break;
+ case 4:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA4, 0,
+ mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL;
+ break;
+ case 5:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA5, 0,
+ mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL;
+ break;
+ case 6:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA6, 0,
+ mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL;
+ break;
+ case 7:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA7, 0,
+ mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL;
+ break;
+ default:
+ break;
+ }
+
+ sdma_rlc_reg_offset = sdma_engine_reg_base
+ queue_id * (mmSDMA0_RLC1_RB_CNTL - mmSDMA0_RLC0_RB_CNTL);
pr_debug("RLC register offset for SDMA%d RLC%d: 0x%x\n", engine_id,
- queue_id, retval);
+ queue_id, sdma_rlc_reg_offset);
- return retval;
+ return sdma_rlc_reg_offset;
}
static int kgd_hqd_sdma_load(struct kgd_dev *kgd, void *mqd,
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()
2020-01-07 21:22 [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset() Yong Zhao
@ 2020-01-07 23:01 ` Felix Kuehling
0 siblings, 0 replies; 7+ messages in thread
From: Felix Kuehling @ 2020-01-07 23:01 UTC (permalink / raw)
To: Yong Zhao, amd-gfx
On 2020-01-07 16:22, Yong Zhao wrote:
> The SOC15_REG_OFFSET() macro needs to dereference adev->reg_offset[IP]
> pointer, which is NULL when there are fewer than 8 sdma engines. Avoid
> that by not initializing the array regardless.
>
> Change-Id: Iabae9bff7546b344720905d5d4a5cfc066a79d25
> Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
> .../drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 65 ++++++++++++-------
> 1 file changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> index 3c119407dc34..2b26925623eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> @@ -71,32 +71,53 @@ static uint32_t get_sdma_rlc_reg_offset(struct amdgpu_device *adev,
> unsigned int engine_id,
> unsigned int queue_id)
> {
> - uint32_t sdma_engine_reg_base[8] = {
> - SOC15_REG_OFFSET(SDMA0, 0,
> - mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA1, 0,
> - mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA2, 0,
> - mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA3, 0,
> - mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA4, 0,
> - mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA5, 0,
> - mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA6, 0,
> - mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA7, 0,
> - mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL
> - };
> -
> - uint32_t retval = sdma_engine_reg_base[engine_id]
> + uint32_t sdma_engine_reg_base;
> + uint32_t sdma_rlc_reg_offset;
> +
> + switch (engine_id) {
> + case 0:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA0, 0,
> + mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL;
> + break;
> + case 1:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA1, 0,
> + mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL;
> + break;
> + case 2:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA2, 0,
> + mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL;
> + break;
> + case 3:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA3, 0,
> + mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL;
> + break;
> + case 4:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA4, 0,
> + mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL;
> + break;
> + case 5:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA5, 0,
> + mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL;
> + break;
> + case 6:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA6, 0,
> + mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL;
> + break;
> + case 7:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA7, 0,
> + mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL;
> + break;
> + default:
> + break;
> + }
> +
> + sdma_rlc_reg_offset = sdma_engine_reg_base
> + queue_id * (mmSDMA0_RLC1_RB_CNTL - mmSDMA0_RLC0_RB_CNTL);
>
> pr_debug("RLC register offset for SDMA%d RLC%d: 0x%x\n", engine_id,
> - queue_id, retval);
> + queue_id, sdma_rlc_reg_offset);
>
> - return retval;
> + return sdma_rlc_reg_offset;
> }
>
> static int kgd_hqd_sdma_load(struct kgd_dev *kgd, void *mqd,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()
@ 2019-12-13 16:38 Yong Zhao
2019-12-16 19:39 ` Felix Kuehling
0 siblings, 1 reply; 7+ messages in thread
From: Yong Zhao @ 2019-12-13 16:38 UTC (permalink / raw)
To: amd-gfx; +Cc: Yong Zhao
This prevents the NULL pointer access when there are fewer than 8 sdma
engines.
Change-Id: Iabae9bff7546b344720905d5d4a5cfc066a79d25
Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
---
.../drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 64 ++++++++++++-------
1 file changed, 42 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
index 3c119407dc34..2ad088f10493 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
@@ -71,32 +71,52 @@ static uint32_t get_sdma_rlc_reg_offset(struct amdgpu_device *adev,
unsigned int engine_id,
unsigned int queue_id)
{
- uint32_t sdma_engine_reg_base[8] = {
- SOC15_REG_OFFSET(SDMA0, 0,
- mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA1, 0,
- mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA2, 0,
- mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA3, 0,
- mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA4, 0,
- mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA5, 0,
- mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA6, 0,
- mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL,
- SOC15_REG_OFFSET(SDMA7, 0,
- mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL
- };
-
- uint32_t retval = sdma_engine_reg_base[engine_id]
+ uint32_t sdma_engine_reg_base;
+ uint32_t sdma_rlc_reg_offset;
+
+ switch (engine_id) {
+ case 0:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA0, 0,
+ mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL;
+ break;
+ case 1:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA1, 0,
+ mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL;
+ break;
+ case 2:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA2, 0,
+ mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL;
+ break;
+ case 3:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA3, 0,
+ mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL;
+ break;
+ case 4:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA4, 0,
+ mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL;
+ break;
+ case 5:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA5, 0,
+ mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL;
+ break;
+ case 6:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA6, 0,
+ mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL;
+ break;
+ case 7:
+ sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA7, 0,
+ mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL;
+ break;
+
+ }
+
+ sdma_rlc_reg_offset = sdma_engine_reg_base
+ queue_id * (mmSDMA0_RLC1_RB_CNTL - mmSDMA0_RLC0_RB_CNTL);
pr_debug("RLC register offset for SDMA%d RLC%d: 0x%x\n", engine_id,
- queue_id, retval);
+ queue_id, sdma_rlc_reg_offset);
- return retval;
+ return sdma_rlc_reg_offset;
}
static int kgd_hqd_sdma_load(struct kgd_dev *kgd, void *mqd,
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()
2019-12-13 16:38 Yong Zhao
@ 2019-12-16 19:39 ` Felix Kuehling
2019-12-16 20:06 ` Zhao, Yong
0 siblings, 1 reply; 7+ messages in thread
From: Felix Kuehling @ 2019-12-16 19:39 UTC (permalink / raw)
To: Yong Zhao, amd-gfx
On 2019-12-13 8:38, Yong Zhao wrote:
> This prevents the NULL pointer access when there are fewer than 8 sdma
> engines.
I don't see where you got a NULL pointer in the old code. Also this
change is in an Arcturus-specific source file. AFAIK Arcturus always has
8 SDMA engines.
The new code is much longer than the old code. I don't see how that's an
improvement. See one more comment inline.
>
> Change-Id: Iabae9bff7546b344720905d5d4a5cfc066a79d25
> Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
> ---
> .../drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 64 ++++++++++++-------
> 1 file changed, 42 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> index 3c119407dc34..2ad088f10493 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> @@ -71,32 +71,52 @@ static uint32_t get_sdma_rlc_reg_offset(struct amdgpu_device *adev,
> unsigned int engine_id,
> unsigned int queue_id)
> {
> - uint32_t sdma_engine_reg_base[8] = {
> - SOC15_REG_OFFSET(SDMA0, 0,
> - mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA1, 0,
> - mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA2, 0,
> - mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA3, 0,
> - mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA4, 0,
> - mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA5, 0,
> - mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA6, 0,
> - mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA7, 0,
> - mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL
> - };
> -
> - uint32_t retval = sdma_engine_reg_base[engine_id]
I'm not sure where you were getting a NULL pointer, but I guess this
could have used a range check to make sure engine_id is < 8 before
indexing into the array. The equivalent in the switch statement would be
a default case. See below.
> + uint32_t sdma_engine_reg_base;
> + uint32_t sdma_rlc_reg_offset;
> +
> + switch (engine_id) {
> + case 0:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA0, 0,
> + mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL;
> + break;
> + case 1:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA1, 0,
> + mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL;
> + break;
> + case 2:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA2, 0,
> + mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL;
> + break;
> + case 3:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA3, 0,
> + mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL;
> + break;
> + case 4:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA4, 0,
> + mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL;
> + break;
> + case 5:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA5, 0,
> + mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL;
> + break;
> + case 6:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA6, 0,
> + mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL;
> + break;
> + case 7:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA7, 0,
> + mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL;
> + break;
> +
Do you need a default case for the switch statement? I think you get a
compiler warning without one.
Regards,
Felix
> + }
> +
> + sdma_rlc_reg_offset = sdma_engine_reg_base
> + queue_id * (mmSDMA0_RLC1_RB_CNTL - mmSDMA0_RLC0_RB_CNTL);
>
> pr_debug("RLC register offset for SDMA%d RLC%d: 0x%x\n", engine_id,
> - queue_id, retval);
> + queue_id, sdma_rlc_reg_offset);
>
> - return retval;
> + return sdma_rlc_reg_offset;
> }
>
> static int kgd_hqd_sdma_load(struct kgd_dev *kgd, void *mqd,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()
2019-12-16 19:39 ` Felix Kuehling
@ 2019-12-16 20:06 ` Zhao, Yong
2019-12-16 20:13 ` Felix Kuehling
0 siblings, 1 reply; 7+ messages in thread
From: Zhao, Yong @ 2019-12-16 20:06 UTC (permalink / raw)
To: Kuehling, Felix, amd-gfx
[-- Attachment #1.1: Type: text/plain, Size: 5353 bytes --]
[AMD Official Use Only - Internal Distribution Only]
The problem happens when we want to reuse the same function for ASICs which have fewer SDMA engines. Some pointers on which SOC15_REG_OFFSET depends for some higher index SDMA engines are 0, causing NULL pointer.
I will fix the default case in switch.
Yong
________________________________
From: Kuehling, Felix <Felix.Kuehling@amd.com>
Sent: Monday, December 16, 2019 2:39 PM
To: Zhao, Yong <Yong.Zhao@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()
On 2019-12-13 8:38, Yong Zhao wrote:
> This prevents the NULL pointer access when there are fewer than 8 sdma
> engines.
I don't see where you got a NULL pointer in the old code. Also this
change is in an Arcturus-specific source file. AFAIK Arcturus always has
8 SDMA engines.
The new code is much longer than the old code. I don't see how that's an
improvement. See one more comment inline.
>
> Change-Id: Iabae9bff7546b344720905d5d4a5cfc066a79d25
> Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
> ---
> .../drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 64 ++++++++++++-------
> 1 file changed, 42 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> index 3c119407dc34..2ad088f10493 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> @@ -71,32 +71,52 @@ static uint32_t get_sdma_rlc_reg_offset(struct amdgpu_device *adev,
> unsigned int engine_id,
> unsigned int queue_id)
> {
> - uint32_t sdma_engine_reg_base[8] = {
> - SOC15_REG_OFFSET(SDMA0, 0,
> - mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA1, 0,
> - mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA2, 0,
> - mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA3, 0,
> - mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA4, 0,
> - mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA5, 0,
> - mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA6, 0,
> - mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL,
> - SOC15_REG_OFFSET(SDMA7, 0,
> - mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL
> - };
> -
> - uint32_t retval = sdma_engine_reg_base[engine_id]
I'm not sure where you were getting a NULL pointer, but I guess this
could have used a range check to make sure engine_id is < 8 before
indexing into the array. The equivalent in the switch statement would be
a default case. See below.
> + uint32_t sdma_engine_reg_base;
> + uint32_t sdma_rlc_reg_offset;
> +
> + switch (engine_id) {
> + case 0:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA0, 0,
> + mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL;
> + break;
> + case 1:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA1, 0,
> + mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL;
> + break;
> + case 2:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA2, 0,
> + mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL;
> + break;
> + case 3:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA3, 0,
> + mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL;
> + break;
> + case 4:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA4, 0,
> + mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL;
> + break;
> + case 5:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA5, 0,
> + mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL;
> + break;
> + case 6:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA6, 0,
> + mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL;
> + break;
> + case 7:
> + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA7, 0,
> + mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL;
> + break;
> +
Do you need a default case for the switch statement? I think you get a
compiler warning without one.
Regards,
Felix
> + }
> +
> + sdma_rlc_reg_offset = sdma_engine_reg_base
> + queue_id * (mmSDMA0_RLC1_RB_CNTL - mmSDMA0_RLC0_RB_CNTL);
>
> pr_debug("RLC register offset for SDMA%d RLC%d: 0x%x\n", engine_id,
> - queue_id, retval);
> + queue_id, sdma_rlc_reg_offset);
>
> - return retval;
> + return sdma_rlc_reg_offset;
> }
>
> static int kgd_hqd_sdma_load(struct kgd_dev *kgd, void *mqd,
[-- Attachment #1.2: Type: text/html, Size: 14002 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()
2019-12-16 20:06 ` Zhao, Yong
@ 2019-12-16 20:13 ` Felix Kuehling
2019-12-16 20:23 ` Christian König
0 siblings, 1 reply; 7+ messages in thread
From: Felix Kuehling @ 2019-12-16 20:13 UTC (permalink / raw)
To: Zhao, Yong, amd-gfx
On 2019-12-16 3:06 p.m., Zhao, Yong wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
> The problem happens when we want to reuse the same function for ASICs
> which have fewer SDMA engines. Some pointers on which SOC15_REG_OFFSET
> depends for some higher index SDMA engines are 0, causing NULL pointer.
The only way to do that would be to copy the code into another source
file that includes different register headers. At that time you can
update the code to support fewer SDMA engines in the new source file.
There is no need to change this Arturus-specific source file.
Regards,
Felix
>
> I will fix the default case in switch.
>
> Yong
>
> ------------------------------------------------------------------------
> *From:* Kuehling, Felix <Felix.Kuehling@amd.com>
> *Sent:* Monday, December 16, 2019 2:39 PM
> *To:* Zhao, Yong <Yong.Zhao@amd.com>; amd-gfx@lists.freedesktop.org
> <amd-gfx@lists.freedesktop.org>
> *Subject:* Re: [PATCH] drm/amdkfd: Improve function
> get_sdma_rlc_reg_offset()
> On 2019-12-13 8:38, Yong Zhao wrote:
> > This prevents the NULL pointer access when there are fewer than 8 sdma
> > engines.
>
> I don't see where you got a NULL pointer in the old code. Also this
> change is in an Arcturus-specific source file. AFAIK Arcturus always has
> 8 SDMA engines.
>
> The new code is much longer than the old code. I don't see how that's an
> improvement. See one more comment inline.
>
>
> >
> > Change-Id: Iabae9bff7546b344720905d5d4a5cfc066a79d25
> > Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
> > ---
> > .../drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 64 ++++++++++++-------
> > 1 file changed, 42 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> > index 3c119407dc34..2ad088f10493 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
> > @@ -71,32 +71,52 @@ static uint32_t get_sdma_rlc_reg_offset(struct
> amdgpu_device *adev,
> > unsigned int engine_id,
> > unsigned int queue_id)
> > {
> > - uint32_t sdma_engine_reg_base[8] = {
> > - SOC15_REG_OFFSET(SDMA0, 0,
> > - mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL,
> > - SOC15_REG_OFFSET(SDMA1, 0,
> > - mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL,
> > - SOC15_REG_OFFSET(SDMA2, 0,
> > - mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL,
> > - SOC15_REG_OFFSET(SDMA3, 0,
> > - mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL,
> > - SOC15_REG_OFFSET(SDMA4, 0,
> > - mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL,
> > - SOC15_REG_OFFSET(SDMA5, 0,
> > - mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL,
> > - SOC15_REG_OFFSET(SDMA6, 0,
> > - mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL,
> > - SOC15_REG_OFFSET(SDMA7, 0,
> > - mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL
> > - };
> > -
> > - uint32_t retval = sdma_engine_reg_base[engine_id]
>
> I'm not sure where you were getting a NULL pointer, but I guess this
> could have used a range check to make sure engine_id is < 8 before
> indexing into the array. The equivalent in the switch statement would be
> a default case. See below.
>
>
> > + uint32_t sdma_engine_reg_base;
> > + uint32_t sdma_rlc_reg_offset;
> > +
> > + switch (engine_id) {
> > + case 0:
> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA0, 0,
> > + mmSDMA0_RLC0_RB_CNTL) -
> mmSDMA0_RLC0_RB_CNTL;
> > + break;
> > + case 1:
> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA1, 0,
> > + mmSDMA1_RLC0_RB_CNTL) -
> mmSDMA1_RLC0_RB_CNTL;
> > + break;
> > + case 2:
> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA2, 0,
> > + mmSDMA2_RLC0_RB_CNTL) -
> mmSDMA2_RLC0_RB_CNTL;
> > + break;
> > + case 3:
> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA3, 0,
> > + mmSDMA3_RLC0_RB_CNTL) -
> mmSDMA3_RLC0_RB_CNTL;
> > + break;
> > + case 4:
> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA4, 0,
> > + mmSDMA4_RLC0_RB_CNTL) -
> mmSDMA4_RLC0_RB_CNTL;
> > + break;
> > + case 5:
> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA5, 0,
> > + mmSDMA5_RLC0_RB_CNTL) -
> mmSDMA5_RLC0_RB_CNTL;
> > + break;
> > + case 6:
> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA6, 0,
> > + mmSDMA6_RLC0_RB_CNTL) -
> mmSDMA6_RLC0_RB_CNTL;
> > + break;
> > + case 7:
> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA7, 0,
> > + mmSDMA7_RLC0_RB_CNTL) -
> mmSDMA7_RLC0_RB_CNTL;
> > + break;
> > +
>
> Do you need a default case for the switch statement? I think you get a
> compiler warning without one.
>
> Regards,
> Felix
>
>
> > + }
> > +
> > + sdma_rlc_reg_offset = sdma_engine_reg_base
> > + queue_id * (mmSDMA0_RLC1_RB_CNTL -
> mmSDMA0_RLC0_RB_CNTL);
> >
> > pr_debug("RLC register offset for SDMA%d RLC%d: 0x%x\n",
> engine_id,
> > - queue_id, retval);
> > + queue_id, sdma_rlc_reg_offset);
> >
> > - return retval;
> > + return sdma_rlc_reg_offset;
> > }
> >
> > static int kgd_hqd_sdma_load(struct kgd_dev *kgd, void *mqd,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()
2019-12-16 20:13 ` Felix Kuehling
@ 2019-12-16 20:23 ` Christian König
0 siblings, 0 replies; 7+ messages in thread
From: Christian König @ 2019-12-16 20:23 UTC (permalink / raw)
To: Felix Kuehling, Zhao, Yong, amd-gfx
Am 16.12.19 um 21:13 schrieb Felix Kuehling:
>
> On 2019-12-16 3:06 p.m., Zhao, Yong wrote:
>>
>> [AMD Official Use Only - Internal Distribution Only]
>>
>>
>> The problem happens when we want to reuse the same function for ASICs
>> which have fewer SDMA engines. Some pointers on which
>> SOC15_REG_OFFSET depends for some higher index SDMA engines are 0,
>> causing NULL pointer.
>
> The only way to do that would be to copy the code into another source
> file that includes different register headers. At that time you can
> update the code to support fewer SDMA engines in the new source file.
> There is no need to change this Arturus-specific source file.
A pretty fundamental design decision for amdgpu is that we want to
duplicate the code for each hardware generation even if that means we
end up with a lot of identical/similar code.
The reason for this is that we have so many hardware generation specific
workarounds and we don't want to break older generations when there is a
new issue found on new ones.
Regards,
Christian.
>
>
> Regards,
> Felix
>
>
>>
>> I will fix the default case in switch.
>>
>> Yong
>>
>> ------------------------------------------------------------------------
>> *From:* Kuehling, Felix <Felix.Kuehling@amd.com>
>> *Sent:* Monday, December 16, 2019 2:39 PM
>> *To:* Zhao, Yong <Yong.Zhao@amd.com>; amd-gfx@lists.freedesktop.org
>> <amd-gfx@lists.freedesktop.org>
>> *Subject:* Re: [PATCH] drm/amdkfd: Improve function
>> get_sdma_rlc_reg_offset()
>> On 2019-12-13 8:38, Yong Zhao wrote:
>> > This prevents the NULL pointer access when there are fewer than 8 sdma
>> > engines.
>>
>> I don't see where you got a NULL pointer in the old code. Also this
>> change is in an Arcturus-specific source file. AFAIK Arcturus always has
>> 8 SDMA engines.
>>
>> The new code is much longer than the old code. I don't see how that's an
>> improvement. See one more comment inline.
>>
>>
>> >
>> > Change-Id: Iabae9bff7546b344720905d5d4a5cfc066a79d25
>> > Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
>> > ---
>> > .../drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 64
>> ++++++++++++-------
>> > 1 file changed, 42 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
>> > index 3c119407dc34..2ad088f10493 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c
>> > @@ -71,32 +71,52 @@ static uint32_t get_sdma_rlc_reg_offset(struct
>> amdgpu_device *adev,
>> > unsigned int engine_id,
>> > unsigned int queue_id)
>> > {
>> > - uint32_t sdma_engine_reg_base[8] = {
>> > - SOC15_REG_OFFSET(SDMA0, 0,
>> > - mmSDMA0_RLC0_RB_CNTL) - mmSDMA0_RLC0_RB_CNTL,
>> > - SOC15_REG_OFFSET(SDMA1, 0,
>> > - mmSDMA1_RLC0_RB_CNTL) - mmSDMA1_RLC0_RB_CNTL,
>> > - SOC15_REG_OFFSET(SDMA2, 0,
>> > - mmSDMA2_RLC0_RB_CNTL) - mmSDMA2_RLC0_RB_CNTL,
>> > - SOC15_REG_OFFSET(SDMA3, 0,
>> > - mmSDMA3_RLC0_RB_CNTL) - mmSDMA3_RLC0_RB_CNTL,
>> > - SOC15_REG_OFFSET(SDMA4, 0,
>> > - mmSDMA4_RLC0_RB_CNTL) - mmSDMA4_RLC0_RB_CNTL,
>> > - SOC15_REG_OFFSET(SDMA5, 0,
>> > - mmSDMA5_RLC0_RB_CNTL) - mmSDMA5_RLC0_RB_CNTL,
>> > - SOC15_REG_OFFSET(SDMA6, 0,
>> > - mmSDMA6_RLC0_RB_CNTL) - mmSDMA6_RLC0_RB_CNTL,
>> > - SOC15_REG_OFFSET(SDMA7, 0,
>> > - mmSDMA7_RLC0_RB_CNTL) - mmSDMA7_RLC0_RB_CNTL
>> > - };
>> > -
>> > - uint32_t retval = sdma_engine_reg_base[engine_id]
>>
>> I'm not sure where you were getting a NULL pointer, but I guess this
>> could have used a range check to make sure engine_id is < 8 before
>> indexing into the array. The equivalent in the switch statement would be
>> a default case. See below.
>>
>>
>> > + uint32_t sdma_engine_reg_base;
>> > + uint32_t sdma_rlc_reg_offset;
>> > +
>> > + switch (engine_id) {
>> > + case 0:
>> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA0, 0,
>> > + mmSDMA0_RLC0_RB_CNTL) -
>> mmSDMA0_RLC0_RB_CNTL;
>> > + break;
>> > + case 1:
>> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA1, 0,
>> > + mmSDMA1_RLC0_RB_CNTL) -
>> mmSDMA1_RLC0_RB_CNTL;
>> > + break;
>> > + case 2:
>> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA2, 0,
>> > + mmSDMA2_RLC0_RB_CNTL) -
>> mmSDMA2_RLC0_RB_CNTL;
>> > + break;
>> > + case 3:
>> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA3, 0,
>> > + mmSDMA3_RLC0_RB_CNTL) -
>> mmSDMA3_RLC0_RB_CNTL;
>> > + break;
>> > + case 4:
>> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA4, 0,
>> > + mmSDMA4_RLC0_RB_CNTL) -
>> mmSDMA4_RLC0_RB_CNTL;
>> > + break;
>> > + case 5:
>> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA5, 0,
>> > + mmSDMA5_RLC0_RB_CNTL) -
>> mmSDMA5_RLC0_RB_CNTL;
>> > + break;
>> > + case 6:
>> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA6, 0,
>> > + mmSDMA6_RLC0_RB_CNTL) -
>> mmSDMA6_RLC0_RB_CNTL;
>> > + break;
>> > + case 7:
>> > + sdma_engine_reg_base = SOC15_REG_OFFSET(SDMA7, 0,
>> > + mmSDMA7_RLC0_RB_CNTL) -
>> mmSDMA7_RLC0_RB_CNTL;
>> > + break;
>> > +
>>
>> Do you need a default case for the switch statement? I think you get a
>> compiler warning without one.
>>
>> Regards,
>> Felix
>>
>>
>> > + }
>> > +
>> > + sdma_rlc_reg_offset = sdma_engine_reg_base
>> > + queue_id * (mmSDMA0_RLC1_RB_CNTL -
>> mmSDMA0_RLC0_RB_CNTL);
>> >
>> > pr_debug("RLC register offset for SDMA%d RLC%d: 0x%x\n",
>> engine_id,
>> > - queue_id, retval);
>> > + queue_id, sdma_rlc_reg_offset);
>> >
>> > - return retval;
>> > + return sdma_rlc_reg_offset;
>> > }
>> >
>> > static int kgd_hqd_sdma_load(struct kgd_dev *kgd, void *mqd,
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-07 23:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-07 21:22 [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset() Yong Zhao
2020-01-07 23:01 ` Felix Kuehling
-- strict thread matches above, loose matches on Subject: below --
2019-12-13 16:38 Yong Zhao
2019-12-16 19:39 ` Felix Kuehling
2019-12-16 20:06 ` Zhao, Yong
2019-12-16 20:13 ` Felix Kuehling
2019-12-16 20:23 ` Christian König
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.