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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

> +	} 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

-- 
Ville Syrjälä
Intel