[PATCH igt] tests/kms_frontbuffer_tracking: skip expected failures

[PATCH igt] tests/kms_frontbuffer_tracking: skip expected failures

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We want to avoid a situation where QA/PRTS stops reporting regressions
because there are way too many subtests failing. So after this commit
we will just SKIP all the expected failures, and we'll start always
failing  a subtest called "no-expected-failures".

With this approach, I'm hoping we'll have a bug report for the
"no-expected-failures subcase fails" bug, then every other single
failure will also get its bug report.

One of the side effects of this commit is that the time it takes to
run everything is greatly improved, because many of the previous
failures took 5-9 seconds to run, and now they take 0s. Failures are
often much slower than successes because they hit the timeouts waiting
for the features to be enabled.

Cc: Thomas Wood <thomas.wood@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Wendy Wang <wendy.wang@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 85 ++++++++++++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 11 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 70007eb..634bf0c 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -62,6 +62,11 @@ IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and "
  * the Kernel side. We don't expose this set of tests by default because (i)
  * they take a long time to test; and (ii) if the feature tests work, then it's
  * very likely that the nop tests will also work.
+ *
+ * In order to avoid a situation where QA just doesn't report any bugs due to
+ * "too many failures", we maintain a list of expected failures that we SKIP. So
+ * before you enable any feature by default, please make sure that we're not
+ * skipping any expected failures! Please check the expected_failure() function.
  */
 struct test_mode {
 	/* Are we going to enable just one monitor, or are we going to setup a
@@ -273,6 +278,8 @@ struct {
 	.stop = true,
 };
 
+int skipped_expected_failures = 0;
+
 static drmModeModeInfoPtr get_connector_smallest_mode(drmModeConnectorPtr c)
 {
 	int i;
@@ -1462,7 +1469,53 @@ static void enable_features_for_test(const struct test_mode *t)
 		psr_enable();
 }
 
-static void check_test_requirements(const struct test_mode *t)
+/* Please read the big comment at the top of this file. */
+static bool expected_failure(const struct test_mode *t, const char *test_name)
+{
+	if (t->feature & FEATURE_FBC) {
+		/* The fix for this one is still under discussion, but will
+		 * probably require some frontbuffer tracking calls inside the
+		 * dirtyfb ioctl, and a change so igt_draw properly calls the
+		 * ioctl. */
+		if (t->method == IGT_DRAW_MMAP_WC)
+			goto fail;
+		/* We're seeing corruption in some cases where crtc->y is
+		 * non-zero. Needs more investication. */
+		if (t->method == IGT_DRAW_MMAP_GTT && t->fbs == FBS_MULTI)
+			goto fail;
+		/* Apparently, we're not disabling FBC when there's a fullscreen
+		 * sprite plane. */
+		if (strcmp(test_name, "fullscreen") == 0)
+			goto fail;
+		/* This is currently failing because it uses mmap-wc. When we
+		 * fix the mmap-wc failures we can test this again. */
+		if (strcmp(test_name, "multidraw") == 0)
+			goto fail;
+	}
+
+	/* For the PSR cases, I didn't really investigate the causes of the
+	 * failures. */
+	if (t->feature & FEATURE_PSR) {
+		if (strcmp(test_name, "multidraw") == 0)
+			goto fail;
+		if (t->fbs == FBS_MULTI)
+			goto fail;
+		if (t->method == IGT_DRAW_MMAP_GTT &&
+		    strcmp(test_name, "draw") == 0) {
+			goto fail;
+		}
+	}
+
+	return false;
+
+fail:
+	igt_debug("Skipping expected failure.\n");
+	skipped_expected_failures++;
+	return true;
+}
+
+static void check_test_requirements(const struct test_mode *t,
+				    const char *test_name)
 {
 	if (t->pipes == PIPE_DUAL)
 		igt_require_f(scnd_mode_params.connector_id,
@@ -1484,6 +1537,9 @@ static void check_test_requirements(const struct test_mode *t)
 
 	if (opt.only_pipes != PIPE_COUNT)
 		igt_require(t->pipes == opt.only_pipes);
+
+	igt_require_f(!expected_failure(t, test_name),
+		      "This test is expected to fail\n");
 }
 
 static void set_crtc_fbs(const struct test_mode *t)
@@ -1528,9 +1584,10 @@ static void set_crtc_fbs(const struct test_mode *t)
 }
 
 static void prepare_subtest(const struct test_mode *t,
-			    struct draw_pattern_info *pattern)
+			    struct draw_pattern_info *pattern,
+			    const char *test_name)
 {
-	check_test_requirements(t);
+	check_test_requirements(t, test_name);
 
 	stop_busy_thread();
 
@@ -1581,7 +1638,7 @@ static void prepare_subtest(const struct test_mode *t,
  */
 static void rte_subtest(const struct test_mode *t)
 {
-	check_test_requirements(t);
+	check_test_requirements(t, "rte");
 
 	disable_features();
 	set_crtc_fbs(t);
@@ -1661,7 +1718,7 @@ static void draw_subtest(const struct test_mode *t)
 		igt_assert(false);
 	}
 
-	prepare_subtest(t, pattern);
+	prepare_subtest(t, pattern, "draw");
 	target = pick_target(t, params);
 
 	for (r = 0; r < pattern->n_rects; r++) {
@@ -1713,7 +1770,7 @@ static void multidraw_subtest(const struct test_mode *t)
 		igt_assert(false);
 	}
 
-	prepare_subtest(t, pattern);
+	prepare_subtest(t, pattern, "multidraw");
 	target = pick_target(t, params);
 
 	for (m = 0; m < IGT_DRAW_METHOD_COUNT; m++) {
@@ -1788,7 +1845,7 @@ static void flip_subtest(const struct test_mode *t)
 		igt_assert(false);
 	}
 
-	prepare_subtest(t, pattern);
+	prepare_subtest(t, pattern, "flip");
 
 	igt_create_fb(drm.fd, params->fb.fb->width, params->fb.fb->height,
 		      DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_X_TILED, &fb2);
@@ -1835,7 +1892,7 @@ static void move_subtest(const struct test_mode *t)
 	struct draw_pattern_info *pattern = &pattern3;
 	bool repeat = false;
 
-	prepare_subtest(t, pattern);
+	prepare_subtest(t, pattern, "move");
 
 	/* Just paint the right color since we start at 0x0. */
 	draw_rect(pattern, pick_target(t, params), t->method, 0);
@@ -1898,7 +1955,7 @@ static void onoff_subtest(const struct test_mode *t)
 	struct modeset_params *params = pick_params(t);
 	struct draw_pattern_info *pattern = &pattern3;
 
-	prepare_subtest(t, pattern);
+	prepare_subtest(t, pattern, "onoff");
 
 	/* Just paint the right color since we start at 0x0. */
 	draw_rect(pattern, pick_target(t, params), t->method, 0);
@@ -1977,7 +2034,7 @@ static void fullscreen_plane_subtest(const struct test_mode *t)
 	int assertions;
 	int rc;
 
-	prepare_subtest(t, pattern);
+	prepare_subtest(t, pattern, "fullscreen");
 
 	rect = pattern->get_rect(&params->fb, 0);
 	igt_create_fb(drm.fd, rect.w, rect.h, DRM_FORMAT_XRGB8888,
@@ -2041,7 +2098,7 @@ static void modesetfrombusy_subtest(const struct test_mode *t)
 	struct modeset_params *params = pick_params(t);
 	struct igt_fb fb2;
 
-	prepare_subtest(t, pattern);
+	prepare_subtest(t, pattern, "modesetfrombusy");
 
 	igt_create_fb(drm.fd, params->fb.fb->width, params->fb.fb->height,
 		      DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_X_TILED, &fb2);
@@ -2361,6 +2418,11 @@ int main(int argc, char *argv[])
 			modesetfrombusy_subtest(&t);
 	TEST_MODE_ITER_END
 
+	/* Change the assertion below to 1 when the expected_failure() function
+	 * returns false for every case. */
+	igt_subtest("no-expected-failures")
+		igt_assert_f(0, "There are expected failures we skipped.\n");
+
 	/*
 	 * TODO: ideas for subtests:
 	 * - Add a new enum to struct test_mode that allows us to specify the
@@ -2370,5 +2432,6 @@ int main(int argc, char *argv[])
 	igt_fixture
 		teardown_environment();
 
+	igt_info("Skipped expected failures: %d\n", skipped_expected_failures);
 	igt_exit();
 }
-- 
2.1.4

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

Re: [PATCH igt] tests/kms_frontbuffer_tracking: skip expected failures

[Daniel Vetter]

On Tue, Jun 23, 2015 at 12:08:33PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We want to avoid a situation where QA/PRTS stops reporting regressions
> because there are way too many subtests failing. So after this commit
> we will just SKIP all the expected failures, and we'll start always
> failing  a subtest called "no-expected-failures".
> 
> With this approach, I'm hoping we'll have a bug report for the
> "no-expected-failures subcase fails" bug, then every other single
> failure will also get its bug report.
> 
> One of the side effects of this commit is that the time it takes to
> run everything is greatly improved, because many of the previous
> failures took 5-9 seconds to run, and now they take 0s. Failures are
> often much slower than successes because they hit the timeouts waiting
> for the features to be enabled.
> 
> Cc: Thomas Wood <thomas.wood@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Wendy Wang <wendy.wang@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Nack. Skip is for hw or kernel support not available, not marking expected
failures. If we want to do that there's ideas floating of sharing expected
failures and blacklists at the piglit level, maybe together with some
"fast testcases" list.

We have a new QA and failing to sensibly manage blacklists and failing to
catch regressions isn't accepted any more. If you don't trust them yet
(understandable) please raise this with Yann&Christophe to make sure they
deliver what you need.
-Daniel

> ---
>  tests/kms_frontbuffer_tracking.c | 85 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 74 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index 70007eb..634bf0c 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -62,6 +62,11 @@ IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and "
>   * the Kernel side. We don't expose this set of tests by default because (i)
>   * they take a long time to test; and (ii) if the feature tests work, then it's
>   * very likely that the nop tests will also work.
> + *
> + * In order to avoid a situation where QA just doesn't report any bugs due to
> + * "too many failures", we maintain a list of expected failures that we SKIP. So
> + * before you enable any feature by default, please make sure that we're not
> + * skipping any expected failures! Please check the expected_failure() function.
>   */
>  struct test_mode {
>  	/* Are we going to enable just one monitor, or are we going to setup a
> @@ -273,6 +278,8 @@ struct {
>  	.stop = true,
>  };
>  
> +int skipped_expected_failures = 0;
> +
>  static drmModeModeInfoPtr get_connector_smallest_mode(drmModeConnectorPtr c)
>  {
>  	int i;
> @@ -1462,7 +1469,53 @@ static void enable_features_for_test(const struct test_mode *t)
>  		psr_enable();
>  }
>  
> -static void check_test_requirements(const struct test_mode *t)
> +/* Please read the big comment at the top of this file. */
> +static bool expected_failure(const struct test_mode *t, const char *test_name)
> +{
> +	if (t->feature & FEATURE_FBC) {
> +		/* The fix for this one is still under discussion, but will
> +		 * probably require some frontbuffer tracking calls inside the
> +		 * dirtyfb ioctl, and a change so igt_draw properly calls the
> +		 * ioctl. */
> +		if (t->method == IGT_DRAW_MMAP_WC)
> +			goto fail;
> +		/* We're seeing corruption in some cases where crtc->y is
> +		 * non-zero. Needs more investication. */
> +		if (t->method == IGT_DRAW_MMAP_GTT && t->fbs == FBS_MULTI)
> +			goto fail;
> +		/* Apparently, we're not disabling FBC when there's a fullscreen
> +		 * sprite plane. */
> +		if (strcmp(test_name, "fullscreen") == 0)
> +			goto fail;
> +		/* This is currently failing because it uses mmap-wc. When we
> +		 * fix the mmap-wc failures we can test this again. */
> +		if (strcmp(test_name, "multidraw") == 0)
> +			goto fail;
> +	}
> +
> +	/* For the PSR cases, I didn't really investigate the causes of the
> +	 * failures. */
> +	if (t->feature & FEATURE_PSR) {
> +		if (strcmp(test_name, "multidraw") == 0)
> +			goto fail;
> +		if (t->fbs == FBS_MULTI)
> +			goto fail;
> +		if (t->method == IGT_DRAW_MMAP_GTT &&
> +		    strcmp(test_name, "draw") == 0) {
> +			goto fail;
> +		}
> +	}
> +
> +	return false;
> +
> +fail:
> +	igt_debug("Skipping expected failure.\n");
> +	skipped_expected_failures++;
> +	return true;
> +}
> +
> +static void check_test_requirements(const struct test_mode *t,
> +				    const char *test_name)
>  {
>  	if (t->pipes == PIPE_DUAL)
>  		igt_require_f(scnd_mode_params.connector_id,
> @@ -1484,6 +1537,9 @@ static void check_test_requirements(const struct test_mode *t)
>  
>  	if (opt.only_pipes != PIPE_COUNT)
>  		igt_require(t->pipes == opt.only_pipes);
> +
> +	igt_require_f(!expected_failure(t, test_name),
> +		      "This test is expected to fail\n");
>  }
>  
>  static void set_crtc_fbs(const struct test_mode *t)
> @@ -1528,9 +1584,10 @@ static void set_crtc_fbs(const struct test_mode *t)
>  }
>  
>  static void prepare_subtest(const struct test_mode *t,
> -			    struct draw_pattern_info *pattern)
> +			    struct draw_pattern_info *pattern,
> +			    const char *test_name)
>  {
> -	check_test_requirements(t);
> +	check_test_requirements(t, test_name);
>  
>  	stop_busy_thread();
>  
> @@ -1581,7 +1638,7 @@ static void prepare_subtest(const struct test_mode *t,
>   */
>  static void rte_subtest(const struct test_mode *t)
>  {
> -	check_test_requirements(t);
> +	check_test_requirements(t, "rte");
>  
>  	disable_features();
>  	set_crtc_fbs(t);
> @@ -1661,7 +1718,7 @@ static void draw_subtest(const struct test_mode *t)
>  		igt_assert(false);
>  	}
>  
> -	prepare_subtest(t, pattern);
> +	prepare_subtest(t, pattern, "draw");
>  	target = pick_target(t, params);
>  
>  	for (r = 0; r < pattern->n_rects; r++) {
> @@ -1713,7 +1770,7 @@ static void multidraw_subtest(const struct test_mode *t)
>  		igt_assert(false);
>  	}
>  
> -	prepare_subtest(t, pattern);
> +	prepare_subtest(t, pattern, "multidraw");
>  	target = pick_target(t, params);
>  
>  	for (m = 0; m < IGT_DRAW_METHOD_COUNT; m++) {
> @@ -1788,7 +1845,7 @@ static void flip_subtest(const struct test_mode *t)
>  		igt_assert(false);
>  	}
>  
> -	prepare_subtest(t, pattern);
> +	prepare_subtest(t, pattern, "flip");
>  
>  	igt_create_fb(drm.fd, params->fb.fb->width, params->fb.fb->height,
>  		      DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_X_TILED, &fb2);
> @@ -1835,7 +1892,7 @@ static void move_subtest(const struct test_mode *t)
>  	struct draw_pattern_info *pattern = &pattern3;
>  	bool repeat = false;
>  
> -	prepare_subtest(t, pattern);
> +	prepare_subtest(t, pattern, "move");
>  
>  	/* Just paint the right color since we start at 0x0. */
>  	draw_rect(pattern, pick_target(t, params), t->method, 0);
> @@ -1898,7 +1955,7 @@ static void onoff_subtest(const struct test_mode *t)
>  	struct modeset_params *params = pick_params(t);
>  	struct draw_pattern_info *pattern = &pattern3;
>  
> -	prepare_subtest(t, pattern);
> +	prepare_subtest(t, pattern, "onoff");
>  
>  	/* Just paint the right color since we start at 0x0. */
>  	draw_rect(pattern, pick_target(t, params), t->method, 0);
> @@ -1977,7 +2034,7 @@ static void fullscreen_plane_subtest(const struct test_mode *t)
>  	int assertions;
>  	int rc;
>  
> -	prepare_subtest(t, pattern);
> +	prepare_subtest(t, pattern, "fullscreen");
>  
>  	rect = pattern->get_rect(&params->fb, 0);
>  	igt_create_fb(drm.fd, rect.w, rect.h, DRM_FORMAT_XRGB8888,
> @@ -2041,7 +2098,7 @@ static void modesetfrombusy_subtest(const struct test_mode *t)
>  	struct modeset_params *params = pick_params(t);
>  	struct igt_fb fb2;
>  
> -	prepare_subtest(t, pattern);
> +	prepare_subtest(t, pattern, "modesetfrombusy");
>  
>  	igt_create_fb(drm.fd, params->fb.fb->width, params->fb.fb->height,
>  		      DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_X_TILED, &fb2);
> @@ -2361,6 +2418,11 @@ int main(int argc, char *argv[])
>  			modesetfrombusy_subtest(&t);
>  	TEST_MODE_ITER_END
>  
> +	/* Change the assertion below to 1 when the expected_failure() function
> +	 * returns false for every case. */
> +	igt_subtest("no-expected-failures")
> +		igt_assert_f(0, "There are expected failures we skipped.\n");
> +
>  	/*
>  	 * TODO: ideas for subtests:
>  	 * - Add a new enum to struct test_mode that allows us to specify the
> @@ -2370,5 +2432,6 @@ int main(int argc, char *argv[])
>  	igt_fixture
>  		teardown_environment();
>  
> +	igt_info("Skipped expected failures: %d\n", skipped_expected_failures);
>  	igt_exit();
>  }
> -- 
> 2.1.4
> 

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