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

[Pekka Paalanen <pekka.paalanen@collabora.com>]

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

On Fri, 2 Feb 2024 10:14:18 -0800
Jessica Zhang <quic_jesszhan@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@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(-)

...

> >> +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.


Thanks,
pq

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