[PATCH] drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+

[Aurabindo Pillai]

On 6/1/23 18:49, Rodrigo Siqueira Jordao wrote:
> Hi Jay,
> 
> On 6/1/23 11:09, Aurabindo Pillai wrote:
>> Due to FPO, firmware will try to change OTG timings, which would only
>> latch if min/max selectors for vtotal are written by the driver.
> 
> Could you elaborate a little bit more about this issue? Right now, do we 
> have some sort of race between firmware and driver? Is this situation 
> causing some problems that we can reproduce? If so, could you also 
> describe it?

Sure. Its not a race condition, but a programming sequence issue. For FPO/FAMS, DMCUB will try to change the output timings by writing to the OTG registers. However, the timings written directly to the OTG registers will not be honoured unless VMIN/VMAX selector registers are programmed with the right bits and trigger source is selected correctly. Its easier to do this from driver since putting the fix into DMCUB firmware will require additional state tracking on when to write these registers and when not to. Although this is the ideal solution, we would like a software solution that is simpler and faster to deploy to the end users. Once the proper solution is made, this workaround can be removed. I will add this to the commit description for v2.
> 
>> Signed-off-by: Aurabindo Pillai <aurabindo.pillai at amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c | 15 +++------------
>>   drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c |  6 ++++++
>>   2 files changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
>> index e1975991e075..633989fd2514 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
>> @@ -930,19 +930,10 @@ void optc1_set_drr(
>>   				OTG_FORCE_LOCK_ON_EVENT, 0,
>>   				OTG_SET_V_TOTAL_MIN_MASK_EN, 0,
>>   				OTG_SET_V_TOTAL_MIN_MASK, 0);
>> -
>> -		// Setup manual flow control for EOF via TRIG_A
>> -		optc->funcs->setup_manual_trigger(optc);
>> -
>> -	} else {
>> -		REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
>> -				OTG_SET_V_TOTAL_MIN_MASK, 0,
>> -				OTG_V_TOTAL_MIN_SEL, 0,
>> -				OTG_V_TOTAL_MAX_SEL, 0,
>> -				OTG_FORCE_LOCK_ON_EVENT, 0);
>> -
>> -		optc->funcs->set_vtotal_min_max(optc, 0, 0);
>>   	}
>> +
>> +	// Setup manual flow control for EOF via TRIG_A
>> +	optc->funcs->setup_manual_trigger(optc);
>>   }
>>   
>>   void optc1_set_vtotal_min_max(struct timing_generator *optc, int vtotal_min, int vtotal_max)
>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
>> index e0edc163d767..042ce082965f 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
>> @@ -455,6 +455,12 @@ void optc2_setup_manual_trigger(struct timing_generator *optc)
>>   {
>>   	struct optc *optc1 = DCN10TG_FROM_TG(optc);
>>   
>> +	REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
> 
> Could you align the below registers right after the open parenthesis?

Will do
> 
>> +                                OTG_V_TOTAL_MIN_SEL, 1,
>> +                                OTG_V_TOTAL_MAX_SEL, 1,
>> +                                OTG_FORCE_LOCK_ON_EVENT, 0,
> 
> Could you add a comment that describes why we want to set the above values?

This comes from hardware documentation. Inorder to change the output timings, these bits need to be set.
> 
>> +                                OTG_SET_V_TOTAL_MIN_MASK, (1 << 1)); /* TRIGA */
> 
> Why do you use (1 << 1)? Why not set the value directly here? Also, in 
> the comment, I guess it should be TRIG_A.

Merely for readability. It conveys that we're setting Trigger source A, which is bit 1 of the field OTG_FORCE_LOCK_ON_EVENT.
> 
>> +
>>   	REG_SET_8(OTG_TRIGA_CNTL, 0,
>>   			OTG_TRIGA_SOURCE_SELECT, 21,
>>   			OTG_TRIGA_SOURCE_PIPE_SELECT, optc->inst,
>