[igt-dev] [PATCH i-g-t 9/9] chamelium: Add a CRC-based display test for randomized planes
Paul Kocialkowski
paul.kocialkowski at bootlin.com
Thu Dec 13 14:39:36 UTC 2018
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 at 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
--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com
More information about the igt-dev
mailing list