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

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

On Tue, Oct 07, 2025 at 11:22:44AM +0530, Nautiyal, Ankit K wrote:
> 
> On 10/7/2025 1:26 AM, Ville Syrjälä wrote:
> > 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.
> 
> Alright can skip this stuff.
> 
> 
> >
> >>   	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.
> 
> As far as I understand it depends on guardband for VRR case.
> For non vrr case : crtc_vtotal - crtc_vactive - scl lines
> For vrr case: guardband length.
> 
> Currently since guardband is equal to vblank length this can be 
> crtc_vtotal - crtc_vactive - scl lines.
> 
> Perhaps with the optimized guardband, we need to set the guardband 
> during intel_vrr_compute_config().
> 
> Later intel_psr_compute_config gets called and then we can check the 
> guardband.

Originally we moved the vblank delay calculation to happen later
because we needed to know about PSR for it to be done correctly.
I think someone will need to try to actually write down all the 
requirements from both PSR and VRR side and sides and come up
with a way to get it all done in the right order, without any
more chicken vs. egg problems.

> 
> 
> >
> >>   	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.
> 
> We can use Vblank end.
> 
> Apart from these places, do you think there are more places where 
> vblank_start adjustement is required?
> For evasion logic and wait for push in case of VRR, we are already using 
> vmin_vtotal - guardband to get the delayed vblank start so we are covered.
> 
> Regards,
> 
> Ankit
> 
> >
> > 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