On 5/22/2024 7:49 AM, Zhang, Jesse(Jie) wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Lijo
>
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Tuesday, May 21, 2024 4:20 PM
> To: Zhang, Jesse(Jie) <Jesse.Zhang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx>; Huang, Tim <Tim.Huang@xxxxxxx>; Yu, Lang <Lang.Yu@xxxxxxx>
> Subject: Re: [PATCH 1/4 V2] drm/amdgpu: fix invadate operation for umsch
>
>
>
> On 5/21/2024 12:46 PM, Jesse Zhang wrote:
>> Since the type of data_size is uint32_t, adev->umsch_mm.data_size - 1
>>>> 16 >> 16 is 0 regardless of the values of its operands
>>
>> So removing the operations upper_32_bits and lower_32_bits.
>>
>> Signed-off-by: Jesse Zhang <Jesse.Zhang@xxxxxxx>
>> Suggested-by: Tim Huang <Tim.Huang@xxxxxxx>
>> ---
>> drivers/gpu/drm/amd/amdgpu/umsch_mm_v4_0.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/umsch_mm_v4_0.c
>> b/drivers/gpu/drm/amd/amdgpu/umsch_mm_v4_0.c
>> index 2c5e7b0a73f9..ce3bb12e3572 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/umsch_mm_v4_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/umsch_mm_v4_0.c
>> @@ -116,9 +116,8 @@ static int umsch_mm_v4_0_load_microcode(struct amdgpu_umsch_mm *umsch)
>> upper_32_bits(adev->umsch_mm.data_start_addr));
>>
>> WREG32_SOC15_UMSCH(regVCN_MES_LOCAL_MASK0_LO,
>> - lower_32_bits(adev->umsch_mm.data_size - 1));
>> - WREG32_SOC15_UMSCH(regVCN_MES_LOCAL_MASK0_HI,
>> - upper_32_bits(adev->umsch_mm.data_size - 1));
>> + adev->umsch_mm.data_size - 1);
>> + WREG32_SOC15_UMSCH(regVCN_MES_LOCAL_MASK0_HI, 0);
>
> cc: Lang
>
> The original programming and the new one doesn't look correct.
>
> I see the below field definitions as per the header. As per this, both LO/HI are 16-bit fields.
>
> vcn/vcn_4_0_5_sh_mask.h:#define VCN_MES_LOCAL_MASK0_HI__MASK0_HI__SHIFT
> 0x0 vcn/vcn_4_0_5_sh_mask.h:#define VCN_MES_LOCAL_MASK0_HI__MASK0_HI_MASK
> 0x0000FFFFL
>
> vcn/vcn_4_0_5_sh_mask.h:#define VCN_MES_LOCAL_MASK0_LO__MASK0_LO__SHIFT
> 0x10 vcn/vcn_4_0_5_sh_mask.h:#define VCN_MES_LOCAL_MASK0_LO__MASK0_LO_MASK
> 0xFFFF0000L
>
> [Zhang, Jesse(Jie)]
>
> The code seem to aligin with Windows side that have same issue. Here is the windows umsch_4_0 write register regVCN_MES_LOCAL_MASK0_LO/regVCN_MES_LOCAL_MASK0_HI
>
> enum umsch_mm_status umsch_mm_engine_init_unsecure_4_0(struct umsch_mm_context* umsch_mm_ip) {
> ...
> temp_data = (uint32_t)umsch_mm_ip->umsch_mm_fw.ucode_info[fw]->data_system_size - 1;
> data = temp_data;
> umsch_mm_cgs_write_register(umsch_mm_ip, umsch_mm_reg_offset(hwip_info, regVCN_MES_LOCAL_MASK0_LO, regVCN_MES_LOCAL_MASK0_LO_BASE_IDX), data, HWBLOCK_VCN);
>
> data = temp_data >> 32;
> umsch_mm_cgs_write_register(umsch_mm_ip, umsch_mm_reg_offset(hwip_info, regVCN_MES_LOCAL_MASK0_HI, regVCN_MES_LOCAL_MASK0_HI_BASE_IDX), data, HWBLOCK_VCN);
> ...
> }
>
> struct umsch_mm_ucode_consts
> {
> ...
> uint32_t data_system_size;
> ...
> }
>
Thanks, checked the MES spec. Looks like the mask field definitions are
wrong. They look like copies of BASE_HI/LO fields which are used for
keeping a 64k aligned 48-bit address.
Anyway, the mask fields are for indicating size of the local heap/stack,
so most likely won't require usage of MASK0_HI.
Thanks,
Lijo
> Thanks
> Jesse
>
>
> Thanks,
> Lijo
>
>>
>> data = adev->firmware.load_type == AMDGPU_FW_LOAD_PSP ?
>> 0 : adev->umsch_mm.data_fw_gpu_addr;
