[PATCH 1/4 V2] drm/amdgpu: fix invadate operation for umsch

[Yu, Lang]

[Public]

>-----Original Message-----
>From: Lazar, Lijo <Lijo.Lazar at amd.com>
>Sent: Wednesday, May 22, 2024 12:57 PM
>To: Zhang, Jesse(Jie) <Jesse.Zhang at amd.com>; amd-
>gfx at lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian
><Christian.Koenig at amd.com>; Huang, Tim <Tim.Huang at amd.com>; Yu, Lang
><Lang.Yu at amd.com>
>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 at amd.com>
>> Sent: Tuesday, May 21, 2024 4:20 PM
>> To: Zhang, Jesse(Jie) <Jesse.Zhang at amd.com>;
>> amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian
>> <Christian.Koenig at amd.com>; Huang, Tim <Tim.Huang at amd.com>; Yu,
>Lang
>> <Lang.Yu at amd.com>
>> 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 at amd.com>
>>> Suggested-by: Tim Huang <Tim.Huang at amd.com>
>>> ---
>>>  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;