Re: [PATCH 11/16] drm/i915/vblank: Add helper to get correct vblank length

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

On Mon, Oct 06, 2025 at 09:58:47AM +0530, Ankit Nautiyal wrote:
> Currently crtc_vblank_start is assumed to be the vblank_start for the fixed
> refresh rate case. That value can be different from the variable refresh
> rate case whenever always_use_vrr_tg()==false. On icl/tgl it's always
> different due to the extra vblank delay, and also on adl+ it could be
> different if we were to use an optimized guardband.
> 
> So places where crtc_vblank_start is used to compute vblank length needs
> change so as to account for cases where vrr is enabled. Specifically
> with vrr.enable the effective vblank length is actually guardband.
> 
> Add a helper to get the correct vblank length for both vrr and fixed
> refresh rate cases. Use this helper where vblank_start is used to
> compute the vblank length.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_pfit.c    | 11 +++++++----
>  drivers/gpu/drm/i915/display/intel_psr.c     |  3 +--
>  drivers/gpu/drm/i915/display/intel_vblank.c  | 10 ++++++++++
>  drivers/gpu/drm/i915/display/intel_vblank.h  |  2 ++
>  drivers/gpu/drm/i915/display/skl_watermark.c |  3 ++-
>  5 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_pfit.c b/drivers/gpu/drm/i915/display/intel_pfit.c
> index 68539e7c2a24..ebbaa1d419ba 100644
> --- a/drivers/gpu/drm/i915/display/intel_pfit.c
> +++ b/drivers/gpu/drm/i915/display/intel_pfit.c
> @@ -14,6 +14,7 @@
>  #include "intel_lvds_regs.h"
>  #include "intel_pfit.h"
>  #include "intel_pfit_regs.h"
> +#include "intel_vblank.h"
>  #include "skl_scaler.h"
>  
>  static int intel_pch_pfit_check_dst_window(const struct intel_crtc_state *crtc_state)
> @@ -306,14 +307,15 @@ centre_horizontally(struct drm_display_mode *adjusted_mode,
>  }
>  
>  static void
> -centre_vertically(struct drm_display_mode *adjusted_mode,
> +centre_vertically(struct intel_crtc_state *crtc_state,
> +		  struct drm_display_mode *adjusted_mode,
>  		  int height)
>  {
>  	u32 border, sync_pos, blank_width, sync_width;
>  
>  	/* keep the vsync and vblank widths constant */
>  	sync_width = adjusted_mode->crtc_vsync_end - adjusted_mode->crtc_vsync_start;
> -	blank_width = adjusted_mode->crtc_vblank_end - adjusted_mode->crtc_vblank_start;
> +	blank_width = intel_crtc_vblank_length(crtc_state);

This pfit stuff is computed way before the guardband, and also only
relevant for ancient gen2-4 hardware. So no point in touching this
stuff IMO.

>  	sync_pos = (blank_width - sync_width + 1) / 2;
>  
>  	border = (adjusted_mode->crtc_vdisplay - height + 1) / 2;
> @@ -392,7 +394,8 @@ static void i9xx_scale_aspect(struct intel_crtc_state *crtc_state,
>  					  PFIT_HORIZ_INTERP_BILINEAR);
>  		}
>  	} else if (scaled_width < scaled_height) { /* letter */
> -		centre_vertically(adjusted_mode,
> +		centre_vertically(crtc_state,
> +				  adjusted_mode,
>  				  scaled_width / pipe_src_w);
>  
>  		*border = LVDS_BORDER_ENABLE;
> @@ -489,7 +492,7 @@ static int gmch_panel_fitting(struct intel_crtc_state *crtc_state,
>  		 * heights and modify the values programmed into the CRTC.
>  		 */
>  		centre_horizontally(adjusted_mode, pipe_src_w);
> -		centre_vertically(adjusted_mode, pipe_src_h);
> +		centre_vertically(crtc_state, adjusted_mode, pipe_src_h);
>  		border = LVDS_BORDER_ENABLE;
>  		break;
>  	case DRM_MODE_SCALE_ASPECT:
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index f7115969b4c5..ae6b94a5d450 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1365,8 +1365,7 @@ static bool wake_lines_fit_into_vblank(struct intel_dp *intel_dp,
>  				       bool aux_less)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
> -	int vblank = crtc_state->hw.adjusted_mode.crtc_vblank_end -
> -		crtc_state->hw.adjusted_mode.crtc_vblank_start;
> +	int vblank = intel_crtc_vblank_length(crtc_state);

I *think* this also gets computed during .compute_config() which is
before the guardband calculation. So if this stuff actually depends on
the guardband then we have a real problem here. And if it doesn't (as
in it really interested in the undelayed vblank length) them maybe it
should just compute it as crtc_vtotal-crtc_vactive.

>  	int wake_lines;
>  
>  	if (aux_less)
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 0b7fcc05e64c..2fc0c1c0bb87 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -767,3 +767,13 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  
>  	return scanline;
>  }
> +
> +int intel_crtc_vblank_length(const struct intel_crtc_state *crtc_state)
> +{
> +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	if (crtc_state->vrr.enable)
> +		return crtc_state->vrr.guardband;
> +	else
> +		return adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vblank_start;

Maybe that should be crtc_vblank_end instead of crtc_vtotal? I guess it
doesn't actually matter given where this gets used.

I think the only case where vblank_end!=vtotal is exactly than ancient
gen2-4 pfit centering stuff. But I've never actually investigated
whether the exact value of vblank_end there even matters. I should do
that one day...

> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.h b/drivers/gpu/drm/i915/display/intel_vblank.h
> index 21fbb08d61d5..98d04cacd65f 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.h
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.h
> @@ -48,4 +48,6 @@ const struct intel_crtc_state *
>  intel_pre_commit_crtc_state(struct intel_atomic_state *state,
>  			    struct intel_crtc *crtc);
>  
> +int intel_crtc_vblank_length(const struct intel_crtc_state *crtc_state);
> +
>  #endif /* __INTEL_VBLANK_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 73e5b2d8ae83..6fb2c78fe29b 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -28,6 +28,7 @@
>  #include "intel_flipq.h"
>  #include "intel_pcode.h"
>  #include "intel_plane.h"
> +#include "intel_vblank.h"
>  #include "intel_wm.h"
>  #include "skl_universal_plane_regs.h"
>  #include "skl_scaler.h"
> @@ -2171,7 +2172,7 @@ skl_is_vblank_too_short(const struct intel_crtc_state *crtc_state,
>  	return crtc_state->framestart_delay +
>  		intel_usecs_to_scanlines(adjusted_mode, latency) +
>  		wm0_lines >
> -		adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vblank_start;
> +		intel_crtc_vblank_length(crtc_state);
>  }
>  
>  int skl_max_wm0_lines(const struct intel_crtc_state *crtc_state)
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel