<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> [AMD Official Use Only - Internal Distribution Only] <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> The problem happens when we want to reuse the same function for ASICs which have fewer SDMA engines. Some pointers on which <span style="font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; font-size: 14.6667px; background-color: rgb(255, 255, 255); display: inline !important">SOC15_REG_OFFSET depends for some higher index SDMA engines are 0, causing NULL pointer. </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; font-size: 14.6667px; background-color: rgb(255, 255, 255); display: inline !important"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; font-size: 14.6667px; background-color: rgb(255, 255, 255); display: inline !important">I will fix the default case in switch.</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; font-size: 14.6667px; background-color: rgb(255, 255, 255); display: inline !important"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; font-size: 14.6667px; background-color: rgb(255, 255, 255); display: inline !important">Yong</div> <div id="appendonsend"></div> <div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)"> </div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">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() <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">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, </div> </div> </div> </body> </html>