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