[PATCH 4/6] drm/i915: Eliminate extra frame from skl-glk sync->async flip change

[Murthy, Arun R]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Friday, April 19, 2024 10:12 PM
> To: Murthy, Arun R <arun.r.murthy at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 4/6] drm/i915: Eliminate extra frame from skl-glk sync-
> >async flip change
> 
> On Fri, Apr 19, 2024 at 06:39:48AM +0000, Murthy, Arun R wrote:
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf
> > > Of Ville Syrjala
> > > Sent: Wednesday, March 20, 2024 9:34 PM
> > > To: intel-gfx at lists.freedesktop.org
> > > Subject: [PATCH 4/6] drm/i915: Eliminate extra frame from skl-glk
> > > sync->async flip change
> > >
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > > On bdw-glk the sync->async flip change takes an extra frame due to
> > > the double buffering behaviour of the async flip plane control bit.
> > >
> > > Since on skl+ we are now explicitly converting the first async flip
> > > to a sync flip (in order to allow changing the modifier and/or
> > > ddb/watermarks) we are now taking two extra frames until async flips
> > > are actually active. We can drop that back down to one frame by
> > > setting the async flip bit already during the sync flip.
> > >
> > > Note that on bdw we don't currently do the extra sync flip (see
> > > intel_plane_do_async_flip()) so technically we wouldn't have to deal
> > > with this in i9xx_plane_update_arm(). But I added the relevant
> > > snippet of code there as well, just in case we ever decide to go for
> > > the extra sync flip on pre-skl platforms as well (we might, for example, want
> to change the fb stride).
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > Logically changes looks good. I see failures in CI.IGT Better to have
> > this green or a Tested-by would be good.
> 
> Those are fixed by https://patchwork.freedesktop.org/series/131518/
> whereas this got tested against the previous version of the igt changes. I'll
> repost to test against the latest igt changes.
> IIRC one can't just reply to the cover letter with a new Test-with :(
> 
Will wait for the CI IGT to get green on this.

Thanks and Regards,
Arun R Murthy
--------------------

> >
> > Thanks and Regards,
> > Arun R Murthy
> > -------------------
> > > ---
> > >  drivers/gpu/drm/i915/display/i9xx_plane.c         |  5 +++++
> > >  drivers/gpu/drm/i915/display/intel_atomic_plane.c | 15 +++++++++++----
> > >  .../gpu/drm/i915/display/skl_universal_plane.c    |  5 +++++
> > >  3 files changed, 21 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > > b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > > index 0279c8aabdd1..76fc7626051b 100644
> > > --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > > @@ -455,6 +455,11 @@ static void i9xx_plane_update_arm(struct
> > > intel_plane *plane,
> > >
> > >  	dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
> > >
> > > +	/* see intel_plane_atomic_calc_changes() */
> > > +	if (plane->need_async_flip_disable_wa &&
> > > +	    crtc_state->async_flip_planes & BIT(plane->id))
> > > +		dspcntr |= DISP_ASYNC_FLIP;
> > > +
> > >  	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> > >
> > >  	if (DISPLAY_VER(dev_priv) >= 4)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > index 769010d0ebc4..7098a34a17c8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > @@ -437,10 +437,6 @@ static bool intel_plane_do_async_flip(struct
> > > intel_plane *plane,
> > >  	 * only X-tile is supported with async flips, though we could
> > >  	 * extend this so other scanout parameters (stride/etc) could
> > >  	 * be changed as well...
> > > -	 *
> > > -	 * FIXME: Platforms with need_async_flip_disable_wa==true will
> > > -	 * now end up doing two sync flips initially. Would be nice to
> > > -	 * combine those into just the one sync flip...
> > >  	 */
> > >  	return DISPLAY_VER(i915) < 9 || old_crtc_state->uapi.async_flip;
> > > } @@
> > > -604,6 +600,17 @@ static int intel_plane_atomic_calc_changes(const
> > > struct intel_crtc_state *old_cr
> > >  	if (intel_plane_do_async_flip(plane, old_crtc_state, new_crtc_state)) {
> > >  		new_crtc_state->do_async_flip = true;
> > >  		new_crtc_state->async_flip_planes |= BIT(plane->id);
> > > +	} else if (plane->need_async_flip_disable_wa &&
> > > +		   new_crtc_state->uapi.async_flip) {
> > > +		/*
> > > +		 * On platforms with double buffered async flip bit we
> > > +		 * set the bit already one frame early during the sync
> > > +		 * flip (see {i9xx,skl}_plane_update_arm()). The
> > > +		 * hardware will therefore be ready to perform a real
> > > +		 * async flip during the next commit, without having
> > > +		 * to wait yet another frame for the bit to latch.
> > > +		 */
> > > +		new_crtc_state->async_flip_planes |= BIT(plane->id);
> > >  	}
> > >
> > >  	return 0;
> > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > index 860574d04f88..ad4c90344f68 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > @@ -1174,6 +1174,11 @@ skl_plane_update_arm(struct intel_plane
> *plane,
> > >  	plane_ctl = plane_state->ctl |
> > >  		skl_plane_ctl_crtc(crtc_state);
> > >
> > > +	/* see intel_plane_atomic_calc_changes() */
> > > +	if (plane->need_async_flip_disable_wa &&
> > > +	    crtc_state->async_flip_planes & BIT(plane->id))
> > > +		plane_ctl |= PLANE_CTL_ASYNC_FLIP;
> > > +
> > >  	if (DISPLAY_VER(dev_priv) >= 10)
> > >  		plane_color_ctl = plane_state->color_ctl |
> > >  			glk_plane_color_ctl_crtc(crtc_state);
> > > --
> > > 2.43.2
> >
> 
> --
> Ville Syrjälä
> Intel