[igt-dev] [i-g-t, v2] tests/kms_plane: Generate reference CRCs for partial coverage too
Jeremy Cline
jcline at redhat.com
Tue Aug 25 21:04:40 UTC 2020
On Tue, Aug 18, 2020 at 03:20:54PM -0400, Lyude wrote:
> From: Lyude Paul <lyude at redhat.com>
>
> There's been a TODO sitting in the kms_plane test suite for a while now
> as a placeholder for actually generating a reference CRC for
> plane-position-hole tests. This means we have never actually been
> verifying whether or not partially covering our hole with a hardware
> plane works displays properly, and have just simply been checking that
> the frame CRC doesn't change when we're not changing the display output.
> On nouveau at least, this has been causing us to pass these tests
> even when we don't correctly program our hardware plane's framebuffer.
>
> So, get rid of that TODO and implement this by converting
> convert_fb_for_mode__position() to a generic convert_fb_for_mode()
> function that allows us to create a colored FB, either with or without a
> series of 64x64 rectangles, and use that in test_grab_crc() to generate
> reference CRCs for the plane-position-hole tests. Additionally, we move
> around all of the test flags into a single enumerator, and make sure
> none of them have overlapping bits so we can correctly tell from
> test_grab_crc() whether or not our reference CRC should include
> rectangles (otherwise, we'll accidentally add rectangles in our
> reference frame for the plane panning tests).
>
> Then, we finally flip the TEST_POSITION_FULLY_COVERED enum to
> TEST_POSITION_PARTIALLY_COVERED, so tests which don't explicitly specify
> partial positioning don't get it in their reference fbs.
>
> v2:
> * Rebase
>
> Signed-off-by: Lyude Paul <lyude at redhat.com>
> ---
> tests/kms_plane.c | 164 +++++++++++++++++++++++++---------------------
> 1 file changed, 91 insertions(+), 73 deletions(-)
>
> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> index 430210d8..1b5e7fe9 100644
> --- a/tests/kms_plane.c
> +++ b/tests/kms_plane.c
> @@ -44,6 +44,11 @@ typedef struct {
> float blue;
> } color_t;
>
> +typedef struct {
> + int x, y;
> + color_t color;
> +} rectangle_t;
> +
> typedef struct {
> int drm_fd;
> igt_display_t display;
> @@ -71,9 +76,52 @@ static void test_fini(data_t *data)
> igt_pipe_crc_free(data->pipe_crc);
> }
>
> +enum {
> + TEST_POSITION_PARTIALLY_COVERED = 1 << 0,
> + TEST_DPMS = 1 << 1,
> + TEST_PANNING_TOP_LEFT = 1 << 2,
> + TEST_PANNING_BOTTOM_RIGHT = 1 << 3,
> + TEST_SUSPEND_RESUME = 1 << 4,
> +};
> +
> +/*
> + * create a colored fb, possibly with a series of 64x64 colored rectangles (used
> + * for position tests)
> + */
> +static void
> +create_fb_for_mode(data_t *data, drmModeModeInfo *mode,
> + color_t *fb_color,
> + const rectangle_t *rects, int rect_cnt,
> + struct igt_fb *fb /* out */)
> +{
It looks like data is only ever used to access the data->drm_fd. It's
real nit-picky, but it might be nice to accept the fd to line up the API
with igt_create_fb() and friends for the sake of familiarity and
consistency. The others also return the fb_id or possible error code,
although it does seem to be ignored by callers so...
After more reviewing I notice this was the prior API so I'd consider
this comment even more nit-picky.
> + unsigned int fb_id;
> + cairo_t *cr;
> +
> + fb_id = igt_create_fb(data->drm_fd,
> + mode->hdisplay, mode->vdisplay,
> + DRM_FORMAT_XRGB8888,
> + LOCAL_DRM_FORMAT_MOD_NONE,
> + fb);
> + igt_assert_fd(fb_id);
> +
> + cr = igt_get_cairo_ctx(data->drm_fd, fb);
> + igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay,
> + fb_color->red, fb_color->green, fb_color->blue);
> + for (int i = 0; i < rect_cnt; i++) {
> + const rectangle_t *rect = &rects[i];
> + igt_paint_color(cr,
> + rect->x, rect->y, 64, 64,
> + rect->color.red,
> + rect->color.green,
> + rect->color.blue);
> + }
> +
> + igt_put_cairo_ctx(cr);
> +}
> +
> static void
> test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
> - color_t *fb_color, igt_crc_t *crc /* out */)
> + color_t *fb_color, unsigned int flags, igt_crc_t *crc /* out */)
> {
> struct igt_fb fb;
> drmModeModeInfo *mode;
> @@ -86,13 +134,19 @@ test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
> primary = igt_output_get_plane(output, 0);
>
> mode = igt_output_get_mode(output);
> - igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> - DRM_FORMAT_XRGB8888,
> - LOCAL_DRM_FORMAT_MOD_NONE,
> - fb_color->red, fb_color->green, fb_color->blue,
> - &fb);
> - igt_plane_set_fb(primary, &fb);
> + if (flags & TEST_POSITION_PARTIALLY_COVERED) {
> + static const rectangle_t rects[] = {
> + { .x = 100, .y = 100, .color = { 0.0, 0.0, 0.0 }},
> + { .x = 132, .y = 132, .color = { 0.0, 1.0, 0.0 }},
> + };
> +
> + create_fb_for_mode(data, mode, fb_color,
> + rects, ARRAY_SIZE(rects), &fb);
> + } else {
> + create_fb_for_mode(data, mode, fb_color, NULL, 0, &fb);
If I understand correctly, this is equivalent to a call to
igt_create_color_fb(). If that is indeed the case, it might be clearer
to just call that here.
> + }
>
> + igt_plane_set_fb(primary, &fb);
> ret = igt_display_try_commit2(&data->display, COMMIT_LEGACY);
> igt_skip_on(ret != 0);
>
> @@ -104,19 +158,24 @@ test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
> igt_remove_fb(data->drm_fd, &fb);
>
> crc_str = igt_crc_to_string(crc);
> - igt_debug("CRC for a (%.02f,%.02f,%.02f) fb: %s\n", fb_color->red,
> - fb_color->green, fb_color->blue, crc_str);
> + igt_debug("CRC for a %s covered (%.02f,%.02f,%.02f) fb: %s\n",
> + flags & TEST_POSITION_PARTIALLY_COVERED ? "partially" : "fully",
> + fb_color->red, fb_color->green, fb_color->blue,
> + crc_str);
> free(crc_str);
> }
>
> /*
> * Plane position test.
> - * - We start by grabbing a reference CRC of a full green fb being scanned
> - * out on the primary plane
> + * - For testing positions that fully cover our hole, we start by grabbing a
> + * reference CRC of a full green fb being scanned out on the primary plane.
> + * For testing positions that only partially cover our hole, we instead use
> + * a full green fb with a partially covered black rectangle.
> * - Then we scannout 2 planes:
> * - the primary plane uses a green fb with a black rectangle
> * - a plane, on top of the primary plane, with a green fb that is set-up
> - * to cover the black rectangle of the primary plane fb
> + * to fully or partially cover the black rectangle of the primary plane
> + * fb
> * The resulting CRC should be identical to the reference CRC
> */
>
> @@ -125,38 +184,6 @@ typedef struct {
> igt_crc_t reference_crc;
> } test_position_t;
>
> -/*
> - * create a green fb with a black rectangle at (rect_x,rect_y) and of size
> - * (rect_w,rect_h)
> - */
> -static void
> -create_fb_for_mode__position(data_t *data, drmModeModeInfo *mode,
> - double rect_x, double rect_y,
> - double rect_w, double rect_h,
> - struct igt_fb *fb /* out */)
> -{
> - unsigned int fb_id;
> - cairo_t *cr;
> -
> - fb_id = igt_create_fb(data->drm_fd,
> - mode->hdisplay, mode->vdisplay,
> - DRM_FORMAT_XRGB8888,
> - LOCAL_DRM_FORMAT_MOD_NONE,
> - fb);
> - igt_assert(fb_id);
> -
> - cr = igt_get_cairo_ctx(data->drm_fd, fb);
> - igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay,
> - 0.0, 1.0, 0.0);
> - igt_paint_color(cr, rect_x, rect_y, rect_w, rect_h, 0.0, 0.0, 0.0);
> - igt_put_cairo_ctx(cr);
> -}
> -
> -enum {
> - TEST_POSITION_FULLY_COVERED = 1 << 0,
> - TEST_DPMS = 1 << 1,
> -};
> -
> static void
> test_plane_position_with_output(data_t *data,
> enum pipe pipe,
> @@ -165,6 +192,7 @@ test_plane_position_with_output(data_t *data,
> igt_crc_t *reference_crc,
> unsigned int flags)
> {
> + rectangle_t rect = { .x = 100, .y = 100, .color = { 0.0, 0.0, 0.0 }};
> igt_plane_t *primary, *sprite;
> struct igt_fb primary_fb, sprite_fb;
> drmModeModeInfo *mode;
> @@ -179,26 +207,26 @@ test_plane_position_with_output(data_t *data,
> primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> sprite = igt_output_get_plane(output, plane);
>
> - create_fb_for_mode__position(data, mode, 100, 100, 64, 64,
> - &primary_fb);
> + create_fb_for_mode(data, mode, &green, &rect, 1, &primary_fb);
> igt_plane_set_fb(primary, &primary_fb);
>
> igt_create_color_fb(data->drm_fd,
> - 64, 64, /* width, height */
> - DRM_FORMAT_XRGB8888,
> - LOCAL_DRM_FORMAT_MOD_NONE,
> - 0.0, 1.0, 0.0,
> - &sprite_fb);
> + 64, 64, /* width, height */
> + DRM_FORMAT_XRGB8888,
> + LOCAL_DRM_FORMAT_MOD_NONE,
> + 0.0, 1.0, 0.0,
> + &sprite_fb);
I'd include any whitespace-only changes separately, but don't know what
the project's preferences are there.
> igt_plane_set_fb(sprite, &sprite_fb);
>
> - if (flags & TEST_POSITION_FULLY_COVERED)
> - igt_plane_set_position(sprite, 100, 100);
> - else
> + if (flags & TEST_POSITION_PARTIALLY_COVERED)
> igt_plane_set_position(sprite, 132, 132);
> + else
> + igt_plane_set_position(sprite, 100, 100);
>
> igt_display_commit(&data->display);
>
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
> + igt_assert_crc_equal(reference_crc, &crc);
>
🎉
> if (flags & TEST_DPMS) {
> kmstest_set_connector_dpms(data->drm_fd,
> @@ -211,12 +239,6 @@ test_plane_position_with_output(data_t *data,
>
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc2);
>
> - if (flags & TEST_POSITION_FULLY_COVERED)
> - igt_assert_crc_equal(reference_crc, &crc);
> - else {
> - ;/* FIXME: missing reference CRCs */
> - }
> -
> igt_assert_crc_equal(&crc, &crc2);
>
> igt_plane_set_fb(primary, NULL);
> @@ -237,8 +259,7 @@ test_plane_position(data_t *data, enum pipe pipe, unsigned int flags)
> igt_require(output);
>
> test_init(data, pipe);
> -
> - test_grab_crc(data, output, pipe, &green, &reference_crc);
> + test_grab_crc(data, output, pipe, &green, flags, &reference_crc);
>
> for (int plane = 1; plane < n_planes; plane++)
> test_plane_position_with_output(data, pipe, plane,
> @@ -287,12 +308,6 @@ create_fb_for_mode__panning(data_t *data, drmModeModeInfo *mode,
> igt_put_cairo_ctx(cr);
> }
>
> -enum {
> - TEST_PANNING_TOP_LEFT = 1 << 0,
> - TEST_PANNING_BOTTOM_RIGHT = 1 << 1,
> - TEST_SUSPEND_RESUME = 1 << 2,
> -};
> -
> static void
> test_plane_panning_with_output(data_t *data,
> enum pipe pipe,
> @@ -355,8 +370,8 @@ test_plane_panning(data_t *data, enum pipe pipe, unsigned int flags)
>
> test_init(data, pipe);
>
> - test_grab_crc(data, output, pipe, &red, &red_crc);
> - test_grab_crc(data, output, pipe, &blue, &blue_crc);
> + test_grab_crc(data, output, pipe, &red, flags, &red_crc);
> + test_grab_crc(data, output, pipe, &blue, flags, &blue_crc);
>
> for (int plane = 1; plane < n_planes; plane++)
> test_plane_panning_with_output(data, pipe, plane, output,
> @@ -961,15 +976,18 @@ run_tests_for_pipe_plane(data_t *data, enum pipe pipe)
> data->crop = 0;
> igt_subtest_f("plane-position-covered-pipe-%s-planes",
> kmstest_pipe_name(pipe))
> - test_plane_position(data, pipe, TEST_POSITION_FULLY_COVERED);
> + test_plane_position(data, pipe, 0);
>
> igt_subtest_f("plane-position-hole-pipe-%s-planes",
> kmstest_pipe_name(pipe))
> - test_plane_position(data, pipe, 0);
> + test_plane_position(data, pipe,
> + TEST_POSITION_PARTIALLY_COVERED);
>
> igt_subtest_f("plane-position-hole-dpms-pipe-%s-planes",
> kmstest_pipe_name(pipe))
> - test_plane_position(data, pipe, TEST_DPMS);
> + test_plane_position(data, pipe,
> + TEST_POSITION_PARTIALLY_COVERED |
> + TEST_DPMS);
>
> igt_subtest_f("plane-panning-top-left-pipe-%s-planes",
> kmstest_pipe_name(pipe))
Very minor nits, overall it looks good to me:
Reviewed-by: Jeremy Cline <jcline at redhat.com>
More information about the igt-dev
mailing list