diff mbox series

[2/4] drm/i915/vrr: Fix guardband/vblank exit length calculation for adl+

Message ID 20221202134412.21943-3-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/vrr: VRR fixes | expand

Commit Message

Ville Syrjälä Dec. 2, 2022, 1:44 p.m. UTC 
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(-)

Comments

Navare, Manasi Dec. 5, 2022, 8:34 p.m. UTC | #1 
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

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

I kind of want to see if this is still breaking if we dont have that +
1?

Manasi

>  	} else {
>  		crtc_state->vrr.pipeline_full =
>  			min(255, crtc_state->vrr.vmin - adjusted_mode->crtc_vdisplay -
> -- 
> 2.37.4
>
Ville Syrjälä Dec. 7, 2022, 3:10 p.m. UTC | #2 
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.

> 
> 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
> >
Navare, Manasi Dec. 7, 2022, 9:05 p.m. UTC | #3 
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
Ville Syrjälä Dec. 7, 2022, 9:35 p.m. UTC | #4 
On Wed, Dec 07, 2022 at 01:05:15PM -0800, Navare, Manasi wrote:
> 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,

Not sure where in bspec you see that. All I see is >vmin,
and it even says you et an extra line if you try to set them
equal. Pretty sure I verified that behaviour on the hw on icl/tgl
since I put the extra -1 to the vmin calculation. Though I haven't
actually tested it on adl+.

> 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
Navare, Manasi Dec. 8, 2022, 6:42 p.m. UTC | #5 
On Wed, Dec 07, 2022 at 11:35:24PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 07, 2022 at 01:05:15PM -0800, Navare, Manasi wrote:
> > 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,
> 
> Not sure where in bspec you see that. All I see is >vmin,
> and it even says you et an extra line if you try to set them
> equal. Pretty sure I verified that behaviour on the hw on icl/tgl
> since I put the extra -1 to the vmin calculation. Though I haven't
> actually tested it on adl+.

You are right for these current platforms it still needs >=Vmin + 1.

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

Manasi

> 
> > 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
> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

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;
 	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;
 	} else {
 		crtc_state->vrr.pipeline_full =
 			min(255, crtc_state->vrr.vmin - adjusted_mode->crtc_vdisplay -