Re: [igt-dev] [V2 i-g-t] tests/kms_cursor_crc: Fix SIGABRT in dynamic subtests

[Melissa Wen <mwen@igalia.com>]

On 11/04, Bhanuprakash Modem wrote:
> All subtests must report success/fail instead of crashing out.
> 
> V2:
> - Fix the coverage of max-size tests
> 
> Fixes: 9494d53d ("tests/kms_cursor_crc: Convert tests to dynamic")
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Signed-off-by: Alaa Emad <aemad@igalia.com>
> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> ---
>  tests/kms_cursor_crc.c | 128 ++++++++++++++++++++++-------------------
>  1 file changed, 68 insertions(+), 60 deletions(-)
> 
> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
> index d51c1f90..68bed666 100644
> --- a/tests/kms_cursor_crc.c
> +++ b/tests/kms_cursor_crc.c
> @@ -700,65 +700,78 @@ 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;
> +	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." },
> +	};
> +	int i;
> +	char name[16];
>  
>  	if (w == 0 && h == 0) {
>  		w = data->cursor_max_w;
>  		h = data->cursor_max_h;
>  
> -		/*
> -		 * No point in doing the "max-size" test if
> -		 * it was already covered by the other tests.
> -		 */
> -		if ((w == h) && (w <= 512) && (h <= 512) &&
> -		    is_power_of_two(w) && is_power_of_two(h)) {
> -			igt_info("Cursor max size %dx%d already covered by other tests\n", w, h);
> -			return;
> -		}
> +		strcpy(name, "max-size");
> +	} else {
> +		snprintf(name, sizeof(name), "%dx%d", w, h);
>  	}
>  
> -	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_fixture
> +		create_cursor_fb(data, w, h);
>  
> -		igt_remove_fb(data->drm_fd, &data->fb);
> -		return;
> -	}
> +	for (i = 0; i < ARRAY_SIZE(size_tests); i++) {
> +		igt_describe(size_tests[i].desc);
> +		igt_subtest_with_dynamic_f("%s-%s", size_tests[i].name, name) {
> +			if (!strcmp(name, "max-size")) {
> +				/*
> +				 * No point in doing the "max-size" test if
> +				 * it was already covered by the other tests.
> +				 */
> +				if ((w == h) && (w <= 512) && (h <= 512) &&
> +				    is_power_of_two(w) && is_power_of_two(h)) {
> +					igt_info("Cursor max size %dx%d already covered by other tests\n", w, h);
> +					continue;
> +				}
> +			}
> +
> +			for_each_pipe(&data->display, pipe) {
> +				data->pipe = pipe;
>  
> -	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);
> +				if (require_cursor_size(data, w, h)) {
I think here we want `!require_cursor_size()` (not require_cursor_size),
since zero means success for drmModeAtomicCommit(), that is used to
check if the cursor size is accepted or not.
> +					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);
> @@ -859,30 +872,25 @@ 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;
> +	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);
> +		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_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-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);
>  	}
> +
> +	run_size_tests(data, 0, 0);
>  }
>  
>  static data_t data;
> -- 
> 2.38.0
>