[i-g-t,9/9] chamelium: Add a CRC-based display test for randomized planes

Submitted by Paul Kocialkowski on Dec. 6, 2018, 2:11 p.m.

Details

Message ID 20181206141132.23349-10-paul.kocialkowski@bootlin.com
State New
Series "Chamelium VC4 plane testing, with T-tiled mode"
Headers show

Commit Message

Paul Kocialkowski Dec. 6, 2018, 2:11 p.m.
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 a CRC.

This test also includes testing for some VC4-specific features, such as
T-tiled mode (in XR24 format), bandwidth limitation and underrun
(that require kernel-side patches that are currently under review).

Since this test does not share much with previous CRC-based 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 kind of fuzzing test.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 tests/kms_chamelium.c | 331 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 331 insertions(+)

Patch hide | download patch | download mbox

diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index 7d95a8bc..a4977309 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -26,6 +26,8 @@ 
 
 #include "config.h"
 #include "igt.h"
+#include "igt_sysfs.h"
+#include "igt_vc4.h"
 
 #include <fcntl.h>
 #include <string.h>
@@ -703,6 +705,330 @@  test_display_frame_dump(data_t *data, struct chamelium_port *port)
 	drmModeFreeConnector(connector);
 }
 
+static uint32_t random_plane_formats[] = {
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_ARGB1555,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_BGR565,
+	DRM_FORMAT_RGB888,
+	DRM_FORMAT_BGR888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_XRGB8888,
+};
+
+static void test_display_planes_random(data_t *data,
+				       struct chamelium_port *port,
+				       enum chamelium_check check)
+{
+	igt_output_t *output;
+	igt_pipe_t *pipe;
+	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;
+	unsigned int overlay_planes_index;
+	bool bandwidth_exceeded = false;
+	bool underrun_detected = false;
+	cairo_surface_t *result_surface;
+	cairo_surface_t *primary_surface;
+	cairo_surface_t *overlay_surface;
+	cairo_t *cr;
+	int captured_frame_count;
+	unsigned int i;
+	char *underrun;
+	int fb_id;
+	int debugfs;
+	int ret;
+
+	igt_assert(check == CHAMELIUM_CHECK_CRC);
+
+	srand(time(NULL));
+
+	reset_state(data, port);
+
+	/* Find the connector and pipe. */
+	output = prepare_output(data, port);
+	igt_assert(output);
+
+	if (is_vc4_device(data->drm_fd)) {
+		debugfs = igt_debugfs_dir(data->drm_fd);
+		igt_assert(debugfs >= 0);
+	}
+
+	pipe = &data->display.pipes[output->pending_pipe];
+
+	mode = igt_output_get_mode(output);
+	igt_assert(mode);
+
+	/* 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);
+
+	igt_plane_set_fb(primary_plane, &primary_fb);
+
+	primary_surface = igt_get_cairo_surface(data->drm_fd, &primary_fb);
+
+	/* 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);
+
+	/* Initialize cairo with the result surface as target. */
+	result_surface = igt_get_cairo_surface(data->drm_fd, &result_fb);
+
+	cr = cairo_create(result_surface);
+
+	/* Blend the primary surface. */
+	cairo_set_source_surface(cr, primary_surface, 0, 0);
+	cairo_surface_destroy(primary_surface);
+	cairo_paint(cr);
+
+	cairo_destroy(cr);
+
+	/* Count available overlay planes. */
+	for (i = 0; i < pipe->n_planes; i++) {
+		igt_plane_t *plane = &pipe->planes[i];
+
+		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
+			overlay_planes_max++;
+	}
+
+	/* Limit the number of planes to a reasonable scene. */
+	if (overlay_planes_max > 4)
+		overlay_planes_max = 4;
+
+	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);
+
+	for (i = 0, overlay_planes_index = 0;
+	     i < pipe->n_planes && overlay_planes_index < overlay_planes_count;
+	     i++) {
+		struct igt_fb *overlay_fb = &overlay_fbs[overlay_planes_index];
+		igt_plane_t *plane = &pipe->planes[i];
+		struct igt_fb pattern_fb;
+		uint32_t overlay_fb_w, overlay_fb_h;
+		int32_t overlay_crtc_x, overlay_crtc_y;
+		uint32_t overlay_crtc_w, overlay_crtc_h;
+		uint32_t overlay_src_x, overlay_src_y;
+		uint32_t overlay_src_w, overlay_src_h;
+		int overlay_surface_x, overlay_surface_y;
+		unsigned int index;
+		unsigned int stride_min;
+		unsigned int stride;
+		bool vc4_t_tiled;
+		uint32_t format;
+
+		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
+			continue;
+
+		/* Randomize width and height in the mode dimensions range. */
+		overlay_fb_w = (rand() % mode->hdisplay) + 1;
+		overlay_fb_h = (rand() % mode->vdisplay) + 1;
+
+		/*
+		 * Randomize source offset, but keep at least half of the
+		 * original size.
+		 */
+		overlay_src_x = rand() % (overlay_fb_w / 2);
+		overlay_src_y = rand() % (overlay_fb_h / 2);
+
+		/*
+		 * The on-crtc size does not include the source offset, so it
+		 * needs to be subtracted to avoid scaling.
+		 */
+		overlay_crtc_w = overlay_fb_w - overlay_src_x;
+		overlay_crtc_h = overlay_fb_h - overlay_src_y;
+
+		/* Same goes for the framebuffer source size. */
+		overlay_src_w = overlay_crtc_w;
+		overlay_src_h = overlay_crtc_h;
+
+		/*
+		 * Randomize the on-crtc position and allow the plane to go
+		 * off-display by up to half of its width and height.
+		 */
+		overlay_crtc_x = (rand() % mode->hdisplay) - overlay_crtc_w / 2;
+		overlay_crtc_y = (rand() % mode->vdisplay) - overlay_crtc_h / 2;
+
+		igt_debug("Plane %d: on-crtc size %dx%d\n",
+			  overlay_planes_index, overlay_crtc_w, overlay_crtc_h);
+		igt_debug("Plane %d: on-crtc position %dx%d\n",
+			  overlay_planes_index, overlay_crtc_x, overlay_crtc_y);
+		igt_debug("Plane %d: in-framebuffer position %dx%d\n",
+			  overlay_planes_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 the use of tiled mode with a 1/4 probability. */
+		index = rand() % 4;
+
+		if (is_vc4_device(data->drm_fd) && index == 0) {
+			format = DRM_FORMAT_XRGB8888;
+			vc4_t_tiled = true;
+
+			igt_debug("Plane %d: VC4 T-tiled %s format\n",
+				  overlay_planes_index, igt_format_str(format));
+		} else {
+			/* Randomize the format to test. */
+			index = rand() % ARRAY_SIZE(random_plane_formats);
+			format = random_plane_formats[index];
+			vc4_t_tiled = false;
+
+			igt_debug("Plane %d: %s format\n", overlay_planes_index,
+				  igt_format_str(format));
+		}
+
+		/* Convert the pattern to the test format if needed. */
+		if (vc4_t_tiled) {
+			fb_id = igt_vc4_fb_t_tiled_convert(overlay_fb,
+							   &pattern_fb);
+			igt_assert(fb_id > 0);
+		} else {
+			stride_min = overlay_fb_w *
+				     igt_format_plane_bpp(format, 0) / 8;
+
+			/* Randomize the stride with at most twice the minimum. */
+			stride = (rand() % stride_min) + stride_min;
+
+			/* Pixman requires the stride to be aligned to 32-byte words. */
+			stride = ALIGN(stride, sizeof(uint32_t));
+
+			igt_debug("Plane %d: using stride %d\n", overlay_planes_index, stride);
+
+			fb_id = igt_fb_convert_with_stride(overlay_fb,
+							   &pattern_fb,
+							   format, stride);
+			igt_assert(fb_id);
+		}
+
+		igt_plane_set_fb(plane, overlay_fb);
+
+		overlay_surface = igt_get_cairo_surface(data->drm_fd,
+							&pattern_fb);
+
+		/* Blend the overlay surface. */
+		overlay_surface_x = overlay_crtc_x - (int) overlay_src_x;
+		overlay_surface_y = overlay_crtc_y - (int) overlay_src_y;
+
+		cr = cairo_create(result_surface);
+
+		cairo_set_source_surface(cr, overlay_surface, overlay_surface_x,
+					 overlay_surface_y);
+		cairo_surface_destroy(overlay_surface);
+
+		/* Clip the surface to a rectangle. */
+		cairo_rectangle(cr, overlay_crtc_x, overlay_crtc_y,
+				overlay_crtc_w, overlay_crtc_h);
+		cairo_clip(cr);
+
+		cairo_paint(cr);
+
+		cairo_destroy(cr);
+
+		/* Configure the plane with framebuffer source coordinates. */
+		igt_plane_set_position(plane, overlay_crtc_x, overlay_crtc_y);
+		igt_plane_set_size(plane, overlay_crtc_w, overlay_crtc_h);
+
+		igt_fb_set_position(overlay_fb, plane, overlay_src_x,
+				    overlay_src_y);
+		igt_fb_set_size(overlay_fb, plane, overlay_src_w,
+				overlay_src_h);
+
+		igt_remove_fb(data->drm_fd, &pattern_fb);
+
+		overlay_planes_index++;
+	}
+
+	cairo_surface_destroy(result_surface);
+
+	fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd,
+							&result_fb);
+
+	if (is_vc4_device(data->drm_fd)) {
+		ret = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
+		bandwidth_exceeded = (ret < 0 && errno == ENOSPC);
+
+		igt_debug("Bandwidth limitation exeeded: %s\n",
+			  bandwidth_exceeded ? "Yes" : "No");
+
+		igt_assert(!bandwidth_exceeded);
+	}
+
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+
+	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);
+
+	if (is_vc4_device(data->drm_fd)) {
+		underrun = igt_sysfs_get(debugfs, "underrun");
+		igt_assert(underrun);
+
+		underrun_detected = (underrun[0] == 'Y');
+		free(underrun);
+
+		igt_debug("Underrun detected: %s\n",
+			  underrun_detected ? "Yes" : "No");
+
+		igt_assert(!underrun_detected);
+
+		close(debugfs);
+	}
+
+	expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc);
+
+	chamelium_assert_crc_eq_or_dump(data->chamelium,
+					expected_crc, crc,
+					&result_fb, i);
+
+	igt_debug("reference CRC: %s\n", igt_crc_to_string(expected_crc));
+	igt_debug("calculated CRC: %s\n", igt_crc_to_string(crc));
+
+	free(expected_crc);
+	free(crc);
+
+	for (i = 0, overlay_planes_index = 0;
+	     i < pipe->n_planes && overlay_planes_index < overlay_planes_count;
+	     i++) {
+		igt_plane_t *plane = &pipe->planes[i];
+		struct igt_fb *overlay_fb = &overlay_fbs[overlay_planes_index];
+
+		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
+			continue;
+
+		igt_remove_fb(data->drm_fd, overlay_fb);
+
+		overlay_planes_index++;
+	}
+
+	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)
 {
@@ -981,6 +1307,11 @@  igt_main
 			test_display_one_mode(&data, port, DRM_FORMAT_NV12,
 					      CHAMELIUM_CHECK_ANALOG, 1);
 
+		connector_subtest("hdmi-crc-planes-random", HDMIA)
+			test_display_planes_random(&data, port,
+						   CHAMELIUM_CHECK_CRC);
+
+
 		connector_subtest("hdmi-frame-dump", HDMIA)
 			test_display_frame_dump(&data, port);
 	}

Comments

Maxime Ripard Dec. 6, 2018, 2:49 p.m.
Hi,

On Thu, Dec 06, 2018 at 03:11:32PM +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 a CRC.
> 
> This test also includes testing for some VC4-specific features, such as
> T-tiled mode (in XR24 format), bandwidth limitation and underrun
> (that require kernel-side patches that are currently under review).

it's not really clear to me why both are part of the same test?  You
don't really need to test the HVS bandwidth limitation with the
patterns, and you don't really need a pattern and a CRC to test the
HVS bandwith?

> Since this test does not share much with previous CRC-based 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 kind of fuzzing test.

That function is also pretty long. While sticking to function that are
under 50-80 lines is not always easy, this one is over 300 lines long
and would definitely benefit being split into many, smaller,
functions.

Maxime
Lyude Paul Dec. 6, 2018, 10:52 p.m.
I agree with Maxime, this definitely should be split up into some more
functions. Some other comments below below

On Thu, 2018-12-06 at 15:11 +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 a CRC.
>
> This test also includes testing for some VC4-specific features, such as
> T-tiled mode (in XR24 format), bandwidth limitation and underrun
> (that require kernel-side patches that are currently under review).
>
> Since this test does not share much with previous CRC-based 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 kind of fuzzing test.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  tests/kms_chamelium.c | 331 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 331 insertions(+)
>
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 7d95a8bc..a4977309 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -26,6 +26,8 @@
>
>  #include "config.h"
>  #include "igt.h"
> +#include "igt_sysfs.h"
> +#include "igt_vc4.h"
>
>  #include <fcntl.h>
>  #include <string.h>
> @@ -703,6 +705,330 @@ test_display_frame_dump(data_t *data, struct
> chamelium_port *port)
>  	drmModeFreeConnector(connector);
>  }
>
> +static uint32_t random_plane_formats[] = {
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_ARGB1555,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_BGR565,
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_BGR888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static void test_display_planes_random(data_t *data,
> +				       struct chamelium_port *port,
> +				       enum chamelium_check check)
> +{
> +	igt_output_t *output;
> +	igt_pipe_t *pipe;
> +	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;
> +	unsigned int overlay_planes_index;
> +	bool bandwidth_exceeded = false;
> +	bool underrun_detected = false;
> +	cairo_surface_t *result_surface;
> +	cairo_surface_t *primary_surface;
> +	cairo_surface_t *overlay_surface;
> +	cairo_t *cr;
> +	int captured_frame_count;
> +	unsigned int i;
> +	char *underrun;
> +	int fb_id;
> +	int debugfs;
> +	int ret;
> +
> +	igt_assert(check == CHAMELIUM_CHECK_CRC);
> +
> +	srand(time(NULL));
> +
> +	reset_state(data, port);
> +
> +	/* Find the connector and pipe. */
> +	output = prepare_output(data, port);
> +	igt_assert(output);
Do we actually need this? We're not running igt_assert on the prepare_output()
function for any of the other tests.
> +
> +	if (is_vc4_device(data->drm_fd)) {
> +		debugfs = igt_debugfs_dir(data->drm_fd);
> +		igt_assert(debugfs >= 0);
> +	}
> +
> +	pipe = &data->display.pipes[output->pending_pipe];
> +
> +	mode = igt_output_get_mode(output);
> +	igt_assert(mode);
This assert doesn't look like it's needed either

> +
> +	/* 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);
> +
> +	igt_plane_set_fb(primary_plane, &primary_fb);
> +
> +	primary_surface = igt_get_cairo_surface(data->drm_fd, &primary_fb);
> +
> +	/* 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);
> +
> +	/* Initialize cairo with the result surface as target. */
> +	result_surface = igt_get_cairo_surface(data->drm_fd, &result_fb);
> +
> +	cr = cairo_create(result_surface);
> +
> +	/* Blend the primary surface. */
> +	cairo_set_source_surface(cr, primary_surface, 0, 0);
> +	cairo_surface_destroy(primary_surface);
> +	cairo_paint(cr);
> +
> +	cairo_destroy(cr);
> +
> +	/* Count available overlay planes. */
> +	for (i = 0; i < pipe->n_planes; i++) {
> +		igt_plane_t *plane = &pipe->planes[i];
> +
> +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> +			overlay_planes_max++;
> +	}
Why not add a helper for this in a seperate patch? This seems quite
useful

> +
> +	/* Limit the number of planes to a reasonable scene. */
> +	if (overlay_planes_max > 4)
> +		overlay_planes_max = 4;
> +
> +	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)
> +
> +	for (i = 0, overlay_planes_index = 0;
> +	     i < pipe->n_planes && overlay_planes_index <
> overlay_planes_count;
> +	     i++) {
> +		struct igt_fb *overlay_fb =
> &overlay_fbs[overlay_planes_index];
> +		igt_plane_t *plane = &pipe->planes[i];
> +		struct igt_fb pattern_fb;
> +		uint32_t overlay_fb_w, overlay_fb_h;
> +		int32_t overlay_crtc_x, overlay_crtc_y;
> +		uint32_t overlay_crtc_w, overlay_crtc_h;
> +		uint32_t overlay_src_x, overlay_src_y;
> +		uint32_t overlay_src_w, overlay_src_h;
> +		int overlay_surface_x, overlay_surface_y;
> +		unsigned int index;
> +		unsigned int stride_min;
> +		unsigned int stride;
> +		bool vc4_t_tiled;
> +		uint32_t format;
> +
> +		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
> +			continue;
We could also add a helper for looping through planes of specific types,
since we seem to do that multiple times here and will probably do it in
future tests at some poit

> +
> +		/* Randomize width and height in the mode dimensions range. */
> +		overlay_fb_w = (rand() % mode->hdisplay) + 1;
> +		overlay_fb_h = (rand() % mode->vdisplay) + 1;
> +
> +		/*
> +		 * Randomize source offset, but keep at least half of the
> +		 * original size.
> +		 */
> +		overlay_src_x = rand() % (overlay_fb_w / 2);
> +		overlay_src_y = rand() % (overlay_fb_h / 2);
> +
> +		/*
> +		 * The on-crtc size does not include the source offset, so it
> +		 * needs to be subtracted to avoid scaling.
> +		 */
> +		overlay_crtc_w = overlay_fb_w - overlay_src_x;
> +		overlay_crtc_h = overlay_fb_h - overlay_src_y;
> +
> +		/* Same goes for the framebuffer source size. */
> +		overlay_src_w = overlay_crtc_w;
> +		overlay_src_h = overlay_crtc_h;
> +
> +		/*
> +		 * Randomize the on-crtc position and allow the plane to go
> +		 * off-display by up to half of its width and height.
> +		 */
> +		overlay_crtc_x = (rand() % mode->hdisplay) - overlay_crtc_w /
> 2;
> +		overlay_crtc_y = (rand() % mode->vdisplay) - overlay_crtc_h /
> 2;
> +
> +		igt_debug("Plane %d: on-crtc size %dx%d\n",
> +			  overlay_planes_index, overlay_crtc_w,
> overlay_crtc_h);
> +		igt_debug("Plane %d: on-crtc position %dx%d\n",
> +			  overlay_planes_index, overlay_crtc_x,
> overlay_crtc_y);
> +		igt_debug("Plane %d: in-framebuffer position %dx%d\n",
> +			  overlay_planes_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 the use of tiled mode with a 1/4 probability. */
> +		index = rand() % 4;
> +
> +		if (is_vc4_device(data->drm_fd) && index == 0) {
> +			format = DRM_FORMAT_XRGB8888;
> +			vc4_t_tiled = true;
> +
> +			igt_debug("Plane %d: VC4 T-tiled %s format\n",
> +				  overlay_planes_index,
> igt_format_str(format));
> +		} else {
> +			/* Randomize the format to test. */
> +			index = rand() % ARRAY_SIZE(random_plane_formats);
> +			format = random_plane_formats[index];
> +			vc4_t_tiled = false;
> +
> +			igt_debug("Plane %d: %s format\n",
> overlay_planes_index,
> +				  igt_format_str(format));
> +		}
> +
> +		/* Convert the pattern to the test format if needed. */
> +		if (vc4_t_tiled) {
> +			fb_id = igt_vc4_fb_t_tiled_convert(overlay_fb,
> +							   &pattern_fb);
> +			igt_assert(fb_id > 0);
> +		} else {
> +			stride_min = overlay_fb_w *
> +				     igt_format_plane_bpp(format, 0) / 8;
> +
> +			/* Randomize the stride with at most twice the
> minimum. */
> +			stride = (rand() % stride_min) + stride_min;
> +
> +			/* Pixman requires the stride to be aligned to 32-byte
> words. */
> +			stride = ALIGN(stride, sizeof(uint32_t));
> +
> +			igt_debug("Plane %d: using stride %d\n",
> overlay_planes_index, stride);
> +
> +			fb_id = igt_fb_convert_with_stride(overlay_fb,
> +							   &pattern_fb,
> +							   format, stride);
> +			igt_assert(fb_id);
> +		}
> +
> +		igt_plane_set_fb(plane, overlay_fb);
> +
> +		overlay_surface = igt_get_cairo_surface(data->drm_fd,
> +							&pattern_fb);
> +
> +		/* Blend the overlay surface. */
> +		overlay_surface_x = overlay_crtc_x - (int) overlay_src_x;
> +		overlay_surface_y = overlay_crtc_y - (int) overlay_src_y;
> +
> +		cr = cairo_create(result_surface);
> +
> +		cairo_set_source_surface(cr, overlay_surface,
> overlay_surface_x,
> +					 overlay_surface_y);
> +		cairo_surface_destroy(overlay_surface);
> +
> +		/* Clip the surface to a rectangle. */
> +		cairo_rectangle(cr, overlay_crtc_x, overlay_crtc_y,
> +				overlay_crtc_w, overlay_crtc_h);
> +		cairo_clip(cr);
> +
> +		cairo_paint(cr);
> +
> +		cairo_destroy(cr);
> +
> +		/* Configure the plane with framebuffer source coordinates. */
> +		igt_plane_set_position(plane, overlay_crtc_x, overlay_crtc_y);
> +		igt_plane_set_size(plane, overlay_crtc_w, overlay_crtc_h);
> +
> +		igt_fb_set_position(overlay_fb, plane, overlay_src_x,
> +				    overlay_src_y);
> +		igt_fb_set_size(overlay_fb, plane, overlay_src_w,
> +				overlay_src_h);
> +
> +		igt_remove_fb(data->drm_fd, &pattern_fb);
> +
> +		overlay_planes_index++;
> +	}
> +
> +	cairo_surface_destroy(result_surface);
> +
> +	fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd,
> +							&result_fb);
> +
> +	if (is_vc4_device(data->drm_fd)) {
> +		ret = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
> +		bandwidth_exceeded = (ret < 0 && errno == ENOSPC);
> +
> +		igt_debug("Bandwidth limitation exeeded: %s\n",
> +			  bandwidth_exceeded ? "Yes" : "No");
> +
> +		igt_assert(!bandwidth_exceeded);
> +	}
Why does this need to be handled separately for VC4 devices? We already
print the return code from a failed atomic commit into the debugging
output, and I'd assume anyone looking at the output from the failure of
one of these tests probably knows what that means. Additionally, if we
wanted to print debugging messages for bandwidth limitations being
exceeded for atomic commits on vc4, seems like it would probably be
better to just add that code to igt_display_try_commit2()

> +
> +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +
> +	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);
> +
> +	if (is_vc4_device(data->drm_fd)) {
> +		underrun = igt_sysfs_get(debugfs, "underrun");
> +		igt_assert(underrun);
> +
> +		underrun_detected = (underrun[0] == 'Y');
> +		free(underrun);
> +
> +		igt_debug("Underrun detected: %s\n",
> +			  underrun_detected ? "Yes" : "No");
> +
> +		igt_assert(!underrun_detected);
> +
> +		close(debugfs);
> +	}
This will definitely be used again in the future, this should also go
into a helper for vc4.

> +
> +	expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc);
> +
> +	chamelium_assert_crc_eq_or_dump(data->chamelium,
> +					expected_crc, crc,
> +					&result_fb, i);
> +
> +	igt_debug("reference CRC: %s\n", igt_crc_to_string(expected_crc));
> +	igt_debug("calculated CRC: %s\n", igt_crc_to_string(crc));
Why not just add this debugging output to
chamelium_assert_crc_eq_or_dump()?

> +
> +	free(expected_crc);
> +	free(crc);
> +
> +	for (i = 0, overlay_planes_index = 0;
> +	     i < pipe->n_planes && overlay_planes_index <
> overlay_planes_count;
> +	     i++) {
> +		igt_plane_t *plane = &pipe->planes[i];
> +		struct igt_fb *overlay_fb =
> &overlay_fbs[overlay_planes_index];
> +
> +		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
> +			continue;
> +
> +		igt_remove_fb(data->drm_fd, overlay_fb);
> +
> +		overlay_planes_index++;
> +	}
> +
> +	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)
>  {
> @@ -981,6 +1307,11 @@ igt_main
>  			test_display_one_mode(&data, port, DRM_FORMAT_NV12,
>  					      CHAMELIUM_CHECK_ANALOG, 1);
>
> +		connector_subtest("hdmi-crc-planes-random", HDMIA)
> +			test_display_planes_random(&data, port,
> +						   CHAMELIUM_CHECK_CRC);
> +
> +
>  		connector_subtest("hdmi-frame-dump", HDMIA)
>  			test_display_frame_dump(&data, port);
>  	}
Paul Kocialkowski Dec. 13, 2018, 2:39 p.m.
Hi,

Thanks for the review!

On Thu, 2018-12-06 at 17:52 -0500, Lyude Paul wrote:
> I agree with Maxime, this definitely should be split up into some more
> functions. Some other comments below below
> 
> On Thu, 2018-12-06 at 15:11 +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 a CRC.
> > 
> > This test also includes testing for some VC4-specific features, such as
> > T-tiled mode (in XR24 format), bandwidth limitation and underrun
> > (that require kernel-side patches that are currently under review).
> > 
> > Since this test does not share much with previous CRC-based 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 kind of fuzzing test.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  tests/kms_chamelium.c | 331 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 331 insertions(+)
> > 
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index 7d95a8bc..a4977309 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -26,6 +26,8 @@
> > 
> >  #include "config.h"
> >  #include "igt.h"
> > +#include "igt_sysfs.h"
> > +#include "igt_vc4.h"
> > 
> >  #include <fcntl.h>
> >  #include <string.h>
> > @@ -703,6 +705,330 @@ test_display_frame_dump(data_t *data, struct
> > chamelium_port *port)
> >  	drmModeFreeConnector(connector);
> >  }
> > 
> > +static uint32_t random_plane_formats[] = {
> > +	DRM_FORMAT_ABGR8888,
> > +	DRM_FORMAT_ARGB1555,
> > +	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_RGB565,
> > +	DRM_FORMAT_BGR565,
> > +	DRM_FORMAT_RGB888,
> > +	DRM_FORMAT_BGR888,
> > +	DRM_FORMAT_XBGR8888,
> > +	DRM_FORMAT_XRGB1555,
> > +	DRM_FORMAT_XRGB8888,
> > +};
> > +
> > +static void test_display_planes_random(data_t *data,
> > +				       struct chamelium_port *port,
> > +				       enum chamelium_check check)
> > +{
> > +	igt_output_t *output;
> > +	igt_pipe_t *pipe;
> > +	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;
> > +	unsigned int overlay_planes_index;
> > +	bool bandwidth_exceeded = false;
> > +	bool underrun_detected = false;
> > +	cairo_surface_t *result_surface;
> > +	cairo_surface_t *primary_surface;
> > +	cairo_surface_t *overlay_surface;
> > +	cairo_t *cr;
> > +	int captured_frame_count;
> > +	unsigned int i;
> > +	char *underrun;
> > +	int fb_id;
> > +	int debugfs;
> > +	int ret;
> > +
> > +	igt_assert(check == CHAMELIUM_CHECK_CRC);
> > +
> > +	srand(time(NULL));
> > +
> > +	reset_state(data, port);
> > +
> > +	/* Find the connector and pipe. */
> > +	output = prepare_output(data, port);
> > +	igt_assert(output);
> Do we actually need this? We're not running igt_assert on the prepare_output()
> function for any of the other tests.

This can indeed be dropped. Looking at prepare_output, a few things
would fail before the returns NULL anyway.

> > +
> > +	if (is_vc4_device(data->drm_fd)) {
> > +		debugfs = igt_debugfs_dir(data->drm_fd);
> > +		igt_assert(debugfs >= 0);
> > +	}
> > +
> > +	pipe = &data->display.pipes[output->pending_pipe];
> > +
> > +	mode = igt_output_get_mode(output);
> > +	igt_assert(mode);
> This assert doesn't look like it's needed either

That function can never return NULL anyway, so you're totally right :)

> > +
> > +	/* 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);
> > +
> > +	igt_plane_set_fb(primary_plane, &primary_fb);
> > +
> > +	primary_surface = igt_get_cairo_surface(data->drm_fd, &primary_fb);
> > +
> > +	/* 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);
> > +
> > +	/* Initialize cairo with the result surface as target. */
> > +	result_surface = igt_get_cairo_surface(data->drm_fd, &result_fb);
> > +
> > +	cr = cairo_create(result_surface);
> > +
> > +	/* Blend the primary surface. */
> > +	cairo_set_source_surface(cr, primary_surface, 0, 0);
> > +	cairo_surface_destroy(primary_surface);
> > +	cairo_paint(cr);
> > +
> > +	cairo_destroy(cr);
> > +
> > +	/* Count available overlay planes. */
> > +	for (i = 0; i < pipe->n_planes; i++) {
> > +		igt_plane_t *plane = &pipe->planes[i];
> > +
> > +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> > +			overlay_planes_max++;
> > +	}
> Why not add a helper for this in a seperate patch? This seems quite
> useful

Yes, defintiely!

> > +
> > +	/* Limit the number of planes to a reasonable scene. */
> > +	if (overlay_planes_max > 4)
> > +		overlay_planes_max = 4;
> > +
> > +	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)
> > +
> > +	for (i = 0, overlay_planes_index = 0;
> > +	     i < pipe->n_planes && overlay_planes_index <
> > overlay_planes_count;
> > +	     i++) {
> > +		struct igt_fb *overlay_fb =
> > &overlay_fbs[overlay_planes_index];
> > +		igt_plane_t *plane = &pipe->planes[i];
> > +		struct igt_fb pattern_fb;
> > +		uint32_t overlay_fb_w, overlay_fb_h;
> > +		int32_t overlay_crtc_x, overlay_crtc_y;
> > +		uint32_t overlay_crtc_w, overlay_crtc_h;
> > +		uint32_t overlay_src_x, overlay_src_y;
> > +		uint32_t overlay_src_w, overlay_src_h;
> > +		int overlay_surface_x, overlay_surface_y;
> > +		unsigned int index;
> > +		unsigned int stride_min;
> > +		unsigned int stride;
> > +		bool vc4_t_tiled;
> > +		uint32_t format;
> > +
> > +		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
> > +			continue;
> We could also add a helper for looping through planes of specific types,
> since we seem to do that multiple times here and will probably do it in
> future tests at some poit

That would definitely make sense, I'll craft something in that
direction in the next revision.

> > +
> > +		/* Randomize width and height in the mode dimensions range. */
> > +		overlay_fb_w = (rand() % mode->hdisplay) + 1;
> > +		overlay_fb_h = (rand() % mode->vdisplay) + 1;
> > +
> > +		/*
> > +		 * Randomize source offset, but keep at least half of the
> > +		 * original size.
> > +		 */
> > +		overlay_src_x = rand() % (overlay_fb_w / 2);
> > +		overlay_src_y = rand() % (overlay_fb_h / 2);
> > +
> > +		/*
> > +		 * The on-crtc size does not include the source offset, so it
> > +		 * needs to be subtracted to avoid scaling.
> > +		 */
> > +		overlay_crtc_w = overlay_fb_w - overlay_src_x;
> > +		overlay_crtc_h = overlay_fb_h - overlay_src_y;
> > +
> > +		/* Same goes for the framebuffer source size. */
> > +		overlay_src_w = overlay_crtc_w;
> > +		overlay_src_h = overlay_crtc_h;
> > +
> > +		/*
> > +		 * Randomize the on-crtc position and allow the plane to go
> > +		 * off-display by up to half of its width and height.
> > +		 */
> > +		overlay_crtc_x = (rand() % mode->hdisplay) - overlay_crtc_w /
> > 2;
> > +		overlay_crtc_y = (rand() % mode->vdisplay) - overlay_crtc_h /
> > 2;
> > +
> > +		igt_debug("Plane %d: on-crtc size %dx%d\n",
> > +			  overlay_planes_index, overlay_crtc_w,
> > overlay_crtc_h);
> > +		igt_debug("Plane %d: on-crtc position %dx%d\n",
> > +			  overlay_planes_index, overlay_crtc_x,
> > overlay_crtc_y);
> > +		igt_debug("Plane %d: in-framebuffer position %dx%d\n",
> > +			  overlay_planes_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 the use of tiled mode with a 1/4 probability. */
> > +		index = rand() % 4;
> > +
> > +		if (is_vc4_device(data->drm_fd) && index == 0) {
> > +			format = DRM_FORMAT_XRGB8888;
> > +			vc4_t_tiled = true;
> > +
> > +			igt_debug("Plane %d: VC4 T-tiled %s format\n",
> > +				  overlay_planes_index,
> > igt_format_str(format));
> > +		} else {
> > +			/* Randomize the format to test. */
> > +			index = rand() % ARRAY_SIZE(random_plane_formats);
> > +			format = random_plane_formats[index];
> > +			vc4_t_tiled = false;
> > +
> > +			igt_debug("Plane %d: %s format\n",
> > overlay_planes_index,
> > +				  igt_format_str(format));
> > +		}
> > +
> > +		/* Convert the pattern to the test format if needed. */
> > +		if (vc4_t_tiled) {
> > +			fb_id = igt_vc4_fb_t_tiled_convert(overlay_fb,
> > +							   &pattern_fb);
> > +			igt_assert(fb_id > 0);
> > +		} else {
> > +			stride_min = overlay_fb_w *
> > +				     igt_format_plane_bpp(format, 0) / 8;
> > +
> > +			/* Randomize the stride with at most twice the
> > minimum. */
> > +			stride = (rand() % stride_min) + stride_min;
> > +
> > +			/* Pixman requires the stride to be aligned to 32-byte
> > words. */
> > +			stride = ALIGN(stride, sizeof(uint32_t));
> > +
> > +			igt_debug("Plane %d: using stride %d\n",
> > overlay_planes_index, stride);
> > +
> > +			fb_id = igt_fb_convert_with_stride(overlay_fb,
> > +							   &pattern_fb,
> > +							   format, stride);
> > +			igt_assert(fb_id);
> > +		}
> > +
> > +		igt_plane_set_fb(plane, overlay_fb);
> > +
> > +		overlay_surface = igt_get_cairo_surface(data->drm_fd,
> > +							&pattern_fb);
> > +
> > +		/* Blend the overlay surface. */
> > +		overlay_surface_x = overlay_crtc_x - (int) overlay_src_x;
> > +		overlay_surface_y = overlay_crtc_y - (int) overlay_src_y;
> > +
> > +		cr = cairo_create(result_surface);
> > +
> > +		cairo_set_source_surface(cr, overlay_surface,
> > overlay_surface_x,
> > +					 overlay_surface_y);
> > +		cairo_surface_destroy(overlay_surface);
> > +
> > +		/* Clip the surface to a rectangle. */
> > +		cairo_rectangle(cr, overlay_crtc_x, overlay_crtc_y,
> > +				overlay_crtc_w, overlay_crtc_h);
> > +		cairo_clip(cr);
> > +
> > +		cairo_paint(cr);
> > +
> > +		cairo_destroy(cr);
> > +
> > +		/* Configure the plane with framebuffer source coordinates. */
> > +		igt_plane_set_position(plane, overlay_crtc_x, overlay_crtc_y);
> > +		igt_plane_set_size(plane, overlay_crtc_w, overlay_crtc_h);
> > +
> > +		igt_fb_set_position(overlay_fb, plane, overlay_src_x,
> > +				    overlay_src_y);
> > +		igt_fb_set_size(overlay_fb, plane, overlay_src_w,
> > +				overlay_src_h);
> > +
> > +		igt_remove_fb(data->drm_fd, &pattern_fb);
> > +
> > +		overlay_planes_index++;
> > +	}
> > +
> > +	cairo_surface_destroy(result_surface);
> > +
> > +	fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd,
> > +							&result_fb);
> > +
> > +	if (is_vc4_device(data->drm_fd)) {
> > +		ret = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
> > +		bandwidth_exceeded = (ret < 0 && errno == ENOSPC);
> > +
> > +		igt_debug("Bandwidth limitation exeeded: %s\n",
> > +			  bandwidth_exceeded ? "Yes" : "No");
> > +
> > +		igt_assert(!bandwidth_exceeded);
> > +	}
> Why does this need to be handled separately for VC4 devices? We already
> print the return code from a failed atomic commit into the debugging
> output, and I'd assume anyone looking at the output from the failure of
> one of these tests probably knows what that means. Additionally, if we
> wanted to print debugging messages for bandwidth limitations being
> exceeded for atomic commits on vc4, seems like it would probably be
> better to just add that code to igt_display_try_commit2()

You're right, after all the following call will produce more or less
the same result and there is nothing really specific to the VC4 there.
I'll drop it in the future.

> > +
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > +
> > +	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);
> > +
> > +	if (is_vc4_device(data->drm_fd)) {
> > +		underrun = igt_sysfs_get(debugfs, "underrun");
> > +		igt_assert(underrun);
> > +
> > +		underrun_detected = (underrun[0] == 'Y');
> > +		free(underrun);
> > +
> > +		igt_debug("Underrun detected: %s\n",
> > +			  underrun_detected ? "Yes" : "No");
> > +
> > +		igt_assert(!underrun_detected);
> > +
> > +		close(debugfs);
> > +	}
> This will definitely be used again in the future, this should also go
> into a helper for vc4.

Since we're having problems with underrun testing and the Chamelium, I
think I'll drop that part for now and maybe re-introduce it as a vc4
helper (it's specific to the vc4 driver in the latest iteration) later.

> > +
> > +	expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc);
> > +
> > +	chamelium_assert_crc_eq_or_dump(data->chamelium,
> > +					expected_crc, crc,
> > +					&result_fb, i);
> > +
> > +	igt_debug("reference CRC: %s\n", igt_crc_to_string(expected_crc));
> > +	igt_debug("calculated CRC: %s\n", igt_crc_to_string(crc));
> Why not just add this debugging output to
> chamelium_assert_crc_eq_or_dump()?

Yes, let's do that.

> > +
> > +	free(expected_crc);
> > +	free(crc);
> > +
> > +	for (i = 0, overlay_planes_index = 0;
> > +	     i < pipe->n_planes && overlay_planes_index <
> > overlay_planes_count;
> > +	     i++) {
> > +		igt_plane_t *plane = &pipe->planes[i];
> > +		struct igt_fb *overlay_fb =
> > &overlay_fbs[overlay_planes_index];
> > +
> > +		if (plane->type != DRM_PLANE_TYPE_OVERLAY)
> > +			continue;
> > +
> > +		igt_remove_fb(data->drm_fd, overlay_fb);
> > +
> > +		overlay_planes_index++;
> > +	}
> > +
> > +	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)
> >  {
> > @@ -981,6 +1307,11 @@ igt_main
> >  			test_display_one_mode(&data, port, DRM_FORMAT_NV12,
> >  					      CHAMELIUM_CHECK_ANALOG, 1);
> > 
> > +		connector_subtest("hdmi-crc-planes-random", HDMIA)
> > +			test_display_planes_random(&data, port,
> > +						   CHAMELIUM_CHECK_CRC);
> > +
> > +
> >  		connector_subtest("hdmi-frame-dump", HDMIA)
> >  			test_display_frame_dump(&data, port);
> >  	}

Cheers,

Paul