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

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

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

Thanks,
Lijo

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