[PATCH] drm/amdkfd: Improve function get_sdma_rlc_reg_offset()
Christian König
ckoenig.leichtzumerken at gmail.com
Mon Dec 16 20:23:54 UTC 2019
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 <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,
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list