Re: [Intel-gfx] [PATCH 2/4] drm/i915/vrr: Fix guardband/vblank exit length calculation for adl+

["Navare, Manasi" <manasi.d.navare@intel.com>]

On Wed, Dec 07, 2022 at 05:10:54PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 05, 2022 at 12:34:25PM -0800, Navare, Manasi wrote:
> > On Fri, Dec 02, 2022 at 03:44:10PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We are miscalculating both the guardband value, and the resulting
> > > vblank exit length on adl+. This means that our start of vblank
> > > (double buffered register latch point) is incorrect, and we also
> > > think that it's not where it actually is (hence vblank evasion/etc.
> > > may not work properly). Fix up the calculations to match the real
> > > hardware behaviour (as reverse engineered by intel_display_poller).
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_vrr.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > index 6655dd2c1684..753e7b211708 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > > @@ -78,10 +78,10 @@ static int intel_vrr_vblank_exit_length(const struct intel_crtc_state *crtc_stat
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >  	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > >  
> > > -	/* The hw imposes the extra scanline before frame start */
> > >  	if (DISPLAY_VER(i915) >= 13)
> > > -		return crtc_state->vrr.guardband + crtc_state->framestart_delay + 1;
> > > +		return crtc_state->vrr.guardband;
> > 
> > This makes sense since with guardband, there is no framestart delay
> 
> framestart delay is still a thing. But it's not something that
> affects how the hardware interprets the guardband value.
> 
> > 
> > >  	else
> > > +		/* The hw imposes the extra scanline before frame start */
> > >  		return crtc_state->vrr.pipeline_full + crtc_state->framestart_delay + 1;
> > >  }
> > >  
> > > @@ -151,7 +151,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
> > >  		 * number of scan lines. Assuming 0 for no DSB.
> > >  		 */
> > >  		crtc_state->vrr.guardband =
> > > -			crtc_state->vrr.vmin - adjusted_mode->crtc_vdisplay;
> > > +			crtc_state->vrr.vmin + 1 - adjusted_mode->crtc_vdisplay;
> > 
> > Why are we adding + 1 here? The bspec says guardband should be :
> > Guardband = Vmin - Vactive - Window2 where in our case Window2 = 0
> > If we need that + 1 to get this working, then perhaps we need to update
> > Bspec?
> 
> flipline is what actaully determines the start of vblank, and
> 'flipline>=vmin+1' always.

Flipline would be always >=vmin as per the bspec, have we tried with
that or if that definitely doesnt work then we need to have this changed
in the bspec.

Either way if this is the only value that works then with this change
added to bspec:

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> 
> > 
> > I kind of want to see if this is still breaking if we dont have that +
> > 1?
> 
> Without it start of vblank happens one line later than where we want it
> to happen.
> 
> > 
> > Manasi
> > 
> > >  	} else {
> > >  		crtc_state->vrr.pipeline_full =
> > >  			min(255, crtc_state->vrr.vmin - adjusted_mode->crtc_vdisplay -
> > > -- 
> > > 2.37.4
> > > 
> 
> -- 
> Ville Syrjälä
> Intel