<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> 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 <div class="moz-cite-prefix">On 5/16/2025 3:07 AM, Christian König wrote: </div> <blockquote type="cite" cite="mid:b52390dc-05a3-463b-9bee-001c6cafb303@amd.com"> <pre wrap="" class="moz-quote-pre">On 5/15/25 18:41, David (Ming Qiang) Wu wrote: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">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 <a class="moz-txt-link-rfc2396E" href="mailto:David.Wu3@amd.com"><David.Wu3@amd.com></a> --- 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); </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> 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. </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre"> 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; } </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> </pre> </blockquote> </body> </html>