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: 4339 bytes --]

On Wed, 28 Feb 2024 10:28:22 -0800
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:

> On 2/5/2024 2:35 AM, Pekka Paalanen wrote:
> > 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.  
> 
> 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?

Sure, why not.

You should be very aware of the format Cairo is using on the FB anyway,
because that's what the kernel gets.


Thanks,
pq

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