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

[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 <Felix.Kuehling at amd.com>
Sent: Monday, December 16, 2019 2:39 PM
To: Zhao, Yong <Yong.Zhao at amd.com>; amd-gfx at lists.freedesktop.org <amd-gfx at 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 at 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,
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20191216/605a5b4d/attachment-0001.htm>