[igt-dev] [PATCH i-g-t] tests/kms_cursor_crc: prevent the entire test from being aborted when a subtest fails

* [igt-dev] [PATCH i-g-t] tests/kms_cursor_crc: prevent the entire test from being aborted when a subtest fails
@ 2022-10-09 15:18 Alaa Emad
  2022-10-09 15:52 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 25+ messages in thread
From: Alaa Emad @ 2022-10-09 15:18 UTC (permalink / raw)
  To: igt-dev; +Cc: petri.latvala

Any test failure prevents the other tests to be run, aborting the entire test loop. This behavior was introduced by commit
9494d53d ("tests/kms_cursor_crc: Convert tests to dynamic").
For instance, when running the cursor-offscreen-32x10 subtest using VKMS,
the execution is aborted instead of SKIP/FAIL, as in the following output:

$ ./tests/kms_cursor_crc --run-subtest cursor-offscreen-32x10
(kms_cursor_crc:7447) igt_fb-CRITICAL: Test assertion failure function igt_create_fb_with_bo_size, file ../lib/igt_fb.c:2078:
(kms_cursor_crc:7447) igt_fb-CRITICAL: Failed assertion: (__kms_addfb(fb->fd, fb->gem_handle, fb->width, fb->height, fb->drm_format, fb->modifier, fb->strides, fb->offsets, fb->num_planes, flags, &fb->fb_id)) == 0
(kms_cursor_crc:7447) igt_fb-CRITICAL: Last errno: 22, Invalid argument
Stack trace:
kms_cursor_crc: ../lib/igt_core.c:1667: igt_fail: Assertion `_igt_dynamic_tests_executed < 0 || dynamic_failed_one' failed.
Received signal SIGABRT.

The test was aborted after convertig tests to dynamic with cursor size 32*10 that because of the assertion in the `igt_create_fb_with_bo_size` when creating cursor in `create_cursor_fb`.

This happened because Within a igt_subtest_with_dynamic block, explicit failure (e.g. igt_assert) is not allowed,
only dynamic subsubtests themselves will produce test results.

The previous approach was running each test for all cursor sizes then move to the next test  so fix this issue
by running all tests for one cursor size one by one, and check if cursor size is
supported or not before running the test.

Fixes: 9494d53d ("tests/kms_cursor_crc: Convert tests to dynamic")

Signed-off-by: Alaa Emad <aemad@igalia.com>
---
 tests/kms_cursor_crc.c | 136 +++++++++++++++++++++--------------------
 1 file changed, 69 insertions(+), 67 deletions(-)

diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
index 8d3426dd..9b620389 100644
--- a/tests/kms_cursor_crc.c
+++ b/tests/kms_cursor_crc.c
@@ -72,7 +72,6 @@ typedef struct {
 	cairo_surface_t *surface;
 	uint32_t devid;
 	double alpha;
-	int vblank_wait_count; /* because of msm */
 } data_t;
 
 #define TEST_DPMS (1<<0)
@@ -125,12 +124,6 @@ static void cursor_disable(data_t *data)
 {
 	igt_plane_set_fb(data->cursor, NULL);
 	igt_plane_set_position(data->cursor, 0, 0);
-	igt_display_commit(&data->display);
-
-	/* do this wait here so it will not need to be added everywhere */
-	igt_wait_for_vblank_count(data->drm_fd,
-				  data->display.pipes[data->pipe].crtc_offset,
-				  data->vblank_wait_count);
 }
 
 static bool chv_cursor_broken(data_t *data, int x)
@@ -209,9 +202,8 @@ static void do_single_test(data_t *data, int x, int y, bool hw_test,
 		igt_display_commit(display);
 
 		/* Extra vblank wait is because nonblocking cursor ioctl */
-		igt_wait_for_vblank_count(data->drm_fd,
-					  display->pipes[data->pipe].crtc_offset,
-					  data->vblank_wait_count);
+		igt_wait_for_vblank(data->drm_fd,
+				display->pipes[data->pipe].crtc_offset);
 
 		igt_pipe_crc_get_current(data->drm_fd, pipe_crc, hwcrc);
 
@@ -249,7 +241,11 @@ static void do_single_test(data_t *data, int x, int y, bool hw_test,
 
 		restore_image(data, swbufidx, &((cursorarea){x, y, data->curw, data->curh}));
 		igt_plane_set_fb(data->primary, &data->primary_fb[swbufidx]);
+
 		igt_display_commit(display);
+		igt_wait_for_vblank(data->drm_fd,
+				display->pipes[data->pipe].crtc_offset);
+
 		igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
 		igt_assert_crc_equal(&crc, hwcrc);
 	}
@@ -267,7 +263,9 @@ static void do_fail_test(data_t *data, int x, int y, int expect)
 	igt_plane_set_position(data->cursor, x, y);
 	ret = igt_display_try_commit2(display, COMMIT_LEGACY);
 
+	igt_plane_set_position(data->cursor, 0, 0);
 	cursor_disable(data);
+	igt_display_commit(display);
 
 	igt_assert_eq(ret, expect);
 }
@@ -689,10 +687,28 @@ static void test_rapid_movement(data_t *data)
 	igt_assert_lt(usec, 0.9 * 400 * 1000000 / data->refresh);
 }
 
-static void run_size_tests(data_t *data, void (*testfunc)(data_t *),
-			   int w, int h)
+static void run_size_tests(data_t *data, int w, int h)
 {
 	enum pipe pipe;
+	int i;
+
+	struct
+	{
+		const char *name;
+		void (*testfunc)(data_t *);
+		const char *desc;
+	} size_tests[] = {
+		{"cursor-onscreen", test_crc_onscreen,
+		 "Check if a given-size cursor is well-positioned inside the screen."},
+		{"cursor-offscreen", test_crc_offscreen,
+		 "Check if a given-size cursor is well-positioned outside the screen."},
+		{"cursor-sliding", test_crc_sliding,
+		 "Check the smooth and pixel-by-pixel given-size cursor movements on horizontal, vertical and diagonal."},
+		{"cursor-random", test_crc_random,
+		 "Check random placement of a cursor with given size."},
+		{"cursor-rapid-movement", test_rapid_movement,
+		 "Check the rapid update of given-size cursor movements."},
+	};
 
 	if (w == 0 && h == 0) {
 		w = data->cursor_max_w;
@@ -709,45 +725,38 @@ static void run_size_tests(data_t *data, void (*testfunc)(data_t *),
 		}
 	}
 
-	create_cursor_fb(data, w, h);
-	if (require_cursor_size(data, w, h)) {
-		igt_info("Cursor size %dx%d not supported by driver\n", w, h);
-
-		igt_remove_fb(data->drm_fd, &data->fb);
-		return;
-	}
+	igt_fixture
+		create_cursor_fb(data, w, h);
 
-	for_each_pipe(&data->display, pipe) {
-		data->pipe = pipe;
-		igt_dynamic_f("pipe-%s-%s",
-			      kmstest_pipe_name(pipe), igt_output_name(data->output))
-			run_test(data, testfunc, w, h);
+	for (i = 0; i < ARRAY_SIZE(size_tests); i++)
+	{
+		igt_describe(size_tests[i].desc);
+		igt_subtest_with_dynamic_f("%s-%dx%d", size_tests[i].name, w, h)
+		{
+			for_each_pipe(&data->display, pipe)
+			{
+				data->pipe = pipe;
+				if (require_cursor_size(data, w, h))
+				{
+					igt_info("Cursor size %dx%d not supported by driver\n", w, h);
+					continue;
+				}
+
+				igt_dynamic_f("pipe-%s-%s",
+							  kmstest_pipe_name(pipe), igt_output_name(data->output))
+					run_test(data, size_tests[i].testfunc, w, h);
+			}
+		}
 	}
 
-	igt_remove_fb(data->drm_fd, &data->fb);
+	igt_fixture
+		igt_remove_fb(data->drm_fd, &data->fb);
 }
 
 static void run_tests_on_pipe(data_t *data)
 {
 	enum pipe pipe;
 	int cursor_size;
-	int i;
-	struct {
-		const char *name;
-		void (*testfunc)(data_t *);
-		const char *desc;
-	} size_tests[] = {
-		{ "cursor-onscreen", test_crc_onscreen,
-			"Check if a given-size cursor is well-positioned inside the screen." },
-		{ "cursor-offscreen", test_crc_offscreen,
-			"Check if a given-size cursor is well-positioned outside the screen." },
-		{ "cursor-sliding", test_crc_sliding,
-			"Check the smooth and pixel-by-pixel given-size cursor movements on horizontal, vertical and diagonal." },
-		{ "cursor-random", test_crc_random,
-			"Check random placement of a cursor with given size." },
-		{ "cursor-rapid-movement", test_rapid_movement,
-			"Check the rapid update of given-size cursor movements." },
-	};
 
 	igt_fixture {
 		data->output = igt_get_single_output_for_pipe(&data->display, pipe);
@@ -848,30 +857,26 @@ static void run_tests_on_pipe(data_t *data)
 	igt_fixture
 		igt_remove_fb(data->drm_fd, &data->fb);
 
-	for (i = 0; i < ARRAY_SIZE(size_tests); i++) {
-		igt_describe(size_tests[i].desc);
-		igt_subtest_group {
-			for (cursor_size = 32; cursor_size <= 512; cursor_size *= 2) {
-				int w = cursor_size;
-				int h = cursor_size;
-
-				igt_subtest_with_dynamic_f("%s-%dx%d", size_tests[i].name, w, h)
-					run_size_tests(data, size_tests[i].testfunc, w, h);
-
-				/*
-				 * Test non-square cursors a bit on the platforms
-				 * that support such things. And make it a bit more
-				 * interesting by using a non-pot height.
-				 */
-				h /= 3;
-				igt_subtest_with_dynamic_f("%s-%dx%d", size_tests[i].name, w, h)
-					run_size_tests(data, size_tests[i].testfunc, w, h);
-			}
+	for (cursor_size = 32; cursor_size <= 512; cursor_size *= 2)
+	{
+		int w = cursor_size;
+		int h = cursor_size;
 
-			igt_subtest_with_dynamic_f("%s-max-size", size_tests[i].name)
-				run_size_tests(data, size_tests[i].testfunc, 0, 0);
-		}
+		igt_subtest_group
+			run_size_tests(data, w, h);
+
+		/*
+		 * Test non-square cursors a bit on the platforms
+		 * that support such things. And make it a bit more
+		 * interesting by using a non-pot height.
+		 */
+		h /= 3;
+
+		igt_subtest_group
+			run_size_tests(data, w, h);
 	}
+	
+	run_size_tests(data, 0, 0);
 }
 
 static data_t data;
@@ -896,8 +901,6 @@ igt_main
 		kmstest_set_vt_graphics_mode();
 
 		igt_require_pipe_crc(data.drm_fd);
-
-		data.vblank_wait_count = is_msm_device(data.drm_fd) ? 2 : 1;
 	}
 
 	data.cursor_max_w = cursor_width;
@@ -913,6 +916,5 @@ igt_main
 		}
 
 		igt_display_fini(&data.display);
-		close(data.drm_fd);
 	}
 }
-- 
2.34.1

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