kms_rotation_crc: 90 degree flip test is not a stress test
PATCH 1/1 kms_rotation_crc: 90 degree flip test is not a stress test
From: Tvrtko Ursulin <[email protected]>
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.
--- tests/intel-ci/extended.testlist | 2 +- tests/kms_rotation_crc.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
tests/intel-ci/extended.testlist +1 -1
@@ -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
tests/kms_rotation_crc.c +2 -2
@@ -650,10 +650,10 @@ igt_maintest_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 ); }
Signature · 6 lines
2.9.4 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
Quoted 5 lines
From: Tvrtko Ursulin <[email protected]>
To the best of my recollection the page flipping test was added simply to start exercising page flips with 90/270 rotation.
Quoted 7 lines
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 11 lines · Chris Wilson
Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
Quoted 10 lines
From: Tvrtko Ursulin <[email protected]>
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.
Quoted 6 lines
Quoted 2 lines
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Tvrtko Ursulin (2017-08-03 14:09:59)
Quoted 20 lines · Chris Wilson
Quoted 11 lines
Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
Quoted 10 lines
From: Tvrtko Ursulin <[email protected]>
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.
Quoted 6 lines
Quoted 2 lines
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)?
Quoted 6 lines
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 28 lines · Chris Wilson
Quoting Tvrtko Ursulin (2017-08-03 14:09:59)
Quoted 20 lines · Chris Wilson
Quoted 11 lines
Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
Quoted 10 lines
From: Tvrtko Ursulin <[email protected]>
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.
Quoted 6 lines
Quoted 2 lines
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)?
Quoted 6 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Tvrtko Ursulin (2017-08-03 14:41:34)
Quoted 34 lines · Chris Wilson
Quoted 28 lines
Quoting Tvrtko Ursulin (2017-08-03 14:09:59)
Quoted 20 lines · Chris Wilson
Quoted 11 lines
Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
Quoted 10 lines
From: Tvrtko Ursulin <[email protected]>
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.
Quoted 6 lines
Quoted 2 lines
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)?
Quoted 6 lines
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.Quoted 3 lines
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.
Quoted 8 lines
Not in
kms_rotation_crc, so one to pencil in on the TODO list.Quoted 2 lines
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. :)
Quoted 5 lines
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 40 lines · Chris Wilson
Quoting Tvrtko Ursulin (2017-08-03 14:41:34)
Quoted 39 lines · Chris Wilson
Quoted 28 lines
Quoting Tvrtko Ursulin (2017-08-03 14:09:59)
Quoted 20 lines · Chris Wilson
Quoted 11 lines
Quoting Tvrtko Ursulin (2017-08-03 13:42:50)
Quoted 10 lines
From: Tvrtko Ursulin <[email protected]>
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.
Quoted 6 lines
Quoted 2 lines
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)?
Quoted 6 lines
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.Quoted 3 lines
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.Quoted 2 lines
PS please add the explanation for why it took longer than expected into the commit log.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 11 lines
Quoted 5 lines
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.
Quoted 2 lines
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Tvrtko Ursulin (2017-08-03 15:33:54)
Quoted 9 lines · Chris Wilson
Quoted 2 lines
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 10 lines · Tvrtko Ursulin
From: Tvrtko Ursulin <[email protected]>
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.
Quoted 7 lines
So remove the stress from the name and reduce the number of flips to three only.
Signed-off-by Tvrtko Ursulin <[email protected]>Cc Daniel Vetter <[email protected]>--- 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.
Quoted 21 lines
tests/kms_rotation_crc.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)tests/intel-ci/extended.testlist +1
-1@@ -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-pc8tests/kms_rotation_crc.c +0
-0
tests/kms_rotation_crc.c igt_main · line 656
Quoted 9 lines
tests/kms_rotation_crc.c +2
-2@@ -650,10 +650,10 @@ igt_maintest_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:
Quoted 6 lines
tests/kms_rotation_crc.c +0
-0data.rotation =IGT_ROTATION_90 ;test_plane_rotation (&data,DRM_PLANE_TYPE_PRIMARY ); }Signature · 1 line
2.9.4
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH v2 1/1 kms_rotation_crc: 90 degree flip test is not a stress test
In reply to Tvrtko UrsulinFrom: Tvrtko Ursulin <[email protected]>
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.
--- tests/intel-ci/extended.testlist | 2 +- tests/kms_rotation_crc.c | 137 ++++++++++++++++++++++++--------------- 2 files changed, 85 insertions(+), 54 deletions(-)
tests/intel-ci/extended.testlist +1 -1
@@ -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
tests/kms_rotation_crc.c +84 -53
184 lines not on this page · raw message
Signature · 6 lines
2.9.4 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 13 lines · Tvrtko Ursulin
From: Tvrtko Ursulin <[email protected]>
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:
Quoted 8 lines
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?
Quoted 5 lines
4. Convert to table driven subtest generation.
Signed-off-by Tvrtko Ursulin <[email protected]>Cc Daniel Vetter <[email protected]>Cc Chris Wilson <[email protected]>
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).
Quoted 216 lines
--- tests/intel-ci/extended.testlist | 2 +- tests/kms_rotation_crc.c | 137 ++++++++++++++++++++++++--------------- 2 files changed, 85 insertions(+), 54 deletions(-)tests/intel-ci/extended.testlist +1
-1@@ -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-pc8tests/kms_rotation_crc.c +84
-53@@ -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_mainigt_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_maintest_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;Signature · 1 line
2.9.4
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 27 lines · Daniel Vetter
Quoted 13 lines · Tvrtko Ursulin
From: Tvrtko Ursulin <[email protected]>
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:
Quoted 8 lines
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?
Quoted 2 lines
4. Convert to table driven subtest generation.
Quoted 7 lines
Quoted 3 lines
Signed-off-by Tvrtko Ursulin <[email protected]>Cc Daniel Vetter <[email protected]>Cc Chris Wilson <[email protected]>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
Quoted 203 lines
Acked-by Daniel Vetter <[email protected]>Quoted 202 lines
--- tests/intel-ci/extended.testlist | 2 +- tests/kms_rotation_crc.c | 137 ++++++++++++++++++++++++--------------- 2 files changed, 85 insertions(+), 54 deletions(-)tests/intel-ci/extended.testlist +1
-1@@ -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-pc8tests/kms_rotation_crc.c +84
-45@@ -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_mainigt_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_maintest_plane_rotation (&data,DRM_PLANE_TYPE_PRIMARY ); }
Quoted 15 lines
Quoted 14 lines
tests/kms_rotation_crc.c +0
-8- 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;Signature · 1 line
2.9.4
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 32 lines · Tvrtko Ursulin
Quoted 31 lines · Daniel Vetter
Quoted 13 lines · Tvrtko Ursulin
From: Tvrtko Ursulin <[email protected]>
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:
Quoted 8 lines
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?
Quoted 5 lines
4. Convert to table driven subtest generation.
Signed-off-by Tvrtko Ursulin <[email protected]>Cc Daniel Vetter <[email protected]>Cc Chris Wilson <[email protected]>
Quoted 5 lines
Quoted 3 lines
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
Quoted 217 lines
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
Quoted 213 lines
Acked-by Daniel Vetter <[email protected]>Quoted 211 lines
--- tests/intel-ci/extended.testlist | 2 +- tests/kms_rotation_crc.c | 137 ++++++++++++++++++++++++--------------- 2 files changed, 85 insertions(+), 54 deletions(-)tests/intel-ci/extended.testlist +1
-1@@ -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-pc8tests/kms_rotation_crc.c +2
-2@@ -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 );}tests/kms_rotation_crc.c +82
-51@@ -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_mainigt_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_maintest_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;Signature · 1 line
2.9.4
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 38 lines · Tvrtko Ursulin
Quoted 36 lines · Tvrtko Ursulin
Quoted 34 lines · Daniel Vetter
Quoted 13 lines · Tvrtko Ursulin
From: Tvrtko Ursulin <[email protected]>
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:
Quoted 8 lines
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?
Quoted 5 lines
4. Convert to table driven subtest generation.
Signed-off-by Tvrtko Ursulin <[email protected]>Cc Daniel Vetter <[email protected]>Cc Chris Wilson <[email protected]>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).
Quoted 8 lines
Quoted 2 lines
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
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 48 lines · Daniel Vetter
Quoted 38 lines · Tvrtko Ursulin
Quoted 36 lines · Tvrtko Ursulin
Quoted 34 lines · Daniel Vetter
Quoted 13 lines · Tvrtko Ursulin
From: Tvrtko Ursulin <[email protected]>
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:
Quoted 8 lines
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?
Quoted 5 lines
4. Convert to table driven subtest generation.
Signed-off-by Tvrtko Ursulin <[email protected]>Cc Daniel Vetter <[email protected]>Cc Chris Wilson <[email protected]>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).
Quoted 8 lines
Quoted 2 lines
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.
Quoted 6 lines
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 97 lines · Tvrtko Ursulin
Hi,
Quoted 83 lines · Katarzyna Dec
Quoted 82 lines · Tvrtko Ursulin
Quoted 28 lines · Daniel Vetter
Quoted 14 lines · Tvrtko Ursulin
From: Tvrtko Ursulin <[email protected]>
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:
Quoted 8 lines
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?
Quoted 2 lines
4. Convert to table driven subtest generation.
Quoted 7 lines
Quoted 3 lines
Signed-off-by Tvrtko Ursulin <[email protected]>Cc Daniel Vetter <[email protected]>Cc Chris Wilson <[email protected]>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
Quoted 35 lines
Acked-by Daniel Vetter <[email protected]>Quoted 34 lines
--- tests/intel-ci/extended.testlist | 2 +- tests/kms_rotation_crc.c | 137 ++++++++++++++++++++++++--------------- 2 files changed, 85 insertions(+), 54 deletions(-)tests/intel-ci/extended.testlist +1
-1@@ -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-pc8tests/kms_rotation_crc.c +2
-2@@ -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) {Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
tests/kms_rotation_crc.c static void test_plane_rotation(data_t *data, int plane_type) · line 353
Quoted 7 lines
tests/kms_rotation_crc.c +0
-0igt_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)
Quoted 4 lines
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 :)
Quoted 36 lines
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
tests/kms_rotation_crc.c +12
-7- }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);Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
tests/kms_rotation_crc.c err_commit: · line 575
Quoted 5 lines
tests/kms_rotation_crc.c +0
-0} 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.
Quoted 3 lines
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.
Quoted 3 lines
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx