diff mbox

[I-G-T,3/3] igt/gem_mocs_settings: Reduce the amount of cascading failures

Message ID 1469784876-33201-5-git-send-email-peter.antoine@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Antoine July 29, 2016, 9:34 a.m. UTC
If one of the previous tests fails then the following tests fail.
This patch means that the following tests do not fail when the previous
test fails (for some cases).

Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 lib/igt_aux.c             | 67 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_aux.h             |  2 ++
 tests/gem_mocs_settings.c | 56 +++++++++++++++++++++------------------
 3 files changed, 99 insertions(+), 26 deletions(-)

Comments

Chris Wilson Aug. 1, 2016, 9:02 a.m. UTC | #1
On Fri, Jul 29, 2016 at 10:34:36AM +0100, Peter Antoine wrote:
> If one of the previous tests fails then the following tests fail.
> This patch means that the following tests do not fail when the previous
> test fails (for some cases).

The problem is just gem_mocs_settings hasn't split its tests up into
subtests.
-Chris
Peter Antoine Aug. 1, 2016, 9:33 a.m. UTC | #2
On Mon, 1 Aug 2016, Chris Wilson wrote:

> On Fri, Jul 29, 2016 at 10:34:36AM +0100, Peter Antoine wrote:
>> If one of the previous tests fails then the following tests fail.
>> This patch means that the following tests do not fail when the previous
>> test fails (for some cases).
>
> The problem is just gem_mocs_settings hasn't split its tests up into
> subtests.
Chris,

Can you expand? The tests are at the minimal size for sensible results (I 
think). The problem is opening the driver for master when the test fails 
then the following tests will fail as master is not closed.

Is there a mechanism in the igt framework for doing this close on 
failure/skip?

If I move the master open code in the fixtures will this get called on all 
exit cases?

Peter.

> -Chris
>
>

--
    Peter Antoine (Android Graphics Driver Software Engineer)
    ---------------------------------------------------------------------
    Intel Corporation (UK) Limited
    Registered No. 1134945 (England)
    Registered Office: Pipers Way, Swindon SN3 1RJ
    VAT No: 860 2173 47
Daniel Vetter Aug. 2, 2016, 2:37 p.m. UTC | #3
On Mon, Aug 01, 2016 at 10:33:17AM +0100, Peter Antoine wrote:
> On Mon, 1 Aug 2016, Chris Wilson wrote:
> 
> > On Fri, Jul 29, 2016 at 10:34:36AM +0100, Peter Antoine wrote:
> > > If one of the previous tests fails then the following tests fail.
> > > This patch means that the following tests do not fail when the previous
> > > test fails (for some cases).
> > 
> > The problem is just gem_mocs_settings hasn't split its tests up into
> > subtests.
> Chris,
> 
> Can you expand? The tests are at the minimal size for sensible results (I
> think). The problem is opening the driver for master when the test fails
> then the following tests will fail as master is not closed.
> 
> Is there a mechanism in the igt framework for doing this close on
> failure/skip?
> 
> If I move the master open code in the fixtures will this get called on all
> exit cases?

For a real test runner you need to run each individual subtest separate
(to avoid contamination). Then process exit will take care of any cleanup
needed with file descriptors. For anything else there's exit handlers, but
they're not 100% reliable

Adding hacks to make subtest runs differently isn't really how igt tests
are meant to be. Hence I concure on Chris' objection here.
-Daniel
Peter Antoine Aug. 2, 2016, 2:47 p.m. UTC | #4
They do run separately. It's just that when one fails it does not close the fd and the next cannot open it as master.
Is there a mechanism for closing the fd (or any other tidy up on close.failure).

As the test runner implements a psudo exception handler it should really have a "final"/"skip" handler so that the tidyup can be done cleanly.

That was the question that was asked and not answered.

Peter. 

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, August 2, 2016 3:37 PM
To: Antoine, Peter <peter.antoine@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [I-G-T 3/3] igt/gem_mocs_settings: Reduce the amount of cascading failures

On Mon, Aug 01, 2016 at 10:33:17AM +0100, Peter Antoine wrote:
> On Mon, 1 Aug 2016, Chris Wilson wrote:
> 
> > On Fri, Jul 29, 2016 at 10:34:36AM +0100, Peter Antoine wrote:
> > > If one of the previous tests fails then the following tests fail.
> > > This patch means that the following tests do not fail when the 
> > > previous test fails (for some cases).
> > 
> > The problem is just gem_mocs_settings hasn't split its tests up into 
> > subtests.
> Chris,
> 
> Can you expand? The tests are at the minimal size for sensible results 
> (I think). The problem is opening the driver for master when the test 
> fails then the following tests will fail as master is not closed.
> 
> Is there a mechanism in the igt framework for doing this close on 
> failure/skip?
> 
> If I move the master open code in the fixtures will this get called on 
> all exit cases?

For a real test runner you need to run each individual subtest separate (to avoid contamination). Then process exit will take care of any cleanup needed with file descriptors. For anything else there's exit handlers, but they're not 100% reliable

Adding hacks to make subtest runs differently isn't really how igt tests are meant to be. Hence I concure on Chris' objection here.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Daniel Vetter Aug. 3, 2016, 7:36 a.m. UTC | #5
On Tue, Aug 02, 2016 at 02:47:13PM +0000, Antoine, Peter wrote:
> They do run separately. It's just that when one fails it does not close the fd and the next cannot open it as master.
> Is there a mechanism for closing the fd (or any other tidy up on close.failure).
> 
> As the test runner implements a psudo exception handler it should really
> have a "final"/"skip" handler so that the tidyup can be done cleanly.
> 
> That was the question that was asked and not answered.

#define igt_finally igt_fixture

At least that's the best thing we came up with when last discussing this.
And I did answer your question by claiming that it's not really a problem
when you run tests (in CI) like they should be run. And for manual testing
I'm not sure it's all that valuable really. Hence why I didn't end up
merging the above with a bit of documentation to explain it.
-Daniel

> 
> Peter. 
> 
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, August 2, 2016 3:37 PM
> To: Antoine, Peter <peter.antoine@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [I-G-T 3/3] igt/gem_mocs_settings: Reduce the amount of cascading failures
> 
> On Mon, Aug 01, 2016 at 10:33:17AM +0100, Peter Antoine wrote:
> > On Mon, 1 Aug 2016, Chris Wilson wrote:
> > 
> > > On Fri, Jul 29, 2016 at 10:34:36AM +0100, Peter Antoine wrote:
> > > > If one of the previous tests fails then the following tests fail.
> > > > This patch means that the following tests do not fail when the 
> > > > previous test fails (for some cases).
> > > 
> > > The problem is just gem_mocs_settings hasn't split its tests up into 
> > > subtests.
> > Chris,
> > 
> > Can you expand? The tests are at the minimal size for sensible results 
> > (I think). The problem is opening the driver for master when the test 
> > fails then the following tests will fail as master is not closed.
> > 
> > Is there a mechanism in the igt framework for doing this close on 
> > failure/skip?
> > 
> > If I move the master open code in the fixtures will this get called on 
> > all exit cases?
> 
> For a real test runner you need to run each individual subtest separate (to avoid contamination). Then process exit will take care of any cleanup needed with file descriptors. For anything else there's exit handlers, but they're not 100% reliable
> 
> Adding hacks to make subtest runs differently isn't really how igt tests are meant to be. Hence I concure on Chris' objection here.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Peter Antoine Aug. 3, 2016, 8:49 a.m. UTC | #6
Drop this patch.

Peter.

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Wednesday, August 3, 2016 8:37 AM
To: Antoine, Peter <peter.antoine@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [I-G-T 3/3] igt/gem_mocs_settings: Reduce the amount of cascading failures

On Tue, Aug 02, 2016 at 02:47:13PM +0000, Antoine, Peter wrote:
> They do run separately. It's just that when one fails it does not close the fd and the next cannot open it as master.
> Is there a mechanism for closing the fd (or any other tidy up on close.failure).
> 
> As the test runner implements a psudo exception handler it should 
> really have a "final"/"skip" handler so that the tidyup can be done cleanly.
> 
> That was the question that was asked and not answered.

#define igt_finally igt_fixture

At least that's the best thing we came up with when last discussing this.
And I did answer your question by claiming that it's not really a problem when you run tests (in CI) like they should be run. And for manual testing I'm not sure it's all that valuable really. Hence why I didn't end up merging the above with a bit of documentation to explain it.
-Daniel

> 
> Peter. 
> 
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of 
> Daniel Vetter
> Sent: Tuesday, August 2, 2016 3:37 PM
> To: Antoine, Peter <peter.antoine@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>; 
> intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [I-G-T 3/3] igt/gem_mocs_settings: Reduce the 
> amount of cascading failures
> 
> On Mon, Aug 01, 2016 at 10:33:17AM +0100, Peter Antoine wrote:
> > On Mon, 1 Aug 2016, Chris Wilson wrote:
> > 
> > > On Fri, Jul 29, 2016 at 10:34:36AM +0100, Peter Antoine wrote:
> > > > If one of the previous tests fails then the following tests fail.
> > > > This patch means that the following tests do not fail when the 
> > > > previous test fails (for some cases).
> > > 
> > > The problem is just gem_mocs_settings hasn't split its tests up 
> > > into subtests.
> > Chris,
> > 
> > Can you expand? The tests are at the minimal size for sensible 
> > results (I think). The problem is opening the driver for master when 
> > the test fails then the following tests will fail as master is not closed.
> > 
> > Is there a mechanism in the igt framework for doing this close on 
> > failure/skip?
> > 
> > If I move the master open code in the fixtures will this get called 
> > on all exit cases?
> 
> For a real test runner you need to run each individual subtest 
> separate (to avoid contamination). Then process exit will take care of 
> any cleanup needed with file descriptors. For anything else there's 
> exit handlers, but they're not 100% reliable
> 
> Adding hacks to make subtest runs differently isn't really how igt tests are meant to be. Hence I concure on Chris' objection here.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
diff mbox

Patch

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 1cb9398..cc3ce26 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -629,6 +629,41 @@  void igt_cleanup_aperture_trashers(void)
 #define SQUELCH ">/dev/null 2>&1"
 
 /**
+ * igt_system_suspend_autoresume_no_assert:
+ *
+ * Execute a system suspend-to-mem cycle and automatically wake up again using
+ * the firmware's resume timer.
+ *
+ * This is very handy for implementing any kind of suspend/resume test.
+ *
+ * This version does not cause an "exception" as the test will need to tidy-up
+ * to allow for subsequent tests to run.
+ */
+bool igt_system_suspend_autoresume_no_assert(void)
+{
+	/* FIXME: Simulation doesn't like suspend/resume, and not even a lighter
+	 * approach using /sys/power/pm_test to just test our driver's callbacks
+	 * seems to fare better. We need to investigate what's going on. */
+	if (igt_run_in_simulation()) {
+		igt_debug("autoresume cannot be used in simulation\n");
+		return false;
+
+	} else if (system("rtcwake -n -s 15 -m mem" SQUELCH) != 0) {
+		igt_debug("rtcwake -n -s 15 -m mem is not supported\n");
+		return false;
+
+	} else if (system("rtcwake -s 15 -m mem") != 0) {
+		igt_debug(
+		     "This failure means that something is wrong with the "
+		     "rtcwake tool or how your distro is set up. This is not "
+		     "a i915.ko or i-g-t bug.\n");
+		return false;
+	}
+
+	return true;
+}
+
+/**
  * igt_system_suspend_autoresume:
  *
  * Execute a system suspend-to-mem cycle and automatically wake up again using
@@ -653,6 +688,38 @@  void igt_system_suspend_autoresume(void)
 }
 
 /**
+ * igt_system_hibernate_autoresume_no_assert:
+ *
+ * Execute a system suspend-to-disk cycle and automatically wake up again using
+ * the firmware's resume timer.
+ *
+ * This is very handy for implementing any kind of hibernate/resume test.
+ *
+ * This version does not cause an "exception" as the test will need to tidy-up
+ * to allow for subsequent tests to run.
+ */
+bool igt_system_hibernate_autoresume_no_assert(void)
+{
+	if (igt_run_in_simulation()) {
+		igt_debug("autoresume cannot be used in simulation\n");
+		return false;
+
+	} else if (system("rtcwake -n -s 30 -m disk" SQUELCH) != 0) {
+		igt_debug("rtcwake -n -s 30 -m disk is not supported\n");
+		return false;
+
+	} else if (system("rtcwake -s 30 -m disk") != 0) {
+		igt_debug(
+		     "This failure means that something is wrong with the "
+		     "rtcwake tool or how your distro is set up. This is not "
+		     "a i915.ko or i-g-t bug.\n");
+		return false;
+	}
+
+	return true;
+}
+
+/**
  * igt_system_hibernate_autoresume:
  *
  * Execute a system suspend-to-disk cycle and automatically wake up again using
diff --git a/lib/igt_aux.h b/lib/igt_aux.h
index be0d2d6..483c444 100644
--- a/lib/igt_aux.h
+++ b/lib/igt_aux.h
@@ -103,7 +103,9 @@  void igt_cleanup_aperture_trashers(void);
 
 /* suspend/hibernate and auto-resume system */
 void igt_system_suspend_autoresume(void);
+bool igt_system_suspend_autoresume_no_assert(void);
 void igt_system_hibernate_autoresume(void);
+bool igt_system_hibernate_autoresume_no_assert(void);
 
 /* dropping priviledges */
 void igt_drop_root(void);
diff --git a/tests/gem_mocs_settings.c b/tests/gem_mocs_settings.c
index 66d02d9..1da7473 100644
--- a/tests/gem_mocs_settings.c
+++ b/tests/gem_mocs_settings.c
@@ -373,6 +373,27 @@  static void test_mocs_values(int fd)
 	}
 }
 
+static void action_test(int fd, unsigned int mode)
+{
+	switch (mode) {
+	case RESET:
+			igt_force_gpu_reset();
+			break;
+	case SUSPEND:
+			if (!igt_system_suspend_autoresume_no_assert()) {
+				close(fd);
+				igt_fail(1);
+			}
+			break;
+	case HIBERNATE:
+			if (!igt_system_hibernate_autoresume_no_assert()) {
+				close(fd);
+				igt_fail(1);
+			}
+			break;
+	}
+}
+
 static void default_context_tests(unsigned mode)
 {
 	int fd = drm_open_driver_master(DRIVER_INTEL);
@@ -380,12 +401,7 @@  static void default_context_tests(unsigned mode)
 	igt_debug("Testing Non/Default Context Engines\n");
 	test_mocs_values(fd);
 
-	switch (mode) {
-	case NONE:	break;
-	case RESET:	igt_force_gpu_reset();	break;
-	case SUSPEND:	igt_system_suspend_autoresume(); break;
-	case HIBERNATE:	igt_system_hibernate_autoresume(); break;
-	}
+	action_test(fd, mode);
 
 	test_mocs_values(fd);
 	close(fd);
@@ -419,12 +435,7 @@  static void default_dirty_tests(unsigned mode)
 				engine);
 	}
 
-	switch (mode) {
-	case NONE:	break;
-	case RESET:	igt_force_gpu_reset();	break;
-	case SUSPEND:	igt_system_suspend_autoresume(); break;
-	case HIBERNATE:	igt_system_hibernate_autoresume(); break;
-	}
+	action_test(fd, mode);
 
 	close(fd);
 
@@ -442,12 +453,7 @@  static void context_save_restore_test(unsigned mode)
 	check_control_registers(fd, I915_EXEC_RENDER, ctx_id, false);
 	check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false);
 
-	switch (mode) {
-	case NONE:	break;
-	case RESET:	igt_force_gpu_reset();	break;
-	case SUSPEND:	igt_system_suspend_autoresume(); break;
-	case HIBERNATE:	igt_system_hibernate_autoresume(); break;
-	}
+	action_test(fd, mode);
 
 	check_control_registers(fd, I915_EXEC_RENDER, ctx_id, false);
 	check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false);
@@ -485,12 +491,7 @@  static void context_dirty_test(unsigned mode)
 	check_control_registers(fd, I915_EXEC_RENDER, ctx_id, true);
 	check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, true);
 
-	switch (mode) {
-	case NONE:	break;
-	case RESET:	igt_force_gpu_reset();	break;
-	case SUSPEND:	igt_system_suspend_autoresume(); break;
-	case HIBERNATE:	igt_system_hibernate_autoresume(); break;
-	}
+	action_test(fd, mode);
 
 	check_control_registers(fd, I915_EXEC_RENDER, ctx_id, true);
 	check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, true);
@@ -562,8 +563,11 @@  static void context_rc6_test(void)
 	check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false);
 
 	res_ms = read_rc6_residency();
-	sleep(3);
-	igt_assert_neq(res_ms, read_rc6_residency());
+	sleep(6);
+	if (res_ms == read_rc6_residency()) {
+		close(fd);
+		igt_assert_neq(res_ms, read_rc6_residency());
+	}
 
 	check_control_registers(fd, I915_EXEC_RENDER, ctx_id, false);
 	check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false);