| Message ID | 20220412205527.174685-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/display/psr: Unset enable_psr2_sel_fetch if other checks in intel_psr2_config_valid() fails | expand |
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... 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? > > 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
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
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
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@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 >
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; } /**
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. 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(+)