tests/kms_plane: Generate reference CRCs for partial coverage too

From: Lyude Paul <[email protected]>

There's been a TODO sitting in the kms_plane test suite for a while now as a placeholder for actually generating a reference CRC for plane-position-hole tests. This means we have never actually been verifying whether or not partially covering our hole with a hardware plane works displays properly, and have just simply been checking that the frame CRC doesn't change when we're not changing the display output. On nouveau at least, this has been causing us to pass these tests even when we don't correctly program our hardware plane's framebuffer.

So, get rid of that TODO and implement this by converting convert_fb_for_mode__position() to a generic convert_fb_for_mode() function that allows us to create a colored FB, either with or without a series of 64x64 rectangles, and use that in test_grab_crc() to generate reference CRCs for the plane-position-hole tests. Additionally, we move around all of the test flags into a single enumerator, and make sure none of them have overlapping bits so we can correctly tell from test_grab_crc() whether or not our reference CRC should include rectangles (otherwise, we'll accidentally add rectangles in our reference frame for the plane panning tests).

Then, we finally flip the TEST_POSITION_FULLY_COVERED enum to TEST_POSITION_PARTIALLY_COVERED, so tests which don't explicitly specify partial positioning don't get it in their reference fbs.

v2:
* Rebase

Signed-off-by Lyude Paul <[email protected]>

---
 tests/kms_plane.c | 164 +++++++++++++++++++++++++---------------------
 1 file changed, 91 insertions(+), 73 deletions(-)

tests/kms_plane.c +13 -0

index 430210d8..1b5e7fe9 100644
@@ -44,6 +44,11 @@ typedef struct {
 	float blue;
 } color_t;
 
+typedef struct {
+	int x, y;
+	color_t color;
+} rectangle_t;
+
 typedef struct {
 	int drm_fd;
 	igt_display_t display;

@@ -71,9 +76,52 @@ static void test_fini(data_t *data)
 	igt_pipe_crc_free(data->pipe_crc);
 }
 
+enum {
+	TEST_POSITION_PARTIALLY_COVERED = 1 << 0,
+	TEST_DPMS                       = 1 << 1,
+	TEST_PANNING_TOP_LEFT           = 1 << 2,
+	TEST_PANNING_BOTTOM_RIGHT       = 1 << 3,
+	TEST_SUSPEND_RESUME             = 1 << 4,
+};
+

tests/kms_plane.c +26 -7

+	unsigned int fb_id;
+	cairo_t *cr;
+
+	fb_id = igt_create_fb(data->drm_fd,
+			      mode->hdisplay, mode->vdisplay,
+			      DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE,
+			      fb);
+	igt_assert_fd(fb_id);
+
+	cr = igt_get_cairo_ctx(data->drm_fd, fb);
+	igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay,
+			fb_color->red, fb_color->green, fb_color->blue);
+	for (int i = 0; i < rect_cnt; i++) {
+		const rectangle_t *rect = &rects[i];
+		igt_paint_color(cr,
+				rect->x, rect->y, 64, 64,
+				rect->color.red,
+				rect->color.green,
+				rect->color.blue);
+	}
+
+	igt_put_cairo_ctx(cr);
+}
+
 static void
 test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
-	      color_t *fb_color, igt_crc_t *crc /* out */)
+	      color_t *fb_color, unsigned int flags, igt_crc_t *crc /* out */)
 {
 	struct igt_fb fb;
 	drmModeModeInfo *mode;

@@ -86,13 +134,19 @@ test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
 	primary = igt_output_get_plane(output, 0);
 
 	mode = igt_output_get_mode(output);
-	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
-			    DRM_FORMAT_XRGB8888,
-			    LOCAL_DRM_FORMAT_MOD_NONE,
-			    fb_color->red, fb_color->green, fb_color->blue,
-			    &fb);
-	igt_plane_set_fb(primary, &fb);

tests/kms_plane.c +14 -39

+	}
 
+	igt_plane_set_fb(primary, &fb);
 	ret = igt_display_try_commit2(&data->display, COMMIT_LEGACY);
 	igt_skip_on(ret != 0);
 

@@ -104,19 +158,24 @@ test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
 	igt_remove_fb(data->drm_fd, &fb);
 
 	crc_str = igt_crc_to_string(crc);
-	igt_debug("CRC for a (%.02f,%.02f,%.02f) fb: %s\n", fb_color->red,
-		  fb_color->green, fb_color->blue, crc_str);
+	igt_debug("CRC for a %s covered (%.02f,%.02f,%.02f) fb: %s\n",
+		  flags & TEST_POSITION_PARTIALLY_COVERED ? "partially" : "fully",
+		  fb_color->red, fb_color->green, fb_color->blue,
+		  crc_str);
 	free(crc_str);
 }
 
 /*
  * Plane position test.
- *   - We start by grabbing a reference CRC of a full green fb being scanned
- *     out on the primary plane
+ *   - For testing positions that fully cover our hole, we start by grabbing a
+ *     reference CRC of a full green fb being scanned out on the primary plane.
+ *     For testing positions that only partially cover our hole, we instead use
+ *     a full green fb with a partially covered black rectangle.
  *   - Then we scannout 2 planes:
  *      - the primary plane uses a green fb with a black rectangle
  *      - a plane, on top of the primary plane, with a green fb that is set-up
- *        to cover the black rectangle of the primary plane fb
+ *        to fully or partially cover the black rectangle of the primary plane
+ *        fb
  *     The resulting CRC should be identical to the reference CRC
  */
 

@@ -125,38 +184,6 @@ typedef struct {
 	igt_crc_t reference_crc;
 } test_position_t;
 
-/*
- * create a green fb with a black rectangle at (rect_x,rect_y) and of size
- * (rect_w,rect_h)
- */
-static void
-create_fb_for_mode__position(data_t *data, drmModeModeInfo *mode,
-			     double rect_x, double rect_y,
-			     double rect_w, double rect_h,
-			     struct igt_fb *fb /* out */)
-{
-	unsigned int fb_id;
-	cairo_t *cr;
-
-	fb_id = igt_create_fb(data->drm_fd,
-				  mode->hdisplay, mode->vdisplay,
-				  DRM_FORMAT_XRGB8888,
-				  LOCAL_DRM_FORMAT_MOD_NONE,
-				  fb);
-	igt_assert(fb_id);
-
-	cr = igt_get_cairo_ctx(data->drm_fd, fb);
-	igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay,
-			    0.0, 1.0, 0.0);
-	igt_paint_color(cr, rect_x, rect_y, rect_w, rect_h, 0.0, 0.0, 0.0);
-	igt_put_cairo_ctx(cr);
-}
-
-enum {
-	TEST_POSITION_FULLY_COVERED = 1 << 0,
-	TEST_DPMS = 1 << 1,
-};
-
 static void
 test_plane_position_with_output(data_t *data,
 				enum pipe pipe,

@@ -165,6 +192,7 @@ test_plane_position_with_output(data_t *data,
 				igt_crc_t *reference_crc,
 				unsigned int flags)
 {
+	rectangle_t rect = { .x = 100, .y = 100, .color = { 0.0, 0.0, 0.0 }};
 	igt_plane_t *primary, *sprite;
 	struct igt_fb primary_fb, sprite_fb;
 	drmModeModeInfo *mode;

@@ -179,26 +207,26 @@ test_plane_position_with_output(data_t *data,
 	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
 	sprite = igt_output_get_plane(output, plane);
 
-	create_fb_for_mode__position(data, mode, 100, 100, 64, 64,
-				     &primary_fb);
+	create_fb_for_mode(data, mode, &green, &rect, 1, &primary_fb);
 	igt_plane_set_fb(primary, &primary_fb);
 
 	igt_create_color_fb(data->drm_fd,

tests/kms_plane.c +0 -3

 	igt_plane_set_fb(sprite, &sprite_fb);
 
-	if (flags & TEST_POSITION_FULLY_COVERED)
-		igt_plane_set_position(sprite, 100, 100);
-	else

tests/kms_plane.c +6 -18

 	if (flags & TEST_DPMS) {
 		kmstest_set_connector_dpms(data->drm_fd,

@@ -211,12 +239,6 @@ test_plane_position_with_output(data_t *data,
 
 	igt_pipe_crc_collect_crc(data->pipe_crc, &crc2);
 
-	if (flags & TEST_POSITION_FULLY_COVERED)
-		igt_assert_crc_equal(reference_crc, &crc);
-	else {
-		;/* FIXME: missing reference CRCs */
-	}
-
 	igt_assert_crc_equal(&crc, &crc2);
 
 	igt_plane_set_fb(primary, NULL);

@@ -237,8 +259,7 @@ test_plane_position(data_t *data, enum pipe pipe, unsigned int flags)
 	igt_require(output);
 
 	test_init(data, pipe);
-
-	test_grab_crc(data, output, pipe, &green, &reference_crc);
+	test_grab_crc(data, output, pipe, &green, flags, &reference_crc);
 
 	for (int plane = 1; plane < n_planes; plane++)
 		test_plane_position_with_output(data, pipe, plane,

@@ -287,12 +308,6 @@ create_fb_for_mode__panning(data_t *data, drmModeModeInfo *mode,
 	igt_put_cairo_ctx(cr);
 }
 
-enum {
-	TEST_PANNING_TOP_LEFT	  = 1 << 0,
-	TEST_PANNING_BOTTOM_RIGHT = 1 << 1,
-	TEST_SUSPEND_RESUME	  = 1 << 2,
-};
-
 static void
 test_plane_panning_with_output(data_t *data,
 			       enum pipe pipe,

@@ -355,8 +370,8 @@ test_plane_panning(data_t *data, enum pipe pipe, unsigned int flags)
 
 	test_init(data, pipe);
 
-	test_grab_crc(data, output, pipe, &red, &red_crc);
-	test_grab_crc(data, output, pipe, &blue, &blue_crc);
+	test_grab_crc(data, output, pipe, &red, flags, &red_crc);
+	test_grab_crc(data, output, pipe, &blue, flags, &blue_crc);
 
 	for (int plane = 1; plane < n_planes; plane++)
 		test_plane_panning_with_output(data, pipe, plane, output,

@@ -961,15 +976,18 @@ run_tests_for_pipe_plane(data_t *data, enum pipe pipe)
 	data->crop = 0;
 	igt_subtest_f("plane-position-covered-pipe-%s-planes",
 		      kmstest_pipe_name(pipe))
-		test_plane_position(data, pipe, TEST_POSITION_FULLY_COVERED);
+		test_plane_position(data, pipe, 0);
 
 	igt_subtest_f("plane-position-hole-pipe-%s-planes",
 		      kmstest_pipe_name(pipe))
-		test_plane_position(data, pipe, 0);
+		test_plane_position(data, pipe,
+				    TEST_POSITION_PARTIALLY_COVERED);

Quoted 67 lines · Lyude

From: Lyude Paul <[email protected]>

There's been a TODO sitting in the kms_plane test suite for a while now as a placeholder for actually generating a reference CRC for plane-position-hole tests. This means we have never actually been verifying whether or not partially covering our hole with a hardware plane works displays properly, and have just simply been checking that the frame CRC doesn't change when we're not changing the display output. On nouveau at least, this has been causing us to pass these tests even when we don't correctly program our hardware plane's framebuffer.

So, get rid of that TODO and implement this by converting convert_fb_for_mode__position() to a generic convert_fb_for_mode() function that allows us to create a colored FB, either with or without a series of 64x64 rectangles, and use that in test_grab_crc() to generate reference CRCs for the plane-position-hole tests. Additionally, we move around all of the test flags into a single enumerator, and make sure none of them have overlapping bits so we can correctly tell from test_grab_crc() whether or not our reference CRC should include rectangles (otherwise, we'll accidentally add rectangles in our reference frame for the plane panning tests).

Then, we finally flip the TEST_POSITION_FULLY_COVERED enum to TEST_POSITION_PARTIALLY_COVERED, so tests which don't explicitly specify partial positioning don't get it in their reference fbs.

v2:
* Rebase

Signed-off-by Lyude Paul <[email protected]>

---
 tests/kms_plane.c | 164 +++++++++++++++++++++++++---------------------
 1 file changed, 91 insertions(+), 73 deletions(-)

tests/kms_plane.c +15 -0

index 430210d8..1b5e7fe9 100644
@@ -44,6 +44,11 @@ typedef struct {
 	float blue;
 } color_t;
 
+typedef struct {
+	int x, y;
+	color_t color;
+} rectangle_t;
+
 typedef struct {
 	int drm_fd;
 	igt_display_t display;

@@ -71,9 +76,52 @@ static void test_fini(data_t *data)
 	igt_pipe_crc_free(data->pipe_crc);
 }
 
+enum {
+	TEST_POSITION_PARTIALLY_COVERED = 1 << 0,
+	TEST_DPMS                       = 1 << 1,
+	TEST_PANNING_TOP_LEFT           = 1 << 2,
+	TEST_PANNING_BOTTOM_RIGHT       = 1 << 3,
+	TEST_SUSPEND_RESUME             = 1 << 4,
+};
+
+/*
+ * create a colored fb, possibly with a series of 64x64 colored rectangles

(used
+ * for position tests)
+ */

tests/kms_plane.c static void test_fini(data_t *data) · line 96

Quoted 6 lines

tests/kms_plane.c +6 -0

+static void
+create_fb_for_mode(data_t *data, drmModeModeInfo *mode,
+		   color_t *fb_color,
+		   const rectangle_t *rects, int rect_cnt,
+		   struct igt_fb *fb /* out */)
+{

It looks like data is only ever used to access the data->drm_fd. It's real nit-picky, but it might be nice to accept the fd to line up the API with igt_create_fb() and friends for the sake of familiarity and consistency. The others also return the fb_id or possible error code, although it does seem to be ignored by callers so...

After more reviewing I notice this was the prior API so I'd consider this comment even more nit-picky.

Quoted 43 lines

tests/kms_plane.c +26 -7

+	unsigned int fb_id;
+	cairo_t *cr;
+
+	fb_id = igt_create_fb(data->drm_fd,
+			      mode->hdisplay, mode->vdisplay,
+			      DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE,
+			      fb);
+	igt_assert_fd(fb_id);
+
+	cr = igt_get_cairo_ctx(data->drm_fd, fb);
+	igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay,
+			fb_color->red, fb_color->green, fb_color->blue);
+	for (int i = 0; i < rect_cnt; i++) {
+		const rectangle_t *rect = &rects[i];
+		igt_paint_color(cr,
+				rect->x, rect->y, 64, 64,
+				rect->color.red,
+				rect->color.green,
+				rect->color.blue);
+	}
+
+	igt_put_cairo_ctx(cr);
+}
+
 static void
 test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
-	      color_t *fb_color, igt_crc_t *crc /* out */)
+	      color_t *fb_color, unsigned int flags, igt_crc_t *crc /* out */)
 {
 	struct igt_fb fb;
 	drmModeModeInfo *mode;

@@ -86,13 +134,19 @@ test_grab_crc(data_t *data, igt_output_t *output, enum
 pipe pipe,
 	primary = igt_output_get_plane(output, 0);
 
 	mode = igt_output_get_mode(output);
-	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
-			    DRM_FORMAT_XRGB8888,
-			    LOCAL_DRM_FORMAT_MOD_NONE,
-			    fb_color->red, fb_color->green, fb_color->blue,
-			    &fb);
-	igt_plane_set_fb(primary, &fb);

tests/kms_plane.c test_grab_crc(data_t *data, igt_output_t *output, enum · line 147

Quoted 10 lines

tests/kms_plane.c +10 -0

+	if (flags & TEST_POSITION_PARTIALLY_COVERED) {
+		static const rectangle_t rects[] = {
+			{ .x = 100, .y = 100, .color = { 0.0, 0.0, 0.0 }},
+			{ .x = 132, .y = 132, .color = { 0.0, 1.0, 0.0 }},
+		};
+
+		create_fb_for_mode(data, mode, fb_color,
+				   rects, ARRAY_SIZE(rects), &fb);
+	} else {
+		create_fb_for_mode(data, mode, fb_color, NULL, 0, &fb);

If I understand correctly, this is equivalent to a call to igt_create_color_fb(). If that is indeed the case, it might be clearer to just call that here.

Quoted 100 lines

tests/kms_plane.c +6 -3

+	}
 
+	igt_plane_set_fb(primary, &fb);
 	ret = igt_display_try_commit2(&data->display, COMMIT_LEGACY);
 	igt_skip_on(ret != 0);

@@ -104,19 +158,24 @@ test_grab_crc(data_t *data, igt_output_t *output, enum
 pipe pipe,
 	igt_remove_fb(data->drm_fd, &fb);
 
 	crc_str = igt_crc_to_string(crc);
-	igt_debug("CRC for a (%.02f,%.02f,%.02f) fb: %s\n", fb_color->red,
-		  fb_color->green, fb_color->blue, crc_str);
+	igt_debug("CRC for a %s covered (%.02f,%.02f,%.02f) fb: %s\n",
+		  flags & TEST_POSITION_PARTIALLY_COVERED ? "partially" :
 "fully",
+		  fb_color->red, fb_color->green, fb_color->blue,
+		  crc_str);
 	free(crc_str);
 }
 
 /*
  * Plane position test.
- *   - We start by grabbing a reference CRC of a full green fb being

scanned
- * out on the primary plane
+ * - For testing positions that fully cover our hole, we start by
grabbing a
+ * reference CRC of a full green fb being scanned out on the primary
plane.
+ * For testing positions that only partially cover our hole, we instead
use
+ * a full green fb with a partially covered black rectangle.
* - Then we scannout 2 planes:
* - the primary plane uses a green fb with a black rectangle
* - a plane, on top of the primary plane, with a green fb that is
set-up
- * to cover the black rectangle of the primary plane fb
+ * to fully or partially cover the black rectangle of the primary
plane
+ * fb
* The resulting CRC should be identical to the reference CRC
*/

tests/kms_plane.c +2 -34

@@ -125,38 +184,6 @@ typedef struct {
 	igt_crc_t reference_crc;
 } test_position_t;
 
-/*
- * create a green fb with a black rectangle at (rect_x,rect_y) and of size
- * (rect_w,rect_h)
- */
-static void
-create_fb_for_mode__position(data_t *data, drmModeModeInfo *mode,
-			     double rect_x, double rect_y,
-			     double rect_w, double rect_h,
-			     struct igt_fb *fb /* out */)
-{
-	unsigned int fb_id;
-	cairo_t *cr;
-
-	fb_id = igt_create_fb(data->drm_fd,
-				  mode->hdisplay, mode->vdisplay,
-				  DRM_FORMAT_XRGB8888,
-				  LOCAL_DRM_FORMAT_MOD_NONE,
-				  fb);
-	igt_assert(fb_id);
-
-	cr = igt_get_cairo_ctx(data->drm_fd, fb);
-	igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay,
-			    0.0, 1.0, 0.0);
-	igt_paint_color(cr, rect_x, rect_y, rect_w, rect_h, 0.0, 0.0, 0.0);
-	igt_put_cairo_ctx(cr);
-}
-
-enum {
-	TEST_POSITION_FULLY_COVERED = 1 << 0,
-	TEST_DPMS = 1 << 1,
-};
-
 static void
 test_plane_position_with_output(data_t *data,
 				enum pipe pipe,

@@ -165,6 +192,7 @@ test_plane_position_with_output(data_t *data,
 				igt_crc_t *reference_crc,
 				unsigned int flags)
 {
+	rectangle_t rect = { .x = 100, .y = 100, .color = { 0.0, 0.0, 0.0 }};
 	igt_plane_t *primary, *sprite;
 	struct igt_fb primary_fb, sprite_fb;
 	drmModeModeInfo *mode;

@@ -179,26 +207,26 @@ test_plane_position_with_output(data_t *data,
 	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
 	sprite = igt_output_get_plane(output, plane);
 
-	create_fb_for_mode__position(data, mode, 100, 100, 64, 64,
-				     &primary_fb);
+	create_fb_for_mode(data, mode, &green, &rect, 1, &primary_fb);
 	igt_plane_set_fb(primary, &primary_fb);
 

tests/kms_plane.c test_plane_position_with_output(data_t *data, · line 218

Quoted 11 lines

tests/kms_plane.c +5 -5

 	igt_create_color_fb(data->drm_fd,
-				64, 64, /* width, height */
-				DRM_FORMAT_XRGB8888,
-				LOCAL_DRM_FORMAT_MOD_NONE,
-				0.0, 1.0, 0.0,
-				&sprite_fb);
+			    64, 64, /* width, height */
+			    DRM_FORMAT_XRGB8888,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    0.0, 1.0, 0.0,
+			    &sprite_fb);

I'd include any whitespace-only changes separately, but don't know what the project's preferences are there.

Quoted 6 lines

tests/kms_plane.c +0 -3

 	igt_plane_set_fb(sprite, &sprite_fb);
 
-	if (flags & TEST_POSITION_FULLY_COVERED)
-		igt_plane_set_position(sprite, 100, 100);
-	else

tests/kms_plane.c test_plane_position_with_output(data_t *data, · line 230

Quoted 10 lines

tests/kms_plane.c +4 -0

+	if (flags & TEST_POSITION_PARTIALLY_COVERED)
 		igt_plane_set_position(sprite, 132, 132);
+	else
+		igt_plane_set_position(sprite, 100, 100);
 
 	igt_display_commit(&data->display);
 
 	igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
+	igt_assert_crc_equal(reference_crc, &crc);
 

🎉

Quoted 64 lines

tests/kms_plane.c +1 -8

 	if (flags & TEST_DPMS) {
 		kmstest_set_connector_dpms(data->drm_fd,

@@ -211,12 +239,6 @@ test_plane_position_with_output(data_t *data,
 
 	igt_pipe_crc_collect_crc(data->pipe_crc, &crc2);
 
-	if (flags & TEST_POSITION_FULLY_COVERED)
-		igt_assert_crc_equal(reference_crc, &crc);
-	else {
-		;/* FIXME: missing reference CRCs */
-	}
-
 	igt_assert_crc_equal(&crc, &crc2);
 
 	igt_plane_set_fb(primary, NULL);

@@ -237,8 +259,7 @@ test_plane_position(data_t *data, enum pipe pipe,
 unsigned int flags)
 	igt_require(output);
 
 	test_init(data, pipe);
-
-	test_grab_crc(data, output, pipe, &green, &reference_crc);
+	test_grab_crc(data, output, pipe, &green, flags, &reference_crc);
 
 	for (int plane = 1; plane < n_planes; plane++)

tests/kms_plane.c +0 -6

 		test_plane_position_with_output(data, pipe, plane,

@@ -287,12 +308,6 @@ create_fb_for_mode__panning(data_t *data,
 drmModeModeInfo *mode,
 	igt_put_cairo_ctx(cr);
 }
 
-enum {
-	TEST_PANNING_TOP_LEFT	  = 1 << 0,
-	TEST_PANNING_BOTTOM_RIGHT = 1 << 1,
-	TEST_SUSPEND_RESUME	  = 1 << 2,
-};
-
 static void
 test_plane_panning_with_output(data_t *data,

tests/kms_plane.c +2 -2

 			       enum pipe pipe,

@@ -355,8 +370,8 @@ test_plane_panning(data_t *data, enum pipe pipe,
 unsigned int flags)
 
 	test_init(data, pipe);
 
-	test_grab_crc(data, output, pipe, &red, &red_crc);
-	test_grab_crc(data, output, pipe, &blue, &blue_crc);
+	test_grab_crc(data, output, pipe, &red, flags, &red_crc);
+	test_grab_crc(data, output, pipe, &blue, flags, &blue_crc);
 
 	for (int plane = 1; plane < n_planes; plane++)

tests/kms_plane.c +3 -2

 		test_plane_panning_with_output(data, pipe, plane, output,

@@ -961,15 +976,18 @@ run_tests_for_pipe_plane(data_t *data, enum pipe pipe)
 	data->crop = 0;
 	igt_subtest_f("plane-position-covered-pipe-%s-planes",
 		      kmstest_pipe_name(pipe))
-		test_plane_position(data, pipe, TEST_POSITION_FULLY_COVERED);
+		test_plane_position(data, pipe, 0);
 
 	igt_subtest_f("plane-position-hole-pipe-%s-planes",
 		      kmstest_pipe_name(pipe))
-		test_plane_position(data, pipe, 0);
+		test_plane_position(data, pipe,
+				    TEST_POSITION_PARTIALLY_COVERED);

tests/kms_plane.c run_tests_for_pipe_plane(data_t *data, enum pipe pipe) · line 993

Quoted 10 lines

tests/kms_plane.c +3 -1

 
 	igt_subtest_f("plane-position-hole-dpms-pipe-%s-planes",
 		      kmstest_pipe_name(pipe))
-		test_plane_position(data, pipe, TEST_DPMS);
+		test_plane_position(data, pipe,
+				    TEST_POSITION_PARTIALLY_COVERED |
+				    TEST_DPMS);
 
 	igt_subtest_f("plane-panning-top-left-pipe-%s-planes",
 		      kmstest_pipe_name(pipe))

Very minor nits, overall it looks good to me:

Reviewed-by Jeremy Cline <[email protected]>

Quoted 242 lines · Jeremy Cline

Quoted 67 lines · Lyude

From: Lyude Paul <[email protected]>

There's been a TODO sitting in the kms_plane test suite for a while now as a placeholder for actually generating a reference CRC for plane-position-hole tests. This means we have never actually been verifying whether or not partially covering our hole with a hardware plane works displays properly, and have just simply been checking that the frame CRC doesn't change when we're not changing the display output. On nouveau at least, this has been causing us to pass these tests even when we don't correctly program our hardware plane's framebuffer.

So, get rid of that TODO and implement this by converting convert_fb_for_mode__position() to a generic convert_fb_for_mode() function that allows us to create a colored FB, either with or without a series of 64x64 rectangles, and use that in test_grab_crc() to generate reference CRCs for the plane-position-hole tests. Additionally, we move around all of the test flags into a single enumerator, and make sure none of them have overlapping bits so we can correctly tell from test_grab_crc() whether or not our reference CRC should include rectangles (otherwise, we'll accidentally add rectangles in our reference frame for the plane panning tests).

Then, we finally flip the TEST_POSITION_FULLY_COVERED enum to TEST_POSITION_PARTIALLY_COVERED, so tests which don't explicitly specify partial positioning don't get it in their reference fbs.

v2:
* Rebase

Signed-off-by Lyude Paul <[email protected]>

---
 tests/kms_plane.c | 164 +++++++++++++++++++++++++---------------------
 1 file changed, 91 insertions(+), 73 deletions(-)

tests/kms_plane.c +15 -0

index 430210d8..1b5e7fe9 100644
@@ -44,6 +44,11 @@ typedef struct {
 	float blue;
 } color_t;
 
+typedef struct {
+	int x, y;
+	color_t color;
+} rectangle_t;
+
 typedef struct {
 	int drm_fd;
 	igt_display_t display;

@@ -71,9 +76,52 @@ static void test_fini(data_t *data)
 	igt_pipe_crc_free(data->pipe_crc);
 }
 
+enum {
+	TEST_POSITION_PARTIALLY_COVERED = 1 << 0,
+	TEST_DPMS                       = 1 << 1,
+	TEST_PANNING_TOP_LEFT           = 1 << 2,
+	TEST_PANNING_BOTTOM_RIGHT       = 1 << 3,
+	TEST_SUSPEND_RESUME             = 1 << 4,
+};
+
+/*
+ * create a colored fb, possibly with a series of 64x64 colored rectangles

(used
+ * for position tests)
+ */

tests/kms_plane.c static void test_fini(data_t *data) · line 96

Quoted 6 lines

tests/kms_plane.c +6 -0

+static void
+create_fb_for_mode(data_t *data, drmModeModeInfo *mode,
+		   color_t *fb_color,
+		   const rectangle_t *rects, int rect_cnt,
+		   struct igt_fb *fb /* out */)
+{

It looks like data is only ever used to access the data->drm_fd. It's real nit-picky, but it might be nice to accept the fd to line up the API with igt_create_fb() and friends for the sake of familiarity and consistency. The others also return the fb_id or possible error code, although it does seem to be ignored by callers so...

After more reviewing I notice this was the prior API so I'd consider this comment even more nit-picky.

Quoted 43 lines

tests/kms_plane.c +26 -7

+	unsigned int fb_id;
+	cairo_t *cr;
+
+	fb_id = igt_create_fb(data->drm_fd,
+			      mode->hdisplay, mode->vdisplay,
+			      DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE,
+			      fb);
+	igt_assert_fd(fb_id);
+
+	cr = igt_get_cairo_ctx(data->drm_fd, fb);
+	igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay,
+			fb_color->red, fb_color->green, fb_color->blue);
+	for (int i = 0; i < rect_cnt; i++) {
+		const rectangle_t *rect = &rects[i];
+		igt_paint_color(cr,
+				rect->x, rect->y, 64, 64,
+				rect->color.red,
+				rect->color.green,
+				rect->color.blue);
+	}
+
+	igt_put_cairo_ctx(cr);
+}
+
 static void
 test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
-	      color_t *fb_color, igt_crc_t *crc /* out */)
+	      color_t *fb_color, unsigned int flags, igt_crc_t *crc /* out */)
 {
 	struct igt_fb fb;
 	drmModeModeInfo *mode;

@@ -86,13 +134,19 @@ test_grab_crc(data_t *data, igt_output_t *output, enum
 pipe pipe,
 	primary = igt_output_get_plane(output, 0);
 
 	mode = igt_output_get_mode(output);
-	igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
-			    DRM_FORMAT_XRGB8888,
-			    LOCAL_DRM_FORMAT_MOD_NONE,
-			    fb_color->red, fb_color->green, fb_color->blue,
-			    &fb);
-	igt_plane_set_fb(primary, &fb);

tests/kms_plane.c test_grab_crc(data_t *data, igt_output_t *output, enum · line 147

Quoted 10 lines

tests/kms_plane.c +10 -0

+	if (flags & TEST_POSITION_PARTIALLY_COVERED) {
+		static const rectangle_t rects[] = {
+			{ .x = 100, .y = 100, .color = { 0.0, 0.0, 0.0 }},
+			{ .x = 132, .y = 132, .color = { 0.0, 1.0, 0.0 }},
+		};
+
+		create_fb_for_mode(data, mode, fb_color,
+				   rects, ARRAY_SIZE(rects), &fb);
+	} else {
+		create_fb_for_mode(data, mode, fb_color, NULL, 0, &fb);

If I understand correctly, this is equivalent to a call to igt_create_color_fb(). If that is indeed the case, it might be clearer to just call that here.

Quoted 100 lines

tests/kms_plane.c +6 -3

+	}
 
+	igt_plane_set_fb(primary, &fb);
 	ret = igt_display_try_commit2(&data->display, COMMIT_LEGACY);
 	igt_skip_on(ret != 0);

@@ -104,19 +158,24 @@ test_grab_crc(data_t *data, igt_output_t *output, enum
 pipe pipe,
 	igt_remove_fb(data->drm_fd, &fb);
 
 	crc_str = igt_crc_to_string(crc);
-	igt_debug("CRC for a (%.02f,%.02f,%.02f) fb: %s\n", fb_color->red,
-		  fb_color->green, fb_color->blue, crc_str);
+	igt_debug("CRC for a %s covered (%.02f,%.02f,%.02f) fb: %s\n",
+		  flags & TEST_POSITION_PARTIALLY_COVERED ? "partially" :
 "fully",
+		  fb_color->red, fb_color->green, fb_color->blue,
+		  crc_str);
 	free(crc_str);
 }
 
 /*
  * Plane position test.
- *   - We start by grabbing a reference CRC of a full green fb being

scanned
- * out on the primary plane
+ * - For testing positions that fully cover our hole, we start by
grabbing a
+ * reference CRC of a full green fb being scanned out on the primary
plane.
+ * For testing positions that only partially cover our hole, we instead
use
+ * a full green fb with a partially covered black rectangle.
* - Then we scannout 2 planes:
* - the primary plane uses a green fb with a black rectangle
* - a plane, on top of the primary plane, with a green fb that is
set-up
- * to cover the black rectangle of the primary plane fb
+ * to fully or partially cover the black rectangle of the primary
plane
+ * fb
* The resulting CRC should be identical to the reference CRC
*/

tests/kms_plane.c +2 -34

@@ -125,38 +184,6 @@ typedef struct {
 	igt_crc_t reference_crc;
 } test_position_t;
 
-/*
- * create a green fb with a black rectangle at (rect_x,rect_y) and of size
- * (rect_w,rect_h)
- */
-static void
-create_fb_for_mode__position(data_t *data, drmModeModeInfo *mode,
-			     double rect_x, double rect_y,
-			     double rect_w, double rect_h,
-			     struct igt_fb *fb /* out */)
-{
-	unsigned int fb_id;
-	cairo_t *cr;
-
-	fb_id = igt_create_fb(data->drm_fd,
-				  mode->hdisplay, mode->vdisplay,
-				  DRM_FORMAT_XRGB8888,
-				  LOCAL_DRM_FORMAT_MOD_NONE,
-				  fb);
-	igt_assert(fb_id);
-
-	cr = igt_get_cairo_ctx(data->drm_fd, fb);
-	igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay,
-			    0.0, 1.0, 0.0);
-	igt_paint_color(cr, rect_x, rect_y, rect_w, rect_h, 0.0, 0.0, 0.0);
-	igt_put_cairo_ctx(cr);
-}
-
-enum {
-	TEST_POSITION_FULLY_COVERED = 1 << 0,
-	TEST_DPMS = 1 << 1,
-};
-
 static void
 test_plane_position_with_output(data_t *data,
 				enum pipe pipe,

@@ -165,6 +192,7 @@ test_plane_position_with_output(data_t *data,
 				igt_crc_t *reference_crc,
 				unsigned int flags)
 {
+	rectangle_t rect = { .x = 100, .y = 100, .color = { 0.0, 0.0, 0.0 }};
 	igt_plane_t *primary, *sprite;
 	struct igt_fb primary_fb, sprite_fb;
 	drmModeModeInfo *mode;

@@ -179,26 +207,26 @@ test_plane_position_with_output(data_t *data,
 	primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
 	sprite = igt_output_get_plane(output, plane);
 
-	create_fb_for_mode__position(data, mode, 100, 100, 64, 64,
-				     &primary_fb);
+	create_fb_for_mode(data, mode, &green, &rect, 1, &primary_fb);
 	igt_plane_set_fb(primary, &primary_fb);
 

Quoted 14 lines

tests/kms_plane.c test_plane_position_with_output(data_t *data, · line 218

Quoted 11 lines

tests/kms_plane.c +5 -5

 	igt_create_color_fb(data->drm_fd,
-				64, 64, /* width, height */
-				DRM_FORMAT_XRGB8888,
-				LOCAL_DRM_FORMAT_MOD_NONE,
-				0.0, 1.0, 0.0,
-				&sprite_fb);
+			    64, 64, /* width, height */
+			    DRM_FORMAT_XRGB8888,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    0.0, 1.0, 0.0,
+			    &sprite_fb);

I'd include any whitespace-only changes separately, but don't know what the project's preferences are there.

FWIW - I think it's usually fine as long as you're already modifying the code around the changes for not-whitespace related reasons, we're not usually as strict about this sort of thing with igt as the linux kernel is