Re: [Intel-gfx] [PATCH 2/2] drm/i915/display/psr: Clear more PSR state during disable

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

On Wed, 2022-04-13 at 07:27 +0000, Hogander, Jouni wrote:
> Hello Jose,
> 
> See my comment below.
> 
> On Tue, 2022-04-12 at 13:55 -0700, José Roberto de Souza wrote:
> > After commit 805f04d42a6b ("drm/i915/display/psr: Use continuos full
> > frame to handle frontbuffer invalidations") was merged we started to
> > get some drm_WARN_ON(&dev_priv->drm, !(tmp &
> > PSR2_MAN_TRK_CTL_ENABLE))
> > in tests that are executed in pipe B.
> > 
> > This is probably due psr2_sel_fetch_cff_enabled being left set during
> > PSR disable in the pipe A, so the PSR2_MAN_TRK_CTL write in
> > intel_psr2_program_trans_man_trk_ctl() is skipped in pipe B and then
> > we get the warning when actually enabling PSR after planes
> > programing.
> > We don't get such warnings when running tests in pipe A because
> > PSR2_MAN_TRK_CTL is only cleared when enabling PSR2 with hardware
> > tracking.
> 
> It sounds a bit scary pipe A would have such impact on pipe B...

Because PSR state is stored in intel_dp.

> 
> drm_WARN_ON(&dev_priv->drm, !(tmp & PSR2_MAN_TRK_CTL_ENABLE))
> 
> is wrong for ADLP. Please keep in mind such bit doesn't exist in ADLP.
> This WARN is actually checking SFF bit on ADLP which is reset by HW
> after sending the update frame. We were just lucky (or unlucky
> depending how you see it) not seeing this earlier. Proper fix would be
> to remove this warning for ADLP?

Okay lets start with that, if we see this issue with a tgl then we can bring this patch again.
But I guess it will happen as this issue started right after the PSR CFF patches were merged.

> 
> > 
> > Was not able to reproduce this issue but cleaning the PSR state
> > disable will not harm anything at all.
> > 
> > Fixes: 805f04d42a6b ("drm/i915/display/psr: Use continuos full frame
> > to handle frontbuffer invalidations")
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5634
> > Cc: Jouni Högander <jouni.hogander@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 8ec7c161284be..06db407e2749f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1353,6 +1353,9 @@ static void intel_psr_disable_locked(struct
> > intel_dp *intel_dp)
> >               drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_RECEIVER_ALPM_CONFIG, 0);
> > 
> >       intel_dp->psr.enabled = false;
> > +     intel_dp->psr.psr2_enabled = false;
> > +     intel_dp->psr.psr2_sel_fetch_enabled = false;
> > +     intel_dp->psr.psr2_sel_fetch_cff_enabled = false;
> >  }
> > 
> >  /**
> 
> BR,
> 
> Jouni Högander