[PATCH v2 0/2] Enable_psr kernel parameter changes

[PATCH v2 0/2] Enable_psr kernel parameter changes

[Jouni Högander]

Currently disabling PSR2 via enable_psr module parameter causes Panel
Replay being disabled as well. This patch changes this by still allowing
Panel Replay even if PSR2 is disabled.

After this patch enable_psr module parameter values are:

-1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
 0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
 1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
 2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
 3 = PSR1 : yes, PSR2 = no,  Panel Replay : no

I.e. values different than -1 and 0 are handled as bitmasks where BIT0
disables PSR2 and BIT1 disables Panel Replay. Enable_psr parameter
doesn't impact Early Transport anymore.

v2:
  - make it more clear that enable_psr is bitmask for disabling different
    PSR modes

Jouni Högander (2):
  drm/i915/psr: Do not disable Early Transport when enable_psr is set
  drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

 .../drm/i915/display/intel_display_params.c   |  6 ++--
 drivers/gpu/drm/i915/display/intel_psr.c      | 35 +++++++++----------
 2 files changed, 20 insertions(+), 21 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] drm/i915/psr: Do not disable Early Transport when enable_psr is set

[Jouni Högander]

Current approach is that Early Transport is disabled in case enable_psr
module parameter is set. Let's ignore enable_psr parameter when choosing if
Early Transport can be used.

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index ae9053919211..a2b5688f0c82 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -265,16 +265,6 @@ static bool psr2_global_enabled(struct intel_dp *intel_dp)
 	}
 }
 
-static bool psr2_su_region_et_global_enabled(struct intel_dp *intel_dp)
-{
-	struct intel_display *display = to_intel_display(intel_dp);
-
-	if (display->params.enable_psr != -1)
-		return false;
-
-	return true;
-}
-
 static bool panel_replay_global_enabled(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
@@ -742,8 +732,7 @@ static bool psr2_su_region_et_valid(struct intel_dp *intel_dp, bool panel_replay
 	return panel_replay ?
 		intel_dp->pr_dpcd[INTEL_PR_DPCD_INDEX(DP_PANEL_REPLAY_CAP_SUPPORT)] &
 		DP_PANEL_REPLAY_EARLY_TRANSPORT_SUPPORT :
-		intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_ET_SUPPORTED &&
-		psr2_su_region_et_global_enabled(intel_dp);
+		intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_ET_SUPPORTED;
 }
 
 static void _panel_replay_enable_sink(struct intel_dp *intel_dp,
-- 
2.43.0

[PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Jouni Högander]

Currently disabling PSR2 via enable_psr module parameter causes Panel
Replay being disabled as well. This patch changes this by still allowing
Panel Replay even if PSR2 is disabled.

After this patch enable_psr module parameter values are:

-1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
 0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
 1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
 2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
 3 = PSR1 : yes, PSR2 = no,  Panel Replay : no

I.e. values different than -1 and 0 are handled as bitmasks where BIT0
disables PSR2 and BIT1 disables Panel Replay.

v2:
  - make it more clear that enable_psr is bitmask for disabling different
    PSR modes

Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
---
 .../drm/i915/display/intel_display_params.c   |  6 ++---
 drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-----
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
index 75316247ee8a..195af19ece5f 100644
--- a/drivers/gpu/drm/i915/display/intel_display_params.c
+++ b/drivers/gpu/drm/i915/display/intel_display_params.c
@@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc, int, 0400,
 	"(default: -1 (use per-chip default))");
 
 intel_display_param_named_unsafe(enable_psr, int, 0400,
-	"Enable PSR "
-	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
-	"Default: -1 (use per-chip default)");
+	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable Panel Replay (BIT1))."
+	"Values different from 0 and -1 are handled as bitmask to disable different PSR modes."
+	"E.g. value 3 disables both PSR2 and Panel Replay. Default: -1 (use per-chip default)");
 
 intel_display_param_named(psr_safest_params, bool, 0400,
 	"Replace PSR VBT parameters by the safest and not optimal ones. This "
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index a2b5688f0c82..959b868672d0 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -254,13 +254,16 @@ static bool psr2_global_enabled(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
 
+	return display->params.enable_psr == -1 ||
+		!(display->params.enable_psr & 0x1);
+}
+
+static bool sel_update_global_enabled(struct intel_dp *intel_dp)
+{
 	switch (intel_dp->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
-	case I915_PSR_DEBUG_DISABLE:
 	case I915_PSR_DEBUG_FORCE_PSR1:
 		return false;
 	default:
-		if (display->params.enable_psr == 1)
-			return false;
 		return true;
 	}
 }
@@ -269,7 +272,8 @@ static bool panel_replay_global_enabled(struct intel_dp *intel_dp)
 {
 	struct intel_display *display = to_intel_display(intel_dp);
 
-	if ((display->params.enable_psr != -1) ||
+	if ((display->params.enable_psr != -1 &&
+	     display->params.enable_psr & 0x2) ||
 	    (intel_dp->psr.debug & I915_PSR_DEBUG_PANEL_REPLAY_DISABLE))
 		return false;
 	return true;
@@ -1415,6 +1419,12 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 	if (!intel_dp->psr.sink_psr2_support)
 		return false;
 
+	if (!psr2_global_enabled(intel_dp)) {
+		drm_dbg_kms(display->drm,
+			    "PSR2 disabled by flag\n");
+		return false;
+	}
+
 	/* JSL and EHL only supports eDP 1.3 */
 	if (display->platform.jasperlake || display->platform.elkhartlake) {
 		drm_dbg_kms(display->drm, "PSR2 not supported by phy\n");
@@ -1517,7 +1527,7 @@ static bool intel_sel_update_config_valid(struct intel_dp *intel_dp,
 		goto unsupported;
 	}
 
-	if (!psr2_global_enabled(intel_dp)) {
+	if (!sel_update_global_enabled(intel_dp)) {
 		drm_dbg_kms(display->drm,
 			    "Selective update disabled by flag\n");
 		goto unsupported;
@@ -1664,7 +1674,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 	u8 active_pipes = 0;
 
 	if (!psr_global_enabled(intel_dp)) {
-		drm_dbg_kms(display->drm, "PSR disabled by flag\n");
+		drm_dbg_kms(display->drm, "PSR/Panel Replay disabled by flag\n");
 		return;
 	}
 
-- 
2.43.0

✓ i915.CI.BAT: success for Enable_psr kernel parameter changes (rev2)

[Patchwork]

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

== Series Details ==

Series: Enable_psr kernel parameter changes (rev2)
URL   : https://patchwork.freedesktop.org/series/151260/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_16831 -> Patchwork_151260v2
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (44 -> 44)
------------------------------

  Additional (1): bat-mtlp-9 
  Missing    (1): fi-snb-2520m 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-mtlp-9:         NOTRUN -> [SKIP][1] ([i915#4613]) +3 other tests skip
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_mmap@basic:
    - bat-mtlp-9:         NOTRUN -> [SKIP][2] ([i915#4083])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@gem_mmap@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-mtlp-9:         NOTRUN -> [SKIP][3] ([i915#4079]) +1 other test skip
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_fence_blits@basic:
    - bat-mtlp-9:         NOTRUN -> [SKIP][4] ([i915#4077]) +2 other tests skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@gem_tiled_fence_blits@basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-mtlp-9:         NOTRUN -> [SKIP][5] ([i915#11681] / [i915#6621])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live:
    - bat-dg2-8:          [PASS][6] -> [DMESG-FAIL][7] ([i915#12061]) +1 other test dmesg-fail
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16831/bat-dg2-8/igt@i915_selftest@live.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-dg2-8/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-mtlp-6:         [PASS][8] -> [DMESG-FAIL][9] ([i915#12061]) +1 other test dmesg-fail
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16831/bat-mtlp-6/igt@i915_selftest@live@workarounds.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-6/igt@i915_selftest@live@workarounds.html
    - bat-arls-6:         [PASS][10] -> [DMESG-FAIL][11] ([i915#12061]) +1 other test dmesg-fail
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16831/bat-arls-6/igt@i915_selftest@live@workarounds.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-arls-6/igt@i915_selftest@live@workarounds.html

  * igt@intel_hwmon@hwmon-read:
    - bat-mtlp-9:         NOTRUN -> [SKIP][12] ([i915#7707]) +1 other test skip
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@intel_hwmon@hwmon-read.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-mtlp-9:         NOTRUN -> [SKIP][13] ([i915#5190])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-mtlp-9:         NOTRUN -> [SKIP][14] ([i915#4212]) +8 other tests skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-mtlp-9:         NOTRUN -> [SKIP][15] ([i915#4213]) +1 other test skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-mtlp-9:         NOTRUN -> [SKIP][16] ([i915#3555] / [i915#3840] / [i915#9159])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-mtlp-9:         NOTRUN -> [SKIP][17]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@psr-primary-mmap-gtt:
    - bat-mtlp-9:         NOTRUN -> [SKIP][18] ([i915#4077] / [i915#9688]) +1 other test skip
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@kms_psr@psr-primary-mmap-gtt.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-mtlp-9:         NOTRUN -> [SKIP][19] ([i915#3555] / [i915#8809])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-gtt:
    - bat-mtlp-9:         NOTRUN -> [SKIP][20] ([i915#3708] / [i915#4077]) +1 other test skip
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@prime_vgem@basic-gtt.html

  * igt@prime_vgem@basic-read:
    - bat-mtlp-9:         NOTRUN -> [SKIP][21] ([i915#3708]) +1 other test skip
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-write:
    - bat-mtlp-9:         NOTRUN -> [SKIP][22] ([i915#10216] / [i915#3708])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-9/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_selftest@live:
    - bat-mtlp-8:         [DMESG-FAIL][23] ([i915#12061]) -> [PASS][24] +1 other test pass
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16831/bat-mtlp-8/igt@i915_selftest@live.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-mtlp-8/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-3:         [DMESG-FAIL][25] ([i915#12061]) -> [PASS][26] +1 other test pass
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16831/bat-arlh-3/igt@i915_selftest@live@workarounds.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-arlh-3/igt@i915_selftest@live@workarounds.html

  
#### Warnings ####

  * igt@dmabuf@all-tests:
    - fi-pnv-d510:        [ABORT][27] ([i915#12904]) -> [INCOMPLETE][28] ([i915#12904]) +1 other test incomplete
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16831/fi-pnv-d510/igt@dmabuf@all-tests.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/fi-pnv-d510/igt@dmabuf@all-tests.html

  * igt@i915_selftest@live:
    - bat-atsm-1:         [DMESG-FAIL][29] ([i915#12061] / [i915#14204]) -> [DMESG-FAIL][30] ([i915#12061] / [i915#13929])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16831/bat-atsm-1/igt@i915_selftest@live.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-atsm-1/igt@i915_selftest@live.html

  * igt@i915_selftest@live@mman:
    - bat-atsm-1:         [DMESG-FAIL][31] ([i915#14204]) -> [DMESG-FAIL][32] ([i915#13929])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16831/bat-atsm-1/igt@i915_selftest@live@mman.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_151260v2/bat-atsm-1/igt@i915_selftest@live@mman.html

  
  [i915#10216]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10216
  [i915#11681]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11681
  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#12904]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12904
  [i915#13929]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13929
  [i915#14204]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14204
  [i915#3555]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3708
  [i915#3840]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3840
  [i915#4077]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4213
  [i915#4613]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4613
  [i915#5190]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/5190
  [i915#6621]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/6621
  [i915#7707]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/7707
  [i915#8809]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/8809
  [i915#9159]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9159
  [i915#9688]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9688


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

  * Linux: CI_DRM_16831 -> Patchwork_151260v2

  CI-20190529: 20190529
  CI_DRM_16831: 7c4324c51cf2fe42e807e87def6176a5424f74b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8447: 8447
  Patchwork_151260v2: 7c4324c51cf2fe42e807e87def6176a5424f74b9 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

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

Re: [PATCH v2 1/2] drm/i915/psr: Do not disable Early Transport when enable_psr is set

[Rodrigo Vivi]

On Wed, Jul 09, 2025 at 10:57:57AM +0300, Jouni Högander wrote:
> Current approach is that Early Transport is disabled in case enable_psr
> module parameter is set. Let's ignore enable_psr parameter when choosing if
> Early Transport can be used.
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index ae9053919211..a2b5688f0c82 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -265,16 +265,6 @@ static bool psr2_global_enabled(struct intel_dp *intel_dp)
>  	}
>  }
>  
> -static bool psr2_su_region_et_global_enabled(struct intel_dp *intel_dp)
> -{
> -	struct intel_display *display = to_intel_display(intel_dp);
> -
> -	if (display->params.enable_psr != -1)
> -		return false;
> -
> -	return true;
> -}
> -
>  static bool panel_replay_global_enabled(struct intel_dp *intel_dp)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
> @@ -742,8 +732,7 @@ static bool psr2_su_region_et_valid(struct intel_dp *intel_dp, bool panel_replay
>  	return panel_replay ?
>  		intel_dp->pr_dpcd[INTEL_PR_DPCD_INDEX(DP_PANEL_REPLAY_CAP_SUPPORT)] &
>  		DP_PANEL_REPLAY_EARLY_TRANSPORT_SUPPORT :
> -		intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_ET_SUPPORTED &&
> -		psr2_su_region_et_global_enabled(intel_dp);
> +		intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_ET_SUPPORTED;
>  }
>  
>  static void _panel_replay_enable_sink(struct intel_dp *intel_dp,
> -- 
> 2.43.0
> 

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Rodrigo Vivi]

On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> Currently disabling PSR2 via enable_psr module parameter causes Panel
> Replay being disabled as well. This patch changes this by still allowing
> Panel Replay even if PSR2 is disabled.
> 
> After this patch enable_psr module parameter values are:
> 
> -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
>  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
>  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
>  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
>  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> 
> I.e. values different than -1 and 0 are handled as bitmasks where BIT0
> disables PSR2 and BIT1 disables Panel Replay.
> 
> v2:
>   - make it more clear that enable_psr is bitmask for disabling different
>     PSR modes
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  .../drm/i915/display/intel_display_params.c   |  6 ++---
>  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-----
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
> index 75316247ee8a..195af19ece5f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> @@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc, int, 0400,
>  	"(default: -1 (use per-chip default))");
>  
>  intel_display_param_named_unsafe(enable_psr, int, 0400,
> -	"Enable PSR "
> -	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> -	"Default: -1 (use per-chip default)");
> +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable Panel Replay (BIT1))."
> +	"Values different from 0 and -1 are handled as bitmask to disable different PSR modes."
> +	"E.g. value 3 disables both PSR2 and Panel Replay. Default: -1 (use per-chip default)");
>  
>  intel_display_param_named(psr_safest_params, bool, 0400,
>  	"Replace PSR VBT parameters by the safest and not optimal ones. This "
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index a2b5688f0c82..959b868672d0 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -254,13 +254,16 @@ static bool psr2_global_enabled(struct intel_dp *intel_dp)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
>  
> +	return display->params.enable_psr == -1 ||
> +		!(display->params.enable_psr & 0x1);
> +}
> +
> +static bool sel_update_global_enabled(struct intel_dp *intel_dp)
> +{
>  	switch (intel_dp->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
> -	case I915_PSR_DEBUG_DISABLE:
>  	case I915_PSR_DEBUG_FORCE_PSR1:
>  		return false;
>  	default:
> -		if (display->params.enable_psr == 1)
> -			return false;
>  		return true;
>  	}
>  }
> @@ -269,7 +272,8 @@ static bool panel_replay_global_enabled(struct intel_dp *intel_dp)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
>  
> -	if ((display->params.enable_psr != -1) ||
> +	if ((display->params.enable_psr != -1 &&
> +	     display->params.enable_psr & 0x2) ||

I believe we should probably define the bits

#define PSR_PARAM_DISABLE_PSR2		BIT(0)
#define PSR_PARAM_DISABLE_PANEL_REPLAY	BIT(1)

likely here in this .c file itself, not needed to be along with the param
but up to you, if you believe it makes more sense and gets clear there...

>  	    (intel_dp->psr.debug & I915_PSR_DEBUG_PANEL_REPLAY_DISABLE))
>  		return false;
>  	return true;
> @@ -1415,6 +1419,12 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  	if (!intel_dp->psr.sink_psr2_support)
>  		return false;
>  
> +	if (!psr2_global_enabled(intel_dp)) {
> +		drm_dbg_kms(display->drm,
> +			    "PSR2 disabled by flag\n");
> +		return false;
> +	}
> +
>  	/* JSL and EHL only supports eDP 1.3 */
>  	if (display->platform.jasperlake || display->platform.elkhartlake) {
>  		drm_dbg_kms(display->drm, "PSR2 not supported by phy\n");
> @@ -1517,7 +1527,7 @@ static bool intel_sel_update_config_valid(struct intel_dp *intel_dp,
>  		goto unsupported;
>  	}
>  
> -	if (!psr2_global_enabled(intel_dp)) {
> +	if (!sel_update_global_enabled(intel_dp)) {
>  		drm_dbg_kms(display->drm,
>  			    "Selective update disabled by flag\n");
>  		goto unsupported;
> @@ -1664,7 +1674,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  	u8 active_pipes = 0;
>  
>  	if (!psr_global_enabled(intel_dp)) {
> -		drm_dbg_kms(display->drm, "PSR disabled by flag\n");
> +		drm_dbg_kms(display->drm, "PSR/Panel Replay disabled by flag\n");
>  		return;
>  	}
>  
> -- 
> 2.43.0
> 

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Ville Syrjälä]

On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> Currently disabling PSR2 via enable_psr module parameter causes Panel
> Replay being disabled as well. This patch changes this by still allowing
> Panel Replay even if PSR2 is disabled.
> 
> After this patch enable_psr module parameter values are:
> 
> -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
>  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
>  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
>  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
>  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> 
> I.e. values different than -1 and 0 are handled as bitmasks where BIT0
> disables PSR2 and BIT1 disables Panel Replay.
> 
> v2:
>   - make it more clear that enable_psr is bitmask for disabling different
>     PSR modes
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  .../drm/i915/display/intel_display_params.c   |  6 ++---
>  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-----
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
> index 75316247ee8a..195af19ece5f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> @@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc, int, 0400,
>  	"(default: -1 (use per-chip default))");
>  
>  intel_display_param_named_unsafe(enable_psr, int, 0400,
> -	"Enable PSR "
> -	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> -	"Default: -1 (use per-chip default)");
> +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable Panel Replay (BIT1))."
> +	"Values different from 0 and -1 are handled as bitmask to disable different PSR modes."
> +	"E.g. value 3 disables both PSR2 and Panel Replay. Default: -1 (use per-chip default)");

This thing is very unintuitive. Why don't we just get replace it
with a new disable_psr modparam that is clearly just a bitmask of
what to disable?

>  
>  intel_display_param_named(psr_safest_params, bool, 0400,
>  	"Replace PSR VBT parameters by the safest and not optimal ones. This "
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index a2b5688f0c82..959b868672d0 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -254,13 +254,16 @@ static bool psr2_global_enabled(struct intel_dp *intel_dp)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
>  
> +	return display->params.enable_psr == -1 ||
> +		!(display->params.enable_psr & 0x1);
> +}
> +
> +static bool sel_update_global_enabled(struct intel_dp *intel_dp)
> +{
>  	switch (intel_dp->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
> -	case I915_PSR_DEBUG_DISABLE:
>  	case I915_PSR_DEBUG_FORCE_PSR1:
>  		return false;
>  	default:
> -		if (display->params.enable_psr == 1)
> -			return false;
>  		return true;
>  	}
>  }
> @@ -269,7 +272,8 @@ static bool panel_replay_global_enabled(struct intel_dp *intel_dp)
>  {
>  	struct intel_display *display = to_intel_display(intel_dp);
>  
> -	if ((display->params.enable_psr != -1) ||
> +	if ((display->params.enable_psr != -1 &&
> +	     display->params.enable_psr & 0x2) ||
>  	    (intel_dp->psr.debug & I915_PSR_DEBUG_PANEL_REPLAY_DISABLE))
>  		return false;
>  	return true;
> @@ -1415,6 +1419,12 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  	if (!intel_dp->psr.sink_psr2_support)
>  		return false;
>  
> +	if (!psr2_global_enabled(intel_dp)) {
> +		drm_dbg_kms(display->drm,
> +			    "PSR2 disabled by flag\n");
> +		return false;
> +	}
> +
>  	/* JSL and EHL only supports eDP 1.3 */
>  	if (display->platform.jasperlake || display->platform.elkhartlake) {
>  		drm_dbg_kms(display->drm, "PSR2 not supported by phy\n");
> @@ -1517,7 +1527,7 @@ static bool intel_sel_update_config_valid(struct intel_dp *intel_dp,
>  		goto unsupported;
>  	}
>  
> -	if (!psr2_global_enabled(intel_dp)) {
> +	if (!sel_update_global_enabled(intel_dp)) {
>  		drm_dbg_kms(display->drm,
>  			    "Selective update disabled by flag\n");
>  		goto unsupported;
> @@ -1664,7 +1674,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  	u8 active_pipes = 0;
>  
>  	if (!psr_global_enabled(intel_dp)) {
> -		drm_dbg_kms(display->drm, "PSR disabled by flag\n");
> +		drm_dbg_kms(display->drm, "PSR/Panel Replay disabled by flag\n");
>  		return;
>  	}
>  
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Hogander, Jouni]

On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> > Currently disabling PSR2 via enable_psr module parameter causes
> > Panel
> > Replay being disabled as well. This patch changes this by still
> > allowing
> > Panel Replay even if PSR2 is disabled.
> > 
> > After this patch enable_psr module parameter values are:
> > 
> > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > 
> > I.e. values different than -1 and 0 are handled as bitmasks where
> > BIT0
> > disables PSR2 and BIT1 disables Panel Replay.
> > 
> > v2:
> >   - make it more clear that enable_psr is bitmask for disabling
> > different
> >     PSR modes
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-
> > ----
> >  2 files changed, 19 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c
> > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > index 75316247ee8a..195af19ece5f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > @@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc,
> > int, 0400,
> >  	"(default: -1 (use per-chip default))");
> >  
> >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > -	"Enable PSR "
> > -	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> > -	"Default: -1 (use per-chip default)");
> > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable
> > Panel Replay (BIT1))."
> > +	"Values different from 0 and -1 are handled as bitmask to
> > disable different PSR modes."
> > +	"E.g. value 3 disables both PSR2 and Panel Replay.
> > Default: -1 (use per-chip default)");
> 
> This thing is very unintuitive. Why don't we just get replace it
> with a new disable_psr modparam that is clearly just a bitmask of
> what to disable?

I was thinkinig we should keep it backward compatible. I know this
parameter is in use.

BR,

Jouni Högander

> >  
> >  intel_display_param_named(psr_safest_params, bool, 0400,
> >  	"Replace PSR VBT parameters by the safest and not optimal
> > ones. This "
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index a2b5688f0c82..959b868672d0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -254,13 +254,16 @@ static bool psr2_global_enabled(struct
> > intel_dp *intel_dp)
> >  {
> >  	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  
> > +	return display->params.enable_psr == -1 ||
> > +		!(display->params.enable_psr & 0x1);
> > +}
> > +
> > +static bool sel_update_global_enabled(struct intel_dp *intel_dp)
> > +{
> >  	switch (intel_dp->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
> > -	case I915_PSR_DEBUG_DISABLE:
> >  	case I915_PSR_DEBUG_FORCE_PSR1:
> >  		return false;
> >  	default:
> > -		if (display->params.enable_psr == 1)
> > -			return false;
> >  		return true;
> >  	}
> >  }
> > @@ -269,7 +272,8 @@ static bool panel_replay_global_enabled(struct
> > intel_dp *intel_dp)
> >  {
> >  	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  
> > -	if ((display->params.enable_psr != -1) ||
> > +	if ((display->params.enable_psr != -1 &&
> > +	     display->params.enable_psr & 0x2) ||
> >  	    (intel_dp->psr.debug &
> > I915_PSR_DEBUG_PANEL_REPLAY_DISABLE))
> >  		return false;
> >  	return true;
> > @@ -1415,6 +1419,12 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  	if (!intel_dp->psr.sink_psr2_support)
> >  		return false;
> >  
> > +	if (!psr2_global_enabled(intel_dp)) {
> > +		drm_dbg_kms(display->drm,
> > +			    "PSR2 disabled by flag\n");
> > +		return false;
> > +	}
> > +
> >  	/* JSL and EHL only supports eDP 1.3 */
> >  	if (display->platform.jasperlake || display-
> > >platform.elkhartlake) {
> >  		drm_dbg_kms(display->drm, "PSR2 not supported by
> > phy\n");
> > @@ -1517,7 +1527,7 @@ static bool
> > intel_sel_update_config_valid(struct intel_dp *intel_dp,
> >  		goto unsupported;
> >  	}
> >  
> > -	if (!psr2_global_enabled(intel_dp)) {
> > +	if (!sel_update_global_enabled(intel_dp)) {
> >  		drm_dbg_kms(display->drm,
> >  			    "Selective update disabled by
> > flag\n");
> >  		goto unsupported;
> > @@ -1664,7 +1674,7 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  	u8 active_pipes = 0;
> >  
> >  	if (!psr_global_enabled(intel_dp)) {
> > -		drm_dbg_kms(display->drm, "PSR disabled by
> > flag\n");
> > +		drm_dbg_kms(display->drm, "PSR/Panel Replay
> > disabled by flag\n");
> >  		return;
> >  	}
> >  
> > -- 
> > 2.43.0
> 

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Rodrigo Vivi]

On Wed, Jul 09, 2025 at 06:11:17PM +0000, Hogander, Jouni wrote:
> On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> > On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> > > Currently disabling PSR2 via enable_psr module parameter causes
> > > Panel
> > > Replay being disabled as well. This patch changes this by still
> > > allowing
> > > Panel Replay even if PSR2 is disabled.
> > > 
> > > After this patch enable_psr module parameter values are:
> > > 
> > > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> > >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> > >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> > >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > > 
> > > I.e. values different than -1 and 0 are handled as bitmasks where
> > > BIT0
> > > disables PSR2 and BIT1 disables Panel Replay.
> > > 
> > > v2:
> > >   - make it more clear that enable_psr is bitmask for disabling
> > > different
> > >     PSR modes
> > > 
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> > >  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-
> > > ----
> > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > index 75316247ee8a..195af19ece5f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > @@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc,
> > > int, 0400,
> > >  	"(default: -1 (use per-chip default))");
> > >  
> > >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > > -	"Enable PSR "
> > > -	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> > > -	"Default: -1 (use per-chip default)");
> > > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable
> > > Panel Replay (BIT1))."
> > > +	"Values different from 0 and -1 are handled as bitmask to
> > > disable different PSR modes."
> > > +	"E.g. value 3 disables both PSR2 and Panel Replay.
> > > Default: -1 (use per-chip default)");
> > 
> > This thing is very unintuitive. Why don't we just get replace it
> > with a new disable_psr modparam that is clearly just a bitmask of
> > what to disable?
> 
> I was thinkinig we should keep it backward compatible. I know this
> parameter is in use.

I agree on keeping this backward compatible.

Also our experience with disable_power_well shows that negative
name in the parameter can be much more unintuitive and confusing.

> 
> BR,
> 
> Jouni Högander
> 
> > >  
> > >  intel_display_param_named(psr_safest_params, bool, 0400,
> > >  	"Replace PSR VBT parameters by the safest and not optimal
> > > ones. This "
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index a2b5688f0c82..959b868672d0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -254,13 +254,16 @@ static bool psr2_global_enabled(struct
> > > intel_dp *intel_dp)
> > >  {
> > >  	struct intel_display *display =
> > > to_intel_display(intel_dp);
> > >  
> > > +	return display->params.enable_psr == -1 ||
> > > +		!(display->params.enable_psr & 0x1);
> > > +}
> > > +
> > > +static bool sel_update_global_enabled(struct intel_dp *intel_dp)
> > > +{
> > >  	switch (intel_dp->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
> > > -	case I915_PSR_DEBUG_DISABLE:
> > >  	case I915_PSR_DEBUG_FORCE_PSR1:
> > >  		return false;
> > >  	default:
> > > -		if (display->params.enable_psr == 1)
> > > -			return false;
> > >  		return true;
> > >  	}
> > >  }
> > > @@ -269,7 +272,8 @@ static bool panel_replay_global_enabled(struct
> > > intel_dp *intel_dp)
> > >  {
> > >  	struct intel_display *display =
> > > to_intel_display(intel_dp);
> > >  
> > > -	if ((display->params.enable_psr != -1) ||
> > > +	if ((display->params.enable_psr != -1 &&
> > > +	     display->params.enable_psr & 0x2) ||
> > >  	    (intel_dp->psr.debug &
> > > I915_PSR_DEBUG_PANEL_REPLAY_DISABLE))
> > >  		return false;
> > >  	return true;
> > > @@ -1415,6 +1419,12 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > >  	if (!intel_dp->psr.sink_psr2_support)
> > >  		return false;
> > >  
> > > +	if (!psr2_global_enabled(intel_dp)) {
> > > +		drm_dbg_kms(display->drm,
> > > +			    "PSR2 disabled by flag\n");
> > > +		return false;
> > > +	}
> > > +
> > >  	/* JSL and EHL only supports eDP 1.3 */
> > >  	if (display->platform.jasperlake || display-
> > > >platform.elkhartlake) {
> > >  		drm_dbg_kms(display->drm, "PSR2 not supported by
> > > phy\n");
> > > @@ -1517,7 +1527,7 @@ static bool
> > > intel_sel_update_config_valid(struct intel_dp *intel_dp,
> > >  		goto unsupported;
> > >  	}
> > >  
> > > -	if (!psr2_global_enabled(intel_dp)) {
> > > +	if (!sel_update_global_enabled(intel_dp)) {
> > >  		drm_dbg_kms(display->drm,
> > >  			    "Selective update disabled by
> > > flag\n");
> > >  		goto unsupported;
> > > @@ -1664,7 +1674,7 @@ void intel_psr_compute_config(struct intel_dp
> > > *intel_dp,
> > >  	u8 active_pipes = 0;
> > >  
> > >  	if (!psr_global_enabled(intel_dp)) {
> > > -		drm_dbg_kms(display->drm, "PSR disabled by
> > > flag\n");
> > > +		drm_dbg_kms(display->drm, "PSR/Panel Replay
> > > disabled by flag\n");
> > >  		return;
> > >  	}
> > >  
> > > -- 
> > > 2.43.0
> > 
> 

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Hogander, Jouni]

On Wed, 2025-07-09 at 09:27 -0400, Rodrigo Vivi wrote:
> On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> > Currently disabling PSR2 via enable_psr module parameter causes
> > Panel
> > Replay being disabled as well. This patch changes this by still
> > allowing
> > Panel Replay even if PSR2 is disabled.
> > 
> > After this patch enable_psr module parameter values are:
> > 
> > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > 
> > I.e. values different than -1 and 0 are handled as bitmasks where
> > BIT0
> > disables PSR2 and BIT1 disables Panel Replay.
> > 
> > v2:
> >   - make it more clear that enable_psr is bitmask for disabling
> > different
> >     PSR modes
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-
> > ----
> >  2 files changed, 19 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c
> > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > index 75316247ee8a..195af19ece5f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > @@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc,
> > int, 0400,
> >  	"(default: -1 (use per-chip default))");
> >  
> >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > -	"Enable PSR "
> > -	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> > -	"Default: -1 (use per-chip default)");
> > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable
> > Panel Replay (BIT1))."
> > +	"Values different from 0 and -1 are handled as bitmask to
> > disable different PSR modes."
> > +	"E.g. value 3 disables both PSR2 and Panel Replay.
> > Default: -1 (use per-chip default)");
> >  
> >  intel_display_param_named(psr_safest_params, bool, 0400,
> >  	"Replace PSR VBT parameters by the safest and not optimal
> > ones. This "
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index a2b5688f0c82..959b868672d0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -254,13 +254,16 @@ static bool psr2_global_enabled(struct
> > intel_dp *intel_dp)
> >  {
> >  	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  
> > +	return display->params.enable_psr == -1 ||
> > +		!(display->params.enable_psr & 0x1);
> > +}
> > +
> > +static bool sel_update_global_enabled(struct intel_dp *intel_dp)
> > +{
> >  	switch (intel_dp->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
> > -	case I915_PSR_DEBUG_DISABLE:
> >  	case I915_PSR_DEBUG_FORCE_PSR1:
> >  		return false;
> >  	default:
> > -		if (display->params.enable_psr == 1)
> > -			return false;
> >  		return true;
> >  	}
> >  }
> > @@ -269,7 +272,8 @@ static bool panel_replay_global_enabled(struct
> > intel_dp *intel_dp)
> >  {
> >  	struct intel_display *display =
> > to_intel_display(intel_dp);
> >  
> > -	if ((display->params.enable_psr != -1) ||
> > +	if ((display->params.enable_psr != -1 &&
> > +	     display->params.enable_psr & 0x2) ||
> 
> I believe we should probably define the bits
> 
> #define PSR_PARAM_DISABLE_PSR2		BIT(0)
> #define PSR_PARAM_DISABLE_PANEL_REPLAY	BIT(1)
> 
> likely here in this .c file itself, not needed to be along with the
> param
> but up to you, if you believe it makes more sense and gets clear
> there...

I have done this change in version 3.

BR,

Jouni Högander

> 
> >  	    (intel_dp->psr.debug &
> > I915_PSR_DEBUG_PANEL_REPLAY_DISABLE))
> >  		return false;
> >  	return true;
> > @@ -1415,6 +1419,12 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  	if (!intel_dp->psr.sink_psr2_support)
> >  		return false;
> >  
> > +	if (!psr2_global_enabled(intel_dp)) {
> > +		drm_dbg_kms(display->drm,
> > +			    "PSR2 disabled by flag\n");
> > +		return false;
> > +	}
> > +
> >  	/* JSL and EHL only supports eDP 1.3 */
> >  	if (display->platform.jasperlake || display-
> > >platform.elkhartlake) {
> >  		drm_dbg_kms(display->drm, "PSR2 not supported by
> > phy\n");
> > @@ -1517,7 +1527,7 @@ static bool
> > intel_sel_update_config_valid(struct intel_dp *intel_dp,
> >  		goto unsupported;
> >  	}
> >  
> > -	if (!psr2_global_enabled(intel_dp)) {
> > +	if (!sel_update_global_enabled(intel_dp)) {
> >  		drm_dbg_kms(display->drm,
> >  			    "Selective update disabled by
> > flag\n");
> >  		goto unsupported;
> > @@ -1664,7 +1674,7 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  	u8 active_pipes = 0;
> >  
> >  	if (!psr_global_enabled(intel_dp)) {
> > -		drm_dbg_kms(display->drm, "PSR disabled by
> > flag\n");
> > +		drm_dbg_kms(display->drm, "PSR/Panel Replay
> > disabled by flag\n");
> >  		return;
> >  	}
> >  
> > -- 
> > 2.43.0
> > 

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Ville Syrjälä]

On Thu, Jul 10, 2025 at 11:42:52AM -0400, Rodrigo Vivi wrote:
> On Wed, Jul 09, 2025 at 06:11:17PM +0000, Hogander, Jouni wrote:
> > On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> > > On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> > > > Currently disabling PSR2 via enable_psr module parameter causes
> > > > Panel
> > > > Replay being disabled as well. This patch changes this by still
> > > > allowing
> > > > Panel Replay even if PSR2 is disabled.
> > > > 
> > > > After this patch enable_psr module parameter values are:
> > > > 
> > > > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> > > >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> > > >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> > > >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > > >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > > > 
> > > > I.e. values different than -1 and 0 are handled as bitmasks where
> > > > BIT0
> > > > disables PSR2 and BIT1 disables Panel Replay.
> > > > 
> > > > v2:
> > > >   - make it more clear that enable_psr is bitmask for disabling
> > > > different
> > > >     PSR modes
> > > > 
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-
> > > > ----
> > > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > index 75316247ee8a..195af19ece5f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > @@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc,
> > > > int, 0400,
> > > >  	"(default: -1 (use per-chip default))");
> > > >  
> > > >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > > > -	"Enable PSR "
> > > > -	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> > > > -	"Default: -1 (use per-chip default)");
> > > > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable
> > > > Panel Replay (BIT1))."
> > > > +	"Values different from 0 and -1 are handled as bitmask to
> > > > disable different PSR modes."
> > > > +	"E.g. value 3 disables both PSR2 and Panel Replay.
> > > > Default: -1 (use per-chip default)");
> > > 
> > > This thing is very unintuitive. Why don't we just get replace it
> > > with a new disable_psr modparam that is clearly just a bitmask of
> > > what to disable?
> > 
> > I was thinkinig we should keep it backward compatible. I know this
> > parameter is in use.
> 
> I agree on keeping this backward compatible.

IMO it's an unusable mess so I wouldn't bother trying to preserve it.
The only value that seems to make any sense currently is =0. If I
need to use any other value I always give up immediately and just
hack the code instead.

If we keep calling it 'enable_psr' then it should clearly be a
bitmask of things to *enable*, not things to *disable*.

> 
> Also our experience with disable_power_well shows that negative
> name in the parameter can be much more unintuitive and confusing.

That one is rather different because it doesn't "disable power wells"
but rather it "disables power well disabling". But yes, it is a very
poor name as well.

Calling it "enable_power_wells" wouldn't really help though.
It should perhaps be something more like 'dont_disable_power_wells'
or 'keep_power_wells_on'.

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Rodrigo Vivi]

On Thu, Jul 10, 2025 at 11:09:42PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 10, 2025 at 11:42:52AM -0400, Rodrigo Vivi wrote:
> > On Wed, Jul 09, 2025 at 06:11:17PM +0000, Hogander, Jouni wrote:
> > > On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> > > > On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> > > > > Currently disabling PSR2 via enable_psr module parameter causes
> > > > > Panel
> > > > > Replay being disabled as well. This patch changes this by still
> > > > > allowing
> > > > > Panel Replay even if PSR2 is disabled.
> > > > > 
> > > > > After this patch enable_psr module parameter values are:
> > > > > 
> > > > > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> > > > >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> > > > >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> > > > >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > > > >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > > > > 
> > > > > I.e. values different than -1 and 0 are handled as bitmasks where
> > > > > BIT0
> > > > > disables PSR2 and BIT1 disables Panel Replay.
> > > > > 
> > > > > v2:
> > > > >   - make it more clear that enable_psr is bitmask for disabling
> > > > > different
> > > > >     PSR modes
> > > > > 
> > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > ---
> > > > >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-
> > > > > ----
> > > > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > index 75316247ee8a..195af19ece5f 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > @@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc,
> > > > > int, 0400,
> > > > >  	"(default: -1 (use per-chip default))");
> > > > >  
> > > > >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > > > > -	"Enable PSR "
> > > > > -	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> > > > > -	"Default: -1 (use per-chip default)");
> > > > > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable
> > > > > Panel Replay (BIT1))."
> > > > > +	"Values different from 0 and -1 are handled as bitmask to
> > > > > disable different PSR modes."
> > > > > +	"E.g. value 3 disables both PSR2 and Panel Replay.
> > > > > Default: -1 (use per-chip default)");
> > > > 
> > > > This thing is very unintuitive. Why don't we just get replace it
> > > > with a new disable_psr modparam that is clearly just a bitmask of
> > > > what to disable?
> > > 
> > > I was thinkinig we should keep it backward compatible. I know this
> > > parameter is in use.
> > 
> > I agree on keeping this backward compatible.
> 
> IMO it's an unusable mess so I wouldn't bother trying to preserve it.
> The only value that seems to make any sense currently is =0. 

fair enough. what about simply removing all the options entirely?
enable_psr=0 keeps disabling it, otherwise enabled it. And we reduce
all the knobs option. Afterall, this should be our end goal anyway.

> If I
> need to use any other value I always give up immediately and just
> hack the code instead.

Well, the param actually exists for us to request reporters to try
different config. The devs can always modify the code.

Question now is, are all these variants useful for collecting debug
information of some sort?

If so, as long as it is documented and we can ask different values,
we should be good.

> 
> If we keep calling it 'enable_psr' then it should clearly be a
> bitmask of things to *enable*, not things to *disable*.
> 
> > 
> > Also our experience with disable_power_well shows that negative
> > name in the parameter can be much more unintuitive and confusing.
> 
> That one is rather different because it doesn't "disable power wells"
> but rather it "disables power well disabling". But yes, it is a very
> poor name as well.
> 
> Calling it "enable_power_wells" wouldn't really help though.
> It should perhaps be something more like 'dont_disable_power_wells'
> or 'keep_power_wells_on'.

okay, fair enough, disable power well is another level of complication.

back to disable_psr idea:

disable_psr=0 == enable PSR? to me this is already uninituitive anyway.
disable_psr=1 == disable PSR1?
disable_psr=2 == disable PSR2? and keep only PSR=1?

I still don't see a clean obvious intuitive way of handling it.
Perhaps what I had suggested another day:

PSR1 = BIT0
PSR2 = BIT1 (PSR2 infers PSR1 enabled)
PANEL_REPLAY = BIT2 (also infers PSR1(and 2?) enabled)

(Peraps even bit3 for early transport?)

This is backwards compatible because

0 = disabled,
1 = up to psr1,
2 = up to psr2, (no panel replay)
3 = up to psr2, (same as 2)
4 = panel replay on
...

> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Ville Syrjälä]

On Thu, Jul 10, 2025 at 05:27:13PM -0400, Rodrigo Vivi wrote:
> On Thu, Jul 10, 2025 at 11:09:42PM +0300, Ville Syrjälä wrote:
> > On Thu, Jul 10, 2025 at 11:42:52AM -0400, Rodrigo Vivi wrote:
> > > On Wed, Jul 09, 2025 at 06:11:17PM +0000, Hogander, Jouni wrote:
> > > > On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> > > > > On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> > > > > > Currently disabling PSR2 via enable_psr module parameter causes
> > > > > > Panel
> > > > > > Replay being disabled as well. This patch changes this by still
> > > > > > allowing
> > > > > > Panel Replay even if PSR2 is disabled.
> > > > > > 
> > > > > > After this patch enable_psr module parameter values are:
> > > > > > 
> > > > > > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> > > > > >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> > > > > >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> > > > > >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > > > > >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > > > > > 
> > > > > > I.e. values different than -1 and 0 are handled as bitmasks where
> > > > > > BIT0
> > > > > > disables PSR2 and BIT1 disables Panel Replay.
> > > > > > 
> > > > > > v2:
> > > > > >   - make it more clear that enable_psr is bitmask for disabling
> > > > > > different
> > > > > >     PSR modes
> > > > > > 
> > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > > ---
> > > > > >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> > > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 22 ++++++++++++++-
> > > > > > ----
> > > > > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > index 75316247ee8a..195af19ece5f 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > @@ -116,9 +116,9 @@ intel_display_param_named_unsafe(enable_fbc,
> > > > > > int, 0400,
> > > > > >  	"(default: -1 (use per-chip default))");
> > > > > >  
> > > > > >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > > > > > -	"Enable PSR "
> > > > > > -	"(0=disabled, 1=enable up to PSR1, 2=enable up to PSR2) "
> > > > > > -	"Default: -1 (use per-chip default)");
> > > > > > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0), 2=disable
> > > > > > Panel Replay (BIT1))."
> > > > > > +	"Values different from 0 and -1 are handled as bitmask to
> > > > > > disable different PSR modes."
> > > > > > +	"E.g. value 3 disables both PSR2 and Panel Replay.
> > > > > > Default: -1 (use per-chip default)");
> > > > > 
> > > > > This thing is very unintuitive. Why don't we just get replace it
> > > > > with a new disable_psr modparam that is clearly just a bitmask of
> > > > > what to disable?
> > > > 
> > > > I was thinkinig we should keep it backward compatible. I know this
> > > > parameter is in use.
> > > 
> > > I agree on keeping this backward compatible.
> > 
> > IMO it's an unusable mess so I wouldn't bother trying to preserve it.
> > The only value that seems to make any sense currently is =0. 
> 
> fair enough. what about simply removing all the options entirely?
> enable_psr=0 keeps disabling it, otherwise enabled it. And we reduce
> all the knobs option. Afterall, this should be our end goal anyway.
> 
> > If I
> > need to use any other value I always give up immediately and just
> > hack the code instead.
> 
> Well, the param actually exists for us to request reporters to try
> different config. The devs can always modify the code.
> 
> Question now is, are all these variants useful for collecting debug
> information of some sort?
> 
> If so, as long as it is documented and we can ask different values,
> we should be good.
> 
> > 
> > If we keep calling it 'enable_psr' then it should clearly be a
> > bitmask of things to *enable*, not things to *disable*.
> > 
> > > 
> > > Also our experience with disable_power_well shows that negative
> > > name in the parameter can be much more unintuitive and confusing.
> > 
> > That one is rather different because it doesn't "disable power wells"
> > but rather it "disables power well disabling". But yes, it is a very
> > poor name as well.
> > 
> > Calling it "enable_power_wells" wouldn't really help though.
> > It should perhaps be something more like 'dont_disable_power_wells'
> > or 'keep_power_wells_on'.
> 
> okay, fair enough, disable power well is another level of complication.
> 
> back to disable_psr idea:
> 
> disable_psr=0 == enable PSR? to me this is already uninituitive anyway.
> disable_psr=1 == disable PSR1?
> disable_psr=2 == disable PSR2? and keep only PSR=1?
> 
> I still don't see a clean obvious intuitive way of handling it.
> Perhaps what I had suggested another day:
> 
> PSR1 = BIT0
> PSR2 = BIT1 (PSR2 infers PSR1 enabled)
> PANEL_REPLAY = BIT2 (also infers PSR1(and 2?) enabled)

With a bitmask I don't think inferring anything is helpful.
If the corresponding bit isn't set then don't use that
mode, period.

Another option would to have a separate named parameter
for each mode. Would be easier to understand but dunno
if we really want to add that many modparams just for this.

> (Peraps even bit3 for early transport?)
> 
> This is backwards compatible because
> 
> 0 = disabled,
> 1 = up to psr1,
> 2 = up to psr2, (no panel replay)
> 3 = up to psr2, (same as 2)
> 4 = panel replay on
> ...
> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Hogander, Jouni]

On Thu, 2025-07-10 at 23:09 +0300, Ville Syrjälä wrote:
> On Thu, Jul 10, 2025 at 11:42:52AM -0400, Rodrigo Vivi wrote:
> > On Wed, Jul 09, 2025 at 06:11:17PM +0000, Hogander, Jouni wrote:
> > > On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> > > > On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander wrote:
> > > > > Currently disabling PSR2 via enable_psr module parameter
> > > > > causes
> > > > > Panel
> > > > > Replay being disabled as well. This patch changes this by
> > > > > still
> > > > > allowing
> > > > > Panel Replay even if PSR2 is disabled.
> > > > > 
> > > > > After this patch enable_psr module parameter values are:
> > > > > 
> > > > > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> > > > >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> > > > >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> > > > >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > > > >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > > > > 
> > > > > I.e. values different than -1 and 0 are handled as bitmasks
> > > > > where
> > > > > BIT0
> > > > > disables PSR2 and BIT1 disables Panel Replay.
> > > > > 
> > > > > v2:
> > > > >   - make it more clear that enable_psr is bitmask for
> > > > > disabling
> > > > > different
> > > > >     PSR modes
> > > > > 
> > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > ---
> > > > >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 22
> > > > > ++++++++++++++-
> > > > > ----
> > > > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git
> > > > > a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > index 75316247ee8a..195af19ece5f 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > @@ -116,9 +116,9 @@
> > > > > intel_display_param_named_unsafe(enable_fbc,
> > > > > int, 0400,
> > > > >  	"(default: -1 (use per-chip default))");
> > > > >  
> > > > >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > > > > -	"Enable PSR "
> > > > > -	"(0=disabled, 1=enable up to PSR1, 2=enable up to
> > > > > PSR2) "
> > > > > -	"Default: -1 (use per-chip default)");
> > > > > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0),
> > > > > 2=disable
> > > > > Panel Replay (BIT1))."
> > > > > +	"Values different from 0 and -1 are handled as
> > > > > bitmask to
> > > > > disable different PSR modes."
> > > > > +	"E.g. value 3 disables both PSR2 and Panel Replay.
> > > > > Default: -1 (use per-chip default)");
> > > > 
> > > > This thing is very unintuitive. Why don't we just get replace
> > > > it
> > > > with a new disable_psr modparam that is clearly just a bitmask
> > > > of
> > > > what to disable?
> > > 
> > > I was thinkinig we should keep it backward compatible. I know
> > > this
> > > parameter is in use.
> > 
> > I agree on keeping this backward compatible.
> 
> IMO it's an unusable mess so I wouldn't bother trying to preserve it.
> The only value that seems to make any sense currently is =0. If I
> need to use any other value I always give up immediately and just
> hack the code instead.

It's unintuitive but not unusable. I have instructed several times bug
reporter to set it to 0/1 to bisect if it's PSR2 problem. We have also
customers using this to disable PSR modes they don't want. As we have
provided this at some point I wouldn't drop it now.

> 
> If we keep calling it 'enable_psr' then it should clearly be a
> bitmask of things to *enable*, not things to *disable*.

Ok, it seems I need inverse the logic to enable rather than disable. I
will do that.

BR,

Jouni Högander

> 
> > 
> > Also our experience with disable_power_well shows that negative
> > name in the parameter can be much more unintuitive and confusing.
> 
> That one is rather different because it doesn't "disable power wells"
> but rather it "disables power well disabling". But yes, it is a very
> poor name as well.
> 
> Calling it "enable_power_wells" wouldn't really help though.
> It should perhaps be something more like 'dont_disable_power_wells'
> or 'keep_power_wells_on'.
> 

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Hogander, Jouni]

On Thu, 2025-07-10 at 17:27 -0400, Rodrigo Vivi wrote:
> On Thu, Jul 10, 2025 at 11:09:42PM +0300, Ville Syrjälä wrote:
> > On Thu, Jul 10, 2025 at 11:42:52AM -0400, Rodrigo Vivi wrote:
> > > On Wed, Jul 09, 2025 at 06:11:17PM +0000, Hogander, Jouni wrote:
> > > > On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> > > > > On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander
> > > > > wrote:
> > > > > > Currently disabling PSR2 via enable_psr module parameter
> > > > > > causes
> > > > > > Panel
> > > > > > Replay being disabled as well. This patch changes this by
> > > > > > still
> > > > > > allowing
> > > > > > Panel Replay even if PSR2 is disabled.
> > > > > > 
> > > > > > After this patch enable_psr module parameter values are:
> > > > > > 
> > > > > > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> > > > > >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> > > > > >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> > > > > >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > > > > >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > > > > > 
> > > > > > I.e. values different than -1 and 0 are handled as bitmasks
> > > > > > where
> > > > > > BIT0
> > > > > > disables PSR2 and BIT1 disables Panel Replay.
> > > > > > 
> > > > > > v2:
> > > > > >   - make it more clear that enable_psr is bitmask for
> > > > > > disabling
> > > > > > different
> > > > > >     PSR modes
> > > > > > 
> > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > > ---
> > > > > >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> > > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 22
> > > > > > ++++++++++++++-
> > > > > > ----
> > > > > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git
> > > > > > a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > index 75316247ee8a..195af19ece5f 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > @@ -116,9 +116,9 @@
> > > > > > intel_display_param_named_unsafe(enable_fbc,
> > > > > > int, 0400,
> > > > > >  	"(default: -1 (use per-chip default))");
> > > > > >  
> > > > > >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > > > > > -	"Enable PSR "
> > > > > > -	"(0=disabled, 1=enable up to PSR1, 2=enable up to
> > > > > > PSR2) "
> > > > > > -	"Default: -1 (use per-chip default)");
> > > > > > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0),
> > > > > > 2=disable
> > > > > > Panel Replay (BIT1))."
> > > > > > +	"Values different from 0 and -1 are handled as
> > > > > > bitmask to
> > > > > > disable different PSR modes."
> > > > > > +	"E.g. value 3 disables both PSR2 and Panel Replay.
> > > > > > Default: -1 (use per-chip default)");
> > > > > 
> > > > > This thing is very unintuitive. Why don't we just get replace
> > > > > it
> > > > > with a new disable_psr modparam that is clearly just a
> > > > > bitmask of
> > > > > what to disable?
> > > > 
> > > > I was thinkinig we should keep it backward compatible. I know
> > > > this
> > > > parameter is in use.
> > > 
> > > I agree on keeping this backward compatible.
> > 
> > IMO it's an unusable mess so I wouldn't bother trying to preserve
> > it.
> > The only value that seems to make any sense currently is =0. 
> 
> fair enough. what about simply removing all the options entirely?
> enable_psr=0 keeps disabling it, otherwise enabled it. And we reduce
> all the knobs option. Afterall, this should be our end goal anyway.
> 
> > If I
> > need to use any other value I always give up immediately and just
> > hack the code instead.
> 
> Well, the param actually exists for us to request reporters to try
> different config. The devs can always modify the code.

No. This not just for us. This is currently used specifically to
disable PSR2 completely on certain setups.

> 
> Question now is, are all these variants useful for collecting debug
> information of some sort?
> 
> If so, as long as it is documented and we can ask different values,
> we should be good.

Yes, I agree on this. I know this is unintuitive, but when you know the
value to use you can achieve what you need.

> 
> > 
> > If we keep calling it 'enable_psr' then it should clearly be a
> > bitmask of things to *enable*, not things to *disable*.
> > 
> > > 
> > > Also our experience with disable_power_well shows that negative
> > > name in the parameter can be much more unintuitive and confusing.
> > 
> > That one is rather different because it doesn't "disable power
> > wells"
> > but rather it "disables power well disabling". But yes, it is a
> > very
> > poor name as well.
> > 
> > Calling it "enable_power_wells" wouldn't really help though.
> > It should perhaps be something more like 'dont_disable_power_wells'
> > or 'keep_power_wells_on'.
> 
> okay, fair enough, disable power well is another level of
> complication.
> 
> back to disable_psr idea:
> 
> disable_psr=0 == enable PSR? to me this is already uninituitive
> anyway.
> disable_psr=1 == disable PSR1?
> disable_psr=2 == disable PSR2? and keep only PSR=1?
> 
> I still don't see a clean obvious intuitive way of handling it.

If the parameter would be used only by us to bisect some PSR problem
this would be ok. Now as we already have lots of users having
*.enable_psr=1 fed by the booloader to kernel I wouldn't change it.

> Perhaps what I had suggested another day:
> 
> PSR1 = BIT0
> PSR2 = BIT1 (PSR2 infers PSR1 enabled)
> PANEL_REPLAY = BIT2 (also infers PSR1(and 2?) enabled)
> 
> (Peraps even bit3 for early transport?)
> 
> This is backwards compatible because
> 
> 0 = disabled,
> 1 = up to psr1,
> 2 = up to psr2, (no panel replay)
> 3 = up to psr2, (same as 2)
> 4 = panel replay on
> ...

BR,
Jouni Högander

> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Hogander, Jouni]

On Fri, 2025-07-11 at 02:11 +0300, Ville Syrjälä wrote:
> On Thu, Jul 10, 2025 at 05:27:13PM -0400, Rodrigo Vivi wrote:
> > On Thu, Jul 10, 2025 at 11:09:42PM +0300, Ville Syrjälä wrote:
> > > On Thu, Jul 10, 2025 at 11:42:52AM -0400, Rodrigo Vivi wrote:
> > > > On Wed, Jul 09, 2025 at 06:11:17PM +0000, Hogander, Jouni
> > > > wrote:
> > > > > On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> > > > > > On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander
> > > > > > wrote:
> > > > > > > Currently disabling PSR2 via enable_psr module parameter
> > > > > > > causes
> > > > > > > Panel
> > > > > > > Replay being disabled as well. This patch changes this by
> > > > > > > still
> > > > > > > allowing
> > > > > > > Panel Replay even if PSR2 is disabled.
> > > > > > > 
> > > > > > > After this patch enable_psr module parameter values are:
> > > > > > > 
> > > > > > > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> > > > > > >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> > > > > > >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> > > > > > >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > > > > > >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > > > > > > 
> > > > > > > I.e. values different than -1 and 0 are handled as
> > > > > > > bitmasks where
> > > > > > > BIT0
> > > > > > > disables PSR2 and BIT1 disables Panel Replay.
> > > > > > > 
> > > > > > > v2:
> > > > > > >   - make it more clear that enable_psr is bitmask for
> > > > > > > disabling
> > > > > > > different
> > > > > > >     PSR modes
> > > > > > > 
> > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > > > ---
> > > > > > >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> > > > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 22
> > > > > > > ++++++++++++++-
> > > > > > > ----
> > > > > > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > > > > > 
> > > > > > > diff --git
> > > > > > > a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > > index 75316247ee8a..195af19ece5f 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > > @@ -116,9 +116,9 @@
> > > > > > > intel_display_param_named_unsafe(enable_fbc,
> > > > > > > int, 0400,
> > > > > > >  	"(default: -1 (use per-chip default))");
> > > > > > >  
> > > > > > >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > > > > > > -	"Enable PSR "
> > > > > > > -	"(0=disabled, 1=enable up to PSR1, 2=enable up
> > > > > > > to PSR2) "
> > > > > > > -	"Default: -1 (use per-chip default)");
> > > > > > > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0),
> > > > > > > 2=disable
> > > > > > > Panel Replay (BIT1))."
> > > > > > > +	"Values different from 0 and -1 are handled as
> > > > > > > bitmask to
> > > > > > > disable different PSR modes."
> > > > > > > +	"E.g. value 3 disables both PSR2 and Panel
> > > > > > > Replay.
> > > > > > > Default: -1 (use per-chip default)");
> > > > > > 
> > > > > > This thing is very unintuitive. Why don't we just get
> > > > > > replace it
> > > > > > with a new disable_psr modparam that is clearly just a
> > > > > > bitmask of
> > > > > > what to disable?
> > > > > 
> > > > > I was thinkinig we should keep it backward compatible. I know
> > > > > this
> > > > > parameter is in use.
> > > > 
> > > > I agree on keeping this backward compatible.
> > > 
> > > IMO it's an unusable mess so I wouldn't bother trying to preserve
> > > it.
> > > The only value that seems to make any sense currently is =0. 
> > 
> > fair enough. what about simply removing all the options entirely?
> > enable_psr=0 keeps disabling it, otherwise enabled it. And we
> > reduce
> > all the knobs option. Afterall, this should be our end goal anyway.
> > 
> > > If I
> > > need to use any other value I always give up immediately and just
> > > hack the code instead.
> > 
> > Well, the param actually exists for us to request reporters to try
> > different config. The devs can always modify the code.
> > 
> > Question now is, are all these variants useful for collecting debug
> > information of some sort?
> > 
> > If so, as long as it is documented and we can ask different values,
> > we should be good.
> > 
> > > 
> > > If we keep calling it 'enable_psr' then it should clearly be a
> > > bitmask of things to *enable*, not things to *disable*.
> > > 
> > > > 
> > > > Also our experience with disable_power_well shows that negative
> > > > name in the parameter can be much more unintuitive and
> > > > confusing.
> > > 
> > > That one is rather different because it doesn't "disable power
> > > wells"
> > > but rather it "disables power well disabling". But yes, it is a
> > > very
> > > poor name as well.
> > > 
> > > Calling it "enable_power_wells" wouldn't really help though.
> > > It should perhaps be something more like
> > > 'dont_disable_power_wells'
> > > or 'keep_power_wells_on'.
> > 
> > okay, fair enough, disable power well is another level of
> > complication.
> > 
> > back to disable_psr idea:
> > 
> > disable_psr=0 == enable PSR? to me this is already uninituitive
> > anyway.
> > disable_psr=1 == disable PSR1?
> > disable_psr=2 == disable PSR2? and keep only PSR=1?
> > 
> > I still don't see a clean obvious intuitive way of handling it.
> > Perhaps what I had suggested another day:
> > 
> > PSR1 = BIT0
> > PSR2 = BIT1 (PSR2 infers PSR1 enabled)
> > PANEL_REPLAY = BIT2 (also infers PSR1(and 2?) enabled)
> 
> With a bitmask I don't think inferring anything is helpful.
> If the corresponding bit isn't set then don't use that
> mode, period.
> 
> Another option would to have a separate named parameter
> for each mode. Would be easier to understand but dunno
> if we really want to add that many modparams just for this.

I'm now thinking adding enable_panel_replay would make most sense:

-1 : Enable chip default (Default)
 0 : Disable
 1 : Enable PR full frame update

Keep enable_psr as it is and remove all bindings to Panel Replay from
there. What do you think?

BR,

Jouni Högander

> 
> > (Peraps even bit3 for early transport?)
> > 
> > This is backwards compatible because
> > 
> > 0 = disabled,
> > 1 = up to psr1,
> > 2 = up to psr2, (no panel replay)
> > 3 = up to psr2, (same as 2)
> > 4 = panel replay on
> > ...
> > 
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 

Re: [PATCH v2 2/2] drm/i915/psr: Do not disable Panel Replay if PSR2 is disabled

[Rodrigo Vivi]

On Fri, Jul 11, 2025 at 10:33:58AM +0000, Hogander, Jouni wrote:
> On Fri, 2025-07-11 at 02:11 +0300, Ville Syrjälä wrote:
> > On Thu, Jul 10, 2025 at 05:27:13PM -0400, Rodrigo Vivi wrote:
> > > On Thu, Jul 10, 2025 at 11:09:42PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Jul 10, 2025 at 11:42:52AM -0400, Rodrigo Vivi wrote:
> > > > > On Wed, Jul 09, 2025 at 06:11:17PM +0000, Hogander, Jouni
> > > > > wrote:
> > > > > > On Wed, 2025-07-09 at 20:03 +0300, Ville Syrjälä wrote:
> > > > > > > On Wed, Jul 09, 2025 at 10:57:58AM +0300, Jouni Högander
> > > > > > > wrote:
> > > > > > > > Currently disabling PSR2 via enable_psr module parameter
> > > > > > > > causes
> > > > > > > > Panel
> > > > > > > > Replay being disabled as well. This patch changes this by
> > > > > > > > still
> > > > > > > > allowing
> > > > > > > > Panel Replay even if PSR2 is disabled.
> > > > > > > > 
> > > > > > > > After this patch enable_psr module parameter values are:
> > > > > > > > 
> > > > > > > > -1 = PSR1 : yes, PSR2 = yes, Panel Replay : yes
> > > > > > > >  0 = PSR1 : no,  PSR2 = no,  Panel Replay : no
> > > > > > > >  1 = PSR1 : yes, PSR2 = no,  Panel Replay : yes
> > > > > > > >  2 = PSR1 : yes, PSR2 = yes, Panel Replay : no
> > > > > > > >  3 = PSR1 : yes, PSR2 = no,  Panel Replay : no
> > > > > > > > 
> > > > > > > > I.e. values different than -1 and 0 are handled as
> > > > > > > > bitmasks where
> > > > > > > > BIT0
> > > > > > > > disables PSR2 and BIT1 disables Panel Replay.
> > > > > > > > 
> > > > > > > > v2:
> > > > > > > >   - make it more clear that enable_psr is bitmask for
> > > > > > > > disabling
> > > > > > > > different
> > > > > > > >     PSR modes
> > > > > > > > 
> > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > > > > ---
> > > > > > > >  .../drm/i915/display/intel_display_params.c   |  6 ++---
> > > > > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 22
> > > > > > > > ++++++++++++++-
> > > > > > > > ----
> > > > > > > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git
> > > > > > > > a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > > > index 75316247ee8a..195af19ece5f 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > > > > > @@ -116,9 +116,9 @@
> > > > > > > > intel_display_param_named_unsafe(enable_fbc,
> > > > > > > > int, 0400,
> > > > > > > >  	"(default: -1 (use per-chip default))");
> > > > > > > >  
> > > > > > > >  intel_display_param_named_unsafe(enable_psr, int, 0400,
> > > > > > > > -	"Enable PSR "
> > > > > > > > -	"(0=disabled, 1=enable up to PSR1, 2=enable up
> > > > > > > > to PSR2) "
> > > > > > > > -	"Default: -1 (use per-chip default)");
> > > > > > > > +	"Enable PSR (0=disabled, 1=disable PSR2 (BIT0),
> > > > > > > > 2=disable
> > > > > > > > Panel Replay (BIT1))."
> > > > > > > > +	"Values different from 0 and -1 are handled as
> > > > > > > > bitmask to
> > > > > > > > disable different PSR modes."
> > > > > > > > +	"E.g. value 3 disables both PSR2 and Panel
> > > > > > > > Replay.
> > > > > > > > Default: -1 (use per-chip default)");
> > > > > > > 
> > > > > > > This thing is very unintuitive. Why don't we just get
> > > > > > > replace it
> > > > > > > with a new disable_psr modparam that is clearly just a
> > > > > > > bitmask of
> > > > > > > what to disable?
> > > > > > 
> > > > > > I was thinkinig we should keep it backward compatible. I know
> > > > > > this
> > > > > > parameter is in use.
> > > > > 
> > > > > I agree on keeping this backward compatible.
> > > > 
> > > > IMO it's an unusable mess so I wouldn't bother trying to preserve
> > > > it.
> > > > The only value that seems to make any sense currently is =0. 
> > > 
> > > fair enough. what about simply removing all the options entirely?
> > > enable_psr=0 keeps disabling it, otherwise enabled it. And we
> > > reduce
> > > all the knobs option. Afterall, this should be our end goal anyway.
> > > 
> > > > If I
> > > > need to use any other value I always give up immediately and just
> > > > hack the code instead.
> > > 
> > > Well, the param actually exists for us to request reporters to try
> > > different config. The devs can always modify the code.
> > > 
> > > Question now is, are all these variants useful for collecting debug
> > > information of some sort?
> > > 
> > > If so, as long as it is documented and we can ask different values,
> > > we should be good.
> > > 
> > > > 
> > > > If we keep calling it 'enable_psr' then it should clearly be a
> > > > bitmask of things to *enable*, not things to *disable*.
> > > > 
> > > > > 
> > > > > Also our experience with disable_power_well shows that negative
> > > > > name in the parameter can be much more unintuitive and
> > > > > confusing.
> > > > 
> > > > That one is rather different because it doesn't "disable power
> > > > wells"
> > > > but rather it "disables power well disabling". But yes, it is a
> > > > very
> > > > poor name as well.
> > > > 
> > > > Calling it "enable_power_wells" wouldn't really help though.
> > > > It should perhaps be something more like
> > > > 'dont_disable_power_wells'
> > > > or 'keep_power_wells_on'.
> > > 
> > > okay, fair enough, disable power well is another level of
> > > complication.
> > > 
> > > back to disable_psr idea:
> > > 
> > > disable_psr=0 == enable PSR? to me this is already uninituitive
> > > anyway.
> > > disable_psr=1 == disable PSR1?
> > > disable_psr=2 == disable PSR2? and keep only PSR=1?
> > > 
> > > I still don't see a clean obvious intuitive way of handling it.
> > > Perhaps what I had suggested another day:
> > > 
> > > PSR1 = BIT0
> > > PSR2 = BIT1 (PSR2 infers PSR1 enabled)
> > > PANEL_REPLAY = BIT2 (also infers PSR1(and 2?) enabled)
> > 
> > With a bitmask I don't think inferring anything is helpful.
> > If the corresponding bit isn't set then don't use that
> > mode, period.
> > 
> > Another option would to have a separate named parameter
> > for each mode. Would be easier to understand but dunno
> > if we really want to add that many modparams just for this.
> 
> I'm now thinking adding enable_panel_replay would make most sense:
> 
> -1 : Enable chip default (Default)
>  0 : Disable
>  1 : Enable PR full frame update
> 
> Keep enable_psr as it is and remove all bindings to Panel Replay from
> there. What do you think?

yeap, perhaps it is the easiest and more intuintive way...

> 
> BR,
> 
> Jouni Högander
> 
> > 
> > > (Peraps even bit3 for early transport?)
> > > 
> > > This is backwards compatible because
> > > 
> > > 0 = disabled,
> > > 1 = up to psr1,
> > > 2 = up to psr2, (no panel replay)
> > > 3 = up to psr2, (same as 2)
> > > 4 = panel replay on
> > > ...
> > > 
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > 
>