[i-g-t,v3,1/2] tests/kms_flip: Set duration for subtest from command line

Submitted by Kahola, Mika on Aug. 9, 2018, 10:12 a.m.

Details

Message ID 1533809554-27925-2-git-send-email-mika.kahola@intel.com
State New
Headers show
Series "tests/kms_flip: Binary mode optimizations" ( rev: 4 ) in IGT

Not browsing as part of any series.

Commit Message

Kahola, Mika Aug. 9, 2018, 10:12 a.m.
To reduce the execution time of kms_flip test on CI, let's move subtest
duration parameter as command line option. The default subtest duration
is 3 seconds for test that require jitter computation and for the rest
of the subtests are run only once.

v2: Run each subtest only once (default action)
v3: Reduces default timeout for tests that require jitter computation (Ville)

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 tests/kms_flip.c | 108 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 75 insertions(+), 33 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index 393d690..4cd1951 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -82,6 +82,8 @@ 
 #define DRM_CAP_TIMESTAMP_MONOTONIC 6
 #endif
 
+#define MAX_DURATION            60
+
 drmModeRes *resources;
 int drm_fd;
 static drm_intel_bufmgr *bufmgr;
@@ -95,6 +97,13 @@  static drmModeConnector *last_connector;
 
 uint32_t *fb_ptr;
 
+/* Command line parameters. */
+struct {
+	int duration;
+} opt = {
+	.duration = 0,
+};
+
 struct type_name {
 	int type;
 	const char *name;
@@ -1521,56 +1530,90 @@  static void test_nonblocking_read(int in)
 	close(fd);
 }
 
+static int opt_handler(int option, int option_index, void *input)
+{
+	switch (option) {
+	case 'd':
+		opt.duration = strtol(optarg, NULL, 0);
+
+		if (opt.duration > MAX_DURATION) {
+			igt_debug("limiting test duration from %ds to %ds\n",
+				  opt.duration, MAX_DURATION);
+			opt.duration = MAX_DURATION;
+		}
+
+		if (opt.duration < 0) {
+			igt_debug("limiting test duration from %ds to %ds\n",
+				  opt.duration, 0);
+			opt.duration = 0;
+		}
+		break;
+	default:
+		igt_assert(false);
+	}
+
+	return 0;
+}
+
+const char *help_str =
+	"  --duration test duration in seconds (default 0s)\n";
+
 int main(int argc, char **argv)
 {
+	struct option long_options[] = {
+		{ "duration", required_argument, NULL, 'd'},
+		{ 0, 0, 0, 0 }
+	};
+
 	struct {
 		int duration;
 		int flags;
 		const char *name;
 	} tests[] = {
-		{ 30, TEST_VBLANK | TEST_CHECK_TS, "wf_vblank-ts-check" },
-		{ 30, TEST_VBLANK | TEST_VBLANK_BLOCK | TEST_CHECK_TS,
+		{ 3, TEST_VBLANK | TEST_CHECK_TS, "wf_vblank-ts-check" },
+		{ 3, TEST_VBLANK | TEST_VBLANK_BLOCK | TEST_CHECK_TS,
 					"blocking-wf_vblank" },
-		{ 30,  TEST_VBLANK | TEST_VBLANK_ABSOLUTE,
+		{ 3, TEST_VBLANK | TEST_VBLANK_ABSOLUTE,
 					"absolute-wf_vblank" },
-		{ 30,  TEST_VBLANK | TEST_VBLANK_BLOCK | TEST_VBLANK_ABSOLUTE,
+		{ 3, TEST_VBLANK | TEST_VBLANK_BLOCK | TEST_VBLANK_ABSOLUTE,
 					"blocking-absolute-wf_vblank" },
-		{ 10, TEST_FLIP | TEST_BASIC, "plain-flip" },
-		{ 30, TEST_FLIP | TEST_EBUSY , "busy-flip" },
-		{ 30, TEST_FLIP | TEST_FENCE_STRESS , "flip-vs-fences" },
-		{ 30, TEST_FLIP | TEST_CHECK_TS, "plain-flip-ts-check" },
-		{ 30, TEST_FLIP | TEST_CHECK_TS | TEST_FB_RECREATE,
+		{ 3, TEST_FLIP | TEST_BASIC, "plain-flip" },
+		{ 3, TEST_FLIP | TEST_EBUSY , "busy-flip" },
+		{ 3, TEST_FLIP | TEST_FENCE_STRESS , "flip-vs-fences" },
+		{ 3, TEST_FLIP | TEST_CHECK_TS, "plain-flip-ts-check" },
+		{ 3, TEST_FLIP | TEST_CHECK_TS | TEST_FB_RECREATE,
 			"plain-flip-fb-recreate" },
-		{ 30, TEST_FLIP | TEST_RMFB | TEST_MODESET , "flip-vs-rmfb" },
-		{ 20, TEST_FLIP | TEST_DPMS | TEST_EINVAL | TEST_BASIC, "flip-vs-dpms" },
-		{ 30,  TEST_FLIP | TEST_PAN, "flip-vs-panning" },
-		{ 20, TEST_FLIP | TEST_MODESET | TEST_EINVAL | TEST_BASIC, "flip-vs-modeset" },
-		{ 30,  TEST_FLIP | TEST_VBLANK_EXPIRED_SEQ,
+		{ 3, TEST_FLIP | TEST_RMFB | TEST_MODESET , "flip-vs-rmfb" },
+		{ 3, TEST_FLIP | TEST_DPMS | TEST_EINVAL | TEST_BASIC, "flip-vs-dpms" },
+		{ 3, TEST_FLIP | TEST_PAN, "flip-vs-panning" },
+		{ 3, TEST_FLIP | TEST_MODESET | TEST_EINVAL | TEST_BASIC, "flip-vs-modeset" },
+		{ 3, TEST_FLIP | TEST_VBLANK_EXPIRED_SEQ,
 					"flip-vs-expired-vblank" },
 
-		{ 30, TEST_FLIP | TEST_VBLANK | TEST_VBLANK_ABSOLUTE |
-		      TEST_CHECK_TS, "flip-vs-absolute-wf_vblank" },
-		{ 10, TEST_FLIP | TEST_VBLANK | TEST_CHECK_TS | TEST_BASIC,
+		{ 3, TEST_FLIP | TEST_VBLANK | TEST_VBLANK_ABSOLUTE |
+		  TEST_CHECK_TS, "flip-vs-absolute-wf_vblank" },
+		{ 3, TEST_FLIP | TEST_VBLANK | TEST_CHECK_TS | TEST_BASIC,
 					"flip-vs-wf_vblank" },
-		{ 30, TEST_FLIP | TEST_VBLANK | TEST_VBLANK_BLOCK |
-			TEST_CHECK_TS, "flip-vs-blocking-wf-vblank" },
-		{ 30, TEST_FLIP | TEST_MODESET | TEST_HANG | TEST_NOEVENT, "flip-vs-modeset-vs-hang" },
-		{ 30, TEST_FLIP | TEST_PAN | TEST_HANG, "flip-vs-panning-vs-hang" },
-		{ 1, TEST_FLIP | TEST_EINVAL | TEST_FB_BAD_TILING, "flip-vs-bad-tiling" },
+		{ 3, TEST_FLIP | TEST_VBLANK | TEST_VBLANK_BLOCK |
+		  TEST_CHECK_TS, "flip-vs-blocking-wf-vblank" },
+		{ 3, TEST_FLIP | TEST_MODESET | TEST_HANG | TEST_NOEVENT, "flip-vs-modeset-vs-hang" },
+		{ 3, TEST_FLIP | TEST_PAN | TEST_HANG, "flip-vs-panning-vs-hang" },
+		{ 3, TEST_FLIP | TEST_EINVAL | TEST_FB_BAD_TILING, "flip-vs-bad-tiling" },
 
-		{ 1, TEST_DPMS_OFF | TEST_MODESET | TEST_FLIP,
+		{ 0, TEST_DPMS_OFF | TEST_MODESET | TEST_FLIP,
 					"flip-vs-dpms-off-vs-modeset" },
-		{ 1, TEST_DPMS_OFF | TEST_MODESET | TEST_FLIP | TEST_SINGLE_BUFFER,
+		{ 0, TEST_DPMS_OFF | TEST_MODESET | TEST_FLIP | TEST_SINGLE_BUFFER,
 					"single-buffer-flip-vs-dpms-off-vs-modeset" },
-		{ 30, TEST_FLIP | TEST_NO_2X_OUTPUT | TEST_DPMS_OFF_OTHERS , "dpms-off-confusion" },
+		{ 0, TEST_FLIP | TEST_NO_2X_OUTPUT | TEST_DPMS_OFF_OTHERS , "dpms-off-confusion" },
 		{ 0, TEST_ENOENT | TEST_NOEVENT, "nonexisting-fb" },
-		{ 10, TEST_DPMS_OFF | TEST_DPMS | TEST_VBLANK_RACE, "dpms-vs-vblank-race" },
-		{ 10, TEST_MODESET | TEST_VBLANK_RACE, "modeset-vs-vblank-race" },
+		{ 3, TEST_DPMS_OFF | TEST_DPMS | TEST_VBLANK_RACE, "dpms-vs-vblank-race" },
+		{ 3, TEST_MODESET | TEST_VBLANK_RACE, "modeset-vs-vblank-race" },
 		{ 0, TEST_BO_TOOBIG | TEST_NO_2X_OUTPUT, "bo-too-big" },
 	};
 	int i;
 
-	igt_subtest_init(argc, argv);
+	igt_subtest_init_parse_opts(&argc, argv, "", long_options, help_str,
+				    opt_handler, NULL);
 
 	igt_fixture {
 		drm_fd = drm_open_driver_master(DRIVER_ANY);
@@ -1595,7 +1638,7 @@  int main(int argc, char **argv)
 		igt_subtest_f("%s%s",
 			      tests[i].flags & TEST_BASIC ? "basic-" : "",
 			      tests[i].name)
-			run_test(tests[i].duration, tests[i].flags);
+			run_test(max(opt.duration, tests[i].duration), tests[i].flags);
 
 		if (tests[i].flags & TEST_NO_2X_OUTPUT)
 			continue;
@@ -1605,7 +1648,7 @@  int main(int argc, char **argv)
 			continue;
 
 		igt_subtest_f( "2x-%s", tests[i].name)
-			run_pair(tests[i].duration, tests[i].flags);
+			run_pair(max(opt.duration, tests[i].duration), tests[i].flags);
 	}
 
 	igt_fork_signal_helper();
@@ -1617,7 +1660,7 @@  int main(int argc, char **argv)
 			continue;
 
 		igt_subtest_f( "%s-interruptible", tests[i].name)
-			run_test(tests[i].duration, tests[i].flags);
+			run_test(max(opt.duration, tests[i].duration), tests[i].flags);
 
 		if (tests[i].flags & TEST_NO_2X_OUTPUT)
 			continue;
@@ -1627,7 +1670,7 @@  int main(int argc, char **argv)
 			continue;
 
 		igt_subtest_f( "2x-%s-interruptible", tests[i].name)
-			run_pair(tests[i].duration, tests[i].flags);
+			run_pair(max(opt.duration, tests[i].duration), tests[i].flags);
 	}
 	igt_stop_signal_helper();
 
@@ -1635,6 +1678,5 @@  int main(int argc, char **argv)
 	 * Let drm_fd leak, since it's needed by the dpms restore
 	 * exit_handler and igt_exit() won't return.
 	 */
-
 	igt_exit();
 }

Comments

Quoting Mika Kahola (2018-08-09 11:12:33)
> To reduce the execution time of kms_flip test on CI, let's move subtest
> duration parameter as command line option. The default subtest duration
> is 3 seconds for test that require jitter computation and for the rest
> of the subtests are run only once.

I disagree with having tests that change between invocations. It's far
too easy for one to make a mistake and not configure the test correctly,
completely fairly to reproduce whatever bug one was seeking.

If you think both durations are interesting for different aspects, test
both.
-Chris
On Thu, 2018-08-09 at 11:20 +0100, Chris Wilson wrote:
> Quoting Mika Kahola (2018-08-09 11:12:33)
> > 
> > To reduce the execution time of kms_flip test on CI, let's move
> > subtest
> > duration parameter as command line option. The default subtest
> > duration
> > is 3 seconds for test that require jitter computation and for the
> > rest
> > of the subtests are run only once.
> I disagree with having tests that change between invocations. It's
> far
> too easy for one to make a mistake and not configure the test
> correctly,
> completely fairly to reproduce whatever bug one was seeking.
> 
So, should I remove the option of setting the duration for the
subtests? Originally, we have timeouts ranging from 0 to 30 seconds,
depending on a test. Based on my tests on GLK, I found out that tests
that require some jitter computation yields the similar results if we
run it for 3 or 30 seconds. When running less than 3 seconds there are
differences in jitter results. That's why I suggested that 3 seconds
would be a sweetspot for the subtests.

Should I just use 3 seconds timeout for all subtests and discard the
possibility to set the timeout? 

> If you think both durations are interesting for different aspects,
> test
> both.
> -Chris
Op 09-08-18 om 12:12 schreef Mika Kahola:
> To reduce the execution time of kms_flip test on CI, let's move subtest
> duration parameter as command line option. The default subtest duration
> is 3 seconds for test that require jitter computation and for the rest
> of the subtests are run only once.
>
> v2: Run each subtest only once (default action)
> v3: Reduces default timeout for tests that require jitter computation (Ville)
>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
I think this is the wrong approach. What we really want to do is kill tests from kms_flip
instead and move them to separate places.

Killing off all the interruptible tests would save 50% of the time. So all we have to do is
making sure that we have tests that test the missing ioctl's in in kms_atomic_interruptible,
and we would save 50% of the time.

After that we have to get rid of the unrelated tests to make the code more readable..
nonexisting-fb, dpms-vs-vblank-race, modeset-vs-vblank-race and bo-too-big would be
better off in a separate testcase.

Same for bad-tiling, but I already had a patch for that.
https://patchwork.freedesktop.org/patch/259361/

This would clean up kms_flip a lot, and make the test more readable.
On Thu, 2018-11-08 at 12:47 +0100, Maarten Lankhorst wrote:
> Op 09-08-18 om 12:12 schreef Mika Kahola:
> > To reduce the execution time of kms_flip test on CI, let's move
> > subtest
> > duration parameter as command line option. The default subtest
> > duration
> > is 3 seconds for test that require jitter computation and for the
> > rest
> > of the subtests are run only once.
> > 
> > v2: Run each subtest only once (default action)
> > v3: Reduces default timeout for tests that require jitter
> > computation (Ville)
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> 
> I think this is the wrong approach. What we really want to do is kill
> tests from kms_flip
> instead and move them to separate places.
> 
> Killing off all the interruptible tests would save 50% of the time.
> So all we have to do is
> making sure that we have tests that test the missing ioctl's in in
> kms_atomic_interruptible,
> and we would save 50% of the time.
kms_atomic_interruptible would probably be more logical place for
interrupt tests. I agree that it would reduce the execution time of the
kms_flip test but in the end of the day we still need to run those
interruptible tests and therefore we would end up increasing the
execution time of the kms_atomic_interruptible test.

Do you feel that we could have overall reduction in test execution if
we move these interruptible tests out from kms_flip? Maybe kms_flip
readability would improve and therefore worth the effort?
> 
> After that we have to get rid of the unrelated tests to make the code
> more readable..
> nonexisting-fb, dpms-vs-vblank-race, modeset-vs-vblank-race and bo-
> too-big would be
> better off in a separate testcase.
> 
> Same for bad-tiling, but I already had a patch for that.
> https://patchwork.freedesktop.org/patch/259361/
> 
> This would clean up kms_flip a lot, and make the test more readable.
>
Op 08-11-18 om 13:45 schreef Kahola, Mika:
> On Thu, 2018-11-08 at 12:47 +0100, Maarten Lankhorst wrote:
>> Op 09-08-18 om 12:12 schreef Mika Kahola:
>>> To reduce the execution time of kms_flip test on CI, let's move
>>> subtest
>>> duration parameter as command line option. The default subtest
>>> duration
>>> is 3 seconds for test that require jitter computation and for the
>>> rest
>>> of the subtests are run only once.
>>>
>>> v2: Run each subtest only once (default action)
>>> v3: Reduces default timeout for tests that require jitter
>>> computation (Ville)
>>>
>>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> I think this is the wrong approach. What we really want to do is kill
>> tests from kms_flip
>> instead and move them to separate places.
>>
>> Killing off all the interruptible tests would save 50% of the time.
>> So all we have to do is
>> making sure that we have tests that test the missing ioctl's in in
>> kms_atomic_interruptible,
>> and we would save 50% of the time.
> kms_atomic_interruptible would probably be more logical place for
> interrupt tests. I agree that it would reduce the execution time of the
> kms_flip test but in the end of the day we still need to run those
> interruptible tests and therefore we would end up increasing the
> execution time of the kms_atomic_interruptible test.
>
> Do you feel that we could have overall reduction in test execution if
> we move these interruptible tests out from kms_flip? Maybe kms_flip
> readability would improve and therefore worth the effort?
The kms_flip ones just double the execution time of the tests, and don't result in more coverage.

Testing the various ioctl's in a controlled fashion would result in better coverage and lower execution time.
Just look at how kms_atomic_interruptible is structure, it should be easy to add more tests to it and actually
guarantees that the ioctl being tested is interrupted.

I definitely hope that removing those tests increase readability, because of all the unrelated API test special
cases being removed, and only actual flip related tests remaining.

~Maarten
On Thu, 2018-11-08 at 14:11 +0100, Maarten Lankhorst wrote:
> Op 08-11-18 om 13:45 schreef Kahola, Mika:
> > On Thu, 2018-11-08 at 12:47 +0100, Maarten Lankhorst wrote:
> > > Op 09-08-18 om 12:12 schreef Mika Kahola:
> > > > To reduce the execution time of kms_flip test on CI, let's move
> > > > subtest
> > > > duration parameter as command line option. The default subtest
> > > > duration
> > > > is 3 seconds for test that require jitter computation and for
> > > > the
> > > > rest
> > > > of the subtests are run only once.
> > > > 
> > > > v2: Run each subtest only once (default action)
> > > > v3: Reduces default timeout for tests that require jitter
> > > > computation (Ville)
> > > > 
> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > 
> > > I think this is the wrong approach. What we really want to do is
> > > kill
> > > tests from kms_flip
> > > instead and move them to separate places.
> > > 
> > > Killing off all the interruptible tests would save 50% of the
> > > time.
> > > So all we have to do is
> > > making sure that we have tests that test the missing ioctl's in
> > > in
> > > kms_atomic_interruptible,
> > > and we would save 50% of the time.
> > 
> > kms_atomic_interruptible would probably be more logical place for
> > interrupt tests. I agree that it would reduce the execution time of
> > the
> > kms_flip test but in the end of the day we still need to run those
> > interruptible tests and therefore we would end up increasing the
> > execution time of the kms_atomic_interruptible test.
> > 
> > Do you feel that we could have overall reduction in test execution
> > if
> > we move these interruptible tests out from kms_flip? Maybe kms_flip
> > readability would improve and therefore worth the effort?
> 
> The kms_flip ones just double the execution time of the tests, and
> don't result in more coverage.
> 
> Testing the various ioctl's in a controlled fashion would result in
> better coverage and lower execution time.
> Just look at how kms_atomic_interruptible is structure, it should be
> easy to add more tests to it and actually
> guarantees that the ioctl being tested is interrupted.
> 
> I definitely hope that removing those tests increase readability,
> because of all the unrelated API test special
> cases being removed, and only actual flip related tests remaining.
Ok. I prepare a patch that removes interruptible tests from kms_flip.
Let's see how kms_flip looks after that change. I also look into adding
the removed tests to kms_atomic_interruptible. 
 
> 
> ~Maarten