[Intel-gfx] [PATCH i-g-t v3] kms_rotation_crc: 90 degree flip test is not a stress test

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-09-08 12:24:07)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> 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.

Thanks for making it easy for me to follow! Sounds great.

> 
> v2:
> 
> 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.
> 
> 4. Convert to table driven subtest generation.
> 
> v3: Remove extended.testlist from the patch.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> ---
>  tests/kms_rotation_crc.c | 137 +++++++++++++++++++++++++++++------------------
>  1 file changed, 84 insertions(+), 53 deletions(-)
> 
> 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.

And back? We only check of the original framebuffer and not the rotated?
Or am I missing the point...

> +                        */
> +                       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);
>  }

Consolidation looks good, and the above changes make sense, but the
comment makes me wonder if there is another CRC check we could do.

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris