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

[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 <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,