On Thu, Sep 07, 2017 at 03:20:19PM +0100, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 07/09/2017 15:13, Katarzyna Dec wrote:
> > On Mon, Sep 04, 2017 at 03:27:26PM +0100, 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.
> > >
> > > 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);
> > Why do we need below changes? (just trying to understand during review)
>
> The move of the flip from after CRC collection to before? I think it makes
> the subtests which flip test more useful so it actually verifies not only
> the flip ioctl succeeded but the expected image is on screen.
>
Thanks for the answer :)
> > > > > - } 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);
> > > > > }
> > I ran all test on drm-tip kernel and flip subtests are failing.
> > Is it a bug or because of changes?
> > I ran on NUC with KBL.
>
> All flip subtests or just the sprite plane ones? I tested on SKL and there
> primary plane works, but the sprite plane seems broken.
>
I run it on drm-tip kernel on NUC with KBL. Only flip tests were faling.
Other passed.
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
