Re: [PATCH i-g-t v2 5/6] tests/kms_atomic: Add solid fill plane subtest

[Jessica Zhang <quic_jesszhan@quicinc.com>]

On 1/24/2024 1:13 AM, Pekka Paalanen wrote:
> On Tue, 23 Jan 2024 15:28:58 -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 | 179 +++++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 161 insertions(+), 18 deletions(-)
>>
>> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
>> old mode 100644
>> new mode 100755
>> index 2b6e9a8f0383..35ac5f198524
>> --- a/tests/kms_atomic.c
>> +++ b/tests/kms_atomic.c
>> @@ -43,6 +43,7 @@
>>   #include <stdint.h>
>>   #include <string.h>
>>   #include <errno.h>
>> +#include <math.h>
>>   #include <xf86drmMode.h>
>>   #include <cairo.h>
>>   #include "drm.h"
>> @@ -101,6 +102,15 @@
>>    * @params-fence:     fence parameters
>>    */
>>   
>> +/**
>> + * SUBTEST: plane-%s-solid-fill
>> + * Description: Test atomic modesetting on %arg[1] using solid fill property.
>> + *
>> + * arg[1]:
>> + *
>> + * @primary:        Primary plane
>> + */
>> +
>>   #ifndef DRM_CAP_CURSOR_WIDTH
>>   #define DRM_CAP_CURSOR_WIDTH 0x8
>>   #endif
>> @@ -122,18 +132,45 @@ typedef struct {
>>   	igt_fb_t fb;
>>   } data_t;
>>   
>> +static struct {
>> +	uint32_t r, g, b;
>> +} colors[8] = {
>> +	/* Basic Colors */
>> +	{ .r = 0x00000000, .g = 0x00000000, .b = 0x00000000 },
>> +	{ .r = 0xffffffff, .g = 0x00000000, .b = 0x00000000 },
>> +	{ .r = 0x00000000, .g = 0xffffffff, .b = 0x00000000 },
>> +	{ .r = 0x00000000, .g = 0x00000000, .b = 0xffffffff },
>> +	{ .r = 0xffffffff, .g = 0xffffffff, .b = 0xffffffff },
>> +
>> +	/* Mid-grays */
>> +	{ .r = 0x40000000, .g = 0x40000000, .b = 0x40000000 },
>> +	{ .r = 0x80000000, .g = 0x80000000, .b = 0x80000000 },
>> +	{ .r = 0xcccccccc, .g = 0xcccccccc, .b = 0xcccccccc },
>> +};
> 
> Hi,
> 
> the above looks good to me. More below.
> 
>> +
>>   enum kms_atomic_check_relax {
>>   	ATOMIC_RELAX_NONE = 0,
>>   	CRTC_RELAX_MODE = (1 << 0),
>>   	PLANE_RELAX_FB = (1 << 1)
>>   };
>>   
>> -static inline int damage_rect_width(struct drm_mode_rect *r)
>> +/*
>> + * Fixed point uint to floating point conversion based on
>> + * Vulkan's UNORM conversion:
>> + *
>> + * https://registry.khronos.org/vulkan/specs/1.3/html/chap3.html#fundamentals-fixedconv
>> + */
>> +static inline double unorm_to_float(uint32_t x)
>> +{
>> +	return x / (pow(2, 32) - 1);
>> +}
>> +
>> +static inline int rect_width(struct drm_mode_rect *r)
>>   {
>>   	return r->x2 - r->x1;
>>   }
>>   
>> -static inline int damage_rect_height(struct drm_mode_rect *r)
>> +static inline int rect_height(struct drm_mode_rect *r)
>>   {
>>   	return r->y2 - r->y1;
>>   }
>> @@ -1156,8 +1193,8 @@ static void atomic_plane_damage(data_t *data)
>>   
>>   	cr_1 = igt_get_cairo_ctx(data->drm_fd, &fb_1);
>>   	igt_paint_color(cr_1, damage[0].x1, damage[0].y1,
>> -			damage_rect_width(&damage[0]),
>> -			damage_rect_height(&damage[0]), 1.0, 0, 0);
>> +			rect_width(&damage[0]),
>> +			rect_height(&damage[0]), 1.0, 0, 0);
>>   	igt_put_cairo_ctx(cr_1);
>>   
>>   	igt_plane_set_fb(data->primary, &fb_1);
>> @@ -1183,8 +1220,8 @@ static void atomic_plane_damage(data_t *data)
>>   	cairo_set_source_surface(cr_2, fb_1.cairo_surface, 0, 0);
>>   	cairo_paint(cr_2);
>>   	igt_paint_color(cr_2, damage[0].x1, damage[0].y1,
>> -			damage_rect_width(&damage[0]),
>> -			damage_rect_height(&damage[0]), 0, 1.0, 0);
>> +			rect_width(&damage[0]),
>> +			rect_height(&damage[0]), 0, 1.0, 0);
>>   	igt_put_cairo_ctx(cr_1);
>>   	igt_put_cairo_ctx(cr_2);
>>   	igt_plane_set_fb(data->primary, &fb_2);
>> @@ -1213,8 +1250,8 @@ static void atomic_plane_damage(data_t *data)
>>   	cairo_set_source_surface(cr_1, fb_2.cairo_surface, 0, 0);
>>   	cairo_paint(cr_1);
>>   	igt_paint_color(cr_1, damage[0].x1, damage[0].y1,
>> -			damage_rect_width(&damage[0]),
>> -			damage_rect_height(&damage[0]), 0, 1.0, 0);
>> +			rect_width(&damage[0]),
>> +			rect_height(&damage[0]), 0, 1.0, 0);
>>   	igt_put_cairo_ctx(cr_2);
>>   	igt_put_cairo_ctx(cr_1);
>>   	igt_plane_set_fb(data->primary, &fb_1);
>> @@ -1238,8 +1275,8 @@ static void atomic_plane_damage(data_t *data)
>>   
>>   	cr_1 = igt_get_cairo_ctx(data->drm_fd, &fb_1);
>>   	igt_paint_color(cr_1, damage[0].x1, damage[0].y1,
>> -			damage_rect_width(&damage[0]),
>> -			damage_rect_height(&damage[0]), 1.0, 1.0, 0);
>> +			rect_width(&damage[0]),
>> +			rect_height(&damage[0]), 1.0, 1.0, 0);
>>   	igt_put_cairo_ctx(cr_1);
>>   	igt_plane_set_fb(data->primary, &fb_1);
>>   	igt_plane_set_size(data->primary, data->fb.width, data->fb.height);
>> @@ -1266,11 +1303,11 @@ static void atomic_plane_damage(data_t *data)
>>   
>>   	cr_1 = igt_get_cairo_ctx(data->drm_fd, &fb_1);
>>   	igt_paint_color(cr_1, damage[0].x1, damage[0].y1,
>> -			damage_rect_width(&damage[0]),
>> -			damage_rect_height(&damage[0]), 0, 1.0, 1.0);
>> +			rect_width(&damage[0]),
>> +			rect_height(&damage[0]), 0, 1.0, 1.0);
>>   	igt_paint_color(cr_1, damage[1].x1, damage[1].y1,
>> -			damage_rect_width(&damage[1]),
>> -			damage_rect_height(&damage[1]), 0, 1.0, 0);
>> +			rect_width(&damage[1]),
>> +			rect_height(&damage[1]), 0, 1.0, 0);
>>   	igt_put_cairo_ctx(cr_1);
>>   	igt_plane_set_fb(data->primary, &fb_1);
>>   	igt_plane_set_size(data->primary, data->fb.width, data->fb.height);
>> @@ -1299,11 +1336,11 @@ static void atomic_plane_damage(data_t *data)
>>   
>>   	cr_1 = igt_get_cairo_ctx(data->drm_fd, &fb_1);
>>   	igt_paint_color(cr_1, damage[0].x1, damage[0].y1,
>> -			damage_rect_width(&damage[0]),
>> -			damage_rect_height(&damage[0]), 1.0, 0, 0);
>> +			rect_width(&damage[0]),
>> +			rect_height(&damage[0]), 1.0, 0, 0);
>>   	igt_paint_color(cr_1, damage[1].x1, damage[1].y1,
>> -			damage_rect_width(&damage[1]),
>> -			damage_rect_height(&damage[1]), 1.0, 1.0, 1.0);
>> +			rect_width(&damage[1]),
>> +			rect_height(&damage[1]), 1.0, 1.0, 1.0);
>>   	igt_put_cairo_ctx(cr_1);
>>   	igt_plane_set_fb(data->primary, &fb_1);
>>   	igt_plane_set_size(data->primary, data->fb.width, data->fb.height);
>> @@ -1322,6 +1359,98 @@ 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)
>> +{
>> +	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, i;
>> +	int ret;
>> +
>> +	igt_require(igt_plane_has_prop(plane, IGT_PLANE_SOLID_FILL));
>> +	igt_require(igt_plane_has_prop(plane, IGT_PLANE_PIXEL_SOURCE));
>> +
>> +	rect.x1 = 0;
>> +	rect.x2 = mode->hdisplay;
>> +	rect.y1 = 0;
>> +	rect.y2 = mode->vdisplay;
>> +
>> +	width = rect_width(&rect);
>> +	height = rect_height(&rect);
>> +
>> +	igt_plane_set_position(plane, rect.x1, rect.y1);
>> +	pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
>> +			    IGT_PIPE_CRC_SOURCE_AUTO);
>> +
>> +
>> +	for (i = 0; i < ARRAY_SIZE(colors); i++) {
>> +		uint32_t r = colors[i].r;
>> +		uint32_t g = colors[i].g;
>> +		uint32_t b = colors[i].b;
>> +
>> +		struct drm_mode_create_blob c;
>> +		struct drm_mode_destroy_blob d;
>> +		struct drm_mode_solid_fill blob_data = {
>> +			.r = r,
>> +			.g = g,
>> +			.b = b,
>> +			.pad = 0x0,
>> +		};
>> +
>> +		c.data = (uintptr_t) &blob_data;
>> +		c.length = sizeof(blob_data);
>> +
>> +		igt_debug("Testing color r: 0x%x (%f), g: 0x%x (%f), b: 0x%x (%f)\n",
>> +				r, unorm_to_float(r),
>> +				g, unorm_to_float(g),
>> +				b, unorm_to_float(b));
>> +
>> +		/* get reference CRC */
>> +		igt_create_color_fb(data->drm_fd, width, height,
>> +				    DRM_FORMAT_ABGR8888, DRM_FORMAT_MOD_LINEAR,
>> +				    unorm_to_float(r),
>> +				    unorm_to_float(g),
>> +				    unorm_to_float(b),
> 
> I'm slightly wary of trusting a third party library (Cairo? Pixman?)
> with the float->u8 conversion. If both Cairo and the kernel do the same
> mistake in conversion, the test would pass. Therefore, this is actually
> testing that the kernel follows Cairo's conversion, and not the UAPI
> definition, should the unthinkable happen that they disagreed.
> 
> Would it at least be possible to verify the FB value against what we
> expect here?

Hi Pekka,

Unfortunately, I don't think there's a way that will let us to do that 
without writeback.

> 
>> +				    &ref_fb);
>> +		igt_plane_set_fb(plane, &ref_fb);
>> +		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_commit_atomic(&data->display,
>> +					  DRM_MODE_ATOMIC_ALLOW_MODESET,
>> +					  NULL);
>> +
>> +		igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &new_crc);
>> +
>> +		igt_assert_crc_equal(&ref_crc, &new_crc);
> 
> Looking good.
> 
>> +
>> +		/* 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);
> 
> Just wondering, why set this at the end and not when you are actually
> setting up the FB? Doesn't IGT sanitize KMS state before tests that
> don't explicitly test boot-up state?

Mostly for efficiency -- I can move the set(PIXEL_SOURCE, "FB") to when 
the reference FB is created, but PIXEL_SOURCE still needs to be set back 
to "FB" before the rmfb is called at the very end.

I can keep the set(PIXEL_SOURCE, "FB") here and move the set(SOLID_FILL, 
0) and commit to outside the loop before the remove_fb(). That way, we 
don't have unnecessary commits between test cases.

Thanks,

Jessica Zhang

> 
> 
> Thanks,
> pq
> 
>> +
>> +		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 +1763,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);
>>
>