Re: [PATCH 6/6] drm/i915/vrr: Use the min static optimized guardband

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

On Thu, Oct 16, 2025 at 03:00:09PM +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.
> 
> Introduce a helper to compute the minimal sufficient guardband.
> On platforms where the VRR timing generator is always ON, we optimize the
> guardband regardless of whether the display is operating in fixed or
> variable refresh rate mode.
> On platforms where the VRR timing generator is not always ON, we optimize
> the guardband only when VRR is enabled.
> 
> Bspec: 70151
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 50 +++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 597008a6c744..732e356e6fac 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -6,12 +6,16 @@
>  
>  #include <drm/drm_print.h>
>  
> +#include "intel_crtc.h"
>  #include "intel_de.h"
>  #include "intel_display_regs.h"
>  #include "intel_display_types.h"
>  #include "intel_dp.h"
> +#include "intel_psr.h"
>  #include "intel_vrr.h"
>  #include "intel_vrr_regs.h"
> +#include "skl_prefill.h"
> +#include "skl_watermark.h"
>  
>  #define FIXED_POINT_PRECISION		100
>  #define CMRR_PRECISION_TOLERANCE	10
> @@ -433,17 +437,59 @@ intel_vrr_max_guardband(struct intel_crtc_state *crtc_state)
>  		   intel_vrr_max_vblank_guardband(crtc_state));
>  }
>  
> +static
> +int intel_vrr_compute_optimized_guardband(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct skl_prefill_ctx prefill_ctx;
> +	int psr_min_guardband = 0;
> +	int sdp_min_guardband = 0;
> +	int prefill_min_guardband;
> +	int prefill_sagv_us;
> +	int guardband;
> +
> +	skl_prefill_init_worst(&prefill_ctx, crtc_state);
> +	prefill_sagv_us = display->sagv.block_time_us;

We want max(sagv, skl_wm_latency(1)) here or else we risk destroying
power savings.

> +	prefill_min_guardband =
> +		skl_prefill_min_guardband(&prefill_ctx,
> +					  crtc_state,
> +					  prefill_sagv_us);
> +
> +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> +		psr_min_guardband = intel_psr_min_guardband(crtc_state);
> +		sdp_min_guardband = intel_dp_sdp_min_guardband(crtc_state, true);
> +	}
> +
> +	guardband = max(sdp_min_guardband, psr_min_guardband);
> +
> +	guardband = max(guardband, prefill_min_guardband);

All these temp variables seem rather pointless. Could just do

guardband = prefill_min_guardband(...);
if (dp)
 guardband = max(guardband, intel_psr_min_guardband(...));
 guardband = max(guardband, intel_dp_sdp_min_guardband(...));
}

I'm also pondering what to do about HDMI. A quick glance at the spec
says a lot of the inforframes are transmitted a line of two after
vsync, but the double buffering point is at delayed vblank. That to
me suggests that for HDMI we'd want to makes sure the start of
vsync happens after delayed vblank. For now it's probably safest to
simply not use the optimized guardband on HDMI. We can take another
stab at it on some later date.

> +
> +	return guardband;
> +}
> +
> +static bool intel_vrr_use_optimized_guardband(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +
> +	return intel_vrr_always_use_vrr_tg(display) || crtc_state->vrr.enable;

I think we should do this in two separate patches:
 1. use optimzied guardband for intel_vrr_always_use_vrr_tg
 2. use if for vrr.enable as well

Easier to revert just 2. on its own if we have to.

> +}
> +
>  void intel_vrr_compute_guardband(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
>  	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>  	struct drm_display_mode *pipe_mode = &crtc_state->hw.pipe_mode;
> +	int guardband;
>  
>  	if (!intel_vrr_possible(crtc_state))
>  		return;
>  
> -	crtc_state->vrr.guardband = min(crtc_state->vrr.vmin - adjusted_mode->crtc_vdisplay,
> -					intel_vrr_max_guardband(crtc_state));
> +	if (intel_vrr_use_optimized_guardband(crtc_state))
> +		guardband = intel_vrr_compute_optimized_guardband(crtc_state);
> +	else
> +		guardband = crtc_state->vrr.vmin - adjusted_mode->crtc_vdisplay;
> +
> +	crtc_state->vrr.guardband = min(guardband, intel_vrr_max_guardband(crtc_state));
>  
>  	if (intel_vrr_always_use_vrr_tg(display)) {
>  		adjusted_mode->crtc_vblank_start  =
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel