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

["Souza, Jose" <jose.souza@intel.com>]

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@test-cursor.
> > > > > > > > 
> > > > > > > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > > > > > > Cc: Jouni Hogander <jouni.hogander@intel.com>
> > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@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.

>