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; > } >
