Re: [PATCH 12/14] drm/i915/vrr: Use static guardband to support seamless LRR switching

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

On 9/8/2025 10:13 PM, Ville Syrjälä wrote:
> On Sun, Sep 07, 2025 at 01:02:39PM +0530, Ankit Nautiyal wrote:
>> In the current VRR implementation, vrr.vmin and vrr.guardband are set such
>> that they do not need to change when switching from fixed refresh rate to
>> variable refresh rate. Specifically, vrr.guardband is always set to match
>> the vblank length. This approach works for most cases, but not for LRR,
>> where the guardband would need to change while the VRR timing generator is
>> still active.
>>
>> With the VRR TG always active, live updates to guardband are unsafe and not
>> recommended. To ensure hardware safety, guardband was moved out of the
>> !fastset block, meaning any change now requires a full modeset.
>> This breaks seamless LRR switching, which was previously supported.
>>
>> Since the problem arises from guardband being matched to the vblank length,
>> solution is to use a minimal, sufficient static value, instead. So we use a
>> static guardband defined during mode-set that fits within the smallest
>> expected vblank and remains unchanged in case of features like LRR where
>> vtotal changes. To compute this minimum guardband we take into account
>> latencies/delays due to different features as mentioned in the Bspec.
>>
>> v2:
>> -Use helpers for dsc/scaler prefill latencies. (Mitul)
>> -Account for pkgc latency and take max of pkgc and sagv latencies.
>> v3: Use new helper for PSR2/Panel Replay latency.
>> v4: Avoid re-setting the Vmin/Flipline for optimized guardband.
>>
>> Bspec: 70151
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> (#v3)
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c |   2 +-
>>   drivers/gpu/drm/i915/display/intel_vrr.c     | 127 ++++++++++++++++++-
>>   drivers/gpu/drm/i915/display/intel_vrr.h     |   3 +-
>>   3 files changed, 128 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index fb072275b1c7..3fa94675d5e1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -4902,7 +4902,6 @@ intel_modeset_pipe_config_late(struct intel_atomic_state *state,
>>   	struct drm_connector *connector;
>>   	int i;
>>   
>> -	intel_vrr_compute_config_late(crtc_state);
>>   
>>   	for_each_new_connector_in_state(&state->base, connector,
>>   					conn_state, i) {
>> @@ -4914,6 +4913,7 @@ intel_modeset_pipe_config_late(struct intel_atomic_state *state,
>>   		    !encoder->compute_config_late)
>>   			continue;
>>   
>> +		intel_vrr_compute_config_late(crtc_state, conn_state);
>>   		ret = encoder->compute_config_late(encoder, crtc_state,
>>   						   conn_state);
>>   		if (ret)
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 855974174afd..fff684eb2514 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -6,12 +6,15 @@
>>   
>>   #include <drm/drm_print.h>
>>   
>> +#include "intel_alpm.h"
>>   #include "intel_de.h"
>>   #include "intel_display_regs.h"
>>   #include "intel_display_types.h"
>>   #include "intel_dp.h"
>>   #include "intel_vrr.h"
>>   #include "intel_vrr_regs.h"
>> +#include "skl_scaler.h"
>> +#include "skl_watermark.h"
>>   
>>   #define FIXED_POINT_PRECISION		100
>>   #define CMRR_PRECISION_TOLERANCE	10
>> @@ -413,15 +416,135 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>   	}
>>   }
>>   
>> -void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
>> +static
>> +int scaler_prefill_latency(struct intel_crtc_state *crtc_state, int linetime_us)
>> +{
>> +	int chroma_downscaling_factor = skl_scaler_chroma_downscale_factor(crtc_state);
>> +	u64 hscale_k, vscale_k;
>> +	int cdclk_adjustment;
>> +	int num_scaler_users;
>> +
>> +	/*
>> +	 * Assuming:
>> +	 * Both scaler enabled.
>> +	 * scaler 1 downscaling factor as 2 x 2 (Horiz x Vert)
>> +	 * scaler 2 downscaling factor as 2 x 1 (Horiz x Vert)
>> +	 * Cdclk Adjustment : 1
>> +	 */
>> +	num_scaler_users = 2;
>> +	hscale_k = 2 * 1000;
>> +	vscale_k = 2 * 1000;
>> +	cdclk_adjustment = 1;
>> +
>> +	return intel_vrr_guardband_scaler_latency(num_scaler_users, hscale_k, vscale_k,
>> +						  chroma_downscaling_factor,
>> +						  cdclk_adjustment,
>> +						  linetime_us);
>> +}
>> +
>> +static
>> +int dsc_prefill_latency(struct intel_crtc_state *crtc_state, int linetime_us)
>> +{
>> +#define MAX_SCALERS 2
>> +	int chroma_downscaling_factor = skl_scaler_chroma_downscale_factor(crtc_state);
>> +	u64 hscale_k[MAX_SCALERS], vscale_k[MAX_SCALERS];
>> +	int cdclk_adjustment;
>> +	int num_scaler_users;
>> +
>> +	/*
>> +	 * Assuming:
>> +	 * Both scaler enabled.
>> +	 * scaler 1 downscaling factor as 2 x 2 (Horiz x Vert)
>> +	 * scaler 2 downscaling factor as 2 x 1 (Horiz x Vert)
>> +	 * Cdclk Adjustment : 1
>> +	 */
>> +	num_scaler_users = MAX_SCALERS;
>> +	hscale_k[0] = 2 * 1000;
>> +	vscale_k[0] = 2 * 1000;
>> +	hscale_k[1] = 2 * 1000;
>> +	vscale_k[1] = 1 * 1000;
>> +
>> +	cdclk_adjustment = 1;
>> +
>> +	return intel_vrr_guardband_dsc_latency(num_scaler_users, hscale_k, vscale_k,
>> +					       chroma_downscaling_factor,
>> +					       cdclk_adjustment,
>> +					       linetime_us);
>> +}
>> +
>> +static
>> +int intel_vrr_compute_guardband(struct intel_crtc_state *crtc_state,
>> +				struct intel_connector *connector)
>> +{
>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>> +	struct intel_display *display = to_intel_display(crtc_state);
>> +	int dsc_prefill_time = 0;
>> +	int psr2_pr_latency = 0;
>> +	int scaler_prefill_time;
>> +	int wm0_prefill_time;
>> +	int pkgc_max_latency;
>> +	int sagv_latency;
>> +	int sdp_latency = 0;
>> +	int guardband_us;
>> +	int linetime_us;
>> +	int guardband;
>> +	int pm_delay;
>> +
>> +	linetime_us = DIV_ROUND_UP(adjusted_mode->crtc_htotal * 1000,
>> +				   adjusted_mode->crtc_clock);
>> +
>> +	pkgc_max_latency = skl_watermark_max_latency(display, 1);
>> +	sagv_latency = display->sagv.block_time_us;
>> +
>> +	/* Assuming max wm0 lines = 4 */
>> +	wm0_prefill_time = 4 * linetime_us + 20;
>> +
>> +	scaler_prefill_time = scaler_prefill_latency(crtc_state, linetime_us);
>> +
>> +	if (crtc_state->dsc.compression_enable)
>> +		dsc_prefill_time = dsc_prefill_latency(crtc_state, linetime_us);
>> +
>> +	pm_delay = crtc_state->framestart_delay +
>> +		   max(sagv_latency, pkgc_max_latency) +
>> +		   wm0_prefill_time +
>> +		   scaler_prefill_time +
>> +		   dsc_prefill_time;
>> +
>> +	switch (connector->base.connector_type) {
>> +	case DRM_MODE_CONNECTOR_eDP:
>> +	case DRM_MODE_CONNECTOR_DisplayPort:
>> +		psr2_pr_latency = intel_alpm_compute_max_link_wake_latency(crtc_state, true);
>> +		sdp_latency = intel_dp_compute_sdp_latency(crtc_state, true);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	guardband_us = max(sdp_latency, psr2_pr_latency);
>> +	guardband_us = max(guardband_us, pm_delay);
>> +
>> +	guardband = DIV_ROUND_UP(guardband_us, linetime_us);
>> +
>> +	/* guardband cannot be more than the Vmax vblank */
>> +	guardband = min(guardband, crtc_state->vrr.vmax - adjusted_mode->crtc_vblank_start);
>> +
>> +	return guardband;
>> +}
>> +
>> +void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state,
>> +				   struct drm_connector_state *conn_state)
>>   {
>>   	struct intel_display *display = to_intel_display(crtc_state);
>>   	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>> +	struct intel_connector *connector =
>> +		to_intel_connector(conn_state->connector);
>>   
>>   	if (!intel_vrr_possible(crtc_state))
>>   		return;
>>   
>> -	if (DISPLAY_VER(display) >= 13) {
>> +	if (intel_vrr_always_use_vrr_tg(display)) {
>> +		crtc_state->vrr.guardband = intel_vrr_compute_guardband(crtc_state, connector);
> This all looks to be in the wrong place. It needs to be done as part of
> intel_crtc_compute_vblank_delay() which then updates crtc_vblank_start
> to reflect reality instead of leaving some incorrect junk in there.
> Presumably that is why you also had to do that
> intel_dsb_wait_for_delayed_vblank() hack.

Hi Ville,

Thanks for for the comments.

Looks like intel_crtc_compute_vblank_delay() indeed is the place to add 
the vblank_delay and get the crtc_vblank_start point to the delayed vblank.

But I think, there are couple of things need to take care with this change:
1) The SCL currently is derived from crtc_vblank_start - vdisplay, which 
is ~1 with guradband = vblank length.
With modified crtc_vblank_start, this will become huge so perhaps we 
need to set it to 1?

2) The intel_dsb_wait_vblank_delay() (which inturn uses 
intel_vrr_real_vblank_delay() for vrr case) also computes a fixed delay 
based on (crtc_vblank_start - vdisplay) lines
Instead of this should we not use intel_dsb_wait_scanline_in() to wait 
only till scanline reaches in range  [flipline decision boundary, vmax 
decision boundary]?

Let me know what you think, I can send a follow-up patch with the 
suggested changes.

Regards,
Ankit


>
>> +	} else if (DISPLAY_VER(display) >= 13) {
>>   		crtc_state->vrr.guardband =
>>   			crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start;
>>   	} else {
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
>> index 950041647e47..362638fd0d66 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
>> @@ -21,7 +21,8 @@ bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
>>   void intel_vrr_check_modeset(struct intel_atomic_state *state);
>>   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_compute_config_late(struct intel_crtc_state *crtc_state,
>> +				   struct drm_connector_state *conn_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_send_push(struct intel_dsb *dsb,
>> -- 
>> 2.45.2