| Message ID | 20240320160424.700-5-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Allow the first async flip to change modifier | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville > Syrjala > Sent: Wednesday, March 20, 2024 9:34 PM > To: intel-gfx@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@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@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. 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
On Fri, Apr 19, 2024 at 06:39:48AM +0000, Murthy, Arun R wrote: > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville > > Syrjala > > Sent: Wednesday, March 20, 2024 9:34 PM > > To: intel-gfx@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@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@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 :( > > 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 >
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Friday, April 19, 2024 10:12 PM > To: Murthy, Arun R <arun.r.murthy@intel.com> > Cc: intel-gfx@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@lists.freedesktop.org> On Behalf > > > Of Ville Syrjala > > > Sent: Wednesday, March 20, 2024 9:34 PM > > > To: intel-gfx@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@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@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
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);