Re: [PATCH 10/13] drm/i915/vrr: Avoid sending PUSH when VRR TG is used with Fixed refresh rate

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 9/3/2024 6:32 PM, Ville Syrjälä wrote:
> On Mon, Sep 02, 2024 at 01:36:31PM +0530, Ankit Nautiyal wrote:
>> As per Bspec:68925: Push enable must be set if not configuring for a
>> fixed refresh rate (i.e Vmin == Flipline == Vmax is not true).
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vrr.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 03af50b9f9eb..5e947465c6e0 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -313,7 +313,7 @@ void intel_vrr_send_push(const struct intel_crtc_state *crtc_state)
>>   	struct intel_display *display = to_intel_display(crtc_state);
>>   	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>>   
>> -	if (!crtc_state->vrr.enable)
>> +	if (!crtc_state->vrr.enable || crtc_state->vrr.fixed_rr)
> Hmm. I wonder if we should keep vrr.enable meaning VRR actually
> enabled... Maybe not as that would complicate the readout/check
> a bit too much perhaps.

I agree this perhaps need to be streamline.

Currently with this patch series vrr.enable
represents whether VRR timing generator is enabled or not.

When VRR timing generator is enabled we can be in one of 3 modes:
1. Acutal variable refresh rate modes    (vrr.enable = true; 
vrr.fixed_rr = false)
2. Fixed refresh rate modes                (vrr.enable = true; 
vrr.fixed_rr = true)
3. Content matched refresh rate mode.    (vrr.enable = true; vrr.cmrr = 
true)

I am open regarding any modification to this or have a new scheme 
(perhaps an enum vrr.mode for the above 3)


>
> But we could have some kind of helper for these "do we need to use
> push?" checks (eg. intel_vrr_use_push()).

Makes sense. I will add these helpers.


Regards,

Ankit

>>   		return;
>>   
>>   	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
>> @@ -325,7 +325,7 @@ bool intel_vrr_is_push_sent(const struct intel_crtc_state *crtc_state)
>>   	struct intel_display *display = to_intel_display(crtc_state);
>>   	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>>   
>> -	if (!crtc_state->vrr.enable)
>> +	if (!crtc_state->vrr.enable || crtc_state->vrr.fixed_rr)
>>   		return false;
>>   
>>   	return intel_de_read(display, TRANS_PUSH(display, cpu_transcoder)) & TRANS_PUSH_SEND;
>> @@ -339,8 +339,9 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>>   	if (!crtc_state->vrr.enable)
>>   		return;
>>   
>> -	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
>> -		       TRANS_PUSH_EN);
>> +	if (!crtc_state->vrr.fixed_rr)
>> +		intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
>> +			       TRANS_PUSH_EN);
>>   
>>   	if (HAS_AS_SDP(display))
>>   		intel_de_write(display,
>> -- 
>> 2.45.2