[Intel-gfx] [PATCH 1/2] drm/i915/display/psr: Unset enable_psr2_sel_fetch if other checks in intel_psr2_config_valid() fails

[Intel-gfx] [PATCH 1/2] drm/i915/display/psr: Unset enable_psr2_sel_fetch if other checks in intel_psr2_config_valid() fails

[José Roberto de Souza]

If any of the PSR2 checks after intel_psr2_sel_fetch_config_valid()
fails, enable_psr2_sel_fetch will be kept enabled causing problems
in the functions that only checks for it and not for has_psr2.

So here moving the check that do not depend on enable_psr2_sel_fetch
and for the remaning ones jumping to a section that unset
enable_psr2_sel_fetch in case of failure to support PSR2.

Fixes: 6e43e276b8c9 ("drm/i915: Initial implementation of PSR2 selective fetch")
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 | 38 +++++++++++++-----------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 5a55010a9b2f7..8ec7c161284be 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -891,6 +891,20 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 		return false;
 	}
 
+	/* Wa_16011303918:adl-p */
+	if (crtc_state->vrr.enable &&
+	    IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) {
+		drm_dbg_kms(&dev_priv->drm,
+			    "PSR2 not enabled, not compatible with HW stepping + VRR\n");
+		return false;
+	}
+
+	if (!_compute_psr2_sdp_prior_scanline_indication(intel_dp, crtc_state)) {
+		drm_dbg_kms(&dev_priv->drm,
+			    "PSR2 not enabled, PSR2 SDP indication do not fit in hblank\n");
+		return false;
+	}
+
 	if (HAS_PSR2_SEL_FETCH(dev_priv)) {
 		if (!intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state) &&
 		    !HAS_PSR_HW_TRACKING(dev_priv)) {
@@ -904,12 +918,12 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 	if (!crtc_state->enable_psr2_sel_fetch &&
 	    IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) {
 		drm_dbg_kms(&dev_priv->drm, "PSR2 HW tracking is not supported this Display stepping\n");
-		return false;
+		goto unsupported;
 	}
 
 	if (!psr2_granularity_check(intel_dp, crtc_state)) {
 		drm_dbg_kms(&dev_priv->drm, "PSR2 not enabled, SU granularity not compatible\n");
-		return false;
+		goto unsupported;
 	}
 
 	if (!crtc_state->enable_psr2_sel_fetch &&
@@ -918,25 +932,15 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 			    "PSR2 not enabled, resolution %dx%d > max supported %dx%d\n",
 			    crtc_hdisplay, crtc_vdisplay,
 			    psr_max_h, psr_max_v);
-		return false;
-	}
-
-	if (!_compute_psr2_sdp_prior_scanline_indication(intel_dp, crtc_state)) {
-		drm_dbg_kms(&dev_priv->drm,
-			    "PSR2 not enabled, PSR2 SDP indication do not fit in hblank\n");
-		return false;
-	}
-
-	/* Wa_16011303918:adl-p */
-	if (crtc_state->vrr.enable &&
-	    IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) {
-		drm_dbg_kms(&dev_priv->drm,
-			    "PSR2 not enabled, not compatible with HW stepping + VRR\n");
-		return false;
+		goto unsupported;
 	}
 
 	tgl_dc3co_exitline_compute_config(intel_dp, crtc_state);
 	return true;
+
+unsupported:
+	crtc_state->enable_psr2_sel_fetch = false;
+	return false;
 }
 
 void intel_psr_compute_config(struct intel_dp *intel_dp,
-- 
2.35.1

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

[José Roberto de Souza]

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(+)

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;
 }
 
 /**
-- 
2.35.1

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

[Hogander, Jouni]

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

Re: [Intel-gfx] [PATCH 1/2] drm/i915/display/psr: Unset enable_psr2_sel_fetch if other checks in intel_psr2_config_valid() fails

[Hogander, Jouni]

On Tue, 2022-04-12 at 13:55 -0700, José Roberto de Souza wrote:
> If any of the PSR2 checks after intel_psr2_sel_fetch_config_valid()
> fails, enable_psr2_sel_fetch will be kept enabled causing problems
> in the functions that only checks for it and not for has_psr2.
> 
> So here moving the check that do not depend on enable_psr2_sel_fetch
> and for the remaning ones jumping to a section that unset
> enable_psr2_sel_fetch in case of failure to support PSR2.

Reviewed-by: Jouni Högander <jouni.hogander@intel.com>

> 
> Fixes: 6e43e276b8c9 ("drm/i915: Initial implementation of PSR2
> selective fetch")
> 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 | 38 +++++++++++++---------
> --
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 5a55010a9b2f7..8ec7c161284be 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -891,6 +891,20 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	/* Wa_16011303918:adl-p */
> +	if (crtc_state->vrr.enable &&
> +	    IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) {
> +		drm_dbg_kms(&dev_priv->drm,
> +			    "PSR2 not enabled, not compatible with HW
> stepping + VRR\n");
> +		return false;
> +	}
> +
> +	if (!_compute_psr2_sdp_prior_scanline_indication(intel_dp,
> crtc_state)) {
> +		drm_dbg_kms(&dev_priv->drm,
> +			    "PSR2 not enabled, PSR2 SDP indication do
> not fit in hblank\n");
> +		return false;
> +	}
> +
>  	if (HAS_PSR2_SEL_FETCH(dev_priv)) {
>  		if (!intel_psr2_sel_fetch_config_valid(intel_dp,
> crtc_state) &&
>  		    !HAS_PSR_HW_TRACKING(dev_priv)) {
> @@ -904,12 +918,12 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  	if (!crtc_state->enable_psr2_sel_fetch &&
>  	    IS_TGL_DISPLAY_STEP(dev_priv, STEP_A0, STEP_C0)) {
>  		drm_dbg_kms(&dev_priv->drm, "PSR2 HW tracking is not
> supported this Display stepping\n");
> -		return false;
> +		goto unsupported;
>  	}
>  
>  	if (!psr2_granularity_check(intel_dp, crtc_state)) {
>  		drm_dbg_kms(&dev_priv->drm, "PSR2 not enabled, SU
> granularity not compatible\n");
> -		return false;
> +		goto unsupported;
>  	}
>  
>  	if (!crtc_state->enable_psr2_sel_fetch &&
> @@ -918,25 +932,15 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  			    "PSR2 not enabled, resolution %dx%d > max
> supported %dx%d\n",
>  			    crtc_hdisplay, crtc_vdisplay,
>  			    psr_max_h, psr_max_v);
> -		return false;
> -	}
> -
> -	if (!_compute_psr2_sdp_prior_scanline_indication(intel_dp,
> crtc_state)) {
> -		drm_dbg_kms(&dev_priv->drm,
> -			    "PSR2 not enabled, PSR2 SDP indication do
> not fit in hblank\n");
> -		return false;
> -	}
> -
> -	/* Wa_16011303918:adl-p */
> -	if (crtc_state->vrr.enable &&
> -	    IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) {
> -		drm_dbg_kms(&dev_priv->drm,
> -			    "PSR2 not enabled, not compatible with HW
> stepping + VRR\n");
> -		return false;
> +		goto unsupported;
>  	}
>  
>  	tgl_dc3co_exitline_compute_config(intel_dp, crtc_state);
>  	return true;
> +
> +unsupported:
> +	crtc_state->enable_psr2_sel_fetch = false;
> +	return false;
>  }
>  
>  void intel_psr_compute_config(struct intel_dp *intel_dp,

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/display/psr: Unset enable_psr2_sel_fetch if other checks in intel_psr2_config_valid() fails

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6162 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/i915/display/psr: Unset enable_psr2_sel_fetch if other checks in intel_psr2_config_valid() fails
URL   : https://patchwork.freedesktop.org/series/102615/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11494 -> Patchwork_102615v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/index.html

Participating hosts (47 -> 37)
------------------------------

  Additional (1): fi-icl-u2 
  Missing    (11): bat-dg1-6 bat-dg1-5 bat-dg2-8 fi-bsw-cyan bat-adlp-6 bat-adlp-4 bat-hsw-1 bat-rpls-1 bat-rpls-2 bat-jsl-2 bat-jsl-1 

Known issues
------------

  Here are the changes found in Patchwork_102615v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-icl-u2:          NOTRUN -> [SKIP][1] ([i915#2190])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-icl-u2/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - fi-icl-u2:          NOTRUN -> [SKIP][2] ([i915#4613]) +3 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-icl-u2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@kms_busy@basic@flip:
    - fi-tgl-u2:          [PASS][3] -> [DMESG-WARN][4] ([i915#402]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11494/fi-tgl-u2/igt@kms_busy@basic@flip.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-tgl-u2/igt@kms_busy@basic@flip.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          NOTRUN -> [SKIP][5] ([fdo#111827]) +8 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-icl-u2:          NOTRUN -> [SKIP][6] ([fdo#109278]) +2 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-icl-u2:          NOTRUN -> [SKIP][7] ([fdo#109285])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-icl-u2/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b:
    - fi-cfl-8109u:       [PASS][8] -> [DMESG-WARN][9] ([i915#62]) +11 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11494/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c:
    - fi-cfl-8109u:       [PASS][10] -> [DMESG-WARN][11] ([i915#5341] / [i915#62])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11494/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-icl-u2:          NOTRUN -> [SKIP][12] ([i915#3555])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-icl-u2/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-userptr:
    - fi-icl-u2:          NOTRUN -> [SKIP][13] ([i915#3301])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-icl-u2/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-bdw-5557u:       NOTRUN -> [FAIL][14] ([i915#4312])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-bdw-5557u/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [INCOMPLETE][15] ([i915#4785]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11494/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
    - fi-snb-2600:        [INCOMPLETE][17] ([i915#3921]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11494/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

  
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
  [i915#5341]: https://gitlab.freedesktop.org/drm/intel/issues/5341
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62


Build changes
-------------

  * Linux: CI_DRM_11494 -> Patchwork_102615v1

  CI-20190529: 20190529
  CI_DRM_11494: 904dda652093b27e67e3ac8370d8eec7893c5c02 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6420: a3885810ccc0ce9e6552a20c910a0a322eca466c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_102615v1: 102615v1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

48daed419db9 drm/i915/display/psr: Clear more PSR state during disable
d67a047a5e2b drm/i915/display/psr: Unset enable_psr2_sel_fetch if other checks in intel_psr2_config_valid() fails

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102615v1/index.html

[-- Attachment #2: Type: text/html, Size: 7165 bytes --]

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

[Souza, Jose]

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

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

[Hogander, Jouni]

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

Re: [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@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
>