[PATCH i-g-t 0/4] Add tests for solid fill planes

[PATCH i-g-t 0/4] Add tests for solid fill planes

[Jessica Zhang]

Add corresponding IGT tests for solid fill planes [1] in kms_atomic.

This series does the following:
- Fix a dangling pipe_crc_t object in the plane_immutable_zpos test
- Add solid_fill and pixel_source to IGT plane property enum
- Change IGT commit behavior to support solid fill planes
- Add dynamic subtests for solid fill planes in kms_atomic

[1] https://patchwork.freedesktop.org/series/110283/

---
Jessica Zhang (4):
      tests/kms_atomic: Free pipe_crc object
      lib: Add support for solid_fill and pixel_source plane properties
      tests/kms_atomic: Add solid fill plane subtest
      tests/kms_atomic: Add subtest for solid fill cursor planes

 lib/igt_kms.c      |  37 ++++++++++++++--
 lib/igt_kms.h      |   4 ++
 tests/kms_atomic.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+), 3 deletions(-)
---
base-commit: ced22f8bf4263ff395dc852c86b682e62a7a1c1b
change-id: 20230810-solid-fill-5acd9b2d42fc

Best regards,
-- 
Jessica Zhang <quic_jesszhan@quicinc.com>

[PATCH i-g-t 0/4] Add tests for solid fill planes

[Jessica Zhang]

Add corresponding IGT tests for solid fill planes [1] in kms_atomic.

This series does the following:
- Fix a dangling pipe_crc_t object in the plane_immutable_zpos test
- Add solid_fill and pixel_source to IGT plane property enum
- Change IGT commit behavior to support solid fill planes
- Add dynamic subtests for solid fill planes in kms_atomic

[1] https://patchwork.freedesktop.org/series/110283/

---
Jessica Zhang (4):
      tests/kms_atomic: Free pipe_crc object
      lib: Add support for solid_fill and pixel_source plane properties
      tests/kms_atomic: Add solid fill plane subtest
      tests/kms_atomic: Add subtest for solid fill cursor planes

 lib/igt_kms.c      |  37 ++++++++++++++--
 lib/igt_kms.h      |   4 ++
 tests/kms_atomic.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+), 3 deletions(-)
---
base-commit: ced22f8bf4263ff395dc852c86b682e62a7a1c1b
change-id: 20230810-solid-fill-5acd9b2d42fc

Best regards,
-- 
Jessica Zhang <quic_jesszhan@quicinc.com>

[PATCH i-g-t 1/4] tests/kms_atomic: Free pipe_crc object

[Jessica Zhang]

Free dangling pipe_crc object in plane_immutable_zpos()

Fixes: 149c3d0b321c ("tests/kms_atomic: Add test to validate immutable zpos")
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 tests/kms_atomic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
index 96cd58571666..2b6e9a8f0383 100644
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -560,6 +560,8 @@ plane_immutable_zpos(data_t *data, igt_output_t *output, enum pipe pipe, int n_p
 		igt_plane_set_fb(plane_upper, NULL);
 	}
 
+	igt_pipe_crc_free(pipe_crc);
+
 	igt_remove_fb(data->drm_fd, &fb_ref);
 	igt_remove_fb(data->drm_fd, &fb_lower);
 	igt_remove_fb(data->drm_fd, &fb_upper);

-- 
2.43.0

[PATCH i-g-t 1/4] tests/kms_atomic: Free pipe_crc object

[Jessica Zhang]

Free dangling pipe_crc object in plane_immutable_zpos()

Fixes: 149c3d0b321c ("tests/kms_atomic: Add test to validate immutable zpos")
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 tests/kms_atomic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
index 96cd58571666..2b6e9a8f0383 100644
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -560,6 +560,8 @@ plane_immutable_zpos(data_t *data, igt_output_t *output, enum pipe pipe, int n_p
 		igt_plane_set_fb(plane_upper, NULL);
 	}
 
+	igt_pipe_crc_free(pipe_crc);
+
 	igt_remove_fb(data->drm_fd, &fb_ref);
 	igt_remove_fb(data->drm_fd, &fb_lower);
 	igt_remove_fb(data->drm_fd, &fb_upper);

-- 
2.43.0

[PATCH i-g-t 2/4] lib: Add support for solid_fill and pixel_source plane properties

[Jessica Zhang]

Add corresponding IGT macros for solid_fill and pixel_source DRM
properties, and change IGT commit behavior to allow for NULL FB commits.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 lib/igt_kms.c | 37 ++++++++++++++++++++++++++++++++++---
 lib/igt_kms.h |  4 ++++
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index e4dea1a604dc..6a339766ca31 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -669,6 +669,8 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 	[IGT_PLANE_HOTSPOT_X] = "HOTSPOT_X",
 	[IGT_PLANE_HOTSPOT_Y] = "HOTSPOT_Y",
 	[IGT_PLANE_FB_ID] = "FB_ID",
+	[IGT_PLANE_SOLID_FILL] = "solid_fill",
+	[IGT_PLANE_PIXEL_SOURCE] = "pixel_source",
 	[IGT_PLANE_CRTC_ID] = "CRTC_ID",
 	[IGT_PLANE_IN_FENCE_FD] = "IN_FENCE_FD",
 	[IGT_PLANE_TYPE] = "type",
@@ -3417,6 +3419,12 @@ static igt_output_t *igt_pipe_get_output(igt_pipe_t *pipe)
 	return NULL;
 }
 
+static uint32_t igt_plane_get_solid_fill_id(igt_plane_t *plane)
+{
+	return plane->values[IGT_PLANE_SOLID_FILL];
+}
+
+
 static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
 {
 	return plane->values[IGT_PLANE_FB_ID];
@@ -3648,7 +3656,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 	struct igt_display *display = primary->pipe->display;
 	igt_output_t *output = igt_pipe_get_output(pipe);
 	drmModeModeInfo *mode;
-	uint32_t fb_id, crtc_id;
+	uint32_t fb_id, solid_fill_id, crtc_id;
 	int ret;
 
 	/* Primary planes can't be windowed when using a legacy commit */
@@ -3665,12 +3673,14 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 
 	crtc_id = pipe->crtc_id;
 	fb_id = output ? igt_plane_get_fb_id(primary) : 0;
-	if (fb_id)
+	solid_fill_id = output ? igt_plane_get_solid_fill_id(primary) : 0;
+
+	if (fb_id || solid_fill_id)
 		mode = igt_output_get_mode(output);
 	else
 		mode = NULL;
 
-	if (fb_id) {
+	if (fb_id || solid_fill_id) {
 		uint32_t src_x = primary->values[IGT_PLANE_SRC_X] >> 16;
 		uint32_t src_y = primary->values[IGT_PLANE_SRC_Y] >> 16;
 
@@ -4935,6 +4945,27 @@ igt_plane_t *igt_output_get_plane_type_index(igt_output_t *output,
 	return igt_pipe_get_plane_type_index(pipe, plane_type, index);
 }
 
+void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
+		int solid_fill_id)
+{
+	igt_pipe_t *pipe = plane->pipe;
+	igt_display_t *display = pipe->display;
+	int width = rect->x2 - rect->x1;
+	int height = rect->y2 - rect->y1;
+
+	igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_ID,
+			solid_fill_id ? pipe->crtc_id : 0);
+	igt_plane_set_prop_value(plane, IGT_PLANE_SOLID_FILL, solid_fill_id);
+
+	plane->gem_handle = 0;
+
+	LOG(display, "%s.%d: plane_set_solid_fill(%d)\n", kmstest_pipe_name(pipe->pipe),
+	    plane->index, solid_fill_id);
+
+	igt_plane_set_position(plane, rect->x1, rect->y1);
+	igt_plane_set_size(plane, width, height);
+}
+
 /**
  * igt_plane_set_fb:
  * @plane: Plane
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index b3882808b42f..9c05272abe1e 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -330,6 +330,8 @@ enum igt_atomic_plane_properties {
 #define IGT_PLANE_COORD_CHANGED_MASK 0xff
 
        IGT_PLANE_FB_ID,
+       IGT_PLANE_SOLID_FILL,
+       IGT_PLANE_PIXEL_SOURCE,
        IGT_PLANE_CRTC_ID,
        IGT_PLANE_IN_FENCE_FD,
        IGT_PLANE_TYPE,
@@ -537,6 +539,8 @@ igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe p
 
 void igt_pipe_request_out_fence(igt_pipe_t *pipe);
 
+void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
+		int solid_fill_id);
 void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
 void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd);
 void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe);

-- 
2.43.0

[PATCH i-g-t 2/4] lib: Add support for solid_fill and pixel_source plane properties

[Jessica Zhang]

Add corresponding IGT macros for solid_fill and pixel_source DRM
properties, and change IGT commit behavior to allow for NULL FB commits.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 lib/igt_kms.c | 37 ++++++++++++++++++++++++++++++++++---
 lib/igt_kms.h |  4 ++++
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index e4dea1a604dc..6a339766ca31 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -669,6 +669,8 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 	[IGT_PLANE_HOTSPOT_X] = "HOTSPOT_X",
 	[IGT_PLANE_HOTSPOT_Y] = "HOTSPOT_Y",
 	[IGT_PLANE_FB_ID] = "FB_ID",
+	[IGT_PLANE_SOLID_FILL] = "solid_fill",
+	[IGT_PLANE_PIXEL_SOURCE] = "pixel_source",
 	[IGT_PLANE_CRTC_ID] = "CRTC_ID",
 	[IGT_PLANE_IN_FENCE_FD] = "IN_FENCE_FD",
 	[IGT_PLANE_TYPE] = "type",
@@ -3417,6 +3419,12 @@ static igt_output_t *igt_pipe_get_output(igt_pipe_t *pipe)
 	return NULL;
 }
 
+static uint32_t igt_plane_get_solid_fill_id(igt_plane_t *plane)
+{
+	return plane->values[IGT_PLANE_SOLID_FILL];
+}
+
+
 static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
 {
 	return plane->values[IGT_PLANE_FB_ID];
@@ -3648,7 +3656,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 	struct igt_display *display = primary->pipe->display;
 	igt_output_t *output = igt_pipe_get_output(pipe);
 	drmModeModeInfo *mode;
-	uint32_t fb_id, crtc_id;
+	uint32_t fb_id, solid_fill_id, crtc_id;
 	int ret;
 
 	/* Primary planes can't be windowed when using a legacy commit */
@@ -3665,12 +3673,14 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
 
 	crtc_id = pipe->crtc_id;
 	fb_id = output ? igt_plane_get_fb_id(primary) : 0;
-	if (fb_id)
+	solid_fill_id = output ? igt_plane_get_solid_fill_id(primary) : 0;
+
+	if (fb_id || solid_fill_id)
 		mode = igt_output_get_mode(output);
 	else
 		mode = NULL;
 
-	if (fb_id) {
+	if (fb_id || solid_fill_id) {
 		uint32_t src_x = primary->values[IGT_PLANE_SRC_X] >> 16;
 		uint32_t src_y = primary->values[IGT_PLANE_SRC_Y] >> 16;
 
@@ -4935,6 +4945,27 @@ igt_plane_t *igt_output_get_plane_type_index(igt_output_t *output,
 	return igt_pipe_get_plane_type_index(pipe, plane_type, index);
 }
 
+void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
+		int solid_fill_id)
+{
+	igt_pipe_t *pipe = plane->pipe;
+	igt_display_t *display = pipe->display;
+	int width = rect->x2 - rect->x1;
+	int height = rect->y2 - rect->y1;
+
+	igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_ID,
+			solid_fill_id ? pipe->crtc_id : 0);
+	igt_plane_set_prop_value(plane, IGT_PLANE_SOLID_FILL, solid_fill_id);
+
+	plane->gem_handle = 0;
+
+	LOG(display, "%s.%d: plane_set_solid_fill(%d)\n", kmstest_pipe_name(pipe->pipe),
+	    plane->index, solid_fill_id);
+
+	igt_plane_set_position(plane, rect->x1, rect->y1);
+	igt_plane_set_size(plane, width, height);
+}
+
 /**
  * igt_plane_set_fb:
  * @plane: Plane
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index b3882808b42f..9c05272abe1e 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -330,6 +330,8 @@ enum igt_atomic_plane_properties {
 #define IGT_PLANE_COORD_CHANGED_MASK 0xff
 
        IGT_PLANE_FB_ID,
+       IGT_PLANE_SOLID_FILL,
+       IGT_PLANE_PIXEL_SOURCE,
        IGT_PLANE_CRTC_ID,
        IGT_PLANE_IN_FENCE_FD,
        IGT_PLANE_TYPE,
@@ -537,6 +539,8 @@ igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe p
 
 void igt_pipe_request_out_fence(igt_pipe_t *pipe);
 
+void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
+		int solid_fill_id);
 void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
 void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd);
 void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe);

-- 
2.43.0

[PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Jessica Zhang]

Add a basic test for solid fill planes.

This test will first commit a single-color framebuffer plane then
a solid fill plane with the same contents. It then validates the solid
fill plane by comparing the resulting CRC with the CRC of the reference
framebuffer commit.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
old mode 100644
new mode 100755
index 2b6e9a8f0383..8f81e65ad84f
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
 	PLANE_RELAX_FB = (1 << 1)
 };
 
+struct solid_fill_blob {
+	uint32_t r;
+	uint32_t g;
+	uint32_t b;
+	uint32_t pad;
+};
+
 static inline int damage_rect_width(struct drm_mode_rect *r)
 {
 	return r->x2 - r->x1;
@@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
 	igt_remove_fb(data->drm_fd, &fb_2);
 }
 
+static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
+{
+	struct drm_mode_create_blob c;
+	struct drm_mode_destroy_blob d;
+	drmModeModeInfo *mode = igt_output_get_mode(output);
+	struct drm_mode_rect rect = { 0 };
+	struct igt_fb ref_fb;
+	igt_pipe_crc_t *pipe_crc;
+	igt_crc_t ref_crc, new_crc;
+	enum pipe pipe = data->pipe->pipe;
+	int height, width;
+	int ret;
+
+	struct solid_fill_blob blob_data = {
+		.r = 0x00000000,
+		.g = 0x00000000,
+		.b = 0xff000000,
+		.pad = 0x0,
+	};
+
+	rect.x1 = 0;
+	rect.x2 = mode->hdisplay;
+	rect.y1 = 0;
+	rect.y2 = mode->vdisplay;
+
+	height = rect.y2 - rect.y1;
+	width = rect.x2 - rect.x1;
+
+	igt_require(igt_plane_has_prop(plane, IGT_PLANE_SOLID_FILL));
+	igt_require(igt_plane_has_prop(plane, IGT_PLANE_PIXEL_SOURCE));
+
+	c.data = (uintptr_t) &blob_data;
+	c.length = sizeof(blob_data);
+
+	pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
+				    IGT_PIPE_CRC_SOURCE_AUTO);
+
+	/* get reference CRC */
+	igt_create_color_fb(data->drm_fd, width, height,
+			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
+			    0.0, 0.0, 1.0, &ref_fb);
+	igt_plane_set_fb(plane, &ref_fb);
+	igt_plane_set_position(plane, rect.x1, rect.y1);
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+
+	igt_pipe_crc_start(pipe_crc);
+	igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
+
+	ret = drmIoctl(data->display.drm_fd, DRM_IOCTL_MODE_CREATEPROPBLOB, &c);
+	igt_assert(ret == 0);
+
+	/* test solid fill plane */
+	igt_plane_set_solid_fill(plane, &rect, c.blob_id);
+	igt_plane_set_prop_enum(plane, IGT_PLANE_PIXEL_SOURCE, "SOLID_FILL");
+
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &new_crc);
+	igt_assert_crc_equal(&ref_crc, &new_crc);
+
+	/* Switch back to FB pixel source */
+	igt_plane_set_prop_value(plane, IGT_PLANE_SOLID_FILL, 0);
+	igt_plane_set_prop_enum(plane, IGT_PLANE_PIXEL_SOURCE, "FB");
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+
+	d.blob_id = c.blob_id;
+	ret = drmIoctl(data->drm_fd, DRM_IOCTL_MODE_DESTROYPROPBLOB, &d);
+	igt_assert(ret == 0);
+
+	igt_pipe_crc_stop(pipe_crc);
+	igt_pipe_crc_free(pipe_crc);
+	igt_remove_fb(data->drm_fd, &ref_fb);
+}
+
 static void atomic_setup(data_t *data, enum pipe pipe, igt_output_t *output)
 {
 	drmModeModeInfo *mode;
@@ -1634,6 +1714,20 @@ igt_main_args("e", NULL, help_str, opt_handler, NULL)
 		}
 	}
 
+	igt_describe("Test case for solid fill primary planes");
+	igt_subtest_with_dynamic("plane-primary-solid-fill") {
+		for_each_pipe_with_single_output(&data.display, pipe, output) {
+			if (!pipe_output_combo_valid(&data.display, pipe, output))
+				continue;
+
+			igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(pipe), igt_output_name(output)) {
+				atomic_setup(&data, pipe, output);
+				test_solid_fill_plane(&data, output, data.primary);
+				atomic_clear(&data, pipe, output);
+			}
+		}
+	}
+
 	igt_fixture {
 		igt_display_fini(&data.display);
 		drm_close_driver(data.drm_fd);

-- 
2.43.0

[PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Jessica Zhang]

Add a basic test for solid fill planes.

This test will first commit a single-color framebuffer plane then
a solid fill plane with the same contents. It then validates the solid
fill plane by comparing the resulting CRC with the CRC of the reference
framebuffer commit.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
old mode 100644
new mode 100755
index 2b6e9a8f0383..8f81e65ad84f
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
 	PLANE_RELAX_FB = (1 << 1)
 };
 
+struct solid_fill_blob {
+	uint32_t r;
+	uint32_t g;
+	uint32_t b;
+	uint32_t pad;
+};
+
 static inline int damage_rect_width(struct drm_mode_rect *r)
 {
 	return r->x2 - r->x1;
@@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
 	igt_remove_fb(data->drm_fd, &fb_2);
 }
 
+static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
+{
+	struct drm_mode_create_blob c;
+	struct drm_mode_destroy_blob d;
+	drmModeModeInfo *mode = igt_output_get_mode(output);
+	struct drm_mode_rect rect = { 0 };
+	struct igt_fb ref_fb;
+	igt_pipe_crc_t *pipe_crc;
+	igt_crc_t ref_crc, new_crc;
+	enum pipe pipe = data->pipe->pipe;
+	int height, width;
+	int ret;
+
+	struct solid_fill_blob blob_data = {
+		.r = 0x00000000,
+		.g = 0x00000000,
+		.b = 0xff000000,
+		.pad = 0x0,
+	};
+
+	rect.x1 = 0;
+	rect.x2 = mode->hdisplay;
+	rect.y1 = 0;
+	rect.y2 = mode->vdisplay;
+
+	height = rect.y2 - rect.y1;
+	width = rect.x2 - rect.x1;
+
+	igt_require(igt_plane_has_prop(plane, IGT_PLANE_SOLID_FILL));
+	igt_require(igt_plane_has_prop(plane, IGT_PLANE_PIXEL_SOURCE));
+
+	c.data = (uintptr_t) &blob_data;
+	c.length = sizeof(blob_data);
+
+	pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
+				    IGT_PIPE_CRC_SOURCE_AUTO);
+
+	/* get reference CRC */
+	igt_create_color_fb(data->drm_fd, width, height,
+			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
+			    0.0, 0.0, 1.0, &ref_fb);
+	igt_plane_set_fb(plane, &ref_fb);
+	igt_plane_set_position(plane, rect.x1, rect.y1);
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+
+	igt_pipe_crc_start(pipe_crc);
+	igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
+
+	ret = drmIoctl(data->display.drm_fd, DRM_IOCTL_MODE_CREATEPROPBLOB, &c);
+	igt_assert(ret == 0);
+
+	/* test solid fill plane */
+	igt_plane_set_solid_fill(plane, &rect, c.blob_id);
+	igt_plane_set_prop_enum(plane, IGT_PLANE_PIXEL_SOURCE, "SOLID_FILL");
+
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &new_crc);
+	igt_assert_crc_equal(&ref_crc, &new_crc);
+
+	/* Switch back to FB pixel source */
+	igt_plane_set_prop_value(plane, IGT_PLANE_SOLID_FILL, 0);
+	igt_plane_set_prop_enum(plane, IGT_PLANE_PIXEL_SOURCE, "FB");
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+
+	d.blob_id = c.blob_id;
+	ret = drmIoctl(data->drm_fd, DRM_IOCTL_MODE_DESTROYPROPBLOB, &d);
+	igt_assert(ret == 0);
+
+	igt_pipe_crc_stop(pipe_crc);
+	igt_pipe_crc_free(pipe_crc);
+	igt_remove_fb(data->drm_fd, &ref_fb);
+}
+
 static void atomic_setup(data_t *data, enum pipe pipe, igt_output_t *output)
 {
 	drmModeModeInfo *mode;
@@ -1634,6 +1714,20 @@ igt_main_args("e", NULL, help_str, opt_handler, NULL)
 		}
 	}
 
+	igt_describe("Test case for solid fill primary planes");
+	igt_subtest_with_dynamic("plane-primary-solid-fill") {
+		for_each_pipe_with_single_output(&data.display, pipe, output) {
+			if (!pipe_output_combo_valid(&data.display, pipe, output))
+				continue;
+
+			igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(pipe), igt_output_name(output)) {
+				atomic_setup(&data, pipe, output);
+				test_solid_fill_plane(&data, output, data.primary);
+				atomic_clear(&data, pipe, output);
+			}
+		}
+	}
+
 	igt_fixture {
 		igt_display_fini(&data.display);
 		drm_close_driver(data.drm_fd);

-- 
2.43.0

[PATCH i-g-t 4/4] tests/kms_atomic: Add subtest for solid fill cursor planes

[Jessica Zhang]

Add a subtest for testing solid fill cursor planes.

This test will commit a solid fill cursor plane on top of a normal
framebuffer primary plane and compare the resulting CRC with the CRC
value of a framebuffer cursor and primary plane of the same contents.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 tests/kms_atomic.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
index 8f81e65ad84f..057fba0d6767 100755
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -1349,10 +1349,18 @@ static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane
 		.pad = 0x0,
 	};
 
-	rect.x1 = 0;
-	rect.x2 = mode->hdisplay;
-	rect.y1 = 0;
-	rect.y2 = mode->vdisplay;
+	if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+	{
+		rect.x1 = 0;
+		rect.x2 = mode->hdisplay;
+		rect.y1 = 0;
+		rect.y2 = mode->vdisplay;
+	} else {
+		rect.x1 = mode->hdisplay / 2 - 50;
+		rect.x2 = mode->hdisplay / 2 + 50;
+		rect.y1 = mode->vdisplay / 2 - 50;
+		rect.y2 = mode->vdisplay / 2 + 50;
+	}
 
 	height = rect.y2 - rect.y1;
 	width = rect.x2 - rect.x1;
@@ -1728,6 +1736,24 @@ igt_main_args("e", NULL, help_str, opt_handler, NULL)
 		}
 	}
 
+	igt_describe("Test case for solid fill cursor planes");
+	igt_subtest_with_dynamic("plane-cursor-solid-fill") {
+		for_each_pipe_with_single_output(&data.display, pipe, output) {
+			igt_plane_t *cursor =
+				igt_pipe_get_plane_type(&data.display.pipes[pipe], DRM_PLANE_TYPE_CURSOR);
+			if (!pipe_output_combo_valid(&data.display, pipe, output))
+				continue;
+			if (!cursor)
+				continue;
+
+			igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(pipe), igt_output_name(output)) {
+				atomic_setup(&data, pipe, output);
+				test_solid_fill_plane(&data, output, cursor);
+				atomic_clear(&data, pipe, output);
+			}
+		}
+	}
+
 	igt_fixture {
 		igt_display_fini(&data.display);
 		drm_close_driver(data.drm_fd);

-- 
2.43.0

[PATCH i-g-t 4/4] tests/kms_atomic: Add subtest for solid fill cursor planes

[Jessica Zhang]

Add a subtest for testing solid fill cursor planes.

This test will commit a solid fill cursor plane on top of a normal
framebuffer primary plane and compare the resulting CRC with the CRC
value of a framebuffer cursor and primary plane of the same contents.

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 tests/kms_atomic.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
index 8f81e65ad84f..057fba0d6767 100755
--- a/tests/kms_atomic.c
+++ b/tests/kms_atomic.c
@@ -1349,10 +1349,18 @@ static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane
 		.pad = 0x0,
 	};
 
-	rect.x1 = 0;
-	rect.x2 = mode->hdisplay;
-	rect.y1 = 0;
-	rect.y2 = mode->vdisplay;
+	if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+	{
+		rect.x1 = 0;
+		rect.x2 = mode->hdisplay;
+		rect.y1 = 0;
+		rect.y2 = mode->vdisplay;
+	} else {
+		rect.x1 = mode->hdisplay / 2 - 50;
+		rect.x2 = mode->hdisplay / 2 + 50;
+		rect.y1 = mode->vdisplay / 2 - 50;
+		rect.y2 = mode->vdisplay / 2 + 50;
+	}
 
 	height = rect.y2 - rect.y1;
 	width = rect.x2 - rect.x1;
@@ -1728,6 +1736,24 @@ igt_main_args("e", NULL, help_str, opt_handler, NULL)
 		}
 	}
 
+	igt_describe("Test case for solid fill cursor planes");
+	igt_subtest_with_dynamic("plane-cursor-solid-fill") {
+		for_each_pipe_with_single_output(&data.display, pipe, output) {
+			igt_plane_t *cursor =
+				igt_pipe_get_plane_type(&data.display.pipes[pipe], DRM_PLANE_TYPE_CURSOR);
+			if (!pipe_output_combo_valid(&data.display, pipe, output))
+				continue;
+			if (!cursor)
+				continue;
+
+			igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(pipe), igt_output_name(output)) {
+				atomic_setup(&data, pipe, output);
+				test_solid_fill_plane(&data, output, cursor);
+				atomic_clear(&data, pipe, output);
+			}
+		}
+	}
+
 	igt_fixture {
 		igt_display_fini(&data.display);
 		drm_close_driver(data.drm_fd);

-- 
2.43.0

✗ Fi.CI.BUILD: failure for Add tests for solid fill planes

[Patchwork]

== Series Details ==

Series: Add tests for solid fill planes
URL   : https://patchwork.freedesktop.org/series/127900/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
2a2b716161007f885855d6dfcb90b526183e624e tests/intel/xe: Remove xe_uevent for now

Tail of build.log:
[1611/1656] Linking target tools/intel_stepping.
[1612/1656] Linking target runner/testdata/abort.
[1613/1656] Linking target tools/intel_watermark.
[1614/1656] Linking target tools/intel_dp_compliance.
[1615/1656] Linking target runner/testdata/successtest.
[1616/1656] Linking target runner/testdata/dynamic.
[1617/1656] Linking target runner/testdata/no-subtests.
[1618/1656] Linking target tools/intel_vbt_decode.
[1619/1656] Linking target runner/testdata/abort-dynamic.
[1620/1656] Linking target tools/intel_gem_info.
[1621/1656] Linking target runner/igt_runner.
[1622/1656] Linking target tools/amd_hdmi_compliance.
[1623/1656] Generating gem_stress.testlist with a meson_exe.py custom command.
[1624/1656] Linking target runner/igt_comms_decoder.
[1625/1656] Linking target tools/intel_l3_parity.
[1626/1656] Linking target tools/msm_dp_compliance.
[1627/1656] Linking target runner/igt_results.
[1628/1656] Linking target tools/intel_reg.
[1629/1656] Linking target runner/igt_resume.
[1630/1656] Linking target runner/testdata/abort-simple.
[1631/1656] Linking target runner/testdata/abort-fixture.
[1632/1656] Linking target runner/runner_json_test.
[1633/1656] Compiling C object 'lib/76b5a35@@i915_perf@sha/meson-generated_.._i915_perf_metrics_mtlgt3.c.o'.
[1634/1656] Compiling C object 'lib/76b5a35@@i915_perf@sha/meson-generated_.._i915_perf_metrics_acmgt1.c.o'.
[1635/1656] Compiling C object 'runner/527aa9f@@runner_test@exe/runner_tests.c.o'.
[1636/1656] Linking target runner/runner_test.
[1637/1656] Compiling C object 'lib/76b5a35@@i915_perf@sha/meson-generated_.._i915_perf_metrics_acmgt2.c.o'.
[1638/1656] Compiling C object 'lib/76b5a35@@i915_perf@sha/meson-generated_.._i915_perf_metrics_acmgt3.c.o'.
[1639/1656] Linking target lib/libi915_perf.so.1.5.
[1640/1656] Generating symbol file 'lib/76b5a35@@i915_perf@sha/libi915_perf.so.1.5.symbols'.
[1641/1656] Linking target tools/i915-perf/i915-perf-configs.
[1642/1656] Linking target tools/i915-perf/i915-perf-reader.
[1643/1656] Linking target tools/i915-perf/i915-perf-recorder.
[1644/1656] Linking target tests/gem_barrier_race.
[1645/1656] Linking target tests/core_hotunplug.
[1646/1656] Linking target tests/perf.
[1647/1656] Generating perf.testlist with a meson_exe.py custom command.
[1648/1656] Generating core_hotunplug.testlist with a meson_exe.py custom command.
[1649/1656] Generating gem_barrier_race.testlist with a meson_exe.py custom command.
[1650/1656] Generating xe_tests.rst with a custom command.
[1651/1656] Generating i915_tests.rst with a custom command.
[1652/1656] Generating kms_tests.rst with a custom command.
FAILED: docs/testplan/kms_tests.rst 
/usr/src/igt-gpu-tools/scripts/igt_doc.py --config /usr/src/igt-gpu-tools/tests/intel/kms_test_config.json --rest docs/testplan/kms_tests.rst --check-testlist --igt-build-path /opt/igt/build
Warning: Missing documentation for igt@kms_atomic@plane-cursor-solid-fill
Warning: Missing documentation for igt@kms_atomic@plane-primary-solid-fill
Please refer: docs/test_documentation.md for more details
[1653/1656] Generating xe_tests.html with a custom command.
[1654/1656] Generating i915_tests.html with a custom command.
ninja: build stopped: subcommand failed.

✗ GitLab.Pipeline: warning for Add tests for solid fill planes

[Patchwork]

== Series Details ==

Series: Add tests for solid fill planes
URL   : https://patchwork.freedesktop.org/series/127900/
State : warning

== Summary ==

Pipeline status: FAILED.

see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/1058246 for the overview.

build:tests-debian-meson has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/52830158):
  Please refer: docs/test_documentation.md for more details
  [1648/1651] Generating i915_tests.html with a custom command.
  [1649/1651] Compiling C object 'runner/527aa9f@@runner_test@exe/runner_tests.c.o'.
  ninja: build stopped: subcommand failed.
  ninja: Entering directory `build'
  [1/731] Generating version.h with a custom command.
  [2/4] Linking target runner/runner_test.
  [3/4] Generating kms_tests.rst with a custom command.
  FAILED: docs/testplan/kms_tests.rst 
  /builds/gfx-ci/igt-ci-tags/scripts/igt_doc.py --config /builds/gfx-ci/igt-ci-tags/tests/intel/kms_test_config.json --rest docs/testplan/kms_tests.rst --check-testlist --igt-build-path /builds/gfx-ci/igt-ci-tags/build
  Warning: Missing documentation for igt@kms_atomic@plane-cursor-solid-fill
  Warning: Missing documentation for igt@kms_atomic@plane-primary-solid-fill
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1702687857:step_script
  section_start:1702687857:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1702687858:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/52830153):
  Please refer: docs/test_documentation.md for more details
  [1654/1657] Generating i915_tests.html with a custom command.
  [1655/1657] Compiling C object 'runner/527aa9f@@runner_test@exe/runner_tests.c.o'.
  ninja: build stopped: subcommand failed.
  ninja: Entering directory `build'
  [1/733] Generating version.h with a custom command.
  [2/4] Linking target runner/runner_test.
  [3/4] Generating kms_tests.rst with a custom command.
  FAILED: docs/testplan/kms_tests.rst 
  /builds/gfx-ci/igt-ci-tags/scripts/igt_doc.py --config /builds/gfx-ci/igt-ci-tags/tests/intel/kms_test_config.json --rest docs/testplan/kms_tests.rst --check-testlist --igt-build-path /builds/gfx-ci/igt-ci-tags/build
  Warning: Missing documentation for igt@kms_atomic@plane-cursor-solid-fill
  Warning: Missing documentation for igt@kms_atomic@plane-primary-solid-fill
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1702687834:step_script
  section_start:1702687834:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1702687835:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora-clang has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/52830157):
  Warning: Missing documentation for igt@kms_atomic@plane-cursor-solid-fill
  Warning: Missing documentation for igt@kms_atomic@plane-primary-solid-fill
  Please refer: docs/test_documentation.md for more details
  [1656/1657] Generating i915_tests.html with a custom command.
  ninja: build stopped: subcommand failed.
  ninja: Entering directory `build'
  [1/733] Generating version.h with a custom command.
  [2/3] Generating kms_tests.rst with a custom command.
  FAILED: docs/testplan/kms_tests.rst 
  /builds/gfx-ci/igt-ci-tags/scripts/igt_doc.py --config /builds/gfx-ci/igt-ci-tags/tests/intel/kms_test_config.json --rest docs/testplan/kms_tests.rst --check-testlist --igt-build-path /builds/gfx-ci/igt-ci-tags/build
  Warning: Missing documentation for igt@kms_atomic@plane-cursor-solid-fill
  Warning: Missing documentation for igt@kms_atomic@plane-primary-solid-fill
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1702687885:step_script
  section_start:1702687885:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1702687885:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora-no-libdrm-nouveau has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/52830156):
  Please refer: docs/test_documentation.md for more details
  [1494/1497] Generating i915_tests.html with a custom command.
  [1495/1497] Compiling C object 'runner/527aa9f@@runner_test@exe/runner_tests.c.o'.
  ninja: build stopped: subcommand failed.
  ninja: Entering directory `build'
  [1/685] Generating version.h with a custom command.
  [2/4] Linking target runner/runner_test.
  [3/4] Generating kms_tests.rst with a custom command.
  FAILED: docs/testplan/kms_tests.rst 
  /builds/gfx-ci/igt-ci-tags/scripts/igt_doc.py --config /builds/gfx-ci/igt-ci-tags/tests/intel/kms_test_config.json --rest docs/testplan/kms_tests.rst --check-testlist --igt-build-path /builds/gfx-ci/igt-ci-tags/build
  Warning: Missing documentation for igt@kms_atomic@plane-cursor-solid-fill
  Warning: Missing documentation for igt@kms_atomic@plane-primary-solid-fill
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1702687832:step_script
  section_start:1702687832:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1702687832:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora-no-libunwind has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/52830154):
  Please refer: docs/test_documentation.md for more details
  [1654/1657] Generating i915_tests.html with a custom command.
  [1655/1657] Compiling C object 'runner/527aa9f@@runner_test@exe/runner_tests.c.o'.
  ninja: build stopped: subcommand failed.
  ninja: Entering directory `build'
  [1/733] Generating version.h with a custom command.
  [2/4] Linking target runner/runner_test.
  [3/4] Generating kms_tests.rst with a custom command.
  FAILED: docs/testplan/kms_tests.rst 
  /builds/gfx-ci/igt-ci-tags/scripts/igt_doc.py --config /builds/gfx-ci/igt-ci-tags/tests/intel/kms_test_config.json --rest docs/testplan/kms_tests.rst --check-testlist --igt-build-path /builds/gfx-ci/igt-ci-tags/build
  Warning: Missing documentation for igt@kms_atomic@plane-cursor-solid-fill
  Warning: Missing documentation for igt@kms_atomic@plane-primary-solid-fill
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1702687835:step_script
  section_start:1702687835:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1702687836:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

build:tests-fedora-oldest-meson has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/52830155):
  Please refer: docs/test_documentation.md for more details
  [1654/1657] Compiling C object 'runner/runner@@runner_test@exe/runner_tests.c.o'.
  [1655/1657] Generating i915_tests.html with a custom command.
  ninja: build stopped: subcommand failed.
  ninja: Entering directory `build'
  [1/733] Generating version.h with a custom command.
  [2/4] Linking target runner/runner_test.
  [3/4] Generating kms_tests.rst with a custom command.
  FAILED: docs/testplan/kms_tests.rst 
  /builds/gfx-ci/igt-ci-tags/scripts/igt_doc.py --config /builds/gfx-ci/igt-ci-tags/tests/intel/kms_test_config.json --rest docs/testplan/kms_tests.rst --check-testlist --igt-build-path /builds/gfx-ci/igt-ci-tags/build
  Warning: Missing documentation for igt@kms_atomic@plane-cursor-solid-fill
  Warning: Missing documentation for igt@kms_atomic@plane-primary-solid-fill
  Please refer: docs/test_documentation.md for more details
  ninja: build stopped: subcommand failed.
  section_end:1702687851:step_script
  section_start:1702687851:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1702687852:cleanup_file_variables
  ERROR: Job failed: exit code 1

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/1058246

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3672 bytes --]

On Fri, 15 Dec 2023 16:40:23 -0800
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:

> Add a basic test for solid fill planes.
> 
> This test will first commit a single-color framebuffer plane then
> a solid fill plane with the same contents. It then validates the solid
> fill plane by comparing the resulting CRC with the CRC of the reference
> framebuffer commit.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> old mode 100644
> new mode 100755
> index 2b6e9a8f0383..8f81e65ad84f
> --- a/tests/kms_atomic.c
> +++ b/tests/kms_atomic.c
> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
>  	PLANE_RELAX_FB = (1 << 1)
>  };
>  
> +struct solid_fill_blob {
> +	uint32_t r;
> +	uint32_t g;
> +	uint32_t b;
> +	uint32_t pad;
> +};
> +
>  static inline int damage_rect_width(struct drm_mode_rect *r)
>  {
>  	return r->x2 - r->x1;
> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
>  	igt_remove_fb(data->drm_fd, &fb_2);
>  }
>  
> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
> +{
> +	struct drm_mode_create_blob c;
> +	struct drm_mode_destroy_blob d;
> +	drmModeModeInfo *mode = igt_output_get_mode(output);
> +	struct drm_mode_rect rect = { 0 };
> +	struct igt_fb ref_fb;
> +	igt_pipe_crc_t *pipe_crc;
> +	igt_crc_t ref_crc, new_crc;
> +	enum pipe pipe = data->pipe->pipe;
> +	int height, width;
> +	int ret;
> +
> +	struct solid_fill_blob blob_data = {
> +		.r = 0x00000000,
> +		.g = 0x00000000,
> +		.b = 0xff000000,
> +		.pad = 0x0,
> +	};

Hi Jessica!

This is the blob sent to KMS as the solid fill color...

...

> +	igt_create_color_fb(data->drm_fd, width, height,
> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> +			    0.0, 0.0, 1.0, &ref_fb);

..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
values, I presume.

So you say that 0xff000000 = 1.0.

However, the patch for the kernel UAPI header says this:

+/**
+ * struct drm_mode_solid_fill - User info for solid fill planes
+ *
+ * This is the userspace API solid fill information structure.
+ *
+ * Userspace can enable solid fill planes by assigning the plane "solid_fill"
+ * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
+ * color and setting the pixel source to "SOLID_FILL".
+ *
+ * For information on the plane property, see drm_plane_create_solid_fill_property()
+ *
+ * @r: Red color value of single pixel
+ * @g: Green color value of single pixel
+ * @b: Blue color value of single pixel
+ * @pad: padding, must be zero
+ */
+struct drm_mode_solid_fill {
+	__u32 r;
+	__u32 g;
+	__u32 b;
+	__u32 pad;
+};

I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
bug in the test.

It would be good to test more than one color:
- 0.0, 0.0, 0.0
- 1.0, 0.0, 0.0
- 0.0, 1.0, 0.0
- 0.0, 0.0, 1.0
- 1.0, 1.0, 1.0

for example. That would get at least the so often used black explicitly
tested, and verify each channel gets mapped correctly rather than only
blue.

It would also be really good to test dim and mid grays, but I assume it
might be difficult to get CRC to match over various hardware. You'd
need to use writeback with an error tolerance. (For watching photos for
example, the background is not usually black but dim gray I believe.)


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Kamil Konieczny]

Hi Pekka,
On 2023-12-18 at 12:12:47 +0200, Pekka Paalanen wrote:
> On Fri, 15 Dec 2023 16:40:23 -0800
> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> 

please subscribe to igt-dev mailinglist, you can do it here:
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Regards,
Kamil

> > Add a basic test for solid fill planes.
> > 
> > This test will first commit a single-color framebuffer plane then
> > a solid fill plane with the same contents. It then validates the solid
> > fill plane by comparing the resulting CRC with the CRC of the reference
> > framebuffer commit.
> > 
> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> > ---
> >  tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> > 
> > diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> > old mode 100644
> > new mode 100755
> > index 2b6e9a8f0383..8f81e65ad84f
> > --- a/tests/kms_atomic.c
> > +++ b/tests/kms_atomic.c
> > @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
> >  	PLANE_RELAX_FB = (1 << 1)
> >  };
> >  
> > +struct solid_fill_blob {
> > +	uint32_t r;
> > +	uint32_t g;
> > +	uint32_t b;
> > +	uint32_t pad;
> > +};
> > +
> >  static inline int damage_rect_width(struct drm_mode_rect *r)
> >  {
> >  	return r->x2 - r->x1;
> > @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
> >  	igt_remove_fb(data->drm_fd, &fb_2);
> >  }
> >  
> > +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
> > +{
> > +	struct drm_mode_create_blob c;
> > +	struct drm_mode_destroy_blob d;
> > +	drmModeModeInfo *mode = igt_output_get_mode(output);
> > +	struct drm_mode_rect rect = { 0 };
> > +	struct igt_fb ref_fb;
> > +	igt_pipe_crc_t *pipe_crc;
> > +	igt_crc_t ref_crc, new_crc;
> > +	enum pipe pipe = data->pipe->pipe;
> > +	int height, width;
> > +	int ret;
> > +
> > +	struct solid_fill_blob blob_data = {
> > +		.r = 0x00000000,
> > +		.g = 0x00000000,
> > +		.b = 0xff000000,
> > +		.pad = 0x0,
> > +	};
> 
> Hi Jessica!
> 
> This is the blob sent to KMS as the solid fill color...
> 
> ...
> 
> > +	igt_create_color_fb(data->drm_fd, width, height,
> > +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> > +			    0.0, 0.0, 1.0, &ref_fb);
> 
> ..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
> values, I presume.
> 
> So you say that 0xff000000 = 1.0.
> 
> However, the patch for the kernel UAPI header says this:
> 
> +/**
> + * struct drm_mode_solid_fill - User info for solid fill planes
> + *
> + * This is the userspace API solid fill information structure.
> + *
> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
> + * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
> + * color and setting the pixel source to "SOLID_FILL".
> + *
> + * For information on the plane property, see drm_plane_create_solid_fill_property()
> + *
> + * @r: Red color value of single pixel
> + * @g: Green color value of single pixel
> + * @b: Blue color value of single pixel
> + * @pad: padding, must be zero
> + */
> +struct drm_mode_solid_fill {
> +	__u32 r;
> +	__u32 g;
> +	__u32 b;
> +	__u32 pad;
> +};
> 
> I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
> format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
> bug in the test.
> 
> It would be good to test more than one color:
> - 0.0, 0.0, 0.0
> - 1.0, 0.0, 0.0
> - 0.0, 1.0, 0.0
> - 0.0, 0.0, 1.0
> - 1.0, 1.0, 1.0
> 
> for example. That would get at least the so often used black explicitly
> tested, and verify each channel gets mapped correctly rather than only
> blue.
> 
> It would also be really good to test dim and mid grays, but I assume it
> might be difficult to get CRC to match over various hardware. You'd
> need to use writeback with an error tolerance. (For watching photos for
> example, the background is not usually black but dim gray I believe.)
> 
> 
> Thanks,
> pq

Re: [PATCH i-g-t 2/4] lib: Add support for solid_fill and pixel_source plane properties

[Kamil Konieczny]

Hi Jessica,
On 2023-12-15 at 16:40:22 -0800, Jessica Zhang wrote:
> Add corresponding IGT macros for solid_fill and pixel_source DRM
> properties, and change IGT commit behavior to allow for NULL FB commits.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  lib/igt_kms.c | 37 ++++++++++++++++++++++++++++++++++---
>  lib/igt_kms.h |  4 ++++
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index e4dea1a604dc..6a339766ca31 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -669,6 +669,8 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>  	[IGT_PLANE_HOTSPOT_X] = "HOTSPOT_X",
>  	[IGT_PLANE_HOTSPOT_Y] = "HOTSPOT_Y",
>  	[IGT_PLANE_FB_ID] = "FB_ID",
> +	[IGT_PLANE_SOLID_FILL] = "solid_fill",
> +	[IGT_PLANE_PIXEL_SOURCE] = "pixel_source",
>  	[IGT_PLANE_CRTC_ID] = "CRTC_ID",
>  	[IGT_PLANE_IN_FENCE_FD] = "IN_FENCE_FD",
>  	[IGT_PLANE_TYPE] = "type",
> @@ -3417,6 +3419,12 @@ static igt_output_t *igt_pipe_get_output(igt_pipe_t *pipe)
>  	return NULL;
>  }
>  
> +static uint32_t igt_plane_get_solid_fill_id(igt_plane_t *plane)
> +{
> +	return plane->values[IGT_PLANE_SOLID_FILL];
> +}
> +
> +

Remove one empty line.

>  static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>  {
>  	return plane->values[IGT_PLANE_FB_ID];
> @@ -3648,7 +3656,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
>  	struct igt_display *display = primary->pipe->display;
>  	igt_output_t *output = igt_pipe_get_output(pipe);
>  	drmModeModeInfo *mode;
> -	uint32_t fb_id, crtc_id;
> +	uint32_t fb_id, solid_fill_id, crtc_id;
>  	int ret;
>  
>  	/* Primary planes can't be windowed when using a legacy commit */
> @@ -3665,12 +3673,14 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
>  
>  	crtc_id = pipe->crtc_id;
>  	fb_id = output ? igt_plane_get_fb_id(primary) : 0;
> -	if (fb_id)
> +	solid_fill_id = output ? igt_plane_get_solid_fill_id(primary) : 0;
> +
> +	if (fb_id || solid_fill_id)
>  		mode = igt_output_get_mode(output);
>  	else
>  		mode = NULL;
>  
> -	if (fb_id) {
> +	if (fb_id || solid_fill_id) {
>  		uint32_t src_x = primary->values[IGT_PLANE_SRC_X] >> 16;
>  		uint32_t src_y = primary->values[IGT_PLANE_SRC_Y] >> 16;
>  
> @@ -4935,6 +4945,27 @@ igt_plane_t *igt_output_get_plane_type_index(igt_output_t *output,
>  	return igt_pipe_get_plane_type_index(pipe, plane_type, index);
>  }
>  

Please add description to each new public function.

Regards,
Kamil

> +void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
> +		int solid_fill_id)
> +{
> +	igt_pipe_t *pipe = plane->pipe;
> +	igt_display_t *display = pipe->display;
> +	int width = rect->x2 - rect->x1;
> +	int height = rect->y2 - rect->y1;
> +
> +	igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_ID,
> +			solid_fill_id ? pipe->crtc_id : 0);
> +	igt_plane_set_prop_value(plane, IGT_PLANE_SOLID_FILL, solid_fill_id);
> +
> +	plane->gem_handle = 0;
> +
> +	LOG(display, "%s.%d: plane_set_solid_fill(%d)\n", kmstest_pipe_name(pipe->pipe),
> +	    plane->index, solid_fill_id);
> +
> +	igt_plane_set_position(plane, rect->x1, rect->y1);
> +	igt_plane_set_size(plane, width, height);
> +}
> +
>  /**
>   * igt_plane_set_fb:
>   * @plane: Plane
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index b3882808b42f..9c05272abe1e 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -330,6 +330,8 @@ enum igt_atomic_plane_properties {
>  #define IGT_PLANE_COORD_CHANGED_MASK 0xff
>  
>         IGT_PLANE_FB_ID,
> +       IGT_PLANE_SOLID_FILL,
> +       IGT_PLANE_PIXEL_SOURCE,
>         IGT_PLANE_CRTC_ID,
>         IGT_PLANE_IN_FENCE_FD,
>         IGT_PLANE_TYPE,
> @@ -537,6 +539,8 @@ igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe p
>  
>  void igt_pipe_request_out_fence(igt_pipe_t *pipe);
>  
> +void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
> +		int solid_fill_id);
>  void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
>  void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd);
>  void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe);
> 
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Kamil Konieczny]

Hi Jessica,
On 2023-12-15 at 16:40:23 -0800, Jessica Zhang wrote:
> Add a basic test for solid fill planes.
> 
> This test will first commit a single-color framebuffer plane then
> a solid fill plane with the same contents. It then validates the solid
> fill plane by comparing the resulting CRC with the CRC of the reference
> framebuffer commit.
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>  tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> old mode 100644
> new mode 100755
> index 2b6e9a8f0383..8f81e65ad84f
> --- a/tests/kms_atomic.c
> +++ b/tests/kms_atomic.c
> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
>  	PLANE_RELAX_FB = (1 << 1)
>  };
>  
> +struct solid_fill_blob {
> +	uint32_t r;
> +	uint32_t g;
> +	uint32_t b;
> +	uint32_t pad;
> +};
> +
>  static inline int damage_rect_width(struct drm_mode_rect *r)
>  {
>  	return r->x2 - r->x1;
> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
>  	igt_remove_fb(data->drm_fd, &fb_2);
>  }
>  
> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
> +{
> +	struct drm_mode_create_blob c;
> +	struct drm_mode_destroy_blob d;
> +	drmModeModeInfo *mode = igt_output_get_mode(output);
> +	struct drm_mode_rect rect = { 0 };
> +	struct igt_fb ref_fb;
> +	igt_pipe_crc_t *pipe_crc;
> +	igt_crc_t ref_crc, new_crc;
> +	enum pipe pipe = data->pipe->pipe;
> +	int height, width;
> +	int ret;
> +
> +	struct solid_fill_blob blob_data = {
> +		.r = 0x00000000,
> +		.g = 0x00000000,
> +		.b = 0xff000000,
> +		.pad = 0x0,
> +	};
> +
> +	rect.x1 = 0;
> +	rect.x2 = mode->hdisplay;
> +	rect.y1 = 0;
> +	rect.y2 = mode->vdisplay;
> +
> +	height = rect.y2 - rect.y1;
> +	width = rect.x2 - rect.x1;
> +
> +	igt_require(igt_plane_has_prop(plane, IGT_PLANE_SOLID_FILL));
> +	igt_require(igt_plane_has_prop(plane, IGT_PLANE_PIXEL_SOURCE));
> +
> +	c.data = (uintptr_t) &blob_data;
> +	c.length = sizeof(blob_data);
> +
> +	pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
> +				    IGT_PIPE_CRC_SOURCE_AUTO);
> +
> +	/* get reference CRC */
> +	igt_create_color_fb(data->drm_fd, width, height,
> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> +			    0.0, 0.0, 1.0, &ref_fb);
> +	igt_plane_set_fb(plane, &ref_fb);
> +	igt_plane_set_position(plane, rect.x1, rect.y1);
> +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +
> +	igt_pipe_crc_start(pipe_crc);
> +	igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
> +
> +	ret = drmIoctl(data->display.drm_fd, DRM_IOCTL_MODE_CREATEPROPBLOB, &c);
> +	igt_assert(ret == 0);
> +
> +	/* test solid fill plane */
> +	igt_plane_set_solid_fill(plane, &rect, c.blob_id);
> +	igt_plane_set_prop_enum(plane, IGT_PLANE_PIXEL_SOURCE, "SOLID_FILL");
> +
> +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +	igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &new_crc);
> +	igt_assert_crc_equal(&ref_crc, &new_crc);
> +
> +	/* Switch back to FB pixel source */
> +	igt_plane_set_prop_value(plane, IGT_PLANE_SOLID_FILL, 0);
> +	igt_plane_set_prop_enum(plane, IGT_PLANE_PIXEL_SOURCE, "FB");
> +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +
> +	d.blob_id = c.blob_id;
> +	ret = drmIoctl(data->drm_fd, DRM_IOCTL_MODE_DESTROYPROPBLOB, &d);
> +	igt_assert(ret == 0);
> +
> +	igt_pipe_crc_stop(pipe_crc);
> +	igt_pipe_crc_free(pipe_crc);
> +	igt_remove_fb(data->drm_fd, &ref_fb);
> +}
> +
>  static void atomic_setup(data_t *data, enum pipe pipe, igt_output_t *output)
>  {
>  	drmModeModeInfo *mode;
> @@ -1634,6 +1714,20 @@ igt_main_args("e", NULL, help_str, opt_handler, NULL)
>  		}
>  	}
>  

Please add also description at begin of file, one at each test added,
something like:

 * SUBTEST: plane-primary-solid-fill
 * Description: fill primary planes with solid color

Above example can be incorrect, please correct it as needed.

For verification look if there are any build warnings at patchwork, like:

Warning: Missing documentation for igt@kms_atomic@plane-cursor-solid-fill
Warning: Missing documentation for igt@kms_atomic@plane-primary-solid-fill

+cc Bhanu

Regards,
Kamil

> +	igt_describe("Test case for solid fill primary planes");
> +	igt_subtest_with_dynamic("plane-primary-solid-fill") {
> +		for_each_pipe_with_single_output(&data.display, pipe, output) {
> +			if (!pipe_output_combo_valid(&data.display, pipe, output))
> +				continue;
> +
> +			igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(pipe), igt_output_name(output)) {
> +				atomic_setup(&data, pipe, output);
> +				test_solid_fill_plane(&data, output, data.primary);
> +				atomic_clear(&data, pipe, output);
> +			}
> +		}
> +	}
> +
>  	igt_fixture {
>  		igt_display_fini(&data.display);
>  		drm_close_driver(data.drm_fd);
> 
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Jessica Zhang]

On 12/18/2023 2:12 AM, Pekka Paalanen wrote:
> On Fri, 15 Dec 2023 16:40:23 -0800
> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> 
>> Add a basic test for solid fill planes.
>>
>> This test will first commit a single-color framebuffer plane then
>> a solid fill plane with the same contents. It then validates the solid
>> fill plane by comparing the resulting CRC with the CRC of the reference
>> framebuffer commit.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 94 insertions(+)
>>
>> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
>> old mode 100644
>> new mode 100755
>> index 2b6e9a8f0383..8f81e65ad84f
>> --- a/tests/kms_atomic.c
>> +++ b/tests/kms_atomic.c
>> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
>>   	PLANE_RELAX_FB = (1 << 1)
>>   };
>>   
>> +struct solid_fill_blob {
>> +	uint32_t r;
>> +	uint32_t g;
>> +	uint32_t b;
>> +	uint32_t pad;
>> +};
>> +
>>   static inline int damage_rect_width(struct drm_mode_rect *r)
>>   {
>>   	return r->x2 - r->x1;
>> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
>>   	igt_remove_fb(data->drm_fd, &fb_2);
>>   }
>>   
>> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
>> +{
>> +	struct drm_mode_create_blob c;
>> +	struct drm_mode_destroy_blob d;
>> +	drmModeModeInfo *mode = igt_output_get_mode(output);
>> +	struct drm_mode_rect rect = { 0 };
>> +	struct igt_fb ref_fb;
>> +	igt_pipe_crc_t *pipe_crc;
>> +	igt_crc_t ref_crc, new_crc;
>> +	enum pipe pipe = data->pipe->pipe;
>> +	int height, width;
>> +	int ret;
>> +
>> +	struct solid_fill_blob blob_data = {
>> +		.r = 0x00000000,
>> +		.g = 0x00000000,
>> +		.b = 0xff000000,
>> +		.pad = 0x0,
>> +	};
> 
> Hi Jessica!
> 
> This is the blob sent to KMS as the solid fill color...
> 
> ...
> 
>> +	igt_create_color_fb(data->drm_fd, width, height,
>> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
>> +			    0.0, 0.0, 1.0, &ref_fb);
> 
> ..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
> values, I presume.
> 
> So you say that 0xff000000 = 1.0.
> 
> However, the patch for the kernel UAPI header says this:
> 
> +/**
> + * struct drm_mode_solid_fill - User info for solid fill planes
> + *
> + * This is the userspace API solid fill information structure.
> + *
> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
> + * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
> + * color and setting the pixel source to "SOLID_FILL".
> + *
> + * For information on the plane property, see drm_plane_create_solid_fill_property()
> + *
> + * @r: Red color value of single pixel
> + * @g: Green color value of single pixel
> + * @b: Blue color value of single pixel
> + * @pad: padding, must be zero
> + */
> +struct drm_mode_solid_fill {
> +	__u32 r;
> +	__u32 g;
> +	__u32 b;
> +	__u32 pad;
> +};
> 
> I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
> format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
> bug in the test.

Hey Pekka,

Ah, thanks for catching this -- I'll change the blob value to 0xffffffff 
so it matches the 1.0.

While we're talking about the UAPI struct, I'll also add the actual 
drm_mode_solid_fill struct to the IGT drm-uapi instead of the current 
workaround.

> 
> It would be good to test more than one color:
> - 0.0, 0.0, 0.0
> - 1.0, 0.0, 0.0
> - 0.0, 1.0, 0.0
> - 0.0, 0.0, 1.0
> - 1.0, 1.0, 1.0

Sounds good, will change the test to validate these combinations.

> 
> for example. That would get at least the so often used black explicitly
> tested, and verify each channel gets mapped correctly rather than only
> blue.
> 
> It would also be really good to test dim and mid grays, but I assume it
> might be difficult to get CRC to match over various hardware. You'd
> need to use writeback with an error tolerance. (For watching photos for
> example, the background is not usually black but dim gray I believe.)

Got it, we can add this to the list of colors to test.

FWIW, I think as long as we keep the test structure as grabbing a 
reference CRC from an FB commit then comparing that to a CRC from a 
solid fill commit, I'm not expecting a difference in CRC values.

Thanks,

Jessica Zhang

> 
> 
> Thanks,
> pq

Re: [PATCH i-g-t 2/4] lib: Add support for solid_fill and pixel_source plane properties

[Jessica Zhang]

On 12/19/2023 5:51 AM, Kamil Konieczny wrote:
> Hi Jessica,
> On 2023-12-15 at 16:40:22 -0800, Jessica Zhang wrote:
>> Add corresponding IGT macros for solid_fill and pixel_source DRM
>> properties, and change IGT commit behavior to allow for NULL FB commits.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   lib/igt_kms.c | 37 ++++++++++++++++++++++++++++++++++---
>>   lib/igt_kms.h |  4 ++++
>>   2 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index e4dea1a604dc..6a339766ca31 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -669,6 +669,8 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>>   	[IGT_PLANE_HOTSPOT_X] = "HOTSPOT_X",
>>   	[IGT_PLANE_HOTSPOT_Y] = "HOTSPOT_Y",
>>   	[IGT_PLANE_FB_ID] = "FB_ID",
>> +	[IGT_PLANE_SOLID_FILL] = "solid_fill",
>> +	[IGT_PLANE_PIXEL_SOURCE] = "pixel_source",
>>   	[IGT_PLANE_CRTC_ID] = "CRTC_ID",
>>   	[IGT_PLANE_IN_FENCE_FD] = "IN_FENCE_FD",
>>   	[IGT_PLANE_TYPE] = "type",
>> @@ -3417,6 +3419,12 @@ static igt_output_t *igt_pipe_get_output(igt_pipe_t *pipe)
>>   	return NULL;
>>   }
>>   
>> +static uint32_t igt_plane_get_solid_fill_id(igt_plane_t *plane)
>> +{
>> +	return plane->values[IGT_PLANE_SOLID_FILL];
>> +}
>> +
>> +
> 
> Remove one empty line.

Hi Kamil,

Thanks for taking a look at this series. Will make the changes you 
suggested in this patch and in the following patch.

Thanks,

Jessica Zhang

> 
>>   static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>>   {
>>   	return plane->values[IGT_PLANE_FB_ID];
>> @@ -3648,7 +3656,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
>>   	struct igt_display *display = primary->pipe->display;
>>   	igt_output_t *output = igt_pipe_get_output(pipe);
>>   	drmModeModeInfo *mode;
>> -	uint32_t fb_id, crtc_id;
>> +	uint32_t fb_id, solid_fill_id, crtc_id;
>>   	int ret;
>>   
>>   	/* Primary planes can't be windowed when using a legacy commit */
>> @@ -3665,12 +3673,14 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
>>   
>>   	crtc_id = pipe->crtc_id;
>>   	fb_id = output ? igt_plane_get_fb_id(primary) : 0;
>> -	if (fb_id)
>> +	solid_fill_id = output ? igt_plane_get_solid_fill_id(primary) : 0;
>> +
>> +	if (fb_id || solid_fill_id)
>>   		mode = igt_output_get_mode(output);
>>   	else
>>   		mode = NULL;
>>   
>> -	if (fb_id) {
>> +	if (fb_id || solid_fill_id) {
>>   		uint32_t src_x = primary->values[IGT_PLANE_SRC_X] >> 16;
>>   		uint32_t src_y = primary->values[IGT_PLANE_SRC_Y] >> 16;
>>   
>> @@ -4935,6 +4945,27 @@ igt_plane_t *igt_output_get_plane_type_index(igt_output_t *output,
>>   	return igt_pipe_get_plane_type_index(pipe, plane_type, index);
>>   }
>>   
> 
> Please add description to each new public function.
> 
> Regards,
> Kamil
> 
>> +void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
>> +		int solid_fill_id)
>> +{
>> +	igt_pipe_t *pipe = plane->pipe;
>> +	igt_display_t *display = pipe->display;
>> +	int width = rect->x2 - rect->x1;
>> +	int height = rect->y2 - rect->y1;
>> +
>> +	igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_ID,
>> +			solid_fill_id ? pipe->crtc_id : 0);
>> +	igt_plane_set_prop_value(plane, IGT_PLANE_SOLID_FILL, solid_fill_id);
>> +
>> +	plane->gem_handle = 0;
>> +
>> +	LOG(display, "%s.%d: plane_set_solid_fill(%d)\n", kmstest_pipe_name(pipe->pipe),
>> +	    plane->index, solid_fill_id);
>> +
>> +	igt_plane_set_position(plane, rect->x1, rect->y1);
>> +	igt_plane_set_size(plane, width, height);
>> +}
>> +
>>   /**
>>    * igt_plane_set_fb:
>>    * @plane: Plane
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index b3882808b42f..9c05272abe1e 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -330,6 +330,8 @@ enum igt_atomic_plane_properties {
>>   #define IGT_PLANE_COORD_CHANGED_MASK 0xff
>>   
>>          IGT_PLANE_FB_ID,
>> +       IGT_PLANE_SOLID_FILL,
>> +       IGT_PLANE_PIXEL_SOURCE,
>>          IGT_PLANE_CRTC_ID,
>>          IGT_PLANE_IN_FENCE_FD,
>>          IGT_PLANE_TYPE,
>> @@ -537,6 +539,8 @@ igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe p
>>   
>>   void igt_pipe_request_out_fence(igt_pipe_t *pipe);
>>   
>> +void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
>> +		int solid_fill_id);
>>   void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
>>   void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd);
>>   void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe);
>>
>> -- 
>> 2.43.0
>>

Re: [PATCH i-g-t 2/4] lib: Add support for solid_fill and pixel_source plane properties

[Jessica Zhang]

On 12/19/2023 5:51 AM, Kamil Konieczny wrote:
> Hi Jessica,
> On 2023-12-15 at 16:40:22 -0800, Jessica Zhang wrote:
>> Add corresponding IGT macros for solid_fill and pixel_source DRM
>> properties, and change IGT commit behavior to allow for NULL FB commits.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   lib/igt_kms.c | 37 ++++++++++++++++++++++++++++++++++---
>>   lib/igt_kms.h |  4 ++++
>>   2 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index e4dea1a604dc..6a339766ca31 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -669,6 +669,8 @@ const char * const igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>>   	[IGT_PLANE_HOTSPOT_X] = "HOTSPOT_X",
>>   	[IGT_PLANE_HOTSPOT_Y] = "HOTSPOT_Y",
>>   	[IGT_PLANE_FB_ID] = "FB_ID",
>> +	[IGT_PLANE_SOLID_FILL] = "solid_fill",
>> +	[IGT_PLANE_PIXEL_SOURCE] = "pixel_source",
>>   	[IGT_PLANE_CRTC_ID] = "CRTC_ID",
>>   	[IGT_PLANE_IN_FENCE_FD] = "IN_FENCE_FD",
>>   	[IGT_PLANE_TYPE] = "type",
>> @@ -3417,6 +3419,12 @@ static igt_output_t *igt_pipe_get_output(igt_pipe_t *pipe)
>>   	return NULL;
>>   }
>>   
>> +static uint32_t igt_plane_get_solid_fill_id(igt_plane_t *plane)
>> +{
>> +	return plane->values[IGT_PLANE_SOLID_FILL];
>> +}
>> +
>> +
> 
> Remove one empty line.

Hi Kamil,

Thanks for taking a look at this series. Will make the changes you 
suggested in this patch and in the following patch.

Thanks,

Jessica Zhang

> 
>>   static uint32_t igt_plane_get_fb_id(igt_plane_t *plane)
>>   {
>>   	return plane->values[IGT_PLANE_FB_ID];
>> @@ -3648,7 +3656,7 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
>>   	struct igt_display *display = primary->pipe->display;
>>   	igt_output_t *output = igt_pipe_get_output(pipe);
>>   	drmModeModeInfo *mode;
>> -	uint32_t fb_id, crtc_id;
>> +	uint32_t fb_id, solid_fill_id, crtc_id;
>>   	int ret;
>>   
>>   	/* Primary planes can't be windowed when using a legacy commit */
>> @@ -3665,12 +3673,14 @@ static int igt_primary_plane_commit_legacy(igt_plane_t *primary,
>>   
>>   	crtc_id = pipe->crtc_id;
>>   	fb_id = output ? igt_plane_get_fb_id(primary) : 0;
>> -	if (fb_id)
>> +	solid_fill_id = output ? igt_plane_get_solid_fill_id(primary) : 0;
>> +
>> +	if (fb_id || solid_fill_id)
>>   		mode = igt_output_get_mode(output);
>>   	else
>>   		mode = NULL;
>>   
>> -	if (fb_id) {
>> +	if (fb_id || solid_fill_id) {
>>   		uint32_t src_x = primary->values[IGT_PLANE_SRC_X] >> 16;
>>   		uint32_t src_y = primary->values[IGT_PLANE_SRC_Y] >> 16;
>>   
>> @@ -4935,6 +4945,27 @@ igt_plane_t *igt_output_get_plane_type_index(igt_output_t *output,
>>   	return igt_pipe_get_plane_type_index(pipe, plane_type, index);
>>   }
>>   
> 
> Please add description to each new public function.
> 
> Regards,
> Kamil
> 
>> +void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
>> +		int solid_fill_id)
>> +{
>> +	igt_pipe_t *pipe = plane->pipe;
>> +	igt_display_t *display = pipe->display;
>> +	int width = rect->x2 - rect->x1;
>> +	int height = rect->y2 - rect->y1;
>> +
>> +	igt_plane_set_prop_value(plane, IGT_PLANE_CRTC_ID,
>> +			solid_fill_id ? pipe->crtc_id : 0);
>> +	igt_plane_set_prop_value(plane, IGT_PLANE_SOLID_FILL, solid_fill_id);
>> +
>> +	plane->gem_handle = 0;
>> +
>> +	LOG(display, "%s.%d: plane_set_solid_fill(%d)\n", kmstest_pipe_name(pipe->pipe),
>> +	    plane->index, solid_fill_id);
>> +
>> +	igt_plane_set_position(plane, rect->x1, rect->y1);
>> +	igt_plane_set_size(plane, width, height);
>> +}
>> +
>>   /**
>>    * igt_plane_set_fb:
>>    * @plane: Plane
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index b3882808b42f..9c05272abe1e 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -330,6 +330,8 @@ enum igt_atomic_plane_properties {
>>   #define IGT_PLANE_COORD_CHANGED_MASK 0xff
>>   
>>          IGT_PLANE_FB_ID,
>> +       IGT_PLANE_SOLID_FILL,
>> +       IGT_PLANE_PIXEL_SOURCE,
>>          IGT_PLANE_CRTC_ID,
>>          IGT_PLANE_IN_FENCE_FD,
>>          IGT_PLANE_TYPE,
>> @@ -537,6 +539,8 @@ igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe p
>>   
>>   void igt_pipe_request_out_fence(igt_pipe_t *pipe);
>>   
>> +void igt_plane_set_solid_fill(igt_plane_t *plane, struct drm_mode_rect *rect,
>> +		int solid_fill_id);
>>   void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb);
>>   void igt_plane_set_fence_fd(igt_plane_t *plane, int fence_fd);
>>   void igt_plane_set_pipe(igt_plane_t *plane, igt_pipe_t *pipe);
>>
>> -- 
>> 2.43.0
>>

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

On Tue, 19 Dec 2023 14:47:20 +0100
Kamil Konieczny <kamil.konieczny@linux.intel.com> wrote:

> Hi Pekka,
> 
> please subscribe to igt-dev mailinglist, you can do it here:
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

I have done that and disabled email delivery.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5397 bytes --]

On Thu, 21 Dec 2023 15:57:51 -0800
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:

> On 12/18/2023 2:12 AM, Pekka Paalanen wrote:
> > On Fri, 15 Dec 2023 16:40:23 -0800
> > Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >   
> >> Add a basic test for solid fill planes.
> >>
> >> This test will first commit a single-color framebuffer plane then
> >> a solid fill plane with the same contents. It then validates the solid
> >> fill plane by comparing the resulting CRC with the CRC of the reference
> >> framebuffer commit.
> >>
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >> ---
> >>   tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 94 insertions(+)
> >>
> >> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> >> old mode 100644
> >> new mode 100755
> >> index 2b6e9a8f0383..8f81e65ad84f
> >> --- a/tests/kms_atomic.c
> >> +++ b/tests/kms_atomic.c
> >> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
> >>   	PLANE_RELAX_FB = (1 << 1)
> >>   };
> >>   
> >> +struct solid_fill_blob {
> >> +	uint32_t r;
> >> +	uint32_t g;
> >> +	uint32_t b;
> >> +	uint32_t pad;
> >> +};
> >> +
> >>   static inline int damage_rect_width(struct drm_mode_rect *r)
> >>   {
> >>   	return r->x2 - r->x1;
> >> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
> >>   	igt_remove_fb(data->drm_fd, &fb_2);
> >>   }
> >>   
> >> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
> >> +{
> >> +	struct drm_mode_create_blob c;
> >> +	struct drm_mode_destroy_blob d;
> >> +	drmModeModeInfo *mode = igt_output_get_mode(output);
> >> +	struct drm_mode_rect rect = { 0 };
> >> +	struct igt_fb ref_fb;
> >> +	igt_pipe_crc_t *pipe_crc;
> >> +	igt_crc_t ref_crc, new_crc;
> >> +	enum pipe pipe = data->pipe->pipe;
> >> +	int height, width;
> >> +	int ret;
> >> +
> >> +	struct solid_fill_blob blob_data = {
> >> +		.r = 0x00000000,
> >> +		.g = 0x00000000,
> >> +		.b = 0xff000000,
> >> +		.pad = 0x0,
> >> +	};  
> > 
> > Hi Jessica!
> > 
> > This is the blob sent to KMS as the solid fill color...
> > 
> > ...
> >   
> >> +	igt_create_color_fb(data->drm_fd, width, height,
> >> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> >> +			    0.0, 0.0, 1.0, &ref_fb);  
> > 
> > ..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
> > values, I presume.
> > 
> > So you say that 0xff000000 = 1.0.
> > 
> > However, the patch for the kernel UAPI header says this:
> > 
> > +/**
> > + * struct drm_mode_solid_fill - User info for solid fill planes
> > + *
> > + * This is the userspace API solid fill information structure.
> > + *
> > + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
> > + * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
> > + * color and setting the pixel source to "SOLID_FILL".
> > + *
> > + * For information on the plane property, see drm_plane_create_solid_fill_property()
> > + *
> > + * @r: Red color value of single pixel
> > + * @g: Green color value of single pixel
> > + * @b: Blue color value of single pixel
> > + * @pad: padding, must be zero
> > + */
> > +struct drm_mode_solid_fill {
> > +	__u32 r;
> > +	__u32 g;
> > +	__u32 b;
> > +	__u32 pad;
> > +};
> > 
> > I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
> > format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
> > bug in the test.  
> 
> Hey Pekka,
> 
> Ah, thanks for catching this -- I'll change the blob value to 0xffffffff 
> so it matches the 1.0.
> 
> While we're talking about the UAPI struct, I'll also add the actual 
> drm_mode_solid_fill struct to the IGT drm-uapi instead of the current 
> workaround.
> 
> > 
> > It would be good to test more than one color:
> > - 0.0, 0.0, 0.0
> > - 1.0, 0.0, 0.0
> > - 0.0, 1.0, 0.0
> > - 0.0, 0.0, 1.0
> > - 1.0, 1.0, 1.0  
> 
> Sounds good, will change the test to validate these combinations.
> 
> > 
> > for example. That would get at least the so often used black explicitly
> > tested, and verify each channel gets mapped correctly rather than only
> > blue.
> > 
> > It would also be really good to test dim and mid grays, but I assume it
> > might be difficult to get CRC to match over various hardware. You'd
> > need to use writeback with an error tolerance. (For watching photos for
> > example, the background is not usually black but dim gray I believe.)  
> 
> Got it, we can add this to the list of colors to test.
> 
> FWIW, I think as long as we keep the test structure as grabbing a 
> reference CRC from an FB commit then comparing that to a CRC from a 
> solid fill commit, I'm not expecting a difference in CRC values.

The worry I had here, is that different hardware may have different
precision for the solid fill. Maybe that can be worked around by
computing the solid fill blob values from the raw FB pixel values? Then
even if something gets rounded/truncated somewhere in the hardware, the
end result should be the same between FB and solid fill, right?

Unless, the hardware precision on solid fill values is less than FB
pixel precision, and the CRC input precision is high enough to show
that difference.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Jessica Zhang]

On 1/11/2024 1:20 AM, Pekka Paalanen wrote:
> On Thu, 21 Dec 2023 15:57:51 -0800
> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> 
>> On 12/18/2023 2:12 AM, Pekka Paalanen wrote:
>>> On Fri, 15 Dec 2023 16:40:23 -0800
>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>    
>>>> Add a basic test for solid fill planes.
>>>>
>>>> This test will first commit a single-color framebuffer plane then
>>>> a solid fill plane with the same contents. It then validates the solid
>>>> fill plane by comparing the resulting CRC with the CRC of the reference
>>>> framebuffer commit.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>> ---
>>>>    tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 94 insertions(+)
>>>>
>>>> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
>>>> old mode 100644
>>>> new mode 100755
>>>> index 2b6e9a8f0383..8f81e65ad84f
>>>> --- a/tests/kms_atomic.c
>>>> +++ b/tests/kms_atomic.c
>>>> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
>>>>    	PLANE_RELAX_FB = (1 << 1)
>>>>    };
>>>>    
>>>> +struct solid_fill_blob {
>>>> +	uint32_t r;
>>>> +	uint32_t g;
>>>> +	uint32_t b;
>>>> +	uint32_t pad;
>>>> +};
>>>> +
>>>>    static inline int damage_rect_width(struct drm_mode_rect *r)
>>>>    {
>>>>    	return r->x2 - r->x1;
>>>> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
>>>>    	igt_remove_fb(data->drm_fd, &fb_2);
>>>>    }
>>>>    
>>>> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
>>>> +{
>>>> +	struct drm_mode_create_blob c;
>>>> +	struct drm_mode_destroy_blob d;
>>>> +	drmModeModeInfo *mode = igt_output_get_mode(output);
>>>> +	struct drm_mode_rect rect = { 0 };
>>>> +	struct igt_fb ref_fb;
>>>> +	igt_pipe_crc_t *pipe_crc;
>>>> +	igt_crc_t ref_crc, new_crc;
>>>> +	enum pipe pipe = data->pipe->pipe;
>>>> +	int height, width;
>>>> +	int ret;
>>>> +
>>>> +	struct solid_fill_blob blob_data = {
>>>> +		.r = 0x00000000,
>>>> +		.g = 0x00000000,
>>>> +		.b = 0xff000000,
>>>> +		.pad = 0x0,
>>>> +	};
>>>
>>> Hi Jessica!
>>>
>>> This is the blob sent to KMS as the solid fill color...
>>>
>>> ...
>>>    
>>>> +	igt_create_color_fb(data->drm_fd, width, height,
>>>> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
>>>> +			    0.0, 0.0, 1.0, &ref_fb);
>>>
>>> ..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
>>> values, I presume.
>>>
>>> So you say that 0xff000000 = 1.0.
>>>
>>> However, the patch for the kernel UAPI header says this:
>>>
>>> +/**
>>> + * struct drm_mode_solid_fill - User info for solid fill planes
>>> + *
>>> + * This is the userspace API solid fill information structure.
>>> + *
>>> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
>>> + * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
>>> + * color and setting the pixel source to "SOLID_FILL".
>>> + *
>>> + * For information on the plane property, see drm_plane_create_solid_fill_property()
>>> + *
>>> + * @r: Red color value of single pixel
>>> + * @g: Green color value of single pixel
>>> + * @b: Blue color value of single pixel
>>> + * @pad: padding, must be zero
>>> + */
>>> +struct drm_mode_solid_fill {
>>> +	__u32 r;
>>> +	__u32 g;
>>> +	__u32 b;
>>> +	__u32 pad;
>>> +};
>>>
>>> I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
>>> format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
>>> bug in the test.
>>
>> Hey Pekka,
>>
>> Ah, thanks for catching this -- I'll change the blob value to 0xffffffff
>> so it matches the 1.0.
>>
>> While we're talking about the UAPI struct, I'll also add the actual
>> drm_mode_solid_fill struct to the IGT drm-uapi instead of the current
>> workaround.
>>
>>>
>>> It would be good to test more than one color:
>>> - 0.0, 0.0, 0.0
>>> - 1.0, 0.0, 0.0
>>> - 0.0, 1.0, 0.0
>>> - 0.0, 0.0, 1.0
>>> - 1.0, 1.0, 1.0
>>
>> Sounds good, will change the test to validate these combinations.
>>
>>>
>>> for example. That would get at least the so often used black explicitly
>>> tested, and verify each channel gets mapped correctly rather than only
>>> blue.
>>>
>>> It would also be really good to test dim and mid grays, but I assume it
>>> might be difficult to get CRC to match over various hardware. You'd
>>> need to use writeback with an error tolerance. (For watching photos for
>>> example, the background is not usually black but dim gray I believe.)
>>
>> Got it, we can add this to the list of colors to test.
>>
>> FWIW, I think as long as we keep the test structure as grabbing a
>> reference CRC from an FB commit then comparing that to a CRC from a
>> solid fill commit, I'm not expecting a difference in CRC values.
> 
> The worry I had here, is that different hardware may have different
> precision for the solid fill. Maybe that can be worked around by
> computing the solid fill blob values from the raw FB pixel values? Then
> even if something gets rounded/truncated somewhere in the hardware, the
> end result should be the same between FB and solid fill, right?

Hi Pekka,

Got it -- I see what you mean.

In that case, can we stick to just testing the basic RGB + black/white 
colors? I want to avoid adding writeback as a dependency for the solid 
fill test since it's not a dependency for the solid fill feature itself.

Thanks,

Jessica Zhang

> 
> Unless, the hardware precision on solid fill values is less than FB
> pixel precision, and the CRC input precision is high enough to show
> that difference.
> 
> 
> Thanks,
> pq

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 6565 bytes --]

On Thu, 18 Jan 2024 15:35:10 -0800
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:

> On 1/11/2024 1:20 AM, Pekka Paalanen wrote:
> > On Thu, 21 Dec 2023 15:57:51 -0800
> > Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >   
> >> On 12/18/2023 2:12 AM, Pekka Paalanen wrote:  
> >>> On Fri, 15 Dec 2023 16:40:23 -0800
> >>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >>>      
> >>>> Add a basic test for solid fill planes.
> >>>>
> >>>> This test will first commit a single-color framebuffer plane then
> >>>> a solid fill plane with the same contents. It then validates the solid
> >>>> fill plane by comparing the resulting CRC with the CRC of the reference
> >>>> framebuffer commit.
> >>>>
> >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>>> ---
> >>>>    tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 94 insertions(+)
> >>>>
> >>>> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> >>>> old mode 100644
> >>>> new mode 100755
> >>>> index 2b6e9a8f0383..8f81e65ad84f
> >>>> --- a/tests/kms_atomic.c
> >>>> +++ b/tests/kms_atomic.c
> >>>> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
> >>>>    	PLANE_RELAX_FB = (1 << 1)
> >>>>    };
> >>>>    
> >>>> +struct solid_fill_blob {
> >>>> +	uint32_t r;
> >>>> +	uint32_t g;
> >>>> +	uint32_t b;
> >>>> +	uint32_t pad;
> >>>> +};
> >>>> +
> >>>>    static inline int damage_rect_width(struct drm_mode_rect *r)
> >>>>    {
> >>>>    	return r->x2 - r->x1;
> >>>> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
> >>>>    	igt_remove_fb(data->drm_fd, &fb_2);
> >>>>    }
> >>>>    
> >>>> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
> >>>> +{
> >>>> +	struct drm_mode_create_blob c;
> >>>> +	struct drm_mode_destroy_blob d;
> >>>> +	drmModeModeInfo *mode = igt_output_get_mode(output);
> >>>> +	struct drm_mode_rect rect = { 0 };
> >>>> +	struct igt_fb ref_fb;
> >>>> +	igt_pipe_crc_t *pipe_crc;
> >>>> +	igt_crc_t ref_crc, new_crc;
> >>>> +	enum pipe pipe = data->pipe->pipe;
> >>>> +	int height, width;
> >>>> +	int ret;
> >>>> +
> >>>> +	struct solid_fill_blob blob_data = {
> >>>> +		.r = 0x00000000,
> >>>> +		.g = 0x00000000,
> >>>> +		.b = 0xff000000,
> >>>> +		.pad = 0x0,
> >>>> +	};  
> >>>
> >>> Hi Jessica!
> >>>
> >>> This is the blob sent to KMS as the solid fill color...
> >>>
> >>> ...
> >>>      
> >>>> +	igt_create_color_fb(data->drm_fd, width, height,
> >>>> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> >>>> +			    0.0, 0.0, 1.0, &ref_fb);  
> >>>
> >>> ..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
> >>> values, I presume.
> >>>
> >>> So you say that 0xff000000 = 1.0.
> >>>
> >>> However, the patch for the kernel UAPI header says this:
> >>>
> >>> +/**
> >>> + * struct drm_mode_solid_fill - User info for solid fill planes
> >>> + *
> >>> + * This is the userspace API solid fill information structure.
> >>> + *
> >>> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
> >>> + * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
> >>> + * color and setting the pixel source to "SOLID_FILL".
> >>> + *
> >>> + * For information on the plane property, see drm_plane_create_solid_fill_property()
> >>> + *
> >>> + * @r: Red color value of single pixel
> >>> + * @g: Green color value of single pixel
> >>> + * @b: Blue color value of single pixel
> >>> + * @pad: padding, must be zero
> >>> + */
> >>> +struct drm_mode_solid_fill {
> >>> +	__u32 r;
> >>> +	__u32 g;
> >>> +	__u32 b;
> >>> +	__u32 pad;
> >>> +};
> >>>
> >>> I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
> >>> format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
> >>> bug in the test.  
> >>
> >> Hey Pekka,
> >>
> >> Ah, thanks for catching this -- I'll change the blob value to 0xffffffff
> >> so it matches the 1.0.
> >>
> >> While we're talking about the UAPI struct, I'll also add the actual
> >> drm_mode_solid_fill struct to the IGT drm-uapi instead of the current
> >> workaround.
> >>  
> >>>
> >>> It would be good to test more than one color:
> >>> - 0.0, 0.0, 0.0
> >>> - 1.0, 0.0, 0.0
> >>> - 0.0, 1.0, 0.0
> >>> - 0.0, 0.0, 1.0
> >>> - 1.0, 1.0, 1.0  
> >>
> >> Sounds good, will change the test to validate these combinations.
> >>  
> >>>
> >>> for example. That would get at least the so often used black explicitly
> >>> tested, and verify each channel gets mapped correctly rather than only
> >>> blue.
> >>>
> >>> It would also be really good to test dim and mid grays, but I assume it
> >>> might be difficult to get CRC to match over various hardware. You'd
> >>> need to use writeback with an error tolerance. (For watching photos for
> >>> example, the background is not usually black but dim gray I believe.)  
> >>
> >> Got it, we can add this to the list of colors to test.
> >>
> >> FWIW, I think as long as we keep the test structure as grabbing a
> >> reference CRC from an FB commit then comparing that to a CRC from a
> >> solid fill commit, I'm not expecting a difference in CRC values.  
> > 
> > The worry I had here, is that different hardware may have different
> > precision for the solid fill. Maybe that can be worked around by
> > computing the solid fill blob values from the raw FB pixel values? Then
> > even if something gets rounded/truncated somewhere in the hardware, the
> > end result should be the same between FB and solid fill, right?  
> 
> Hi Pekka,
> 
> Got it -- I see what you mean.
> 
> In that case, can we stick to just testing the basic RGB + black/white 
> colors? I want to avoid adding writeback as a dependency for the solid 
> fill test since it's not a dependency for the solid fill feature itself.

Up to you. My worries here are hypothetical, I don't know about
hardware. It's also possible to adjust tests later if people find false
positives or false negatives.

Personally, I'd start with optimistically strict tests. False negatives
will be found by driver developers easily, while false negatives would
need an actual user reporting a bug.


Thanks,
pq

> > 
> > Unless, the hardware precision on solid fill values is less than FB
> > pixel precision, and the CRC input precision is high enough to show
> > that difference.
> > 
> > 
> > Thanks,
> > pq  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Abhinav Kumar]

On 1/19/2024 1:00 AM, Pekka Paalanen wrote:
> On Thu, 18 Jan 2024 15:35:10 -0800
> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> 
>> On 1/11/2024 1:20 AM, Pekka Paalanen wrote:
>>> On Thu, 21 Dec 2023 15:57:51 -0800
>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>    
>>>> On 12/18/2023 2:12 AM, Pekka Paalanen wrote:
>>>>> On Fri, 15 Dec 2023 16:40:23 -0800
>>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>>       
>>>>>> Add a basic test for solid fill planes.
>>>>>>
>>>>>> This test will first commit a single-color framebuffer plane then
>>>>>> a solid fill plane with the same contents. It then validates the solid
>>>>>> fill plane by comparing the resulting CRC with the CRC of the reference
>>>>>> framebuffer commit.
>>>>>>
>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>> ---
>>>>>>     tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 94 insertions(+)
>>>>>>
>>>>>> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
>>>>>> old mode 100644
>>>>>> new mode 100755
>>>>>> index 2b6e9a8f0383..8f81e65ad84f
>>>>>> --- a/tests/kms_atomic.c
>>>>>> +++ b/tests/kms_atomic.c
>>>>>> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
>>>>>>     	PLANE_RELAX_FB = (1 << 1)
>>>>>>     };
>>>>>>     
>>>>>> +struct solid_fill_blob {
>>>>>> +	uint32_t r;
>>>>>> +	uint32_t g;
>>>>>> +	uint32_t b;
>>>>>> +	uint32_t pad;
>>>>>> +};
>>>>>> +
>>>>>>     static inline int damage_rect_width(struct drm_mode_rect *r)
>>>>>>     {
>>>>>>     	return r->x2 - r->x1;
>>>>>> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
>>>>>>     	igt_remove_fb(data->drm_fd, &fb_2);
>>>>>>     }
>>>>>>     
>>>>>> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
>>>>>> +{
>>>>>> +	struct drm_mode_create_blob c;
>>>>>> +	struct drm_mode_destroy_blob d;
>>>>>> +	drmModeModeInfo *mode = igt_output_get_mode(output);
>>>>>> +	struct drm_mode_rect rect = { 0 };
>>>>>> +	struct igt_fb ref_fb;
>>>>>> +	igt_pipe_crc_t *pipe_crc;
>>>>>> +	igt_crc_t ref_crc, new_crc;
>>>>>> +	enum pipe pipe = data->pipe->pipe;
>>>>>> +	int height, width;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	struct solid_fill_blob blob_data = {
>>>>>> +		.r = 0x00000000,
>>>>>> +		.g = 0x00000000,
>>>>>> +		.b = 0xff000000,
>>>>>> +		.pad = 0x0,
>>>>>> +	};
>>>>>
>>>>> Hi Jessica!
>>>>>
>>>>> This is the blob sent to KMS as the solid fill color...
>>>>>
>>>>> ...
>>>>>       
>>>>>> +	igt_create_color_fb(data->drm_fd, width, height,
>>>>>> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
>>>>>> +			    0.0, 0.0, 1.0, &ref_fb);
>>>>>
>>>>> ..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
>>>>> values, I presume.
>>>>>
>>>>> So you say that 0xff000000 = 1.0.
>>>>>
>>>>> However, the patch for the kernel UAPI header says this:
>>>>>
>>>>> +/**
>>>>> + * struct drm_mode_solid_fill - User info for solid fill planes
>>>>> + *
>>>>> + * This is the userspace API solid fill information structure.
>>>>> + *
>>>>> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
>>>>> + * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
>>>>> + * color and setting the pixel source to "SOLID_FILL".
>>>>> + *
>>>>> + * For information on the plane property, see drm_plane_create_solid_fill_property()
>>>>> + *
>>>>> + * @r: Red color value of single pixel
>>>>> + * @g: Green color value of single pixel
>>>>> + * @b: Blue color value of single pixel
>>>>> + * @pad: padding, must be zero
>>>>> + */
>>>>> +struct drm_mode_solid_fill {
>>>>> +	__u32 r;
>>>>> +	__u32 g;
>>>>> +	__u32 b;
>>>>> +	__u32 pad;
>>>>> +};
>>>>>
>>>>> I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
>>>>> format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
>>>>> bug in the test.
>>>>
>>>> Hey Pekka,
>>>>
>>>> Ah, thanks for catching this -- I'll change the blob value to 0xffffffff
>>>> so it matches the 1.0.
>>>>
>>>> While we're talking about the UAPI struct, I'll also add the actual
>>>> drm_mode_solid_fill struct to the IGT drm-uapi instead of the current
>>>> workaround.
>>>>   
>>>>>
>>>>> It would be good to test more than one color:
>>>>> - 0.0, 0.0, 0.0
>>>>> - 1.0, 0.0, 0.0
>>>>> - 0.0, 1.0, 0.0
>>>>> - 0.0, 0.0, 1.0
>>>>> - 1.0, 1.0, 1.0
>>>>
>>>> Sounds good, will change the test to validate these combinations.
>>>>   
>>>>>
>>>>> for example. That would get at least the so often used black explicitly
>>>>> tested, and verify each channel gets mapped correctly rather than only
>>>>> blue.
>>>>>
>>>>> It would also be really good to test dim and mid grays, but I assume it
>>>>> might be difficult to get CRC to match over various hardware. You'd
>>>>> need to use writeback with an error tolerance. (For watching photos for
>>>>> example, the background is not usually black but dim gray I believe.)
>>>>
>>>> Got it, we can add this to the list of colors to test.
>>>>
>>>> FWIW, I think as long as we keep the test structure as grabbing a
>>>> reference CRC from an FB commit then comparing that to a CRC from a
>>>> solid fill commit, I'm not expecting a difference in CRC values.
>>>
>>> The worry I had here, is that different hardware may have different
>>> precision for the solid fill. Maybe that can be worked around by
>>> computing the solid fill blob values from the raw FB pixel values? Then
>>> even if something gets rounded/truncated somewhere in the hardware, the
>>> end result should be the same between FB and solid fill, right?
>>
>> Hi Pekka,
>>
>> Got it -- I see what you mean.
>>
>> In that case, can we stick to just testing the basic RGB + black/white
>> colors? I want to avoid adding writeback as a dependency for the solid
>> fill test since it's not a dependency for the solid fill feature itself.
> 
> Up to you. My worries here are hypothetical, I don't know about
> hardware. It's also possible to adjust tests later if people find false
> positives or false negatives.
> 
> Personally, I'd start with optimistically strict tests. False negatives
> will be found by driver developers easily, while false negatives would
> need an actual user reporting a bug.
> 
> 
> Thanks,
> pq
> 

I agree with Jessica here. We can start with the basic RGB + black/white.

We dont want to put an extra writeback dependency on this test as there 
is no hardware dependency of solid fill planes with writeback connector.

If others would like to extend this later on based on the writeback 
client cap to add another sub-test that would be a separate effort.

>>>
>>> Unless, the hardware precision on solid fill values is less than FB
>>> pixel precision, and the CRC input precision is high enough to show
>>> that difference.
>>>
>>>
>>> Thanks,
>>> pq
> 

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 7730 bytes --]

On Fri, 19 Jan 2024 09:40:52 -0800
Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:

> On 1/19/2024 1:00 AM, Pekka Paalanen wrote:
> > On Thu, 18 Jan 2024 15:35:10 -0800
> > Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >   
> >> On 1/11/2024 1:20 AM, Pekka Paalanen wrote:  
> >>> On Thu, 21 Dec 2023 15:57:51 -0800
> >>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >>>      
> >>>> On 12/18/2023 2:12 AM, Pekka Paalanen wrote:  
> >>>>> On Fri, 15 Dec 2023 16:40:23 -0800
> >>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >>>>>         
> >>>>>> Add a basic test for solid fill planes.
> >>>>>>
> >>>>>> This test will first commit a single-color framebuffer plane then
> >>>>>> a solid fill plane with the same contents. It then validates the solid
> >>>>>> fill plane by comparing the resulting CRC with the CRC of the reference
> >>>>>> framebuffer commit.
> >>>>>>
> >>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>>>>> ---
> >>>>>>     tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>     1 file changed, 94 insertions(+)
> >>>>>>
> >>>>>> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
> >>>>>> old mode 100644
> >>>>>> new mode 100755
> >>>>>> index 2b6e9a8f0383..8f81e65ad84f
> >>>>>> --- a/tests/kms_atomic.c
> >>>>>> +++ b/tests/kms_atomic.c
> >>>>>> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
> >>>>>>     	PLANE_RELAX_FB = (1 << 1)
> >>>>>>     };
> >>>>>>     
> >>>>>> +struct solid_fill_blob {
> >>>>>> +	uint32_t r;
> >>>>>> +	uint32_t g;
> >>>>>> +	uint32_t b;
> >>>>>> +	uint32_t pad;
> >>>>>> +};
> >>>>>> +
> >>>>>>     static inline int damage_rect_width(struct drm_mode_rect *r)
> >>>>>>     {
> >>>>>>     	return r->x2 - r->x1;
> >>>>>> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
> >>>>>>     	igt_remove_fb(data->drm_fd, &fb_2);
> >>>>>>     }
> >>>>>>     
> >>>>>> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
> >>>>>> +{
> >>>>>> +	struct drm_mode_create_blob c;
> >>>>>> +	struct drm_mode_destroy_blob d;
> >>>>>> +	drmModeModeInfo *mode = igt_output_get_mode(output);
> >>>>>> +	struct drm_mode_rect rect = { 0 };
> >>>>>> +	struct igt_fb ref_fb;
> >>>>>> +	igt_pipe_crc_t *pipe_crc;
> >>>>>> +	igt_crc_t ref_crc, new_crc;
> >>>>>> +	enum pipe pipe = data->pipe->pipe;
> >>>>>> +	int height, width;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	struct solid_fill_blob blob_data = {
> >>>>>> +		.r = 0x00000000,
> >>>>>> +		.g = 0x00000000,
> >>>>>> +		.b = 0xff000000,
> >>>>>> +		.pad = 0x0,
> >>>>>> +	};  
> >>>>>
> >>>>> Hi Jessica!
> >>>>>
> >>>>> This is the blob sent to KMS as the solid fill color...
> >>>>>
> >>>>> ...
> >>>>>         
> >>>>>> +	igt_create_color_fb(data->drm_fd, width, height,
> >>>>>> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> >>>>>> +			    0.0, 0.0, 1.0, &ref_fb);  
> >>>>>
> >>>>> ..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
> >>>>> values, I presume.
> >>>>>
> >>>>> So you say that 0xff000000 = 1.0.
> >>>>>
> >>>>> However, the patch for the kernel UAPI header says this:
> >>>>>
> >>>>> +/**
> >>>>> + * struct drm_mode_solid_fill - User info for solid fill planes
> >>>>> + *
> >>>>> + * This is the userspace API solid fill information structure.
> >>>>> + *
> >>>>> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
> >>>>> + * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
> >>>>> + * color and setting the pixel source to "SOLID_FILL".
> >>>>> + *
> >>>>> + * For information on the plane property, see drm_plane_create_solid_fill_property()
> >>>>> + *
> >>>>> + * @r: Red color value of single pixel
> >>>>> + * @g: Green color value of single pixel
> >>>>> + * @b: Blue color value of single pixel
> >>>>> + * @pad: padding, must be zero
> >>>>> + */
> >>>>> +struct drm_mode_solid_fill {
> >>>>> +	__u32 r;
> >>>>> +	__u32 g;
> >>>>> +	__u32 b;
> >>>>> +	__u32 pad;
> >>>>> +};
> >>>>>
> >>>>> I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
> >>>>> format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
> >>>>> bug in the test.  
> >>>>
> >>>> Hey Pekka,
> >>>>
> >>>> Ah, thanks for catching this -- I'll change the blob value to 0xffffffff
> >>>> so it matches the 1.0.
> >>>>
> >>>> While we're talking about the UAPI struct, I'll also add the actual
> >>>> drm_mode_solid_fill struct to the IGT drm-uapi instead of the current
> >>>> workaround.
> >>>>     
> >>>>>
> >>>>> It would be good to test more than one color:
> >>>>> - 0.0, 0.0, 0.0
> >>>>> - 1.0, 0.0, 0.0
> >>>>> - 0.0, 1.0, 0.0
> >>>>> - 0.0, 0.0, 1.0
> >>>>> - 1.0, 1.0, 1.0  
> >>>>
> >>>> Sounds good, will change the test to validate these combinations.
> >>>>     
> >>>>>
> >>>>> for example. That would get at least the so often used black explicitly
> >>>>> tested, and verify each channel gets mapped correctly rather than only
> >>>>> blue.
> >>>>>
> >>>>> It would also be really good to test dim and mid grays, but I assume it
> >>>>> might be difficult to get CRC to match over various hardware. You'd
> >>>>> need to use writeback with an error tolerance. (For watching photos for
> >>>>> example, the background is not usually black but dim gray I believe.)  
> >>>>
> >>>> Got it, we can add this to the list of colors to test.
> >>>>
> >>>> FWIW, I think as long as we keep the test structure as grabbing a
> >>>> reference CRC from an FB commit then comparing that to a CRC from a
> >>>> solid fill commit, I'm not expecting a difference in CRC values.  
> >>>
> >>> The worry I had here, is that different hardware may have different
> >>> precision for the solid fill. Maybe that can be worked around by
> >>> computing the solid fill blob values from the raw FB pixel values? Then
> >>> even if something gets rounded/truncated somewhere in the hardware, the
> >>> end result should be the same between FB and solid fill, right?  
> >>
> >> Hi Pekka,
> >>
> >> Got it -- I see what you mean.
> >>
> >> In that case, can we stick to just testing the basic RGB + black/white
> >> colors? I want to avoid adding writeback as a dependency for the solid
> >> fill test since it's not a dependency for the solid fill feature itself.  
> > 
> > Up to you. My worries here are hypothetical, I don't know about
> > hardware. It's also possible to adjust tests later if people find false
> > positives or false negatives.
> > 
> > Personally, I'd start with optimistically strict tests. False negatives
> > will be found by driver developers easily, while false negatives would
> > need an actual user reporting a bug.
> > 
> > 
> > Thanks,
> > pq
> >   
> 
> I agree with Jessica here. We can start with the basic RGB + black/white.
> 
> We dont want to put an extra writeback dependency on this test as there 
> is no hardware dependency of solid fill planes with writeback connector.

Yes, but you can still test mid-gray pixel values with CRC. Craft the
test values to carefully match the FB pixel values, and see if any
driver has a problem with it. Maybe it just works.


Thanks,
pq

> If others would like to extend this later on based on the writeback 
> client cap to add another sub-test that would be a separate effort.
> 
> >>>
> >>> Unless, the hardware precision on solid fill values is less than FB
> >>> pixel precision, and the CRC input precision is high enough to show
> >>> that difference.
> >>>
> >>>
> >>> Thanks,
> >>> pq  
> >   


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Jessica Zhang]

On 1/21/2024 11:56 PM, Pekka Paalanen wrote:
> On Fri, 19 Jan 2024 09:40:52 -0800
> Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> 
>> On 1/19/2024 1:00 AM, Pekka Paalanen wrote:
>>> On Thu, 18 Jan 2024 15:35:10 -0800
>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>    
>>>> On 1/11/2024 1:20 AM, Pekka Paalanen wrote:
>>>>> On Thu, 21 Dec 2023 15:57:51 -0800
>>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>>       
>>>>>> On 12/18/2023 2:12 AM, Pekka Paalanen wrote:
>>>>>>> On Fri, 15 Dec 2023 16:40:23 -0800
>>>>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>>>>          
>>>>>>>> Add a basic test for solid fill planes.
>>>>>>>>
>>>>>>>> This test will first commit a single-color framebuffer plane then
>>>>>>>> a solid fill plane with the same contents. It then validates the solid
>>>>>>>> fill plane by comparing the resulting CRC with the CRC of the reference
>>>>>>>> framebuffer commit.
>>>>>>>>
>>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>>> ---
>>>>>>>>      tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 94 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
>>>>>>>> old mode 100644
>>>>>>>> new mode 100755
>>>>>>>> index 2b6e9a8f0383..8f81e65ad84f
>>>>>>>> --- a/tests/kms_atomic.c
>>>>>>>> +++ b/tests/kms_atomic.c
>>>>>>>> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
>>>>>>>>      	PLANE_RELAX_FB = (1 << 1)
>>>>>>>>      };
>>>>>>>>      
>>>>>>>> +struct solid_fill_blob {
>>>>>>>> +	uint32_t r;
>>>>>>>> +	uint32_t g;
>>>>>>>> +	uint32_t b;
>>>>>>>> +	uint32_t pad;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>      static inline int damage_rect_width(struct drm_mode_rect *r)
>>>>>>>>      {
>>>>>>>>      	return r->x2 - r->x1;
>>>>>>>> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
>>>>>>>>      	igt_remove_fb(data->drm_fd, &fb_2);
>>>>>>>>      }
>>>>>>>>      
>>>>>>>> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
>>>>>>>> +{
>>>>>>>> +	struct drm_mode_create_blob c;
>>>>>>>> +	struct drm_mode_destroy_blob d;
>>>>>>>> +	drmModeModeInfo *mode = igt_output_get_mode(output);
>>>>>>>> +	struct drm_mode_rect rect = { 0 };
>>>>>>>> +	struct igt_fb ref_fb;
>>>>>>>> +	igt_pipe_crc_t *pipe_crc;
>>>>>>>> +	igt_crc_t ref_crc, new_crc;
>>>>>>>> +	enum pipe pipe = data->pipe->pipe;
>>>>>>>> +	int height, width;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	struct solid_fill_blob blob_data = {
>>>>>>>> +		.r = 0x00000000,
>>>>>>>> +		.g = 0x00000000,
>>>>>>>> +		.b = 0xff000000,
>>>>>>>> +		.pad = 0x0,
>>>>>>>> +	};
>>>>>>>
>>>>>>> Hi Jessica!
>>>>>>>
>>>>>>> This is the blob sent to KMS as the solid fill color...
>>>>>>>
>>>>>>> ...
>>>>>>>          
>>>>>>>> +	igt_create_color_fb(data->drm_fd, width, height,
>>>>>>>> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
>>>>>>>> +			    0.0, 0.0, 1.0, &ref_fb);
>>>>>>>
>>>>>>> ..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
>>>>>>> values, I presume.
>>>>>>>
>>>>>>> So you say that 0xff000000 = 1.0.
>>>>>>>
>>>>>>> However, the patch for the kernel UAPI header says this:
>>>>>>>
>>>>>>> +/**
>>>>>>> + * struct drm_mode_solid_fill - User info for solid fill planes
>>>>>>> + *
>>>>>>> + * This is the userspace API solid fill information structure.
>>>>>>> + *
>>>>>>> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
>>>>>>> + * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
>>>>>>> + * color and setting the pixel source to "SOLID_FILL".
>>>>>>> + *
>>>>>>> + * For information on the plane property, see drm_plane_create_solid_fill_property()
>>>>>>> + *
>>>>>>> + * @r: Red color value of single pixel
>>>>>>> + * @g: Green color value of single pixel
>>>>>>> + * @b: Blue color value of single pixel
>>>>>>> + * @pad: padding, must be zero
>>>>>>> + */
>>>>>>> +struct drm_mode_solid_fill {
>>>>>>> +	__u32 r;
>>>>>>> +	__u32 g;
>>>>>>> +	__u32 b;
>>>>>>> +	__u32 pad;
>>>>>>> +};
>>>>>>>
>>>>>>> I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
>>>>>>> format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
>>>>>>> bug in the test.
>>>>>>
>>>>>> Hey Pekka,
>>>>>>
>>>>>> Ah, thanks for catching this -- I'll change the blob value to 0xffffffff
>>>>>> so it matches the 1.0.
>>>>>>
>>>>>> While we're talking about the UAPI struct, I'll also add the actual
>>>>>> drm_mode_solid_fill struct to the IGT drm-uapi instead of the current
>>>>>> workaround.
>>>>>>      
>>>>>>>
>>>>>>> It would be good to test more than one color:
>>>>>>> - 0.0, 0.0, 0.0
>>>>>>> - 1.0, 0.0, 0.0
>>>>>>> - 0.0, 1.0, 0.0
>>>>>>> - 0.0, 0.0, 1.0
>>>>>>> - 1.0, 1.0, 1.0
>>>>>>
>>>>>> Sounds good, will change the test to validate these combinations.
>>>>>>      
>>>>>>>
>>>>>>> for example. That would get at least the so often used black explicitly
>>>>>>> tested, and verify each channel gets mapped correctly rather than only
>>>>>>> blue.
>>>>>>>
>>>>>>> It would also be really good to test dim and mid grays, but I assume it
>>>>>>> might be difficult to get CRC to match over various hardware. You'd
>>>>>>> need to use writeback with an error tolerance. (For watching photos for
>>>>>>> example, the background is not usually black but dim gray I believe.)
>>>>>>
>>>>>> Got it, we can add this to the list of colors to test.
>>>>>>
>>>>>> FWIW, I think as long as we keep the test structure as grabbing a
>>>>>> reference CRC from an FB commit then comparing that to a CRC from a
>>>>>> solid fill commit, I'm not expecting a difference in CRC values.
>>>>>
>>>>> The worry I had here, is that different hardware may have different
>>>>> precision for the solid fill. Maybe that can be worked around by
>>>>> computing the solid fill blob values from the raw FB pixel values? Then
>>>>> even if something gets rounded/truncated somewhere in the hardware, the
>>>>> end result should be the same between FB and solid fill, right?
>>>>
>>>> Hi Pekka,
>>>>
>>>> Got it -- I see what you mean.
>>>>
>>>> In that case, can we stick to just testing the basic RGB + black/white
>>>> colors? I want to avoid adding writeback as a dependency for the solid
>>>> fill test since it's not a dependency for the solid fill feature itself.
>>>
>>> Up to you. My worries here are hypothetical, I don't know about
>>> hardware. It's also possible to adjust tests later if people find false
>>> positives or false negatives.
>>>
>>> Personally, I'd start with optimistically strict tests. False negatives
>>> will be found by driver developers easily, while false negatives would
>>> need an actual user reporting a bug.
>>>
>>>
>>> Thanks,
>>> pq
>>>    
>>
>> I agree with Jessica here. We can start with the basic RGB + black/white.
>>
>> We dont want to put an extra writeback dependency on this test as there
>> is no hardware dependency of solid fill planes with writeback connector.
> 
> Yes, but you can still test mid-gray pixel values with CRC. Craft the
> test values to carefully match the FB pixel values, and see if any
> driver has a problem with it. Maybe it just works.

Hi Pekka,

Sounds good. I'll add a few mid gray test cases with pixel values that 
work for us.

Thanks,

Jessica Zhang

> 
> 
> Thanks,
> pq
> 
>> If others would like to extend this later on based on the writeback
>> client cap to add another sub-test that would be a separate effort.
>>
>>>>>
>>>>> Unless, the hardware precision on solid fill values is less than FB
>>>>> pixel precision, and the CRC input precision is high enough to show
>>>>> that difference.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> pq
>>>    
>