Re: [PATCH i-g-t v2] kms_rotation_crc: 90 degree flip test is not a stress test

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 04/09/2017 15:27, Tvrtko Ursulin wrote:
> On 07/08/2017 16:53, Daniel Vetter wrote:
> > On Fri, Aug 04, 2017 at 09:43:41AM +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > >
> > > To the best of my recollection the page flipping test was added
> > > simply to start exercising page flips with 90/270 rotation.
> > >
> > > There is no need to do 60 flips which can take quite some time,
> > > because we do 60 flips against each pipe and each fb geometry.
> > >
> > > Also, calling this a stress test is also not matching the
> > > original idea of the test.
> > >
> > > Several changes:
> > >
> > > 1. Remove the stress from the name and reduce the number of
> > > flips to one only.
> > >
> > > 2. Move the page flip before CRC collection for a more useful
> > > test.
> > >
> > > 3. Add more flipping tests, for different rotation and sprite
> > > planes.
> >
> > I assume you didn't make the test overall slower with this?
> >
> > > 4. Convert to table driven subtest generation.
> > >
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >
> > Didn't do a full review, but sounds all reasonable. And I assume you've
> > tested this at least locally (the igt patchwork CI instance doesn't do
> > full runs ... yet).
>
> Yes I've ran it on SKL. It adds more subtests so the runtime is longer, 
> but it also finds new bugs.

Runtime is longer compared to v1 of this patch BTW, I wasn't clear. It 
is roughly on par with the situation without this patch.

But v2 adds a lot more subtests, three of which fail (flip on a rotated 
sprite plane). So there is value in it I think even like that.

Tvrtko

> Ideally someone with display-fu picks this up, fixes the bug(s) this 
> exposes (or tells me the test is wrong), so we are not merging known 
> failing tests? Or even that is OK?
>
> Tvrtko
>
> > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > ---
> > >   tests/intel-ci/extended.testlist |   2 +-
> > >   tests/kms_rotation_crc.c         | 137 
> > > ++++++++++++++++++++++++---------------
> > >   2 files changed, 85 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/tests/intel-ci/extended.testlist 
> > > b/tests/intel-ci/extended.testlist
> > > index 17eed013f810..fb71091758c6 100644
> > > --- a/tests/intel-ci/extended.testlist
> > > +++ b/tests/intel-ci/extended.testlist
> > > @@ -82,7 +82,7 @@ igt@gem_ringfill@vebox-bomb
> > >   igt@gem_userptr_blits@stress-mm
> > >   igt@gem_userptr_blits@stress-mm-invalidate-close
> > >   igt@gem_userptr_blits@stress-mm-invalidate-close-overlap
> > > -igt@kms_rotation_crc@primary-rotation-90-flip-stress
> > > +igt@kms_rotation_crc@primary-rotation-90-flip
> > >   igt@pm_rpm@gem-execbuf-stress
> > >   igt@pm_rpm@gem-execbuf-stress-extra-wait
> > >   igt@pm_rpm@gem-execbuf-stress-pc8
> > > diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
> > > index 83e37f126f40..20f1adb67769 100644
> > > --- a/tests/kms_rotation_crc.c
> > > +++ b/tests/kms_rotation_crc.c
> > > @@ -41,7 +41,7 @@ typedef struct {
> > >       int pos_y;
> > >       uint32_t override_fmt;
> > >       uint64_t override_tiling;
> > > -    unsigned int flip_stress;
> > > +    unsigned int flips;
> > >   } data_t;
> > >   static void
> > > @@ -219,7 +219,7 @@ static void prepare_fbs(data_t *data, 
> > > igt_output_t *output,
> > >       igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, 
> > > &data->fb);
> > > -    if (data->flip_stress) {
> > > +    if (data->flips) {
> > >           igt_create_fb(data->gfx_fd, w, h, pixel_format, tiling, 
> > > &data->fb_flip);
> > >           paint_squares(data, IGT_ROTATION_0, &data->fb_flip, 0.92);
> > >       }
> > > @@ -351,15 +351,17 @@ static void test_plane_rotation(data_t *data, 
> > > int plane_type)
> > >               ret = igt_display_try_commit2(display, commit);
> > >               if (data->override_fmt || data->override_tiling) {
> > >                   igt_assert_eq(ret, -EINVAL);
> > > -            } else {
> > > -                igt_assert_eq(ret, 0);
> > > -                igt_pipe_crc_collect_crc(data->pipe_crc,
> > > -                              &crc_output);
> > > -                igt_assert_crc_equal(&data->ref_crc,
> > > -                              &crc_output);
> > > +                continue;
> > >               }
> > > -            flip_count = data->flip_stress;
> > > +            /* Verify commit was ok. */
> > > +            igt_assert_eq(ret, 0);
> > > +
> > > +            /*
> > > +             * If flips are requested flip away and back before
> > > +             * checking CRC.
> > > +             */
> > > +            flip_count = data->flips;
> > >               while (flip_count--) {
> > >                   ret = drmModePageFlip(data->gfx_fd,
> > >                                 output->config.crtc->crtc_id,
> > > @@ -376,6 +378,9 @@ static void test_plane_rotation(data_t *data, int 
> > > plane_type)
> > >                   igt_assert_eq(ret, 0);
> > >                   wait_for_pageflip(data->gfx_fd);
> > >               }
> > > +
> > > +            igt_pipe_crc_collect_crc(data->pipe_crc, &crc_output);
> > > +            igt_assert_crc_equal(&data->ref_crc, &crc_output);
> > >           }
> > >           valid_tests++;
> > > @@ -569,8 +574,66 @@ err_commit:
> > >       igt_assert_eq(ret, 0);
> > >   }
> > > +static const char *plane_test_str(unsigned plane)
> > > +{
> > > +    switch (plane) {
> > > +    case DRM_PLANE_TYPE_PRIMARY:
> > > +        return "primary";
> > > +    case DRM_PLANE_TYPE_OVERLAY:
> > > +        return "sprite";
> > > +    case DRM_PLANE_TYPE_CURSOR:
> > > +        return "cursor";
> > > +    default:
> > > +        igt_assert(0);
> > > +    }
> > > +}
> > > +
> > > +static const char *rot_test_str(igt_rotation_t rot)
> > > +{
> > > +    switch (rot) {
> > > +    case IGT_ROTATION_90:
> > > +        return "90";
> > > +    case IGT_ROTATION_180:
> > > +        return "180";
> > > +    case IGT_ROTATION_270:
> > > +        return "270";
> > > +    default:
> > > +        igt_assert(0);
> > > +    }
> > > +}
> > > +
> > > +static const char *flip_test_str(unsigned flips)
> > > +{
> > > +    if (flips > 1)
> > > +        return "-flip-stress";
> > > +    else if (flips)
> > > +        return "-flip";
> > > +    else
> > > +        return "";
> > > +}
> > > +
> > >   igt_main
> > >   {
> > > +    struct rot_subtest {
> > > +        unsigned plane;
> > > +        igt_rotation_t rot;
> > > +        unsigned flips;
> > > +    } *subtest, subtests[] = {
> > > +        { DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_90, 0 },
> > > +        { DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_180, 0 },
> > > +        { DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_270, 0 },
> > > +        { DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_90, 1 },
> > > +        { DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_180, 1 },
> > > +        { DRM_PLANE_TYPE_PRIMARY, IGT_ROTATION_270, 1 },
> > > +        { DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_90, 0 },
> > > +        { DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_180, 0 },
> > > +        { DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_270, 0 },
> > > +        { DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_90, 1 },
> > > +        { DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_180, 1 },
> > > +        { DRM_PLANE_TYPE_OVERLAY, IGT_ROTATION_270, 1 },
> > > +        { DRM_PLANE_TYPE_CURSOR, IGT_ROTATION_180, 0 },
> > > +        { 0, 0, 0}
> > > +    };
> > >       data_t data = {};
> > >       int gen = 0;
> > > @@ -586,43 +649,19 @@ igt_main
> > >           igt_display_init(&data.display, data.gfx_fd);
> > >       }
> > > -    igt_subtest_f("primary-rotation-180") {
> > > -        data.rotation = IGT_ROTATION_180;
> > > -        test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> > > -    }
> > > -
> > > -    igt_subtest_f("sprite-rotation-180") {
> > > -        data.rotation = IGT_ROTATION_180;
> > > -        test_plane_rotation(&data, DRM_PLANE_TYPE_OVERLAY);
> > > -    }
> > > -
> > > -    igt_subtest_f("cursor-rotation-180") {
> > > -        data.rotation = IGT_ROTATION_180;
> > > -        test_plane_rotation(&data, DRM_PLANE_TYPE_CURSOR);
> > > -    }
> > > -
> > > -    igt_subtest_f("primary-rotation-90") {
> > > -        igt_require(gen >= 9);
> > > -        data.rotation = IGT_ROTATION_90;
> > > -        test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> > > -    }
> > > -    igt_subtest_f("primary-rotation-270") {
> > > -        igt_require(gen >= 9);
> > > -        data.rotation = IGT_ROTATION_270;
> > > -        test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> > > -    }
> > > -
> > > -    igt_subtest_f("sprite-rotation-90") {
> > > -        igt_require(gen >= 9);
> > > -        data.rotation = IGT_ROTATION_90;
> > > -        test_plane_rotation(&data, DRM_PLANE_TYPE_OVERLAY);
> > > -    }
> > > -
> > > -    igt_subtest_f("sprite-rotation-270") {
> > > -        igt_require(gen >= 9);
> > > -        data.rotation = IGT_ROTATION_270;
> > > -        test_plane_rotation(&data, DRM_PLANE_TYPE_OVERLAY);
> > > +    for (subtest = subtests; subtest->rot; subtest++) {
> > > +        igt_subtest_f("%s-rotation-%s%s",
> > > +                  plane_test_str(subtest->plane),
> > > +                  rot_test_str(subtest->rot),
> > > +                  flip_test_str(subtest->flips)) {
> > > +            igt_require(!(subtest->rot &
> > > +                    (IGT_ROTATION_90 | IGT_ROTATION_270)) ||
> > > +                    gen >= 9);
> > > +            data.rotation = subtest->rot;
> > > +            data.flips = subtest->flips;
> > > +            test_plane_rotation(&data, subtest->plane);
> > > +        }
> > >       }
> > >       igt_subtest_f("sprite-rotation-90-pos-100-0") {
> > > @@ -650,14 +689,6 @@ igt_main
> > >           test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> > >       }
> > > -    igt_subtest_f("primary-rotation-90-flip-stress") {
> > > -        igt_require(gen >= 9);
> > > -        data.override_tiling = 0;
> > > -        data.flip_stress = 60;
> > > -        data.rotation = IGT_ROTATION_90;
> > > -        test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
> > > -    }
> > > -
> > >       igt_subtest_f("primary-rotation-90-Y-tiled") {
> > >           enum pipe pipe;
> > >           igt_output_t *output;
> > > --
> > > 2.9.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx