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

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

[David (Ming Qiang) Wu]

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@amd.com>
---
 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;
 }
 
-- 
2.49.0

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

[David (Ming Qiang) Wu]

The addition of register read-back in VCN v2.0 is intended to prevent
potential race conditions.

Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
index b8d835c9e17e..148b651be7ca 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
@@ -978,6 +978,12 @@ static int vcn_v2_0_start_dpg_mode(struct amdgpu_vcn_inst *vinst, bool indirect)
 	/* Unstall DPG */
 	WREG32_P(SOC15_REG_OFFSET(UVD, 0, mmUVD_POWER_STATUS),
 		0, ~UVD_POWER_STATUS__STALL_DPG_POWER_UP_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;
 }
 
@@ -1152,6 +1158,11 @@ static int vcn_v2_0_start(struct amdgpu_vcn_inst *vinst)
 	WREG32_SOC15(UVD, 0, mmUVD_RB_SIZE2, ring->ring_size / 4);
 	fw_shared->multi_queue.encode_lowlatency_queue_mode &= ~FW_QUEUE_RING_RESET;
 
+	/* 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;
 }
 
@@ -1183,6 +1194,11 @@ static int vcn_v2_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;
 }
 
@@ -1248,6 +1264,11 @@ static int vcn_v2_0_stop(struct amdgpu_vcn_inst *vinst)
 	vcn_v2_0_enable_clock_gating(vinst);
 	vcn_v2_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(VCN, 0, mmUVD_STATUS);
+
 power_off:
 	if (adev->pm.dpm_enabled)
 		amdgpu_dpm_enable_vcn(adev, false, 0);
-- 
2.49.0

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

[David (Ming Qiang) Wu]

The addition of register read-back in VCN v2.5 is intended to prevent
potential race conditions.

Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
index 3eec1b8feaee..58b527a6b795 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
@@ -1158,6 +1158,11 @@ static int vcn_v2_5_start_dpg_mode(struct amdgpu_vcn_inst *vinst, bool indirect)
 	WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, mmUVD_POWER_STATUS),
 		0, ~UVD_POWER_STATUS__STALL_DPG_POWER_UP_MASK);
 
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, inst_idx, mmUVD_STATUS);
+
 	return 0;
 }
 
@@ -1343,6 +1348,11 @@ static int vcn_v2_5_start(struct amdgpu_vcn_inst *vinst)
 	WREG32_SOC15(VCN, i, mmUVD_RB_SIZE2, ring->ring_size / 4);
 	fw_shared->multi_queue.encode_lowlatency_queue_mode &= ~FW_QUEUE_RING_RESET;
 
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, i, mmUVD_STATUS);
+
 	return 0;
 }
 
@@ -1569,6 +1579,11 @@ static int vcn_v2_5_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
 	WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, 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(VCN, inst_idx, mmUVD_STATUS);
+
 	return 0;
 }
 
@@ -1635,6 +1650,10 @@ static int vcn_v2_5_stop(struct amdgpu_vcn_inst *vinst)
 		 UVD_POWER_STATUS__UVD_POWER_STATUS_MASK,
 		 ~UVD_POWER_STATUS__UVD_POWER_STATUS_MASK);
 
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, i, mmUVD_STATUS);
 done:
 	if (adev->pm.dpm_enabled)
 		amdgpu_dpm_enable_vcn(adev, false, i);
-- 
2.49.0

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

[David (Ming Qiang) Wu]

The addition of register read-back in VCN v3.0 is intended to prevent
potential race conditions.

Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
index 0b19f0ab4480..9fb0d5380589 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
@@ -1173,6 +1173,11 @@ static int vcn_v3_0_start_dpg_mode(struct amdgpu_vcn_inst *vinst, bool indirect)
 	WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, mmUVD_POWER_STATUS),
 		0, ~UVD_POWER_STATUS__STALL_DPG_POWER_UP_MASK);
 
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, inst_idx, mmUVD_STATUS);
+
 	return 0;
 }
 
@@ -1360,6 +1365,11 @@ static int vcn_v3_0_start(struct amdgpu_vcn_inst *vinst)
 		fw_shared->multi_queue.encode_lowlatency_queue_mode &= cpu_to_le32(~FW_QUEUE_RING_RESET);
 	}
 
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, i, mmUVD_STATUS);
+
 	return 0;
 }
 
@@ -1602,6 +1612,11 @@ static int vcn_v3_0_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
 	WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, 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(VCN, inst_idx, mmUVD_STATUS);
+
 	return 0;
 }
 
@@ -1674,6 +1689,11 @@ static int vcn_v3_0_stop(struct amdgpu_vcn_inst *vinst)
 	/* enable VCN power gating */
 	vcn_v3_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(VCN, i, mmUVD_STATUS);
+
 done:
 	if (adev->pm.dpm_enabled)
 		amdgpu_dpm_enable_vcn(adev, false, i);
-- 
2.49.0

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

[David (Ming Qiang) Wu]

The addition of register read-back in VCN v4.0.0 is intended to prevent
potential race conditions.

Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
index 8fff470bce87..5acdf8fd5a62 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
@@ -1122,6 +1122,11 @@ static int vcn_v4_0_start_dpg_mode(struct amdgpu_vcn_inst *vinst, bool indirect)
 			ring->doorbell_index << VCN_RB1_DB_CTRL__OFFSET__SHIFT |
 			VCN_RB1_DB_CTRL__EN_MASK);
 
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, inst_idx, regUVD_RB_WPTR);
+
 	return 0;
 }
 
@@ -1303,6 +1308,11 @@ static int vcn_v4_0_start(struct amdgpu_vcn_inst *vinst)
 	WREG32_SOC15(VCN, i, 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, i, regUVD_RB_WPTR);
+
 	return 0;
 }
 
@@ -1583,6 +1593,11 @@ static void vcn_v4_0_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
 	/* disable dynamic power gating mode */
 	WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, 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, inst_idx, regUVD_STATUS);
 }
 
 /**
@@ -1666,6 +1681,11 @@ static int vcn_v4_0_stop(struct amdgpu_vcn_inst *vinst)
 	/* enable VCN power gating */
 	vcn_v4_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(VCN, i, regUVD_STATUS);
+
 done:
 	if (adev->pm.dpm_enabled)
 		amdgpu_dpm_enable_vcn(adev, false, i);
-- 
2.49.0

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

[David (Ming Qiang) Wu]

The addition of register read-back in VCN v4.0.3 is intended to prevent
potential race conditions.

Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
index 712e1fba33ce..872ab1193fac 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
@@ -970,6 +970,11 @@ static int vcn_v4_0_3_start_dpg_mode(struct amdgpu_vcn_inst *vinst,
 	/*resetting done, fw can check RB ring */
 	fw_shared->sq.queue_mode &= cpu_to_le32(~FW_QUEUE_RING_RESET);
 
+	/* 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;
 }
 
@@ -1363,6 +1368,12 @@ static int vcn_v4_0_3_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);
+
 	return 0;
 }
 
@@ -1446,6 +1457,11 @@ static int vcn_v4_0_3_stop(struct amdgpu_vcn_inst *vinst)
 	/* apply HW clock gating */
 	vcn_v4_0_3_enable_clock_gating(vinst);
 
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, vcn_inst, regUVD_STATUS);
+
 Done:
 	return 0;
 }
-- 
2.49.0

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

[David (Ming Qiang) Wu]

The addition of register read-back in VCN v4.0.5 is intended to prevent
potential race conditions.

Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v4_0_5.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_5.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_5.c
index a09f9a2dd471..46dced751249 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_5.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_5.c
@@ -1034,9 +1034,10 @@ static int vcn_v4_0_5_start_dpg_mode(struct amdgpu_vcn_inst *vinst,
 			ring->doorbell_index << VCN_RB1_DB_CTRL__OFFSET__SHIFT |
 			VCN_RB1_DB_CTRL__EN_MASK);
 
-	/* Keeping one read-back to ensure all register writes are done, otherwise
-	 * it may introduce race conditions */
-	RREG32_SOC15(VCN, inst_idx, regVCN_RB1_DB_CTRL);
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, inst_idx, regUVD_STATUS);
 
 	return 0;
 }
@@ -1220,9 +1221,10 @@ static int vcn_v4_0_5_start(struct amdgpu_vcn_inst *vinst)
 	WREG32_SOC15(VCN, i, 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, i, regVCN_RB_ENABLE);
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, i, regUVD_STATUS);
 
 	return 0;
 }
@@ -1254,6 +1256,11 @@ static void vcn_v4_0_5_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
 	/* disable dynamic power gating mode */
 	WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, 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, inst_idx, regUVD_STATUS);
 }
 
 /**
@@ -1337,6 +1344,11 @@ static int vcn_v4_0_5_stop(struct amdgpu_vcn_inst *vinst)
 	/* enable VCN power gating */
 	vcn_v4_0_5_enable_static_power_gating(vinst);
 
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, i, regUVD_STATUS);
+
 done:
 	if (adev->pm.dpm_enabled)
 		amdgpu_dpm_enable_vcn(adev, false, i);
-- 
2.49.0

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

[David (Ming Qiang) Wu]

The addition of register read-back in VCN v5.0.0 is intended to prevent
potential race conditions.

Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v5_0_0.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_0.c
index 27dcc6f37a73..f8e3f0b882da 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_0.c
@@ -794,6 +794,11 @@ static int vcn_v5_0_0_start_dpg_mode(struct amdgpu_vcn_inst *vinst,
 		ring->doorbell_index << VCN_RB1_DB_CTRL__OFFSET__SHIFT |
 		VCN_RB1_DB_CTRL__EN_MASK);
 
+	/* Keeping one read-back to ensure all register writes are done,
+	 * otherwise it may introduce race conditions.
+	 */
+	RREG32_SOC15(VCN, inst_idx, regUVD_STATUS);
+
 	return 0;
 }
 
@@ -946,6 +951,11 @@ static int vcn_v5_0_0_start(struct amdgpu_vcn_inst *vinst)
 	WREG32_SOC15(VCN, i, 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, i, regUVD_STATUS);
+
 	return 0;
 }
 
@@ -977,6 +987,11 @@ static void vcn_v5_0_0_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
 	WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, 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, inst_idx, regUVD_STATUS);
+
 	return;
 }
 
@@ -1058,6 +1073,11 @@ static int vcn_v5_0_0_stop(struct amdgpu_vcn_inst *vinst)
 	/* enable VCN power gating */
 	vcn_v5_0_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(VCN, i, regUVD_STATUS);
+
 done:
 	if (adev->pm.dpm_enabled)
 		amdgpu_dpm_enable_vcn(adev, false, i);
-- 
2.49.0

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

[David (Ming Qiang) Wu]

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@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);
 
 	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;
 }
 
-- 
2.49.0

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

[Dong, Ruijing]

[AMD Official Use Only - AMD Internal Distribution Only]

-----Original Message-----
From: Wu, David <David.Wu3@amd.com>
Sent: Thursday, May 15, 2025 12:41 PM
To: amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Liu, Leo <Leo.Liu@amd.com>; Jiang, Sonny <Sonny.Jiang@amd.com>; Dong, Ruijing <Ruijing.Dong@amd.com>
Subject: [PATCH v2 5/9] drm/amdgpu: read back register after written

The addition of register read-back in VCN v4.0.0 is intended to prevent potential race conditions.

Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
index 8fff470bce87..5acdf8fd5a62 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
@@ -1122,6 +1122,11 @@ static int vcn_v4_0_start_dpg_mode(struct amdgpu_vcn_inst *vinst, bool indirect)
                        ring->doorbell_index << VCN_RB1_DB_CTRL__OFFSET__SHIFT |
                        VCN_RB1_DB_CTRL__EN_MASK);

+       /* Keeping one read-back to ensure all register writes are done,
+        * otherwise it may introduce race conditions.
+        */
+       RREG32_SOC15(VCN, inst_idx, regUVD_RB_WPTR);
+



Use the same register regUVD_STATUS?


        return 0;
 }

@@ -1303,6 +1308,11 @@ static int vcn_v4_0_start(struct amdgpu_vcn_inst *vinst)
        WREG32_SOC15(VCN, i, 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, i, regUVD_RB_WPTR);
+
        return 0;
 }

@@ -1583,6 +1593,11 @@ static void vcn_v4_0_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
        /* disable dynamic power gating mode */
        WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, 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, inst_idx, regUVD_STATUS);
 }

 /**
@@ -1666,6 +1681,11 @@ static int vcn_v4_0_stop(struct amdgpu_vcn_inst *vinst)
        /* enable VCN power gating */
        vcn_v4_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(VCN, i, regUVD_STATUS);
+
 done:
        if (adev->pm.dpm_enabled)
                amdgpu_dpm_enable_vcn(adev, false, i);
--
2.49.0

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

[David Wu]

On 2025-05-15 13:25, Dong, Ruijing wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> -----Original Message-----
> From: Wu, David <David.Wu3@amd.com>
> Sent: Thursday, May 15, 2025 12:41 PM
> To: amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Liu, Leo <Leo.Liu@amd.com>; Jiang, Sonny <Sonny.Jiang@amd.com>; Dong, Ruijing <Ruijing.Dong@amd.com>
> Subject: [PATCH v2 5/9] drm/amdgpu: read back register after written
>
> The addition of register read-back in VCN v4.0.0 is intended to prevent potential race conditions.
>
> Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
> index 8fff470bce87..5acdf8fd5a62 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
> @@ -1122,6 +1122,11 @@ static int vcn_v4_0_start_dpg_mode(struct amdgpu_vcn_inst *vinst, bool indirect)
>                          ring->doorbell_index << VCN_RB1_DB_CTRL__OFFSET__SHIFT |
>                          VCN_RB1_DB_CTRL__EN_MASK);
>
> +       /* Keeping one read-back to ensure all register writes are done,
> +        * otherwise it may introduce race conditions.
> +        */
> +       RREG32_SOC15(VCN, inst_idx, regUVD_RB_WPTR);
> +
>
>
>
> Use the same register regUVD_STATUS?
good catch - I will change them.
David
>
>
>          return 0;
>   }
>
> @@ -1303,6 +1308,11 @@ static int vcn_v4_0_start(struct amdgpu_vcn_inst *vinst)
>          WREG32_SOC15(VCN, i, 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, i, regUVD_RB_WPTR);
> +
>          return 0;
>   }
>
> @@ -1583,6 +1593,11 @@ static void vcn_v4_0_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
>          /* disable dynamic power gating mode */
>          WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, 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, inst_idx, regUVD_STATUS);
>   }
>
>   /**
> @@ -1666,6 +1681,11 @@ static int vcn_v4_0_stop(struct amdgpu_vcn_inst *vinst)
>          /* enable VCN power gating */
>          vcn_v4_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(VCN, i, regUVD_STATUS);
> +
>   done:
>          if (adev->pm.dpm_enabled)
>                  amdgpu_dpm_enable_vcn(adev, false, i);
> --
> 2.49.0
>

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

[Dong, Ruijing]

[AMD Official Use Only - AMD Internal Distribution Only]

With the change,

The series is
Reviewed-by: Ruijing Dong <ruijing.dong@amd.com>

Thanks,
Ruijing

-----Original Message-----
From: Wu, David <David.Wu3@amd.com>
Sent: Thursday, May 15, 2025 1:30 PM
To: Dong, Ruijing <Ruijing.Dong@amd.com>; Wu, David <David.Wu3@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Liu, Leo <Leo.Liu@amd.com>; Jiang, Sonny <Sonny.Jiang@amd.com>
Subject: Re: [PATCH v2 5/9] drm/amdgpu: read back register after written


On 2025-05-15 13:25, Dong, Ruijing wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> -----Original Message-----
> From: Wu, David <David.Wu3@amd.com>
> Sent: Thursday, May 15, 2025 12:41 PM
> To: amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Liu, Leo <Leo.Liu@amd.com>; Jiang, Sonny <Sonny.Jiang@amd.com>; Dong, Ruijing <Ruijing.Dong@amd.com>
> Subject: [PATCH v2 5/9] drm/amdgpu: read back register after written
>
> The addition of register read-back in VCN v4.0.0 is intended to prevent potential race conditions.
>
> Signed-off-by: David (Ming Qiang) Wu <David.Wu3@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
> index 8fff470bce87..5acdf8fd5a62 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
> @@ -1122,6 +1122,11 @@ static int vcn_v4_0_start_dpg_mode(struct amdgpu_vcn_inst *vinst, bool indirect)
>                          ring->doorbell_index << VCN_RB1_DB_CTRL__OFFSET__SHIFT |
>                          VCN_RB1_DB_CTRL__EN_MASK);
>
> +       /* Keeping one read-back to ensure all register writes are done,
> +        * otherwise it may introduce race conditions.
> +        */
> +       RREG32_SOC15(VCN, inst_idx, regUVD_RB_WPTR);
> +
>
>
>
> Use the same register regUVD_STATUS?
good catch - I will change them.
David
>
>
>          return 0;
>   }
>
> @@ -1303,6 +1308,11 @@ static int vcn_v4_0_start(struct amdgpu_vcn_inst *vinst)
>          WREG32_SOC15(VCN, i, 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, i, regUVD_RB_WPTR);
> +
>          return 0;
>   }
>
> @@ -1583,6 +1593,11 @@ static void vcn_v4_0_stop_dpg_mode(struct amdgpu_vcn_inst *vinst)
>          /* disable dynamic power gating mode */
>          WREG32_P(SOC15_REG_OFFSET(VCN, inst_idx, 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, inst_idx, regUVD_STATUS);
>   }
>
>   /**
> @@ -1666,6 +1681,11 @@ static int vcn_v4_0_stop(struct amdgpu_vcn_inst *vinst)
>          /* enable VCN power gating */
>          vcn_v4_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(VCN, i, regUVD_STATUS);
> +
>   done:
>          if (adev->pm.dpm_enabled)
>                  amdgpu_dpm_enable_vcn(adev, false, i);
> --
> 2.49.0
>

Re: [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@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;
>  }
>  

Re: [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@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@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;
>  }
>  

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

[Wu, David]

[-- Attachment #1: Type: text/plain, Size: 3363 bytes --]

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<David.Wu3@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;
>>   }
>>   

[-- Attachment #2: Type: text/html, Size: 4165 bytes --]

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

[Christian König]

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 <David.Wu3@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;
>>>  }
>>>  
> 

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

[Wu, David]

[-- Attachment #1: Type: text/plain, Size: 4297 bytes --]

hahaha! ok - I can drop this one (as no need but rather make it clear 
that any register read should do)
I kind of disagree about guarantee  - as long as the theory(read post 
write on PCIe will allow writes hit hardware) hold it is guaranteed. Why 
I ask for testing is to prove it works as expected which I am confident 
about.

Thanks for input. I will drop this change regardless.
David

On 5/16/2025 1:45 PM, Christian König wrote:
> 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<David.Wu3@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;
>>>>   }
>>>>   

[-- Attachment #2: Type: text/html, Size: 5050 bytes --]

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

[Lazar, Lijo]

On 5/17/2025 2:16 AM, Wu, David wrote:
> hahaha! ok - I can drop this one (as no need but rather make it clear
> that any register read should do)
> I kind of disagree about guarantee  - as long as the theory(read post
> write on PCIe will allow writes hit hardware) hold it is guaranteed. Why
> I ask for testing is to prove it works as expected which I am confident
> about.

As mentioned previously, a doorbell doesn't pass a register write on VCN
5.0.1/VCN 4.0.3 hardware over PCIe - this was already confirmed by
hardware team in a debug we had before. It could pass within SOC, but
that path is not required unless really required. There is no need to do
this to remain 'consistent' across SOCs. All SOCs don't behave the same
and there is no need to carry a VCN 1.0 programming model to VCN 5
generation or vice versa.

Thanks,
Lijo

> 
> Thanks for input. I will drop this change regardless.
> David
> 
> On 5/16/2025 1:45 PM, Christian König wrote:
>> 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 <David.Wu3@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;
>>>>>  }
>>>>>  
>