Re: [igt-dev] [PATCH i-g-t v3 21/21] chamelium: Add a display test for randomized planes

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

Hi,

On Tue, 2019-01-15 at 17:56 -0500, Lyude Paul wrote:
> On Fri, 2019-01-11 at 10:05 +0100, Paul Kocialkowski wrote:
> > This introduces a new test for the Chamelium, that sets up planes
> > with randomized properties such as the format, dimensions, position,
> > in-framebuffer offsets and stride. The Chamelium capture is checked
> > against the reference generated by cairo with either a CRC or the
> > checkerboard-specific comparison method.
> > 
> > This test also includes testing of the VC4-specific T-tiled and
> > SAND-tiled modes, in all formats supported by the hardware.
> > 
> > Since this test does not share much with previous Chamelium display
> > tests (especially regarding KMS configuration), most of the code is
> > not shared with other tests.
> > 
> > This test can be derived with reproducible properties for regression
> > testing in the future. For now, it serves as a fuzzing test
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  tests/kms_chamelium.c | 446 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 446 insertions(+)
> > 
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index 2779d74f55b6..5963964878ba 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -26,6 +26,7 @@
> >  
> >  #include "config.h"
> >  #include "igt.h"
> > +#include "igt_vc4.h"
> >  
> >  #include <fcntl.h>
> >  #include <string.h>
> > @@ -702,6 +703,443 @@ test_display_frame_dump(data_t *data, struct
> > chamelium_port *port)
> >  	drmModeFreeConnector(connector);
> >  }
> >  
> > +static void randomize_plane_format_stride(igt_plane_t *plane, uint32_t
> > width,
> > +					  uint32_t *format, size_t *stride,
> > +					  bool *tiled, bool allow_yuv)
> > +{
> > +	size_t stride_min;
> > +	uint32_t *formats_array;
> > +	unsigned int formats_count;
> > +	unsigned int count = 0;
> > +	unsigned int i;
> > +	int index;
> > +
> > +	igt_format_array_fill(&formats_array, &formats_count, allow_yuv);
> > +
> > +	/* First pass to count the supported formats. */
> > +	for (i = 0; i < formats_count; i++)
> > +		if (igt_plane_has_format_mod(plane, formats_array[i],
> > +					     DRM_FORMAT_MOD_LINEAR))
> > +			count++;
> > +
> > +	igt_assert(count > 0);
> > +
> > +	index = rand() % count;
> > +
> > +	/* Second pass to get the index-th supported format. */
> > +	for (i = 0; i < formats_count; i++) {
> > +		if (!igt_plane_has_format_mod(plane, formats_array[i],
> > +					      DRM_FORMAT_MOD_LINEAR))
> > +			continue;
> > +
> > +		if (!index--) {
> > +			*format = formats_array[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	free(formats_array);
> > +
> > +	igt_assert(index < 0);
> > +
> > +	stride_min = width * igt_format_plane_bpp(*format, 0) / 8;
> > +
> > +	/* Randomize the stride with at most twice the minimum. */
> > +	*stride = (rand() % stride_min) + stride_min;
> Maybe need to change the comment or fix this line? Given
> 
> rand() = 9999999
> stride_min = 10
> 
> This would come out to 19, e.g. less then twice the maximum. Maybe you meant
> 
> *stride = (rand() % (stride_min + 1)) + stride_min;

Good catch, thanks! I think I'll just update the comments to be
consistent though, having the limit included or not does not cause
adverse effects in practice (in these cases only, I think I had already
taken care of the ones where it's significant) and I find it slightly
more readable with the limit excluded.

[...]

> > +static void prepare_tiled_plane(data_t *data, igt_plane_t *plane,
> > +				unsigned int index, uint32_t format,
> > +				struct igt_fb *overlay_fb)
> > +{
> > +	struct igt_fb tiled_fb;
> > +	unsigned int fb_id;
> > +
> > +	if (is_vc4_device(data->drm_fd) &&
> > +	    igt_vc4_plane_supports_t_tiling(plane, format)) {
> > +		igt_debug("Plane %d: using VC4 T-tiling\n", index);
> > +
> > +		fb_id = igt_vc4_fb_t_tiled_convert(&tiled_fb, overlay_fb);
> > +	} else if (is_vc4_device(data->drm_fd) &&
> > +		   igt_vc4_plane_supports_sand_tiling(plane, format, 256)) {
> > +		/* Randomize the column height with at most twice the height.
> > */
> > +		size_t column_height = (rand() % overlay_fb->height) +
> > +				       overlay_fb->height;
> > +
> > +		igt_debug("Plane %d: using VC4 SAND256 tiling with column
> > height %ld\n",
> > +			  index, column_height);
> > +
> > +		fb_id = vc4_fb_sand_tiled_convert(&tiled_fb, overlay_fb, 256,
> > +						  column_height);
> > +	} else {
> > +		return;
> > +	}
> 
> Bikeshed: Why not remove the two is_vc4_device(data->drm_fd) checks and just
> put this at the top of prepare_tiled_plane():
> 
> if (!is_vc4_device(data->drm_fd))
> 	return;
> 
> Would make these conditionals a bit easier to read

I have dropped that check altogether. After all, there is no need to
double-check for VC4 if the modifiers were reported by the driver.

> > +
> > +	igt_assert(fb_id > 0);
> > +
> > +	/* Make the tiled framebuffer the overlay. */
> > +	igt_remove_fb(data->drm_fd, overlay_fb);
> > +	*overlay_fb = tiled_fb;
> > +}
> > +
> > +static void prepare_randomized_plane(data_t *data,
> > +				     drmModeModeInfo *mode,
> > +				     igt_plane_t *plane,
> > +				     struct igt_fb *overlay_fb,
> > +				     unsigned int index,
> > +				     cairo_surface_t *result_surface,
> > +				     bool allow_scaling, bool allow_yuv)
> > +{
> > +	struct igt_fb pattern_fb;
> > +	uint32_t overlay_fb_w, overlay_fb_h;
> > +	uint32_t overlay_src_w, overlay_src_h;
> > +	uint32_t overlay_src_x, overlay_src_y;
> > +	int32_t overlay_crtc_x, overlay_crtc_y;
> > +	uint32_t overlay_crtc_w, overlay_crtc_h;
> > +	uint32_t format;
> > +	size_t stride;
> > +	bool tiled;
> > +	int fb_id;
> > +
> > +	randomize_plane_dimensions(mode, &overlay_fb_w, &overlay_fb_h,
> > +				   &overlay_src_w, &overlay_src_h,
> > +				   &overlay_src_x, &overlay_src_y,
> > +				   &overlay_crtc_w, &overlay_crtc_h,
> > +				   &overlay_crtc_x, &overlay_crtc_y,
> > +				   allow_scaling);
> > +
> > +	igt_debug("Plane %d: framebuffer size %dx%d\n", index,
> > +		  overlay_fb_w, overlay_fb_h);
> > +	igt_debug("Plane %d: on-crtc size %dx%d\n", index,
> > +		  overlay_crtc_w, overlay_crtc_h);
> > +	igt_debug("Plane %d: on-crtc position %dx%d\n", index,
> > +		  overlay_crtc_x, overlay_crtc_y);
> > +	igt_debug("Plane %d: in-framebuffer size %dx%d\n", index,
> > +		  overlay_src_w, overlay_src_h);
> > +	igt_debug("Plane %d: in-framebuffer position %dx%d\n", index,
> > +		  overlay_src_x, overlay_src_y);
> > +
> > +	/* Get a pattern framebuffer for the overlay plane. */
> > +	fb_id = chamelium_get_pattern_fb(data, overlay_fb_w, overlay_fb_h,
> > +					 DRM_FORMAT_XRGB8888, 32,
> > &pattern_fb);
> > +	igt_assert(fb_id > 0);
> > +
> > +	randomize_plane_format_stride(plane, overlay_fb_w, &format, &stride,
> > +				      &tiled, allow_yuv);
> > +
> > +	igt_debug("Plane %d: %s format with stride %ld\n", index,
> > +		  igt_format_str(format), stride);
> > +
> > +	fb_id = igt_fb_convert_with_stride(overlay_fb, &pattern_fb, format,
> > +					   stride);
> > +	igt_assert(fb_id > 0);
> > +
> > +	if (tiled)
> > +		prepare_tiled_plane(data, plane, index, format, overlay_fb);
> > +
> > +	blit_plane_cairo(data, result_surface, overlay_src_w, overlay_src_h,
> > +			 overlay_src_x, overlay_src_y,
> > +			 overlay_crtc_w, overlay_crtc_h,
> > +			 overlay_crtc_x, overlay_crtc_y, &pattern_fb);
> > +
> > +	configure_plane(plane, overlay_src_w, overlay_src_h,
> > +			overlay_src_x, overlay_src_y,
> > +			overlay_crtc_w, overlay_crtc_h,
> > +			overlay_crtc_x, overlay_crtc_y, overlay_fb);
> > +
> > +	/* Remove the original pattern framebuffer. */
> > +	igt_remove_fb(data->drm_fd, &pattern_fb);
> > +}
> > +
> > +static void test_display_planes_random(data_t *data,
> > +				       struct chamelium_port *port,
> > +				       enum chamelium_check check)
> > +{
> > +	igt_output_t *output;
> > +	drmModeModeInfo *mode;
> > +	igt_plane_t *primary_plane;
> > +	struct igt_fb primary_fb;
> > +	struct igt_fb result_fb;
> > +	struct igt_fb *overlay_fbs;
> > +	igt_crc_t *crc;
> > +	igt_crc_t *expected_crc;
> > +	struct chamelium_fb_crc_async_data *fb_crc;
> > +	unsigned int overlay_planes_max = 0;
> > +	unsigned int overlay_planes_count;
> > +	cairo_surface_t *result_surface;
> > +	int captured_frame_count;
> > +	bool allow_scaling;
> > +	bool allow_yuv;
> > +	unsigned int i;
> > +	unsigned int fb_id;
> > +
> > +	switch (check) {
> > +	case CHAMELIUM_CHECK_CRC:
> > +		allow_scaling = false;
> > +		allow_yuv = false;
> > +		break;
> > +	case CHAMELIUM_CHECK_CHECKERBOARD:
> > +		allow_scaling = true;
> > +		allow_yuv = true;
> > +		break;
> > +	default:
> > +		igt_assert(false);
> > +	}
> > +
> > +	srand(time(NULL));
> > +
> > +	reset_state(data, port);
> > +
> > +	/* Find the connector and pipe. */
> > +	output = prepare_output(data, port);
> > +
> > +	mode = igt_output_get_mode(output);
> > +
> > +	/* Get a framebuffer for the primary plane. */
> > +	primary_plane = igt_output_get_plane_type(output,
> > DRM_PLANE_TYPE_PRIMARY);
> > +	igt_assert(primary_plane);
> > +
> > +	fb_id = chamelium_get_pattern_fb(data, mode->hdisplay, mode->vdisplay,
> > +					 DRM_FORMAT_XRGB8888, 64,
> > &primary_fb);
> > +	igt_assert(fb_id > 0);
> > +
> > +	/* Get a framebuffer for the cairo composition result. */
> > +	fb_id = igt_create_fb(data->drm_fd, mode->hdisplay,
> > +			      mode->vdisplay, DRM_FORMAT_XRGB8888,
> > +			      LOCAL_DRM_FORMAT_MOD_NONE, &result_fb);
> > +	igt_assert(fb_id > 0);
> > +
> > +	result_surface = igt_get_cairo_surface(data->drm_fd, &result_fb);
> > +
> > +	/* Paint the primary framebuffer on the result surface. */
> > +	blit_plane_cairo(data, result_surface, 0, 0, 0, 0, 0, 0, 0, 0,
> > +			 &primary_fb);
> > +
> > +	/* Configure the primary plane. */
> > +	igt_plane_set_fb(primary_plane, &primary_fb);
> > +
> > +	overlay_planes_max =
> > +		igt_output_count_plane_type(output, DRM_PLANE_TYPE_OVERLAY);
> > +
> > +	/* Limit the number of planes to a reasonable scene. */
> > +	if (overlay_planes_max > 4)
> > +		overlay_planes_max = 4;
> 
> Why not just use max() here?

Sure, I'll do that!

> > +
> > +	overlay_planes_count = (rand() % overlay_planes_max) + 1;
> > +	igt_debug("Using %d overlay planes\n", overlay_planes_count);
> > +
> > +	overlay_fbs = calloc(sizeof(struct igt_fb), overlay_planes_count);
> igt_assert(overlay_fbs);
> 
> Other then that, looks really good!

Great, thanks a lot for the review!

Cheers,

Paul

> > +
> > +	for (i = 0; i < overlay_planes_count; i++) {
> > +		struct igt_fb *overlay_fb = &overlay_fbs[i];
> > +		igt_plane_t *plane =
> > +			igt_output_get_plane_type_index(output,
> > +							DRM_PLANE_TYPE_OVERLAY
> > ,
> > +							i);
> > +		igt_assert(plane);
> > +
> > +		prepare_randomized_plane(data, mode, plane, overlay_fb, i,
> > +					 result_surface, allow_scaling,
> > +					 allow_yuv);
> > +	}
> > +
> > +	cairo_surface_destroy(result_surface);
> > +
> > +	if (check == CHAMELIUM_CHECK_CRC)
> > +		fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd,
> > +								&result_fb);
> > +
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > +
> > +	if (check == CHAMELIUM_CHECK_CRC) {
> > +		chamelium_capture(data->chamelium, port, 0, 0, 0, 0, 1);
> > +		crc = chamelium_read_captured_crcs(data->chamelium,
> > +						   &captured_frame_count);
> > +
> > +		igt_assert(captured_frame_count == 1);
> > +
> > +		expected_crc =
> > chamelium_calculate_fb_crc_async_finish(fb_crc);
> > +
> > +		chamelium_assert_crc_eq_or_dump(data->chamelium,
> > +						expected_crc, crc,
> > +						&result_fb, 0);
> > +
> > +		free(expected_crc);
> > +		free(crc);
> > +	} else if (check == CHAMELIUM_CHECK_CHECKERBOARD) {
> > +		struct chamelium_frame_dump *dump;
> > +
> > +		dump = chamelium_port_dump_pixels(data->chamelium, port, 0, 0,
> > +						  0, 0);
> > +		chamelium_assert_frame_match_or_dump(data->chamelium, port,
> > +						     dump, &result_fb, check);
> > +		chamelium_destroy_frame_dump(dump);
> > +	}
> > +
> > +	for (i = 0; i < overlay_planes_count; i++) {
> > +		struct igt_fb *overlay_fb = &overlay_fbs[i];
> > +		igt_plane_t *plane;
> > +
> > +		plane = igt_output_get_plane_type_index(output,
> > +							DRM_PLANE_TYPE_OVERLAY
> > ,
> > +							i);
> > +		igt_assert(plane);
> > +
> > +		igt_remove_fb(data->drm_fd, overlay_fb);
> > +	}
> > +
> > +	free(overlay_fbs);
> > +
> > +	igt_remove_fb(data->drm_fd, &primary_fb);
> > +	igt_remove_fb(data->drm_fd, &result_fb);
> > +}
> > +
> >  static void
> >  test_hpd_without_ddc(data_t *data, struct chamelium_port *port)
> >  {
> > @@ -976,6 +1414,10 @@ igt_main
> >  			test_display_one_mode(&data, port,
> > DRM_FORMAT_XRGB1555,
> >  					      CHAMELIUM_CHECK_CRC, 1);
> >  
> > +		connector_subtest("hdmi-crc-planes-random", HDMIA)
> > +			test_display_planes_random(&data, port,
> > +						   CHAMELIUM_CHECK_CRC);
> > +
> >  		connector_subtest("hdmi-cmp-nv12", HDMIA)
> >  			test_display_one_mode(&data, port, DRM_FORMAT_NV12,
> >  					      CHAMELIUM_CHECK_CHECKERBOARD,
> > 1);
> > @@ -1008,6 +1450,10 @@ igt_main
> >  			test_display_one_mode(&data, port, DRM_FORMAT_YVU422,
> >  					      CHAMELIUM_CHECK_CHECKERBOARD,
> > 1);
> >  
> > +		connector_subtest("hdmi-cmp-planes-random", HDMIA)
> > +			test_display_planes_random(&data, port,
> > +						   CHAMELIUM_CHECK_CHECKERBOAR
> > D);
> > +
> >  		connector_subtest("hdmi-frame-dump", HDMIA)
> >  			test_display_frame_dump(&data, port);
> >  	}
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev