Re: [igt-dev] [PATCH i-g-t 5/8] tests/i915/kms_flip_tiling: Generalize away copy-pasta

On Mon, Oct 18, 2021 at 12:32:18PM +0530, Karthik B S wrote:

On 10/14/2021 3:47 AM, Ville Syrjala wrote:

quoted

From: Ville Syrjälä <redacted>

Replace the huge swaths of copypasta by just looping over the
set of modifiers reported by the plane, and testing each against
the others (and itself).

Signed-off-by: Ville Syrjälä <redacted>
---
  tests/i915/kms_flip_tiling.c | 209 +++++------------------------------
  1 file changed, 30 insertions(+), 179 deletions(-)

diff --git a/tests/i915/kms_flip_tiling.c b/tests/i915/kms_flip_tiling.c
index 63e0b8b9648f..e2e6619baeca 100644
--- a/tests/i915/kms_flip_tiling.c
+++ b/tests/i915/kms_flip_tiling.c

@@ -161,195 +161,46 @@ igt_main
  		igt_display_require(&data.display, data.drm_fd);
  	}
  
-	/*
-	 * Test that a page flip from a tiled buffer to a linear one works
-	 * correctly. First, it sets the crtc with the linear buffer and
-	 * generates a reference crc for the pipe. Then, the crtc is set with
-	 * the tiled one and page flip to the linear one issued. A new crc is
-	 * generated and compared to the reference one.
-	 */
-
-	igt_describe("Check pageflip from tiled buffer to linear one works correctly with x tiling");
-	igt_subtest_with_dynamic("flip-changes-tiling") {
-		uint64_t modifier[2] = { I915_FORMAT_MOD_X_TILED,
-					 DRM_FORMAT_MOD_LINEAR };
+	igt_describe("Check pageflip between modifiers");
+	igt_subtest_with_dynamic("flip-change-tiling") {
  		enum pipe pipe;
  
-		for (int i = 0; i < ARRAY_SIZE(modifier); i++)
-			igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
-
  		for_each_pipe_with_valid_output(&data.display, pipe, output) {
-			igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
-				test_flip_tiling(&data, pipe, output, modifier);
-			test_cleanup(&data, pipe, output);
-		}
-	}
-
-	igt_describe("Check pageflip from tiled buffer to linear one works correctly with y tiling");
-	igt_subtest_with_dynamic("flip-changes-tiling-Y") {
-		uint64_t modifier[2] = { I915_FORMAT_MOD_Y_TILED,
-					 DRM_FORMAT_MOD_LINEAR };
-		enum pipe pipe;
+			igt_plane_t *plane;
  
-		igt_require_fb_modifiers(data.drm_fd);
+			igt_output_set_pipe(output, pipe);
  
-		for (int i = 0; i < ARRAY_SIZE(modifier); i++)
-			igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
+			plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
  
-		igt_require(data.gen >= 9);
+			for (int i = 0; i < plane->format_mod_count; i++) {
+				if (plane->formats[i] != data.testformat)
+					continue;
  
-		for_each_pipe_with_valid_output(&data.display, pipe, output) {
-			igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
-				test_flip_tiling(&data, pipe, output, modifier);
-			test_cleanup(&data, pipe, output);
-		}
-	}
-
-	igt_describe("Check pageflip from tiled buffer to linear one works correctly with yf tiling");
-	igt_subtest_with_dynamic("flip-changes-tiling-Yf") {
-		uint64_t modifier[2] = { I915_FORMAT_MOD_Yf_TILED,
-					 DRM_FORMAT_MOD_LINEAR };
-		enum pipe pipe;
+				for (int j = 0; j < plane->format_mod_count; j++) {
+					uint64_t modifier[2] = {
+						plane->modifiers[i],
+						plane->modifiers[j],
+					};
  
-		igt_require_fb_modifiers(data.drm_fd);
+					if (plane->formats[j] != data.testformat)
+						continue;

Move this check before assigning modifier[]?

Then I'd have to split the declaration and assignment.

quoted

  
-		for (int i = 0; i < ARRAY_SIZE(modifier); i++)
-			igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
-
-		igt_require(data.gen >= 9);
-
-		for_each_pipe_with_valid_output(&data.display, pipe, output) {
-			igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
-				test_flip_tiling(&data, pipe, output, modifier);
-			test_cleanup(&data, pipe, output);
-		}
-	}
-
-	/*
-	 * Test that a page flip from a tiled buffer to another tiled one works
-	 * correctly. First, it sets the crtc with the tiled buffer and
-	 * generates a reference crc for the pipe. Then a page flip to second
-	 * tiled buffer is issued. A new crc is generated and compared to the
-	 * reference one.
-	 */
-
-	igt_describe("Check pageflip from tiled buffer to another tiled one works correctly with x tiling");
-	igt_subtest_with_dynamic("flip-X-tiled") {
-		uint64_t modifier[2] = { I915_FORMAT_MOD_X_TILED,
-					 I915_FORMAT_MOD_X_TILED };
-		enum pipe pipe;
-
-		for (int i = 0; i < ARRAY_SIZE(modifier); i++)
-			igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
-
-		for_each_pipe_with_valid_output(&data.display, pipe, output) {
-			igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
-				test_flip_tiling(&data, pipe, output, modifier);
-			test_cleanup(&data, pipe, output);
-		}
-	}
-
-	igt_describe("Check pageflip from tiled buffer to another tiled one works correctly with y tiling");
-	igt_subtest_with_dynamic("flip-Y-tiled") {
-		uint64_t modifier[2] = { I915_FORMAT_MOD_Y_TILED,
-					 I915_FORMAT_MOD_Y_TILED };
-		enum pipe pipe;
-
-		igt_require_fb_modifiers(data.drm_fd);
-
-		for (int i = 0; i < ARRAY_SIZE(modifier); i++)
-			igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
-
-		igt_require(data.gen >= 9);
-
-		for_each_pipe_with_valid_output(&data.display, pipe, output) {
-			igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
-				test_flip_tiling(&data, pipe, output, modifier);
-			test_cleanup(&data, pipe, output);
-		}
-	}
-
-	igt_describe("Check pageflip from tiled buffer to another tiled one works correctly with yf tiling");
-	igt_subtest_with_dynamic("flip-Yf-tiled") {
-		uint64_t modifier[2] = { I915_FORMAT_MOD_Yf_TILED,
-					 I915_FORMAT_MOD_Yf_TILED };
-		enum pipe pipe;
+					igt_require(igt_plane_has_format_mod(plane,
+									     data.testformat,
+									     modifier[0]));
+					igt_require(igt_plane_has_format_mod(plane,
+									     data.testformat,
+									     modifier[1]));

Is this still required? We are already only taking modifiers which are 
supported by this format? Please correct me if I'm missing something here.

Not missing anything, these can go. We already checked for this by hand.

Originally I forgot that plane->modifiers[]+plane->formats[] actually
form a SoA tuple, so I just iterated plane->modifiers[] without skipping
anything, thinking that would just iterate each modifier once. And with
that apporach I would have still needed the igt_plane_has_format_mod().
However as I rediscovered the truth I then added the manual
plane->format[] checks which make these redundant.

With these minor updates, the patch looks good to me.

Reviewed-by: Karthik B S <redacted>

Thanks.

-- 
Ville Syrjälä
Intel