[PATCH] drm/i915/display: Ensure enough lines between delayed VBlank and VBlank

[Murthy, Arun R]

> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander at intel.com>
> Sent: Tuesday, April 22, 2025 10:16 AM
> To: Murthy, Arun R <arun.r.murthy at intel.com>; intel-xe at lists.freedesktop.org;
> intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/display: Ensure enough lines between delayed
> VBlank and VBlank
> 
> On Sun, 2025-04-20 at 15:50 +0000, Murthy, Arun R wrote:
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf
> > > Of Jouni Högander
> > > Sent: Thursday, April 17, 2025 11:54 PM
> > > To: intel-gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> > > Cc: Hogander, Jouni <jouni.hogander at intel.com>
> > > Subject: [PATCH] drm/i915/display: Ensure enough lines between
> > > delayed VBlank and VBlank
> > >
> > > To deterministically capture the transition of the state machine
> > > going from SRDOFFACK to IDLE, the delayed V. Blank should be at
> > > least one line after the non-delayed V. Blank.
> > >
> > > Ensure this by following instructions from Bspec.
> > >
> > > Bspec: 69897
> > > Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 11 ++++++++++-
> > >  drivers/gpu/drm/i915/display/intel_vrr.c     | 18
> > > ++++++++++++++++--
> > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index db524d01e574d..94156efa5aa93 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -2747,9 +2747,18 @@ static void
> > > intel_set_transcoder_timings_lrr(const
> > > struct intel_crtc_state *crtc
> > >  	}
> > >
> > >  	if (DISPLAY_VER(display) >= 13) {
> > Changes looks good. But per Bspec 69985 looks like this change is not
> > applicable for Xe3+
> 
> How about if I change it like this:
> 
> int min_lat =  intel_vrr_always_use_vrr_tg(display) || crtc_state-
> >vrr.enable ? 1 : 0;
> 
The func intel_vrr_always_use_vrr_tg return true if display >=30, so should it be !intel_vrr_always_use_vrr_tg()

Thanks and Regards,
Arun R Murthy
--------------------
> also guardband could be:
> 
> if (intel_vrr_always_use_vrr_tg(display) || crtc_state->vrr.enable)
>     guardband = max(crtc_state->vrr.vmin - adjusted_mode-
> >crtc_vblank_start, crtc_state->vrr.vmax - adjusted_mode->crtc_vdisplay
> - 1);
> else
>     guardband = crtc_state->vrr.vmin - adjusted_mode-
> >crtc_vblank_start;
> 
> What do you think?
> 
> BR,
> 
> Jouni Högander
> 
> >
> > Thanks and Regards,
> > Arun R Murthy
> > -------------------
> > > +		/*
> > > +		 * Comment on SRD_STATUS register in Bspec:
> > > +		 *
> > > +		 * To deterministically capture the transition of
> > > the state
> > > +		 * machine going from SRDOFFACK to IDLE, the
> > > delayed V. Blank
> > > +		 * should be at least one line after the non-
> > > delayed V. Blank.
> > > +		 *
> > > +		 * Legacy TG: TRANS_SET_CONTEXT_LATENCY > 0
> > > +		 */
> > >  		intel_de_write(display,
> > >  			       TRANS_SET_CONTEXT_LATENCY(display,
> > > cpu_transcoder),
> > > -			       crtc_vblank_start - crtc_vdisplay);
> > > +			       max(crtc_vblank_start -
> > > crtc_vdisplay, 1));
> > >
> > >  		/*
> > >  		 * VBLANK_START not used by hw, just clear it diff --git
> > > a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > index c6565baf815a1..3a27ded45ee04 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > @@ -422,8 +422,22 @@ void intel_vrr_compute_config_late(struct
> > > intel_crtc_state *crtc_state)
> > >  		return;
> > >
> > >  	if (DISPLAY_VER(display) >= 13) {
> > > -		crtc_state->vrr.guardband =
> > > -			crtc_state->vrr.vmin - adjusted_mode-
> > > > crtc_vblank_start;
> > > +		/*
> > > +		 * Comment on SRD_STATUS register in Bspec:
> > > +		 *
> > > +		 * To deterministically capture the transition of
> > > the state
> > > +		 * machine going from SRDOFFACK to IDLE, the
> > > delayed V. Blank
> > > +		 * should be at least one line after the non-
> > > delayed V. Blank.
> > > +		 * This can be done by ensuring the VRR Guardband
> > > programming is
> > > +		 * less than the non-delayed V. Blank.
> > > +		 *
> > > +		 * TRANS_VRR_CTL[ VRR Guardband ] <
> > > (TRANS_VRR_VMAX[
> > > VRR Vmax ]
> > > +		 * - TRANS_VTOTAL[ Vertical Active ])
> > > +		 */
> > > +		crtc_state->vrr.guardband = min(crtc_state-
> > > >vrr.vmin -
> > > +						adjusted_mode-
> > > > crtc_vblank_start,
> > > +						crtc_state-
> > > >vrr.vmax -
> > > +						adjusted_mode-
> > > >crtc_vdisplay
> > > - 1);
> > >  	} else {
> > >  		/* hardware imposes one extra scanline somewhere */
> > >  		crtc_state->vrr.pipeline_full =
> > > --
> > > 2.43.0
> >