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

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@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 test each pipe and then each fb geometry. And
calling this a stress test is also not matching the original
idea of the test.

So remove the stress from the name and reduce the number of
flips to three only.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 tests/intel-ci/extended.testlist | 2 +-
 tests/kms_rotation_crc.c         | 4 ++--
 2 files changed, 3 insertions(+), 3 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..a04a2e68547d 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -650,10 +650,10 @@ igt_main
 		test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
 	}
 
-	igt_subtest_f("primary-rotation-90-flip-stress") {
+	igt_subtest_f("primary-rotation-90-flip") {
 		igt_require(gen >= 9);
 		data.override_tiling = 0;
-		data.flip_stress = 60;
+		data.flip_stress = 3;
 		data.rotation = IGT_ROTATION_90;
 		test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
 	}
-- 
2.9.4

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

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
> From: Tvrtko Ursulin <tvrtko.ursulin@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 test each pipe and then each fb geometry. And
> calling this a stress test is also not matching the original
> idea of the test.
> 
> So remove the stress from the name and reduce the number of
> flips to three only.

Considering this found a bug, do we have an explicit test that says a
rotated page flip takes no longer than a vblank (given the right
conditions, i.e subsequent flips)?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 03/08/2017 13:53, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
>> From: Tvrtko Ursulin <tvrtko.ursulin@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 test each pipe and then each fb geometry. And
>> calling this a stress test is also not matching the original
>> idea of the test.
>>
>> So remove the stress from the name and reduce the number of
>> flips to three only.
> 
> Considering this found a bug, do we have an explicit test that says a
> rotated page flip takes no longer than a vblank (given the right
> conditions, i.e subsequent flips)?

That was just me misremembering how the test work, wasn't a bug. Once I 
looked at the code in more detail I realized the test does much more 
flipping than it initially seemed. Num_pipes * 4 fb geometries * 2 
framebuffers * 60 flips. In total around 8 seconds of flipping per pipe. 
So the 25 second runtime is in line with 3 pipes at 60Hz plus some test 
setup time.

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-08-03 14:09:59)
> 
> On 03/08/2017 13:53, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@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 test each pipe and then each fb geometry. And
> >> calling this a stress test is also not matching the original
> >> idea of the test.
> >>
> >> So remove the stress from the name and reduce the number of
> >> flips to three only.
> > 
> > Considering this found a bug, do we have an explicit test that says a
> > rotated page flip takes no longer than a vblank (given the right
> > conditions, i.e subsequent flips)?
> 
> That was just me misremembering how the test work, wasn't a bug. Once I 
> looked at the code in more detail I realized the test does much more 
> flipping than it initially seemed. Num_pipes * 4 fb geometries * 2 
> framebuffers * 60 flips. In total around 8 seconds of flipping per pipe. 
> So the 25 second runtime is in line with 3 pipes at 60Hz plus some test 
> setup time.

Ah. But do we have a test that says we can hit vrefresh with rotated
pipes? I know we check the timings for the ordinary case, but from a
quick check of the likely suspects, I can't see such a test.

PS please add the explanation for why it took longer than expected into
the commit log.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 03/08/2017 14:27, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-08-03 14:09:59)
>>
>> On 03/08/2017 13:53, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@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 test each pipe and then each fb geometry. And
>>>> calling this a stress test is also not matching the original
>>>> idea of the test.
>>>>
>>>> So remove the stress from the name and reduce the number of
>>>> flips to three only.
>>>
>>> Considering this found a bug, do we have an explicit test that says a
>>> rotated page flip takes no longer than a vblank (given the right
>>> conditions, i.e subsequent flips)?
>>
>> That was just me misremembering how the test work, wasn't a bug. Once I
>> looked at the code in more detail I realized the test does much more
>> flipping than it initially seemed. Num_pipes * 4 fb geometries * 2
>> framebuffers * 60 flips. In total around 8 seconds of flipping per pipe.
>> So the 25 second runtime is in line with 3 pipes at 60Hz plus some test
>> setup time.
> 
> Ah. But do we have a test that says we can hit vrefresh with rotated
> pipes? I know we check the timings for the ordinary case, but from a
> quick check of the likely suspects, I can't see such a test.

Not in kms_rotation_crc, so one to pencil in on the TODO list.

> PS please add the explanation for why it took longer than expected into
> the commit log.

I mentioned in the commit why it takes long ("...60 flips which can take 
quite some time because we test each pipe and then each fb 
geometry..."), but I am not sure that is longer than expected. Or to 
better say I am not sure what is expected. One could even say, since the 
test had stress in its name, that 25 seconds was not unexpected. :) Now 
it is not a stress test any more and it finishes in five seconds so all 
is good. I am not sure what exactly you think I should add? The formula 
for exact number of flips test was doing?

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-08-03 14:41:34)
> 
> On 03/08/2017 14:27, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-08-03 14:09:59)
> >>
> >> On 03/08/2017 13:53, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin@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 test each pipe and then each fb geometry. And
> >>>> calling this a stress test is also not matching the original
> >>>> idea of the test.
> >>>>
> >>>> So remove the stress from the name and reduce the number of
> >>>> flips to three only.
> >>>
> >>> Considering this found a bug, do we have an explicit test that says a
> >>> rotated page flip takes no longer than a vblank (given the right
> >>> conditions, i.e subsequent flips)?
> >>
> >> That was just me misremembering how the test work, wasn't a bug. Once I
> >> looked at the code in more detail I realized the test does much more
> >> flipping than it initially seemed. Num_pipes * 4 fb geometries * 2
> >> framebuffers * 60 flips. In total around 8 seconds of flipping per pipe.
> >> So the 25 second runtime is in line with 3 pipes at 60Hz plus some test
> >> setup time.
> > 
> > Ah. But do we have a test that says we can hit vrefresh with rotated
> > pipes? I know we check the timings for the ordinary case, but from a
> > quick check of the likely suspects, I can't see such a test.
> 
> Not in kms_rotation_crc, so one to pencil in on the TODO list.
> 
> > PS please add the explanation for why it took longer than expected into
> > the commit log.
> 
> I mentioned in the commit why it takes long ("...60 flips which can take 
> quite some time because we test each pipe and then each fb 
> geometry..."), but I am not sure that is longer than expected.

Even on a second read, it is still 60 flips, not 60 flips per
combination. :)

> better say I am not sure what is expected. One could even say, since the 
> test had stress in its name, that 25 seconds was not unexpected. :) Now 
> it is not a stress test any more and it finishes in five seconds so all 
> is good. I am not sure what exactly you think I should add? The formula 
> for exact number of flips test was doing?

Language. The test is a crc check, how many unique frames do we show?
What changes between every flip that applies any stress to the driver,
i.e. causes either the hw or driver to do something different? If we are
looking for a race, can crc even spot it? Do we gain anything from this
test by even displaying a second frame?

Ultimately, is the path through the driver taken by each frame a good
enough metric to decide if the test has achieved its maximal coverage?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 03/08/2017 15:19, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-08-03 14:41:34)
>>
>> On 03/08/2017 14:27, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2017-08-03 14:09:59)
>>>>
>>>> On 03/08/2017 13:53, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@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 test each pipe and then each fb geometry. And
>>>>>> calling this a stress test is also not matching the original
>>>>>> idea of the test.
>>>>>>
>>>>>> So remove the stress from the name and reduce the number of
>>>>>> flips to three only.
>>>>>
>>>>> Considering this found a bug, do we have an explicit test that says a
>>>>> rotated page flip takes no longer than a vblank (given the right
>>>>> conditions, i.e subsequent flips)?
>>>>
>>>> That was just me misremembering how the test work, wasn't a bug. Once I
>>>> looked at the code in more detail I realized the test does much more
>>>> flipping than it initially seemed. Num_pipes * 4 fb geometries * 2
>>>> framebuffers * 60 flips. In total around 8 seconds of flipping per pipe.
>>>> So the 25 second runtime is in line with 3 pipes at 60Hz plus some test
>>>> setup time.
>>>
>>> Ah. But do we have a test that says we can hit vrefresh with rotated
>>> pipes? I know we check the timings for the ordinary case, but from a
>>> quick check of the likely suspects, I can't see such a test.
>>
>> Not in kms_rotation_crc, so one to pencil in on the TODO list.
>>
>>> PS please add the explanation for why it took longer than expected into
>>> the commit log.
>>
>> I mentioned in the commit why it takes long ("...60 flips which can take
>> quite some time because we test each pipe and then each fb
>> geometry..."), but I am not sure that is longer than expected.
> 
> Even on a second read, it is still 60 flips, not 60 flips per
> combination. :)

Ah ok, it isn't fully understandable. That's what happens with quick 
patches which only change one trivial thing.

>> better say I am not sure what is expected. One could even say, since the
>> test had stress in its name, that 25 seconds was not unexpected. :) Now
>> it is not a stress test any more and it finishes in five seconds so all
>> is good. I am not sure what exactly you think I should add? The formula
>> for exact number of flips test was doing?
> 
> Language. The test is a crc check, how many unique frames do we show?
> What changes between every flip that applies any stress to the driver,
> i.e. causes either the hw or driver to do something different? If we are
> looking for a race, can crc even spot it? Do we gain anything from this
> test by even displaying a second frame?

I don't think we gain much by doing more than one flip and there are no 
crc checks or anything. So from that pov it is not the best test. Should 
be probably changed so in flip mode it flips before the crc collection.

> Ultimately, is the path through the driver taken by each frame a good
> enough metric to decide if the test has achieved its maximal coverage?

There should be a ton more coverage that we should add before calling it 
maximal coverage. Like tiling changes between flips, which is currently 
tested only for unrotated fbs. some rotation changes might also be 
possible between flips?

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-08-03 15:33:54)
> 
> On 03/08/2017 15:19, Chris Wilson wrote:
> > Ultimately, is the path through the driver taken by each frame a good
> > enough metric to decide if the test has achieved its maximal coverage?
> 
> There should be a ton more coverage that we should add before calling it 
> maximal coverage. Like tiling changes between flips, which is currently 
> tested only for unrotated fbs. some rotation changes might also be 
> possible between flips?

That's fine. My question is more about can we assume driver coverage as
a reasonable guide for hw coverage, or rather how often can the hw do
something different when we take an identical path through the driver?

The worst offender I can think of is memory ordering, where the hw isn't
as strict as we would like and we spend lots of effort in trying to
enforce + check behaviour. But even then, our enforcement only takes a
few different patterns so the number of tests we need should be finite.
But I feel the trap there is by only testing what the driver does, we
never test what it does *not* do. And that is my conundrum how to find
the missing tests; how to reduce tests to achieve similar edge coverage
through the driver is easy enough (cf american fuzzy loop).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Aug 03, 2017 at 01:42:50PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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 test each pipe and then each fb geometry. And
> calling this a stress test is also not matching the original
> idea of the test.
> 
> So remove the stress from the name and reduce the number of
> flips to three only.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  tests/intel-ci/extended.testlist | 2 +-

fyi, extended.testlist is dead, definitely for everything minus
gem_|prime_, because we just run them all. I guess time for me to create a
patch to remove it.

>  tests/kms_rotation_crc.c         | 4 ++--
>  2 files changed, 3 insertions(+), 3 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..a04a2e68547d 100644
> --- a/tests/kms_rotation_crc.c
> +++ b/tests/kms_rotation_crc.c
> @@ -650,10 +650,10 @@ igt_main
>  		test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
>  	}
>  
> -	igt_subtest_f("primary-rotation-90-flip-stress") {
> +	igt_subtest_f("primary-rotation-90-flip") {
>  		igt_require(gen >= 9);
>  		data.override_tiling = 0;
> -		data.flip_stress = 60;
> +		data.flip_stress = 3;

s/flip_stress/flips/ as a bikeshed, to avoid confusion. Either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  		data.rotation = IGT_ROTATION_90;
>  		test_plane_rotation(&data, DRM_PLANE_TYPE_PRIMARY);
>  	}
> -- 
> 2.9.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@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.

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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Fri, Aug 04, 2017 at 09:43:41AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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.
> 
> 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@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

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).

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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@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.
>>
>> 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@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 
> 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@ffwll.ch>
>> ---
>>   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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@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.
>>>
>>> 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@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> 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@ffwll.ch>
>>> ---
>>>   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Mon, Sep 04, 2017 at 03:36:56PM +0100, Tvrtko Ursulin wrote:
> 
> 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@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.
> > > > 
> > > > 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@intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > 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.

Failing new subtests is ok imo, but pls do give a heads-up to CI folks
before merging (but I think as long as it's failing stable, it shouldn't
cause noise). Anything that takes out the machine or driver isn't ok, and
needs to be fixed first.

So sounds all good to me to go ahead.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 04/09/2017 15:43, Daniel Vetter wrote:
> On Mon, Sep 04, 2017 at 03:36:56PM +0100, Tvrtko Ursulin wrote:
>>
>> 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@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.
>>>>>
>>>>> 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@intel.com>
>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>
>>>> 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.
> 
> Failing new subtests is ok imo, but pls do give a heads-up to CI folks
> before merging (but I think as long as it's failing stable, it shouldn't
> cause noise). Anything that takes out the machine or driver isn't ok, and
> needs to be fixed first.
> 
> So sounds all good to me to go ahead.

I thought we said full review rules on IGT, no? So I won't be merging 
this until someone can look at it in more detail.

Tvrtko

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

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

[Katarzyna Dec]

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@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.
> > > > > 
> > > > > 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@intel.com>
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > 
> > > > 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@ffwll.ch>
> > > > > ---
> > > > >    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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx