Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()

2020-01-07 Thread Felix Kuehling 

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 

Reviewed-by: Felix Kuehling 

---
  .../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

[PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()

2020-01-07 Thread 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 
---
 .../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

Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()

2019-12-16 Thread Christian König 

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 
*Sent:* Monday, December 16, 2019 2:39 PM
*To:* Zhao, Yong ; 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 
> ---
>   .../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;

> + 

Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()

2019-12-16 Thread 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.



Regards,
  Felix




I will fix the default case in switch.

Yong


*From:* Kuehling, Felix 
*Sent:* Monday, December 16, 2019 2:39 PM
*To:* Zhao, Yong ; 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 
> ---
>   .../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

Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()

2019-12-16 Thread Zhao, Yong 
[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 
Sent: Monday, December 16, 2019 2:39 PM
To: Zhao, Yong ; 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 
> ---
>   .../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_

Re: [PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()

2019-12-16 Thread Felix Kuehling 

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 
---
  .../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

[PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()

2019-12-13 Thread Yong Zhao 
This prevents the NULL pointer access when there are fewer than 8 sdma
engines.

Change-Id: Iabae9bff7546b344720905d5d4a5cfc066a79d25
Signed-off-by: Yong Zhao 
---
 .../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