Re: [PATCH i-g-t] tests/kms_pipe_crc_basic: Add tests for supported formats

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 18.6.2024 13.42, Santhosh Reddy, Guddati wrote:
> Current kms_pipe_crc_basic supports crc check for planar formats like NV12 and
> XRGB8888, added subtest to validate crc for all the supported formats

This sound like what kms_plane pixel format tests already do. Also this 
subtest overall seems to be in wrong place, kms_pipe_crc_basic is to 
"Tests behaviour of CRC" like it says at top of the file and not to test 
pixel formats.

How does this differ from already available pixel format tests on kms_plane?

/Juha-Pekka

> ---
>   tests/kms_pipe_crc_basic.c | 61 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 61 insertions(+)
> 
> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
> index 0c19745bc..b8a0689e4 100644
> --- a/tests/kms_pipe_crc_basic.c
> +++ b/tests/kms_pipe_crc_basic.c
> @@ -82,6 +82,11 @@
>    *              CRTC does not cause issues.
>    */
>   
> +/**
> + * SUBTEST: compare-crc-sanitycheck-supported-formats
> + * Description: Basic sanity check for CRC mismatches with supported formats.
> + */
> +
>   static bool extended;
>   static enum pipe active_pipes[IGT_MAX_PIPES];
>   static uint32_t last_pipe;
> @@ -357,6 +362,33 @@ static bool pipe_output_combo_valid(igt_display_t *display,
>   
>   data_t data = {0, };
>   
> +/**
> + * @brief Retrieves the supported formats for a given output and pipe.
> + *
> + * This function returns an array of supported formats for a specific output and pipe.
> + *
> + * @param output The output for which to retrieve the supported formats.
> + * @param count  A pointer to store the number of supported formats.
> + * @param pipe   The pipe for which to retrieve the supported formats.
> + *
> + * @return An array of supported formats for the given output and pipe.
> + */
> +static uint32_t* get_supported_formats(igt_output_t *output, uint32_t *count, enum pipe pipe)
> +{
> +	igt_plane_t *primary;
> +	uint32_t *formats = NULL;
> +
> +	igt_output_set_pipe(output, pipe);
> +	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> +
> +	*count = primary->format_mod_count;
> +	formats = (uint32_t *) malloc(sizeof(uint32_t) * (*count + 1));
> +	for (uint32_t i = 0; i < *count; i++) {
> +		formats[i] = primary->formats[i];
> +	}
> +	return formats;
> +}
> +
>   static int opt_handler(int opt, int opt_index, void *_data)
>   {
>   	switch (opt) {
> @@ -377,6 +409,8 @@ igt_main_args("e", NULL, help_str, opt_handler, NULL)
>   {
>   	enum pipe pipe;
>   	igt_output_t *output;
> +	uint32_t *supported_formats = NULL;
> +	uint32_t count = 0;
>   	struct {
>   		const char *name;
>   		unsigned flags;
> @@ -498,6 +532,33 @@ igt_main_args("e", NULL, help_str, opt_handler, NULL)
>   		}
>   	}
>   
> +	igt_describe("Basic sanity check for CRC mismatches with supported formats");
> +	igt_subtest_with_dynamic("compare-crc-sanitycheck-supported-formats") {
> +		for_each_pipe_with_single_output(&data.display, pipe, output) {
> +			if (simulation_constraint(pipe))
> +				continue;
> +
> +			if(!pipe_output_combo_valid(&data.display, pipe, output))
> +				continue;
> +
> +			supported_formats = get_supported_formats(output, &count, pipe);
> +			for (i=0; i<count; i++) {
> +
> +				// Skip the formats that are not supported
> +				if (supported_formats[i] == DRM_FORMAT_NV12 || supported_formats[i] == DRM_FORMAT_XRGB8888 \
> +						|| supported_formats[i] == DRM_FORMAT_C8) {
> +					igt_info("Skipping format "IGT_FORMAT_FMT" \n", IGT_FORMAT_ARGS(supported_formats[i]));
> +					continue;
> +				}
> +
> +				igt_info("Testing format "IGT_FORMAT_FMT" \n", IGT_FORMAT_ARGS(supported_formats[i]));
> +				igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(pipe), output->name) {
> +					test_compare_crc(&data, pipe, output, supported_formats[i]);
> +				}
> +			}
> +		}
> +	}
> +
>   	igt_fixture {
>   		igt_display_fini(&data.display);
>   		drm_close_driver(data.drm_fd);