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

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Wed, 2022-04-13 at 15:59 +0000, Souza, Jose wrote:
> 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.

Is intel_dp shared between pipes?

> 
> > 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?

Checked today this and found out that my comment is not valid. I.e.
this bit is not cleared by HW. This was actually partial frame bit
which is _not_ reset by the HW.

Still for clarity this check should be modified as
PSR2_MAN_TRK_CTL_ENABLE doesn't exist 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.

You are right. We are probably bitten by this later. I'm sorry for
mixing the single full frame and the partial frame bits.

> 
> > > 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