[PATCH i-g-t v4 4/4] tests/kms_writeback: Add dump for valid clone mode

[Abhinav Kumar]

Hi IGT maintainers

I have given some comments below so that we can skip the test if no 
valid clones of WB output are found  but at a top level, I wanted to get 
your feedback that if we validate every clone of WB output with it in a 
pair-wise fashion, thats something which works for all vendors?

This will not test the case where lets say WB, output A, output B are 
all clones of each other and we want to do a 3-way clone mode as it will 
only test,

1) WB with output A
2) WB with output B

But atleast it validates all of WB's clones one pair at a time.

So wanted to know if we can make this a generic test-case of 
kms_writeback OR we need to protect this with some cmdline option.

Thanks

Abhinav
On 9/25/2024 1:37 PM, Jessica Zhang wrote:
> From: Esha Bharadwaj <quic_ebharadw at quicinc.com>
> 
> Move the regular writeback dump into its own subtest and add a subtest to
> dump pairs of cloned non-writeback and writeback encoders
> 
> Signed-off-by: Esha Bharadwaj <quic_ebharadw at quicinc.com>
> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
> ---
>   tests/kms_writeback.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 82 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
> index 4155961a0..cdbb9e436 100644
> --- a/tests/kms_writeback.c
> +++ b/tests/kms_writeback.c
> @@ -47,6 +47,13 @@
>   #include "sw_sync.h"
>   
>   /**
> + * SUBTEST: dump-writeback
> + * Description: Dump a non-cloned writeback buffer
> + *
> + * SUBTEST: dump-valid-clones
> + * Description: Dump all valid clone pairs of writeback and non-writeback
> + *              connectors
> + *
>    * SUBTEST: writeback-check-output-XRGB2101010
>    * Description: Check XRGB2101010 writeback output with CRC validation
>    *
> @@ -450,7 +457,7 @@ static void do_single_commit(igt_output_t *output, igt_plane_t *plane, igt_fb_t
>   }
>   
>   static void commit_and_dump_fb(igt_display_t *display, igt_output_t *output, igt_plane_t *plane,
> -			        igt_fb_t *input_fb, drmModeModeInfo *mode)
> +			       igt_fb_t *input_fb, drmModeModeInfo *mode, char *suffix)
>   {
>   	cairo_surface_t *fb_surface_out;
>   	char filepath_out[PATH_MAX];
> @@ -471,7 +478,8 @@ static void commit_and_dump_fb(igt_display_t *display, igt_output_t *output, igt
>   	do_single_commit(output, plane, input_fb, &output_fb);
>   
>   	fb_surface_out = igt_get_cairo_surface(display->drm_fd, &output_fb);
> -	snprintf(filepath_out, PATH_MAX, "%s/%s.png", path_name, file_name);
> +	snprintf(filepath_out, PATH_MAX, "%s/%s-%s.png", path_name, file_name, suffix);
> +
>   	status = cairo_surface_write_to_png(fb_surface_out, filepath_out);
>   	igt_assert_eq(status, CAIRO_STATUS_SUCCESS);
>   	cairo_surface_destroy(fb_surface_out);
> @@ -555,6 +563,7 @@ igt_main_args("b:c:f:dl", long_options, help_str, opt_handler, NULL)
>   	drmModeModeInfo mode;
>   	unsigned int fb_id;
>   	int ret;
> +	char dump_suffix[PATH_MAX];
>   
>   	memset(&display, 0, sizeof(display));
>   
> @@ -603,9 +612,78 @@ igt_main_args("b:c:f:dl", long_options, help_str, opt_handler, NULL)
>   
>   		if (data.list_modes)
>   			list_writeback_modes(&display);
> -		if (data.dump_check)
> -			commit_and_dump_fb(&display, output, plane, &input_fb, &mode);
>   	}
> +
> +	/*
> +	 * When dump_check is high, the following subtests will be run.
> +	 * These tests will be skipped if list_modes flag is high.
> +	 */
> +	igt_describe("Dump a non-cloned writeback buffer");
> +	igt_subtest("dump-writeback") {
> +		igt_skip_on(!data.dump_check || data.list_modes);
> +		snprintf(dump_suffix, PATH_MAX, "encoder%d", output->id);
> +		commit_and_dump_fb(&display, output, plane, &input_fb, &mode, dump_suffix);
> +	}
> +
> +	igt_describe("Dump valid clone pairs of writeback and non-writeback connectors");
> +	igt_subtest("dump-valid-clones") {
> +		igt_output_t *nonwb_output;
> +		enum pipe curr_pipe;
> +		drmModeModeInfo *nonwb_mode;
> +		igt_fb_t clone_fb;
> +
> +		/*
> +		 * Drop the dump flag check once calling writeback_sequence() is
> +		 * supported
> +		 */
> +		igt_skip_on(!data.dump_check || data.list_modes);
> +		igt_skip_on_f(!(data.supported_colors & XRGB8888), "DRM_FORMAT_XRGB8888 is unsupported\n");
> +		igt_require(fb_id > 0);
> +		for_each_output(&display, nonwb_output) {
> +			if ((nonwb_output->config.connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
> +					|| igt_output_clone_valid(display.drm_fd, output, nonwb_output))
> +				continue;
> +
> +			/* Set up non-writeback cloned output */
> +			curr_pipe = output->pending_pipe;
> +			nonwb_mode = igt_output_get_mode(nonwb_output);
> +
> +			igt_output_set_pipe(nonwb_output, curr_pipe);
> +			igt_output_override_mode(output, nonwb_mode);
> +
> +			fb_id = igt_create_fb(display.drm_fd, nonwb_mode->hdisplay,
> +				      nonwb_mode->vdisplay,
> +				      DRM_FORMAT_XRGB8888,
> +				      DRM_FORMAT_MOD_LINEAR,
> +				      &clone_fb);
> +			igt_assert(fb_id >= 0);
> +
> +			/*
> +			 * Currently, only dump the resulting frame. This is
> +			 * because the igt_fb_get_fnv1a_crc() is unable to
> +			 * calculate CRCs for non-32 bit aligned resolutions.
> +			 *
> +			 * Once there is a workaround for this issue, this can
> +			 * be changed to:
> +			 *    if (dump_check flag set)
> +			 *        commit_and_dump_fb()
> +			 *    else
> +			 *        writeback_sequence()
> +			 */

With 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/27ba1f4756f12a3694dce6d456aed947f22a8e34 
merged, this comment is no longer true.

We should drop the dump_check restriction for clones test.

We should also skip this test if there were no possible_clones found for 
the WB output.