[igt-dev] [PATCH 0/2] tests/kms_writeback: Test cleanup

[igt-dev] [PATCH 0/2] tests/kms_writeback: Test cleanup

[Nidhi Gupta]

*** BLURB HERE ***

Nidhi Gupta (2):
  tests/kms_writeback: Convert tests to dynamic
  tests/kms_writeback: Test cleanup

 tests/kms_writeback.c | 59 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 15 deletions(-)

-- 
2.36.0

[igt-dev] [PATCH 1/2] tests/kms_writeback: Convert tests to dynamic

[Nidhi Gupta]

Convert the existing subtests to dynamic subtests.

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
---
 tests/kms_writeback.c | 57 +++++++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 15 deletions(-)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index 9d134585..3781fa34 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -214,32 +214,38 @@ static void test_invalid_parameters(igt_output_t *output, igt_fb_t *valid_fb, ig
 		uint32_t fb_id;
 		bool ptr_valid;
 		int32_t *out_fence_ptr;
+		const char *name;
 	} invalid_tests[] = {
 		{
 			/* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */
 			.fb_id = 0,
 			.ptr_valid = true,
 			.out_fence_ptr = &out_fence,
+			.name = "WRITEBACK_OUT_FENCE_PTR-set",
 		},
 		{
 			/* Invalid output buffer. */
 			.fb_id = invalid_fb->fb_id,
 			.ptr_valid = true,
 			.out_fence_ptr = &out_fence,
+			.name = "Invalid-output-buffer",
 		},
 		{
 			/* Invalid WRITEBACK_OUT_FENCE_PTR. */
 			.fb_id = valid_fb->fb_id,
 			.ptr_valid = false,
 			.out_fence_ptr = (int32_t *)0x8,
+			.name = "Invalid-WRITEBACK_OUT_FENCE_PTR",
 		},
 	};
 
 	for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) {
-		ret = do_writeback_test(output, invalid_tests[i].fb_id,
-					invalid_tests[i].out_fence_ptr,
-					invalid_tests[i].ptr_valid);
-		igt_assert(ret != 0);
+		igt_dynamic_f("%s", invalid_tests[i].name) {
+			ret = do_writeback_test(output, invalid_tests[i].fb_id,
+						invalid_tests[i].out_fence_ptr,
+						invalid_tests[i].ptr_valid);
+			igt_assert(ret != 0);
+		}
 	}
 }
 
@@ -247,18 +253,39 @@ static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *
 {
 
 	int ret;
+	struct {
+		const char *name;
+		uint32_t fb_id;
+		int i, expected_ret;
+	} fb_id_tests[] = {
 
-	/* Invalid object for WRITEBACK_FB_ID */
-	ret = do_writeback_test(output, output->id, NULL, false);
-	igt_assert(ret == -EINVAL);
+		{
+			.name = "Invalid-object",
+			.fb_id = output->id,
+			.expected_ret = -EINVAL,
+		},
+
+		{
+			.name = "Zero-WRITEBACK_FB_ID",
+			.fb_id = 0,
+			.expected_ret = 0,
+		},
+
+		{
+			.name = "Valid-output-buffer",
+			.fb_id = valid_fb->fb_id,
+			.expected_ret = 0,
+		},
+	};
+
+	for (int i = 0; i < ARRAY_SIZE(fb_id_tests); i++) {
+		igt_dynamic_f("%s", fb_id_tests[i].name) {
+			ret = do_writeback_test(output, fb_id_tests[i].fb_id, NULL, false);
+			igt_assert(ret == fb_id_tests[i].expected_ret);
+		}
+	}
 
-	/* Zero WRITEBACK_FB_ID */
-	ret = do_writeback_test(output, 0, NULL, false);
-	igt_assert(ret == 0);
 
-	/* Valid output buffer */
-	ret = do_writeback_test(output, valid_fb->fb_id, NULL, false);
-	igt_assert(ret == 0);
 }
 
 static void fill_fb(igt_fb_t *fb, uint32_t pixel)
@@ -553,7 +580,7 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	igt_describe("Writeback has a couple of parameters linked together"
 		     "(output framebuffer and fence); this test goes through"
 		     "the combination of possible bad options");
-	igt_subtest("writeback-invalid-parameters") {
+	igt_subtest_with_dynamic("writeback-invalid-parameters") {
 		igt_fb_t invalid_output_fb;
 
 		igt_skip_on(data.dump_check || data.list_modes);
@@ -570,7 +597,7 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	}
 
 	igt_describe("Validate WRITEBACK_FB_ID with valid and invalid options");
-	igt_subtest("writeback-fb-id") {
+	igt_subtest_with_dynamic("writeback-fb-id") {
 		igt_fb_t output_fb;
 
 		igt_skip_on(data.dump_check || data.list_modes);
-- 
2.36.0

[igt-dev] [PATCH 2/2] tests/kms_writeback: Test cleanup

[Nidhi Gupta]

Sanitize the system state before starting the subtest.

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
---
 tests/kms_writeback.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index 3781fa34..5149e302 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -521,6 +521,8 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 
 		kmstest_set_vt_graphics_mode();
 
+		igt_display_reset(display);
+
 		igt_display_require(&display, display.drm_fd);
 
 		igt_require(display.is_atomic);
-- 
2.36.0

Re: [igt-dev] [PATCH 2/2] tests/kms_writeback: Test cleanup

[Modem, Bhanuprakash]

On Thu-22-09-2022 06:28 am, Nidhi Gupta wrote:
> Sanitize the system state before starting the subtest.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>   tests/kms_writeback.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
> index 3781fa34..5149e302 100644
> --- a/tests/kms_writeback.c
> +++ b/tests/kms_writeback.c
> @@ -521,6 +521,8 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
>   
>   		kmstest_set_vt_graphics_mode();
>   
> +		igt_display_reset(display);
> +
>   		igt_display_require(&display, display.drm_fd);

Please drop this I can see igt_display_require() is already present.
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/tests/kms_writeback.c#n493


Found some other cleanups:

Example:
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/tests/kms_writeback.c#n113
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/tests/kms_writeback.c#n144

Why do we need hard-coded values for override_mode? Can't we use default 
mode?


https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/tests/kms_writeback.c#n536
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/tests/kms_writeback.c#n559
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/tests/kms_writeback.c#n576
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/tests/kms_writeback.c#n592

As this check is common in all subtests, please move this to igt_fixture

- Bhanu

>   
>   		igt_require(display.is_atomic);

Re: [igt-dev] [PATCH 1/2] tests/kms_writeback: Convert tests to dynamic

[Modem, Bhanuprakash]

On Thu-22-09-2022 06:28 am, Nidhi Gupta wrote:
> Convert the existing subtests to dynamic subtests.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>   tests/kms_writeback.c | 57 +++++++++++++++++++++++++++++++------------
>   1 file changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
> index 9d134585..3781fa34 100644
> --- a/tests/kms_writeback.c
> +++ b/tests/kms_writeback.c
> @@ -214,32 +214,38 @@ static void test_invalid_parameters(igt_output_t *output, igt_fb_t *valid_fb, ig
>   		uint32_t fb_id;
>   		bool ptr_valid;
>   		int32_t *out_fence_ptr;
> +		const char *name;
>   	} invalid_tests[] = {
>   		{
>   			/* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */
>   			.fb_id = 0,
>   			.ptr_valid = true,
>   			.out_fence_ptr = &out_fence,
> +			.name = "WRITEBACK_OUT_FENCE_PTR-set",

Please avoid mixed case, also use "-" instead of "_".

This comment is applicable to all places.

>   		},
>   		{
>   			/* Invalid output buffer. */
>   			.fb_id = invalid_fb->fb_id,
>   			.ptr_valid = true,
>   			.out_fence_ptr = &out_fence,
> +			.name = "Invalid-output-buffer",
>   		},
>   		{
>   			/* Invalid WRITEBACK_OUT_FENCE_PTR. */
>   			.fb_id = valid_fb->fb_id,
>   			.ptr_valid = false,
>   			.out_fence_ptr = (int32_t *)0x8,
> +			.name = "Invalid-WRITEBACK_OUT_FENCE_PTR",
>   		},
>   	};
>   
>   	for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) {
> -		ret = do_writeback_test(output, invalid_tests[i].fb_id,
> -					invalid_tests[i].out_fence_ptr,
> -					invalid_tests[i].ptr_valid);
> -		igt_assert(ret != 0);
> +		igt_dynamic_f("%s", invalid_tests[i].name) {
> +			ret = do_writeback_test(output, invalid_tests[i].fb_id,
> +						invalid_tests[i].out_fence_ptr,
> +						invalid_tests[i].ptr_valid);
> +			igt_assert(ret != 0);
> +		}
>   	}
>   }
>   
> @@ -247,18 +253,39 @@ static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *
>   {
>   
>   	int ret;
> +	struct {
> +		const char *name;
> +		uint32_t fb_id;
> +		int i, expected_ret;
> +	} fb_id_tests[] = {
>   
> -	/* Invalid object for WRITEBACK_FB_ID */
> -	ret = do_writeback_test(output, output->id, NULL, false);
> -	igt_assert(ret == -EINVAL);
> +		{
> +			.name = "Invalid-object",
> +			.fb_id = output->id,
> +			.expected_ret = -EINVAL,
> +		},
> +

Newlines are not required.

> +		{
> +			.name = "Zero-WRITEBACK_FB_ID",
> +			.fb_id = 0,
> +			.expected_ret = 0,
> +		},
> +
> +		{
> +			.name = "Valid-output-buffer",
> +			.fb_id = valid_fb->fb_id,
> +			.expected_ret = 0,
> +		},
> +	};
> +
> +	for (int i = 0; i < ARRAY_SIZE(fb_id_tests); i++) {
> +		igt_dynamic_f("%s", fb_id_tests[i].name) {
> +			ret = do_writeback_test(output, fb_id_tests[i].fb_id, NULL, false);
> +			igt_assert(ret == fb_id_tests[i].expected_ret);
> +		}
> +	}
>   
> -	/* Zero WRITEBACK_FB_ID */
> -	ret = do_writeback_test(output, 0, NULL, false);
> -	igt_assert(ret == 0);
>   
> -	/* Valid output buffer */
> -	ret = do_writeback_test(output, valid_fb->fb_id, NULL, false);
> -	igt_assert(ret == 0);
>   }
>   
>   static void fill_fb(igt_fb_t *fb, uint32_t pixel)
> @@ -553,7 +580,7 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
>   	igt_describe("Writeback has a couple of parameters linked together"
>   		     "(output framebuffer and fence); this test goes through"
>   		     "the combination of possible bad options");
> -	igt_subtest("writeback-invalid-parameters") {
> +	igt_subtest_with_dynamic("writeback-invalid-parameters") {
>   		igt_fb_t invalid_output_fb;
>   
>   		igt_skip_on(data.dump_check || data.list_modes);
> @@ -570,7 +597,7 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
>   	}
>   
>   	igt_describe("Validate WRITEBACK_FB_ID with valid and invalid options");
> -	igt_subtest("writeback-fb-id") {
> +	igt_subtest_with_dynamic("writeback-fb-id") {
>   		igt_fb_t output_fb;
>   
>   		igt_skip_on(data.dump_check || data.list_modes);

[igt-dev] ✗ Fi.CI.BUILD: failure for tests/kms_writeback: Test Cleanup (rev3)

[Patchwork]

== Series Details ==

Series: tests/kms_writeback: Test Cleanup (rev3)
URL   : https://patchwork.freedesktop.org/series/107764/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
129bea58b9424f0a0da995311a219fdf57732594 kms_flip_event_leak: Create dynamic subtests

[325/672] Linking target tests/kms_dp_aux_dev
[326/672] Linking target tests/kms_display_modes
[327/672] Linking target tests/kms_flip
[328/672] Linking target tests/kms_dp_tiled_display
[329/672] Linking target tests/kms_invalid_mode
[330/672] Linking target tests/kms_hdmi_inject
[331/672] Linking target tests/kms_flip_event_leak
[332/672] Linking target benchmarks/gem_blt
[333/672] Linking target benchmarks/gem_busy
[334/672] Linking target tests/kms_getfb
[335/672] Linking target tests/kms_hdr
[336/672] Linking target tests/kms_dsc
[337/672] Linking target tests/gen3_render_tiledy_blits
[338/672] Linking target tests/kms_lease
[339/672] Linking target tests/kms_multipipe_modeset
[340/672] Linking target tests/kms_panel_fitting
[341/672] Linking target tests/kms_pipe_crc_basic
[342/672] Linking target tests/kms_plane_cursor
[343/672] Linking target tests/kms_plane
[344/672] Linking target tests/kms_plane_alpha_blend
[345/672] Linking target tests/kms_plane_lowres
[346/672] Linking target tests/kms_plane_scaling
[347/672] Linking target tests/kms_plane_multiple
[348/672] Linking target tests/kms_prop_blob
[349/672] Linking target tests/kms_prime
[350/672] Linking target tests/kms_rotation_crc
[351/672] Linking target tests/kms_properties
[352/672] Linking target tests/kms_rmfb
[353/672] Linking target tests/kms_scaling_modes
[354/672] Linking target tests/kms_selftest
[355/672] Linking target tests/kms_sequence
[356/672] Linking target tests/kms_vrr
[357/672] Linking target tests/kms_setmode
[358/672] Linking target tests/kms_sysfs_edid_timing
[359/672] Linking target tests/kms_tv_load_detect
[360/672] Linking target tests/kms_universal_plane
[361/672] Linking target tests/kms_vblank
[362/672] Compiling C object tests/kms_writeback.p/kms_writeback.c.o
FAILED: tests/kms_writeback.p/kms_writeback.c.o 
ccache cc -Itests/kms_writeback.p -Itests -I../tests -I../include/drm-uapi -I../include/linux-uapi -Ilib -I../lib -I../lib/stubs/syscalls -I. -I.. -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/libpng12 -I/usr/include/libdrm -I/usr/include/libdrm/nouveau -I/usr/include/x86_64-linux-gnu -I/usr/include/alsa -I/usr/include -I/home/cidrm/kernel_headers/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu11 -O2 -g -D_GNU_SOURCE -include config.h -D_FORTIFY_SOURCE=2 -Wbad-function-cast -Wdeclaration-after-statement -Wformat=2 -Wimplicit-fallthrough=0 -Wlogical-op -Wmissing-declarations -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wold-style-definition -Wpointer-arith -Wredundant-decls -Wshadow -Wstrict-prototypes -Wuninitialized -Wunused -Wno-clobbered -Wno-maybe-uninitialized -Wno-missing-field-initializers -Wno-pointer-arith -Wno-sign-compare -Wno-type-limits -Wno-unused-parameter -Wno-unused-result -Werror=address -Werror=array-bounds -Werror=implicit -Werror=init-self -Werror=int-to-pointer-cast -Werror=main -Werror=missing-braces -Werror=nonnull -Werror=pointer-to-int-cast -Werror=return-type -Werror=sequence-point -Werror=trigraphs -Werror=write-strings -fno-builtin-malloc -fno-builtin-calloc -fcommon -pthread -MD -MQ tests/kms_writeback.p/kms_writeback.c.o -MF tests/kms_writeback.p/kms_writeback.c.o.d -o tests/kms_writeback.p/kms_writeback.c.o -c ../tests/kms_writeback.c
../tests/kms_writeback.c: In function ‘__igt_unique____real_main507’:
../tests/kms_writeback.c:524:21: error: incompatible type for argument 1 of ‘igt_display_reset’
   igt_display_reset(display);
                     ^~~~~~~
In file included from ../lib/igt.h:38,
                 from ../tests/kms_writeback.c:31:
../lib/igt_kms.h:455:39: note: expected ‘igt_display_t *’ {aka ‘struct igt_display *’} but argument is of type ‘igt_display_t’ {aka ‘struct igt_display’}
 void igt_display_reset(igt_display_t *display);
                        ~~~~~~~~~~~~~~~^~~~~~~
ninja: build stopped: subcommand failed.

[igt-dev] [PATCH 2/2] tests/kms_writeback: Test cleanup

[Nidhi Gupta]

v1: move igt_skip_on(data.dump_check || data.list_modes)
    to igt_fixture as it is common for all subtests.
    (Bhanu)
v2: replaced hard coded mode with default mode.
    (Bhanu)

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
---
 tests/kms_writeback.c | 52 +++++++++++++------------------------------
 1 file changed, 15 insertions(+), 37 deletions(-)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index 9d134585..8846d6d8 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -107,46 +107,27 @@ static bool check_writeback_config(igt_display_t *display, igt_output_t *output,
 
 static igt_output_t *kms_writeback_get_output(igt_display_t *display)
 {
-	int i;
 	enum pipe pipe;
+	igt_output_t *output;
 
-	drmModeModeInfo override_mode = {
-		.clock = 25175,
-		.hdisplay = 640,
-		.hsync_start = 656,
-		.hsync_end = 752,
-		.htotal = 800,
-		.hskew = 0,
-		.vdisplay = 480,
-		.vsync_start = 490,
-		.vsync_end = 492,
-		.vtotal = 525,
-		.vscan = 0,
-		.vrefresh = 60,
-		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
-		.name = {"640x480-60"},
-	};
-
-	for (i = 0; i < display->n_outputs; i++) {
-		igt_output_t *output = &display->outputs[i];
+	drmModeModeInfo override_mode;
 
+	for_each_pipe_with_valid_output(display, pipe, output) {
+		igt_output_set_pipe(output, pipe);
 		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
 			continue;
 
-		for_each_pipe(display, pipe) {
-			igt_output_set_pipe(output, pipe);
+		override_mode = *igt_output_get_mode(output);
 
-			if (data.custom_mode)
-				override_mode = data.user_mode;
-			if (data.builtin_mode)
-				override_mode = output->config.connector->modes[data.mode_index];
-
-			if (check_writeback_config(display, output, override_mode)) {
-				igt_debug("Using connector %u:%s on pipe %d\n",
-					  output->config.connector->connector_id,
-					  output->name, pipe);
+		if (data.custom_mode)
+			override_mode = data.user_mode;
+		if (data.builtin_mode)
+			override_mode = output->config.connector->modes[data.mode_index];
+		if (check_writeback_config(display, output, override_mode)) {
+			igt_debug("Using connector %u:%s on pipe %d\n",
+				   output->config.connector->connector_id,
+				   output->name, pipe);
 				return output;
-			}
 		}
 
 		igt_debug("We found %u:%s, but this test will not be able to use it.\n",
@@ -155,7 +136,6 @@ static igt_output_t *kms_writeback_get_output(igt_display_t *display)
 		/* Restore any connectors we don't use, so we don't trip on them later */
 		kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED);
 	}
-
 	return NULL;
 }
 
@@ -498,6 +478,8 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 
 		igt_require(display.is_atomic);
 
+		igt_skip_on(data.dump_check || data.list_modes);
+
 		output = kms_writeback_get_output(&display);
 		igt_require(output);
 
@@ -533,7 +515,6 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 		drmModePropertyBlobRes *formats_blob;
 		const char *valid_chars;
 
-		igt_skip_on(data.dump_check || data.list_modes);
 		formats_blob = get_writeback_formats_blob(output);
 		valid_chars = "01234568 ABCGNRUVXY";
 
@@ -556,7 +537,6 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	igt_subtest("writeback-invalid-parameters") {
 		igt_fb_t invalid_output_fb;
 
-		igt_skip_on(data.dump_check || data.list_modes);
 		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay / 2,
 				      mode.vdisplay / 2,
 				      DRM_FORMAT_XRGB8888,
@@ -573,7 +553,6 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	igt_subtest("writeback-fb-id") {
 		igt_fb_t output_fb;
 
-		igt_skip_on(data.dump_check || data.list_modes);
 		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
 				      DRM_FORMAT_XRGB8888,
 				      DRM_FORMAT_MOD_LINEAR,
@@ -589,7 +568,6 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	igt_subtest("writeback-check-output") {
 		igt_fb_t output_fb;
 
-		igt_skip_on(data.dump_check || data.list_modes);
 		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
 				      DRM_FORMAT_XRGB8888,
 				      igt_fb_mod_to_tiling(0),
-- 
2.36.0

[igt-dev] [PATCH 2/2] tests/kms_writeback: Test cleanup

[Jeevan B]

From: Nidhi Gupta <nidhi1.gupta@intel.com>

v1: move igt_skip_on(data.dump_check || data.list_modes)
    to igt_fixture as it is common for all subtests.
    (Bhanu)
v2: replaced hard coded mode with default mode.
    (Bhanu)

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
---
 tests/kms_writeback.c | 52 +++++++++++++------------------------------
 1 file changed, 15 insertions(+), 37 deletions(-)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index 9d134585..8846d6d8 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -107,46 +107,27 @@ static bool check_writeback_config(igt_display_t *display, igt_output_t *output,
 
 static igt_output_t *kms_writeback_get_output(igt_display_t *display)
 {
-	int i;
 	enum pipe pipe;
+	igt_output_t *output;
 
-	drmModeModeInfo override_mode = {
-		.clock = 25175,
-		.hdisplay = 640,
-		.hsync_start = 656,
-		.hsync_end = 752,
-		.htotal = 800,
-		.hskew = 0,
-		.vdisplay = 480,
-		.vsync_start = 490,
-		.vsync_end = 492,
-		.vtotal = 525,
-		.vscan = 0,
-		.vrefresh = 60,
-		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
-		.name = {"640x480-60"},
-	};
-
-	for (i = 0; i < display->n_outputs; i++) {
-		igt_output_t *output = &display->outputs[i];
+	drmModeModeInfo override_mode;
 
+	for_each_pipe_with_valid_output(display, pipe, output) {
+		igt_output_set_pipe(output, pipe);
 		if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
 			continue;
 
-		for_each_pipe(display, pipe) {
-			igt_output_set_pipe(output, pipe);
+		override_mode = *igt_output_get_mode(output);
 
-			if (data.custom_mode)
-				override_mode = data.user_mode;
-			if (data.builtin_mode)
-				override_mode = output->config.connector->modes[data.mode_index];
-
-			if (check_writeback_config(display, output, override_mode)) {
-				igt_debug("Using connector %u:%s on pipe %d\n",
-					  output->config.connector->connector_id,
-					  output->name, pipe);
+		if (data.custom_mode)
+			override_mode = data.user_mode;
+		if (data.builtin_mode)
+			override_mode = output->config.connector->modes[data.mode_index];
+		if (check_writeback_config(display, output, override_mode)) {
+			igt_debug("Using connector %u:%s on pipe %d\n",
+				   output->config.connector->connector_id,
+				   output->name, pipe);
 				return output;
-			}
 		}
 
 		igt_debug("We found %u:%s, but this test will not be able to use it.\n",
@@ -155,7 +136,6 @@ static igt_output_t *kms_writeback_get_output(igt_display_t *display)
 		/* Restore any connectors we don't use, so we don't trip on them later */
 		kmstest_force_connector(display->drm_fd, output->config.connector, FORCE_CONNECTOR_UNSPECIFIED);
 	}
-
 	return NULL;
 }
 
@@ -498,6 +478,8 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 
 		igt_require(display.is_atomic);
 
+		igt_skip_on(data.dump_check || data.list_modes);
+
 		output = kms_writeback_get_output(&display);
 		igt_require(output);
 
@@ -533,7 +515,6 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 		drmModePropertyBlobRes *formats_blob;
 		const char *valid_chars;
 
-		igt_skip_on(data.dump_check || data.list_modes);
 		formats_blob = get_writeback_formats_blob(output);
 		valid_chars = "01234568 ABCGNRUVXY";
 
@@ -556,7 +537,6 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	igt_subtest("writeback-invalid-parameters") {
 		igt_fb_t invalid_output_fb;
 
-		igt_skip_on(data.dump_check || data.list_modes);
 		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay / 2,
 				      mode.vdisplay / 2,
 				      DRM_FORMAT_XRGB8888,
@@ -573,7 +553,6 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	igt_subtest("writeback-fb-id") {
 		igt_fb_t output_fb;
 
-		igt_skip_on(data.dump_check || data.list_modes);
 		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
 				      DRM_FORMAT_XRGB8888,
 				      DRM_FORMAT_MOD_LINEAR,
@@ -589,7 +568,6 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	igt_subtest("writeback-check-output") {
 		igt_fb_t output_fb;
 
-		igt_skip_on(data.dump_check || data.list_modes);
 		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
 				      DRM_FORMAT_XRGB8888,
 				      igt_fb_mod_to_tiling(0),
-- 
2.36.0

Re: [igt-dev] [PATCH 2/2] tests/kms_writeback: Test cleanup

[Jessica Zhang]

Hi Nidhi,

On 9/8/2022 1:48 AM, Nidhi Gupta wrote:
> Sanitize the system state before starting the subtest.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>   tests/kms_writeback.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
> index 2c5421ce..79a5760f 100644
> --- a/tests/kms_writeback.c
> +++ b/tests/kms_writeback.c
> @@ -186,6 +186,7 @@ static int do_writeback_test(igt_output_t *output, uint32_t fb_id,
>   {
>   	int ret;
>   	igt_display_t *display = output->display;
> +	igt_display_reset(display);

I might not have communicated this clearly in the v1 comments, but 
igt_display_reset() will undo all the setup done in the first 
igt_fixture [1] and disable the CRTCs. Furthermore, it would cause the 
commit in do_writeback_test() [2] to fail, since the CRTC would be 
disabled. If you want to call igt_display_reset(), you'll have to re-do 
the setup within that fixture.

Thanks,

Jessica Zhang

[1] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_writeback.c#L491

[2] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_writeback.c#L198

>   	struct kmstest_connector_config *config = &output->config;
>   
>   	igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id);
> -- 
> 2.36.0
> 

[igt-dev] [PATCH 2/2] tests/kms_writeback: Test cleanup

[Nidhi Gupta]

Sanitize the system state before starting the subtest.

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
---
 tests/kms_writeback.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index 2c5421ce..79a5760f 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -186,6 +186,7 @@ static int do_writeback_test(igt_output_t *output, uint32_t fb_id,
 {
 	int ret;
 	igt_display_t *display = output->display;
+	igt_display_reset(display);
 	struct kmstest_connector_config *config = &output->config;
 
 	igt_output_set_prop_value(output, IGT_CONNECTOR_CRTC_ID, config->crtc->crtc_id);
-- 
2.36.0

Re: [igt-dev] [PATCH 2/2] tests/kms_writeback: Test Cleanup

[Jessica Zhang]

Hi Nidhi,

On 8/25/2022 2:06 PM, Nidhi Gupta wrote:
> Sanitize the system state before starting the subtest.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>   tests/kms_writeback.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
> index 2508b1e1..0c6c64b3 100644
> --- a/tests/kms_writeback.c
> +++ b/tests/kms_writeback.c
> @@ -169,6 +169,7 @@ static void detach_crtc(igt_display_t *display, igt_output_t *output)
>   	if (get_writeback_fb_id(output) == 0)
>   		return;
>   
> +	igt_display_fini(&display);

igt_display_fini() frees the memory for various members of the 
igt_display_t struct [1]. Since there's already a call to 
igt_display_fini() in the same fixture that detach_crtc() is called [2], 
adding an extra call here will cause a double free.

[1] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/lib/igt_kms.c#L2752

[2] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_writeback.c#L607

>   	igt_output_set_pipe(output, PIPE_NONE);
>   	igt_display_commit2(display, COMMIT_ATOMIC);
>   }
> @@ -580,6 +581,9 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
>   		     "the combination of possible bad options");
>   	igt_subtest_with_dynamic("writeback-invalid-parameters") {
>   		igt_fb_t invalid_output_fb;
> +		igt_display_reset(&display);
> +		igt_display_commit(&display);

I have a doubt about this:

There's a lot of set up going on in the initial igt_fixture here [3], 
specifically within kms_writeback_get_output() [4], where the output 
pipe is set.

If you just reset the display then do a commit, the output->pending pipe 
will be set to NONE within igt_output_reset() [5]. This will cause the 
commit to detach the CRTC (in a similar manner to what detach_crtc() 
does [6]).

If you don't redo the setup from the initial igt_fixture after resetting 
the display, the commits within do_writeback_test() will fail since 
there won't be a CRTC attached to the connector.

Thanks,

Jessica Zhang

[3] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_writeback.c#L491

[4] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_writeback.c#L108

[5] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/lib/igt_kms.c#L2180

[6] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_writeback.c#L167

> +
>   
>   		igt_skip_on(data.dump_check || data.list_modes);
>   		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay / 2,
> @@ -597,6 +601,8 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
>   	igt_describe("Validate WRITEBACK_FB_ID with valid and invalid options");
>   	igt_subtest_with_dynamic("writeback-fb-id") {
>   		igt_fb_t output_fb;
> +		igt_display_reset(&display);
> +		igt_display_commit(&display);
>   
>   		igt_skip_on(data.dump_check || data.list_modes);
>   		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
> @@ -613,6 +619,8 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
>   	igt_describe("Check writeback output with CRC validation");
>   	igt_subtest_with_dynamic("writeback-check-output") {
>   		igt_fb_t output_fb;
> +		igt_display_reset(&display);
> +		igt_display_commit(&display);
>   
>   		igt_skip_on(data.dump_check || data.list_modes);
>   		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
> -- 
> 2.36.0
> 

[igt-dev] [PATCH 2/2] tests/kms_writeback: Test Cleanup

[Nidhi Gupta]

Sanitize the system state before starting the subtest.

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
---
 tests/kms_writeback.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c
index 2508b1e1..0c6c64b3 100644
--- a/tests/kms_writeback.c
+++ b/tests/kms_writeback.c
@@ -169,6 +169,7 @@ static void detach_crtc(igt_display_t *display, igt_output_t *output)
 	if (get_writeback_fb_id(output) == 0)
 		return;
 
+	igt_display_fini(&display);
 	igt_output_set_pipe(output, PIPE_NONE);
 	igt_display_commit2(display, COMMIT_ATOMIC);
 }
@@ -580,6 +581,9 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 		     "the combination of possible bad options");
 	igt_subtest_with_dynamic("writeback-invalid-parameters") {
 		igt_fb_t invalid_output_fb;
+		igt_display_reset(&display);
+		igt_display_commit(&display);
+
 
 		igt_skip_on(data.dump_check || data.list_modes);
 		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay / 2,
@@ -597,6 +601,8 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	igt_describe("Validate WRITEBACK_FB_ID with valid and invalid options");
 	igt_subtest_with_dynamic("writeback-fb-id") {
 		igt_fb_t output_fb;
+		igt_display_reset(&display);
+		igt_display_commit(&display);
 
 		igt_skip_on(data.dump_check || data.list_modes);
 		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
@@ -613,6 +619,8 @@ igt_main_args("b:c:dl", long_options, help_str, opt_handler, NULL)
 	igt_describe("Check writeback output with CRC validation");
 	igt_subtest_with_dynamic("writeback-check-output") {
 		igt_fb_t output_fb;
+		igt_display_reset(&display);
+		igt_display_commit(&display);
 
 		igt_skip_on(data.dump_check || data.list_modes);
 		fb_id = igt_create_fb(display.drm_fd, mode.hdisplay, mode.vdisplay,
-- 
2.36.0