Re: [PATCH 31/35] drm/i915/vrr: Always set vrr vmax/vmin/flipline in vrr_{enable/disable}

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

On 1/25/2025 3:23 AM, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 08:30:16PM +0530, Ankit Nautiyal wrote:
>> To work seamlessly between variable and fixed timings,
>> intel_vrr_{enable,disable}() should just flip between the fixed and
>> variable timings in vmin/flipline/vmax.
>>
>> The idea is to just do this for all the platforms, regardless of whether
>> we also toggle the VRR_CTL enable bit there.
>>
>> For platforms for which vrr timing generator is always set, VRR_CTL
>> enable bit does not need to toggle, so modify the vrr_{enable/disable}
>> for this.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c |  5 ++-
>>   drivers/gpu/drm/i915/display/intel_vrr.c     | 44 ++++++++++++--------
>>   drivers/gpu/drm/i915/display/intel_vrr.h     |  4 +-
>>   3 files changed, 31 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 7bdd41158a93..a0d6360f4cda 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -1310,7 +1310,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
>>   	intel_psr_pre_plane_update(state, crtc);
>>   
>>   	if (intel_crtc_vrr_disabling(state, crtc)) {
>> -		intel_vrr_disable(old_crtc_state);
>> +		intel_vrr_disable(old_crtc_state, false);
>>   		intel_crtc_update_active_timings(old_crtc_state, false);
>>   	}
>>   
>> @@ -1751,6 +1751,7 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>>   {
>>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	struct intel_display *display = to_intel_display(crtc_state);
>>   	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>>   
>>   	if (crtc_state->has_pch_encoder) {
>> @@ -7161,7 +7162,7 @@ static void commit_pipe_post_planes(struct intel_atomic_state *state,
>>   		skl_detach_scalers(new_crtc_state);
>>   
>>   	if (intel_crtc_vrr_enabling(state, crtc))
>> -		intel_vrr_enable(new_crtc_state);
>> +		intel_vrr_enable(new_crtc_state, false);
>>   }
>>   
>>   static void intel_enable_crtc(struct intel_atomic_state *state,
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index ccc40867c10a..10a9bcb8daae 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -496,7 +496,7 @@ bool intel_vrr_is_push_sent(const struct intel_crtc_state *crtc_state)
>>   	return intel_de_read(display, TRANS_PUSH(display, cpu_transcoder)) & TRANS_PUSH_SEND;
>>   }
>>   
>> -void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>> +void intel_vrr_enable(const struct intel_crtc_state *crtc_state, bool always_use_vrr_tg)
> That new parameter shouldn't be needed. We should already know whether
> we're always using the VRR timing generator or not.
>
>>   {
>>   	struct intel_display *display = to_intel_display(crtc_state);
>>   	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> @@ -507,21 +507,25 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>>   	if (!intel_vrrtg_is_enabled(crtc_state))
>>   		return;
>>   
>> -	if (intel_vrr_use_push(crtc_state))
>> -		intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
>> -			       TRANS_PUSH_EN);
>> +	intel_vrr_set_transcoder_timings(crtc_state);
> That guy probably does a few too many things for us.
> Either we need to chop it up or not even use it.
> We just want vmax/vmin/flipline updated here.
Yes right. The below code snippet seems more appropriate for this case.
>
> So I'm thinking this should look more or less like this:
> vrr_enable() {
> 	write(VMAX, crtc_state->vrr.vmax - 1);
> 	write(VMIN, crtc_state->vrr.vmin - 1);
> 	write(FLIPLINE, crtc_state->vrr.flipline - 1);
>
> 	if (!always_use_vrr_tg) {
> 		enable PUSH
> 		enable VRR_VTL
> 		wait for VRR status
> 	}
> }
>
> vrr_disable() {
> 	if (!always_use_vrr_tg) {
> 		disable VRR_VTL
> 		wait for VRR status
> 		disable PUSH
> 	}
>
> 	write(VMAX, intel_vrr_fixed_rr_vmax(crtc_state) - 1);
> 	write(VMIN, intel_vrr_fixed_rr_vmin(crtc_state) - 1);
> 	write(FLIPLINE, intel_vrr_fixed_rr_flipline(crtc_state) - 1);
> }
>
> And similarly during modeset enable sequence we should also
> always program those fixed timings, then turn on the VRR TG at
> an appropriate spot (if always using it), and let the later
> vrr_enable() (if necessary) switch to the real VRR timings.
> That way it works alsmost the same regardless of whether
> whether we always use the VRR TG or not.

Alright, I got the idea. Will try this next.

There is one doubt about intel_vrr_enabling/disabling helper.

Earlier we were using is_{enabling/disabling}(vrr.enable) where 
vrr.enable was tracking if variable timing is used. We would still be 
tracking here the same thing right ?

Since I have removed vrr.enable and using vrr.mode, the macros:

is_{enabling/disabling}(feature, old_crtc_state, new_crtc_state) wont work.

Should I still keep vrr.enable? Or perhaps modify the conditions in 
intel_vrr_enabling/disabling?


>
> The fixed_rr stuff could be written like so (then they would work
> for all platforms, if anyone feels like trying this mode of
> operation on ICL/TGL as well):
>
> intel_vrr_fixed_rr_vtotal()
> {
>          if (DISPLAY_VER >= 13)
>                  return crtc_vtotal;
> 	else
>              	return crtc_vtotal -
> 	                intel_vrr_real_vblank_delay();
> }
>
> intel_vrr_fixed_rr_vmax()
> {
>          return intel_vrr_fixed_rr_vtotal();
> }
>
> intel_vrr_fixed_rr_vmin()
> {
>          return intel_vrr_fixed_rr_vtotal() -
>                  intel_vrr_flipline_offset();
> }
>
> intel_vrr_fixed_rr_flipline()
> {
>          return intel_vrr_fixed_rr_vtotal();
> }

This is clear, will use these in vrr_disable.

Regards,

Ankit

>>   
>> -	if (crtc_state->vrr.mode == INTEL_VRRTG_MODE_CMRR) {
>> -		intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
>> -			       VRR_CTL_VRR_ENABLE | VRR_CTL_CMRR_ENABLE |
>> -			       trans_vrr_ctl(crtc_state));
>> -	} else {
>> -		intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
>> -			       VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
>> +	if (!always_use_vrr_tg) {
>> +		if (intel_vrr_use_push(crtc_state))
>> +			intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
>> +				       TRANS_PUSH_EN);
>> +
>> +		if (crtc_state->vrr.mode == INTEL_VRRTG_MODE_CMRR) {
>> +			intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
>> +				       VRR_CTL_VRR_ENABLE | VRR_CTL_CMRR_ENABLE |
>> +				       trans_vrr_ctl(crtc_state));
>> +		} else {
>> +			intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
>> +				       VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
>> +		}
>>   	}
>>   }
>>   
>> -void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
>> +void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state, bool always_use_vrr_tg)
>>   {
>>   	struct intel_display *display = to_intel_display(old_crtc_state);
>>   	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
>> @@ -532,12 +536,16 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
>>   	if (!intel_vrrtg_is_enabled(old_crtc_state))
>>   		return;
>>   
>> -	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
>> -		       trans_vrr_ctl(old_crtc_state));
>> -	intel_de_wait_for_clear(display,
>> -				TRANS_VRR_STATUS(display, cpu_transcoder),
>> -				VRR_STATUS_VRR_EN_LIVE, 1000);
>> -	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), 0);
>> +	if (!always_use_vrr_tg) {
>> +		intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
>> +			       trans_vrr_ctl(old_crtc_state));
>> +		intel_de_wait_for_clear(display,
>> +					TRANS_VRR_STATUS(display, cpu_transcoder),
>> +					VRR_STATUS_VRR_EN_LIVE, 1000);
>> +		intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), 0);
>> +	}
>> +
>> +	intel_vrr_set_transcoder_timings(old_crtc_state);
>>   }
>>   
>>   void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
>> index 8d53aab3590d..da6a86cee0e8 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
>> @@ -22,11 +22,11 @@ void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>   			      struct drm_connector_state *conn_state);
>>   void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state);
>>   void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state);
>> -void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
>> +void intel_vrr_enable(const struct intel_crtc_state *crtc_state, bool always_use_vrr_tg);
>>   void intel_vrr_send_push(struct intel_dsb *dsb,
>>   			 const struct intel_crtc_state *crtc_state);
>>   bool intel_vrr_is_push_sent(const struct intel_crtc_state *crtc_state);
>> -void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state);
>> +void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state, bool always_use_vrr_tg);
>>   void intel_vrr_get_config(struct intel_crtc_state *crtc_state);
>>   int intel_vrr_vmax_vtotal(const struct intel_crtc_state *crtc_state);
>>   int intel_vrr_vmin_vtotal(const struct intel_crtc_state *crtc_state);
>> -- 
>> 2.45.2