[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 at 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
>