[PATCH v3 0/2] tests/kms_async_flips: Create subtest for overlay planes

[PATCH v3 0/2] tests/kms_async_flips: Create subtest for overlay planes

[André Almeida]

This patchset creates a subtest for overlay planes. This is supported by amdgpu
and is merged in the kernel:

https://lore.kernel.org/lkml/173948734065.719858.7405160715916126757.b4-ty@linaro.org/

v3:
 - Fixed a bug that was mixing `alternate_sync_async = true` with the overlay path
 - Refactored how the test parameters are being set/unset (patch 1/2)
 - Fixed a bug where a primary buffer was being used with an overlay plane

v2: Add test description for GitLab compilation

André Almeida (2):
  kms_async_flips: Refactor data options
  tests/kms_async_flips: Create subtest for overlay planes

 tests/kms_async_flips.c | 72 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 60 insertions(+), 12 deletions(-)

-- 
2.49.0

[PATCH v3 1/2] kms_async_flips: Refactor data options

[André Almeida]

Setting the test data options as true and false for every test is error
prone. Instead, reset all the data to false at the end of a test and
just set the needed options to true before running a test.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
 tests/kms_async_flips.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
index da426f753..d4af9b383 100644
--- a/tests/kms_async_flips.c
+++ b/tests/kms_async_flips.c
@@ -708,6 +708,9 @@ static void run_test(data_t *data, void (*test)(data_t *))
 			test(data);
 		}
 	}
+
+	data->alternate_sync_async = false;
+	data->atomic_path = false;
 }
 
 static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))
@@ -738,6 +741,9 @@ static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))
 			}
 		}
 	}
+
+	data->alternate_sync_async = false;
+	data->atomic_path = false;
 }
 
 static data_t data;
@@ -757,6 +763,9 @@ igt_main
 
 		if (is_intel_device(data.drm_fd))
 			data.bops = buf_ops_create(data.drm_fd);
+
+		data.alternate_sync_async = false;
+		data.atomic_path = false;
 	}
 
 	igt_describe("Verify the async flip functionality and the fps during async flips");
@@ -766,8 +775,6 @@ igt_main
 
 		igt_describe("Wait for page flip events in between successive asynchronous flips");
 		igt_subtest_with_dynamic("async-flip-with-page-flip-events") {
-			data.alternate_sync_async = false;
-			data.atomic_path = false;
 			if (is_intel_device(data.drm_fd))
 				run_test_with_modifiers(&data, test_async_flip);
 			else
@@ -777,7 +784,6 @@ igt_main
 		igt_describe("Wait for page flip events in between successive "
 			     "asynchronous flips using atomic path");
 		igt_subtest_with_dynamic("async-flip-with-page-flip-events-atomic") {
-			data.alternate_sync_async = false;
 			data.atomic_path = true;
 			if (is_intel_device(data.drm_fd))
 				run_test_with_modifiers(&data, test_async_flip);
@@ -788,7 +794,6 @@ igt_main
 		igt_describe("Alternate between sync and async flips");
 		igt_subtest_with_dynamic("alternate-sync-async-flip") {
 			data.alternate_sync_async = true;
-			data.atomic_path = false;
 			run_test(&data, test_async_flip);
 		}
 
@@ -802,7 +807,6 @@ igt_main
 		igt_describe("Verify that the async flip timestamp does not "
 			     "coincide with either previous or next vblank");
 		igt_subtest_with_dynamic("test-time-stamp") {
-			data.atomic_path = false;
 			run_test(&data, test_timestamp);
 		}
 
@@ -825,7 +829,6 @@ igt_main
 			      "PSR2 sel fetch causes cursor to be added to primary plane "
 			      "pages flips and async flip is not supported in cursor\n");
 
-		data.atomic_path = false;
 		run_test(&data, test_cursor);
 	}
 
@@ -853,7 +856,6 @@ igt_main
 		igt_require(igt_display_has_format_mod(&data.display, DRM_FORMAT_XRGB8888,
 						       I915_FORMAT_MOD_Y_TILED));
 
-		data.atomic_path = false;
 		run_test(&data, test_invalid);
 	}
 
@@ -876,7 +878,6 @@ igt_main
 		/* Devices without CRC can't run this test */
 		igt_require_pipe_crc(data.drm_fd);
 
-		data.atomic_path = false;
 		run_test(&data, test_crc);
 	}
 
-- 
2.49.0

Re: [PATCH v3 1/2] kms_async_flips: Refactor data options

[Melissa Wen]

On 03/20, André Almeida wrote:
> Setting the test data options as true and false for every test is error
> prone. Instead, reset all the data to false at the end of a test and
> just set the needed options to true before running a test.
> 
> Signed-off-by: André Almeida <andrealmeid@igalia.com>

Hey, thanks for addressing this issue.

Besides that missing cleanup steps, I detected other issues on the atomic async flip subtests introduce by:
296891df ("tests/kms_async_flips: subtests to validate async flips on atomic path")
when running in a kernel without atomic async flip support.

1 - those subtests don't check the `DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP` cap
before perform atomic async flip tests and, therefore, they fail instead
of skips.

2 - the test sequence is not easy to follow and somewhat error prone
since it mixes legacy and atomic paths all the time. I think it should
be better if we keep atomic tests separated from the legacy path, i.e.
in a different igt_subtest_group.

As you are already fixing this mess, how about solve these other
problems too? :) It can also make the whole async flip test more
stable.

One more comment below

> ---
>  tests/kms_async_flips.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> index da426f753..d4af9b383 100644
> --- a/tests/kms_async_flips.c
> +++ b/tests/kms_async_flips.c
> @@ -708,6 +708,9 @@ static void run_test(data_t *data, void (*test)(data_t *))
>  			test(data);
>  		}
>  	}
> +
> +	data->alternate_sync_async = false;
> +	data->atomic_path = false;
>  }
>  
>  static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))
> @@ -738,6 +741,9 @@ static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))
>  			}
>  		}
>  	}
> +
> +	data->alternate_sync_async = false;
> +	data->atomic_path = false;
>  }
>  
>  static data_t data;
> @@ -757,6 +763,9 @@ igt_main
>  
>  		if (is_intel_device(data.drm_fd))
>  			data.bops = buf_ops_create(data.drm_fd);
> +
> +		data.alternate_sync_async = false;
> +		data.atomic_path = false;

It would be easier to maintain if we modularize cleanup steps in a
single function, such as, `test_data_cleanup`. WDYT?

BR,

Melissa

>  	}
>  
>  	igt_describe("Verify the async flip functionality and the fps during async flips");
> @@ -766,8 +775,6 @@ igt_main
>  
>  		igt_describe("Wait for page flip events in between successive asynchronous flips");
>  		igt_subtest_with_dynamic("async-flip-with-page-flip-events") {
> -			data.alternate_sync_async = false;
> -			data.atomic_path = false;
>  			if (is_intel_device(data.drm_fd))
>  				run_test_with_modifiers(&data, test_async_flip);
>  			else
> @@ -777,7 +784,6 @@ igt_main
>  		igt_describe("Wait for page flip events in between successive "
>  			     "asynchronous flips using atomic path");
>  		igt_subtest_with_dynamic("async-flip-with-page-flip-events-atomic") {
> -			data.alternate_sync_async = false;
>  			data.atomic_path = true;
>  			if (is_intel_device(data.drm_fd))
>  				run_test_with_modifiers(&data, test_async_flip);
> @@ -788,7 +794,6 @@ igt_main
>  		igt_describe("Alternate between sync and async flips");
>  		igt_subtest_with_dynamic("alternate-sync-async-flip") {
>  			data.alternate_sync_async = true;
> -			data.atomic_path = false;
>  			run_test(&data, test_async_flip);
>  		}
>  
> @@ -802,7 +807,6 @@ igt_main
>  		igt_describe("Verify that the async flip timestamp does not "
>  			     "coincide with either previous or next vblank");
>  		igt_subtest_with_dynamic("test-time-stamp") {
> -			data.atomic_path = false;
>  			run_test(&data, test_timestamp);
>  		}
>  
> @@ -825,7 +829,6 @@ igt_main
>  			      "PSR2 sel fetch causes cursor to be added to primary plane "
>  			      "pages flips and async flip is not supported in cursor\n");
>  
> -		data.atomic_path = false;
>  		run_test(&data, test_cursor);
>  	}
>  
> @@ -853,7 +856,6 @@ igt_main
>  		igt_require(igt_display_has_format_mod(&data.display, DRM_FORMAT_XRGB8888,
>  						       I915_FORMAT_MOD_Y_TILED));
>  
> -		data.atomic_path = false;
>  		run_test(&data, test_invalid);
>  	}
>  
> @@ -876,7 +878,6 @@ igt_main
>  		/* Devices without CRC can't run this test */
>  		igt_require_pipe_crc(data.drm_fd);
>  
> -		data.atomic_path = false;
>  		run_test(&data, test_crc);
>  	}
>  
> -- 
> 2.49.0
> 

[PATCH v3 2/2] tests/kms_async_flips: Create subtest for overlay planes

[André Almeida]

amdgpu can perform async flips in overlay planes as well, so create a
test for that.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
 tests/kms_async_flips.c | 55 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
index d4af9b383..bca646664 100644
--- a/tests/kms_async_flips.c
+++ b/tests/kms_async_flips.c
@@ -105,12 +105,14 @@ typedef struct {
 	uint32_t crtc_id;
 	uint32_t refresh_rate;
 	struct igt_fb bufs[NUM_FBS];
+	struct igt_fb bufs_overlay[NUM_FBS];
 	igt_display_t display;
 	igt_output_t *output;
 	unsigned long flip_timestamp_us;
 	double flip_interval;
 	uint64_t modifier;
 	igt_plane_t *plane;
+	igt_plane_t *plane_overlay;
 	igt_pipe_crc_t *pipe_crc;
 	igt_crc_t ref_crc;
 	int flip_count;
@@ -122,6 +124,7 @@ typedef struct {
 	bool allow_fail;
 	struct buf_ops *bops;
 	bool atomic_path;
+	bool overlay_path;
 } data_t;
 
 static void flip_handler(int fd_, unsigned int sequence, unsigned int tv_sec,
@@ -207,6 +210,18 @@ static void require_monotonic_timestamp(int fd)
 		      "Monotonic timestamps not supported\n");
 }
 
+static void require_overlay_flip_support(data_t *data)
+{
+	struct igt_fb *bufs = data->bufs_overlay;
+	igt_plane_t *plane = data->plane_overlay;
+	int flags = DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT;
+
+	igt_plane_set_fb(plane, &bufs[0]);
+
+	igt_require_f(!igt_display_try_commit_atomic(&data->display, flags, data),
+		      "Async flip for overlay planes not supported\n");
+}
+
 static void test_init(data_t *data)
 {
 	drmModeModeInfo *mode;
@@ -222,6 +237,8 @@ static void test_init(data_t *data)
 	igt_output_set_pipe(data->output, data->pipe);
 
 	data->plane = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
+	if (data->overlay_path)
+		data->plane_overlay = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_OVERLAY);
 }
 
 static void test_init_fbs(data_t *data)
@@ -242,16 +259,27 @@ static void test_init_fbs(data_t *data)
 		prev_modifier = data->modifier;
 
 		if (data->bufs[0].fb_id) {
-			for (i = 0; i < NUM_FBS; i++)
+			for (i = 0; i < NUM_FBS; i++) {
 				igt_remove_fb(data->drm_fd, &data->bufs[i]);
+				if (data->overlay_path)
+					igt_remove_fb(data->drm_fd, &data->bufs_overlay[i]);
+			}
 		}
 
-		for (i = 0; i < NUM_FBS; i++)
+		for (i = 0; i < NUM_FBS; i++) {
 			make_fb(data, &data->bufs[i], width, height, i);
+			if (data->overlay_path)
+				make_fb(data, &data->bufs_overlay[i], width, height, i);
+		}
 	}
 
 	igt_plane_set_fb(data->plane, &data->bufs[0]);
 	igt_plane_set_size(data->plane, width, height);
+
+	if (data->overlay_path) {
+		igt_plane_set_fb(data->plane_overlay, &data->bufs[0]);
+		igt_plane_set_size(data->plane_overlay, width, height);
+	}
 }
 
 static bool async_flip_needs_extra_frame(data_t *data)
@@ -279,12 +307,17 @@ static bool async_flip_needs_extra_frame(data_t *data)
 static int perform_flip(data_t *data, int frame, int flags)
 {
 	int ret;
+	igt_plane_t *plane;
+	struct igt_fb *bufs;
+
+	plane = data->overlay_path ? data->plane_overlay : data->plane;
+	bufs = data->overlay_path ? data->bufs_overlay : data->bufs;
 
 	if (!data->atomic_path) {
 		ret = drmModePageFlip(data->drm_fd, data->crtc_id,
-				      data->bufs[frame % NUM_FBS].fb_id, flags, data);
+				     bufs[frame % NUM_FBS].fb_id, flags, data);
 	} else {
-		igt_plane_set_fb(data->plane, &data->bufs[frame % NUM_FBS]);
+		igt_plane_set_fb(plane, &bufs[frame % NUM_FBS]);
 		ret = igt_display_try_commit_atomic(&data->display, flags, data);
 	}
 
@@ -300,6 +333,9 @@ static void test_async_flip(data_t *data)
 
 	igt_display_commit2(&data->display, data->display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
 
+	if (data->overlay_path)
+		require_overlay_flip_support(data);
+
 	gettimeofday(&start, NULL);
 	frame = 1;
 	do {
@@ -711,6 +747,7 @@ static void run_test(data_t *data, void (*test)(data_t *))
 
 	data->alternate_sync_async = false;
 	data->atomic_path = false;
+	data->overlay_path = false;
 }
 
 static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))
@@ -744,6 +781,7 @@ static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))
 
 	data->alternate_sync_async = false;
 	data->atomic_path = false;
+	data->overlay_path = false;
 }
 
 static data_t data;
@@ -766,6 +804,7 @@ igt_main
 
 		data.alternate_sync_async = false;
 		data.atomic_path = false;
+		data.overlay_path = false;
 	}
 
 	igt_describe("Verify the async flip functionality and the fps during async flips");
@@ -804,6 +843,14 @@ igt_main
 			run_test(&data, test_async_flip);
 		}
 
+		igt_describe("Verify overlay planes with async flips in atomic API");
+		igt_subtest_with_dynamic("overlay-atomic") {
+			igt_require(is_amdgpu_device(data.drm_fd));
+			data.atomic_path = true;
+			data.overlay_path = true;
+			run_test(&data, test_async_flip);
+		}
+
 		igt_describe("Verify that the async flip timestamp does not "
 			     "coincide with either previous or next vblank");
 		igt_subtest_with_dynamic("test-time-stamp") {
-- 
2.49.0

Re: [PATCH v3 2/2] tests/kms_async_flips: Create subtest for overlay planes

[Melissa Wen]

On 03/20, André Almeida wrote:
> amdgpu can perform async flips in overlay planes as well, so create a
> test for that.
> 
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> ---
>  tests/kms_async_flips.c | 55 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> index d4af9b383..bca646664 100644
> --- a/tests/kms_async_flips.c
> +++ b/tests/kms_async_flips.c
> @@ -105,12 +105,14 @@ typedef struct {
>  	uint32_t crtc_id;
>  	uint32_t refresh_rate;
>  	struct igt_fb bufs[NUM_FBS];
> +	struct igt_fb bufs_overlay[NUM_FBS];
>  	igt_display_t display;
>  	igt_output_t *output;
>  	unsigned long flip_timestamp_us;
>  	double flip_interval;
>  	uint64_t modifier;
>  	igt_plane_t *plane;
> +	igt_plane_t *plane_overlay;
>  	igt_pipe_crc_t *pipe_crc;
>  	igt_crc_t ref_crc;
>  	int flip_count;
> @@ -122,6 +124,7 @@ typedef struct {
>  	bool allow_fail;
>  	struct buf_ops *bops;
>  	bool atomic_path;
> +	bool overlay_path;
>  } data_t;
>  
>  static void flip_handler(int fd_, unsigned int sequence, unsigned int tv_sec,
> @@ -207,6 +210,18 @@ static void require_monotonic_timestamp(int fd)
>  		      "Monotonic timestamps not supported\n");
>  }
>  
> +static void require_overlay_flip_support(data_t *data)
> +{
> +	struct igt_fb *bufs = data->bufs_overlay;
> +	igt_plane_t *plane = data->plane_overlay;
> +	int flags = DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT;
> +
> +	igt_plane_set_fb(plane, &bufs[0]);
> +
> +	igt_require_f(!igt_display_try_commit_atomic(&data->display, flags, data),
> +		      "Async flip for overlay planes not supported\n");
> +}
> +
>  static void test_init(data_t *data)
>  {
>  	drmModeModeInfo *mode;
> @@ -222,6 +237,8 @@ static void test_init(data_t *data)
>  	igt_output_set_pipe(data->output, data->pipe);
>  
>  	data->plane = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
> +	if (data->overlay_path)
> +		data->plane_overlay = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_OVERLAY);
>  }
>  
>  static void test_init_fbs(data_t *data)
> @@ -242,16 +259,27 @@ static void test_init_fbs(data_t *data)
>  		prev_modifier = data->modifier;
>  
>  		if (data->bufs[0].fb_id) {
> -			for (i = 0; i < NUM_FBS; i++)
> +			for (i = 0; i < NUM_FBS; i++) {
>  				igt_remove_fb(data->drm_fd, &data->bufs[i]);
> +				if (data->overlay_path)
> +					igt_remove_fb(data->drm_fd, &data->bufs_overlay[i]);
> +			}
>  		}
>  
> -		for (i = 0; i < NUM_FBS; i++)
> +		for (i = 0; i < NUM_FBS; i++) {
>  			make_fb(data, &data->bufs[i], width, height, i);
> +			if (data->overlay_path)
> +				make_fb(data, &data->bufs_overlay[i], width, height, i);
> +		}
>  	}
>  
>  	igt_plane_set_fb(data->plane, &data->bufs[0]);
>  	igt_plane_set_size(data->plane, width, height);
> +
> +	if (data->overlay_path) {
> +		igt_plane_set_fb(data->plane_overlay, &data->bufs[0]);
> +		igt_plane_set_size(data->plane_overlay, width, height);
> +	}
>  }
>  
>  static bool async_flip_needs_extra_frame(data_t *data)
> @@ -279,12 +307,17 @@ static bool async_flip_needs_extra_frame(data_t *data)
>  static int perform_flip(data_t *data, int frame, int flags)
>  {
>  	int ret;
> +	igt_plane_t *plane;
> +	struct igt_fb *bufs;
> +
> +	plane = data->overlay_path ? data->plane_overlay : data->plane;
> +	bufs = data->overlay_path ? data->bufs_overlay : data->bufs;
>  
>  	if (!data->atomic_path) {
>  		ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> -				      data->bufs[frame % NUM_FBS].fb_id, flags, data);
> +				     bufs[frame % NUM_FBS].fb_id, flags, data);
>  	} else {
> -		igt_plane_set_fb(data->plane, &data->bufs[frame % NUM_FBS]);
> +		igt_plane_set_fb(plane, &bufs[frame % NUM_FBS]);
>  		ret = igt_display_try_commit_atomic(&data->display, flags, data);
>  	}
>  
> @@ -300,6 +333,9 @@ static void test_async_flip(data_t *data)
>  
>  	igt_display_commit2(&data->display, data->display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
>  
> +	if (data->overlay_path)
> +		require_overlay_flip_support(data);
> +
>  	gettimeofday(&start, NULL);
>  	frame = 1;
>  	do {
> @@ -711,6 +747,7 @@ static void run_test(data_t *data, void (*test)(data_t *))
>  
>  	data->alternate_sync_async = false;
>  	data->atomic_path = false;
> +	data->overlay_path = false;
>  }
>  
>  static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))
> @@ -744,6 +781,7 @@ static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))
>  
>  	data->alternate_sync_async = false;
>  	data->atomic_path = false;
> +	data->overlay_path = false;
>  }
>  
>  static data_t data;
> @@ -766,6 +804,7 @@ igt_main
>  
>  		data.alternate_sync_async = false;
>  		data.atomic_path = false;
> +		data.overlay_path = false;

As commented in the previous patch, we can make it simpler and reduce
future issues by cleaning data in a single call.

>  	}
>  
>  	igt_describe("Verify the async flip functionality and the fps during async flips");
> @@ -804,6 +843,14 @@ igt_main
>  			run_test(&data, test_async_flip);
>  		}

Also, compilation fails on my side due to missing documentation. You've
added it in the previous version and removed in this one.

BR,

Melissa
>  
> +		igt_describe("Verify overlay planes with async flips in atomic API");
> +		igt_subtest_with_dynamic("overlay-atomic") {
> +			igt_require(is_amdgpu_device(data.drm_fd));
> +			data.atomic_path = true;
> +			data.overlay_path = true;
> +			run_test(&data, test_async_flip);
> +		}
> +
>  		igt_describe("Verify that the async flip timestamp does not "
>  			     "coincide with either previous or next vblank");
>  		igt_subtest_with_dynamic("test-time-stamp") {
> -- 
> 2.49.0
>