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 <[email protected]>
> ---
> 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
