[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 11:08 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 Tue, 2025-04-22 at 04:59 +0000, Murthy, Arun R wrote:
> > > -----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()
> 
> Yes, that is wrong in my proposal. Do you agree generally the change?
> 
Yes looks good!

Thanks and Regards,
Arun R Murthy
--------------------
> BR,
> 
> Jouni Högander
> 
> >
> > 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
> > > >
> >