Re: [PATCH 16/20] drm/i915/display: Use fixed rr timings in intel_set_transcoder_timings_lrr()

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

On 2/26/2025 8:30 PM, Ville Syrjälä wrote:
> On Mon, Feb 24, 2025 at 11:47:13AM +0530, Ankit Nautiyal wrote:
>> To have Guardband/Pipeline_full reconfigured seamlessly, move the
>> guardband and pipeline_full checks out from the pure !fastset block in
>> intel_pipe_config_compare().
>> Update the intel_set_transcoder_timings_lrr() function to use
>> fixed refresh rate timings for platforms which always use
>> VRR timing generator.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 43 ++++++++++++++++----
>>   drivers/gpu/drm/i915/display/intel_vrr.c     | 13 +++++-
>>   drivers/gpu/drm/i915/display/intel_vrr.h     |  2 +
>>   3 files changed, 48 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 752ed44d8892..d3c12bc3645c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -2854,6 +2854,9 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
>>   	intel_de_write(dev_priv, TRANS_VTOTAL(dev_priv, cpu_transcoder),
>>   		       VACTIVE(crtc_vdisplay - 1) |
>>   		       VTOTAL(crtc_vtotal - 1));
>> +
>> +	intel_vrr_set_fixed_rr_timings(crtc_state);
>> +	intel_vrr_update_guardband(crtc_state, false);
>>   }
>>   
>>   static void intel_set_pipe_src_size(const struct intel_crtc_state *crtc_state)
>> @@ -5127,17 +5130,36 @@ pipe_config_cx0pll_mismatch(struct drm_printer *p, bool fastset,
>>   	intel_cx0pll_dump_hw_state(display, b);
>>   }
>>   
>> -static bool allow_vblank_delay_fastset(const struct intel_crtc_state *old_crtc_state)
>> +static
>> +bool allow_vrr_guardband_fastset(const struct intel_crtc_state *old_crtc_state,
>> +				 const struct intel_crtc_state *new_crtc_state)
>> +{
>> +	struct intel_display *display = to_intel_display(old_crtc_state);
>> +
>> +	if (!HAS_VRR(display))
>> +		return false;
>> +
>> +	if (DISPLAY_VER(display) >= 13)
>> +		return new_crtc_state->vrr.guardband != old_crtc_state->vrr.guardband;
>> +
>> +	return new_crtc_state->vrr.pipeline_full != old_crtc_state->vrr.pipeline_full;
>> +}
> None of this stuff should be necessary. allow_vblank_delay_fastset()
> only says whether we're allowed the fastset or not. It isn't supposed
> to check if it's necessary or not.
>
>> +
>> +static bool allow_vblank_delay_fastset(const struct intel_crtc_state *old_crtc_state,
>> +				       const struct intel_crtc_state *new_crtc_state)
>>   {
>>   	struct intel_display *display = to_intel_display(old_crtc_state);
>>   
>>   	/*
>> -	 * Allow fastboot to fix up vblank delay (handled via LRR
>> -	 * codepaths), a bit dodgy as the registers aren't
>> +	 * Allow fastboot to fix up vblank delay, vrr guardband/pipeline_full
>> +	 * (handled via LRR codepaths), a bit dodgy as the registers aren't
>>   	 * double buffered but seems to be working more or less...
>>   	 */
>> -	return HAS_LRR(display) && old_crtc_state->inherited &&
>> -		!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DSI);
>> +	if (!HAS_LRR(display) || intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DSI))
>> +		return false;
>> +
>> +	return old_crtc_state->inherited ||
>> +	       allow_vrr_guardband_fastset(old_crtc_state, new_crtc_state);
>>   }
>>   
>>   bool
>> @@ -5273,7 +5295,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>>   	PIPE_CONF_CHECK_I(name.crtc_hsync_start); \
>>   	PIPE_CONF_CHECK_I(name.crtc_hsync_end); \
>>   	PIPE_CONF_CHECK_I(name.crtc_vdisplay); \
>> -	if (!fastset || !allow_vblank_delay_fastset(current_config)) \
>> +	if (!fastset || !allow_vblank_delay_fastset(current_config, pipe_config)) \
>>   		PIPE_CONF_CHECK_I(name.crtc_vblank_start); \
>>   	PIPE_CONF_CHECK_I(name.crtc_vsync_start); \
>>   	PIPE_CONF_CHECK_I(name.crtc_vsync_end); \
>> @@ -5569,8 +5591,6 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>>   		PIPE_CONF_CHECK_I(vrr.vmin);
>>   		PIPE_CONF_CHECK_I(vrr.vmax);
>>   		PIPE_CONF_CHECK_I(vrr.flipline);
>> -		PIPE_CONF_CHECK_I(vrr.pipeline_full);
>> -		PIPE_CONF_CHECK_I(vrr.guardband);
>>   		PIPE_CONF_CHECK_I(vrr.vsync_start);
>>   		PIPE_CONF_CHECK_I(vrr.vsync_end);
>>   		PIPE_CONF_CHECK_LLI(cmrr.cmrr_m);
>> @@ -5578,6 +5598,11 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>>   		PIPE_CONF_CHECK_BOOL(cmrr.enable);
>>   	}
>>   
>> +	if (!fastset || !allow_vblank_delay_fastset(current_config, pipe_config)) {
>> +		PIPE_CONF_CHECK_I(vrr.guardband);
>> +		PIPE_CONF_CHECK_I(vrr.pipeline_full);
>> +	}
> I'd like to see one patch that just moves these completely out
> of the !fastset block, and a second patch that adds the fastboot
> exception. That way if we do see any problems from the live
> guardband reprogramming we can just revert it.
I have one doubt.

If we remove guardband/pipeline_full from the !fastset block, the 
VRR-seamless-switch case will start failing.

This is because the test tries a virtual LRR mode, resulting in a change 
in the VRR guardband.

Since its not in fastset block, we will need a full modeset, but the 
test expects a seamless switch.

Regards,

Ankit


>
> Hmm, except you haven't changed the vrr_compute_config() yet at
> this point in the series, so changing this would break both fastset
> and state checker, I think. So we might have to move this patch to be
> later in the series.
>
>> +
>>   #undef PIPE_CONF_CHECK_X
>>   #undef PIPE_CONF_CHECK_I
>>   #undef PIPE_CONF_CHECK_LLI
>> @@ -5875,7 +5900,7 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
>>   		drm_dbg_kms(&i915->drm, "[CRTC:%d:%s] fastset requirement not met, forcing full modeset\n",
>>   			    crtc->base.base.id, crtc->base.name);
>>   	} else {
>> -		if (allow_vblank_delay_fastset(old_crtc_state))
>> +		if (allow_vblank_delay_fastset(old_crtc_state, new_crtc_state))
>>   			new_crtc_state->update_lrr = true;
>>   		new_crtc_state->uapi.mode_changed = false;
>>   	}
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 11b7edbdb116..e77f5b483b09 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -284,7 +284,6 @@ int intel_vrr_fixed_rr_flipline(const struct intel_crtc_state *crtc_state)
>>   	return intel_vrr_fixed_rr_vtotal(crtc_state);
>>   }
>>   
>> -static
>>   void intel_vrr_set_fixed_rr_timings(const struct intel_crtc_state *crtc_state)
>>   {
>>   	struct intel_display *display = to_intel_display(crtc_state);
>> @@ -677,3 +676,15 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>>   	if (crtc_state->vrr.enable)
>>   		crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>>   }
>> +
>> +void intel_vrr_update_guardband(const struct intel_crtc_state *crtc_state, bool needs_modeset)
>> +{
>> +	struct intel_display *display = to_intel_display(crtc_state);
>> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> +	u32 vrr_ctl = trans_vrr_ctl(crtc_state);
>> +
>> +	if (intel_vrr_always_use_vrr_tg(display) && !needs_modeset)
>> +		vrr_ctl |= VRR_CTL_VRR_ENABLE;
>> +
>> +	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), vrr_ctl);
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
>> index 514822577e8a..e4e9cadcdf9e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
>> @@ -35,5 +35,7 @@ int intel_vrr_vmin_vtotal(const struct intel_crtc_state *crtc_state);
>>   int intel_vrr_vmax_vblank_start(const struct intel_crtc_state *crtc_state);
>>   int intel_vrr_vmin_vblank_start(const struct intel_crtc_state *crtc_state);
>>   int intel_vrr_vblank_delay(const struct intel_crtc_state *crtc_state);
>> +void intel_vrr_update_guardband(const struct intel_crtc_state *crtc_state, bool needs_modeset);
>> +void intel_vrr_set_fixed_rr_timings(const struct intel_crtc_state *crtc_state);
>>   
>>   #endif /* __INTEL_VRR_H__ */
>> -- 
>> 2.45.2