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