On 25/04/16 09:51, Daniel Vetter wrote:
On Fri, Apr 22, 2016 at 04:11:11PM +0100, Chris Wilson wrote:
On Fri, Apr 22, 2016 at 03:41:55PM +0100, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
I would argue it is enough to test one pipe in the BAT set.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
tests/kms_pipe_crc_basic.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
index 4de53bc77a3a..291775934758 100644
--- a/tests/kms_pipe_crc_basic.c
+++ b/tests/kms_pipe_crc_basic.c
@@ -180,6 +180,9 @@ static void test_read_crc(data_t *data, int pipe, unsigned flags)
data_t data = {0, };
+#define test_prefix(i) ((i) == 0 ? "basic-" : "")
+#define pipe_name(i) ((i) + 'A')
+
igt_main
{
igt_skip_on_simulation();
@@ -196,39 +199,39 @@ igt_main
igt_display_init(&data.display, data.drm_fd);
}
- igt_subtest("bad-pipe")
+ igt_subtest("basic-bad-pipe")
test_bad_command(&data, "pipe D none");
- igt_subtest("bad-source")
+ igt_subtest("basic-bad-source")
test_bad_command(&data, "pipe A foo");
- igt_subtest("bad-nb-words-1")
+ igt_subtest("basic-bad-nb-words-1")
test_bad_command(&data, "pipe foo");
- igt_subtest("bad-nb-words-3")
+ igt_subtest("basic-bad-nb-words-3")
test_bad_command(&data, "pipe A none option");
for (int i = 0; i < 3; i++) {
- igt_subtest_f("read-crc-pipe-%c", 'A'+i)
+ igt_subtest_f("%sread-crc-pipe-%c", test_prefix(i), pipe_name(i))
So the CRC is the backchannel through which we measure the output on the
screen matches expectations. Do any of the following belong in the basic
test set? Having demonstrated that the CRC is functional, all the rest
are components of other tests - and if a bug if found in any of the
other basic tests, one can run the entire kms_pipe_crc to sanity check
the test suite itself.
The reason I wanted to include kms_pipe_crc_basic was not to validate the
kernel driver (it's a side-effect), but to make sure the CRC support is
really stable. Otherwise we can't rely on a full BAT run at all.
Okay, so these patches are more about defining on what full BAT is, not
trying to break full BAT.
And yes every single subtest (including testing on all the pipes) included
in there is for a bug in the CRC code that actually happened.
I know that we're not there yet with CI, and we don't yet run all the nice
kms_*crc tests we have. But imo this is crucial prep work for that, and
we really shouldn't reduce test coverage in this area. Same reason we have
1 edp sink crc testcase, essentially it's to make sure that we have a
reasonable basis to run the full test set.
Imo BAT should guarantee two things:
- no insta-fireworks
- all the validation stuff actually works and running more tests will give
you valid results
From the above reasons I'm against this patch.
My general idea was that the per-patch BAT (todays basic set) could get
away with running on one pipe, and then the overnight one (which we do
not have yet) would run the more extensive set.
So even if BAT runs everything on pipe A, it is still self-validating.
It is not that some tests would run on pipe A and some on all pipes,
that at least was the idea.
Motivation simply was that the current BAT run did 80 backlight on-off
cycles on my BDW RVP and just waiting for eDP to turn on 80 times takes
~50 seconds in total. The test runtime was around eight minutes here.
When I turned off legacy fbdev that went to ~4 minutes. And then with
these two (and other two) patches I shaved off 1-2 minute more.
Considering the time spent vs the typical system usage, which is fewer
display on-offs vs. much much more GEM operations, the current state
seemed like a sub-optimal balance to me. Especially in the context of
not allowing more GEM basic tests since the whole suite takes so long.
Also, for the kms_flip patches I posted I should probably have added
cloned test cases for the ones that I modified. So that we have for
example basic-plain-flip (one pipe) and plain-flip (all pipes as today).
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
