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

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

[Jeevan B]

*** BLURB HERE ***

Nidhi Gupta (1):
  tests/kms_writeback: Test cleanup

Zbigniew Kempczyński (1):
  lib/intel_memory_region: Remove function which returns batch size in
    regions

 lib/i915/intel_memory_region.c | 14 ---------
 lib/i915/intel_memory_region.h |  1 -
 tests/kms_writeback.c          | 52 ++++++++++------------------------
 3 files changed, 15 insertions(+), 52 deletions(-)

-- 
2.36.0

[igt-dev] [PATCH 1/2] lib/intel_memory_region: Remove function which returns batch size in regions

[Jeevan B]

From: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>

Function gem_get_batch_size() is confusing as it might return different
size thus deceiving reader of object real size created in different
memory regions.

Let's remove it and enforce igt user to be aware that kernel may alter
size where it is necessary.

v2: reword commit message (Kamil)

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/i915/intel_memory_region.c | 14 --------------
 lib/i915/intel_memory_region.h |  1 -
 2 files changed, 15 deletions(-)

diff --git a/lib/i915/intel_memory_region.c b/lib/i915/intel_memory_region.c
index 568bace9..d80cb3a0 100644
--- a/lib/i915/intel_memory_region.c
+++ b/lib/i915/intel_memory_region.c
@@ -92,20 +92,6 @@ const char *get_memory_region_name(uint32_t region)
 	igt_assert_f(false, "Unknown memory region");
 }
 
-/**
- *  gem_get_batch_size:
- *  @fd: open i915 drm file descriptor
- *  @mem_region_type: used memory_region type
- *
- *  With introduction of LMEM we observe different page sizes for those two
- *  memory regions. Without this helper funtion we may be prone to forget
- *  about setting proper page size.
- */
-uint32_t gem_get_batch_size(int fd, uint8_t mem_region_type)
-{
-	return (mem_region_type == I915_MEMORY_CLASS_DEVICE) ? 65536 : 4096;
-}
-
 /**
  * gem_get_query_memory_regions:
  * @fd: open i915 drm file descriptor
diff --git a/lib/i915/intel_memory_region.h b/lib/i915/intel_memory_region.h
index fd04df83..425bda0e 100644
--- a/lib/i915/intel_memory_region.h
+++ b/lib/i915/intel_memory_region.h
@@ -56,7 +56,6 @@
 bool gem_has_query_support(int fd);
 
 const char *get_memory_region_name(uint32_t region);
-uint32_t gem_get_batch_size(int fd, uint8_t mem_region_type);
 
 struct drm_i915_query_memory_regions *gem_get_query_memory_regions(int fd);
 
-- 
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

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

[Nidhi Gupta]

From: Jeevan B <jeevan.b@intel.com>

*** BLURB HERE ***

Nidhi Gupta (1):
  tests/kms_writeback: Test cleanup

Zbigniew Kempczyński (1):
  lib/intel_memory_region: Remove function which returns batch size in
    regions

 lib/i915/intel_memory_region.c | 14 ---------
 lib/i915/intel_memory_region.h |  1 -
 tests/kms_writeback.c          | 52 ++++++++++------------------------
 3 files changed, 15 insertions(+), 52 deletions(-)

-- 
2.36.0

[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 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 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);

[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 | 58 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 15 deletions(-)

-- 
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 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 0/2] tests/kms_writeback: Test Cleanup

[Nidhi Gupta]

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

 tests/kms_writeback.c | 65 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 16 deletions(-)

-- 
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

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
>