[PATCH 2/5] drm/amdgpu: Use the dynamic IP based offset for register access for SOC15
Christian König
ckoenig.leichtzumerken at gmail.com
Thu Nov 30 08:33:16 UTC 2017
> The reason to use define here is the original function didn't take the adev as parameter.
Yeah, I've seen that and this is exactly the reason why I want to avoid
using a macro here :)
If you don't want to change every caller then at least make the macro
name capital, so that we can see in the code that this isn't a function.
Regards,
Christian.
Am 29.11.2017 um 21:38 schrieb Liu, Shaoyun:
> Thanks for the review . The reason to use define here is the original function didn't take the adev as parameter . If I change it here , all the caller of this function need to be changed . If you think that's proper way I can change it. But do you think it's better to put it in another change so this patch will looks much more cleaner .
>
> Regards
> Shaoyun.liu
>
> -----Original Message-----
> From: Christian König [mailto:ckoenig.leichtzumerken at gmail.com]
> Sent: Wednesday, November 29, 2017 3:05 PM
> To: Liu, Shaoyun; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 2/5] drm/amdgpu: Use the dynamic IP based offset for register access for SOC15
>
> Am 29.11.2017 um 20:09 schrieb Shaoyun Liu:
>> Change-Id: I29f33ee3b4bbd6737f3426385a9e8452fb528a67
>> Signed-off-by: Shaoyun Liu <Shaoyun.Liu at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 21 +++----------------
>> drivers/gpu/drm/amd/amdgpu/soc15_common.h | 34 ++++++++-----------------------
>> 2 files changed, 11 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> index 4c55f21..e324c66 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>> @@ -107,24 +107,9 @@
>> SOC15_REG_OFFSET(SDMA0, 0, mmSDMA0_GB_ADDR_CONFIG_READ), 0x0018773f, 0x00000002
>> };
>>
>> -static u32 sdma_v4_0_get_reg_offset(u32 instance, u32
>> internal_offset) -{
>> - u32 base = 0;
>> -
>> - switch (instance) {
>> - case 0:
>> - base = SDMA0_BASE.instance[0].segment[0];
>> - break;
>> - case 1:
>> - base = SDMA1_BASE.instance[0].segment[0];
>> - break;
>> - default:
>> - BUG();
>> - break;
>> - }
>> -
>> - return base + internal_offset;
>> -}
>> +#define sdma_v4_0_get_reg_offset(inst, offset) ( 0 == inst ? \
>> + (adev->reg_offset[SDMA0_HWIP][0][0] + offset) : \
>> + (adev->reg_offset[SDMA1_HWIP][0][0] + offset))
> Please keep that a function, not a define.
>
> Apart from that looks really good to me.
>
> Christian.
>
>>
>> static void sdma_v4_0_init_golden_registers(struct amdgpu_device *adev)
>> {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15_common.h
>> b/drivers/gpu/drm/amd/amdgpu/soc15_common.h
>> index 7a8e4e28..62a6e21 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/soc15_common.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/soc15_common.h
>> @@ -54,42 +54,24 @@ struct nbio_pcie_index_data {
>>
>> (ip##_BASE__INST##inst##_SEG4 + reg)))))
>>
>> #define WREG32_FIELD15(ip, idx, reg, field, val) \
>> - WREG32(SOC15_REG_OFFSET(ip, idx, mm##reg), (RREG32(SOC15_REG_OFFSET(ip, idx, mm##reg)) & ~REG_FIELD_MASK(reg, field)) | (val) << REG_FIELD_SHIFT(reg, field))
>> + WREG32(adev->reg_offset[ip##_HWIP][idx][mm##reg##_BASE_IDX] + mm##reg, \
>> + (RREG32(adev->reg_offset[ip##_HWIP][idx][mm##reg##_BASE_IDX] + mm##reg) \
>> + & ~REG_FIELD_MASK(reg, field)) | (val) << REG_FIELD_SHIFT(reg,
>> +field))
>>
>> #define RREG32_SOC15(ip, inst, reg) \
>> - RREG32( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
>> - (1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
>> - (2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
>> - (3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
>> - (ip##_BASE__INST##inst##_SEG4 + reg))))))
>> + RREG32(adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] + reg)
>>
>> #define RREG32_SOC15_OFFSET(ip, inst, reg, offset) \
>> - RREG32( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
>> - (1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
>> - (2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
>> - (3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
>> - (ip##_BASE__INST##inst##_SEG4 + reg))))) + offset)
>> + RREG32((adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] + reg) +
>> +offset)
>>
>> #define WREG32_SOC15(ip, inst, reg, value) \
>> - WREG32( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
>> - (1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
>> - (2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
>> - (3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
>> - (ip##_BASE__INST##inst##_SEG4 + reg))))), value)
>> + WREG32((adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] + reg),
>> +value)
>>
>> #define WREG32_SOC15_NO_KIQ(ip, inst, reg, value) \
>> - WREG32_NO_KIQ( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
>> - (1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
>> - (2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
>> - (3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
>> - (ip##_BASE__INST##inst##_SEG4 + reg))))), value)
>> + WREG32_NO_KIQ((adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] +
>> +reg), value)
>>
>> #define WREG32_SOC15_OFFSET(ip, inst, reg, offset, value) \
>> - WREG32( (0 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG0 + reg : \
>> - (1 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG1 + reg : \
>> - (2 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG2 + reg : \
>> - (3 == reg##_BASE_IDX ? ip##_BASE__INST##inst##_SEG3 + reg : \
>> - (ip##_BASE__INST##inst##_SEG4 + reg))))) + offset, value)
>> + WREG32((adev->reg_offset[ip##_HWIP][inst][reg##_BASE_IDX] + reg) +
>> +offset, value)
>>
>> #endif
>>
> _______________________________________________
> 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