[igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC
Mika Kahola
mika.kahola at intel.com
Fri Mar 9 11:25:44 UTC 2018
On Fri, 2018-03-09 at 09:40 +0100, Maarten Lankhorst wrote:
> Op 28-02-18 om 14:44 schreef Mika Kahola:
> >
> > In CI runs we every now and then fail to read correct CRC yielding
> > an error
> > when comparing reference and grabbed CRC's. Let's first fix the
> > test so that
> > we drain the pipe first and then read the correct CRC. While at it,
> > let's
> > simplify the test by combining legacy and atomic tests into a one
> > common
> > function.
> >
> > v2: We don't need to drain pipe when we grab first CRC
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=103166
> > Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> > ---
> > tests/kms_plane_multiple.c | 142 ++++++++++++---------------------
> > ------------
> > 1 file changed, 38 insertions(+), 104 deletions(-)
> >
> > diff --git a/tests/kms_plane_multiple.c
> > b/tests/kms_plane_multiple.c
> > index 95b7138..ff8ede3 100644
> > --- a/tests/kms_plane_multiple.c
> > +++ b/tests/kms_plane_multiple.c
> > @@ -32,7 +32,6 @@
> >
> > IGT_TEST_DESCRIPTION("Test atomic mode setting with multiple
> > planes ");
> >
> > -#define MAX_CRCS 2
> > #define SIZE_PLANE 256
> > #define SIZE_CURSOR 128
> > #define LOOP_FOREVER -1
> > @@ -46,6 +45,7 @@ typedef struct {
> > typedef struct {
> > int drm_fd;
> > igt_display_t display;
> > + igt_crc_t ref_crc;
> > igt_pipe_crc_t *pipe_crc;
> > igt_plane_t **plane;
> > struct igt_fb *fb;
> > @@ -92,20 +92,23 @@ static void test_fini(data_t *data,
> > igt_output_t *output, int n_planes)
> > igt_output_set_pipe(output, PIPE_ANY);
> >
> > igt_pipe_crc_free(data->pipe_crc);
> > + data->pipe_crc = NULL;
> >
> > free(data->plane);
> > data->plane = NULL;
> > - free(data->fb);
> > - data->fb = NULL;
> > +
> > + igt_remove_fb(data->drm_fd, data->fb);
> > +
> > + igt_display_reset(&data->display);
> > }
> >
> > static void
> > -test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
> > bool atomic,
> > - color_t *color, uint64_t tiling, igt_crc_t **crc /*
> > out */)
> > +test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
> > int commit,
> > + color_t *color, uint64_t tiling)
> > {
> > drmModeModeInfo *mode;
> > igt_plane_t *primary;
> > - int ret, n;
> > + int ret;
> >
> > igt_output_set_pipe(output, pipe);
> >
> > @@ -122,13 +125,16 @@ test_grab_crc(data_t *data, igt_output_t
> > *output, enum pipe pipe, bool atomic,
> >
> > igt_plane_set_fb(data->plane[primary->index], &data-
> > >fb[primary->index]);
> >
> > - ret = igt_display_try_commit2(&data->display,
> > - atomic ? COMMIT_ATOMIC :
> > COMMIT_LEGACY);
> > + ret = igt_display_try_commit2(&data->display, commit);
> > igt_skip_on(ret != 0);
> >
> > - igt_pipe_crc_start(data->pipe_crc);
> > - n = igt_pipe_crc_get_crcs(data->pipe_crc, 1, crc);
> > - igt_assert_eq(n, 1);
> > + if (commit == COMMIT_LEGACY) {
> > + igt_pipe_crc_collect_crc(data->pipe_crc, &data-
> > >ref_crc);
> > + } else {
> > + igt_pipe_crc_start(data->pipe_crc);
> > + igt_pipe_crc_drain(data->pipe_crc);
> > + igt_pipe_crc_get_single(data->pipe_crc, &data-
> > >ref_crc);
> > + }
> Why the 2 different paths?
The *_start *_get_single method didn't work for COMMIT_ATOMIC so I
decided to use legacy way for COMMIT_LEGACY. Maybe we could after all
drop the legacy commits from this test. Originally, this test was all
about atomic tests.
>
> And again, after pipe_crc_start no _drain() is needed, we already
> have a good CRC.
Yeah, this thing is an echo from the past. I remember we already
discussed this when previous patch was under review.
Thanks for the review!
> >
> > }
> >
> > /*
> > @@ -239,17 +245,13 @@ prepare_planes(data_t *data, enum pipe
> > pipe_id, color_t *color,
> > }
> >
> > static void
> > -test_atomic_plane_position_with_output(data_t *data, enum pipe
> > pipe,
> > - igt_output_t *output, int
> > n_planes,
> > - uint64_t tiling)
> > +test_plane_position_with_output(data_t *data, enum pipe pipe,
> > + igt_output_t *output, int
> > n_planes,
> > + uint64_t tiling, int commit)
> > {
> > - char buf[256];
> > - struct drm_event *e = (void *)buf;
> > color_t blue = { 0.0f, 0.0f, 1.0f };
> > - igt_crc_t *ref = NULL;
> > - igt_crc_t *crc = NULL;
> > - unsigned int vblank_start, vblank_stop;
> > - int i, n, ret;
> > + igt_crc_t crc;
> > + int i;
> > int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> > bool loop_forever;
> > char info[256];
> > @@ -269,89 +271,27 @@ test_atomic_plane_position_with_output(data_t
> > *data, enum pipe pipe,
> >
> > test_init(data, pipe, n_planes);
> >
> > - test_grab_crc(data, output, pipe, true, &blue, tiling,
> > &ref);
> > + test_grab_crc(data, output, pipe, commit, &blue, tiling);
> >
> > i = 0;
> > while (i < iterations || loop_forever) {
> > prepare_planes(data, pipe, &blue, tiling,
> > n_planes, output);
> >
> > - vblank_start = kmstest_get_vblank(data-
> > >display.drm_fd, pipe,
> > - DRM_VBLANK_NEXTO
> > NMISS);
> > + igt_display_commit2(&data->display, commit);
> >
> > - igt_display_commit_atomic(&data->display,
> > - DRM_MODE_PAGE_FLIP_EVENT
> > ,
> > - &data->display);
> > -
> > - igt_set_timeout(1, "Stuck on page flip");
> > -
> > - ret = read(data->display.drm_fd, buf,
> > sizeof(buf));
> > - igt_assert(ret >= 0);
> > -
> > - vblank_stop = kmstest_get_vblank(data-
> > >display.drm_fd, pipe, 0);
> > - igt_assert_eq(e->type, DRM_EVENT_FLIP_COMPLETE);
> > - igt_reset_timeout();
> > -
> > - n = igt_pipe_crc_get_crcs(data->pipe_crc,
> > vblank_stop - vblank_start, &crc);
> > -
> > - igt_assert(vblank_stop - vblank_start <=
> > MAX_CRCS);
> > - igt_assert_eq(n, vblank_stop - vblank_start);
> > -
> > - igt_assert_crc_equal(ref, crc);
> > -
> > - i++;
> > - }
> > -
> > - igt_pipe_crc_stop(data->pipe_crc);
> > -
> > - test_fini(data, output, n_planes);
> > -}
> > -
> > -static void
> > -test_legacy_plane_position_with_output(data_t *data, enum pipe
> > pipe,
> > - igt_output_t *output, int
> > n_planes,
> > - uint64_t tiling)
> > -{
> > - color_t blue = { 0.0f, 0.0f, 1.0f };
> > - igt_crc_t *ref = NULL;
> > - igt_crc_t *crc = NULL;
> > - int i, n;
> > - int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> > - bool loop_forever;
> > - char info[256];
> > -
> > - if (opt.iterations == LOOP_FOREVER) {
> > - loop_forever = true;
> > - sprintf(info, "forever");
> > - } else {
> > - loop_forever = false;
> > - sprintf(info, "for %d %s",
> > - iterations, iterations > 1 ? "iterations"
> > : "iteration");
> > - }
> > -
> > - igt_info("Testing connector %s using pipe %s with %d
> > planes %s with seed %d\n",
> > - igt_output_name(output), kmstest_pipe_name(pipe),
> > n_planes,
> > - info, opt.seed);
> > -
> > - test_init(data, pipe, n_planes);
> > -
> > - test_grab_crc(data, output, pipe, false, &blue, tiling,
> > &ref);
> > -
> > - i = 0;
> > - while (i < iterations || loop_forever) {
> > - prepare_planes(data, pipe, &blue, tiling,
> > n_planes, output);
> > -
> > - igt_display_commit2(&data->display,
> > COMMIT_LEGACY);
> > -
> > - n = igt_pipe_crc_get_crcs(data->pipe_crc,
> > MAX_CRCS, &crc);
> > -
> > - igt_assert_eq(n, MAX_CRCS);
> > -
> > - igt_assert_crc_equal(ref, crc);
> > + if (commit == COMMIT_LEGACY) {
> > + igt_pipe_crc_collect_crc(data->pipe_crc,
> > &crc);
> > + } else {
> > + igt_pipe_crc_drain(data->pipe_crc);
> > + igt_pipe_crc_get_single(data->pipe_crc,
> > &crc);
> > + }
> Again why the separate paths?
> >
> > + igt_assert_crc_equal(&data->ref_crc, &crc);
> >
> > i++;
> > }
> >
> > - igt_pipe_crc_stop(data->pipe_crc);
> > + if (commit != COMMIT_LEGACY)
> > + igt_pipe_crc_stop(data->pipe_crc);
> >
> > test_fini(data, output, n_planes);
> > }
> > @@ -378,17 +318,11 @@ test_plane_position(data_t *data, enum pipe
> > pipe, bool atomic, uint64_t tiling)
> >
> > connected_outs = 0;
> > for_each_valid_output_on_pipe(&data->display, pipe,
> > output) {
> > - if (atomic)
> > - test_atomic_plane_position_with_output(dat
> > a, pipe,
> > - out
> > put,
> > - n_p
> > lanes,
> > - til
> > ing);
> > - else
> > - test_legacy_plane_position_with_output(dat
> > a, pipe,
> > - out
> > put,
> > - n_p
> > lanes,
> > - til
> > ing);
> > -
> > + test_plane_position_with_output(data, pipe,
> > + output,
> > + n_planes,
> > + tiling,
> > + atomic ?
> > COMMIT_ATOMIC : COMMIT_LEGACY);
> > connected_outs++;
> > }
> >
>
--
Mika Kahola - Intel OTC
More information about the igt-dev
mailing list