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

Dhinakaran Pandiyan dhinakaran.pandiyan at intel.com
Wed Jan 16 06:44:19 UTC 2019


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 at intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at 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);



More information about the igt-dev mailing list