Well testing only falsifies things. I agree that it should server the same purpose, but we don't have a guarantee for that and as far as I can see there is not need to switch to a different register.
So this change seems superfluous if not dangerous to me. Regards, Christian. On 5/16/25 18:23, Wu, David wrote: > Hi Christian, > For this change on VCN5.0.1 I will leave it to Sonny for a test. Since the > readback is for each VCN instance it should work for that clock domain. As > Alex has pointed out that readback post all writes will let the writes hit > hardware, using UVD_STATUS instead of VCN_RB1_DB_CTRL should serve the same > purpose. I also tested it on STRIX VCN4.0.5 and it works ( using UVD_STATUS > instead of VCN_RB1_DB_CTRL ). > > Sonny - Would you be able to test this simple change? > > Thanks, > David > On 5/16/2025 3:07 AM, Christian König wrote: >> 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 <[email protected]> >>> --- >>> 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; >>> } >>> >
