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

Submitted by José Roberto de Souza on Jan. 12, 2019, 1:46 a.m.

Details

Message ID 20190112014607.13446-10-jose.souza@intel.com
State New
Series "Series without cover letter"
Headers show

Commit Message

José Roberto de Souza Jan. 12, 2019, 1:46 a.m.
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(-)

Patch hide | download patch | download mbox

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);

Comments

Rodrigo Vivi Jan. 14, 2019, 6:25 p.m.
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
>
Dhinakaran Pandiyan Jan. 16, 2019, 6:44 a.m.
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);
José Roberto de Souza Jan. 17, 2019, 9:43 p.m.
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);
Dhinakaran Pandiyan Jan. 17, 2019, 9:51 p.m.
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);
José Roberto de Souza Jan. 17, 2019, 11:05 p.m.
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);
Dhinakaran Pandiyan Jan. 17, 2019, 11:09 p.m.
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);
José Roberto de Souza Jan. 17, 2019, 11:41 p.m.
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);