[igt-dev] [PATCH i-g-t 01/10] lib/psr: Add a macro with the maximum lenght of i915_edp_psr_status and use it

[igt-dev] [PATCH i-g-t 01/10] lib/psr: Add a macro with the maximum lenght of i915_edp_psr_status and use it

[José Roberto de Souza]

So every function reading i915_edp_psr_status can allocate a buffer
long enough.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c                    | 2 +-
 lib/igt_psr.h                    | 2 ++
 tests/kms_fbcon_fbt.c            | 6 +++---
 tests/kms_frontbuffer_tracking.c | 2 +-
 tests/kms_psr.c                  | 2 +-
 5 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index 0ddfb64f..c105bb6e 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -28,7 +28,7 @@
 static bool psr_active(int debugfs_fd, bool check_active)
 {
 	bool active;
-	char buf[512];
+	char buf[PSR_STATUS_MAX_LEN];
 
 	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
 				sizeof(buf));
diff --git a/lib/igt_psr.h b/lib/igt_psr.h
index b9693822..96f7bedf 100644
--- a/lib/igt_psr.h
+++ b/lib/igt_psr.h
@@ -28,6 +28,8 @@
 #include "igt_core.h"
 #include "igt_aux.h"
 
+#define PSR_STATUS_MAX_LEN 512
+
 bool psr_wait_entry(int debugfs_fd);
 bool psr_wait_exit(int debugfs_fd);
 bool psr_enable(int debugfs_fd);
diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
index 24d3ad90..0ad53266 100644
--- a/tests/kms_fbcon_fbt.c
+++ b/tests/kms_fbcon_fbt.c
@@ -170,7 +170,7 @@ static void set_mode_for_one_screen(struct drm_info *drm, struct igt_fb *fb,
 
 static bool psr_supported_on_chipset(int debugfs_fd)
 {
-	char buf[256];
+	char buf[PSR_STATUS_MAX_LEN];
 	int ret;
 
 	ret = igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status",
@@ -188,7 +188,7 @@ static bool connector_can_psr(drmModeConnectorPtr connector)
 
 static void psr_print_status(int debugfs_fd)
 {
-	static char buf[256];
+	static char buf[PSR_STATUS_MAX_LEN];
 
 	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
 				sizeof(buf));
@@ -197,7 +197,7 @@ static void psr_print_status(int debugfs_fd)
 
 static bool psr_is_enabled(int debugfs_fd)
 {
-	char buf[256];
+	char buf[PSR_STATUS_MAX_LEN];
 
 	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
 				sizeof(buf));
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 5ab28319..c366fecf 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1419,7 +1419,7 @@ static void teardown_fbc(void)
 
 static bool psr_sink_has_support(void)
 {
-	char buf[256];
+	char buf[PSR_STATUS_MAX_LEN];
 
 	debugfs_read("i915_edp_psr_status", buf);
 	if (*buf == '\0') /* !HAS_PSR -> -ENODEV*/
diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index d00e552f..20b69892 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -191,7 +191,7 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color)
 
 static bool sink_support(data_t *data)
 {
-	char buf[512];
+	char buf[PSR_STATUS_MAX_LEN];
 
 	igt_debugfs_simple_read(data->debugfs_fd, "i915_edp_psr_status",
 			 buf, sizeof(buf));
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t 02/10] tests/psr: Share the code check if sink supports PSR

[José Roberto de Souza]

The same code checking if sink supports PSR was spread into 3 tests,
better move it to lib and reuse.

v2: splitted previous patch into this one and the next one(Dhinakaran)

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c                    | 10 ++++++++++
 lib/igt_psr.h                    |  1 +
 tests/kms_fbcon_fbt.c            | 15 +--------------
 tests/kms_frontbuffer_tracking.c | 13 +------------
 tests/kms_psr.c                  |  8 +-------
 5 files changed, 14 insertions(+), 33 deletions(-)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index c105bb6e..82012e6d 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -129,3 +129,13 @@ bool psr_disable(int debugfs_fd)
 {
 	return psr_set(debugfs_fd, false);
 }
+
+bool psr_sink_support(int debugfs_fd)
+{
+	char buf[PSR_STATUS_MAX_LEN];
+	int ret;
+
+	ret = igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
+				      sizeof(buf));
+	return ret > 0 && strstr(buf, "Sink_Support: yes\n");
+}
diff --git a/lib/igt_psr.h b/lib/igt_psr.h
index 96f7bedf..d3336c2d 100644
--- a/lib/igt_psr.h
+++ b/lib/igt_psr.h
@@ -34,5 +34,6 @@ bool psr_wait_entry(int debugfs_fd);
 bool psr_wait_exit(int debugfs_fd);
 bool psr_enable(int debugfs_fd);
 bool psr_disable(int debugfs_fd);
+bool psr_sink_support(int debugfs_fd);
 
 #endif
diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
index 0ad53266..2823b47a 100644
--- a/tests/kms_fbcon_fbt.c
+++ b/tests/kms_fbcon_fbt.c
@@ -168,19 +168,6 @@ static void set_mode_for_one_screen(struct drm_info *drm, struct igt_fb *fb,
 	igt_assert_eq(rc, 0);
 }
 
-static bool psr_supported_on_chipset(int debugfs_fd)
-{
-	char buf[PSR_STATUS_MAX_LEN];
-	int ret;
-
-	ret = igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status",
-				      buf, sizeof(buf));
-	if (ret < 0)
-		return false;
-
-	return strstr(buf, "Sink_Support: yes\n");
-}
-
 static bool connector_can_psr(drmModeConnectorPtr connector)
 {
 	return (connector->connector_type == DRM_MODE_CONNECTOR_eDP);
@@ -239,7 +226,7 @@ struct feature {
 	.connector_possible_fn = connector_can_fbc,
 	.enable = fbc_modparam_enable,
 }, psr = {
-	.supported_on_chipset = psr_supported_on_chipset,
+	.supported_on_chipset = psr_sink_support,
 	.wait_until_enabled = psr_wait_until_enabled,
 	.connector_possible_fn = connector_can_psr,
 	.enable = psr_debugfs_enable,
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index c366fecf..42f4c289 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1417,17 +1417,6 @@ static void teardown_fbc(void)
 {
 }
 
-static bool psr_sink_has_support(void)
-{
-	char buf[PSR_STATUS_MAX_LEN];
-
-	debugfs_read("i915_edp_psr_status", buf);
-	if (*buf == '\0') /* !HAS_PSR -> -ENODEV*/
-		return false;
-
-	return strstr(buf, "Sink_Support: yes\n");
-}
-
 static void setup_psr(void)
 {
 	if (prim_mode_params.output->config.connector->connector_type !=
@@ -1436,7 +1425,7 @@ static void setup_psr(void)
 		return;
 	}
 
-	if (!psr_sink_has_support()) {
+	if (!psr_sink_support(drm.debugfs)) {
 		igt_info("Can't test PSR: not supported by sink.\n");
 		return;
 	}
diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index 20b69892..855679b0 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -191,13 +191,7 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color)
 
 static bool sink_support(data_t *data)
 {
-	char buf[PSR_STATUS_MAX_LEN];
-
-	igt_debugfs_simple_read(data->debugfs_fd, "i915_edp_psr_status",
-			 buf, sizeof(buf));
-
-	return data->with_psr_disabled ||
-		strstr(buf, "Sink_Support: yes\n");
+	return data->with_psr_disabled || psr_sink_support(data->debugfs_fd);
 }
 
 static bool psr_wait_entry_if_enabled(data_t *data)
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t 03/10] lib/psr: Add support to new modified i915_edp_psr_status output

[José Roberto de Souza]

The kernel patch 'drm/i915: Refactor PSR status debugfs' changed the
output of i915_edp_psr_status, so adding support to the new output
here while keeping the support to the old one for a while.

In psr_active() we were overdoing, we just need to check the source
state to know if PSR is active and doing that we keep compability to
old and new i915_edp_psr_status output.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index 82012e6d..a59ff94e 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -32,8 +32,8 @@ static bool psr_active(int debugfs_fd, bool check_active)
 
 	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
 				sizeof(buf));
-	active = strstr(buf, "HW Enabled & Active bit: yes\n") &&
-		(strstr(buf, "SRDENT") || strstr(buf, "SLEEP"));
+
+	active = strstr(buf, "SRDENT") || strstr(buf, "SLEEP");
 	return check_active ? active : !active;
 }
 
@@ -137,5 +137,6 @@ bool psr_sink_support(int debugfs_fd)
 
 	ret = igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
 				      sizeof(buf));
-	return ret > 0 && strstr(buf, "Sink_Support: yes\n");
+	return ret > 0 && (strstr(buf, "Sink_Support: yes\n") ||
+			   strstr(buf, "Sink support: yes"));
 }
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t 04/10] lib/psr: Only care about DEEP_SLEEP state for PSR2

[José Roberto de Souza]

To check if PSR is active it search for SRDENT for PSR1 and it was
searching for SLEEP for PSR2 but it should really seach for
DEEP_SLEEP as in this state display block is actualy saving a
substancial amount of power.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index a59ff94e..83c5b986 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -33,7 +33,7 @@ static bool psr_active(int debugfs_fd, bool check_active)
 	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
 				sizeof(buf));
 
-	active = strstr(buf, "SRDENT") || strstr(buf, "SLEEP");
+	active = strstr(buf, "SRDENT") || strstr(buf, "DEEP_SLEEP");
 	return check_active ? active : !active;
 }
 
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t 05/10] lib/psr: Rename psr_wait_exit to psr_wait_update

[José Roberto de Souza]

This is a initial preparation for PSR2 test support, as in PSR2 a
update to screen could mean that PSR is still active and the screen
will be update by a selective update this renamed is necessary.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c                    | 2 +-
 lib/igt_psr.h                    | 2 +-
 tests/kms_frontbuffer_tracking.c | 3 ++-
 tests/kms_psr.c                  | 2 +-
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index 83c5b986..5edec6a2 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -42,7 +42,7 @@ bool psr_wait_entry(int debugfs_fd)
 	return igt_wait(psr_active(debugfs_fd, true), 500, 20);
 }
 
-bool psr_wait_exit(int debugfs_fd)
+bool psr_wait_update(int debugfs_fd)
 {
 	return igt_wait(psr_active(debugfs_fd, false), 40, 10);
 }
diff --git a/lib/igt_psr.h b/lib/igt_psr.h
index d3336c2d..62c680ee 100644
--- a/lib/igt_psr.h
+++ b/lib/igt_psr.h
@@ -31,7 +31,7 @@
 #define PSR_STATUS_MAX_LEN 512
 
 bool psr_wait_entry(int debugfs_fd);
-bool psr_wait_exit(int debugfs_fd);
+bool psr_wait_update(int debugfs_fd);
 bool psr_enable(int debugfs_fd);
 bool psr_disable(int debugfs_fd);
 bool psr_sink_support(int debugfs_fd);
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 42f4c289..cd7dda91 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1625,7 +1625,8 @@ static void do_status_assertions(int flags)
 		igt_assert_f(psr_wait_entry(drm.debugfs),
 			     "PSR still disabled\n");
 	else if (flags & ASSERT_PSR_DISABLED)
-		igt_assert_f(psr_wait_exit(drm.debugfs), "PSR still enabled\n");
+		igt_assert_f(psr_wait_update(drm.debugfs),
+			     "PSR still enabled\n");
 }
 
 static void __do_assertions(const struct test_mode *t, int flags,
diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index 855679b0..fb3bd7a4 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -289,7 +289,7 @@ static void run_test(data_t *data)
 		expected = "screen GREEN";
 		break;
 	}
-	igt_assert(psr_wait_exit(data->debugfs_fd));
+	igt_assert(psr_wait_update(data->debugfs_fd));
 	manual(expected);
 }
 
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t 06/10] lib/psr: Make psr_wait_entry and psr_wait_update aware of the PSR version tested

[José Roberto de Souza]

This way we can test both PSR version separated.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c                    | 26 ++++++++++++++++++--------
 lib/igt_psr.h                    |  9 +++++++--
 tests/kms_frontbuffer_tracking.c |  4 ++--
 tests/kms_psr.c                  |  4 ++--
 4 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index 5edec6a2..c6638c2c 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -25,26 +25,36 @@
 #include "igt_sysfs.h"
 #include <errno.h>
 
-static bool psr_active(int debugfs_fd, bool check_active)
+static bool psr_state_check(int debugfs_fd, const char *state)
 {
-	bool active;
 	char buf[PSR_STATUS_MAX_LEN];
 
 	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
 				sizeof(buf));
 
-	active = strstr(buf, "SRDENT") || strstr(buf, "DEEP_SLEEP");
-	return check_active ? active : !active;
+	return strstr(buf, state);
 }
 
-bool psr_wait_entry(int debugfs_fd)
+static inline const char *psr_active_state_get(enum psr_mode mode)
 {
-	return igt_wait(psr_active(debugfs_fd, true), 500, 20);
+	return mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
 }
 
-bool psr_wait_update(int debugfs_fd)
+/*
+ * For PSR1, we wait until PSR is active. We wait until DEEP_SLEEP for PSR2.
+ */
+bool psr_wait_entry(int debugfs_fd, enum psr_mode mode)
 {
-	return igt_wait(psr_active(debugfs_fd, false), 40, 10);
+	const char *state = psr_active_state_get(mode);
+
+	return igt_wait(psr_state_check(debugfs_fd, state), 500, 20);
+}
+
+bool psr_wait_update(int debugfs_fd, enum psr_mode mode)
+{
+	const char *state = psr_active_state_get(mode);
+
+	return igt_wait(!psr_state_check(debugfs_fd, state), 40, 10);
 }
 
 static ssize_t psr_write(int debugfs_fd, const char *buf)
diff --git a/lib/igt_psr.h b/lib/igt_psr.h
index 62c680ee..4fff77ec 100644
--- a/lib/igt_psr.h
+++ b/lib/igt_psr.h
@@ -30,8 +30,13 @@
 
 #define PSR_STATUS_MAX_LEN 512
 
-bool psr_wait_entry(int debugfs_fd);
-bool psr_wait_update(int debugfs_fd);
+enum psr_mode {
+	PSR_MODE_1,
+	PSR_MODE_2
+};
+
+bool psr_wait_entry(int debugfs_fd, enum psr_mode mode);
+bool psr_wait_update(int debugfs_fd, enum psr_mode mode);
 bool psr_enable(int debugfs_fd);
 bool psr_disable(int debugfs_fd);
 bool psr_sink_support(int debugfs_fd);
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index cd7dda91..ed9a039a 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1622,10 +1622,10 @@ static void do_status_assertions(int flags)
 	}
 
 	if (flags & ASSERT_PSR_ENABLED)
-		igt_assert_f(psr_wait_entry(drm.debugfs),
+		igt_assert_f(psr_wait_entry(drm.debugfs, PSR_MODE_1),
 			     "PSR still disabled\n");
 	else if (flags & ASSERT_PSR_DISABLED)
-		igt_assert_f(psr_wait_update(drm.debugfs),
+		igt_assert_f(psr_wait_update(drm.debugfs, PSR_MODE_1),
 			     "PSR still enabled\n");
 }
 
diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index fb3bd7a4..23d000bd 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -199,7 +199,7 @@ static bool psr_wait_entry_if_enabled(data_t *data)
 	if (data->with_psr_disabled)
 		return true;
 
-	return psr_wait_entry(data->debugfs_fd);
+	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
 }
 
 static inline void manual(const char *expected)
@@ -289,7 +289,7 @@ static void run_test(data_t *data)
 		expected = "screen GREEN";
 		break;
 	}
-	igt_assert(psr_wait_update(data->debugfs_fd));
+	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
 	manual(expected);
 }
 
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t 07/10] lib/psr: Drop support to old kernels without new PSR debugfs interface

[José Roberto de Souza]

The nexts patches will add PSR2 tests and for that we need the new
PSR debugfs interface that was released in kernel 4.20 so it will not
break for any updated system and we can drop support to the old
debugsfs and the support to kernels without even a debugfs interface.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c | 35 +++++++----------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index c6638c2c..5cc0fbc2 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -73,27 +73,7 @@ static int has_psr_debugfs(int debugfs_fd)
 	 * -ENODEV is returned when PSR is unavailable.
 	 */
 	ret = psr_write(debugfs_fd, "0xf");
-	if (ret == -EINVAL)
-		return 0;
-	else if (ret < 0)
-		return ret;
-
-	/* legacy debugfs api, we enabled irqs by writing, disable them. */
-	psr_write(debugfs_fd, "0");
-	return -EINVAL;
-}
-
-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;
+	return ret == -EINVAL ? 0 : ret;
 }
 
 static int psr_restore_debugfs_fd = -1;
@@ -109,16 +89,15 @@ static bool psr_set(int debugfs_fd, bool enable)
 
 	ret = has_psr_debugfs(debugfs_fd);
 	if (ret == -ENODEV) {
-		igt_skip_on_f(enable, "PSR not available\n");
+		igt_skip("PSR not available\n");
+		return false;
+	} else if (ret) {
+		igt_skip("PSR debugfs interface not available\n");
 		return false;
 	}
 
-	if (ret == -EINVAL) {
-		ret = psr_modparam_set(enable);
-	} else {
-		ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
-		igt_assert(ret > 0);
-	}
+	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
+	igt_assert(ret > 0);
 
 	/* Restore original value on exit */
 	if (psr_restore_debugfs_fd == -1) {
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t 08/10] lib/psr: Add PSR2 support to the remaning psr functions

[José Roberto de Souza]

Add the mode parameter to psr_enable() and psr_sink_support() so PSR1
and PSR2 can be tested separated.
For now all PSR tests will run only with PSR1 and the tests for PSR2
will come in the future.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 lib/igt_psr.c                    | 39 +++++++++++++++++++++++++-------
 lib/igt_psr.h                    |  4 ++--
 tests/kms_fbcon_fbt.c            |  9 ++++++--
 tests/kms_frontbuffer_tracking.c |  4 ++--
 tests/kms_psr.c                  |  5 ++--
 5 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index 5cc0fbc2..d7028f6c 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -83,9 +83,10 @@ static void restore_psr_debugfs(int sig)
 	psr_write(psr_restore_debugfs_fd, "0");
 }
 
-static bool psr_set(int debugfs_fd, bool enable)
+static bool psr_set(int debugfs_fd, enum psr_mode mode)
 {
 	int ret;
+	const char *debug_val;
 
 	ret = has_psr_debugfs(debugfs_fd);
 	if (ret == -ENODEV) {
@@ -96,7 +97,18 @@ static bool psr_set(int debugfs_fd, bool enable)
 		return false;
 	}
 
-	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
+	switch (mode) {
+	case PSR_MODE_1:
+		debug_val = "0x3";
+		break;
+	case PSR_MODE_2:
+		debug_val = "0x2";
+		break;
+	default:
+		debug_val = "0x1";
+	}
+
+	ret = psr_write(debugfs_fd, debug_val);
 	igt_assert(ret > 0);
 
 	/* Restore original value on exit */
@@ -109,23 +121,34 @@ static bool psr_set(int debugfs_fd, bool enable)
 	return ret;
 }
 
-bool psr_enable(int debugfs_fd)
+bool psr_enable(int debugfs_fd, enum psr_mode mode)
 {
-	return psr_set(debugfs_fd, true);
+	return psr_set(debugfs_fd, mode);
 }
 
 bool psr_disable(int debugfs_fd)
 {
-	return psr_set(debugfs_fd, false);
+	/* Any mode different than PSR_MODE_1/2 will disable PSR */
+	return psr_set(debugfs_fd, PSR_MODE_2 + 1);
 }
 
-bool psr_sink_support(int debugfs_fd)
+bool psr_sink_support(int debugfs_fd, enum psr_mode mode)
 {
 	char buf[PSR_STATUS_MAX_LEN];
 	int ret;
 
 	ret = igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
 				      sizeof(buf));
-	return ret > 0 && (strstr(buf, "Sink_Support: yes\n") ||
-			   strstr(buf, "Sink support: yes"));
+	if (ret < 1)
+		return false;
+
+	if (mode == PSR_MODE_1)
+		return strstr(buf, "Sink_Support: yes\n") ||
+		       strstr(buf, "Sink support: yes");
+	else
+		/*
+		 * i915 requires PSR version 0x03 that is PSR2 + SU with
+		 * Y-coordinate to support PSR2
+		 */
+		return strstr(buf, "Sink support: yes [0x03]");
 }
diff --git a/lib/igt_psr.h b/lib/igt_psr.h
index 4fff77ec..7e7017bf 100644
--- a/lib/igt_psr.h
+++ b/lib/igt_psr.h
@@ -37,8 +37,8 @@ enum psr_mode {
 
 bool psr_wait_entry(int debugfs_fd, enum psr_mode mode);
 bool psr_wait_update(int debugfs_fd, enum psr_mode mode);
-bool psr_enable(int debugfs_fd);
+bool psr_enable(int debugfs_fd, enum psr_mode);
 bool psr_disable(int debugfs_fd);
-bool psr_sink_support(int debugfs_fd);
+bool psr_sink_support(int debugfs_fd, enum psr_mode);
 
 #endif
diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
index 2823b47a..9d0d5a36 100644
--- a/tests/kms_fbcon_fbt.c
+++ b/tests/kms_fbcon_fbt.c
@@ -199,6 +199,11 @@ static bool psr_wait_until_enabled(int debugfs_fd)
 	return r;
 }
 
+static bool psr_supported_on_chipset(int debugfs_fd)
+{
+	return psr_sink_support(debugfs_fd, PSR_MODE_1);
+}
+
 static void disable_features(int debugfs_fd)
 {
 	igt_set_module_param_int("enable_fbc", 0);
@@ -212,7 +217,7 @@ static inline void fbc_modparam_enable(int debugfs_fd)
 
 static inline void psr_debugfs_enable(int debugfs_fd)
 {
-	psr_enable(debugfs_fd);
+	psr_enable(debugfs_fd, PSR_MODE_1);
 }
 
 struct feature {
@@ -226,7 +231,7 @@ struct feature {
 	.connector_possible_fn = connector_can_fbc,
 	.enable = fbc_modparam_enable,
 }, psr = {
-	.supported_on_chipset = psr_sink_support,
+	.supported_on_chipset = psr_supported_on_chipset,
 	.wait_until_enabled = psr_wait_until_enabled,
 	.connector_possible_fn = connector_can_psr,
 	.enable = psr_debugfs_enable,
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index ed9a039a..609f7b41 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1425,7 +1425,7 @@ static void setup_psr(void)
 		return;
 	}
 
-	if (!psr_sink_support(drm.debugfs)) {
+	if (!psr_sink_support(drm.debugfs, PSR_MODE_1)) {
 		igt_info("Can't test PSR: not supported by sink.\n");
 		return;
 	}
@@ -1722,7 +1722,7 @@ static bool enable_features_for_test(const struct test_mode *t)
 	if (t->feature & FEATURE_FBC)
 		fbc_enable();
 	if (t->feature & FEATURE_PSR)
-		ret = psr_enable(drm.debugfs);
+		ret = psr_enable(drm.debugfs, PSR_MODE_1);
 	if (t->feature & FEATURE_DRRS)
 		drrs_enable();
 
diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index 23d000bd..c31dc317 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -191,7 +191,8 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color)
 
 static bool sink_support(data_t *data)
 {
-	return data->with_psr_disabled || psr_sink_support(data->debugfs_fd);
+	return data->with_psr_disabled ||
+	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
 }
 
 static bool psr_wait_entry_if_enabled(data_t *data)
@@ -410,7 +411,7 @@ int main(int argc, char *argv[])
 		data.devid = intel_get_drm_devid(data.drm_fd);
 
 		if (!data.with_psr_disabled)
-			psr_enable(data.debugfs_fd);
+			psr_enable(data.debugfs_fd, PSR_MODE_1);
 
 		igt_require_f(sink_support(&data),
 			      "Sink does not support PSR\n");
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t 09/10] test/psr: Add a generic function to setup each test

[José Roberto de Souza]

When the PSR2 tests were added it will be necessary switch between
PSR versions, so lets add test_setup() and make it call
setup_test_plane() and assert if PSR is active as it is the base for
every test.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 tests/kms_psr.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index c31dc317..8f2ef89c 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -60,6 +60,7 @@ typedef struct {
 	int drm_fd;
 	int debugfs_fd;
 	enum operations op;
+	int test_plane_id;
 	uint32_t devid;
 	uint32_t crtc_id;
 	igt_display_t display;
@@ -366,6 +367,12 @@ static void setup_test_plane(data_t *data, int test_plane)
 	igt_display_commit(&data->display);
 }
 
+static void test_setup(data_t *data)
+{
+	setup_test_plane(data, data->test_plane_id);
+	igt_assert(psr_wait_entry_if_enabled(data));
+}
+
 static void dpms_off_on(data_t *data)
 {
 	kmstest_set_connector_dpms(data->drm_fd, data->output->config.connector,
@@ -424,14 +431,14 @@ int main(int argc, char *argv[])
 	}
 
 	igt_subtest("basic") {
-		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
-		igt_assert(psr_wait_entry_if_enabled(&data));
+		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
+		test_setup(&data);
 		test_cleanup(&data);
 	}
 
 	igt_subtest("no_drrs") {
-		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
-		igt_assert(psr_wait_entry_if_enabled(&data));
+		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
+		test_setup(&data);
 		igt_assert(drrs_disabled(&data));
 		test_cleanup(&data);
 	}
@@ -439,8 +446,8 @@ int main(int argc, char *argv[])
 	for (op = PAGE_FLIP; op <= RENDER; op++) {
 		igt_subtest_f("primary_%s", op_str(op)) {
 			data.op = op;
-			setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
-			igt_assert(psr_wait_entry_if_enabled(&data));
+			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
+			test_setup(&data);
 			run_test(&data);
 			test_cleanup(&data);
 		}
@@ -449,8 +456,8 @@ int main(int argc, char *argv[])
 	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
 		igt_subtest_f("sprite_%s", op_str(op)) {
 			data.op = op;
-			setup_test_plane(&data, DRM_PLANE_TYPE_OVERLAY);
-			igt_assert(psr_wait_entry_if_enabled(&data));
+			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
+			test_setup(&data);
 			run_test(&data);
 			test_cleanup(&data);
 		}
@@ -459,8 +466,8 @@ int main(int argc, char *argv[])
 	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
 		igt_subtest_f("cursor_%s", op_str(op)) {
 			data.op = op;
-			setup_test_plane(&data, DRM_PLANE_TYPE_CURSOR);
-			igt_assert(psr_wait_entry_if_enabled(&data));
+			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
+			test_setup(&data);
 			run_test(&data);
 			test_cleanup(&data);
 		}
@@ -468,8 +475,8 @@ int main(int argc, char *argv[])
 
 	igt_subtest_f("dpms") {
 		data.op = RENDER;
-		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
-		igt_assert(psr_wait_entry_if_enabled(&data));
+		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
+		test_setup(&data);
 		dpms_off_on(&data);
 		run_test(&data);
 		test_cleanup(&data);
@@ -477,8 +484,8 @@ int main(int argc, char *argv[])
 
 	igt_subtest_f("suspend") {
 		data.op = PLANE_ONOFF;
-		setup_test_plane(&data, DRM_PLANE_TYPE_CURSOR);
-		igt_assert(psr_wait_entry_if_enabled(&data));
+		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
+		test_setup(&data);
 		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
 					      SUSPEND_TEST_NONE);
 		igt_assert(psr_wait_entry_if_enabled(&data));
-- 
2.20.1

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

[igt-dev] [PATCH i-g-t 10/10] tests/psr: Add the same test coverage that we have for PSR1 to PSR2

[José Roberto de Souza]

The main tests for PSR1 check if hardware tracking is detecting
changes in planes when modifing it in different ways and now
those tests will also run for PSR2 if supported by source and sink.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 tests/kms_psr.c | 119 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 110 insertions(+), 9 deletions(-)

diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index 8f2ef89c..b2202cd3 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -61,6 +61,7 @@ typedef struct {
 	int debugfs_fd;
 	enum operations op;
 	int test_plane_id;
+	enum psr_mode op_psr_mode;
 	uint32_t devid;
 	uint32_t crtc_id;
 	igt_display_t display;
@@ -72,6 +73,7 @@ typedef struct {
 	drmModeModeInfo *mode;
 	igt_output_t *output;
 	bool with_psr_disabled;
+	bool supports_psr2;
 } data_t;
 
 static void create_cursor_fb(data_t *data)
@@ -190,10 +192,10 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color)
 	gem_bo_busy(data->drm_fd, handle);
 }
 
-static bool sink_support(data_t *data)
+static bool sink_support(data_t *data, enum psr_mode mode)
 {
 	return data->with_psr_disabled ||
-	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
+	       psr_sink_support(data->debugfs_fd, mode);
 }
 
 static bool psr_wait_entry_if_enabled(data_t *data)
@@ -201,7 +203,23 @@ static bool psr_wait_entry_if_enabled(data_t *data)
 	if (data->with_psr_disabled)
 		return true;
 
-	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
+	return psr_wait_entry(data->debugfs_fd, data->op_psr_mode);
+}
+
+static bool psr_wait_update_if_enabled(data_t *data)
+{
+	if (data->with_psr_disabled)
+		return true;
+
+	return psr_wait_update(data->debugfs_fd, data->op_psr_mode);
+}
+
+static bool psr_enable_if_enabled(data_t *data)
+{
+	if (data->with_psr_disabled)
+		return true;
+
+	return psr_enable(data->debugfs_fd, data->op_psr_mode);
 }
 
 static inline void manual(const char *expected)
@@ -291,7 +309,7 @@ static void run_test(data_t *data)
 		expected = "screen GREEN";
 		break;
 	}
-	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
+	igt_assert(psr_wait_update_if_enabled(data));
 	manual(expected);
 }
 
@@ -369,6 +387,7 @@ static void setup_test_plane(data_t *data, int test_plane)
 
 static void test_setup(data_t *data)
 {
+	psr_enable_if_enabled(data);
 	setup_test_plane(data, data->test_plane_id);
 	igt_assert(psr_wait_entry_if_enabled(data));
 }
@@ -399,7 +418,7 @@ static int opt_handler(int opt, int opt_index, void *_data)
 int main(int argc, char *argv[])
 {
 	const char *help_str =
-	       "  --no-psr\tRun test without PSR.";
+	       "  --no-psr\tRun test without PSR/PSR2.";
 	static struct option long_options[] = {
 		{"no-psr", 0, 0, 'n'},
 		{ 0, 0, 0, 0 }
@@ -417,12 +436,15 @@ int main(int argc, char *argv[])
 		kmstest_set_vt_graphics_mode();
 		data.devid = intel_get_drm_devid(data.drm_fd);
 
-		if (!data.with_psr_disabled)
-			psr_enable(data.debugfs_fd, PSR_MODE_1);
-
-		igt_require_f(sink_support(&data),
+		igt_require_f(sink_support(&data, PSR_MODE_1),
 			      "Sink does not support PSR\n");
 
+		if (sink_support(&data, PSR_MODE_2)) {
+			data.op_psr_mode = PSR_MODE_2;
+			psr_enable_if_enabled(&data);
+			data.supports_psr2 = psr_wait_entry_if_enabled(&data);
+		}
+
 		data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
 		igt_assert(data.bufmgr);
 		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
@@ -431,12 +453,31 @@ int main(int argc, char *argv[])
 	}
 
 	igt_subtest("basic") {
+		data.op_psr_mode = PSR_MODE_1;
+		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
+		test_setup(&data);
+		test_cleanup(&data);
+	}
+
+	igt_subtest("psr2_basic") {
+		igt_require(data.supports_psr2);
+		data.op_psr_mode = PSR_MODE_2;
 		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
 		test_setup(&data);
 		test_cleanup(&data);
 	}
 
 	igt_subtest("no_drrs") {
+		data.op_psr_mode = PSR_MODE_1;
+		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
+		test_setup(&data);
+		igt_assert(drrs_disabled(&data));
+		test_cleanup(&data);
+	}
+
+	igt_subtest("psr2_no_drrs") {
+		igt_require(data.supports_psr2);
+		data.op_psr_mode = PSR_MODE_2;
 		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
 		test_setup(&data);
 		igt_assert(drrs_disabled(&data));
@@ -445,6 +486,17 @@ int main(int argc, char *argv[])
 
 	for (op = PAGE_FLIP; op <= RENDER; op++) {
 		igt_subtest_f("primary_%s", op_str(op)) {
+			data.op_psr_mode = PSR_MODE_1;
+			data.op = op;
+			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
+			test_setup(&data);
+			run_test(&data);
+			test_cleanup(&data);
+		}
+
+		igt_subtest_f("psr2_primary_%s", op_str(op)) {
+			igt_require(data.supports_psr2);
+			data.op_psr_mode = PSR_MODE_2;
 			data.op = op;
 			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
 			test_setup(&data);
@@ -455,6 +507,18 @@ int main(int argc, char *argv[])
 
 	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
 		igt_subtest_f("sprite_%s", op_str(op)) {
+			igt_require(data.supports_psr2);
+			data.op_psr_mode = PSR_MODE_1;
+			data.op = op;
+			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
+			test_setup(&data);
+			run_test(&data);
+			test_cleanup(&data);
+		}
+
+		igt_subtest_f("psr2_sprite_%s", op_str(op)) {
+			igt_require(data.supports_psr2);
+			data.op_psr_mode = PSR_MODE_2;
 			data.op = op;
 			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
 			test_setup(&data);
@@ -465,6 +529,17 @@ int main(int argc, char *argv[])
 
 	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
 		igt_subtest_f("cursor_%s", op_str(op)) {
+			data.op_psr_mode = PSR_MODE_1;
+			data.op = op;
+			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
+			test_setup(&data);
+			run_test(&data);
+			test_cleanup(&data);
+		}
+
+		igt_subtest_f("psr2_cursor_%s", op_str(op)) {
+			igt_require(data.supports_psr2);
+			data.op_psr_mode = PSR_MODE_2;
 			data.op = op;
 			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
 			test_setup(&data);
@@ -474,6 +549,18 @@ int main(int argc, char *argv[])
 	}
 
 	igt_subtest_f("dpms") {
+		data.op_psr_mode = PSR_MODE_1;
+		data.op = RENDER;
+		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
+		test_setup(&data);
+		dpms_off_on(&data);
+		run_test(&data);
+		test_cleanup(&data);
+	}
+
+	igt_subtest_f("psr2_dpms") {
+		igt_require(data.supports_psr2);
+		data.op_psr_mode = PSR_MODE_2;
 		data.op = RENDER;
 		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
 		test_setup(&data);
@@ -483,6 +570,20 @@ int main(int argc, char *argv[])
 	}
 
 	igt_subtest_f("suspend") {
+		data.op_psr_mode = PSR_MODE_1;
+		data.op = PLANE_ONOFF;
+		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
+		test_setup(&data);
+		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
+					      SUSPEND_TEST_NONE);
+		igt_assert(psr_wait_entry_if_enabled(&data));
+		run_test(&data);
+		test_cleanup(&data);
+	}
+
+	igt_subtest_f("psr2_suspend") {
+		igt_require(data.supports_psr2);
+		data.op_psr_mode = PSR_MODE_2;
 		data.op = PLANE_ONOFF;
 		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
 		test_setup(&data);
-- 
2.20.1

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

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,01/10] lib/psr: Add a macro with the maximum lenght of i915_edp_psr_status and use it

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,01/10] lib/psr: Add a macro with the maximum lenght of i915_edp_psr_status and use it
URL   : https://patchwork.freedesktop.org/series/55112/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5408 -> IGTPW_2228
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_psr@sprite_plane_onoff:
    - fi-icl-u3:          PASS -> {SKIP}
    - fi-icl-u2:          PASS -> {SKIP}

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-skl-6600u:       PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       PASS -> FAIL [fdo#108767]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  
#### Possible fixes ####

  * igt@gem_mmap_gtt@basic:
    - fi-glk-dsi:         INCOMPLETE [fdo#103359] / [k.org#198133] -> PASS

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-kbl-7567u:       DMESG-WARN [fdo#105602] / [fdo#108529] -> PASS +1

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        DMESG-FAIL [fdo#108735] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
    - fi-kbl-7567u:       {SKIP} [fdo#109271] -> PASS +33

  * igt@pm_rpm@basic-pci-d3-state:
    - fi-byt-j1900:       {SKIP} [fdo#109271] -> PASS
    - fi-bsw-kefka:       {SKIP} [fdo#109271] -> PASS

  * igt@pm_rpm@basic-rte:
    - fi-byt-j1900:       FAIL [fdo#108800] -> PASS
    - fi-bsw-kefka:       FAIL [fdo#108800] -> PASS

  * igt@pm_rpm@module-reload:
    - fi-kbl-7567u:       DMESG-WARN [fdo#108529] -> PASS

  
#### Warnings ####

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       DMESG-FAIL [fdo#105079] -> DMESG-WARN [fdo#108473]

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

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#108473]: https://bugs.freedesktop.org/show_bug.cgi?id=108473
  [fdo#108529]: https://bugs.freedesktop.org/show_bug.cgi?id=108529
  [fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
  [fdo#108767]: https://bugs.freedesktop.org/show_bug.cgi?id=108767
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


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

  Missing    (4): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


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

    * IGT: IGT_4764 -> IGTPW_2228

  CI_DRM_5408: bd0d0bbe6b5a740429be306d7eea41703daf08cc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2228: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2228/
  IGT_4764: db19bccc1c220884baf2136e41683a3c2ba6aa24 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@kms_psr@psr2_basic
+igt@kms_psr@psr2_cursor_blt
+igt@kms_psr@psr2_cursor_mmap_cpu
+igt@kms_psr@psr2_cursor_mmap_gtt
+igt@kms_psr@psr2_cursor_plane_move
+igt@kms_psr@psr2_cursor_plane_onoff
+igt@kms_psr@psr2_cursor_render
+igt@kms_psr@psr2_dpms
+igt@kms_psr@psr2_no_drrs
+igt@kms_psr@psr2_primary_blt
+igt@kms_psr@psr2_primary_mmap_cpu
+igt@kms_psr@psr2_primary_mmap_gtt
+igt@kms_psr@psr2_primary_page_flip
+igt@kms_psr@psr2_primary_render
+igt@kms_psr@psr2_sprite_blt
+igt@kms_psr@psr2_sprite_mmap_cpu
+igt@kms_psr@psr2_sprite_mmap_gtt
+igt@kms_psr@psr2_sprite_plane_move
+igt@kms_psr@psr2_sprite_plane_onoff
+igt@kms_psr@psr2_sprite_render
+igt@kms_psr@psr2_suspend

== Logs ==

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

[igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,01/10] lib/psr: Add a macro with the maximum lenght of i915_edp_psr_status and use it

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,01/10] lib/psr: Add a macro with the maximum lenght of i915_edp_psr_status and use it
URL   : https://patchwork.freedesktop.org/series/55112/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5408_full -> IGTPW_2228_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
    - shard-apl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_color@pipe-a-degamma:
    - shard-apl:          PASS -> FAIL [fdo#104782] / [fdo#108145]

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665] +1
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_crc@cursor-256x85-onscreen:
    - shard-apl:          PASS -> FAIL [fdo#103232] +4

  * igt@kms_cursor_crc@cursor-64x64-sliding:
    - shard-kbl:          PASS -> FAIL [fdo#103232]

  * igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
    - shard-glk:          PASS -> FAIL [fdo#105454] / [fdo#106509]

  * igt@kms_plane@pixel-format-pipe-a-planes-source-clamping:
    - shard-apl:          PASS -> FAIL [fdo#108948]

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-glk:          PASS -> FAIL [fdo#108948]

  * igt@kms_plane@pixel-format-pipe-c-planes:
    - shard-apl:          PASS -> FAIL [fdo#103166] +2

  * igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
    - shard-apl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166] +3

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-kbl:          PASS -> FAIL [fdo#103166]

  
#### Possible fixes ####

  * igt@kms_available_modes_crc@available_mode_test_crc:
    - shard-apl:          FAIL [fdo#106641] -> PASS

  * igt@kms_cursor_crc@cursor-256x256-random:
    - shard-glk:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-kbl:          DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS +14

  * igt@kms_cursor_crc@cursor-64x64-dpms:
    - shard-kbl:          FAIL [fdo#103232] -> PASS +1
    - shard-apl:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_plane@plane-position-covered-pipe-a-planes:
    - shard-glk:          FAIL [fdo#103166] -> PASS +3

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS +2

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
    - shard-glk:          FAIL [fdo#108145] -> PASS

  * igt@pm_rc6_residency@rc6-accuracy:
    - shard-snb:          {SKIP} [fdo#109271] -> PASS

  
#### Warnings ####

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          DMESG-WARN [fdo#105604] -> DMESG-FAIL [fdo#108950]

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

  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105454]: https://bugs.freedesktop.org/show_bug.cgi?id=105454
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#105604]: https://bugs.freedesktop.org/show_bug.cgi?id=105604
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#106509]: https://bugs.freedesktop.org/show_bug.cgi?id=106509
  [fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#108950]: https://bugs.freedesktop.org/show_bug.cgi?id=108950
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


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

    * IGT: IGT_4764 -> IGTPW_2228
    * Piglit: piglit_4509 -> None

  CI_DRM_5408: bd0d0bbe6b5a740429be306d7eea41703daf08cc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2228: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2228/
  IGT_4764: db19bccc1c220884baf2136e41683a3c2ba6aa24 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [igt-dev] [PATCH i-g-t 10/10] tests/psr: Add the same test coverage that we have for PSR1 to PSR2

[Rodrigo Vivi]

On Fri, Jan 11, 2019 at 05:46:07PM -0800, José Roberto de Souza wrote:
> The main tests for PSR1 check if hardware tracking is detecting
> changes in planes when modifing it in different ways and now
> those tests will also run for PSR2 if supported by source and sink.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

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

> ---
>  tests/kms_psr.c | 119 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 110 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index 8f2ef89c..b2202cd3 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -61,6 +61,7 @@ typedef struct {
>  	int debugfs_fd;
>  	enum operations op;
>  	int test_plane_id;
> +	enum psr_mode op_psr_mode;
>  	uint32_t devid;
>  	uint32_t crtc_id;
>  	igt_display_t display;
> @@ -72,6 +73,7 @@ typedef struct {
>  	drmModeModeInfo *mode;
>  	igt_output_t *output;
>  	bool with_psr_disabled;
> +	bool supports_psr2;
>  } data_t;
>  
>  static void create_cursor_fb(data_t *data)
> @@ -190,10 +192,10 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color)
>  	gem_bo_busy(data->drm_fd, handle);
>  }
>  
> -static bool sink_support(data_t *data)
> +static bool sink_support(data_t *data, enum psr_mode mode)
>  {
>  	return data->with_psr_disabled ||
> -	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> +	       psr_sink_support(data->debugfs_fd, mode);
>  }
>  
>  static bool psr_wait_entry_if_enabled(data_t *data)
> @@ -201,7 +203,23 @@ static bool psr_wait_entry_if_enabled(data_t *data)
>  	if (data->with_psr_disabled)
>  		return true;
>  
> -	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> +	return psr_wait_entry(data->debugfs_fd, data->op_psr_mode);
> +}
> +
> +static bool psr_wait_update_if_enabled(data_t *data)
> +{
> +	if (data->with_psr_disabled)
> +		return true;
> +
> +	return psr_wait_update(data->debugfs_fd, data->op_psr_mode);
> +}
> +
> +static bool psr_enable_if_enabled(data_t *data)
> +{
> +	if (data->with_psr_disabled)
> +		return true;
> +
> +	return psr_enable(data->debugfs_fd, data->op_psr_mode);
>  }
>  
>  static inline void manual(const char *expected)
> @@ -291,7 +309,7 @@ static void run_test(data_t *data)
>  		expected = "screen GREEN";
>  		break;
>  	}
> -	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
> +	igt_assert(psr_wait_update_if_enabled(data));
>  	manual(expected);
>  }
>  
> @@ -369,6 +387,7 @@ static void setup_test_plane(data_t *data, int test_plane)
>  
>  static void test_setup(data_t *data)
>  {
> +	psr_enable_if_enabled(data);
>  	setup_test_plane(data, data->test_plane_id);
>  	igt_assert(psr_wait_entry_if_enabled(data));
>  }
> @@ -399,7 +418,7 @@ static int opt_handler(int opt, int opt_index, void *_data)
>  int main(int argc, char *argv[])
>  {
>  	const char *help_str =
> -	       "  --no-psr\tRun test without PSR.";
> +	       "  --no-psr\tRun test without PSR/PSR2.";
>  	static struct option long_options[] = {
>  		{"no-psr", 0, 0, 'n'},
>  		{ 0, 0, 0, 0 }
> @@ -417,12 +436,15 @@ int main(int argc, char *argv[])
>  		kmstest_set_vt_graphics_mode();
>  		data.devid = intel_get_drm_devid(data.drm_fd);
>  
> -		if (!data.with_psr_disabled)
> -			psr_enable(data.debugfs_fd, PSR_MODE_1);
> -
> -		igt_require_f(sink_support(&data),
> +		igt_require_f(sink_support(&data, PSR_MODE_1),
>  			      "Sink does not support PSR\n");
>  
> +		if (sink_support(&data, PSR_MODE_2)) {
> +			data.op_psr_mode = PSR_MODE_2;
> +			psr_enable_if_enabled(&data);
> +			data.supports_psr2 = psr_wait_entry_if_enabled(&data);
> +		}
> +
>  		data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
>  		igt_assert(data.bufmgr);
>  		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> @@ -431,12 +453,31 @@ int main(int argc, char *argv[])
>  	}
>  
>  	igt_subtest("basic") {
> +		data.op_psr_mode = PSR_MODE_1;
> +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> +		test_setup(&data);
> +		test_cleanup(&data);
> +	}
> +
> +	igt_subtest("psr2_basic") {
> +		igt_require(data.supports_psr2);
> +		data.op_psr_mode = PSR_MODE_2;
>  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
>  		test_setup(&data);
>  		test_cleanup(&data);
>  	}
>  
>  	igt_subtest("no_drrs") {
> +		data.op_psr_mode = PSR_MODE_1;
> +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> +		test_setup(&data);
> +		igt_assert(drrs_disabled(&data));
> +		test_cleanup(&data);
> +	}
> +
> +	igt_subtest("psr2_no_drrs") {
> +		igt_require(data.supports_psr2);
> +		data.op_psr_mode = PSR_MODE_2;
>  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
>  		test_setup(&data);
>  		igt_assert(drrs_disabled(&data));
> @@ -445,6 +486,17 @@ int main(int argc, char *argv[])
>  
>  	for (op = PAGE_FLIP; op <= RENDER; op++) {
>  		igt_subtest_f("primary_%s", op_str(op)) {
> +			data.op_psr_mode = PSR_MODE_1;
> +			data.op = op;
> +			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> +			test_setup(&data);
> +			run_test(&data);
> +			test_cleanup(&data);
> +		}
> +
> +		igt_subtest_f("psr2_primary_%s", op_str(op)) {
> +			igt_require(data.supports_psr2);
> +			data.op_psr_mode = PSR_MODE_2;
>  			data.op = op;
>  			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
>  			test_setup(&data);
> @@ -455,6 +507,18 @@ int main(int argc, char *argv[])
>  
>  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
>  		igt_subtest_f("sprite_%s", op_str(op)) {
> +			igt_require(data.supports_psr2);
> +			data.op_psr_mode = PSR_MODE_1;
> +			data.op = op;
> +			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> +			test_setup(&data);
> +			run_test(&data);
> +			test_cleanup(&data);
> +		}
> +
> +		igt_subtest_f("psr2_sprite_%s", op_str(op)) {
> +			igt_require(data.supports_psr2);
> +			data.op_psr_mode = PSR_MODE_2;
>  			data.op = op;
>  			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
>  			test_setup(&data);
> @@ -465,6 +529,17 @@ int main(int argc, char *argv[])
>  
>  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
>  		igt_subtest_f("cursor_%s", op_str(op)) {
> +			data.op_psr_mode = PSR_MODE_1;
> +			data.op = op;
> +			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> +			test_setup(&data);
> +			run_test(&data);
> +			test_cleanup(&data);
> +		}
> +
> +		igt_subtest_f("psr2_cursor_%s", op_str(op)) {
> +			igt_require(data.supports_psr2);
> +			data.op_psr_mode = PSR_MODE_2;
>  			data.op = op;
>  			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
>  			test_setup(&data);
> @@ -474,6 +549,18 @@ int main(int argc, char *argv[])
>  	}
>  
>  	igt_subtest_f("dpms") {
> +		data.op_psr_mode = PSR_MODE_1;
> +		data.op = RENDER;
> +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> +		test_setup(&data);
> +		dpms_off_on(&data);
> +		run_test(&data);
> +		test_cleanup(&data);
> +	}
> +
> +	igt_subtest_f("psr2_dpms") {
> +		igt_require(data.supports_psr2);
> +		data.op_psr_mode = PSR_MODE_2;
>  		data.op = RENDER;
>  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
>  		test_setup(&data);
> @@ -483,6 +570,20 @@ int main(int argc, char *argv[])
>  	}
>  
>  	igt_subtest_f("suspend") {
> +		data.op_psr_mode = PSR_MODE_1;
> +		data.op = PLANE_ONOFF;
> +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> +		test_setup(&data);
> +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
> +					      SUSPEND_TEST_NONE);
> +		igt_assert(psr_wait_entry_if_enabled(&data));
> +		run_test(&data);
> +		test_cleanup(&data);
> +	}
> +
> +	igt_subtest_f("psr2_suspend") {
> +		igt_require(data.supports_psr2);
> +		data.op_psr_mode = PSR_MODE_2;
>  		data.op = PLANE_ONOFF;
>  		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
>  		test_setup(&data);
> -- 
> 2.20.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 03/10] lib/psr: Add support to new modified i915_edp_psr_status output

[Rodrigo Vivi]

On Fri, Jan 11, 2019 at 05:46:00PM -0800, José Roberto de Souza wrote:
> The kernel patch 'drm/i915: Refactor PSR status debugfs' changed the
> output of i915_edp_psr_status, so adding support to the new output
> here while keeping the support to the old one for a while.
> 
> In psr_active() we were overdoing, we just need to check the source
> state to know if PSR is active and doing that we keep compability to
> old and new i915_edp_psr_status output.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

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

> ---
>  lib/igt_psr.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index 82012e6d..a59ff94e 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -32,8 +32,8 @@ static bool psr_active(int debugfs_fd, bool check_active)
>  
>  	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
>  				sizeof(buf));
> -	active = strstr(buf, "HW Enabled & Active bit: yes\n") &&
> -		(strstr(buf, "SRDENT") || strstr(buf, "SLEEP"));
> +
> +	active = strstr(buf, "SRDENT") || strstr(buf, "SLEEP");
>  	return check_active ? active : !active;
>  }
>  
> @@ -137,5 +137,6 @@ bool psr_sink_support(int debugfs_fd)
>  
>  	ret = igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
>  				      sizeof(buf));
> -	return ret > 0 && strstr(buf, "Sink_Support: yes\n");
> +	return ret > 0 && (strstr(buf, "Sink_Support: yes\n") ||
> +			   strstr(buf, "Sink support: yes"));
>  }
> -- 
> 2.20.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 05/10] lib/psr: Rename psr_wait_exit to psr_wait_update

[Dhinakaran Pandiyan]

On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> This is a initial preparation for PSR2 test support, as in PSR2 a
> update to screen could mean that PSR is still active and the screen
> will be update by a selective update this renamed is necessary.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Patches 1 - 5
 Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

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

Re: [igt-dev] [PATCH i-g-t 06/10] lib/psr: Make psr_wait_entry and psr_wait_update aware of the PSR version tested

[Dhinakaran Pandiyan]

On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> This way we can test both PSR version separated.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  lib/igt_psr.c                    | 26 ++++++++++++++++++--------
>  lib/igt_psr.h                    |  9 +++++++--
>  tests/kms_frontbuffer_tracking.c |  4 ++--
>  tests/kms_psr.c                  |  4 ++--
>  4 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index 5edec6a2..c6638c2c 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -25,26 +25,36 @@
>  #include "igt_sysfs.h"
>  #include <errno.h>
>  
> -static bool psr_active(int debugfs_fd, bool check_active)
> +static bool psr_state_check(int debugfs_fd, const char *state)
nit: Is there a plan to use this function for anything else? Making
this function check only for the active state is more straightforward
IMO

-static bool psr_state_check(int debugfs_fd, const char *state)
+static bool psr_active_check(int debugfs_fd, enum psr_mode mode)
 {
        char buf[PSR_STATUS_MAX_LEN];
+       char *state;
 
+       state = mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
        igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
                                sizeof(buf));
 
        return strstr(buf, state);
 }
>  {
> -	bool active;
>  	char buf[PSR_STATUS_MAX_LEN];
>  
>  	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
>  				sizeof(buf));
>  
> -	active = strstr(buf, "SRDENT") || strstr(buf, "DEEP_SLEEP");
> -	return check_active ? active : !active;
> +	return strstr(buf, state);
>  }
>  
> -bool psr_wait_entry(int debugfs_fd)
> +static inline const char *psr_active_state_get(enum psr_mode mode)
>  {
> -	return igt_wait(psr_active(debugfs_fd, true), 500, 20);
> +	return mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
>  }
>  
> -bool psr_wait_update(int debugfs_fd)
> +/*
> + * For PSR1, we wait until PSR is active. We wait until DEEP_SLEEP
> for PSR2.
> + */
> +bool psr_wait_entry(int debugfs_fd, enum psr_mode mode)
>  {
> -	return igt_wait(psr_active(debugfs_fd, false), 40, 10);
> +	const char *state = psr_active_state_get(mode);
> +
> +	return igt_wait(psr_state_check(debugfs_fd, state), 500, 20);
> +}
> +
> +bool psr_wait_update(int debugfs_fd, enum psr_mode mode)
> +{
> +	const char *state = psr_active_state_get(mode);
> +
> +	return igt_wait(!psr_state_check(debugfs_fd, state), 40, 10);
>  }
>  
>  static ssize_t psr_write(int debugfs_fd, const char *buf)
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> index 62c680ee..4fff77ec 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -30,8 +30,13 @@
>  
>  #define PSR_STATUS_MAX_LEN 512
>  
> -bool psr_wait_entry(int debugfs_fd);
> -bool psr_wait_update(int debugfs_fd);
> +enum psr_mode {
Add a 
	PSR_MODE_DISABLE, too?
> +	PSR_MODE_1,
> +	PSR_MODE_2
> +};
> +
> +bool psr_wait_entry(int debugfs_fd, enum psr_mode mode);
> +bool psr_wait_update(int debugfs_fd, enum psr_mode mode);
>  bool psr_enable(int debugfs_fd);
>  bool psr_disable(int debugfs_fd);
>  bool psr_sink_support(int debugfs_fd);
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index cd7dda91..ed9a039a 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -1622,10 +1622,10 @@ static void do_status_assertions(int flags)
>  	}
>  
>  	if (flags & ASSERT_PSR_ENABLED)
> -		igt_assert_f(psr_wait_entry(drm.debugfs),
> +		igt_assert_f(psr_wait_entry(drm.debugfs, PSR_MODE_1),
>  			     "PSR still disabled\n");
>  	else if (flags & ASSERT_PSR_DISABLED)
> -		igt_assert_f(psr_wait_update(drm.debugfs),
> +		igt_assert_f(psr_wait_update(drm.debugfs, PSR_MODE_1),
>  			     "PSR still enabled\n");
>  }
>  
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index fb3bd7a4..23d000bd 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -199,7 +199,7 @@ static bool psr_wait_entry_if_enabled(data_t
> *data)
>  	if (data->with_psr_disabled)
>  		return true;
>  
> -	return psr_wait_entry(data->debugfs_fd);
> +	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
>  }
>  
>  static inline void manual(const char *expected)
> @@ -289,7 +289,7 @@ static void run_test(data_t *data)
>  		expected = "screen GREEN";
>  		break;
>  	}
> -	igt_assert(psr_wait_update(data->debugfs_fd));
> +	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
>  	manual(expected);
>  }
>  

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

Re: [igt-dev] [PATCH i-g-t 07/10] lib/psr: Drop support to old kernels without new PSR debugfs interface

[Dhinakaran Pandiyan]

On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> The nexts patches will add PSR2 tests and for that we need the new
> PSR debugfs interface that was released in kernel 4.20 so it will not
> break for any updated system and we can drop support to the old
> debugsfs and the support to kernels without even a debugfs interface.

Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does not
support the new interface?


> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  lib/igt_psr.c | 35 +++++++----------------------------
>  1 file changed, 7 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index c6638c2c..5cc0fbc2 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -73,27 +73,7 @@ static int has_psr_debugfs(int debugfs_fd)
>  	 * -ENODEV is returned when PSR is unavailable.
>  	 */
>  	ret = psr_write(debugfs_fd, "0xf");
> -	if (ret == -EINVAL)
> -		return 0;
> -	else if (ret < 0)
> -		return ret;
> -
> -	/* legacy debugfs api, we enabled irqs by writing, disable
> them. */
> -	psr_write(debugfs_fd, "0");
> -	return -EINVAL;
> -}
> -
> -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;
> +	return ret == -EINVAL ? 0 : ret;
>  }
>  
>  static int psr_restore_debugfs_fd = -1;
> @@ -109,16 +89,15 @@ static bool psr_set(int debugfs_fd, bool enable)
>  
>  	ret = has_psr_debugfs(debugfs_fd);
>  	if (ret == -ENODEV) {
> -		igt_skip_on_f(enable, "PSR not available\n");
> +		igt_skip("PSR not available\n");
> +		return false;
> +	} else if (ret) {
> +		igt_skip("PSR debugfs interface not available\n");
>  		return false;
>  	}
>  
> -	if (ret == -EINVAL) {
> -		ret = psr_modparam_set(enable);
> -	} else {
> -		ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> -		igt_assert(ret > 0);
> -	}
> +	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> +	igt_assert(ret > 0);
>  
>  	/* Restore original value on exit */
>  	if (psr_restore_debugfs_fd == -1) {

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

Re: [igt-dev] [PATCH i-g-t 08/10] lib/psr: Add PSR2 support to the remaning psr functions

[Dhinakaran Pandiyan]

On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> Add the mode parameter to psr_enable() and psr_sink_support() so PSR1
> and PSR2 can be tested separated.
> For now all PSR tests will run only with PSR1 and the tests for PSR2
> will come in the future.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  lib/igt_psr.c                    | 39 +++++++++++++++++++++++++-----
> --
>  lib/igt_psr.h                    |  4 ++--
>  tests/kms_fbcon_fbt.c            |  9 ++++++--
>  tests/kms_frontbuffer_tracking.c |  4 ++--
>  tests/kms_psr.c                  |  5 ++--
>  5 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index 5cc0fbc2..d7028f6c 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -83,9 +83,10 @@ static void restore_psr_debugfs(int sig)
>  	psr_write(psr_restore_debugfs_fd, "0");
>  }
>  
> -static bool psr_set(int debugfs_fd, bool enable)
> +static bool psr_set(int debugfs_fd, enum psr_mode mode)
>  {
>  	int ret;
> +	const char *debug_val;
>  
>  	ret = has_psr_debugfs(debugfs_fd);
>  	if (ret == -ENODEV) {
> @@ -96,7 +97,18 @@ static bool psr_set(int debugfs_fd, bool enable)
>  		return false;
>  	}
>  
> -	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> +	switch (mode) {
> +	case PSR_MODE_1:
> +		debug_val = "0x3";
> +		break;
> +	case PSR_MODE_2:
> +		debug_val = "0x2";
> +		break;
> +	default:
> +		debug_val = "0x1";
> +	}
> +
> +	ret = psr_write(debugfs_fd, debug_val);
>  	igt_assert(ret > 0);
>  
>  	/* Restore original value on exit */
> @@ -109,23 +121,34 @@ static bool psr_set(int debugfs_fd, bool
> enable)
>  	return ret;
>  }
>  
> -bool psr_enable(int debugfs_fd)
> +bool psr_enable(int debugfs_fd, enum psr_mode mode)
>  {
> -	return psr_set(debugfs_fd, true);
> +	return psr_set(debugfs_fd, mode);
>  }
>  
>  bool psr_disable(int debugfs_fd)
>  {
> -	return psr_set(debugfs_fd, false);
> +	/* Any mode different than PSR_MODE_1/2 will disable PSR */
Please consider adding PSR_MODE_DISABLE to get rid of this comment.

> +	return psr_set(debugfs_fd, PSR_MODE_2 + 1);
>  }
>  
> -bool psr_sink_support(int debugfs_fd)
> +bool psr_sink_support(int debugfs_fd, enum psr_mode mode)
>  {
>  	char buf[PSR_STATUS_MAX_LEN];
>  	int ret;
>  
>  	ret = igt_debugfs_simple_read(debugfs_fd,
> "i915_edp_psr_status", buf,
>  				      sizeof(buf));
> -	return ret > 0 && (strstr(buf, "Sink_Support: yes\n") ||
> -			   strstr(buf, "Sink support: yes"));
> +	if (ret < 1)
> +		return false;
> +
> +	if (mode == PSR_MODE_1)
> +		return strstr(buf, "Sink_Support: yes\n") ||
> +		       strstr(buf, "Sink support: yes");
> +	else
> +		/*
> +		 * i915 requires PSR version 0x03 that is PSR2 + SU
> with
> +		 * Y-coordinate to support PSR2
> +		 */
> +		return strstr(buf, "Sink support: yes [0x03]");
For some reason, I thought we also print whether the sink supports PSR1
or PSR2 in debugfs. Hope it's not too late, did we ever consider

Sink support: {No, PSR1, PSR2} [<PSR dpcd version>]?

>  }
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> index 4fff77ec..7e7017bf 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -37,8 +37,8 @@ enum psr_mode {
>  
>  bool psr_wait_entry(int debugfs_fd, enum psr_mode mode);
>  bool psr_wait_update(int debugfs_fd, enum psr_mode mode);
> -bool psr_enable(int debugfs_fd);
> +bool psr_enable(int debugfs_fd, enum psr_mode);
>  bool psr_disable(int debugfs_fd);
> -bool psr_sink_support(int debugfs_fd);
> +bool psr_sink_support(int debugfs_fd, enum psr_mode);
>  
>  #endif
> diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
> index 2823b47a..9d0d5a36 100644
> --- a/tests/kms_fbcon_fbt.c
> +++ b/tests/kms_fbcon_fbt.c
> @@ -199,6 +199,11 @@ static bool psr_wait_until_enabled(int
> debugfs_fd)
>  	return r;
>  }
>  
> +static bool psr_supported_on_chipset(int debugfs_fd)
> +{
> +	return psr_sink_support(debugfs_fd, PSR_MODE_1);
> +}
> +
>  static void disable_features(int debugfs_fd)
>  {
>  	igt_set_module_param_int("enable_fbc", 0);
> @@ -212,7 +217,7 @@ static inline void fbc_modparam_enable(int
> debugfs_fd)
>  
>  static inline void psr_debugfs_enable(int debugfs_fd)
>  {
> -	psr_enable(debugfs_fd);
> +	psr_enable(debugfs_fd, PSR_MODE_1);
>  }
>  
>  struct feature {
> @@ -226,7 +231,7 @@ struct feature {
>  	.connector_possible_fn = connector_can_fbc,
>  	.enable = fbc_modparam_enable,
>  }, psr = {
> -	.supported_on_chipset = psr_sink_support,
> +	.supported_on_chipset = psr_supported_on_chipset,
>  	.wait_until_enabled = psr_wait_until_enabled,
>  	.connector_possible_fn = connector_can_psr,
>  	.enable = psr_debugfs_enable,
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index ed9a039a..609f7b41 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -1425,7 +1425,7 @@ static void setup_psr(void)
>  		return;
>  	}
>  
> -	if (!psr_sink_support(drm.debugfs)) {
> +	if (!psr_sink_support(drm.debugfs, PSR_MODE_1)) {
>  		igt_info("Can't test PSR: not supported by sink.\n");
>  		return;
>  	}
> @@ -1722,7 +1722,7 @@ static bool enable_features_for_test(const
> struct test_mode *t)
>  	if (t->feature & FEATURE_FBC)
>  		fbc_enable();
>  	if (t->feature & FEATURE_PSR)
> -		ret = psr_enable(drm.debugfs);
> +		ret = psr_enable(drm.debugfs, PSR_MODE_1);
>  	if (t->feature & FEATURE_DRRS)
>  		drrs_enable();
>  
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index 23d000bd..c31dc317 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -191,7 +191,8 @@ static void fill_render(data_t *data, uint32_t
> handle, unsigned char color)
>  
>  static bool sink_support(data_t *data)
>  {
> -	return data->with_psr_disabled || psr_sink_support(data-
> >debugfs_fd);
> +	return data->with_psr_disabled ||
> +	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
>  }
>  
>  static bool psr_wait_entry_if_enabled(data_t *data)
> @@ -410,7 +411,7 @@ int main(int argc, char *argv[])
>  		data.devid = intel_get_drm_devid(data.drm_fd);
>  
Store it in struct data so that the value stays consistent throughout
the test case?
>  		if (!data.with_psr_disabled)
> -			psr_enable(data.debugfs_fd);
> +			psr_enable(data.debugfs_fd, PSR_MODE_1);
>  
>  		igt_require_f(sink_support(&data),
>  			      "Sink does not support PSR\n");

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

Re: [igt-dev] [PATCH i-g-t 10/10] tests/psr: Add the same test coverage that we have for PSR1 to PSR2

[Dhinakaran Pandiyan]

On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> The main tests for PSR1 check if hardware tracking is detecting
> changes in planes when modifing it in different ways and now
> those tests will also run for PSR2 if supported by source and sink.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  tests/kms_psr.c | 119 ++++++++++++++++++++++++++++++++++++++++++++
> ----
>  1 file changed, 110 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index 8f2ef89c..b2202cd3 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -61,6 +61,7 @@ typedef struct {
>  	int debugfs_fd;
>  	enum operations op;
>  	int test_plane_id;
> +	enum psr_mode op_psr_mode;
>  	uint32_t devid;
>  	uint32_t crtc_id;
>  	igt_display_t display;
> @@ -72,6 +73,7 @@ typedef struct {
>  	drmModeModeInfo *mode;
>  	igt_output_t *output;
>  	bool with_psr_disabled;
> +	bool supports_psr2;
>  } data_t;
>  
>  static void create_cursor_fb(data_t *data)
> @@ -190,10 +192,10 @@ static void fill_render(data_t *data, uint32_t
> handle, unsigned char color)
>  	gem_bo_busy(data->drm_fd, handle);
>  }
>  
> -static bool sink_support(data_t *data)
> +static bool sink_support(data_t *data, enum psr_mode mode)
>  {
>  	return data->with_psr_disabled ||
> -	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> +	       psr_sink_support(data->debugfs_fd, mode);
>  }
>  
>  static bool psr_wait_entry_if_enabled(data_t *data)
> @@ -201,7 +203,23 @@ static bool psr_wait_entry_if_enabled(data_t
> *data)
>  	if (data->with_psr_disabled)
>  		return true;
>  
> -	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> +	return psr_wait_entry(data->debugfs_fd, data->op_psr_mode);
> +}
> +
> +static bool psr_wait_update_if_enabled(data_t *data)
> +{
> +	if (data->with_psr_disabled)
> +		return true;
> +
> +	return psr_wait_update(data->debugfs_fd, data->op_psr_mode);
> +}
> +
> +static bool psr_enable_if_enabled(data_t *data)
This function name is quite confusing. What do you think of using
macros like this

#define PSR_WAIT_ENTRY(data, mode)
	return data->with_psr_disabled || psr_wait_entry(data-
>debugfs_fd, mode);
...


> +{
> +	if (data->with_psr_disabled)
> +		return true;
> +
> +	return psr_enable(data->debugfs_fd, data->op_psr_mode);
>  }
>  
>  static inline void manual(const char *expected)
> @@ -291,7 +309,7 @@ static void run_test(data_t *data)
>  		expected = "screen GREEN";
>  		break;
>  	}
> -	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
> +	igt_assert(psr_wait_update_if_enabled(data));
>  	manual(expected);
>  }
>  
> @@ -369,6 +387,7 @@ static void setup_test_plane(data_t *data, int
> test_plane)
>  
>  static void test_setup(data_t *data)
>  {
> +	psr_enable_if_enabled(data);
>  	setup_test_plane(data, data->test_plane_id);
>  	igt_assert(psr_wait_entry_if_enabled(data));
>  }
> @@ -399,7 +418,7 @@ static int opt_handler(int opt, int opt_index,
> void *_data)
>  int main(int argc, char *argv[])
>  {
>  	const char *help_str =
> -	       "  --no-psr\tRun test without PSR.";
> +	       "  --no-psr\tRun test without PSR/PSR2.";
>  	static struct option long_options[] = {
>  		{"no-psr", 0, 0, 'n'},
>  		{ 0, 0, 0, 0 }
> @@ -417,12 +436,15 @@ int main(int argc, char *argv[])
>  		kmstest_set_vt_graphics_mode();
>  		data.devid = intel_get_drm_devid(data.drm_fd);
>  
> -		if (!data.with_psr_disabled)
> -			psr_enable(data.debugfs_fd, PSR_MODE_1);
> -
> -		igt_require_f(sink_support(&data),
> +		igt_require_f(sink_support(&data, PSR_MODE_1),
>  			      "Sink does not support PSR\n");
>  
> +		if (sink_support(&data, PSR_MODE_2)) {
> +			data.op_psr_mode = PSR_MODE_2;
> +			psr_enable_if_enabled(&data);
There is another enable in test_setup() that the PSR2 subtests call.
Why do we need two?

> +			data.supports_psr2 =
> psr_wait_entry_if_enabled(&data);
> +		}
> +
>  		data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd,
> 4096);
>  		igt_assert(data.bufmgr);
>  		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> @@ -431,12 +453,31 @@ int main(int argc, char *argv[])
>  	}
>  
>  	igt_subtest("basic") {
> +		data.op_psr_mode = PSR_MODE_1;
> +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> +		test_setup(&data);
> +		test_cleanup(&data);
> +	}
Run this in a loop (PSR_MODE_1...PSR_MODE_2) instead of repeating the
same calls? 

> +
> +	igt_subtest("psr2_basic") {
> +		igt_require(data.supports_psr2);
We don't have to generate this subtest if the sink does not support
PSR2. kms_frontbuffer_tracking generates subtests dynamically, we can
use the same approach here.

> +		data.op_psr_mode = PSR_MODE_2;
>  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
>  		test_setup(&data);
>  		test_cleanup(&data);
>  	}
>  
>  	igt_subtest("no_drrs") {
> +		data.op_psr_mode = PSR_MODE_1;
> +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> +		test_setup(&data);
> +		igt_assert(drrs_disabled(&data));
> +		test_cleanup(&data);
> +	}
> +
> +	igt_subtest("psr2_no_drrs") {
> +		igt_require(data.supports_psr2);
> +		data.op_psr_mode = PSR_MODE_2;
>  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
>  		test_setup(&data);
>  		igt_assert(drrs_disabled(&data));
> @@ -445,6 +486,17 @@ int main(int argc, char *argv[])
>  
>  	for (op = PAGE_FLIP; op <= RENDER; op++) {
>  		igt_subtest_f("primary_%s", op_str(op)) {
> +			data.op_psr_mode = PSR_MODE_1;
> +			data.op = op;
> +			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> +			test_setup(&data);
> +			run_test(&data);
> +			test_cleanup(&data);
> +		}
> +
> +		igt_subtest_f("psr2_primary_%s", op_str(op)) {
> +			igt_require(data.supports_psr2);
> +			data.op_psr_mode = PSR_MODE_2;
>  			data.op = op;
>  			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
>  			test_setup(&data);
> @@ -455,6 +507,18 @@ int main(int argc, char *argv[])
>  
>  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
>  		igt_subtest_f("sprite_%s", op_str(op)) {
> +			igt_require(data.supports_psr2);
> +			data.op_psr_mode = PSR_MODE_1;
> +			data.op = op;
> +			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> +			test_setup(&data);
> +			run_test(&data);
> +			test_cleanup(&data);
> +		}
> +
> +		igt_subtest_f("psr2_sprite_%s", op_str(op)) {
> +			igt_require(data.supports_psr2);
> +			data.op_psr_mode = PSR_MODE_2;
>  			data.op = op;
>  			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
>  			test_setup(&data);
> @@ -465,6 +529,17 @@ int main(int argc, char *argv[])
>  
>  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
>  		igt_subtest_f("cursor_%s", op_str(op)) {
> +			data.op_psr_mode = PSR_MODE_1;
> +			data.op = op;
> +			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> +			test_setup(&data);
> +			run_test(&data);
> +			test_cleanup(&data);
> +		}
> +
> +		igt_subtest_f("psr2_cursor_%s", op_str(op)) {
> +			igt_require(data.supports_psr2);
> +			data.op_psr_mode = PSR_MODE_2;
>  			data.op = op;
>  			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
>  			test_setup(&data);
> @@ -474,6 +549,18 @@ int main(int argc, char *argv[])
>  	}
>  
>  	igt_subtest_f("dpms") {
> +		data.op_psr_mode = PSR_MODE_1;
> +		data.op = RENDER;
> +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> +		test_setup(&data);
> +		dpms_off_on(&data);
> +		run_test(&data);
> +		test_cleanup(&data);
> +	}
> +
> +	igt_subtest_f("psr2_dpms") {
> +		igt_require(data.supports_psr2);
> +		data.op_psr_mode = PSR_MODE_2;
>  		data.op = RENDER;
>  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
>  		test_setup(&data);
> @@ -483,6 +570,20 @@ int main(int argc, char *argv[])
>  	}
>  
>  	igt_subtest_f("suspend") {
> +		data.op_psr_mode = PSR_MODE_1;
> +		data.op = PLANE_ONOFF;
> +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> +		test_setup(&data);
> +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
> +					      SUSPEND_TEST_NONE);
> +		igt_assert(psr_wait_entry_if_enabled(&data));
> +		run_test(&data);
> +		test_cleanup(&data);
> +	}
> +
> +	igt_subtest_f("psr2_suspend") {
> +		igt_require(data.supports_psr2);
> +		data.op_psr_mode = PSR_MODE_2;
>  		data.op = PLANE_ONOFF;
>  		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
>  		test_setup(&data);

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

Re: [igt-dev] [PATCH i-g-t 06/10] lib/psr: Make psr_wait_entry and psr_wait_update aware of the PSR version tested

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 5435 bytes --]

On Tue, 2019-01-15 at 21:19 -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > This way we can test both PSR version separated.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  lib/igt_psr.c                    | 26 ++++++++++++++++++--------
> >  lib/igt_psr.h                    |  9 +++++++--
> >  tests/kms_frontbuffer_tracking.c |  4 ++--
> >  tests/kms_psr.c                  |  4 ++--
> >  4 files changed, 29 insertions(+), 14 deletions(-)
> > 
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > index 5edec6a2..c6638c2c 100644
> > --- a/lib/igt_psr.c
> > +++ b/lib/igt_psr.c
> > @@ -25,26 +25,36 @@
> >  #include "igt_sysfs.h"
> >  #include <errno.h>
> >  
> > -static bool psr_active(int debugfs_fd, bool check_active)
> > +static bool psr_state_check(int debugfs_fd, const char *state)
> nit: Is there a plan to use this function for anything else? Making
> this function check only for the active state is more straightforward
> IMO
> 
> -static bool psr_state_check(int debugfs_fd, const char *state)
> +static bool psr_active_check(int debugfs_fd, enum psr_mode mode)
>  {
>         char buf[PSR_STATUS_MAX_LEN];
> +       char *state;
>  
> +       state = mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
>         igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status",
> buf,
>                                 sizeof(buf));
>  
>         return strstr(buf, state);
>  }

Yeah, I don't see other use than check for active state.
I will apply this change, thanks.

> >  {
> > -	bool active;
> >  	char buf[PSR_STATUS_MAX_LEN];
> >  
> >  	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
> >  				sizeof(buf));
> >  
> > -	active = strstr(buf, "SRDENT") || strstr(buf, "DEEP_SLEEP");
> > -	return check_active ? active : !active;
> > +	return strstr(buf, state);
> >  }
> >  
> > -bool psr_wait_entry(int debugfs_fd)
> > +static inline const char *psr_active_state_get(enum psr_mode mode)
> >  {
> > -	return igt_wait(psr_active(debugfs_fd, true), 500, 20);
> > +	return mode == PSR_MODE_1 ? "SRDENT" : "DEEP_SLEEP";
> >  }
> >  
> > -bool psr_wait_update(int debugfs_fd)
> > +/*
> > + * For PSR1, we wait until PSR is active. We wait until DEEP_SLEEP
> > for PSR2.
> > + */
> > +bool psr_wait_entry(int debugfs_fd, enum psr_mode mode)
> >  {
> > -	return igt_wait(psr_active(debugfs_fd, false), 40, 10);
> > +	const char *state = psr_active_state_get(mode);
> > +
> > +	return igt_wait(psr_state_check(debugfs_fd, state), 500, 20);
> > +}
> > +
> > +bool psr_wait_update(int debugfs_fd, enum psr_mode mode)
> > +{
> > +	const char *state = psr_active_state_get(mode);
> > +
> > +	return igt_wait(!psr_state_check(debugfs_fd, state), 40, 10);
> >  }
> >  
> >  static ssize_t psr_write(int debugfs_fd, const char *buf)
> > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > index 62c680ee..4fff77ec 100644
> > --- a/lib/igt_psr.h
> > +++ b/lib/igt_psr.h
> > @@ -30,8 +30,13 @@
> >  
> >  #define PSR_STATUS_MAX_LEN 512
> >  
> > -bool psr_wait_entry(int debugfs_fd);
> > -bool psr_wait_update(int debugfs_fd);
> > +enum psr_mode {
> Add a 
> 	PSR_MODE_DISABLE, too?

To keep all functions simple we should not add this, otherwise we would
need to earlier return in psr_wait_entry(), psr_wait_update() and
psr_sink_support() when PSR_MODE_DISABLE is set? Or calling those
funciton with PSR_MODE_DISABLE mean that we should call psr_disable()?!

I guess is better just keep PSR_MODE_1 and PSR_MODE_2.

> > +	PSR_MODE_1,
> > +	PSR_MODE_2
> > +};
> > +
> > +bool psr_wait_entry(int debugfs_fd, enum psr_mode mode);
> > +bool psr_wait_update(int debugfs_fd, enum psr_mode mode);
> >  bool psr_enable(int debugfs_fd);
> >  bool psr_disable(int debugfs_fd);
> >  bool psr_sink_support(int debugfs_fd);
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index cd7dda91..ed9a039a 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1622,10 +1622,10 @@ static void do_status_assertions(int flags)
> >  	}
> >  
> >  	if (flags & ASSERT_PSR_ENABLED)
> > -		igt_assert_f(psr_wait_entry(drm.debugfs),
> > +		igt_assert_f(psr_wait_entry(drm.debugfs, PSR_MODE_1),
> >  			     "PSR still disabled\n");
> >  	else if (flags & ASSERT_PSR_DISABLED)
> > -		igt_assert_f(psr_wait_update(drm.debugfs),
> > +		igt_assert_f(psr_wait_update(drm.debugfs, PSR_MODE_1),
> >  			     "PSR still enabled\n");
> >  }
> >  
> > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > index fb3bd7a4..23d000bd 100644
> > --- a/tests/kms_psr.c
> > +++ b/tests/kms_psr.c
> > @@ -199,7 +199,7 @@ static bool psr_wait_entry_if_enabled(data_t
> > *data)
> >  	if (data->with_psr_disabled)
> >  		return true;
> >  
> > -	return psr_wait_entry(data->debugfs_fd);
> > +	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> >  }
> >  
> >  static inline void manual(const char *expected)
> > @@ -289,7 +289,7 @@ static void run_test(data_t *data)
> >  		expected = "screen GREEN";
> >  		break;
> >  	}
> > -	igt_assert(psr_wait_update(data->debugfs_fd));
> > +	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
> >  	manual(expected);
> >  }
> >  

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [igt-dev] [PATCH i-g-t 07/10] lib/psr: Drop support to old kernels without new PSR debugfs interface

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 2629 bytes --]

On Tue, 2019-01-15 at 21:21 -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > The nexts patches will add PSR2 tests and for that we need the new
> > PSR debugfs interface that was released in kernel 4.20 so it will
> > not
> > break for any updated system and we can drop support to the old
> > debugsfs and the support to kernels without even a debugfs
> > interface.
> 
> Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does not
> support the new interface?

We could but why keep supporting old kernels? My experience of running
a newer IGT over a old kernel or on the contrary was always
problematic.
	

> 
> 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  lib/igt_psr.c | 35 +++++++----------------------------
> >  1 file changed, 7 insertions(+), 28 deletions(-)
> > 
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > index c6638c2c..5cc0fbc2 100644
> > --- a/lib/igt_psr.c
> > +++ b/lib/igt_psr.c
> > @@ -73,27 +73,7 @@ static int has_psr_debugfs(int debugfs_fd)
> >  	 * -ENODEV is returned when PSR is unavailable.
> >  	 */
> >  	ret = psr_write(debugfs_fd, "0xf");
> > -	if (ret == -EINVAL)
> > -		return 0;
> > -	else if (ret < 0)
> > -		return ret;
> > -
> > -	/* legacy debugfs api, we enabled irqs by writing, disable
> > them. */
> > -	psr_write(debugfs_fd, "0");
> > -	return -EINVAL;
> > -}
> > -
> > -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;
> > +	return ret == -EINVAL ? 0 : ret;
> >  }
> >  
> >  static int psr_restore_debugfs_fd = -1;
> > @@ -109,16 +89,15 @@ static bool psr_set(int debugfs_fd, bool
> > enable)
> >  
> >  	ret = has_psr_debugfs(debugfs_fd);
> >  	if (ret == -ENODEV) {
> > -		igt_skip_on_f(enable, "PSR not available\n");
> > +		igt_skip("PSR not available\n");
> > +		return false;
> > +	} else if (ret) {
> > +		igt_skip("PSR debugfs interface not available\n");
> >  		return false;
> >  	}
> >  
> > -	if (ret == -EINVAL) {
> > -		ret = psr_modparam_set(enable);
> > -	} else {
> > -		ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > -		igt_assert(ret > 0);
> > -	}
> > +	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > +	igt_assert(ret > 0);
> >  
> >  	/* Restore original value on exit */
> >  	if (psr_restore_debugfs_fd == -1) {

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [igt-dev] [PATCH i-g-t 08/10] lib/psr: Add PSR2 support to the remaning psr functions

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 7530 bytes --]

On Tue, 2019-01-15 at 21:33 -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > Add the mode parameter to psr_enable() and psr_sink_support() so
> > PSR1
> > and PSR2 can be tested separated.
> > For now all PSR tests will run only with PSR1 and the tests for
> > PSR2
> > will come in the future.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  lib/igt_psr.c                    | 39 +++++++++++++++++++++++++---
> > --
> > --
> >  lib/igt_psr.h                    |  4 ++--
> >  tests/kms_fbcon_fbt.c            |  9 ++++++--
> >  tests/kms_frontbuffer_tracking.c |  4 ++--
> >  tests/kms_psr.c                  |  5 ++--
> >  5 files changed, 45 insertions(+), 16 deletions(-)
> > 
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > index 5cc0fbc2..d7028f6c 100644
> > --- a/lib/igt_psr.c
> > +++ b/lib/igt_psr.c
> > @@ -83,9 +83,10 @@ static void restore_psr_debugfs(int sig)
> >  	psr_write(psr_restore_debugfs_fd, "0");
> >  }
> >  
> > -static bool psr_set(int debugfs_fd, bool enable)
> > +static bool psr_set(int debugfs_fd, enum psr_mode mode)
> >  {
> >  	int ret;
> > +	const char *debug_val;
> >  
> >  	ret = has_psr_debugfs(debugfs_fd);
> >  	if (ret == -ENODEV) {
> > @@ -96,7 +97,18 @@ static bool psr_set(int debugfs_fd, bool enable)
> >  		return false;
> >  	}
> >  
> > -	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > +	switch (mode) {
> > +	case PSR_MODE_1:
> > +		debug_val = "0x3";
> > +		break;
> > +	case PSR_MODE_2:
> > +		debug_val = "0x2";
> > +		break;
> > +	default:
> > +		debug_val = "0x1";
> > +	}
> > +
> > +	ret = psr_write(debugfs_fd, debug_val);
> >  	igt_assert(ret > 0);
> >  
> >  	/* Restore original value on exit */
> > @@ -109,23 +121,34 @@ static bool psr_set(int debugfs_fd, bool
> > enable)
> >  	return ret;
> >  }
> >  
> > -bool psr_enable(int debugfs_fd)
> > +bool psr_enable(int debugfs_fd, enum psr_mode mode)
> >  {
> > -	return psr_set(debugfs_fd, true);
> > +	return psr_set(debugfs_fd, mode);
> >  }
> >  
> >  bool psr_disable(int debugfs_fd)
> >  {
> > -	return psr_set(debugfs_fd, false);
> > +	/* Any mode different than PSR_MODE_1/2 will disable PSR */
> Please consider adding PSR_MODE_DISABLE to get rid of this comment.

I have commented in the previous patch, my opinion is that is better
have this comments and handle this way than let user call all other
functions with PSR_MODE_DISABLE.

> 
> > +	return psr_set(debugfs_fd, PSR_MODE_2 + 1);
> >  }
> >  
> > -bool psr_sink_support(int debugfs_fd)
> > +bool psr_sink_support(int debugfs_fd, enum psr_mode mode)
> >  {
> >  	char buf[PSR_STATUS_MAX_LEN];
> >  	int ret;
> >  
> >  	ret = igt_debugfs_simple_read(debugfs_fd,
> > "i915_edp_psr_status", buf,
> >  				      sizeof(buf));
> > -	return ret > 0 && (strstr(buf, "Sink_Support: yes\n") ||
> > -			   strstr(buf, "Sink support: yes"));
> > +	if (ret < 1)
> > +		return false;
> > +
> > +	if (mode == PSR_MODE_1)
> > +		return strstr(buf, "Sink_Support: yes\n") ||
> > +		       strstr(buf, "Sink support: yes");
> > +	else
> > +		/*
> > +		 * i915 requires PSR version 0x03 that is PSR2 + SU
> > with
> > +		 * Y-coordinate to support PSR2
> > +		 */
> > +		return strstr(buf, "Sink support: yes [0x03]");
> For some reason, I thought we also print whether the sink supports
> PSR1
> or PSR2 in debugfs. Hope it's not too late, did we ever consider
> 
> Sink support: {No, PSR1, PSR2} [<PSR dpcd version>]?

Okay, I will change kernel and IGT to have this debugfs output.
We should print it just based on the DPCD version? Like version 0x2 is
PSR2 but we don't support PSR2 in that version of panel, same as a
version 0x3 that do not require the Y-coordinate.

> 
> >  }
> > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > index 4fff77ec..7e7017bf 100644
> > --- a/lib/igt_psr.h
> > +++ b/lib/igt_psr.h
> > @@ -37,8 +37,8 @@ enum psr_mode {
> >  
> >  bool psr_wait_entry(int debugfs_fd, enum psr_mode mode);
> >  bool psr_wait_update(int debugfs_fd, enum psr_mode mode);
> > -bool psr_enable(int debugfs_fd);
> > +bool psr_enable(int debugfs_fd, enum psr_mode);
> >  bool psr_disable(int debugfs_fd);
> > -bool psr_sink_support(int debugfs_fd);
> > +bool psr_sink_support(int debugfs_fd, enum psr_mode);
> >  
> >  #endif
> > diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
> > index 2823b47a..9d0d5a36 100644
> > --- a/tests/kms_fbcon_fbt.c
> > +++ b/tests/kms_fbcon_fbt.c
> > @@ -199,6 +199,11 @@ static bool psr_wait_until_enabled(int
> > debugfs_fd)
> >  	return r;
> >  }
> >  
> > +static bool psr_supported_on_chipset(int debugfs_fd)
> > +{
> > +	return psr_sink_support(debugfs_fd, PSR_MODE_1);
> > +}
> > +
> >  static void disable_features(int debugfs_fd)
> >  {
> >  	igt_set_module_param_int("enable_fbc", 0);
> > @@ -212,7 +217,7 @@ static inline void fbc_modparam_enable(int
> > debugfs_fd)
> >  
> >  static inline void psr_debugfs_enable(int debugfs_fd)
> >  {
> > -	psr_enable(debugfs_fd);
> > +	psr_enable(debugfs_fd, PSR_MODE_1);
> >  }
> >  
> >  struct feature {
> > @@ -226,7 +231,7 @@ struct feature {
> >  	.connector_possible_fn = connector_can_fbc,
> >  	.enable = fbc_modparam_enable,
> >  }, psr = {
> > -	.supported_on_chipset = psr_sink_support,
> > +	.supported_on_chipset = psr_supported_on_chipset,
> >  	.wait_until_enabled = psr_wait_until_enabled,
> >  	.connector_possible_fn = connector_can_psr,
> >  	.enable = psr_debugfs_enable,
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index ed9a039a..609f7b41 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1425,7 +1425,7 @@ static void setup_psr(void)
> >  		return;
> >  	}
> >  
> > -	if (!psr_sink_support(drm.debugfs)) {
> > +	if (!psr_sink_support(drm.debugfs, PSR_MODE_1)) {
> >  		igt_info("Can't test PSR: not supported by sink.\n");
> >  		return;
> >  	}
> > @@ -1722,7 +1722,7 @@ static bool enable_features_for_test(const
> > struct test_mode *t)
> >  	if (t->feature & FEATURE_FBC)
> >  		fbc_enable();
> >  	if (t->feature & FEATURE_PSR)
> > -		ret = psr_enable(drm.debugfs);
> > +		ret = psr_enable(drm.debugfs, PSR_MODE_1);
> >  	if (t->feature & FEATURE_DRRS)
> >  		drrs_enable();
> >  
> > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > index 23d000bd..c31dc317 100644
> > --- a/tests/kms_psr.c
> > +++ b/tests/kms_psr.c
> > @@ -191,7 +191,8 @@ static void fill_render(data_t *data, uint32_t
> > handle, unsigned char color)
> >  
> >  static bool sink_support(data_t *data)
> >  {
> > -	return data->with_psr_disabled || psr_sink_support(data-
> > > debugfs_fd);
> > +	return data->with_psr_disabled ||
> > +	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> >  }
> >  
> >  static bool psr_wait_entry_if_enabled(data_t *data)
> > @@ -410,7 +411,7 @@ int main(int argc, char *argv[])
> >  		data.devid = intel_get_drm_devid(data.drm_fd);
> >  
> Store it in struct data so that the value stays consistent throughout
> the test case?
> >  		if (!data.with_psr_disabled)
> > -			psr_enable(data.debugfs_fd);
> > +			psr_enable(data.debugfs_fd, PSR_MODE_1);
> >  
> >  		igt_require_f(sink_support(&data),
> >  			      "Sink does not support PSR\n");

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [igt-dev] [PATCH i-g-t 07/10] lib/psr: Drop support to old kernels without new PSR debugfs interface

[Dhinakaran Pandiyan]

On Wed, 2019-01-16 at 18:07 -0800, Souza, Jose wrote:
> On Tue, 2019-01-15 at 21:21 -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > The nexts patches will add PSR2 tests and for that we need the
> > > new
> > > PSR debugfs interface that was released in kernel 4.20 so it will
> > > not
> > > break for any updated system and we can drop support to the old
> > > debugsfs and the support to kernels without even a debugfs
> > > interface.
> > 
> > Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does
> > not
> > support the new interface?
> 
> We could but why keep supporting old kernels? My experience of
> running
> a newer IGT over a old kernel or on the contrary was always
> problematic.

Downstream OS'es can package a newer IGT with an older kernel. In this
case, it's not much of an effort to continue support for PSR1 tests
from what I can tell. 
> 	
> 
> > 
> > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  lib/igt_psr.c | 35 +++++++----------------------------
> > >  1 file changed, 7 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index c6638c2c..5cc0fbc2 100644
> > > --- a/lib/igt_psr.c
> > > +++ b/lib/igt_psr.c
> > > @@ -73,27 +73,7 @@ static int has_psr_debugfs(int debugfs_fd)
> > >  	 * -ENODEV is returned when PSR is unavailable.
> > >  	 */
> > >  	ret = psr_write(debugfs_fd, "0xf");
> > > -	if (ret == -EINVAL)
> > > -		return 0;
> > > -	else if (ret < 0)
> > > -		return ret;
> > > -
> > > -	/* legacy debugfs api, we enabled irqs by writing, disable
> > > them. */
> > > -	psr_write(debugfs_fd, "0");
> > > -	return -EINVAL;
> > > -}
> > > -
> > > -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;
> > > +	return ret == -EINVAL ? 0 : ret;
> > >  }
> > >  
> > >  static int psr_restore_debugfs_fd = -1;
> > > @@ -109,16 +89,15 @@ static bool psr_set(int debugfs_fd, bool
> > > enable)
> > >  
> > >  	ret = has_psr_debugfs(debugfs_fd);
> > >  	if (ret == -ENODEV) {
> > > -		igt_skip_on_f(enable, "PSR not available\n");
> > > +		igt_skip("PSR not available\n");
> > > +		return false;
> > > +	} else if (ret) {
> > > +		igt_skip("PSR debugfs interface not available\n");
> > >  		return false;
> > >  	}
> > >  
> > > -	if (ret == -EINVAL) {
> > > -		ret = psr_modparam_set(enable);
> > > -	} else {
> > > -		ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > > -		igt_assert(ret > 0);
> > > -	}
> > > +	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > > +	igt_assert(ret > 0);
> > >  
> > >  	/* Restore original value on exit */
> > >  	if (psr_restore_debugfs_fd == -1) {

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

Re: [igt-dev] [PATCH i-g-t 08/10] lib/psr: Add PSR2 support to the remaning psr functions

[Dhinakaran Pandiyan]

On Wed, 2019-01-16 at 18:14 -0800, Souza, Jose wrote:
> On Tue, 2019-01-15 at 21:33 -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > Add the mode parameter to psr_enable() and psr_sink_support() so
> > > PSR1
> > > and PSR2 can be tested separated.
> > > For now all PSR tests will run only with PSR1 and the tests for
> > > PSR2
> > > will come in the future.
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  lib/igt_psr.c                    | 39 +++++++++++++++++++++++++-
> > > --
> > > --
> > > --
> > >  lib/igt_psr.h                    |  4 ++--
> > >  tests/kms_fbcon_fbt.c            |  9 ++++++--
> > >  tests/kms_frontbuffer_tracking.c |  4 ++--
> > >  tests/kms_psr.c                  |  5 ++--
> > >  5 files changed, 45 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index 5cc0fbc2..d7028f6c 100644
> > > --- a/lib/igt_psr.c
> > > +++ b/lib/igt_psr.c
> > > @@ -83,9 +83,10 @@ static void restore_psr_debugfs(int sig)
> > >  	psr_write(psr_restore_debugfs_fd, "0");
> > >  }
> > >  
> > > -static bool psr_set(int debugfs_fd, bool enable)
> > > +static bool psr_set(int debugfs_fd, enum psr_mode mode)
> > >  {
> > >  	int ret;
> > > +	const char *debug_val;
> > >  
> > >  	ret = has_psr_debugfs(debugfs_fd);
> > >  	if (ret == -ENODEV) {
> > > @@ -96,7 +97,18 @@ static bool psr_set(int debugfs_fd, bool
> > > enable)
> > >  		return false;
> > >  	}
> > >  
> > > -	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > > +	switch (mode) {
> > > +	case PSR_MODE_1:
> > > +		debug_val = "0x3";
> > > +		break;
> > > +	case PSR_MODE_2:
> > > +		debug_val = "0x2";
> > > +		break;
> > > +	default:
> > > +		debug_val = "0x1";
> > > +	}
> > > +
> > > +	ret = psr_write(debugfs_fd, debug_val);
> > >  	igt_assert(ret > 0);
> > >  
> > >  	/* Restore original value on exit */
> > > @@ -109,23 +121,34 @@ static bool psr_set(int debugfs_fd, bool
> > > enable)
> > >  	return ret;
> > >  }
> > >  
> > > -bool psr_enable(int debugfs_fd)
> > > +bool psr_enable(int debugfs_fd, enum psr_mode mode)
> > >  {
> > > -	return psr_set(debugfs_fd, true);
> > > +	return psr_set(debugfs_fd, mode);
> > >  }
> > >  
> > >  bool psr_disable(int debugfs_fd)
> > >  {
> > > -	return psr_set(debugfs_fd, false);
> > > +	/* Any mode different than PSR_MODE_1/2 will disable PSR */
> > 
> > Please consider adding PSR_MODE_DISABLE to get rid of this comment.
> 
> I have commented in the previous patch, my opinion is that is better
> have this comments and handle this way than let user call all other
> functions with PSR_MODE_DISABLE.
Let's go ahead as is and improve this later.

> 
> > 
> > > +	return psr_set(debugfs_fd, PSR_MODE_2 + 1);
> > >  }
> > >  
> > > -bool psr_sink_support(int debugfs_fd)
> > > +bool psr_sink_support(int debugfs_fd, enum psr_mode mode)
> > >  {
> > >  	char buf[PSR_STATUS_MAX_LEN];
> > >  	int ret;
> > >  
> > >  	ret = igt_debugfs_simple_read(debugfs_fd,
> > > "i915_edp_psr_status", buf,
> > >  				      sizeof(buf));
> > > -	return ret > 0 && (strstr(buf, "Sink_Support: yes\n") ||
> > > -			   strstr(buf, "Sink support: yes"));
> > > +	if (ret < 1)
> > > +		return false;
> > > +
> > > +	if (mode == PSR_MODE_1)
> > > +		return strstr(buf, "Sink_Support: yes\n") ||
> > > +		       strstr(buf, "Sink support: yes");
> > > +	else
> > > +		/*
> > > +		 * i915 requires PSR version 0x03 that is PSR2 + SU
> > > with
> > > +		 * Y-coordinate to support PSR2
> > > +		 */
> > > +		return strstr(buf, "Sink support: yes [0x03]");
> > 
> > For some reason, I thought we also print whether the sink supports
> > PSR1
> > or PSR2 in debugfs. Hope it's not too late, did we ever consider
> > 
> > Sink support: {No, PSR1, PSR2} [<PSR dpcd version>]?
> 
> Okay, I will change kernel and IGT to have this debugfs output.
> We should print it just based on the DPCD version? Like version 0x2
> is
> PSR2 but we don't support PSR2 in that version of panel, same as a
> version 0x3 that do not require the Y-coordinate.
Oh yeah, that can get confusing without the debug message. The version
info from the DPCD is clearer even if not descriptive. Feel free to
leave it as is.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


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

Re: [igt-dev] [PATCH i-g-t 07/10] lib/psr: Drop support to old kernels without new PSR debugfs interface

[Petri Latvala]

On Thu, Jan 17, 2019 at 02:07:27AM +0000, Souza, Jose wrote:
> On Tue, 2019-01-15 at 21:21 -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > The nexts patches will add PSR2 tests and for that we need the new
> > > PSR debugfs interface that was released in kernel 4.20 so it will
> > > not
> > > break for any updated system and we can drop support to the old
> > > debugsfs and the support to kernels without even a debugfs
> > > interface.
> > 
> > Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does not
> > support the new interface?
> 
> We could but why keep supporting old kernels? My experience of running
> a newer IGT over a old kernel or on the contrary was always
> problematic.


Any problematic experiences you might have encountered should be filed
as bugs. We do support old kernels (to a length) and shouldn't
knowingly break support for them without an informed decision.


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

Re: [igt-dev] [PATCH i-g-t 07/10] lib/psr: Drop support to old kernels without new PSR debugfs interface

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 3557 bytes --]

On Wed, 2019-01-16 at 18:16 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2019-01-16 at 18:07 -0800, Souza, Jose wrote:
> > On Tue, 2019-01-15 at 21:21 -0800, Dhinakaran Pandiyan wrote:
> > > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > > The nexts patches will add PSR2 tests and for that we need the
> > > > new
> > > > PSR debugfs interface that was released in kernel 4.20 so it
> > > > will
> > > > not
> > > > break for any updated system and we can drop support to the old
> > > > debugsfs and the support to kernels without even a debugfs
> > > > interface.
> > > 
> > > Can't we have psr_set(mode == PSR_MODE_2) skip if the kernel does
> > > not
> > > support the new interface?
> > 
> > We could but why keep supporting old kernels? My experience of
> > running
> > a newer IGT over a old kernel or on the contrary was always
> > problematic.
> 
> Downstream OS'es can package a newer IGT with an older kernel. In
> this
> case, it's not much of an effort to continue support for PSR1 tests
> from what I can tell. 

Other problem here it that with the enable_psr parameter we don't
control what PSR version is going to be enabled, if possible PSR2 will
be enabled causing tests to fail in the first psr_wait_entry().

But okay, I will drop this patch.



> > 	
> > 
> > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  lib/igt_psr.c | 35 +++++++----------------------------
> > > >  1 file changed, 7 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > > index c6638c2c..5cc0fbc2 100644
> > > > --- a/lib/igt_psr.c
> > > > +++ b/lib/igt_psr.c
> > > > @@ -73,27 +73,7 @@ static int has_psr_debugfs(int debugfs_fd)
> > > >  	 * -ENODEV is returned when PSR is unavailable.
> > > >  	 */
> > > >  	ret = psr_write(debugfs_fd, "0xf");
> > > > -	if (ret == -EINVAL)
> > > > -		return 0;
> > > > -	else if (ret < 0)
> > > > -		return ret;
> > > > -
> > > > -	/* legacy debugfs api, we enabled irqs by writing,
> > > > disable
> > > > them. */
> > > > -	psr_write(debugfs_fd, "0");
> > > > -	return -EINVAL;
> > > > -}
> > > > -
> > > > -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;
> > > > +	return ret == -EINVAL ? 0 : ret;
> > > >  }
> > > >  
> > > >  static int psr_restore_debugfs_fd = -1;
> > > > @@ -109,16 +89,15 @@ static bool psr_set(int debugfs_fd, bool
> > > > enable)
> > > >  
> > > >  	ret = has_psr_debugfs(debugfs_fd);
> > > >  	if (ret == -ENODEV) {
> > > > -		igt_skip_on_f(enable, "PSR not available\n");
> > > > +		igt_skip("PSR not available\n");
> > > > +		return false;
> > > > +	} else if (ret) {
> > > > +		igt_skip("PSR debugfs interface not
> > > > available\n");
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	if (ret == -EINVAL) {
> > > > -		ret = psr_modparam_set(enable);
> > > > -	} else {
> > > > -		ret = psr_write(debugfs_fd, enable ? "0x3" :
> > > > "0x1");
> > > > -		igt_assert(ret > 0);
> > > > -	}
> > > > +	ret = psr_write(debugfs_fd, enable ? "0x3" : "0x1");
> > > > +	igt_assert(ret > 0);
> > > >  
> > > >  	/* Restore original value on exit */
> > > >  	if (psr_restore_debugfs_fd == -1) {

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [igt-dev] [PATCH i-g-t 10/10] tests/psr: Add the same test coverage that we have for PSR1 to PSR2

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 9500 bytes --]

On Tue, 2019-01-15 at 22:44 -0800, Dhinakaran Pandiyan wrote:
> On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > The main tests for PSR1 check if hardware tracking is detecting
> > changes in planes when modifing it in different ways and now
> > those tests will also run for PSR2 if supported by source and sink.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  tests/kms_psr.c | 119 ++++++++++++++++++++++++++++++++++++++++++++
> > ----
> >  1 file changed, 110 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > index 8f2ef89c..b2202cd3 100644
> > --- a/tests/kms_psr.c
> > +++ b/tests/kms_psr.c
> > @@ -61,6 +61,7 @@ typedef struct {
> >  	int debugfs_fd;
> >  	enum operations op;
> >  	int test_plane_id;
> > +	enum psr_mode op_psr_mode;
> >  	uint32_t devid;
> >  	uint32_t crtc_id;
> >  	igt_display_t display;
> > @@ -72,6 +73,7 @@ typedef struct {
> >  	drmModeModeInfo *mode;
> >  	igt_output_t *output;
> >  	bool with_psr_disabled;
> > +	bool supports_psr2;
> >  } data_t;
> >  
> >  static void create_cursor_fb(data_t *data)
> > @@ -190,10 +192,10 @@ static void fill_render(data_t *data,
> > uint32_t
> > handle, unsigned char color)
> >  	gem_bo_busy(data->drm_fd, handle);
> >  }
> >  
> > -static bool sink_support(data_t *data)
> > +static bool sink_support(data_t *data, enum psr_mode mode)
> >  {
> >  	return data->with_psr_disabled ||
> > -	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> > +	       psr_sink_support(data->debugfs_fd, mode);
> >  }
> >  
> >  static bool psr_wait_entry_if_enabled(data_t *data)
> > @@ -201,7 +203,23 @@ static bool psr_wait_entry_if_enabled(data_t
> > *data)
> >  	if (data->with_psr_disabled)
> >  		return true;
> >  
> > -	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> > +	return psr_wait_entry(data->debugfs_fd, data->op_psr_mode);
> > +}
> > +
> > +static bool psr_wait_update_if_enabled(data_t *data)
> > +{
> > +	if (data->with_psr_disabled)
> > +		return true;
> > +
> > +	return psr_wait_update(data->debugfs_fd, data->op_psr_mode);
> > +}
> > +
> > +static bool psr_enable_if_enabled(data_t *data)
> This function name is quite confusing. What do you think of using
> macros like this
> 
> #define PSR_WAIT_ENTRY(data, mode)
> 	return data->with_psr_disabled || psr_wait_entry(data-
> > debugfs_fd, mode);
> ...


I'm happy with both approaches but I guess we can change that in a
patch on top of this one.

> 
> 
> > +{
> > +	if (data->with_psr_disabled)
> > +		return true;
> > +
> > +	return psr_enable(data->debugfs_fd, data->op_psr_mode);
> >  }
> >  
> >  static inline void manual(const char *expected)
> > @@ -291,7 +309,7 @@ static void run_test(data_t *data)
> >  		expected = "screen GREEN";
> >  		break;
> >  	}
> > -	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
> > +	igt_assert(psr_wait_update_if_enabled(data));
> >  	manual(expected);
> >  }
> >  
> > @@ -369,6 +387,7 @@ static void setup_test_plane(data_t *data, int
> > test_plane)
> >  
> >  static void test_setup(data_t *data)
> >  {
> > +	psr_enable_if_enabled(data);
> >  	setup_test_plane(data, data->test_plane_id);
> >  	igt_assert(psr_wait_entry_if_enabled(data));
> >  }
> > @@ -399,7 +418,7 @@ static int opt_handler(int opt, int opt_index,
> > void *_data)
> >  int main(int argc, char *argv[])
> >  {
> >  	const char *help_str =
> > -	       "  --no-psr\tRun test without PSR.";
> > +	       "  --no-psr\tRun test without PSR/PSR2.";
> >  	static struct option long_options[] = {
> >  		{"no-psr", 0, 0, 'n'},
> >  		{ 0, 0, 0, 0 }
> > @@ -417,12 +436,15 @@ int main(int argc, char *argv[])
> >  		kmstest_set_vt_graphics_mode();
> >  		data.devid = intel_get_drm_devid(data.drm_fd);
> >  
> > -		if (!data.with_psr_disabled)
> > -			psr_enable(data.debugfs_fd, PSR_MODE_1);
> > -
> > -		igt_require_f(sink_support(&data),
> > +		igt_require_f(sink_support(&data, PSR_MODE_1),
> >  			      "Sink does not support PSR\n");
> >  
> > +		if (sink_support(&data, PSR_MODE_2)) {
> > +			data.op_psr_mode = PSR_MODE_2;
> > +			psr_enable_if_enabled(&data);
> There is another enable in test_setup() that the PSR2 subtests call.
> Why do we need two?

Here I'm enabling by hand to check if source and sink support PSR2 so
data.supports_psr2 is set with the right value and we skip the PSR2
tests when not supported.


> 
> > +			data.supports_psr2 =
> > psr_wait_entry_if_enabled(&data);
> > +		}
> > +
> >  		data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd,
> > 4096);
> >  		igt_assert(data.bufmgr);
> >  		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > @@ -431,12 +453,31 @@ int main(int argc, char *argv[])
> >  	}
> >  
> >  	igt_subtest("basic") {
> > +		data.op_psr_mode = PSR_MODE_1;
> > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > +		test_setup(&data);
> > +		test_cleanup(&data);
> > +	}
> Run this in a loop (PSR_MODE_1...PSR_MODE_2) instead of repeating the
> same calls? 

I think have separated tests is better because CI reports will show
what version of PSR has a regression, we can run specific tests while
debuging...

> 
> > +
> > +	igt_subtest("psr2_basic") {
> > +		igt_require(data.supports_psr2);
> We don't have to generate this subtest if the sink does not support
> PSR2. kms_frontbuffer_tracking generates subtests dynamically, we can
> use the same approach here.
> 
> > +		data.op_psr_mode = PSR_MODE_2;
> >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> >  		test_setup(&data);
> >  		test_cleanup(&data);
> >  	}
> >  
> >  	igt_subtest("no_drrs") {
> > +		data.op_psr_mode = PSR_MODE_1;
> > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > +		test_setup(&data);
> > +		igt_assert(drrs_disabled(&data));
> > +		test_cleanup(&data);
> > +	}
> > +
> > +	igt_subtest("psr2_no_drrs") {
> > +		igt_require(data.supports_psr2);
> > +		data.op_psr_mode = PSR_MODE_2;
> >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> >  		test_setup(&data);
> >  		igt_assert(drrs_disabled(&data));
> > @@ -445,6 +486,17 @@ int main(int argc, char *argv[])
> >  
> >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> >  		igt_subtest_f("primary_%s", op_str(op)) {
> > +			data.op_psr_mode = PSR_MODE_1;
> > +			data.op = op;
> > +			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > +			test_setup(&data);
> > +			run_test(&data);
> > +			test_cleanup(&data);
> > +		}
> > +
> > +		igt_subtest_f("psr2_primary_%s", op_str(op)) {
> > +			igt_require(data.supports_psr2);
> > +			data.op_psr_mode = PSR_MODE_2;
> >  			data.op = op;
> >  			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> >  			test_setup(&data);
> > @@ -455,6 +507,18 @@ int main(int argc, char *argv[])
> >  
> >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> >  		igt_subtest_f("sprite_%s", op_str(op)) {
> > +			igt_require(data.supports_psr2);
> > +			data.op_psr_mode = PSR_MODE_1;
> > +			data.op = op;
> > +			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> > +			test_setup(&data);
> > +			run_test(&data);
> > +			test_cleanup(&data);
> > +		}
> > +
> > +		igt_subtest_f("psr2_sprite_%s", op_str(op)) {
> > +			igt_require(data.supports_psr2);
> > +			data.op_psr_mode = PSR_MODE_2;
> >  			data.op = op;
> >  			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> >  			test_setup(&data);
> > @@ -465,6 +529,17 @@ int main(int argc, char *argv[])
> >  
> >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> >  		igt_subtest_f("cursor_%s", op_str(op)) {
> > +			data.op_psr_mode = PSR_MODE_1;
> > +			data.op = op;
> > +			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > +			test_setup(&data);
> > +			run_test(&data);
> > +			test_cleanup(&data);
> > +		}
> > +
> > +		igt_subtest_f("psr2_cursor_%s", op_str(op)) {
> > +			igt_require(data.supports_psr2);
> > +			data.op_psr_mode = PSR_MODE_2;
> >  			data.op = op;
> >  			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> >  			test_setup(&data);
> > @@ -474,6 +549,18 @@ int main(int argc, char *argv[])
> >  	}
> >  
> >  	igt_subtest_f("dpms") {
> > +		data.op_psr_mode = PSR_MODE_1;
> > +		data.op = RENDER;
> > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > +		test_setup(&data);
> > +		dpms_off_on(&data);
> > +		run_test(&data);
> > +		test_cleanup(&data);
> > +	}
> > +
> > +	igt_subtest_f("psr2_dpms") {
> > +		igt_require(data.supports_psr2);
> > +		data.op_psr_mode = PSR_MODE_2;
> >  		data.op = RENDER;
> >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> >  		test_setup(&data);
> > @@ -483,6 +570,20 @@ int main(int argc, char *argv[])
> >  	}
> >  
> >  	igt_subtest_f("suspend") {
> > +		data.op_psr_mode = PSR_MODE_1;
> > +		data.op = PLANE_ONOFF;
> > +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > +		test_setup(&data);
> > +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
> > +					      SUSPEND_TEST_NONE);
> > +		igt_assert(psr_wait_entry_if_enabled(&data));
> > +		run_test(&data);
> > +		test_cleanup(&data);
> > +	}
> > +
> > +	igt_subtest_f("psr2_suspend") {
> > +		igt_require(data.supports_psr2);
> > +		data.op_psr_mode = PSR_MODE_2;
> >  		data.op = PLANE_ONOFF;
> >  		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> >  		test_setup(&data);

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [igt-dev] [PATCH i-g-t 10/10] tests/psr: Add the same test coverage that we have for PSR1 to PSR2

[Dhinakaran Pandiyan]

On Thu, 2019-01-17 at 13:43 -0800, Souza, Jose wrote:
> On Tue, 2019-01-15 at 22:44 -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > The main tests for PSR1 check if hardware tracking is detecting
> > > changes in planes when modifing it in different ways and now
> > > those tests will also run for PSR2 if supported by source and
> > > sink.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  tests/kms_psr.c | 119
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > > ----
> > >  1 file changed, 110 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > index 8f2ef89c..b2202cd3 100644
> > > --- a/tests/kms_psr.c
> > > +++ b/tests/kms_psr.c
> > > @@ -61,6 +61,7 @@ typedef struct {
> > >  	int debugfs_fd;
> > >  	enum operations op;
> > >  	int test_plane_id;
> > > +	enum psr_mode op_psr_mode;
> > >  	uint32_t devid;
> > >  	uint32_t crtc_id;
> > >  	igt_display_t display;
> > > @@ -72,6 +73,7 @@ typedef struct {
> > >  	drmModeModeInfo *mode;
> > >  	igt_output_t *output;
> > >  	bool with_psr_disabled;
> > > +	bool supports_psr2;
> > >  } data_t;
> > >  
> > >  static void create_cursor_fb(data_t *data)
> > > @@ -190,10 +192,10 @@ static void fill_render(data_t *data,
> > > uint32_t
> > > handle, unsigned char color)
> > >  	gem_bo_busy(data->drm_fd, handle);
> > >  }
> > >  
> > > -static bool sink_support(data_t *data)
> > > +static bool sink_support(data_t *data, enum psr_mode mode)
> > >  {
> > >  	return data->with_psr_disabled ||
> > > -	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> > > +	       psr_sink_support(data->debugfs_fd, mode);
> > >  }
> > >  
> > >  static bool psr_wait_entry_if_enabled(data_t *data)
> > > @@ -201,7 +203,23 @@ static bool psr_wait_entry_if_enabled(data_t
> > > *data)
> > >  	if (data->with_psr_disabled)
> > >  		return true;
> > >  
> > > -	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> > > +	return psr_wait_entry(data->debugfs_fd, data->op_psr_mode);
> > > +}
> > > +
> > > +static bool psr_wait_update_if_enabled(data_t *data)
> > > +{
> > > +	if (data->with_psr_disabled)
> > > +		return true;
> > > +
> > > +	return psr_wait_update(data->debugfs_fd, data->op_psr_mode);
> > > +}
> > > +
> > > +static bool psr_enable_if_enabled(data_t *data)
> > 
> > This function name is quite confusing. What do you think of using
> > macros like this
> > 
> > #define PSR_WAIT_ENTRY(data, mode)
> > 	return data->with_psr_disabled || psr_wait_entry(data-
> > > debugfs_fd, mode);
> > 
> > ...
> 
> 
> I'm happy with both approaches but I guess we can change that in a
> patch on top of this one.
> 
> > 
> > 
> > > +{
> > > +	if (data->with_psr_disabled)
> > > +		return true;
> > > +
> > > +	return psr_enable(data->debugfs_fd, data->op_psr_mode);
> > >  }
> > >  
> > >  static inline void manual(const char *expected)
> > > @@ -291,7 +309,7 @@ static void run_test(data_t *data)
> > >  		expected = "screen GREEN";
> > >  		break;
> > >  	}
> > > -	igt_assert(psr_wait_update(data->debugfs_fd, PSR_MODE_1));
> > > +	igt_assert(psr_wait_update_if_enabled(data));
> > >  	manual(expected);
> > >  }
> > >  
> > > @@ -369,6 +387,7 @@ static void setup_test_plane(data_t *data,
> > > int
> > > test_plane)
> > >  
> > >  static void test_setup(data_t *data)
> > >  {
> > > +	psr_enable_if_enabled(data);
> > >  	setup_test_plane(data, data->test_plane_id);
> > >  	igt_assert(psr_wait_entry_if_enabled(data));
> > >  }
> > > @@ -399,7 +418,7 @@ static int opt_handler(int opt, int
> > > opt_index,
> > > void *_data)
> > >  int main(int argc, char *argv[])
> > >  {
> > >  	const char *help_str =
> > > -	       "  --no-psr\tRun test without PSR.";
> > > +	       "  --no-psr\tRun test without PSR/PSR2.";
> > >  	static struct option long_options[] = {
> > >  		{"no-psr", 0, 0, 'n'},
> > >  		{ 0, 0, 0, 0 }
> > > @@ -417,12 +436,15 @@ int main(int argc, char *argv[])
> > >  		kmstest_set_vt_graphics_mode();
> > >  		data.devid = intel_get_drm_devid(data.drm_fd);
> > >  
> > > -		if (!data.with_psr_disabled)
> > > -			psr_enable(data.debugfs_fd, PSR_MODE_1);
> > > -
> > > -		igt_require_f(sink_support(&data),
> > > +		igt_require_f(sink_support(&data, PSR_MODE_1),
> > >  			      "Sink does not support PSR\n");
> > >  
> > > +		if (sink_support(&data, PSR_MODE_2)) {
> > > +			data.op_psr_mode = PSR_MODE_2;
> > > +			psr_enable_if_enabled(&data);
> > 
> > There is another enable in test_setup() that the PSR2 subtests
> > call.
> > Why do we need two?
> 
> Here I'm enabling by hand to check if source and sink support PSR2 so
> data.supports_psr2 is set with the right value and we skip the PSR2
> tests when not supported.

How do you distinguish that from a true failure where PSR2 should have
got enabled but it doesn't?
> 
> 
> > 
> > > +			data.supports_psr2 =
> > > psr_wait_entry_if_enabled(&data);
> > > +		}
> > > +
> > >  		data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd,
> > > 4096);
> > >  		igt_assert(data.bufmgr);
> > >  		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > > @@ -431,12 +453,31 @@ int main(int argc, char *argv[])
> > >  	}
> > >  
> > >  	igt_subtest("basic") {
> > > +		data.op_psr_mode = PSR_MODE_1;
> > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > +		test_setup(&data);
> > > +		test_cleanup(&data);
> > > +	}
> > 
> > Run this in a loop (PSR_MODE_1...PSR_MODE_2) instead of repeating
> > the
> > same calls? 
> 
> I think have separated tests is better because CI reports will show
> what version of PSR has a regression, we can run specific tests while
> debuging...


Calling igt_subtest() in a loop should generate separate subtests iiuc.

> 
> > 
> > > +
> > > +	igt_subtest("psr2_basic") {
> > > +		igt_require(data.supports_psr2);
> > 
> > We don't have to generate this subtest if the sink does not support
> > PSR2. kms_frontbuffer_tracking generates subtests dynamically, we
> > can
> > use the same approach here.
> > 
> > > +		data.op_psr_mode = PSR_MODE_2;
> > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > >  		test_setup(&data);
> > >  		test_cleanup(&data);
> > >  	}
> > >  
> > >  	igt_subtest("no_drrs") {
> > > +		data.op_psr_mode = PSR_MODE_1;
> > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > +		test_setup(&data);
> > > +		igt_assert(drrs_disabled(&data));
> > > +		test_cleanup(&data);
> > > +	}
> > > +
> > > +	igt_subtest("psr2_no_drrs") {
> > > +		igt_require(data.supports_psr2);
> > > +		data.op_psr_mode = PSR_MODE_2;
> > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > >  		test_setup(&data);
> > >  		igt_assert(drrs_disabled(&data));
> > > @@ -445,6 +486,17 @@ int main(int argc, char *argv[])
> > >  
> > >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> > >  		igt_subtest_f("primary_%s", op_str(op)) {
> > > +			data.op_psr_mode = PSR_MODE_1;
> > > +			data.op = op;
> > > +			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > +			test_setup(&data);
> > > +			run_test(&data);
> > > +			test_cleanup(&data);
> > > +		}
> > > +
> > > +		igt_subtest_f("psr2_primary_%s", op_str(op)) {
> > > +			igt_require(data.supports_psr2);
> > > +			data.op_psr_mode = PSR_MODE_2;
> > >  			data.op = op;
> > >  			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > >  			test_setup(&data);
> > > @@ -455,6 +507,18 @@ int main(int argc, char *argv[])
> > >  
> > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > >  		igt_subtest_f("sprite_%s", op_str(op)) {
> > > +			igt_require(data.supports_psr2);
> > > +			data.op_psr_mode = PSR_MODE_1;
> > > +			data.op = op;
> > > +			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> > > +			test_setup(&data);
> > > +			run_test(&data);
> > > +			test_cleanup(&data);
> > > +		}
> > > +
> > > +		igt_subtest_f("psr2_sprite_%s", op_str(op)) {
> > > +			igt_require(data.supports_psr2);
> > > +			data.op_psr_mode = PSR_MODE_2;
> > >  			data.op = op;
> > >  			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> > >  			test_setup(&data);
> > > @@ -465,6 +529,17 @@ int main(int argc, char *argv[])
> > >  
> > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > >  		igt_subtest_f("cursor_%s", op_str(op)) {
> > > +			data.op_psr_mode = PSR_MODE_1;
> > > +			data.op = op;
> > > +			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > +			test_setup(&data);
> > > +			run_test(&data);
> > > +			test_cleanup(&data);
> > > +		}
> > > +
> > > +		igt_subtest_f("psr2_cursor_%s", op_str(op)) {
> > > +			igt_require(data.supports_psr2);
> > > +			data.op_psr_mode = PSR_MODE_2;
> > >  			data.op = op;
> > >  			data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > >  			test_setup(&data);
> > > @@ -474,6 +549,18 @@ int main(int argc, char *argv[])
> > >  	}
> > >  
> > >  	igt_subtest_f("dpms") {
> > > +		data.op_psr_mode = PSR_MODE_1;
> > > +		data.op = RENDER;
> > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > +		test_setup(&data);
> > > +		dpms_off_on(&data);
> > > +		run_test(&data);
> > > +		test_cleanup(&data);
> > > +	}
> > > +
> > > +	igt_subtest_f("psr2_dpms") {
> > > +		igt_require(data.supports_psr2);
> > > +		data.op_psr_mode = PSR_MODE_2;
> > >  		data.op = RENDER;
> > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > >  		test_setup(&data);
> > > @@ -483,6 +570,20 @@ int main(int argc, char *argv[])
> > >  	}
> > >  
> > >  	igt_subtest_f("suspend") {
> > > +		data.op_psr_mode = PSR_MODE_1;
> > > +		data.op = PLANE_ONOFF;
> > > +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > +		test_setup(&data);
> > > +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
> > > +					      SUSPEND_TEST_NONE);
> > > +		igt_assert(psr_wait_entry_if_enabled(&data));
> > > +		run_test(&data);
> > > +		test_cleanup(&data);
> > > +	}
> > > +
> > > +	igt_subtest_f("psr2_suspend") {
> > > +		igt_require(data.supports_psr2);
> > > +		data.op_psr_mode = PSR_MODE_2;
> > >  		data.op = PLANE_ONOFF;
> > >  		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > >  		test_setup(&data);

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

Re: [igt-dev] [PATCH i-g-t 10/10] tests/psr: Add the same test coverage that we have for PSR1 to PSR2

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 11600 bytes --]

On Thu, 2019-01-17 at 13:51 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-01-17 at 13:43 -0800, Souza, Jose wrote:
> > On Tue, 2019-01-15 at 22:44 -0800, Dhinakaran Pandiyan wrote:
> > > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > > The main tests for PSR1 check if hardware tracking is detecting
> > > > changes in planes when modifing it in different ways and now
> > > > those tests will also run for PSR2 if supported by source and
> > > > sink.
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  tests/kms_psr.c | 119
> > > > ++++++++++++++++++++++++++++++++++++++++++++
> > > > ----
> > > >  1 file changed, 110 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > > index 8f2ef89c..b2202cd3 100644
> > > > --- a/tests/kms_psr.c
> > > > +++ b/tests/kms_psr.c
> > > > @@ -61,6 +61,7 @@ typedef struct {
> > > >  	int debugfs_fd;
> > > >  	enum operations op;
> > > >  	int test_plane_id;
> > > > +	enum psr_mode op_psr_mode;
> > > >  	uint32_t devid;
> > > >  	uint32_t crtc_id;
> > > >  	igt_display_t display;
> > > > @@ -72,6 +73,7 @@ typedef struct {
> > > >  	drmModeModeInfo *mode;
> > > >  	igt_output_t *output;
> > > >  	bool with_psr_disabled;
> > > > +	bool supports_psr2;
> > > >  } data_t;
> > > >  
> > > >  static void create_cursor_fb(data_t *data)
> > > > @@ -190,10 +192,10 @@ static void fill_render(data_t *data,
> > > > uint32_t
> > > > handle, unsigned char color)
> > > >  	gem_bo_busy(data->drm_fd, handle);
> > > >  }
> > > >  
> > > > -static bool sink_support(data_t *data)
> > > > +static bool sink_support(data_t *data, enum psr_mode mode)
> > > >  {
> > > >  	return data->with_psr_disabled ||
> > > > -	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> > > > +	       psr_sink_support(data->debugfs_fd, mode);
> > > >  }
> > > >  
> > > >  static bool psr_wait_entry_if_enabled(data_t *data)
> > > > @@ -201,7 +203,23 @@ static bool
> > > > psr_wait_entry_if_enabled(data_t
> > > > *data)
> > > >  	if (data->with_psr_disabled)
> > > >  		return true;
> > > >  
> > > > -	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> > > > +	return psr_wait_entry(data->debugfs_fd, data-
> > > > >op_psr_mode);
> > > > +}
> > > > +
> > > > +static bool psr_wait_update_if_enabled(data_t *data)
> > > > +{
> > > > +	if (data->with_psr_disabled)
> > > > +		return true;
> > > > +
> > > > +	return psr_wait_update(data->debugfs_fd, data-
> > > > >op_psr_mode);
> > > > +}
> > > > +
> > > > +static bool psr_enable_if_enabled(data_t *data)
> > > 
> > > This function name is quite confusing. What do you think of using
> > > macros like this
> > > 
> > > #define PSR_WAIT_ENTRY(data, mode)
> > > 	return data->with_psr_disabled || psr_wait_entry(data-
> > > > debugfs_fd, mode);
> > > 
> > > ...
> > 
> > I'm happy with both approaches but I guess we can change that in a
> > patch on top of this one.
> > 
> > > 
> > > > +{
> > > > +	if (data->with_psr_disabled)
> > > > +		return true;
> > > > +
> > > > +	return psr_enable(data->debugfs_fd, data->op_psr_mode);
> > > >  }
> > > >  
> > > >  static inline void manual(const char *expected)
> > > > @@ -291,7 +309,7 @@ static void run_test(data_t *data)
> > > >  		expected = "screen GREEN";
> > > >  		break;
> > > >  	}
> > > > -	igt_assert(psr_wait_update(data->debugfs_fd,
> > > > PSR_MODE_1));
> > > > +	igt_assert(psr_wait_update_if_enabled(data));
> > > >  	manual(expected);
> > > >  }
> > > >  
> > > > @@ -369,6 +387,7 @@ static void setup_test_plane(data_t *data,
> > > > int
> > > > test_plane)
> > > >  
> > > >  static void test_setup(data_t *data)
> > > >  {
> > > > +	psr_enable_if_enabled(data);
> > > >  	setup_test_plane(data, data->test_plane_id);
> > > >  	igt_assert(psr_wait_entry_if_enabled(data));
> > > >  }
> > > > @@ -399,7 +418,7 @@ static int opt_handler(int opt, int
> > > > opt_index,
> > > > void *_data)
> > > >  int main(int argc, char *argv[])
> > > >  {
> > > >  	const char *help_str =
> > > > -	       "  --no-psr\tRun test without PSR.";
> > > > +	       "  --no-psr\tRun test without PSR/PSR2.";
> > > >  	static struct option long_options[] = {
> > > >  		{"no-psr", 0, 0, 'n'},
> > > >  		{ 0, 0, 0, 0 }
> > > > @@ -417,12 +436,15 @@ int main(int argc, char *argv[])
> > > >  		kmstest_set_vt_graphics_mode();
> > > >  		data.devid = intel_get_drm_devid(data.drm_fd);
> > > >  
> > > > -		if (!data.with_psr_disabled)
> > > > -			psr_enable(data.debugfs_fd,
> > > > PSR_MODE_1);
> > > > -
> > > > -		igt_require_f(sink_support(&data),
> > > > +		igt_require_f(sink_support(&data, PSR_MODE_1),
> > > >  			      "Sink does not support PSR\n");
> > > >  
> > > > +		if (sink_support(&data, PSR_MODE_2)) {
> > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > +			psr_enable_if_enabled(&data);
> > > 
> > > There is another enable in test_setup() that the PSR2 subtests
> > > call.
> > > Why do we need two?
> > 
> > Here I'm enabling by hand to check if source and sink support PSR2
> > so
> > data.supports_psr2 is set with the right value and we skip the PSR2
> > tests when not supported.
> 
> How do you distinguish that from a true failure where PSR2 should
> have
> got enabled but it doesn't?

We can't distinguish but after the first CI reports if a machine that
had PSR2 tests running suddenly start to skip PSR2 tests it will be
reported by CI.

> > 
> > > > +			data.supports_psr2 =
> > > > psr_wait_entry_if_enabled(&data);
> > > > +		}
> > > > +
> > > >  		data.bufmgr =
> > > > drm_intel_bufmgr_gem_init(data.drm_fd,
> > > > 4096);
> > > >  		igt_assert(data.bufmgr);
> > > >  		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > > > @@ -431,12 +453,31 @@ int main(int argc, char *argv[])
> > > >  	}
> > > >  
> > > >  	igt_subtest("basic") {
> > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > +		test_setup(&data);
> > > > +		test_cleanup(&data);
> > > > +	}
> > > 
> > > Run this in a loop (PSR_MODE_1...PSR_MODE_2) instead of repeating
> > > the
> > > same calls? 
> > 
> > I think have separated tests is better because CI reports will show
> > what version of PSR has a regression, we can run specific tests
> > while
> > debuging...
> 
> Calling igt_subtest() in a loop should generate separate subtests
> iiuc.

Ohh now I got what you want to do, looks good I will change to this.

But we will need to generate PSR2 tests for every machine, as the
igt_fixture() do not run when getting the list of subtests and CI
probably uses that to split the tests through the shards.


> 
> > > > +
> > > > +	igt_subtest("psr2_basic") {
> > > > +		igt_require(data.supports_psr2);
> > > 
> > > We don't have to generate this subtest if the sink does not
> > > support
> > > PSR2. kms_frontbuffer_tracking generates subtests dynamically, we
> > > can
> > > use the same approach here.
> > > 
> > > > +		data.op_psr_mode = PSR_MODE_2;
> > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > >  		test_setup(&data);
> > > >  		test_cleanup(&data);
> > > >  	}
> > > >  
> > > >  	igt_subtest("no_drrs") {
> > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > +		test_setup(&data);
> > > > +		igt_assert(drrs_disabled(&data));
> > > > +		test_cleanup(&data);
> > > > +	}
> > > > +
> > > > +	igt_subtest("psr2_no_drrs") {
> > > > +		igt_require(data.supports_psr2);
> > > > +		data.op_psr_mode = PSR_MODE_2;
> > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > >  		test_setup(&data);
> > > >  		igt_assert(drrs_disabled(&data));
> > > > @@ -445,6 +486,17 @@ int main(int argc, char *argv[])
> > > >  
> > > >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> > > >  		igt_subtest_f("primary_%s", op_str(op)) {
> > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > +			data.op = op;
> > > > +			data.test_plane_id =
> > > > DRM_PLANE_TYPE_PRIMARY;
> > > > +			test_setup(&data);
> > > > +			run_test(&data);
> > > > +			test_cleanup(&data);
> > > > +		}
> > > > +
> > > > +		igt_subtest_f("psr2_primary_%s", op_str(op)) {
> > > > +			igt_require(data.supports_psr2);
> > > > +			data.op_psr_mode = PSR_MODE_2;
> > > >  			data.op = op;
> > > >  			data.test_plane_id =
> > > > DRM_PLANE_TYPE_PRIMARY;
> > > >  			test_setup(&data);
> > > > @@ -455,6 +507,18 @@ int main(int argc, char *argv[])
> > > >  
> > > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > > >  		igt_subtest_f("sprite_%s", op_str(op)) {
> > > > +			igt_require(data.supports_psr2);
> > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > +			data.op = op;
> > > > +			data.test_plane_id =
> > > > DRM_PLANE_TYPE_OVERLAY;
> > > > +			test_setup(&data);
> > > > +			run_test(&data);
> > > > +			test_cleanup(&data);
> > > > +		}
> > > > +
> > > > +		igt_subtest_f("psr2_sprite_%s", op_str(op)) {
> > > > +			igt_require(data.supports_psr2);
> > > > +			data.op_psr_mode = PSR_MODE_2;
> > > >  			data.op = op;
> > > >  			data.test_plane_id =
> > > > DRM_PLANE_TYPE_OVERLAY;
> > > >  			test_setup(&data);
> > > > @@ -465,6 +529,17 @@ int main(int argc, char *argv[])
> > > >  
> > > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > > >  		igt_subtest_f("cursor_%s", op_str(op)) {
> > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > +			data.op = op;
> > > > +			data.test_plane_id =
> > > > DRM_PLANE_TYPE_CURSOR;
> > > > +			test_setup(&data);
> > > > +			run_test(&data);
> > > > +			test_cleanup(&data);
> > > > +		}
> > > > +
> > > > +		igt_subtest_f("psr2_cursor_%s", op_str(op)) {
> > > > +			igt_require(data.supports_psr2);
> > > > +			data.op_psr_mode = PSR_MODE_2;
> > > >  			data.op = op;
> > > >  			data.test_plane_id =
> > > > DRM_PLANE_TYPE_CURSOR;
> > > >  			test_setup(&data);
> > > > @@ -474,6 +549,18 @@ int main(int argc, char *argv[])
> > > >  	}
> > > >  
> > > >  	igt_subtest_f("dpms") {
> > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > +		data.op = RENDER;
> > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > +		test_setup(&data);
> > > > +		dpms_off_on(&data);
> > > > +		run_test(&data);
> > > > +		test_cleanup(&data);
> > > > +	}
> > > > +
> > > > +	igt_subtest_f("psr2_dpms") {
> > > > +		igt_require(data.supports_psr2);
> > > > +		data.op_psr_mode = PSR_MODE_2;
> > > >  		data.op = RENDER;
> > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > >  		test_setup(&data);
> > > > @@ -483,6 +570,20 @@ int main(int argc, char *argv[])
> > > >  	}
> > > >  
> > > >  	igt_subtest_f("suspend") {
> > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > +		data.op = PLANE_ONOFF;
> > > > +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > > +		test_setup(&data);
> > > > +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM
> > > > ,
> > > > +					      SUSPEND_TEST_NONE
> > > > );
> > > > +		igt_assert(psr_wait_entry_if_enabled(&data));
> > > > +		run_test(&data);
> > > > +		test_cleanup(&data);
> > > > +	}
> > > > +
> > > > +	igt_subtest_f("psr2_suspend") {
> > > > +		igt_require(data.supports_psr2);
> > > > +		data.op_psr_mode = PSR_MODE_2;
> > > >  		data.op = PLANE_ONOFF;
> > > >  		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > >  		test_setup(&data);

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [igt-dev] [PATCH i-g-t 10/10] tests/psr: Add the same test coverage that we have for PSR1 to PSR2

[Pandiyan, Dhinakaran]

On Thu, 2019-01-17 at 15:05 -0800, Souza, Jose wrote:
> On Thu, 2019-01-17 at 13:51 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2019-01-17 at 13:43 -0800, Souza, Jose wrote:
> > > On Tue, 2019-01-15 at 22:44 -0800, Dhinakaran Pandiyan wrote:
> > > > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza wrote:
> > > > > The main tests for PSR1 check if hardware tracking is
> > > > > detecting
> > > > > changes in planes when modifing it in different ways and now
> > > > > those tests will also run for PSR2 if supported by source and
> > > > > sink.
> > > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  tests/kms_psr.c | 119
> > > > > ++++++++++++++++++++++++++++++++++++++++++++
> > > > > ----
> > > > >  1 file changed, 110 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > > > index 8f2ef89c..b2202cd3 100644
> > > > > --- a/tests/kms_psr.c
> > > > > +++ b/tests/kms_psr.c
> > > > > @@ -61,6 +61,7 @@ typedef struct {
> > > > >  	int debugfs_fd;
> > > > >  	enum operations op;
> > > > >  	int test_plane_id;
> > > > > +	enum psr_mode op_psr_mode;
> > > > >  	uint32_t devid;
> > > > >  	uint32_t crtc_id;
> > > > >  	igt_display_t display;
> > > > > @@ -72,6 +73,7 @@ typedef struct {
> > > > >  	drmModeModeInfo *mode;
> > > > >  	igt_output_t *output;
> > > > >  	bool with_psr_disabled;
> > > > > +	bool supports_psr2;
> > > > >  } data_t;
> > > > >  
> > > > >  static void create_cursor_fb(data_t *data)
> > > > > @@ -190,10 +192,10 @@ static void fill_render(data_t *data,
> > > > > uint32_t
> > > > > handle, unsigned char color)
> > > > >  	gem_bo_busy(data->drm_fd, handle);
> > > > >  }
> > > > >  
> > > > > -static bool sink_support(data_t *data)
> > > > > +static bool sink_support(data_t *data, enum psr_mode mode)
> > > > >  {
> > > > >  	return data->with_psr_disabled ||
> > > > > -	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> > > > > +	       psr_sink_support(data->debugfs_fd, mode);
> > > > >  }
> > > > >  
> > > > >  static bool psr_wait_entry_if_enabled(data_t *data)
> > > > > @@ -201,7 +203,23 @@ static bool
> > > > > psr_wait_entry_if_enabled(data_t
> > > > > *data)
> > > > >  	if (data->with_psr_disabled)
> > > > >  		return true;
> > > > >  
> > > > > -	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> > > > > +	return psr_wait_entry(data->debugfs_fd, data-
> > > > > > op_psr_mode);
> > > > > 
> > > > > +}
> > > > > +
> > > > > +static bool psr_wait_update_if_enabled(data_t *data)
> > > > > +{
> > > > > +	if (data->with_psr_disabled)
> > > > > +		return true;
> > > > > +
> > > > > +	return psr_wait_update(data->debugfs_fd, data-
> > > > > > op_psr_mode);
> > > > > 
> > > > > +}
> > > > > +
> > > > > +static bool psr_enable_if_enabled(data_t *data)
> > > > 
> > > > This function name is quite confusing. What do you think of
> > > > using
> > > > macros like this
> > > > 
> > > > #define PSR_WAIT_ENTRY(data, mode)
> > > > 	return data->with_psr_disabled || psr_wait_entry(data-
> > > > > debugfs_fd, mode);
> > > > 
> > > > ...
> > > 
> > > I'm happy with both approaches but I guess we can change that in
> > > a
> > > patch on top of this one.
> > > 
> > > > 
> > > > > +{
> > > > > +	if (data->with_psr_disabled)
> > > > > +		return true;
> > > > > +
> > > > > +	return psr_enable(data->debugfs_fd, data->op_psr_mode);
> > > > >  }
> > > > >  
> > > > >  static inline void manual(const char *expected)
> > > > > @@ -291,7 +309,7 @@ static void run_test(data_t *data)
> > > > >  		expected = "screen GREEN";
> > > > >  		break;
> > > > >  	}
> > > > > -	igt_assert(psr_wait_update(data->debugfs_fd,
> > > > > PSR_MODE_1));
> > > > > +	igt_assert(psr_wait_update_if_enabled(data));
> > > > >  	manual(expected);
> > > > >  }
> > > > >  
> > > > > @@ -369,6 +387,7 @@ static void setup_test_plane(data_t
> > > > > *data,
> > > > > int
> > > > > test_plane)
> > > > >  
> > > > >  static void test_setup(data_t *data)
> > > > >  {
> > > > > +	psr_enable_if_enabled(data);
> > > > >  	setup_test_plane(data, data->test_plane_id);
> > > > >  	igt_assert(psr_wait_entry_if_enabled(data));
> > > > >  }
> > > > > @@ -399,7 +418,7 @@ static int opt_handler(int opt, int
> > > > > opt_index,
> > > > > void *_data)
> > > > >  int main(int argc, char *argv[])
> > > > >  {
> > > > >  	const char *help_str =
> > > > > -	       "  --no-psr\tRun test without PSR.";
> > > > > +	       "  --no-psr\tRun test without PSR/PSR2.";
> > > > >  	static struct option long_options[] = {
> > > > >  		{"no-psr", 0, 0, 'n'},
> > > > >  		{ 0, 0, 0, 0 }
> > > > > @@ -417,12 +436,15 @@ int main(int argc, char *argv[])
> > > > >  		kmstest_set_vt_graphics_mode();
> > > > >  		data.devid = intel_get_drm_devid(data.drm_fd);
> > > > >  
> > > > > -		if (!data.with_psr_disabled)
> > > > > -			psr_enable(data.debugfs_fd,
> > > > > PSR_MODE_1);
> > > > > -
> > > > > -		igt_require_f(sink_support(&data),
> > > > > +		igt_require_f(sink_support(&data, PSR_MODE_1),
> > > > >  			      "Sink does not support PSR\n");
> > > > >  
> > > > > +		if (sink_support(&data, PSR_MODE_2)) {
> > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > > +			psr_enable_if_enabled(&data);
> > > > 
> > > > There is another enable in test_setup() that the PSR2 subtests
> > > > call.
> > > > Why do we need two?
> > > 
> > > Here I'm enabling by hand to check if source and sink support
> > > PSR2
> > > so
> > > data.supports_psr2 is set with the right value and we skip the
> > > PSR2
> > > tests when not supported.
> > 
> > How do you distinguish that from a true failure where PSR2 should
> > have
> > got enabled but it doesn't?
> 
> We can't distinguish but after the first CI reports if a machine that
> had PSR2 tests running suddenly start to skip PSR2 tests it will be
> reported by CI.
> 
Remove this code and let the test fail instead, easier to notice.

> > > 
> > > > > +			data.supports_psr2 =
> > > > > psr_wait_entry_if_enabled(&data);
> > > > > +		}
> > > > > +
> > > > >  		data.bufmgr =
> > > > > drm_intel_bufmgr_gem_init(data.drm_fd,
> > > > > 4096);
> > > > >  		igt_assert(data.bufmgr);
> > > > >  		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > > > > @@ -431,12 +453,31 @@ int main(int argc, char *argv[])
> > > > >  	}
> > > > >  
> > > > >  	igt_subtest("basic") {
> > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > +		test_setup(&data);
> > > > > +		test_cleanup(&data);
> > > > > +	}
> > > > 
> > > > Run this in a loop (PSR_MODE_1...PSR_MODE_2) instead of
> > > > repeating
> > > > the
> > > > same calls? 
> > > 
> > > I think have separated tests is better because CI reports will
> > > show
> > > what version of PSR has a regression, we can run specific tests
> > > while
> > > debuging...
> > 
> > Calling igt_subtest() in a loop should generate separate subtests
> > iiuc.
> 
> Ohh now I got what you want to do, looks good I will change to this.
> 
> But we will need to generate PSR2 tests for every machine, as the
> igt_fixture() do not run when getting the list of subtests and CI
> probably uses that to split the tests through the shards.
> 
> 
> > 
> > > > > +
> > > > > +	igt_subtest("psr2_basic") {
> > > > > +		igt_require(data.supports_psr2);
> > > > 
> > > > We don't have to generate this subtest if the sink does not
> > > > support
> > > > PSR2. kms_frontbuffer_tracking generates subtests dynamically,
> > > > we
> > > > can
> > > > use the same approach here.
> > > > 
> > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > >  		test_setup(&data);
> > > > >  		test_cleanup(&data);
> > > > >  	}
> > > > >  
> > > > >  	igt_subtest("no_drrs") {
> > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > +		test_setup(&data);
> > > > > +		igt_assert(drrs_disabled(&data));
> > > > > +		test_cleanup(&data);
> > > > > +	}
> > > > > +
> > > > > +	igt_subtest("psr2_no_drrs") {
> > > > > +		igt_require(data.supports_psr2);
> > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > >  		test_setup(&data);
> > > > >  		igt_assert(drrs_disabled(&data));
> > > > > @@ -445,6 +486,17 @@ int main(int argc, char *argv[])
> > > > >  
> > > > >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> > > > >  		igt_subtest_f("primary_%s", op_str(op)) {
> > > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > > +			data.op = op;
> > > > > +			data.test_plane_id =
> > > > > DRM_PLANE_TYPE_PRIMARY;
> > > > > +			test_setup(&data);
> > > > > +			run_test(&data);
> > > > > +			test_cleanup(&data);
> > > > > +		}
> > > > > +
> > > > > +		igt_subtest_f("psr2_primary_%s", op_str(op)) {
> > > > > +			igt_require(data.supports_psr2);
> > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > >  			data.op = op;
> > > > >  			data.test_plane_id =
> > > > > DRM_PLANE_TYPE_PRIMARY;
> > > > >  			test_setup(&data);
> > > > > @@ -455,6 +507,18 @@ int main(int argc, char *argv[])
> > > > >  
> > > > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > > > >  		igt_subtest_f("sprite_%s", op_str(op)) {
> > > > > +			igt_require(data.supports_psr2);
> > > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > > +			data.op = op;
> > > > > +			data.test_plane_id =
> > > > > DRM_PLANE_TYPE_OVERLAY;
> > > > > +			test_setup(&data);
> > > > > +			run_test(&data);
> > > > > +			test_cleanup(&data);
> > > > > +		}
> > > > > +
> > > > > +		igt_subtest_f("psr2_sprite_%s", op_str(op)) {
> > > > > +			igt_require(data.supports_psr2);
> > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > >  			data.op = op;
> > > > >  			data.test_plane_id =
> > > > > DRM_PLANE_TYPE_OVERLAY;
> > > > >  			test_setup(&data);
> > > > > @@ -465,6 +529,17 @@ int main(int argc, char *argv[])
> > > > >  
> > > > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > > > >  		igt_subtest_f("cursor_%s", op_str(op)) {
> > > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > > +			data.op = op;
> > > > > +			data.test_plane_id =
> > > > > DRM_PLANE_TYPE_CURSOR;
> > > > > +			test_setup(&data);
> > > > > +			run_test(&data);
> > > > > +			test_cleanup(&data);
> > > > > +		}
> > > > > +
> > > > > +		igt_subtest_f("psr2_cursor_%s", op_str(op)) {
> > > > > +			igt_require(data.supports_psr2);
> > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > >  			data.op = op;
> > > > >  			data.test_plane_id =
> > > > > DRM_PLANE_TYPE_CURSOR;
> > > > >  			test_setup(&data);
> > > > > @@ -474,6 +549,18 @@ int main(int argc, char *argv[])
> > > > >  	}
> > > > >  
> > > > >  	igt_subtest_f("dpms") {
> > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > +		data.op = RENDER;
> > > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > +		test_setup(&data);
> > > > > +		dpms_off_on(&data);
> > > > > +		run_test(&data);
> > > > > +		test_cleanup(&data);
> > > > > +	}
> > > > > +
> > > > > +	igt_subtest_f("psr2_dpms") {
> > > > > +		igt_require(data.supports_psr2);
> > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > >  		data.op = RENDER;
> > > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > >  		test_setup(&data);
> > > > > @@ -483,6 +570,20 @@ int main(int argc, char *argv[])
> > > > >  	}
> > > > >  
> > > > >  	igt_subtest_f("suspend") {
> > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > +		data.op = PLANE_ONOFF;
> > > > > +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > > > +		test_setup(&data);
> > > > > +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM
> > > > > ,
> > > > > +					      SUSPEND_TEST_NONE
> > > > > );
> > > > > +		igt_assert(psr_wait_entry_if_enabled(&data));
> > > > > +		run_test(&data);
> > > > > +		test_cleanup(&data);
> > > > > +	}
> > > > > +
> > > > > +	igt_subtest_f("psr2_suspend") {
> > > > > +		igt_require(data.supports_psr2);
> > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > >  		data.op = PLANE_ONOFF;
> > > > >  		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > > >  		test_setup(&data);
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 10/10] tests/psr: Add the same test coverage that we have for PSR1 to PSR2

[Souza, Jose]

[-- Attachment #1.1: Type: text/plain, Size: 13614 bytes --]

On Thu, 2019-01-17 at 15:09 -0800, Pandiyan, Dhinakaran wrote:
> On Thu, 2019-01-17 at 15:05 -0800, Souza, Jose wrote:
> > On Thu, 2019-01-17 at 13:51 -0800, Dhinakaran Pandiyan wrote:
> > > On Thu, 2019-01-17 at 13:43 -0800, Souza, Jose wrote:
> > > > On Tue, 2019-01-15 at 22:44 -0800, Dhinakaran Pandiyan wrote:
> > > > > On Fri, 2019-01-11 at 17:46 -0800, José Roberto de Souza
> > > > > wrote:
> > > > > > The main tests for PSR1 check if hardware tracking is
> > > > > > detecting
> > > > > > changes in planes when modifing it in different ways and
> > > > > > now
> > > > > > those tests will also run for PSR2 if supported by source
> > > > > > and
> > > > > > sink.
> > > > > > 
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > >  tests/kms_psr.c | 119
> > > > > > ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > ----
> > > > > >  1 file changed, 110 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > > > > index 8f2ef89c..b2202cd3 100644
> > > > > > --- a/tests/kms_psr.c
> > > > > > +++ b/tests/kms_psr.c
> > > > > > @@ -61,6 +61,7 @@ typedef struct {
> > > > > >  	int debugfs_fd;
> > > > > >  	enum operations op;
> > > > > >  	int test_plane_id;
> > > > > > +	enum psr_mode op_psr_mode;
> > > > > >  	uint32_t devid;
> > > > > >  	uint32_t crtc_id;
> > > > > >  	igt_display_t display;
> > > > > > @@ -72,6 +73,7 @@ typedef struct {
> > > > > >  	drmModeModeInfo *mode;
> > > > > >  	igt_output_t *output;
> > > > > >  	bool with_psr_disabled;
> > > > > > +	bool supports_psr2;
> > > > > >  } data_t;
> > > > > >  
> > > > > >  static void create_cursor_fb(data_t *data)
> > > > > > @@ -190,10 +192,10 @@ static void fill_render(data_t *data,
> > > > > > uint32_t
> > > > > > handle, unsigned char color)
> > > > > >  	gem_bo_busy(data->drm_fd, handle);
> > > > > >  }
> > > > > >  
> > > > > > -static bool sink_support(data_t *data)
> > > > > > +static bool sink_support(data_t *data, enum psr_mode mode)
> > > > > >  {
> > > > > >  	return data->with_psr_disabled ||
> > > > > > -	       psr_sink_support(data->debugfs_fd, PSR_MODE_1);
> > > > > > +	       psr_sink_support(data->debugfs_fd, mode);
> > > > > >  }
> > > > > >  
> > > > > >  static bool psr_wait_entry_if_enabled(data_t *data)
> > > > > > @@ -201,7 +203,23 @@ static bool
> > > > > > psr_wait_entry_if_enabled(data_t
> > > > > > *data)
> > > > > >  	if (data->with_psr_disabled)
> > > > > >  		return true;
> > > > > >  
> > > > > > -	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1);
> > > > > > +	return psr_wait_entry(data->debugfs_fd, data-
> > > > > > > op_psr_mode);
> > > > > > 
> > > > > > +}
> > > > > > +
> > > > > > +static bool psr_wait_update_if_enabled(data_t *data)
> > > > > > +{
> > > > > > +	if (data->with_psr_disabled)
> > > > > > +		return true;
> > > > > > +
> > > > > > +	return psr_wait_update(data->debugfs_fd, data-
> > > > > > > op_psr_mode);
> > > > > > 
> > > > > > +}
> > > > > > +
> > > > > > +static bool psr_enable_if_enabled(data_t *data)
> > > > > 
> > > > > This function name is quite confusing. What do you think of
> > > > > using
> > > > > macros like this
> > > > > 
> > > > > #define PSR_WAIT_ENTRY(data, mode)
> > > > > 	return data->with_psr_disabled || psr_wait_entry(data-
> > > > > > debugfs_fd, mode);
> > > > > 
> > > > > ...
> > > > 
> > > > I'm happy with both approaches but I guess we can change that
> > > > in
> > > > a
> > > > patch on top of this one.
> > > > 
> > > > > > +{
> > > > > > +	if (data->with_psr_disabled)
> > > > > > +		return true;
> > > > > > +
> > > > > > +	return psr_enable(data->debugfs_fd, data->op_psr_mode);
> > > > > >  }
> > > > > >  
> > > > > >  static inline void manual(const char *expected)
> > > > > > @@ -291,7 +309,7 @@ static void run_test(data_t *data)
> > > > > >  		expected = "screen GREEN";
> > > > > >  		break;
> > > > > >  	}
> > > > > > -	igt_assert(psr_wait_update(data->debugfs_fd,
> > > > > > PSR_MODE_1));
> > > > > > +	igt_assert(psr_wait_update_if_enabled(data));
> > > > > >  	manual(expected);
> > > > > >  }
> > > > > >  
> > > > > > @@ -369,6 +387,7 @@ static void setup_test_plane(data_t
> > > > > > *data,
> > > > > > int
> > > > > > test_plane)
> > > > > >  
> > > > > >  static void test_setup(data_t *data)
> > > > > >  {
> > > > > > +	psr_enable_if_enabled(data);
> > > > > >  	setup_test_plane(data, data->test_plane_id);
> > > > > >  	igt_assert(psr_wait_entry_if_enabled(data));
> > > > > >  }
> > > > > > @@ -399,7 +418,7 @@ static int opt_handler(int opt, int
> > > > > > opt_index,
> > > > > > void *_data)
> > > > > >  int main(int argc, char *argv[])
> > > > > >  {
> > > > > >  	const char *help_str =
> > > > > > -	       "  --no-psr\tRun test without PSR.";
> > > > > > +	       "  --no-psr\tRun test without PSR/PSR2.";
> > > > > >  	static struct option long_options[] = {
> > > > > >  		{"no-psr", 0, 0, 'n'},
> > > > > >  		{ 0, 0, 0, 0 }
> > > > > > @@ -417,12 +436,15 @@ int main(int argc, char *argv[])
> > > > > >  		kmstest_set_vt_graphics_mode();
> > > > > >  		data.devid = intel_get_drm_devid(data.drm_fd);
> > > > > >  
> > > > > > -		if (!data.with_psr_disabled)
> > > > > > -			psr_enable(data.debugfs_fd,
> > > > > > PSR_MODE_1);
> > > > > > -
> > > > > > -		igt_require_f(sink_support(&data),
> > > > > > +		igt_require_f(sink_support(&data, PSR_MODE_1),
> > > > > >  			      "Sink does not support PSR\n");
> > > > > >  
> > > > > > +		if (sink_support(&data, PSR_MODE_2)) {
> > > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > > > +			psr_enable_if_enabled(&data);
> > > > > 
> > > > > There is another enable in test_setup() that the PSR2
> > > > > subtests
> > > > > call.
> > > > > Why do we need two?
> > > > 
> > > > Here I'm enabling by hand to check if source and sink support
> > > > PSR2
> > > > so
> > > > data.supports_psr2 is set with the right value and we skip the
> > > > PSR2
> > > > tests when not supported.
> > > 
> > > How do you distinguish that from a true failure where PSR2 should
> > > have
> > > got enabled but it doesn't?
> > 
> > We can't distinguish but after the first CI reports if a machine
> > that
> > had PSR2 tests running suddenly start to skip PSR2 tests it will be
> > reported by CI.
> > 
> Remove this code and let the test fail instead, easier to notice.

Okay, I will change to:

if (intel_gen(intel_get_drm_devid(data.drm_fd)) >= 9)
	data.supports_psr2 = sink_support(&data, PSR_MODE_2);

Or would be better add this gen check inside of sink_support()?
This is for a case where a PSR2 panels is placed in a machine with with
a gen that do not support PSR2.

> 
> > > > > > +			data.supports_psr2 =
> > > > > > psr_wait_entry_if_enabled(&data);
> > > > > > +		}
> > > > > > +
> > > > > >  		data.bufmgr =
> > > > > > drm_intel_bufmgr_gem_init(data.drm_fd,
> > > > > > 4096);
> > > > > >  		igt_assert(data.bufmgr);
> > > > > >  		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > > > > > @@ -431,12 +453,31 @@ int main(int argc, char *argv[])
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_subtest("basic") {
> > > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > > +		test_setup(&data);
> > > > > > +		test_cleanup(&data);
> > > > > > +	}
> > > > > 
> > > > > Run this in a loop (PSR_MODE_1...PSR_MODE_2) instead of
> > > > > repeating
> > > > > the
> > > > > same calls? 
> > > > 
> > > > I think have separated tests is better because CI reports will
> > > > show
> > > > what version of PSR has a regression, we can run specific tests
> > > > while
> > > > debuging...
> > > 
> > > Calling igt_subtest() in a loop should generate separate subtests
> > > iiuc.
> > 
> > Ohh now I got what you want to do, looks good I will change to
> > this.
> > 
> > But we will need to generate PSR2 tests for every machine, as the
> > igt_fixture() do not run when getting the list of subtests and CI
> > probably uses that to split the tests through the shards.
> > 
> > 
> > > > > > +
> > > > > > +	igt_subtest("psr2_basic") {
> > > > > > +		igt_require(data.supports_psr2);
> > > > > 
> > > > > We don't have to generate this subtest if the sink does not
> > > > > support
> > > > > PSR2. kms_frontbuffer_tracking generates subtests
> > > > > dynamically,
> > > > > we
> > > > > can
> > > > > use the same approach here.
> > > > > 
> > > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > >  		test_setup(&data);
> > > > > >  		test_cleanup(&data);
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_subtest("no_drrs") {
> > > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > > +		test_setup(&data);
> > > > > > +		igt_assert(drrs_disabled(&data));
> > > > > > +		test_cleanup(&data);
> > > > > > +	}
> > > > > > +
> > > > > > +	igt_subtest("psr2_no_drrs") {
> > > > > > +		igt_require(data.supports_psr2);
> > > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > >  		test_setup(&data);
> > > > > >  		igt_assert(drrs_disabled(&data));
> > > > > > @@ -445,6 +486,17 @@ int main(int argc, char *argv[])
> > > > > >  
> > > > > >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> > > > > >  		igt_subtest_f("primary_%s", op_str(op)) {
> > > > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > > > +			data.op = op;
> > > > > > +			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_PRIMARY;
> > > > > > +			test_setup(&data);
> > > > > > +			run_test(&data);
> > > > > > +			test_cleanup(&data);
> > > > > > +		}
> > > > > > +
> > > > > > +		igt_subtest_f("psr2_primary_%s", op_str(op)) {
> > > > > > +			igt_require(data.supports_psr2);
> > > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > > >  			data.op = op;
> > > > > >  			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_PRIMARY;
> > > > > >  			test_setup(&data);
> > > > > > @@ -455,6 +507,18 @@ int main(int argc, char *argv[])
> > > > > >  
> > > > > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > > > > >  		igt_subtest_f("sprite_%s", op_str(op)) {
> > > > > > +			igt_require(data.supports_psr2);
> > > > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > > > +			data.op = op;
> > > > > > +			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_OVERLAY;
> > > > > > +			test_setup(&data);
> > > > > > +			run_test(&data);
> > > > > > +			test_cleanup(&data);
> > > > > > +		}
> > > > > > +
> > > > > > +		igt_subtest_f("psr2_sprite_%s", op_str(op)) {
> > > > > > +			igt_require(data.supports_psr2);
> > > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > > >  			data.op = op;
> > > > > >  			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_OVERLAY;
> > > > > >  			test_setup(&data);
> > > > > > @@ -465,6 +529,17 @@ int main(int argc, char *argv[])
> > > > > >  
> > > > > >  	for (op = MMAP_GTT; op <= PLANE_ONOFF; op++) {
> > > > > >  		igt_subtest_f("cursor_%s", op_str(op)) {
> > > > > > +			data.op_psr_mode = PSR_MODE_1;
> > > > > > +			data.op = op;
> > > > > > +			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_CURSOR;
> > > > > > +			test_setup(&data);
> > > > > > +			run_test(&data);
> > > > > > +			test_cleanup(&data);
> > > > > > +		}
> > > > > > +
> > > > > > +		igt_subtest_f("psr2_cursor_%s", op_str(op)) {
> > > > > > +			igt_require(data.supports_psr2);
> > > > > > +			data.op_psr_mode = PSR_MODE_2;
> > > > > >  			data.op = op;
> > > > > >  			data.test_plane_id =
> > > > > > DRM_PLANE_TYPE_CURSOR;
> > > > > >  			test_setup(&data);
> > > > > > @@ -474,6 +549,18 @@ int main(int argc, char *argv[])
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_subtest_f("dpms") {
> > > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > > +		data.op = RENDER;
> > > > > > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > > +		test_setup(&data);
> > > > > > +		dpms_off_on(&data);
> > > > > > +		run_test(&data);
> > > > > > +		test_cleanup(&data);
> > > > > > +	}
> > > > > > +
> > > > > > +	igt_subtest_f("psr2_dpms") {
> > > > > > +		igt_require(data.supports_psr2);
> > > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > > >  		data.op = RENDER;
> > > > > >  		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > > > > >  		test_setup(&data);
> > > > > > @@ -483,6 +570,20 @@ int main(int argc, char *argv[])
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_subtest_f("suspend") {
> > > > > > +		data.op_psr_mode = PSR_MODE_1;
> > > > > > +		data.op = PLANE_ONOFF;
> > > > > > +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > > > > +		test_setup(&data);
> > > > > > +		igt_system_suspend_autoresume(SUSPEND_STATE_MEM
> > > > > > ,
> > > > > > +					      SUSPEND_TEST_NONE
> > > > > > );
> > > > > > +		igt_assert(psr_wait_entry_if_enabled(&data));
> > > > > > +		run_test(&data);
> > > > > > +		test_cleanup(&data);
> > > > > > +	}
> > > > > > +
> > > > > > +	igt_subtest_f("psr2_suspend") {
> > > > > > +		igt_require(data.supports_psr2);
> > > > > > +		data.op_psr_mode = PSR_MODE_2;
> > > > > >  		data.op = PLANE_ONOFF;
> > > > > >  		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > > > > >  		test_setup(&data);

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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