|
[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@xxxxxxx>
Sent: Monday, December 16, 2019 2:39 PM To: Zhao, Yong <Yong.Zhao@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxx> > --- > .../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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
