[PATCH i-g-t v2 2/2] tests/kms_async_flips: use in_formats_async for async modifiers

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Reddy Guddati, Santhosh <santhosh.reddy.guddati at intel.com>
> Sent: Friday, February 21, 2025 11:27 AM
> To: igt-dev at lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy at intel.com>; B S, Karthik
> <karthik.b.s at intel.com>; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah at intel.com>; Reddy Guddati, Santhosh
> <santhosh.reddy.guddati at intel.com>
> Subject: [PATCH i-g-t v2 2/2] tests/kms_async_flips: use in_formats_async for
> async modifiers
> 
> Add support for dynamic plane and modifiers in async flip with modifiers
> 

The commit message is not that clear

> V2: Improve run_test_with_modifiers to set data formats based on async
>     formats.
>     Update make_fb to use data formats instead of hard coded format
> 
> Signed-off-by: Santhosh Reddy Guddati <santhosh.reddy.guddati at intel.com>
> ---
>  tests/kms_async_flips.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c index
> 4a72be7b5..38fc2ced4 100644
> --- a/tests/kms_async_flips.c
> +++ b/tests/kms_async_flips.c
> @@ -126,6 +126,7 @@ typedef struct {
>  	bool allow_fail;
>  	struct buf_ops *bops;
>  	bool atomic_path;
> +	unsigned int plane_format;
>  } data_t;
> 
>  static void flip_handler(int fd_, unsigned int sequence, unsigned int tv_sec,
> @@ -197,7 +198,7 @@ static void make_fb(data_t *data, struct igt_fb *fb,
> 
>  	rec_width = width / (NUM_FBS * 2);
> 
> -	igt_create_color_fb(data->drm_fd, width, height,
> DRM_FORMAT_XRGB8888,
> +	igt_create_color_fb(data->drm_fd, width, height, data->plane_format,
>  			    data->modifier, 0.0, 0.0, 0.5, fb);
> 
>  	cr = igt_get_cairo_ctx(data->drm_fd, fb); @@ -718,17 +719,34 @@
> static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))  {
>  	for_each_pipe_with_valid_output(&data->display, data->pipe, data-
> >output) {
>  		test_init(data);
> +		static int j;
> 
> -		for (int i = 0; i < data->plane->format_mod_count; i++) {
> -			if (data->plane->formats[i] !=
> DRM_FORMAT_XRGB8888)
> +		for (int i = 0; i < data->plane->async_format_mod_count; i++) {
> +
> +			/* Check if the format is already tested */
> +			bool format_already_tested = false;
> +
> +			for ( ; j < i; j++) {
> +				if (data->plane->async_formats[j] == data-
> >plane->async_formats[i] &&
> +				    data->plane->async_formats[j] != data-
> >plane->async_modifiers[i]) {
> +					format_already_tested = true;
> +					j = i+1;
> +					break;
> +				}
> +			}
> +
> +			if (format_already_tested)

This logic looks unnecessarily complicated. What are we trying to achieve here? If a format is already tested with a modifier, don't use that format again?

If so, I don't quite understand the reasoning behind it. Why can't we just iterate over each format for every modifier? (which also increases our coverage)

Regards

Chaitanya

>  				continue;
> 
>  			data->allow_fail = true;
> -			data->modifier = data->plane->modifiers[i];
> +			data->modifier = data->plane->async_modifiers[i];
> +			data->plane_format = data->plane->async_formats[i];
> +
> 
> -			igt_dynamic_f("pipe-%s-%s-%s",
> kmstest_pipe_name(data->pipe),
> +			igt_dynamic_f("pipe-%s-%s-%s-%s",
> kmstest_pipe_name(data->pipe),
>  				      data->output->name,
> -				      igt_fb_modifier_name(data->modifier)) {
> +				      igt_fb_modifier_name(data->modifier),
> +				      igt_format_str(data->plane_format)) {
>  				      /*
>  				       * FIXME: joiner+async flip is busted
> currently in KMD.
>  				       * Remove this check once the issues are
> fixed in KMD.
> @@ -761,6 +779,7 @@ igt_main
> 
>  		if (is_intel_device(data.drm_fd))
>  			data.bops = buf_ops_create(data.drm_fd);
> +		data.plane_format = DRM_FORMAT_XRGB8888;
>  	}
> 
>  	igt_describe("Verify the async flip functionality and the fps during
> async flips");
> --
> 2.25.1