[PATCH i-g-t 0/2] check modparam before enabling psr mode

[PATCH i-g-t 0/2] check modparam before enabling psr mode

[Kunal Joshi]

with i915.enable_psr modparam, one can enable required psr version

i915.enable_psr = 0 PSR_1 and PSR_2 tests should skip
i915.enable_psr = 1 PSR_1 tests should run but PSR_2 tests should skip
i915.enable_psr = 2 PSR_1 and PSR_2 tests should run

fix test to check modparam before enabling through psr_debug debugfs

Kunal Joshi (2):
  lib/igt_psr: add helper to check of given psr mode can be enabled
  tests/intel/kms_psr: enable given psr mode only after checking
    enable_psr modparam

 lib/igt_psr.c         | 23 +++++++++++++++++++++++
 lib/igt_psr.h         |  1 +
 tests/intel/kms_psr.c | 12 ++++++++++++
 3 files changed, 36 insertions(+)

-- 
2.25.1

[PATCH i-g-t 0/2] check modparam before enabling psr mode

[Kunal Joshi]

with i915.enable_psr modparam, one can enable required psr version

i915.enable_psr = 0 PSR_1 and PSR_2 tests should skip
i915.enable_psr = 1 PSR_1 tests should run but PSR_2 tests should skip
i915.enable_psr = 2 PSR_1 and PSR_2 tests should run

fix test to check modparam before enabling through psr_debug debugfs

Kunal Joshi (2):
  lib/igt_psr: add helper to check of given psr mode can be enabled
  tests/intel/kms_psr: enable given psr mode only after checking
    enable_psr modparam

 lib/igt_psr.c         | 20 ++++++++++++++++++++
 lib/igt_psr.h         |  1 +
 tests/intel/kms_psr.c | 12 ++++++++++++
 3 files changed, 33 insertions(+)

-- 
2.25.1

[PATCH i-g-t 1/2] lib/igt_psr: add helper to check of given psr mode can be enabled

[Kunal Joshi]

Check if given psr mode can be enabled by reading enable_psr
modparam

Cc: Jeevan B <jeevan.b@intel.com>
Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
Reviewed-by: Jeevan B <jeevan.b@intel.com>
---
 lib/igt_psr.c | 20 ++++++++++++++++++++
 lib/igt_psr.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index 663bac163..ac214fcfc 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -394,3 +394,23 @@ enum psr_mode psr_get_mode(int debugfs_fd)
 
 	return PSR_DISABLED;
 }
+
+/**
+ * is_psr_enable_possible
+ * Check if given psr mode can be enabled by reading enable_psr
+ * modparam
+ *
+ * Returns:
+ * True if given psr mode can be enabled, false otherwise.
+ */
+bool is_psr_enable_possible(int drm_fd, enum psr_mode mode)
+{
+	char *param_value;
+	int enable_psr;
+
+	param_value = __igt_params_get(drm_fd, "enable_psr");
+	igt_assert_f(param_value, "Could not read enable_psr modparam\n");
+	enable_psr = atoi(param_value);
+	free(param_value);
+	return enable_psr > mode;
+}
diff --git a/lib/igt_psr.h b/lib/igt_psr.h
index 36711c0d4..82a4e8c5e 100644
--- a/lib/igt_psr.h
+++ b/lib/igt_psr.h
@@ -61,5 +61,6 @@ bool i915_psr2_selective_fetch_check(int drm_fd);
 
 bool i915_psr2_sel_fetch_to_psr1(int drm_fd);
 void i915_psr2_sel_fetch_restore(int drm_fd);
+bool is_psr_enable_possible(int drm_fd, enum psr_mode mode);
 
 #endif
-- 
2.25.1

[PATCH i-g-t 2/2] tests/intel/kms_psr: enable given psr mode only after checking enable_psr modparam

[Kunal Joshi]

don't enable a psr mode with psr_debug debugfs, instead check what's
the modparam value for i915_enable_psr

Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
Reviewed-by: Jeevan B <jeevan.b@intel.com>
Tested-by: Vidya Srinivas <vidya.srinivas@intel.com>
---
 tests/intel/kms_psr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/intel/kms_psr.c b/tests/intel/kms_psr.c
index 349671b00..521d4c708 100644
--- a/tests/intel/kms_psr.c
+++ b/tests/intel/kms_psr.c
@@ -497,16 +497,28 @@ static void fill_render(data_t *data, const struct igt_fb *fb,
 
 static bool psr_wait_entry_if_enabled(data_t *data)
 {
+	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
+		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
+			 data->op_psr_mode);
+
 	return psr_wait_entry(data->debugfs_fd, data->op_psr_mode, data->output);
 }
 
 static bool psr_wait_update_if_enabled(data_t *data)
 {
+	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
+		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
+			 data->op_psr_mode);
+
 	return psr_wait_update(data->debugfs_fd, data->op_psr_mode, data->output);
 }
 
 static bool psr_enable_if_enabled(data_t *data)
 {
+	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
+		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
+			 data->op_psr_mode);
+
 	return psr_enable(data->drm_fd, data->debugfs_fd, data->op_psr_mode);
 }
 
-- 
2.25.1

✓ CI.xeBAT: success for check modparam before enabling psr mode (rev2)

[Patchwork]

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

== Series Details ==

Series: check modparam before enabling psr mode (rev2)
URL   : https://patchwork.freedesktop.org/series/129092/
State : success

== Summary ==

CI Bug Log - changes from XEIGT_7690_BAT -> XEIGTPW_10581_BAT
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (4 -> 4)
------------------------------

  No changes in participating hosts


Changes
-------

  No changes found


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

  * IGT: IGT_7690 -> IGTPW_10581
  * Linux: xe-669-741c439503ee0986a7b7c5b590cbd9813b8841f6 -> xe-674-0496fe20d8ed5e73ed186d293ef67e7bae4658eb

  IGTPW_10581: 10581
  IGT_7690: aa45298ff675abbe6bf8f04ae186e2388c35f03a @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  xe-669-741c439503ee0986a7b7c5b590cbd9813b8841f6: 741c439503ee0986a7b7c5b590cbd9813b8841f6
  xe-674-0496fe20d8ed5e73ed186d293ef67e7bae4658eb: 0496fe20d8ed5e73ed186d293ef67e7bae4658eb

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10581/index.html

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

✗ Fi.CI.BAT: failure for check modparam before enabling psr mode (rev2)

[Patchwork]

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

== Series Details ==

Series: check modparam before enabling psr mode (rev2)
URL   : https://patchwork.freedesktop.org/series/129092/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14168 -> IGTPW_10581
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_10581 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_10581, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

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

Participating hosts (38 -> 35)
------------------------------

  Additional (1): bat-kbl-2 
  Missing    (4): bat-rpls-2 bat-atsm-1 fi-snb-2520m fi-pnv-d510 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_10581:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - fi-tgl-1115g4:      [PASS][1] -> [SKIP][2] +5 other tests skip
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html

  * igt@kms_pipe_crc_basic@hang-read-crc:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][3] +6 other tests skip
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@kms_pipe_crc_basic@hang-read-crc.html

  
#### Warnings ####

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-tgl-1115g4:      [SKIP][4] ([i915#4103]) -> [SKIP][5] +1 other test skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_dsc@dsc-basic:
    - fi-tgl-1115g4:      [SKIP][6] ([i915#3555] / [i915#3840]) -> [SKIP][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@kms_dsc@dsc-basic.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@kms_dsc@dsc-basic.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@fbdev@info:
    - bat-kbl-2:          NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#1849])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/bat-kbl-2/igt@fbdev@info.html
    - fi-tgl-1115g4:      [PASS][9] -> [SKIP][10] ([i915#1849] / [i915#2582])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@fbdev@info.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@fbdev@info.html

  * igt@fbdev@nullptr:
    - fi-tgl-1115g4:      [PASS][11] -> [SKIP][12] ([i915#2582]) +3 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@fbdev@nullptr.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@fbdev@nullptr.html

  * igt@gem_lmem_swapping@basic:
    - fi-apl-guc:         NOTRUN -> [SKIP][13] ([fdo#109271] / [i915#4613]) +3 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-apl-guc/igt@gem_lmem_swapping@basic.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-kbl-2:          NOTRUN -> [SKIP][14] ([fdo#109271]) +35 other tests skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/bat-kbl-2/igt@gem_lmem_swapping@parallel-random-engines.html

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

  * igt@i915_hangman@error-state-basic:
    - bat-mtlp-6:         [PASS][16] -> [ABORT][17] ([i915#9414])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/bat-mtlp-6/igt@i915_hangman@error-state-basic.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/bat-mtlp-6/igt@i915_hangman@error-state-basic.html

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

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

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

  * igt@kms_busy@basic:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][21] ([i915#4303])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@kms_busy@basic.html

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

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

  * igt@kms_flip@basic-plain-flip:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][24] ([i915#3637]) +3 other tests skip
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@kms_flip@basic-plain-flip.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-mtlp-8:         NOTRUN -> [SKIP][25] ([fdo#109285])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/bat-mtlp-8/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-mtlp-8:         NOTRUN -> [SKIP][26] ([i915#5274])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/bat-mtlp-8/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-tgl-1115g4:      [PASS][27] -> [SKIP][28] ([i915#1849])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@kms_frontbuffer_tracking@basic.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_hdmi_inject@inject-audio:
    - fi-apl-guc:         NOTRUN -> [SKIP][29] ([fdo#109271]) +13 other tests skip
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-apl-guc/igt@kms_hdmi_inject@inject-audio.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         NOTRUN -> [SKIP][30] ([i915#9197]) +1 other test skip
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

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

  * igt@prime_vgem@basic-fence-flip:
    - fi-tgl-1115g4:      [PASS][32] -> [SKIP][33] ([fdo#109295])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/fi-tgl-1115g4/igt@prime_vgem@basic-fence-flip.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/fi-tgl-1115g4/igt@prime_vgem@basic-fence-flip.html

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

  * igt@prime_vgem@basic-fence-read:
    - bat-mtlp-8:         NOTRUN -> [SKIP][35] ([i915#3708]) +2 other tests skip
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/bat-mtlp-8/igt@prime_vgem@basic-fence-read.html

  
#### Possible fixes ####

  * igt@i915_hangman@error-state-basic:
    - bat-mtlp-8:         [ABORT][36] ([i915#9414]) -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14168/bat-mtlp-8/igt@i915_hangman@error-state-basic.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10581/bat-mtlp-8/igt@i915_hangman@error-state-basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4303]: https://gitlab.freedesktop.org/drm/intel/issues/4303
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
  [i915#9159]: https://gitlab.freedesktop.org/drm/intel/issues/9159
  [i915#9197]: https://gitlab.freedesktop.org/drm/intel/issues/9197
  [i915#9414]: https://gitlab.freedesktop.org/drm/intel/issues/9414
  [i915#9688]: https://gitlab.freedesktop.org/drm/intel/issues/9688


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

  * CI: CI-20190529 -> None
  * IGT: IGT_7690 -> IGTPW_10581

  CI-20190529: 20190529
  CI_DRM_14168: 0496fe20d8ed5e73ed186d293ef67e7bae4658eb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_10581: 10581
  IGT_7690: aa45298ff675abbe6bf8f04ae186e2388c35f03a @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

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

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

Re: [PATCH i-g-t 1/2] lib/igt_psr: add helper to check of given psr mode can be enabled

[Modem, Bhanuprakash]

Hi Kunal,

On 24-01-2024 04:04 pm, Kunal Joshi wrote:
> Check if given psr mode can be enabled by reading enable_psr
> modparam
> 
> Cc: Jeevan B <jeevan.b@intel.com>
> Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> Reviewed-by: Jeevan B <jeevan.b@intel.com>
> ---
>   lib/igt_psr.c | 20 ++++++++++++++++++++
>   lib/igt_psr.h |  1 +
>   2 files changed, 21 insertions(+)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index 663bac163..ac214fcfc 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -394,3 +394,23 @@ enum psr_mode psr_get_mode(int debugfs_fd)
>   
>   	return PSR_DISABLED;
>   }
> +
> +/**
> + * is_psr_enable_possible
> + * Check if given psr mode can be enabled by reading enable_psr
> + * modparam
> + *
> + * Returns:
> + * True if given psr mode can be enabled, false otherwise.
> + */
> +bool is_psr_enable_possible(int drm_fd, enum psr_mode mode)
> +{
> +	char *param_value;
> +	int enable_psr;
> +
> +	param_value = __igt_params_get(drm_fd, "enable_psr");
> +	igt_assert_f(param_value, "Could not read enable_psr modparam\n");

IMHO, Please don't assert here. Instead, allow the user to decide if 
modparam "enable_psr" is not available.

- Bhanu

> +	enable_psr = atoi(param_value);
> +	free(param_value);
> +	return enable_psr > mode;
> +}
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> index 36711c0d4..82a4e8c5e 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -61,5 +61,6 @@ bool i915_psr2_selective_fetch_check(int drm_fd);
>   
>   bool i915_psr2_sel_fetch_to_psr1(int drm_fd);
>   void i915_psr2_sel_fetch_restore(int drm_fd);
> +bool is_psr_enable_possible(int drm_fd, enum psr_mode mode);
>   
>   #endif

RE: [PATCH i-g-t 1/2] lib/igt_psr: add helper to check of given psr mode can be enabled

[Joshi, Kunal1]

Hell bhanu

-----Original Message-----
From: Modem, Bhanuprakash <bhanuprakash.modem@intel.com> 
Sent: Wednesday, January 24, 2024 5:28 PM
To: Joshi, Kunal1 <kunal1.joshi@intel.com>; igt-dev@lists.freedesktop.org
Cc: B, Jeevan <jeevan.b@intel.com>
Subject: Re: [PATCH i-g-t 1/2] lib/igt_psr: add helper to check of given psr mode can be enabled

Hi Kunal,

On 24-01-2024 04:04 pm, Kunal Joshi wrote:
> Check if given psr mode can be enabled by reading enable_psr modparam
> 
> Cc: Jeevan B <jeevan.b@intel.com>
> Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> Reviewed-by: Jeevan B <jeevan.b@intel.com>
> ---
>   lib/igt_psr.c | 20 ++++++++++++++++++++
>   lib/igt_psr.h |  1 +
>   2 files changed, 21 insertions(+)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c index 663bac163..ac214fcfc 
> 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -394,3 +394,23 @@ enum psr_mode psr_get_mode(int debugfs_fd)
>   
>   	return PSR_DISABLED;
>   }
> +
> +/**
> + * is_psr_enable_possible
> + * Check if given psr mode can be enabled by reading enable_psr
> + * modparam
> + *
> + * Returns:
> + * True if given psr mode can be enabled, false otherwise.
> + */
> +bool is_psr_enable_possible(int drm_fd, enum psr_mode mode) {
> +	char *param_value;
> +	int enable_psr;
> +
> +	param_value = __igt_params_get(drm_fd, "enable_psr");
> +	igt_assert_f(param_value, "Could not read enable_psr modparam\n");

IMHO, Please don't assert here. Instead, allow the user to decide if modparam "enable_psr" is not available.

- Bhanu

I think it's fine to assert since we expect enable_psr to be present,
since the caller may not handle this, one less thing to worry 😊

I can see this is done similarly at many place 
example :- intel_pipe_output_combo_valid

> +	enable_psr = atoi(param_value);
> +	free(param_value);
> +	return enable_psr > mode;
> +}
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h index 36711c0d4..82a4e8c5e 
> 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -61,5 +61,6 @@ bool i915_psr2_selective_fetch_check(int drm_fd);
>   
>   bool i915_psr2_sel_fetch_to_psr1(int drm_fd);
>   void i915_psr2_sel_fetch_restore(int drm_fd);
> +bool is_psr_enable_possible(int drm_fd, enum psr_mode mode);
>   
>   #endif[
Thanks and Regards
[Kunal Joshi] 

Re: [PATCH i-g-t 1/2] lib/igt_psr: add helper to check of given psr mode can be enabled

[Modem, Bhanuprakash]

Hi Kunal,

On 24-01-2024 05:36 pm, Joshi, Kunal1 wrote:
> Hell bhanu
> 
> -----Original Message-----
> From: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> Sent: Wednesday, January 24, 2024 5:28 PM
> To: Joshi, Kunal1 <kunal1.joshi@intel.com>; igt-dev@lists.freedesktop.org
> Cc: B, Jeevan <jeevan.b@intel.com>
> Subject: Re: [PATCH i-g-t 1/2] lib/igt_psr: add helper to check of given psr mode can be enabled
> 
> Hi Kunal,
> 
> On 24-01-2024 04:04 pm, Kunal Joshi wrote:
>> Check if given psr mode can be enabled by reading enable_psr modparam
>>
>> Cc: Jeevan B <jeevan.b@intel.com>
>> Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
>> Reviewed-by: Jeevan B <jeevan.b@intel.com>
>> ---
>>    lib/igt_psr.c | 20 ++++++++++++++++++++
>>    lib/igt_psr.h |  1 +
>>    2 files changed, 21 insertions(+)
>>
>> diff --git a/lib/igt_psr.c b/lib/igt_psr.c index 663bac163..ac214fcfc
>> 100644
>> --- a/lib/igt_psr.c
>> +++ b/lib/igt_psr.c
>> @@ -394,3 +394,23 @@ enum psr_mode psr_get_mode(int debugfs_fd)
>>    
>>    	return PSR_DISABLED;
>>    }
>> +
>> +/**
>> + * is_psr_enable_possible
>> + * Check if given psr mode can be enabled by reading enable_psr
>> + * modparam
>> + *
>> + * Returns:
>> + * True if given psr mode can be enabled, false otherwise.
>> + */
>> +bool is_psr_enable_possible(int drm_fd, enum psr_mode mode) {
>> +	char *param_value;
>> +	int enable_psr;
>> +
>> +	param_value = __igt_params_get(drm_fd, "enable_psr");
>> +	igt_assert_f(param_value, "Could not read enable_psr modparam\n");
> 
> IMHO, Please don't assert here. Instead, allow the user to decide if modparam "enable_psr" is not available.
> 
> - Bhanu
> 
> I think it's fine to assert since we expect enable_psr to be present,
> since the caller may not handle this, one less thing to worry 😊

I am just thinking about the scenario, where the panel is capable of PSR 
but failed to read "enable_psr".

As enable_psr is already present, I guess, we are OK with this change.

Acked-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>

> 
> I can see this is done similarly at many place
> example :- intel_pipe_output_combo_valid

Then we must review & fix them if required :-)

- Bhanu

> 
>> +	enable_psr = atoi(param_value);
>> +	free(param_value);
>> +	return enable_psr > mode;
>> +}
>> diff --git a/lib/igt_psr.h b/lib/igt_psr.h index 36711c0d4..82a4e8c5e
>> 100644
>> --- a/lib/igt_psr.h
>> +++ b/lib/igt_psr.h
>> @@ -61,5 +61,6 @@ bool i915_psr2_selective_fetch_check(int drm_fd);
>>    
>>    bool i915_psr2_sel_fetch_to_psr1(int drm_fd);
>>    void i915_psr2_sel_fetch_restore(int drm_fd);
>> +bool is_psr_enable_possible(int drm_fd, enum psr_mode mode);
>>    
>>    #endif[
> Thanks and Regards
> [Kunal Joshi]

Re: [PATCH i-g-t 2/2] tests/intel/kms_psr: enable given psr mode only after checking enable_psr modparam

[Modem, Bhanuprakash]

Hi Kunal,

On 24-01-2024 04:04 pm, Kunal Joshi wrote:
> don't enable a psr mode with psr_debug debugfs, instead check what's
> the modparam value for i915_enable_psr
> 
> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> Reviewed-by: Jeevan B <jeevan.b@intel.com>
> Tested-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>   tests/intel/kms_psr.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/tests/intel/kms_psr.c b/tests/intel/kms_psr.c
> index 349671b00..521d4c708 100644
> --- a/tests/intel/kms_psr.c
> +++ b/tests/intel/kms_psr.c
> @@ -497,16 +497,28 @@ static void fill_render(data_t *data, const struct igt_fb *fb,
>   
>   static bool psr_wait_entry_if_enabled(data_t *data)
>   {
> +	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
> +		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
> +			 data->op_psr_mode);

This change will cause the skips in dynamic subtests. Skipping the 
dynamic subtest is not recommended, instead don't run the test.

So the proper place for this check would be before calling the 
igt_dynamic().

- Bhanu

> +
>   	return psr_wait_entry(data->debugfs_fd, data->op_psr_mode, data->output);
>   }
>   
>   static bool psr_wait_update_if_enabled(data_t *data)
>   {
> +	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
> +		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
> +			 data->op_psr_mode);
> +
>   	return psr_wait_update(data->debugfs_fd, data->op_psr_mode, data->output);
>   }
>   
>   static bool psr_enable_if_enabled(data_t *data)
>   {
> +	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
> +		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
> +			 data->op_psr_mode);
> +
>   	return psr_enable(data->drm_fd, data->debugfs_fd, data->op_psr_mode);
>   }
>   

RE: [PATCH i-g-t 2/2] tests/intel/kms_psr: enable given psr mode only after checking enable_psr modparam

[Joshi, Kunal1]

Hello Bhanu,

-----Original Message-----
From: Modem, Bhanuprakash <bhanuprakash.modem@intel.com> 
Sent: Wednesday, January 24, 2024 8:13 PM
To: Joshi, Kunal1 <kunal1.joshi@intel.com>; igt-dev@lists.freedesktop.org; B, Jeevan <jeevan.b@intel.com>; Srinivas, Vidya <vidya.srinivas@intel.com>
Subject: Re: [PATCH i-g-t 2/2] tests/intel/kms_psr: enable given psr mode only after checking enable_psr modparam

Hi Kunal,

On 24-01-2024 04:04 pm, Kunal Joshi wrote:
> don't enable a psr mode with psr_debug debugfs, instead check what's 
> the modparam value for i915_enable_psr
> 
> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
> Reviewed-by: Jeevan B <jeevan.b@intel.com>
> Tested-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>   tests/intel/kms_psr.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/tests/intel/kms_psr.c b/tests/intel/kms_psr.c index 
> 349671b00..521d4c708 100644
> --- a/tests/intel/kms_psr.c
> +++ b/tests/intel/kms_psr.c
> @@ -497,16 +497,28 @@ static void fill_render(data_t *data, const 
> struct igt_fb *fb,
>   
>   static bool psr_wait_entry_if_enabled(data_t *data)
>   {
> +	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
> +		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
> +			 data->op_psr_mode);

This change will cause the skips in dynamic subtests. Skipping the dynamic subtest is not recommended, instead don't run the test.

So the proper place for this check would be before calling the igt_dynamic().

- Bhanu

I would advise we change this approach and skip dynamic test
instead of using continue and potentially hiding an issue.

> +
>   	return psr_wait_entry(data->debugfs_fd, data->op_psr_mode, data->output);
>   }
>   
>   static bool psr_wait_update_if_enabled(data_t *data)
>   {
> +	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
> +		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
> +			 data->op_psr_mode);
> +
>   	return psr_wait_update(data->debugfs_fd, data->op_psr_mode, data->output);
>   }
>   
>   static bool psr_enable_if_enabled(data_t *data)
>   {
> +	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
> +		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
> +			 data->op_psr_mode);
> +
>   	return psr_enable(data->drm_fd, data->debugfs_fd, data->op_psr_mode);
>   }
>
Thanks and Regards
Kunal Joshi