Re: [PATCH i-g-t 5/9] tests/kms_sharpness_filter: Add filter-tap subtest

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 12/9/2024 5:16 PM, Swati Sharma wrote:
> Subtest is added to verify that following a resolution change,
> distinct taps are selected i.e.
>
> TAP 3: mode->hdisplay <= 1920 && mode->vdisplay <= 1080
> TAP 5: mode->hdisplay > 1920 && mode->vdisplay > 1080

mode->hdisplay > 1920  and <= 3840; mode->vdisplay > 1080 and <= 2160


> TAP 7: mode->hdisplay >= 3840 && mode->vdisplay >= 2160
>
> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
> ---
>   tests/kms_sharpness_filter.c | 79 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 79 insertions(+)
>
> diff --git a/tests/kms_sharpness_filter.c b/tests/kms_sharpness_filter.c
> index aa200b16a..f91c444f7 100644
> --- a/tests/kms_sharpness_filter.c
> +++ b/tests/kms_sharpness_filter.c
> @@ -37,6 +37,9 @@
>    *
>    * SUBTEST: filter-toggle
>    * Description: Verify toggling between enabling and disabling content adaptive sharpness filter.
> + *
> + * SUBTEST: filter-tap
> + * Description: Verify that following a resolution change, distinct taps are selected.

Hmm.. we cannot verify whether distinct taps are selected or not. What 
we can verify is that sharpness works with different resolutions 
(implicitly different taps).

>   */
>   
>   IGT_TEST_DESCRIPTION("Test to validate content adaptive sharpness filter");
> @@ -47,10 +50,15 @@ IGT_TEST_DESCRIPTION("Test to validate content adaptive sharpness filter");
>    * is seen without corruption for each subtest.
>    */
>   
> +#define TAP_3				3
> +#define TAP_5				5
> +#define TAP_7				7
>   #define DISABLE_FILTER			0
>   #define MIN_FILTER_STRENGTH		1
>   #define MID_FILTER_STRENGTH		128
>   #define MAX_FILTER_STRENGTH		255
> +#define MAX_PIXELS_FOR_3_TAP_FILTER	(1920 * 1080)
> +#define MAX_PIXELS_FOR_5_TAP_FILTER	(3840 * 2160)
>   #define NROUNDS				10
>   
>   enum test_type {
> @@ -60,6 +68,7 @@ enum test_type {
>   	TEST_FILTER_FORMATS,
>   	TEST_FILTER_STRENGTH,
>   	TEST_FILTER_TOGGLE,
> +	TEST_FILTER_TAP,
>   };
>   
>   const int filter_strength_list[] = {
> @@ -69,6 +78,11 @@ const int filter_strength_list[] = {
>   	(MID_FILTER_STRENGTH + MAX_FILTER_STRENGTH) / 2,
>   	MAX_FILTER_STRENGTH,
>   };
> +const int filter_tap_list[] = {
> +	TAP_3,
> +	TAP_5,
> +	TAP_7,
> +};
>   static const struct {
>   	uint64_t modifier;
>   	const char *name;
> @@ -99,6 +113,7 @@ typedef struct {
>   	igt_plane_t *plane[4];
>   	drmModeModeInfo *mode;
>   	int filter_strength;
> +	int filter_tap;
>   	uint64_t modifier;
>   	const char *modifier_name;
>   	uint32_t format;
> @@ -151,6 +166,31 @@ static void cleanup(data_t *data)
>   	cleanup_fbs(data);
>   }
>   
> +static void get_modes_for_filter_taps(igt_output_t *output, drmModeModeInfo *mode[3])
> +{
> +	drmModeConnector *connector = output->config.connector;
> +	int total_pixels = 0;
> +
> +	/*
> +	 * TAP 3: mode->hdisplay <= 1920 && mode->vdisplay <= 1080
> +	 * TAP 5: mode->hdisplay > 1920 && mode->vdisplay > 1080

Same as above.


> +	 * TAP 7: mode->hdisplay >= 3840 && mode->vdisplay >= 2160
> +	 */
> +	for (int i = 0; i < connector->count_modes; i++) {
> +		total_pixels = connector->modes[i].hdisplay * connector->modes[i].vdisplay;
> +
> +		if (total_pixels <= MAX_PIXELS_FOR_3_TAP_FILTER)
> +			mode[0] = &connector->modes[i];
> +
> +		if (total_pixels > MAX_PIXELS_FOR_3_TAP_FILTER &&
> +		    total_pixels <= MAX_PIXELS_FOR_5_TAP_FILTER)
> +			mode[1] = &connector->modes[i];
> +
> +		if (total_pixels > MAX_PIXELS_FOR_5_TAP_FILTER)
> +			mode[2] = &connector->modes[i];
> +	}
> +}
> +
>   static int test_filter_toggle(data_t *data)
>   {
>   	int ret = 0;
> @@ -237,6 +277,34 @@ run_sharpness_filter_test(data_t *data, enum test_type type)
>   				continue;
>   			}
>   
> +			if (type == TEST_FILTER_TAP) {
> +				drmModeModeInfo *modes[3] = { NULL, NULL, NULL };
> +				int num_taps = ARRAY_SIZE(filter_tap_list);
> +
> +				igt_assert(num_taps == 3);
> +
> +				get_modes_for_filter_taps(output, modes);
> +				for (int k = 0; k < 3; k++) {

Usually 'i' is used as iterator unless other identifier makes more sense 
or perhaps when nested loops are used.

Lets stick to the common practice. Same thing in other patches in the 
series.

Regards,

Ankit

> +					data->filter_tap = filter_tap_list[k];
> +					if (!modes[k])
> +						continue;
> +					data->mode = modes[k];
> +				        igt_info("Mode %dx%d@%d on output %s\n", data->mode->hdisplay, data->mode->vdisplay,
> +						  data->mode->vrefresh, igt_output_name(data->output));
> +					igt_output_override_mode(data->output, data->mode);
> +
> +					snprintf(name, sizeof(name), "-tap-%d", data->filter_tap);
> +					igt_dynamic_f("pipe-%s-%s%s", kmstest_pipe_name(data->pipe_id),
> +						       data->output->name, name)
> +						test_sharpness_filter(data, type);
> +				}
> +
> +				if (data->limited)
> +					break;
> +
> +				continue;
> +			}
> +
>   			switch (type) {
>   			case TEST_FILTER_BASIC:
>   				snprintf(name, sizeof(name), "-basic");
> @@ -379,6 +447,17 @@ igt_main_args("l", NULL, help_str, opt_handler, &data)
>   		run_sharpness_filter_test(&data, TEST_FILTER_TOGGLE);
>   	}
>   
> +	igt_describe("Verify that following a resolution change, "
> +		     "distict taps are selected.");
> +	igt_subtest_with_dynamic("filter-tap") {
> +		data.modifier = DRM_FORMAT_MOD_LINEAR;
> +		data.rotation = IGT_ROTATION_0;
> +		data.format = DRM_FORMAT_XRGB8888;
> +		data.filter_strength = MID_FILTER_STRENGTH;
> +
> +		run_sharpness_filter_test(&data, TEST_FILTER_TAP);
> +	}
> +
>   	igt_fixture {
>   		igt_display_fini(&data.display);
>   		drm_close_driver(data.drm_fd);