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

["Reddy Guddati, Santhosh" <santhosh.reddy.guddati@intel.com>]

Hi Chaitanya,

Thanks for reviewing the changes. Please find the comments inline.

On 17-03-2025 16:26, Borah, Chaitanya Kumar wrote:
> 
> 
>> -----Original Message-----
>> From: Reddy Guddati, Santhosh <santhosh.reddy.guddati@intel.com>
>> Sent: Tuesday, March 11, 2025 2:24 PM
>> To: igt-dev@lists.freedesktop.org
>> Cc: Murthy, Arun R <arun.r.murthy@intel.com>; B S, Karthik
>> <karthik.b.s@intel.com>; Borah, Chaitanya Kumar
>> <chaitanya.kumar.borah@intel.com>; Reddy Guddati, Santhosh
>> <santhosh.reddy.guddati@intel.com>
>> Subject: [PATCH i-g-t v5 2/2] tests/kms_async_flips: use in_formats_async for
>> async modifiers
>>
>> Utilise IN_FORMATS_ASYNC property exposed to get the list of async
>> supported modifier/format pair and improve the test coverage by iterating
>> through all the supported modifier format pairs.
>>
>> 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
>>
>> V3: Update commit message, remove complicated iterations (Chaitanya)
>>
>> V4: Reduce the format+modifier combinations to reduce time needed to
>>      execute the tests. (Chaitanya)
>>
>> Signed-off-by: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
>> ---
>>   tests/kms_async_flips.c | 76 +++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 70 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c index
>> da426f753..0e2b0afec 100644
>> --- a/tests/kms_async_flips.c
>> +++ b/tests/kms_async_flips.c
>> @@ -36,6 +36,7 @@
>>   #include "igt.h"
>>   #include "igt_aux.h"
>>   #include "igt_psr.h"
>> +#include "igt_vec.h"
>>   #include <sys/ioctl.h>
>>   #include <sys/time.h>
>>   #include <poll.h>
>> @@ -122,8 +123,14 @@ typedef struct {
>>   	bool allow_fail;
>>   	struct buf_ops *bops;
>>   	bool atomic_path;
>> +	unsigned int plane_format;
>>   } data_t;
>>
>> +struct format_mod {
>> +	uint64_t modifier;
>> +	uint32_t format;
>> +};
>> +
>>   static void flip_handler(int fd_, unsigned int sequence, unsigned int tv_sec,
>>   			 unsigned int tv_usec, void *_data)
>>   {
>> @@ -193,7 +200,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); @@ -710,21 +717,75 @@
>> static void run_test(data_t *data, void (*test)(data_t *))
>>   	}
>>   }
>>
>> +static bool skip_async_format_mod(data_t *data,
>> +			    uint32_t format, uint64_t modifier,
>> +			    struct igt_vec *tested_formats)
>> +{
>> +	/* igt doesn't know how to sw generate UBWC: */
>> +	if (is_msm_device(data->drm_fd) &&
>> +	    modifier == DRM_FORMAT_MOD_QCOM_COMPRESSED)
>> +		return true;
>> +
>> +	/* VEBOX just hangs with an actual 10bpc format */
>> +	if (igt_fb_is_gen12_mc_ccs_modifier(modifier) &&
>> +	    igt_reduce_format(format) == DRM_FORMAT_XRGB2101010)
>> +		return true;
>> +
>> +	/* test each format "class" only once in non-extended tests */
>> +	if (modifier != DRM_FORMAT_MOD_LINEAR) {
>> +		struct format_mod rf = {
>> +			.format = igt_reduce_format(format),
>> +			.modifier = modifier,
>> +		};
>> +
>> +		if (igt_vec_index(tested_formats, &rf) >= 0)
>> +			return true;
>> +
>> +		igt_vec_push(tested_formats, &rf);
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   static void run_test_with_modifiers(data_t *data, void (*test)(data_t *))  {
>> +	struct format_mod ref = {};
>> +	struct igt_vec tested_formats;
>> +
>> +	ref.format = DRM_FORMAT_ARGB8888;
>> +	ref.modifier = DRM_FORMAT_MOD_LINEAR;
>> +
>> +	igt_vec_init(&tested_formats, sizeof(struct format_mod));
>> +
>>   	for_each_pipe_with_valid_output(&data->display, data->pipe, data-
>>> output) {
>>   		test_init(data);
>> +		for (int i = 0; i < data->plane->async_format_mod_count; i++) {
>> +			struct format_mod f = {
>> +				.format = data->plane->async_formats[i],
>> +				.modifier = data->plane->async_modifiers[i],
>> +			};
>>
>> -		for (int i = 0; i < data->plane->format_mod_count; i++) {
>> -			if (data->plane->formats[i] !=
>> DRM_FORMAT_XRGB8888)
>> +			if (ref.format == f.format && ref.modifier ==
>> f.modifier)
>>   				continue;
> 
> What is this check for?
> 
> The rest of the test looks logical.
> 

Removed these extra checks in rev v6 as the checks are redundant.> Regards
> 
> Chaitanya
> 
>>
>> +			if (skip_async_format_mod(data, f.format, f.modifier,
>> &tested_formats)) {
>> +				igt_debug("Skipping format "
>> IGT_FORMAT_FMT " / modifier "
>> +					   IGT_MODIFIER_FMT " on %s.%u\n",
>> +					   IGT_FORMAT_ARGS(f.format),
>> +					   IGT_MODIFIER_ARGS(f.modifier),
>> +					   kmstest_pipe_name(data->pipe),
>> +					   data->plane->index);
>> +				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.
>> @@ -738,6 +799,8 @@ static void run_test_with_modifiers(data_t *data, void
>> (*test)(data_t *))
>>   			}
>>   		}
>>   	}
>> +
>> +	igt_vec_fini(&tested_formats);
>>   }
>>
>>   static data_t data;
>> @@ -757,6 +820,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.34.1
>