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

["Zhang, Jesse(Jie)" <Jesse.Zhang@xxxxxxx>]

[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
Jesse


Thanks,
Lijo

>
>       data = adev->firmware.load_type == AMDGPU_FW_LOAD_PSP ?
>              0 : adev->umsch_mm.data_fw_gpu_addr;