[igt-dev] [PATCH i-g-t v3 21/21] chamelium: Add a display test for randomized planes
Paul Kocialkowski
paul.kocialkowski at bootlin.com
Fri Jan 25 13:58:56 UTC 2019
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 at 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
More information about the igt-dev
mailing list