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

[Christian König]

On 5/15/25 18:41, David (Ming Qiang) Wu wrote:
> The addition of register read-back in VCN v5.0.1 is intended to prevent
> potential race conditions.
> 
> Signed-off-by: David (Ming Qiang) Wu <David.Wu3 at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> index 60ee6e02e6ac..79d36d48a6b6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> @@ -657,8 +657,11 @@ static int vcn_v5_0_1_start_dpg_mode(struct amdgpu_vcn_inst *vinst,
>  	WREG32_SOC15(VCN, vcn_inst, regVCN_RB1_DB_CTRL,
>  		ring->doorbell_index << VCN_RB1_DB_CTRL__OFFSET__SHIFT |
>  		VCN_RB1_DB_CTRL__EN_MASK);
> -	/* Read DB_CTRL to flush the write DB_CTRL command. */
> -	RREG32_SOC15(VCN, vcn_inst, regVCN_RB1_DB_CTRL);
> +
> +	/* Keeping one read-back to ensure all register writes are done,
> +	 * otherwise it may introduce race conditions.
> +	 */
> +	RREG32_SOC15(VCN, vcn_inst, regUVD_STATUS);

I'm not sure that this is a good idea.

The read back from specific registers was usually to sync up with the clock driving those registers, e.g. the VCN_RB1_DB_CTRL register here.

Could be that for VCN we only have one clock domain, but if you would do this for one of the old PLLs for example I can guarantee that it won't work.

Regards,
Christian.


>  
>  	return 0;
>  }
> @@ -809,6 +812,11 @@ static int vcn_v5_0_1_start(struct amdgpu_vcn_inst *vinst)
>  	WREG32_SOC15(VCN, vcn_inst, regVCN_RB_ENABLE, tmp);
>  	fw_shared->sq.queue_mode &= ~(FW_QUEUE_RING_RESET | FW_QUEUE_DPG_HOLD_OFF);
>  
> +	/* Keeping one read-back to ensure all register writes are done,
> +	 * otherwise it may introduce race conditions.
> +	 */
> +	RREG32_SOC15(VCN, vcn_inst, regUVD_STATUS);
> +
>  	return 0;
>  }
>  
> @@ -843,6 +851,11 @@ static void vcn_v5_0_1_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
>  	/* disable dynamic power gating mode */
>  	WREG32_P(SOC15_REG_OFFSET(VCN, vcn_inst, regUVD_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(VCN, vcn_inst, regUVD_STATUS);
>  }
>  
>  /**
> @@ -918,6 +931,11 @@ static int vcn_v5_0_1_stop(struct amdgpu_vcn_inst *vinst)
>  	/* clear status */
>  	WREG32_SOC15(VCN, vcn_inst, regUVD_STATUS, 0);
>  
> +	/* Keeping one read-back to ensure all register writes are done,
> +	 * otherwise it may introduce race conditions.
> +	 */
> +	RREG32_SOC15(VCN, vcn_inst, regUVD_STATUS);
> +
>  	return 0;
>  }
>