All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
@ 2018-08-14 12:22 Maarten Lankhorst
  2018-08-14 12:22 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v4 Maarten Lankhorst
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Maarten Lankhorst @ 2018-08-14 12:22 UTC (permalink / raw
  To: igt-dev

It's harmful to write to enable_psr at runtime, and the patch that allows
us to change i915_edp_psr_debug with the panel running will require us
to abandon the module parameter. Hence the userspace change needs to be
put in IGT first before we can change it at kernel time.

Toggling it to debugfs will mean we can skip a modeset when changing our
feature set.

Changes since v1:
- Rebase with the previous patches dropped.
Changes since v2:
- Rebase on top of new api in i915_edp_psr_debug.
Changes since v3:
- Enable IRQ debugging for extra logging.
- Force PSR1 mode. (dhnkrn)
- Move PSR enable/disable functions to lib/igt_psr. (dhnkrn)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 lib/igt_psr.c                    | 90 +++++++++++++++++++++++++++++++-
 lib/igt_psr.h                    |  2 +
 tests/kms_frontbuffer_tracking.c |  6 +--
 3 files changed, 93 insertions(+), 5 deletions(-)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index c979b0b59936..393702037861 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -21,7 +21,9 @@
  * IN THE SOFTWARE.
  */
 
-#include  "igt_psr.h"
+#include "igt_psr.h"
+#include "igt_sysfs.h"
+#include <errno.h>
 
 bool psr_active(int fd, bool check_active)
 {
@@ -38,3 +40,89 @@ bool psr_wait_entry(int fd)
 {
 	return igt_wait(psr_active(fd, true), 500, 1);
 }
+
+static ssize_t psr_write(int fd, const char *buf)
+{
+	return igt_sysfs_write(fd, "i915_edp_psr_debug", buf, strlen(buf));
+}
+
+static int has_psr_debugfs(int fd)
+{
+	int ret;
+
+	/*
+	 * Check if new PSR debugfs api is usable by writing an invalid value.
+	 * Legacy mode will return OK here, debugfs api will return -EINVAL.
+	 * -ENODEV is returned.
+	 */
+	ret = psr_write(fd, "0xf");
+	if (ret == -EINVAL)
+		return 0;
+	else if (ret >= 0)
+		return -EINVAL;
+	else
+		return ret;
+}
+
+static bool psr_modparam_set(int val)
+{
+	static int oldval = -1;
+
+	igt_set_module_param_int("enable_psr", val);
+
+	if (val == oldval)
+		return false;
+
+	oldval = val;
+	return true;
+}
+
+static int psr_restore_debugfs_fd = -1;
+
+static void restore_psr_debugfs(int sig)
+{
+	psr_write(psr_restore_debugfs_fd, "0");
+}
+
+static bool psr_set(int fd, bool enable)
+{
+	int ret;
+
+	igt_skip_on_f(fd < 0, "Could not open debugfs dir\n");
+
+	ret = has_psr_debugfs(fd);
+	if (ret == -ENODEV) {
+		igt_skip_on_f(enable, "PSR not available\n");
+		return false;
+	}
+
+	/*
+	 * Restore original debugfs value on exit, even for
+	 * legacy case, because writing 0xf will set the debug flag
+	 * regardless.
+	 */
+	if (psr_restore_debugfs_fd == -1) {
+		psr_restore_debugfs_fd = dup(fd);
+		igt_assert(psr_restore_debugfs_fd >= 0);
+		igt_install_exit_handler(restore_psr_debugfs);
+	}
+
+	if (ret == -EINVAL) {
+		ret = psr_modparam_set(enable);
+	} else {
+		ret = psr_write(fd, enable ? "0x13" : "1");
+		igt_assert(ret > 0);
+	}
+
+	return ret;
+}
+
+bool psr_enable(int fd)
+{
+	return psr_set(fd, true);
+}
+
+bool psr_disable(int fd)
+{
+	return psr_set(fd, false);
+}
diff --git a/lib/igt_psr.h b/lib/igt_psr.h
index 980f85e04b3e..0ef22c3d1258 100644
--- a/lib/igt_psr.h
+++ b/lib/igt_psr.h
@@ -30,5 +30,7 @@
 
 bool psr_wait_entry(int fd);
 bool psr_active(int fd, bool check_active);
+bool psr_enable(int fd);
+bool psr_disable(int fd);
 
 #endif
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 1dfd7c1cee8d..7ea2f697184d 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -941,8 +941,6 @@ static bool drrs_wait_until_rr_switch_to_low(void)
 
 #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
 #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
-#define psr_enable() igt_set_module_param_int("enable_psr", 1)
-#define psr_disable() igt_set_module_param_int("enable_psr", 0)
 #define drrs_enable()	drrs_set(1)
 #define drrs_disable()	drrs_set(0)
 
@@ -1137,7 +1135,7 @@ static void disable_features(const struct test_mode *t)
 		return;
 
 	fbc_disable();
-	psr_disable();
+	psr_disable(drm.debugfs);
 	drrs_disable();
 }
 
@@ -1719,7 +1717,7 @@ static void enable_features_for_test(const struct test_mode *t)
 	if (t->feature & FEATURE_FBC)
 		fbc_enable();
 	if (t->feature & FEATURE_PSR)
-		psr_enable();
+		psr_enable(drm.debugfs);
 	if (t->feature & FEATURE_DRRS)
 		drrs_enable();
 }
-- 
2.18.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v4.
  2018-08-14 12:22 [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4 Maarten Lankhorst
@ 2018-08-14 12:22 ` Maarten Lankhorst
  2018-08-17  6:53   ` Dhinakaran Pandiyan
  2018-08-14 12:48 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] lib/psr: Add support for toggling edp psr through debugfs, v4 Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Maarten Lankhorst @ 2018-08-14 12:22 UTC (permalink / raw
  To: igt-dev

CRC capturing enables the display, then disables it again. With
igt_display we can use igt_display_reset to restore the original state,
without committing it to the hw.

All subtests first set their own state anyway, so we can save up on
the number of commits.

Changes since v1:
- Try to avoid modesets for PSR if the kernel supports it, but otherwise force
  a modeset for the changes to take effect.
Changes since v2:
- Rebase on top of previous PSR changes.
Changes since v3:
- Rebase on move to lib/igt_psr.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 tests/kms_frontbuffer_tracking.c | 57 ++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 7ea2f697184d..2cec30da2750 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1129,14 +1129,14 @@ static void unset_all_crtcs(void)
 	igt_display_commit(&drm.display);
 }
 
-static void disable_features(const struct test_mode *t)
+static bool disable_features(const struct test_mode *t)
 {
 	if (t->feature == FEATURE_DEFAULT)
-		return;
+		return false;
 
 	fbc_disable();
-	psr_disable(drm.debugfs);
 	drrs_disable();
+	return psr_disable(drm.debugfs);
 }
 
 static void *busy_thread_func(void *data)
@@ -1220,7 +1220,7 @@ static void init_blue_crc(enum pixel_format format)
 
 	print_crc("Blue CRC:  ", &blue_crcs[format].crc);
 
-	unset_all_crtcs();
+	igt_display_reset(&drm.display);
 
 	igt_remove_fb(drm.fd, &blue);
 
@@ -1272,7 +1272,7 @@ static void init_crcs(enum pixel_format format,
 		print_crc("", &pattern->crcs[format][r]);
 	}
 
-	unset_all_crtcs();
+	igt_display_reset(&drm.display);
 
 	for (r = 0; r < pattern->n_rects; r++)
 		igt_remove_fb(drm.fd, &tmp_fbs[r]);
@@ -1695,6 +1695,22 @@ static void enable_scnd_screen_and_wait(const struct test_mode *t)
 	do_assertions(ASSERT_NO_ACTION_CHANGE);
 }
 
+static void enable_both_screens_and_wait(const struct test_mode *t)
+{
+	fill_fb_region(&prim_mode_params.primary, COLOR_PRIM_BG);
+	fill_fb_region(&scnd_mode_params.primary, COLOR_SCND_BG);
+
+	__set_mode_for_params(&prim_mode_params);
+	__set_mode_for_params(&scnd_mode_params);
+
+	igt_display_commit2(&drm.display, drm.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+
+	wanted_crc = &blue_crcs[t->format].crc;
+	fbc_update_last_action();
+
+	do_assertions(ASSERT_NO_ACTION_CHANGE);
+}
+
 static void set_region_for_test(const struct test_mode *t,
 				struct fb_region *reg)
 {
@@ -1709,17 +1725,21 @@ static void set_region_for_test(const struct test_mode *t,
 	do_assertions(ASSERT_NO_ACTION_CHANGE);
 }
 
-static void enable_features_for_test(const struct test_mode *t)
+static bool enable_features_for_test(const struct test_mode *t)
 {
+	bool ret = false;
+
 	if (t->feature == FEATURE_DEFAULT)
-		return;
+		return false;
 
 	if (t->feature & FEATURE_FBC)
 		fbc_enable();
 	if (t->feature & FEATURE_PSR)
-		psr_enable(drm.debugfs);
+		ret = psr_enable(drm.debugfs);
 	if (t->feature & FEATURE_DRRS)
 		drrs_enable();
+
+	return ret;
 }
 
 static void check_test_requirements(const struct test_mode *t)
@@ -1801,28 +1821,40 @@ static void set_crtc_fbs(const struct test_mode *t)
 static void prepare_subtest_data(const struct test_mode *t,
 				 struct draw_pattern_info *pattern)
 {
+	bool need_modeset;
+
 	check_test_requirements(t);
 
 	stop_busy_thread();
 
-	disable_features(t);
+	need_modeset = disable_features(t);
 	set_crtc_fbs(t);
 
 	if (t->screen == SCREEN_OFFSCREEN)
 		fill_fb_region(&offscreen_fb, COLOR_OFFSCREEN_BG);
 
-	unset_all_crtcs();
+	igt_display_reset(&drm.display);
+	if (need_modeset)
+		igt_display_commit(&drm.display);
 
 	init_blue_crc(t->format);
 	if (pattern)
 		init_crcs(t->format, pattern);
 
-	enable_features_for_test(t);
+	igt_display_reset(&drm.display);
+
+	need_modeset = enable_features_for_test(t);
+	if (need_modeset)
+		igt_display_commit(&drm.display);
 }
 
 static void prepare_subtest_screens(const struct test_mode *t)
 {
-	enable_prim_screen_and_wait(t);
+	if (t->pipes == PIPE_DUAL)
+		enable_both_screens_and_wait(t);
+	else
+		enable_prim_screen_and_wait(t);
+
 	if (t->screen == SCREEN_PRIM) {
 		if (t->plane == PLANE_CUR)
 			set_region_for_test(t, &prim_mode_params.cursor);
@@ -1833,7 +1865,6 @@ static void prepare_subtest_screens(const struct test_mode *t)
 	if (t->pipes == PIPE_SINGLE)
 		return;
 
-	enable_scnd_screen_and_wait(t);
 	if (t->screen == SCREEN_SCND) {
 		if (t->plane == PLANE_CUR)
 			set_region_for_test(t, &scnd_mode_params.cursor);
-- 
2.18.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
  2018-08-14 12:22 [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4 Maarten Lankhorst
  2018-08-14 12:22 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v4 Maarten Lankhorst
@ 2018-08-14 12:48 ` Patchwork
  2018-08-14 15:29 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
  2018-08-16  0:26 ` [igt-dev] [PATCH i-g-t 1/2] " Dhinakaran Pandiyan
  3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-08-14 12:48 UTC (permalink / raw
  To: Maarten Lankhorst; +Cc: igt-dev

== Series Details ==

Series: series starting with [i-g-t,1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
URL   : https://patchwork.freedesktop.org/series/48179/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4664 -> IGTPW_1708 =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1708 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1708, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48179/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@drv_selftest@live_uncore:
      fi-pnv-d510:        PASS -> SKIP +4

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_workarounds:
      {fi-cfl-8109u}:     PASS -> DMESG-FAIL (fdo#107292)
      fi-cnl-psr:         PASS -> DMESG-FAIL (fdo#107292)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#107362)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         DMESG-FAIL (fdo#107164) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#107139, fdo#105128) -> PASS

    igt@kms_chamelium@dp-crc-fast:
      fi-kbl-7500u:       DMESG-FAIL (fdo#103841) -> PASS

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

  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362


== Participating hosts (54 -> 48) ==

  Missing    (6): fi-ilk-m540 fi-bxt-dsi fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * IGT: IGT_4593 -> IGTPW_1708

  CI_DRM_4664: 19e458884fe1d8d10e453529933199250cc8821f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1708: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1708/
  IGT_4593: c88e219c6e890d89b7836c5e248ffedf334d55a2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1708/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
  2018-08-14 12:22 [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4 Maarten Lankhorst
  2018-08-14 12:22 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v4 Maarten Lankhorst
  2018-08-14 12:48 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] lib/psr: Add support for toggling edp psr through debugfs, v4 Patchwork
@ 2018-08-14 15:29 ` Patchwork
  2018-08-16  0:26 ` [igt-dev] [PATCH i-g-t 1/2] " Dhinakaran Pandiyan
  3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-08-14 15:29 UTC (permalink / raw
  To: Maarten Lankhorst; +Cc: igt-dev

== Series Details ==

Series: series starting with [i-g-t,1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
URL   : https://patchwork.freedesktop.org/series/48179/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4593_full -> IGTPW_1708_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1708_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1708_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48179/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_busy@extended-bsd1:
      shard-snb:          SKIP -> INCOMPLETE (fdo#105411)

    igt@gem_exec_schedule@pi-ringfull-render:
      shard-glk:          NOTRUN -> FAIL (fdo#103158)

    igt@gem_workarounds@suspend-resume:
      shard-glk:          PASS -> FAIL (fdo#103375)

    igt@kms_chv_cursor_fail@pipe-b-128x128-bottom-edge:
      shard-kbl:          PASS -> DMESG-WARN (fdo#105602, fdo#103558) +5

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
      shard-glk:          PASS -> FAIL (fdo#103167)

    igt@pm_rpm@system-suspend-modeset:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    
    ==== Possible fixes ====

    igt@drv_suspend@shrink:
      shard-snb:          INCOMPLETE (fdo#106886, fdo#105411) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#102887, fdo#105363) -> PASS

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-snb:          FAIL (fdo#103925) -> PASS

    igt@perf_pmu@busy-accuracy-50-bcs0:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * IGT: IGT_4593 -> IGTPW_1708
    * Linux: CI_DRM_4660 -> CI_DRM_4664

  CI_DRM_4660: 9cd882754c1017e68ed9d2c8d57dc326530522fe @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4664: 19e458884fe1d8d10e453529933199250cc8821f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1708: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1708/
  IGT_4593: c88e219c6e890d89b7836c5e248ffedf334d55a2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1708/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
  2018-08-14 12:22 [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4 Maarten Lankhorst
                   ` (2 preceding siblings ...)
  2018-08-14 15:29 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
@ 2018-08-16  0:26 ` Dhinakaran Pandiyan
  2018-08-16 11:41   ` Maarten Lankhorst
  3 siblings, 1 reply; 9+ messages in thread
From: Dhinakaran Pandiyan @ 2018-08-16  0:26 UTC (permalink / raw
  To: Maarten Lankhorst, igt-dev

On Tue, 2018-08-14 at 14:22 +0200, Maarten Lankhorst wrote:
> It's harmful to write to enable_psr at runtime, and the patch that
> allows
> us to change i915_edp_psr_debug with the panel running will require
> us
> to abandon the module parameter. Hence the userspace change needs to
> be
> put in IGT first before we can change it at kernel time.
> 
> Toggling it to debugfs will mean we can skip a modeset when changing
> our
> feature set.
> 
> Changes since v1:
> - Rebase with the previous patches dropped.
> Changes since v2:
> - Rebase on top of new api in i915_edp_psr_debug.
> Changes since v3:
> - Enable IRQ debugging for extra logging.
> - Force PSR1 mode. (dhnkrn)
> - Move PSR enable/disable functions to lib/igt_psr. (dhnkrn)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  lib/igt_psr.c                    | 90
> +++++++++++++++++++++++++++++++-
>  lib/igt_psr.h                    |  2 +
>  tests/kms_frontbuffer_tracking.c |  6 +--
>  3 files changed, 93 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index c979b0b59936..393702037861 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -21,7 +21,9 @@
>   * IN THE SOFTWARE.
>   */
>  
> -#include  "igt_psr.h"
> +#include "igt_psr.h"
> +#include "igt_sysfs.h"
> +#include <errno.h>
>  
>  bool psr_active(int fd, bool check_active)
>  {
> @@ -38,3 +40,89 @@ bool psr_wait_entry(int fd)
>  {
>  	return igt_wait(psr_active(fd, true), 500, 1);
>  }
> +
> +static ssize_t psr_write(int fd, const char *buf)
> +{
> +	return igt_sysfs_write(fd, "i915_edp_psr_debug", buf,
> strlen(buf));
> +}
> +
> +static int has_psr_debugfs(int fd)
> +{
> +	int ret;
> +
> +	/*
> +	 * Check if new PSR debugfs api is usable by writing an
> invalid value.
> +	 * Legacy mode will return OK here, debugfs api will return
> -EINVAL.
> +	 * -ENODEV is returned.
> +	 */
> +	ret = psr_write(fd, "0xf");
> +	if (ret == -EINVAL)
> +		return 0;
> +	else if (ret >= 0)
> +		return -EINVAL;
We should disable debug here, PSR interrupts aren't used in IGTs. Looks
 like you sent this patch before I responded to your question about
interrupts.


> +	else
> +		return ret;
> +}
> +
> +static bool psr_modparam_set(int val)
> +{
> +	static int oldval = -1;
> +
> +	igt_set_module_param_int("enable_psr", val);
> +
> +	if (val == oldval)
> +		return false;
> +
> +	oldval = val;
> +	return true;
> +}
> +
> +static int psr_restore_debugfs_fd = -1;
> +
> +static void restore_psr_debugfs(int sig)
> +{
> +	psr_write(psr_restore_debugfs_fd, "0");
> +}
> +
> +static bool psr_set(int fd, bool enable)
> +{
> +	int ret;
> +
> +	igt_skip_on_f(fd < 0, "Could not open debugfs dir\n");
> +
> +	ret = has_psr_debugfs(fd);
> +	if (ret == -ENODEV) {
> +		igt_skip_on_f(enable, "PSR not available\n");
> +		return false;
> +	}
> +
> +	/*
> +	 * Restore original debugfs value on exit, even for
> +	 * legacy case, because writing 0xf will set the debug flag
> +	 * regardless.
> +	 */
> +	if (psr_restore_debugfs_fd == -1) {
> +		psr_restore_debugfs_fd = dup(fd);
> +		igt_assert(psr_restore_debugfs_fd >= 0);
> +		igt_install_exit_handler(restore_psr_debugfs);
> +	}
> +
> +	if (ret == -EINVAL) {
> +		ret = psr_modparam_set(enable);
> +	} else {
> +		ret = psr_write(fd, enable ? "0x13" : "1");
"0x3" to avoid enabling interrupts.

And am not quite sure how this return translates to need_modeset in the
next patch. Why is a modeset needed if psr_write() successfully wrote
all the bytes?

> +		igt_assert(ret > 0);
> +	}
> +
> +	return ret;
> +}
> +
> +bool psr_enable(int fd)
> +{
> +	return psr_set(fd, true);
> +}
> +
> +bool psr_disable(int fd)
> +{
> +	return psr_set(fd, false);
> +}
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> index 980f85e04b3e..0ef22c3d1258 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -30,5 +30,7 @@
>  
>  bool psr_wait_entry(int fd);
>  bool psr_active(int fd, bool check_active);
> +bool psr_enable(int fd);
> +bool psr_disable(int fd);
>  
>  #endif
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index 1dfd7c1cee8d..7ea2f697184d 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -941,8 +941,6 @@ static bool
> drrs_wait_until_rr_switch_to_low(void)
>  
>  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
>  #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
>  #define drrs_enable()	drrs_set(1)
>  #define drrs_disable()	drrs_set(0)
>  
> @@ -1137,7 +1135,7 @@ static void disable_features(const struct
> test_mode *t)
>  		return;
>  
>  	fbc_disable();
> -	psr_disable();
> +	psr_disable(drm.debugfs);
>  	drrs_disable();
>  }
>  
> @@ -1719,7 +1717,7 @@ static void enable_features_for_test(const
> struct test_mode *t)
>  	if (t->feature & FEATURE_FBC)
>  		fbc_enable();
>  	if (t->feature & FEATURE_PSR)
> -		psr_enable();
> +		psr_enable(drm.debugfs);
>  	if (t->feature & FEATURE_DRRS)
>  		drrs_enable();
>  }
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
  2018-08-16  0:26 ` [igt-dev] [PATCH i-g-t 1/2] " Dhinakaran Pandiyan
@ 2018-08-16 11:41   ` Maarten Lankhorst
  2018-08-16 20:31     ` Dhinakaran Pandiyan
  0 siblings, 1 reply; 9+ messages in thread
From: Maarten Lankhorst @ 2018-08-16 11:41 UTC (permalink / raw
  To: dhinakaran.pandiyan, igt-dev

Op 16-08-18 om 02:26 schreef Dhinakaran Pandiyan:
> On Tue, 2018-08-14 at 14:22 +0200, Maarten Lankhorst wrote:
>> It's harmful to write to enable_psr at runtime, and the patch that
>> allows
>> us to change i915_edp_psr_debug with the panel running will require
>> us
>> to abandon the module parameter. Hence the userspace change needs to
>> be
>> put in IGT first before we can change it at kernel time.
>>
>> Toggling it to debugfs will mean we can skip a modeset when changing
>> our
>> feature set.
>>
>> Changes since v1:
>> - Rebase with the previous patches dropped.
>> Changes since v2:
>> - Rebase on top of new api in i915_edp_psr_debug.
>> Changes since v3:
>> - Enable IRQ debugging for extra logging.
>> - Force PSR1 mode. (dhnkrn)
>> - Move PSR enable/disable functions to lib/igt_psr. (dhnkrn)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  lib/igt_psr.c                    | 90
>> +++++++++++++++++++++++++++++++-
>>  lib/igt_psr.h                    |  2 +
>>  tests/kms_frontbuffer_tracking.c |  6 +--
>>  3 files changed, 93 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
>> index c979b0b59936..393702037861 100644
>> --- a/lib/igt_psr.c
>> +++ b/lib/igt_psr.c
>> @@ -21,7 +21,9 @@
>>   * IN THE SOFTWARE.
>>   */
>>  
>> -#include  "igt_psr.h"
>> +#include "igt_psr.h"
>> +#include "igt_sysfs.h"
>> +#include <errno.h>
>>  
>>  bool psr_active(int fd, bool check_active)
>>  {
>> @@ -38,3 +40,89 @@ bool psr_wait_entry(int fd)
>>  {
>>  	return igt_wait(psr_active(fd, true), 500, 1);
>>  }
>> +
>> +static ssize_t psr_write(int fd, const char *buf)
>> +{
>> +	return igt_sysfs_write(fd, "i915_edp_psr_debug", buf,
>> strlen(buf));
>> +}
>> +
>> +static int has_psr_debugfs(int fd)
>> +{
>> +	int ret;
>> +
>> +	/*
>> +	 * Check if new PSR debugfs api is usable by writing an
>> invalid value.
>> +	 * Legacy mode will return OK here, debugfs api will return
>> -EINVAL.
>> +	 * -ENODEV is returned.
>> +	 */
>> +	ret = psr_write(fd, "0xf");
>> +	if (ret == -EINVAL)
>> +		return 0;
>> +	else if (ret >= 0)
>> +		return -EINVAL;
> We should disable debug here, PSR interrupts aren't used in IGTs. Looks
>  like you sent this patch before I responded to your question about
> interrupts.
>
>
>> +	else
>> +		return ret;
>> +}
>> +
>> +static bool psr_modparam_set(int val)
>> +{
>> +	static int oldval = -1;
>> +
>> +	igt_set_module_param_int("enable_psr", val);
>> +
>> +	if (val == oldval)
>> +		return false;
>> +
>> +	oldval = val;
>> +	return true;
>> +}
>> +
>> +static int psr_restore_debugfs_fd = -1;
>> +
>> +static void restore_psr_debugfs(int sig)
>> +{
>> +	psr_write(psr_restore_debugfs_fd, "0");
>> +}
>> +
>> +static bool psr_set(int fd, bool enable)
>> +{
>> +	int ret;
>> +
>> +	igt_skip_on_f(fd < 0, "Could not open debugfs dir\n");
>> +
>> +	ret = has_psr_debugfs(fd);
>> +	if (ret == -ENODEV) {
>> +		igt_skip_on_f(enable, "PSR not available\n");
>> +		return false;
>> +	}
>> +
>> +	/*
>> +	 * Restore original debugfs value on exit, even for
>> +	 * legacy case, because writing 0xf will set the debug flag
>> +	 * regardless.
>> +	 */
>> +	if (psr_restore_debugfs_fd == -1) {
>> +		psr_restore_debugfs_fd = dup(fd);
>> +		igt_assert(psr_restore_debugfs_fd >= 0);
>> +		igt_install_exit_handler(restore_psr_debugfs);
>> +	}
>> +
>> +	if (ret == -EINVAL) {
>> +		ret = psr_modparam_set(enable);
>> +	} else {
>> +		ret = psr_write(fd, enable ? "0x13" : "1");
> "0x3" to avoid enabling interrupts.
>
> And am not quite sure how this return translates to need_modeset in the
> next patch. Why is a modeset needed if psr_write() successfully wrote
> all the bytes?
Oops, should reset to false in that case.

Any other comments on the series or is it good with those 2 fixes?

~Maarten
>> +		igt_assert(ret > 0);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +bool psr_enable(int fd)
>> +{
>> +	return psr_set(fd, true);
>> +}
>> +
>> +bool psr_disable(int fd)
>> +{
>> +	return psr_set(fd, false);
>> +}
>> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
>> index 980f85e04b3e..0ef22c3d1258 100644
>> --- a/lib/igt_psr.h
>> +++ b/lib/igt_psr.h
>> @@ -30,5 +30,7 @@
>>  
>>  bool psr_wait_entry(int fd);
>>  bool psr_active(int fd, bool check_active);
>> +bool psr_enable(int fd);
>> +bool psr_disable(int fd);
>>  
>>  #endif
>> diff --git a/tests/kms_frontbuffer_tracking.c
>> b/tests/kms_frontbuffer_tracking.c
>> index 1dfd7c1cee8d..7ea2f697184d 100644
>> --- a/tests/kms_frontbuffer_tracking.c
>> +++ b/tests/kms_frontbuffer_tracking.c
>> @@ -941,8 +941,6 @@ static bool
>> drrs_wait_until_rr_switch_to_low(void)
>>  
>>  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
>>  #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
>> -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
>> -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
>>  #define drrs_enable()	drrs_set(1)
>>  #define drrs_disable()	drrs_set(0)
>>  
>> @@ -1137,7 +1135,7 @@ static void disable_features(const struct
>> test_mode *t)
>>  		return;
>>  
>>  	fbc_disable();
>> -	psr_disable();
>> +	psr_disable(drm.debugfs);
>>  	drrs_disable();
>>  }
>>  
>> @@ -1719,7 +1717,7 @@ static void enable_features_for_test(const
>> struct test_mode *t)
>>  	if (t->feature & FEATURE_FBC)
>>  		fbc_enable();
>>  	if (t->feature & FEATURE_PSR)
>> -		psr_enable();
>> +		psr_enable(drm.debugfs);
>>  	if (t->feature & FEATURE_DRRS)
>>  		drrs_enable();
>>  }


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
  2018-08-16 11:41   ` Maarten Lankhorst
@ 2018-08-16 20:31     ` Dhinakaran Pandiyan
  2018-08-17  3:40       ` Dhinakaran Pandiyan
  0 siblings, 1 reply; 9+ messages in thread
From: Dhinakaran Pandiyan @ 2018-08-16 20:31 UTC (permalink / raw
  To: Maarten Lankhorst, igt-dev

On Thu, 2018-08-16 at 13:41 +0200, Maarten Lankhorst wrote:
> Op 16-08-18 om 02:26 schreef Dhinakaran Pandiyan:
> > On Tue, 2018-08-14 at 14:22 +0200, Maarten Lankhorst wrote:
> > > It's harmful to write to enable_psr at runtime, and the patch
> > > that
> > > allows
> > > us to change i915_edp_psr_debug with the panel running will
> > > require
> > > us
> > > to abandon the module parameter. Hence the userspace change needs
> > > to
> > > be
> > > put in IGT first before we can change it at kernel time.
> > > 
> > > Toggling it to debugfs will mean we can skip a modeset when
> > > changing
> > > our
> > > feature set.
> > > 
> > > Changes since v1:
> > > - Rebase with the previous patches dropped.
> > > Changes since v2:
> > > - Rebase on top of new api in i915_edp_psr_debug.
> > > Changes since v3:
> > > - Enable IRQ debugging for extra logging.
> > > - Force PSR1 mode. (dhnkrn)
> > > - Move PSR enable/disable functions to lib/igt_psr. (dhnkrn)
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > om>
> > > ---
> > >  lib/igt_psr.c                    | 90
> > > +++++++++++++++++++++++++++++++-
> > >  lib/igt_psr.h                    |  2 +
> > >  tests/kms_frontbuffer_tracking.c |  6 +--
> > >  3 files changed, 93 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index c979b0b59936..393702037861 100644
> > > --- a/lib/igt_psr.c
> > > +++ b/lib/igt_psr.c
> > > @@ -21,7 +21,9 @@
> > >   * IN THE SOFTWARE.
> > >   */
> > >  
> > > -#include  "igt_psr.h"
> > > +#include "igt_psr.h"
> > > +#include "igt_sysfs.h"
> > > +#include <errno.h>
> > >  
> > >  bool psr_active(int fd, bool check_active)
> > >  {
> > > @@ -38,3 +40,89 @@ bool psr_wait_entry(int fd)
> > >  {
> > >  	return igt_wait(psr_active(fd, true), 500, 1);
> > >  }
> > > +
> > > +static ssize_t psr_write(int fd, const char *buf)
> > > +{
> > > +	return igt_sysfs_write(fd, "i915_edp_psr_debug", buf,
> > > strlen(buf));
> > > +}
> > > +
> > > +static int has_psr_debugfs(int fd)
> > > +{
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * Check if new PSR debugfs api is usable by writing an
> > > invalid value.
> > > +	 * Legacy mode will return OK here, debugfs api will
> > > return
> > > -EINVAL.
> > > +	 * -ENODEV is returned.
> > > +	 */
> > > +	ret = psr_write(fd, "0xf");
> > > +	if (ret == -EINVAL)
> > > +		return 0;
> > > +	else if (ret >= 0)
> > > +		return -EINVAL;
> > 
> > We should disable debug here, PSR interrupts aren't used in IGTs.
> > Looks
> >  like you sent this patch before I responded to your question about
> > interrupts.
> > 
> > 
> > > +	else
> > > +		return ret;
> > > +}
> > > +
> > > +static bool psr_modparam_set(int val)
> > > +{
> > > +	static int oldval = -1;
> > > +
> > > +	igt_set_module_param_int("enable_psr", val);
> > > +
> > > +	if (val == oldval)
> > > +		return false;
> > > +
> > > +	oldval = val;
> > > +	return true;
> > > +}
> > > +
> > > +static int psr_restore_debugfs_fd = -1;
> > > +
> > > +static void restore_psr_debugfs(int sig)
> > > +{
> > > +	psr_write(psr_restore_debugfs_fd, "0");
> > > +}
> > > +
> > > +static bool psr_set(int fd, bool enable)
> > > +{
> > > +	int ret;
> > > +
> > > +	igt_skip_on_f(fd < 0, "Could not open debugfs dir\n");
> > > +
> > > +	ret = has_psr_debugfs(fd);
> > > +	if (ret == -ENODEV) {
> > > +		igt_skip_on_f(enable, "PSR not available\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Restore original debugfs value on exit, even for
> > > +	 * legacy case, because writing 0xf will set the debug
> > > flag
> > > +	 * regardless.
> > > +	 */
> > > +	if (psr_restore_debugfs_fd == -1) {
> > > +		psr_restore_debugfs_fd = dup(fd);
> > > +		igt_assert(psr_restore_debugfs_fd >= 0);
> > > +		igt_install_exit_handler(restore_psr_debugfs);
> > > +	}
> > > +
> > > +	if (ret == -EINVAL) {
> > > +		ret = psr_modparam_set(enable);
> > > +	} else {
> > > +		ret = psr_write(fd, enable ? "0x13" : "1");
> > 
> > "0x3" to avoid enabling interrupts.
> > 
> > And am not quite sure how this return translates to need_modeset in
> > the
> > next patch. Why is a modeset needed if psr_write() successfully
> > wrote
> > all the bytes?
> 
> Oops, should reset to false in that case.
> 
> Any other comments on the series or is it good with those 2 fixes?

I need a bit more time to review patch 2/2, not very familiar with the
flow in frontbuffer_tracking.c


> 
> ~Maarten
> > > +		igt_assert(ret > 0);
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +bool psr_enable(int fd)
> > > +{
> > > +	return psr_set(fd, true);
> > > +}
> > > +
> > > +bool psr_disable(int fd)
> > > +{
> > > +	return psr_set(fd, false);
> > > +}
> > > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > > index 980f85e04b3e..0ef22c3d1258 100644
> > > --- a/lib/igt_psr.h
> > > +++ b/lib/igt_psr.h
> > > @@ -30,5 +30,7 @@
> > >  
> > >  bool psr_wait_entry(int fd);
> > >  bool psr_active(int fd, bool check_active);
> > > +bool psr_enable(int fd);
> > > +bool psr_disable(int fd);
> > >  
> > >  #endif
> > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > b/tests/kms_frontbuffer_tracking.c
> > > index 1dfd7c1cee8d..7ea2f697184d 100644
> > > --- a/tests/kms_frontbuffer_tracking.c
> > > +++ b/tests/kms_frontbuffer_tracking.c
> > > @@ -941,8 +941,6 @@ static bool
> > > drrs_wait_until_rr_switch_to_low(void)
> > >  
> > >  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
> > >  #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> > > -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> > > -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
> > >  #define drrs_enable()	drrs_set(1)
> > >  #define drrs_disable()	drrs_set(0)
> > >  
> > > @@ -1137,7 +1135,7 @@ static void disable_features(const struct
> > > test_mode *t)
> > >  		return;
> > >  
> > >  	fbc_disable();
> > > -	psr_disable();
> > > +	psr_disable(drm.debugfs);
> > >  	drrs_disable();
> > >  }
> > >  
> > > @@ -1719,7 +1717,7 @@ static void enable_features_for_test(const
> > > struct test_mode *t)
> > >  	if (t->feature & FEATURE_FBC)
> > >  		fbc_enable();
> > >  	if (t->feature & FEATURE_PSR)
> > > -		psr_enable();
> > > +		psr_enable(drm.debugfs);
> > >  	if (t->feature & FEATURE_DRRS)
> > >  		drrs_enable();
> > >  }
> 
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
-- 
-DK
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4.
  2018-08-16 20:31     ` Dhinakaran Pandiyan
@ 2018-08-17  3:40       ` Dhinakaran Pandiyan
  0 siblings, 0 replies; 9+ messages in thread
From: Dhinakaran Pandiyan @ 2018-08-17  3:40 UTC (permalink / raw
  To: Maarten Lankhorst, igt-dev

On Thu, 2018-08-16 at 13:31 -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-08-16 at 13:41 +0200, Maarten Lankhorst wrote:
> > Op 16-08-18 om 02:26 schreef Dhinakaran Pandiyan:
> > > On Tue, 2018-08-14 at 14:22 +0200, Maarten Lankhorst wrote:
> > > > It's harmful to write to enable_psr at runtime, and the patch
> > > > that
> > > > allows
> > > > us to change i915_edp_psr_debug with the panel running will
> > > > require
> > > > us
> > > > to abandon the module parameter. Hence the userspace change
> > > > needs
> > > > to
> > > > be
> > > > put in IGT first before we can change it at kernel time.
> > > > 
> > > > Toggling it to debugfs will mean we can skip a modeset when
> > > > changing
> > > > our
> > > > feature set.
> > > > 
> > > > Changes since v1:
> > > > - Rebase with the previous patches dropped.
> > > > Changes since v2:
> > > > - Rebase on top of new api in i915_edp_psr_debug.
> > > > Changes since v3:
> > > > - Enable IRQ debugging for extra logging.
> > > > - Force PSR1 mode. (dhnkrn)
> > > > - Move PSR enable/disable functions to lib/igt_psr. (dhnkrn)
> > > > 
> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel
> > > > .c
> > > > om>
> > > > ---
> > > >  lib/igt_psr.c                    | 90
> > > > +++++++++++++++++++++++++++++++-
> > > >  lib/igt_psr.h                    |  2 +
> > > >  tests/kms_frontbuffer_tracking.c |  6 +--
> > > >  3 files changed, 93 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > > index c979b0b59936..393702037861 100644
> > > > --- a/lib/igt_psr.c
> > > > +++ b/lib/igt_psr.c
> > > > @@ -21,7 +21,9 @@
> > > >   * IN THE SOFTWARE.
> > > >   */
> > > >  
> > > > -#include  "igt_psr.h"
> > > > +#include "igt_psr.h"
> > > > +#include "igt_sysfs.h"
> > > > +#include <errno.h>
> > > >  
> > > >  bool psr_active(int fd, bool check_active)
> > > >  {
> > > > @@ -38,3 +40,89 @@ bool psr_wait_entry(int fd)
> > > >  {
> > > >  	return igt_wait(psr_active(fd, true), 500, 1);
> > > >  }
> > > > +
> > > > +static ssize_t psr_write(int fd, const char *buf)
> > > > +{
> > > > +	return igt_sysfs_write(fd, "i915_edp_psr_debug", buf,
> > > > strlen(buf));
> > > > +}
> > > > +
> > > > +static int has_psr_debugfs(int fd)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	/*
> > > > +	 * Check if new PSR debugfs api is usable by writing
> > > > an
> > > > invalid value.
> > > > +	 * Legacy mode will return OK here, debugfs api will
> > > > return
> > > > -EINVAL.
> > > > +	 * -ENODEV is returned.
> > > > +	 */
> > > > +	ret = psr_write(fd, "0xf");
> > > > +	if (ret == -EINVAL)
> > > > +		return 0;
> > > > +	else if (ret >= 0)
> > > > +		return -EINVAL;
> > > 
> > > We should disable debug here, PSR interrupts aren't used in IGTs.
> > > Looks
> > >  like you sent this patch before I responded to your question
> > > about
> > > interrupts.
> > > 
> > > 
> > > > +	else
> > > > +		return ret;
> > > > +}
> > > > +
> > > > +static bool psr_modparam_set(int val)
> > > > +{
> > > > +	static int oldval = -1;
> > > > +
> > > > +	igt_set_module_param_int("enable_psr", val);
> > > > +
> > > > +	if (val == oldval)
> > > > +		return false;
> > > > +
> > > > +	oldval = val;
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static int psr_restore_debugfs_fd = -1;
> > > > +
> > > > +static void restore_psr_debugfs(int sig)
> > > > +{
> > > > +	psr_write(psr_restore_debugfs_fd, "0");
> > > > +}
> > > > +
> > > > +static bool psr_set(int fd, bool enable)
The meaning of this bool return is non-obvious within this library. Add
some documentation perhaps?
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	igt_skip_on_f(fd < 0, "Could not open debugfs dir\n");
> > > > +
> > > > +	ret = has_psr_debugfs(fd);
> > > > +	if (ret == -ENODEV) {
> > > > +		igt_skip_on_f(enable, "PSR not available\n");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Restore original debugfs value on exit, even for
> > > > +	 * legacy case, because writing 0xf will set the debug
> > > > flag
> > > > +	 * regardless.
I supposed this will be changed in the next version as there is no use
for interrupts in IGTs

> > > > +	 */
> > > > +	if (psr_restore_debugfs_fd == -1) {
> > > > +		psr_restore_debugfs_fd = dup(fd);
> > > > +		igt_assert(psr_restore_debugfs_fd >= 0);
> > > > +		igt_install_exit_handler(restore_psr_debugfs);
> > > > +	}
> > > > +
> > > > +	if (ret == -EINVAL) {
> > > > +		ret = psr_modparam_set(enable);
> > > > +	} else {
> > > > +		ret = psr_write(fd, enable ? "0x13" : "1");
OCD nitpick, feel free to ignore, it'd be nice to consistently use hex
everywhere.

With these minor changes dealt with, this patch is good.

-DK
> > > 
> > > "0x3" to avoid enabling interrupts.
> > > 
> > > And am not quite sure how this return translates to need_modeset
> > > in
> > > the
> > > next patch. Why is a modeset needed if psr_write() successfully
> > > wrote
> > > all the bytes?
> > 
> > Oops, should reset to false in that case.
> > 
> > Any other comments on the series or is it good with those 2 fixes?
> 
> I need a bit more time to review patch 2/2, not very familiar with
> the
> flow in frontbuffer_tracking.c
> 
> 
> > 
> > ~Maarten
> > > > +		igt_assert(ret > 0);
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +bool psr_enable(int fd)
> > > > +{
> > > > +	return psr_set(fd, true);
> > > > +}
> > > > +
> > > > +bool psr_disable(int fd)
> > > > +{
> > > > +	return psr_set(fd, false);
> > > > +}
> > > > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > > > index 980f85e04b3e..0ef22c3d1258 100644
> > > > --- a/lib/igt_psr.h
> > > > +++ b/lib/igt_psr.h
> > > > @@ -30,5 +30,7 @@
> > > >  
> > > >  bool psr_wait_entry(int fd);
> > > >  bool psr_active(int fd, bool check_active);
> > > > +bool psr_enable(int fd);
> > > > +bool psr_disable(int fd);
> > > >  
> > > >  #endif
> > > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > > b/tests/kms_frontbuffer_tracking.c
> > > > index 1dfd7c1cee8d..7ea2f697184d 100644
> > > > --- a/tests/kms_frontbuffer_tracking.c
> > > > +++ b/tests/kms_frontbuffer_tracking.c
> > > > @@ -941,8 +941,6 @@ static bool
> > > > drrs_wait_until_rr_switch_to_low(void)
> > > >  
> > > >  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
> > > >  #define fbc_disable() igt_set_module_param_int("enable_fbc",
> > > > 0)
> > > > -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> > > > -#define psr_disable() igt_set_module_param_int("enable_psr",
> > > > 0)
> > > >  #define drrs_enable()	drrs_set(1)
> > > >  #define drrs_disable()	drrs_set(0)
> > > >  
> > > > @@ -1137,7 +1135,7 @@ static void disable_features(const struct
> > > > test_mode *t)
> > > >  		return;
> > > >  
> > > >  	fbc_disable();
> > > > -	psr_disable();
> > > > +	psr_disable(drm.debugfs);
> > > >  	drrs_disable();
> > > >  }
> > > >  
> > > > @@ -1719,7 +1717,7 @@ static void
> > > > enable_features_for_test(const
> > > > struct test_mode *t)
> > > >  	if (t->feature & FEATURE_FBC)
> > > >  		fbc_enable();
> > > >  	if (t->feature & FEATURE_PSR)
> > > > -		psr_enable();
> > > > +		psr_enable(drm.debugfs);
> > > >  	if (t->feature & FEATURE_DRRS)
> > > >  		drrs_enable();
> > > >  }
> > 
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
-- 
-DK
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v4.
  2018-08-14 12:22 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v4 Maarten Lankhorst
@ 2018-08-17  6:53   ` Dhinakaran Pandiyan
  0 siblings, 0 replies; 9+ messages in thread
From: Dhinakaran Pandiyan @ 2018-08-17  6:53 UTC (permalink / raw
  To: Maarten Lankhorst, igt-dev

On Tue, 2018-08-14 at 14:22 +0200, Maarten Lankhorst wrote:
> CRC capturing enables the display, then disables it again. With
> igt_display we can use igt_display_reset to restore the original
> state,
> without committing it to the hw.
> 
> All subtests first set their own state anyway, so we can save up on
> the number of commits.
> 
> Changes since v1:
> - Try to avoid modesets for PSR if the kernel supports it, but
> otherwise force
>   a modeset for the changes to take effect.
> Changes since v2:
> - Rebase on top of previous PSR changes.
> Changes since v3:
> - Rebase on move to lib/igt_psr.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 57 ++++++++++++++++++++++++----
> ----
>  1 file changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index 7ea2f697184d..2cec30da2750 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -1129,14 +1129,14 @@ static void unset_all_crtcs(void)
>  	igt_display_commit(&drm.display);
>  }
>  
> -static void disable_features(const struct test_mode *t)
> +static bool disable_features(const struct test_mode *t)
>  {
>  	if (t->feature == FEATURE_DEFAULT)
> -		return;
> +		return false;
>  
>  	fbc_disable();
> -	psr_disable(drm.debugfs);
>  	drrs_disable();
> +	return psr_disable(drm.debugfs);
>  }
>  
>  static void *busy_thread_func(void *data)
> @@ -1220,7 +1220,7 @@ static void init_blue_crc(enum pixel_format
> format)
>  
>  	print_crc("Blue CRC:  ", &blue_crcs[format].crc);
>  
> -	unset_all_crtcs();
> +	igt_display_reset(&drm.display);
>  
>  	igt_remove_fb(drm.fd, &blue);
>  
> @@ -1272,7 +1272,7 @@ static void init_crcs(enum pixel_format format,
>  		print_crc("", &pattern->crcs[format][r]);
>  	}
>  
> -	unset_all_crtcs();
> +	igt_display_reset(&drm.display);
>  
>  	for (r = 0; r < pattern->n_rects; r++)
>  		igt_remove_fb(drm.fd, &tmp_fbs[r]);
> @@ -1695,6 +1695,22 @@ static void enable_scnd_screen_and_wait(const
> struct test_mode *t)
>  	do_assertions(ASSERT_NO_ACTION_CHANGE);
>  }
>  
> +static void enable_both_screens_and_wait(const struct test_mode *t)
> +{
> +	fill_fb_region(&prim_mode_params.primary, COLOR_PRIM_BG);
> +	fill_fb_region(&scnd_mode_params.primary, COLOR_SCND_BG);
> +
> +	__set_mode_for_params(&prim_mode_params);
> +	__set_mode_for_params(&scnd_mode_params);
> +
> +	igt_display_commit2(&drm.display, drm.display.is_atomic ?
> COMMIT_ATOMIC : COMMIT_LEGACY);
> +
> +	wanted_crc = &blue_crcs[t->format].crc;
> +	fbc_update_last_action();
> +
> +	do_assertions(ASSERT_NO_ACTION_CHANGE);
> +}
> +
>  static void set_region_for_test(const struct test_mode *t,
>  				struct fb_region *reg)
>  {
> @@ -1709,17 +1725,21 @@ static void set_region_for_test(const struct
> test_mode *t,
>  	do_assertions(ASSERT_NO_ACTION_CHANGE);
>  }
>  
> -static void enable_features_for_test(const struct test_mode *t)
> +static bool enable_features_for_test(const struct test_mode *t)
>  {
> +	bool ret = false;
> +
>  	if (t->feature == FEATURE_DEFAULT)
> -		return;
> +		return false;
>  
>  	if (t->feature & FEATURE_FBC)
>  		fbc_enable();
>  	if (t->feature & FEATURE_PSR)
> -		psr_enable(drm.debugfs);
> +		ret = psr_enable(drm.debugfs);
>  	if (t->feature & FEATURE_DRRS)
>  		drrs_enable();
> +
> +	return ret;
>  }
>  
>  static void check_test_requirements(const struct test_mode *t)
> @@ -1801,28 +1821,40 @@ static void set_crtc_fbs(const struct
> test_mode *t)
>  static void prepare_subtest_data(const struct test_mode *t,
>  				 struct draw_pattern_info *pattern)
>  {
> +	bool need_modeset;
> +
>  	check_test_requirements(t);
>  
>  	stop_busy_thread();
>  
> -	disable_features(t);
> +	need_modeset = disable_features(t);
>  	set_crtc_fbs(t);
>  
>  	if (t->screen == SCREEN_OFFSCREEN)
>  		fill_fb_region(&offscreen_fb, COLOR_OFFSCREEN_BG);
>  
> -	unset_all_crtcs();
> +	igt_display_reset(&drm.display);
> +	if (need_modeset)
> +		igt_display_commit(&drm.display);
>  
>  	init_blue_crc(t->format);
>  	if (pattern)
>  		init_crcs(t->format, pattern);
>  
> -	enable_features_for_test(t);
> +	igt_display_reset(&drm.display);

Both init_blue_crc() and init_crc() call igt_display_reset(). Why is a
another reset required here?

Feel like there's too much going on in this one patch. Can you please
split the modeset eliminations you are implementing? I think changes
corresponding to PSR enabling, CRC initialization and the refactoring
of enable_both_screens_and_wait() all warrant to be individual patches.


-DK

> +
> +	need_modeset = enable_features_for_test(t);
> +	if (need_modeset)
> +		igt_display_commit(&drm.display);
>  }
>  
>  static void prepare_subtest_screens(const struct test_mode *t)
>  {
> -	enable_prim_screen_and_wait(t);
> +	if (t->pipes == PIPE_DUAL)
> +		enable_both_screens_and_wait(t);
> +	else
> +		enable_prim_screen_and_wait(t);
> +
>  	if (t->screen == SCREEN_PRIM) {
>  		if (t->plane == PLANE_CUR)
>  			set_region_for_test(t,
> &prim_mode_params.cursor);
> @@ -1833,7 +1865,6 @@ static void prepare_subtest_screens(const
> struct test_mode *t)
>  	if (t->pipes == PIPE_SINGLE)
>  		return;
>  
> -	enable_scnd_screen_and_wait(t);
>  	if (t->screen == SCREEN_SCND) {
>  		if (t->plane == PLANE_CUR)
>  			set_region_for_test(t,
> &scnd_mode_params.cursor);
-- 
-DK
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-08-17  6:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-14 12:22 [igt-dev] [PATCH i-g-t 1/2] lib/psr: Add support for toggling edp psr through debugfs, v4 Maarten Lankhorst
2018-08-14 12:22 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_frontbuffer_tracking: Remove redundant modesets during subtest start, v4 Maarten Lankhorst
2018-08-17  6:53   ` Dhinakaran Pandiyan
2018-08-14 12:48 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] lib/psr: Add support for toggling edp psr through debugfs, v4 Patchwork
2018-08-14 15:29 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2018-08-16  0:26 ` [igt-dev] [PATCH i-g-t 1/2] " Dhinakaran Pandiyan
2018-08-16 11:41   ` Maarten Lankhorst
2018-08-16 20:31     ` Dhinakaran Pandiyan
2018-08-17  3:40       ` Dhinakaran Pandiyan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.