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

[Souza, Jose]

On Thu, 2022-04-14 at 11:08 +0000, Hogander, Jouni wrote:
> 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?

Userspace can make any pipe drive any port, that is what this tests are doing.

> 
> > 
> > > 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 at intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza at 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
>