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

["Wu, David" <davidwu2@amd.com>]

[-- 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 --]