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

[Jessica Zhang]

On 2/5/2024 2:35 AM, Pekka Paalanen wrote:
> On Fri, 2 Feb 2024 10:14:18 -0800
> Jessica Zhang <quic_jesszhan at quicinc.com> wrote:
> 
>> On 1/24/2024 1:13 AM, Pekka Paalanen wrote:
>>> On Tue, 23 Jan 2024 15:28:58 -0800
>>> Jessica Zhang <quic_jesszhan at 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 at quicinc.com>
>>>> ---
>>>>    tests/kms_atomic.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++------
>>>>    1 file changed, 161 insertions(+), 18 deletions(-)
> 
> ...
> 
>>>> +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.
> 
> I would be surprised if IGT used anything other than Cairo's image
> backend. Can't you simply read bytes back from where Cairo wrote them?
> Checking just one pixel would suffice.

Ack. I do see that we can use cairo_image_surface_get_data(), though it 
returns an ARGB8888 (specifically CAIRO_FORMAT_ARGB32) color.

I can add a to_argb() helper (like what we did for 
_dpu_plane_get_abgr_fill_color() in the MSM driver) to convert the 
RGB323232 color to ARGB8888 (and setting alpha to 0xFF) then comparing 
the resulting value to what we get from *_get_data().

What do you think?

Thanks,

Jessica Zhang

> 
> 
> Thanks,
> pq