[Public]
>-----Original Message-----
>From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
>Sent: Wednesday, May 22, 2024 12:57 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/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.
Yes, the programing is aligned with windows side.
There is a typo " invadate " in the patch title.
Regards,
Lang
>Thanks,
>Lijo
>
>> Thanks
>> Jesse
>>
>>
>> Thanks,
>> Lijo
>>
>>>
>>> data = adev->firmware.load_type == AMDGPU_FW_LOAD_PSP ?
>>> 0 : adev->umsch_mm.data_fw_gpu_addr;
