> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Sent: Friday, April 19, 2024 10:12 PM > To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxxxxxxxx> On Behalf > > > Of Ville Syrjala > > > Sent: Wednesday, March 20, 2024 9:34 PM > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > Subject: [PATCH 4/6] drm/i915: Eliminate extra frame from skl-glk > > > sync->async flip change > > > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxxxx> > > > > 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
