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

[Lazar, Lijo]

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.

Thanks,
Lijo

> Thanks
> Jesse
> 
> 
> Thanks,
> Lijo
> 
>>
>>       data = adev->firmware.load_type == AMDGPU_FW_LOAD_PSP ?
>>              0 : adev->umsch_mm.data_fw_gpu_addr;