[Intel-gfx] [PATCH] drm/i915/psr2: Do full fetches when doing async flips

[Souza, Jose]

On Fri, 2021-10-29 at 22:55 +0000, Souza, Jose wrote:
> On Fri, 2021-10-29 at 09:22 +0300, Ville Syrjälä wrote:
> > On Thu, Oct 28, 2021 at 08:18:48PM +0000, Souza, Jose wrote:
> > > On Thu, 2021-10-28 at 20:46 +0300, Ville Syrjälä wrote:
> > > > On Thu, Oct 28, 2021 at 05:43:51PM +0000, Souza, Jose wrote:
> > > > > On Thu, 2021-10-28 at 20:38 +0300, Ville Syrjälä wrote:
> > > > > > On Thu, Oct 28, 2021 at 05:02:41PM +0000, Souza, Jose wrote:
> > > > > > > On Thu, 2021-10-28 at 16:32 +0300, Ville Syrjälä wrote:
> > > > > > > > On Wed, Oct 27, 2021 at 11:48:55AM -0700, José Roberto de Souza wrote:
> > > > > > > > > Async flips are not supported by selective fetch and we had a check
> > > > > > > > > for that but that check was only executed when doing modesets.
> > > > > > > > > So moving this check to the page flip path, so it can be properly
> > > > > > > > > handled.
> > > > > > > > > 
> > > > > > > > > This fix a failure in kms_async_flips at test-cursor.
> > > > > > > > > 
> > > > > > > > > Cc: Mika Kahola <mika.kahola at intel.com>
> > > > > > > > > Cc: Jouni Hogander <jouni.hogander at intel.com>
> > > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/i915/display/intel_psr.c | 8 ++------
> > > > > > > > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > index 8d08e3cf08c1f..ce6850ed72c60 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > > @@ -729,12 +729,6 @@ static bool intel_psr2_sel_fetch_config_valid(struct intel_dp *intel_dp,
> > > > > > > > >  		return false;
> > > > > > > > >  	}
> > > > > > > > >  
> > > > > > > > > -	if (crtc_state->uapi.async_flip) {
> > > > > > > > > -		drm_dbg_kms(&dev_priv->drm,
> > > > > > > > > -			    "PSR2 sel fetch not enabled, async flip enabled\n");
> > > > > > > > > -		return false;
> > > > > > > > > -	}
> > > > > > > > > -
> > > > > > > > >  	/* Wa_14010254185 Wa_14010103792 */
> > > > > > > > >  	if (IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) {
> > > > > > > > >  		drm_dbg_kms(&dev_priv->drm,
> > > > > > > > > @@ -1592,6 +1586,8 @@ static bool psr2_sel_fetch_pipe_state_supported(const struct intel_crtc_state *c
> > > > > > > > >  {
> > > > > > > > >  	if (crtc_state->scaler_state.scaler_id >= 0)
> > > > > > > > >  		return false;
> > > > > > > > > +	if (crtc_state->uapi.async_flip)
> > > > > > > > > +		return false;
> > > > > > > > 
> > > > > > > > This looks dodgy. Pretty sure we can't turn off this thing during
> > > > > > > > an async flip. So I think the correct short term fix is to not do
> > > > > > > > async flips with psr2 enabled. The longer term fix would involve
> > > > > > > > using the same approach Stan is preparing for the async flip
> > > > > > > > watermark tweaking, which is to convert the first async flip into
> > > > > > > > a sync flip.
> > > 
> > > You mean do something like this?
> > > 
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 3b5a8e971343f..7d29f8c9de0da 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -7710,7 +7710,8 @@ static void kill_bigjoiner_slave(struct intel_atomic_state *state,
> > >  static int intel_atomic_check_async(struct intel_atomic_state *state, struct intel_crtc *crtc)
> > >  {
> > >         struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > -       const struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> > > +       const struct intel_crtc_state *old_crtc_state;
> > > +       struct intel_crtc_state *new_crtc_state;
> > >         const struct intel_plane_state *new_plane_state, *old_plane_state;
> > >         struct intel_plane *plane;
> > >         int i;
> > > @@ -7718,6 +7719,12 @@ static int intel_atomic_check_async(struct intel_atomic_state *state, struct int
> > >         old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> > >         new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > > 
> > > +       if (new_crtc_state->enable_psr2_sel_fetch) {
> > > +               drm_dbg_kms(&i915->drm, "PSR2 selective fetch not compatible with async flip, doing a sync flip instead\n");
> > > +               new_crtc_state->uapi.async_flip = false;
> > > +               return 0;
> > > +       }
> > 
> > It should just return -EINVAL here. And I'd put the somewhere after the
> > needs_modeset/hw.active checks to keep things in some kind of
> > reasonable order.
> 
> Okay, easy do that but that would not cause any issues for desktop environments?
> We advertise async flip capability but state will always be rejected when PSR2 is enabled.
> 
> Will also need to switch to PSR1 or skip all kms_async_flip tests when PSR2 selective fetch is enabled.

Please disregard the above comment and this whole patch.
Did more debug and found better ways to solve all async flip + PSR problems.


IGT patches will change the failure to skip as we don't have a way around it: https://patchwork.freedesktop.org/series/96439/
Kernel patch fixing possible corruptions with async flip: https://patchwork.freedesktop.org/series/96440/


> 
> > 
>