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

[Tvrtko Ursulin]

On 08/09/2017 15:06, Chris Wilson wrote:
> 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...

Yes, hm, it would be really bad if both flips silently did nothing, but 
the last CRC would still match. So yes, still a holey hole there.

(Note we are not changing rotation with the flip here, just flipping 
between two equally rotated fbs.)

I can see two options:

a) Stick a crc not equal assert in between the flips.
b) Collect a second CRC and check both.

I will have to asses the runtime impact of all the options. At the 
moment test setup time is quite long. Is it CRC collection, or modeset..

>> +                        */
>> +                       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.
Thanks.

Regards,

Tvrtko

> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>