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

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

On Fri, Oct 17, 2025 at 10:32:01AM +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.
> 
> v2:
> - Use max of sagv latency and skl_wm_latency(1) for PM delay
>   computation. (Ville)
> - Avoid guardband optimization for HDMI for now. (Ville)
> - Allow guardband optimization only for platforms with
>   intel_vrr_always_use_vrr_tg = true. (Ville)
> - Add comments for PM delay and a #TODO note for HDMI.
> 
> Bspec: 70151
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 62 +++++++++++++++++++++++-
>  1 file changed, 60 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..cd7bed358984 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,71 @@ 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 prefill_min_guardband;
> +	int prefill_latency_us;
> +	int guardband = 0;
> +
> +	skl_prefill_init_worst(&prefill_ctx, crtc_state);
> +
> +	/*
> +	 * The SoC power controller runs SAGV mutually exclusive with package C states,
> +	 * so the max of package C and SAGV latencies is used to compute the min prefill guardband.
> +	 * PM delay = max(sagv_latency, pkgc_max_latency (highest enabled wm level 1 and up))
> +	 */
> +	prefill_latency_us = max(display->sagv.block_time_us,
> +				 skl_watermark_max_latency(display, 1));
> +	prefill_min_guardband =
> +		skl_prefill_min_guardband(&prefill_ctx,
> +					  crtc_state,
> +					  prefill_latency_us);

This could be just

guardband = skl_prefill_min_guardband(...)

and you can then get rid of the prefill_min_guardband
variable as well.

But in general lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	if (intel_crtc_has_dp_encoder(crtc_state)) {
> +		guardband = max(guardband, intel_psr_min_guardband(crtc_state));
> +		guardband = max(guardband, intel_dp_sdp_min_guardband(crtc_state, true));
> +	}
> +
> +	guardband = max(guardband, prefill_min_guardband);
> +
> +	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);
> +
> +	/*
> +	 * #TODO: Enable optimized guardband for HDMI
> +	 * For HDMI lot of infoframes are transmitted a line or two after vsync.
> +	 * Since with optimized guardband the double bufferring point is at delayed vblank,
> +	 * we need to ensure that vsync happens after delayed vblank for the HDMI case.
> +	 */
> +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> +		return false;
> +
> +	return intel_vrr_always_use_vrr_tg(display);
> +}
> +
>  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