[PATCH v2 1/9] drm/amdgpu: read back register after written

[Christian König]

On 5/15/25 18:40, David (Ming Qiang) Wu wrote:
> V2: use common register UVD_STATUS for readback (standard PCI MMIO
>     behavior, i.e. readback post all writes to let the writes hit
>     the hardware)
>     add read-back in ..._stop() for more coverage.
> 
> Similar to the changes made for VCN v4.0.5 where readback to post the
> writes to avoid race with the doorbell, the addition of register
> readback support in other VCN versions is intended to prevent potential
> race conditions, even though such issues have not been observed yet.
> This change ensures consistency across different VCN variants and helps
> avoid similar issues. The overhead introduced is negligible.
> 
> Signed-off-by: David (Ming Qiang) Wu <David.Wu3 at amd.com>

Exactly for the comment on patch #9 this looks good to me.

With my comment cleared up feel free to add Acked-by: Christian König <christian.koenig at amd.com>

Regards,
Christian.


> ---
>  drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> index 21b57c29bf7d..c74947705d77 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> @@ -1009,6 +1009,11 @@ static int vcn_v1_0_start_spg_mode(struct amdgpu_vcn_inst *vinst)
>  
>  	jpeg_v1_0_start(adev, 0);
>  
> +	/* Keeping one read-back to ensure all register writes are done,
> +	 * otherwise it may introduce race conditions.
> +	 */
> +	RREG32_SOC15(UVD, 0, mmUVD_STATUS);
> +
>  	return 0;
>  }
>  
> @@ -1154,6 +1159,11 @@ static int vcn_v1_0_start_dpg_mode(struct amdgpu_vcn_inst *vinst)
>  
>  	jpeg_v1_0_start(adev, 1);
>  
> +	/* Keeping one read-back to ensure all register writes are done,
> +	 * otherwise it may introduce race conditions.
> +	 */
> +	RREG32_SOC15(UVD, 0, mmUVD_STATUS);
> +
>  	return 0;
>  }
>  
> @@ -1216,6 +1226,12 @@ static int vcn_v1_0_stop_spg_mode(struct amdgpu_vcn_inst *vinst)
>  
>  	vcn_v1_0_enable_clock_gating(vinst);
>  	vcn_1_0_enable_static_power_gating(vinst);
> +
> +	/* Keeping one read-back to ensure all register writes are done,
> +	 * otherwise it may introduce race conditions.
> +	 */
> +	RREG32_SOC15(UVD, 0, mmUVD_STATUS);
> +
>  	return 0;
>  }
>  
> @@ -1250,6 +1266,11 @@ static int vcn_v1_0_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
>  	WREG32_P(SOC15_REG_OFFSET(UVD, 0, mmUVD_POWER_STATUS), 0,
>  			~UVD_POWER_STATUS__UVD_PG_MODE_MASK);
>  
> +	/* Keeping one read-back to ensure all register writes are done,
> +	 * otherwise it may introduce race conditions.
> +	 */
> +	RREG32_SOC15(UVD, 0, mmUVD_STATUS);
> +
>  	return 0;
>  }
>