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

[Hogander, Jouni]

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?

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
> > > 
>