intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set
@ 2016-04-22 14:41 Tvrtko Ursulin
  2016-04-22 14:41 ` [PATCH i-g-t 2/2] kms_pipe_crc_basic: Rename test to kms_pipe_crc Tvrtko Ursulin
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Tvrtko Ursulin @ 2016-04-22 14:41 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I would argue it is enough to test one pipe in the BAT set.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 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))
 			test_read_crc(&data, i, 0);
 
-		igt_subtest_f("read-crc-pipe-%c-frame-sequence", 'A'+i)
+		igt_subtest_f("%sread-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
 			test_read_crc(&data, i, TEST_SEQUENCE);
 
-		igt_subtest_f("nonblocking-crc-pipe-%c", 'A'+i)
+		igt_subtest_f("%snonblocking-crc-pipe-%c", test_prefix(i), pipe_name(i))
 			test_read_crc(&data, i, TEST_NONBLOCK);
 
-		igt_subtest_f("nonblocking-crc-pipe-%c-frame-sequence", 'A'+i)
+		igt_subtest_f("%snonblocking-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
 			test_read_crc(&data, i, TEST_SEQUENCE | TEST_NONBLOCK);
 
-		igt_subtest_f("suspend-read-crc-pipe-%c", 'A'+i) {
+		igt_subtest_f("%ssuspend-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
 			igt_skip_on(i >= data.display.n_pipes);
 			igt_system_suspend_autoresume();
 
 			test_read_crc(&data, i, 0);
 		}
 
-		igt_subtest_f("hang-read-crc-pipe-%c", 'A'+i) {
+		igt_subtest_f("%shang-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
 			igt_hang_ring_t hang =
 				igt_hang_ring(data.drm_fd, I915_EXEC_RENDER);
 			test_read_crc(&data, i, 0);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH i-g-t 2/2] kms_pipe_crc_basic: Rename test to kms_pipe_crc
  2016-04-22 14:41 [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set Tvrtko Ursulin
@ 2016-04-22 14:41 ` Tvrtko Ursulin
  2016-04-22 15:11 ` [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set Chris Wilson
  2016-04-25  7:00 ` Jani Nikula
  2 siblings, 0 replies; 8+ messages in thread
From: Tvrtko Ursulin @ 2016-04-22 14:41 UTC (permalink / raw)
  To: Intel-gfx

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=a, Size: 15300 bytes --]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Previous patch has marked individual subtests as basic so now
rename the test binary name accordingly.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/Makefile.sources     |   2 +-
 tests/kms_pipe_crc.c       | 246 +++++++++++++++++++++++++++++++++++++++++++++
 tests/kms_pipe_crc_basic.c | 246 ---------------------------------------------
 3 files changed, 247 insertions(+), 247 deletions(-)
 create mode 100644 tests/kms_pipe_crc.c
 delete mode 100644 tests/kms_pipe_crc_basic.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index abcf32558c66..adfec2990af3 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -92,7 +92,7 @@ TESTS_progs_M = \
 	kms_mmio_vs_cs_flip \
 	kms_pipe_b_c_ivb \
 	kms_pipe_color \
-	kms_pipe_crc_basic \
+	kms_pipe_crc \
 	kms_plane \
 	kms_psr_sink_crc \
 	kms_render \
diff --git a/tests/kms_pipe_crc.c b/tests/kms_pipe_crc.c
new file mode 100644
index 000000000000..291775934758
--- /dev/null
+++ b/tests/kms_pipe_crc.c
@@ -0,0 +1,246 @@
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include "igt.h"
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+
+
+typedef struct {
+	int drm_fd;
+	igt_display_t display;
+	struct igt_fb fb;
+} data_t;
+
+static struct {
+	double r, g, b;
+	igt_crc_t crc;
+} colors[2] = {
+	{ .r = 0.0, .g = 1.0, .b = 0.0 },
+	{ .r = 0.0, .g = 1.0, .b = 1.0 },
+};
+
+static void test_bad_command(data_t *data, const char *cmd)
+{
+	FILE *ctl;
+	size_t written;
+
+	ctl = igt_debugfs_fopen("i915_display_crc_ctl", "r+");
+	written = fwrite(cmd, 1, strlen(cmd), ctl);
+	fflush(ctl);
+	igt_assert_eq(written, strlen(cmd));
+	igt_assert(ferror(ctl));
+	igt_assert_eq(errno, EINVAL);
+
+	fclose(ctl);
+}
+
+#define N_CRCS	3
+
+#define TEST_SEQUENCE (1<<0)
+#define TEST_NONBLOCK (1<<1)
+
+static int
+test_read_crc_for_output(data_t *data, int pipe, igt_output_t *output,
+			 unsigned flags)
+{
+	igt_display_t *display = &data->display;
+	igt_plane_t *primary;
+	drmModeModeInfo *mode;
+	igt_pipe_crc_t *pipe_crc;
+	igt_crc_t *crcs = NULL;
+	int c, j;
+
+	for (c = 0; c < ARRAY_SIZE(colors); c++) {
+		char *crc_str;
+		int n_crcs;
+
+		igt_output_set_pipe(output, pipe);
+		igt_display_commit(display);
+
+		if (!output->valid) {
+			igt_output_set_pipe(output, PIPE_ANY);
+			return 0;
+		}
+
+		igt_debug("Clearing the fb with color (%.02lf,%.02lf,%.02lf)\n",
+			  colors[c].r, colors[c].g, colors[c].b);
+
+		mode = igt_output_get_mode(output);
+		igt_create_color_fb(data->drm_fd,
+					mode->hdisplay, mode->vdisplay,
+					DRM_FORMAT_XRGB8888,
+					LOCAL_DRM_FORMAT_MOD_NONE,
+					colors[c].r,
+					colors[c].g,
+					colors[c].b,
+					&data->fb);
+
+		primary = igt_output_get_plane(output, 0);
+		igt_plane_set_fb(primary, &data->fb);
+
+		igt_display_commit(display);
+
+		if (flags & TEST_NONBLOCK)
+			pipe_crc = igt_pipe_crc_new_nonblock(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
+		else
+			pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
+
+		igt_pipe_crc_start(pipe_crc);
+
+		/* wait for N_CRCS vblanks and the corresponding N_CRCS CRCs */
+		if (flags & TEST_NONBLOCK) {
+			int i;
+
+			for (i = 0; i < N_CRCS; i++)
+				igt_wait_for_vblank(data->drm_fd, pipe);
+
+			n_crcs = igt_pipe_crc_get_crcs(pipe_crc, N_CRCS * 3, &crcs);
+			/* allow a one frame difference */
+			igt_assert_lte(n_crcs, N_CRCS + 1);
+			igt_assert_lte(N_CRCS, n_crcs + 1);
+		} else {
+			n_crcs = igt_pipe_crc_get_crcs(pipe_crc, N_CRCS, &crcs);
+			igt_assert_eq(n_crcs, N_CRCS);
+		}
+
+		igt_pipe_crc_stop(pipe_crc);
+
+		/*
+		 * save the CRC in colors so it can be compared to the CRC of
+		 * other fbs
+		 */
+		colors[c].crc = crcs[0];
+
+		crc_str = igt_crc_to_string(&crcs[0]);
+		igt_debug("CRC for this fb: %s\n", crc_str);
+		free(crc_str);
+
+		/* and ensure that they'are all equal, we haven't changed the fb */
+		for (j = 0; j < (n_crcs - 1); j++)
+			igt_assert_crc_equal(&crcs[j], &crcs[j + 1]);
+
+		if (flags & TEST_SEQUENCE)
+			for (j = 0; j < (n_crcs - 1); j++)
+				igt_assert_eq(crcs[j].frame + 1, crcs[j + 1].frame);
+
+		free(crcs);
+		igt_pipe_crc_free(pipe_crc);
+		igt_remove_fb(data->drm_fd, &data->fb);
+		igt_plane_set_fb(primary, NULL);
+
+		igt_output_set_pipe(output, PIPE_ANY);
+	}
+
+	return 1;
+}
+
+static void test_read_crc(data_t *data, int pipe, unsigned flags)
+{
+	igt_display_t *display = &data->display;
+	int valid_connectors = 0;
+	igt_output_t *output;
+
+	igt_skip_on(pipe >= data->display.n_pipes);
+
+	for_each_connected_output(display, output) {
+
+		igt_info("%s: Testing connector %s using pipe %s\n",
+			 igt_subtest_name(), igt_output_name(output),
+			 kmstest_pipe_name(pipe));
+
+		valid_connectors += test_read_crc_for_output(data, pipe, output, flags);
+	}
+
+	igt_require_f(valid_connectors, "No connector found for pipe %i\n", pipe);
+}
+
+data_t data = {0, };
+
+#define test_prefix(i) ((i) == 0 ? "basic-" : "")
+#define pipe_name(i) ((i) + 'A')
+
+igt_main
+{
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
+
+		igt_enable_connectors();
+
+		kmstest_set_vt_graphics_mode();
+
+		igt_require_pipe_crc();
+
+		igt_display_init(&data.display, data.drm_fd);
+	}
+
+	igt_subtest("basic-bad-pipe")
+		test_bad_command(&data, "pipe D none");
+
+	igt_subtest("basic-bad-source")
+		test_bad_command(&data, "pipe A foo");
+
+	igt_subtest("basic-bad-nb-words-1")
+		test_bad_command(&data, "pipe foo");
+
+	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("%sread-crc-pipe-%c", test_prefix(i), pipe_name(i))
+			test_read_crc(&data, i, 0);
+
+		igt_subtest_f("%sread-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
+			test_read_crc(&data, i, TEST_SEQUENCE);
+
+		igt_subtest_f("%snonblocking-crc-pipe-%c", test_prefix(i), pipe_name(i))
+			test_read_crc(&data, i, TEST_NONBLOCK);
+
+		igt_subtest_f("%snonblocking-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
+			test_read_crc(&data, i, TEST_SEQUENCE | TEST_NONBLOCK);
+
+		igt_subtest_f("%ssuspend-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
+			igt_skip_on(i >= data.display.n_pipes);
+			igt_system_suspend_autoresume();
+
+			test_read_crc(&data, i, 0);
+		}
+
+		igt_subtest_f("%shang-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
+			igt_hang_ring_t hang =
+				igt_hang_ring(data.drm_fd, I915_EXEC_RENDER);
+			test_read_crc(&data, i, 0);
+			igt_post_hang_ring(data.drm_fd, hang);
+			test_read_crc(&data, i, 0);
+		}
+	}
+
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
+}
diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
deleted file mode 100644
index 291775934758..000000000000
--- a/tests/kms_pipe_crc_basic.c
+++ /dev/null
@@ -1,246 +0,0 @@
-/*
- * Copyright © 2013 Intel Corporation
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the next
- * paragraph) shall be included in all copies or substantial portions of the
- * Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
- * IN THE SOFTWARE.
- *
- */
-
-#include "igt.h"
-#include <errno.h>
-#include <stdbool.h>
-#include <stdio.h>
-#include <string.h>
-
-
-typedef struct {
-	int drm_fd;
-	igt_display_t display;
-	struct igt_fb fb;
-} data_t;
-
-static struct {
-	double r, g, b;
-	igt_crc_t crc;
-} colors[2] = {
-	{ .r = 0.0, .g = 1.0, .b = 0.0 },
-	{ .r = 0.0, .g = 1.0, .b = 1.0 },
-};
-
-static void test_bad_command(data_t *data, const char *cmd)
-{
-	FILE *ctl;
-	size_t written;
-
-	ctl = igt_debugfs_fopen("i915_display_crc_ctl", "r+");
-	written = fwrite(cmd, 1, strlen(cmd), ctl);
-	fflush(ctl);
-	igt_assert_eq(written, strlen(cmd));
-	igt_assert(ferror(ctl));
-	igt_assert_eq(errno, EINVAL);
-
-	fclose(ctl);
-}
-
-#define N_CRCS	3
-
-#define TEST_SEQUENCE (1<<0)
-#define TEST_NONBLOCK (1<<1)
-
-static int
-test_read_crc_for_output(data_t *data, int pipe, igt_output_t *output,
-			 unsigned flags)
-{
-	igt_display_t *display = &data->display;
-	igt_plane_t *primary;
-	drmModeModeInfo *mode;
-	igt_pipe_crc_t *pipe_crc;
-	igt_crc_t *crcs = NULL;
-	int c, j;
-
-	for (c = 0; c < ARRAY_SIZE(colors); c++) {
-		char *crc_str;
-		int n_crcs;
-
-		igt_output_set_pipe(output, pipe);
-		igt_display_commit(display);
-
-		if (!output->valid) {
-			igt_output_set_pipe(output, PIPE_ANY);
-			return 0;
-		}
-
-		igt_debug("Clearing the fb with color (%.02lf,%.02lf,%.02lf)\n",
-			  colors[c].r, colors[c].g, colors[c].b);
-
-		mode = igt_output_get_mode(output);
-		igt_create_color_fb(data->drm_fd,
-					mode->hdisplay, mode->vdisplay,
-					DRM_FORMAT_XRGB8888,
-					LOCAL_DRM_FORMAT_MOD_NONE,
-					colors[c].r,
-					colors[c].g,
-					colors[c].b,
-					&data->fb);
-
-		primary = igt_output_get_plane(output, 0);
-		igt_plane_set_fb(primary, &data->fb);
-
-		igt_display_commit(display);
-
-		if (flags & TEST_NONBLOCK)
-			pipe_crc = igt_pipe_crc_new_nonblock(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
-		else
-			pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
-
-		igt_pipe_crc_start(pipe_crc);
-
-		/* wait for N_CRCS vblanks and the corresponding N_CRCS CRCs */
-		if (flags & TEST_NONBLOCK) {
-			int i;
-
-			for (i = 0; i < N_CRCS; i++)
-				igt_wait_for_vblank(data->drm_fd, pipe);
-
-			n_crcs = igt_pipe_crc_get_crcs(pipe_crc, N_CRCS * 3, &crcs);
-			/* allow a one frame difference */
-			igt_assert_lte(n_crcs, N_CRCS + 1);
-			igt_assert_lte(N_CRCS, n_crcs + 1);
-		} else {
-			n_crcs = igt_pipe_crc_get_crcs(pipe_crc, N_CRCS, &crcs);
-			igt_assert_eq(n_crcs, N_CRCS);
-		}
-
-		igt_pipe_crc_stop(pipe_crc);
-
-		/*
-		 * save the CRC in colors so it can be compared to the CRC of
-		 * other fbs
-		 */
-		colors[c].crc = crcs[0];
-
-		crc_str = igt_crc_to_string(&crcs[0]);
-		igt_debug("CRC for this fb: %s\n", crc_str);
-		free(crc_str);
-
-		/* and ensure that they'are all equal, we haven't changed the fb */
-		for (j = 0; j < (n_crcs - 1); j++)
-			igt_assert_crc_equal(&crcs[j], &crcs[j + 1]);
-
-		if (flags & TEST_SEQUENCE)
-			for (j = 0; j < (n_crcs - 1); j++)
-				igt_assert_eq(crcs[j].frame + 1, crcs[j + 1].frame);
-
-		free(crcs);
-		igt_pipe_crc_free(pipe_crc);
-		igt_remove_fb(data->drm_fd, &data->fb);
-		igt_plane_set_fb(primary, NULL);
-
-		igt_output_set_pipe(output, PIPE_ANY);
-	}
-
-	return 1;
-}
-
-static void test_read_crc(data_t *data, int pipe, unsigned flags)
-{
-	igt_display_t *display = &data->display;
-	int valid_connectors = 0;
-	igt_output_t *output;
-
-	igt_skip_on(pipe >= data->display.n_pipes);
-
-	for_each_connected_output(display, output) {
-
-		igt_info("%s: Testing connector %s using pipe %s\n",
-			 igt_subtest_name(), igt_output_name(output),
-			 kmstest_pipe_name(pipe));
-
-		valid_connectors += test_read_crc_for_output(data, pipe, output, flags);
-	}
-
-	igt_require_f(valid_connectors, "No connector found for pipe %i\n", pipe);
-}
-
-data_t data = {0, };
-
-#define test_prefix(i) ((i) == 0 ? "basic-" : "")
-#define pipe_name(i) ((i) + 'A')
-
-igt_main
-{
-	igt_skip_on_simulation();
-
-	igt_fixture {
-		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
-
-		igt_enable_connectors();
-
-		kmstest_set_vt_graphics_mode();
-
-		igt_require_pipe_crc();
-
-		igt_display_init(&data.display, data.drm_fd);
-	}
-
-	igt_subtest("basic-bad-pipe")
-		test_bad_command(&data, "pipe D none");
-
-	igt_subtest("basic-bad-source")
-		test_bad_command(&data, "pipe A foo");
-
-	igt_subtest("basic-bad-nb-words-1")
-		test_bad_command(&data, "pipe foo");
-
-	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("%sread-crc-pipe-%c", test_prefix(i), pipe_name(i))
-			test_read_crc(&data, i, 0);
-
-		igt_subtest_f("%sread-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
-			test_read_crc(&data, i, TEST_SEQUENCE);
-
-		igt_subtest_f("%snonblocking-crc-pipe-%c", test_prefix(i), pipe_name(i))
-			test_read_crc(&data, i, TEST_NONBLOCK);
-
-		igt_subtest_f("%snonblocking-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
-			test_read_crc(&data, i, TEST_SEQUENCE | TEST_NONBLOCK);
-
-		igt_subtest_f("%ssuspend-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
-			igt_skip_on(i >= data.display.n_pipes);
-			igt_system_suspend_autoresume();
-
-			test_read_crc(&data, i, 0);
-		}
-
-		igt_subtest_f("%shang-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
-			igt_hang_ring_t hang =
-				igt_hang_ring(data.drm_fd, I915_EXEC_RENDER);
-			test_read_crc(&data, i, 0);
-			igt_post_hang_ring(data.drm_fd, hang);
-			test_read_crc(&data, i, 0);
-		}
-	}
-
-	igt_fixture {
-		igt_display_fini(&data.display);
-	}
-}
-- 
1.9.1


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set
  2016-04-22 14:41 [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set Tvrtko Ursulin
  2016-04-22 14:41 ` [PATCH i-g-t 2/2] kms_pipe_crc_basic: Rename test to kms_pipe_crc Tvrtko Ursulin
@ 2016-04-22 15:11 ` Chris Wilson
  2016-04-25  8:51   ` Daniel Vetter
  2016-04-25  7:00 ` Jani Nikula
  2 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2016-04-22 15:11 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx

On Fri, Apr 22, 2016 at 03:41:55PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> I would argue it is enough to test one pipe in the BAT set.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  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.

As far as I can see this is a test that tests the BAT and not the
proposed changes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set
  2016-04-22 14:41 [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set Tvrtko Ursulin
  2016-04-22 14:41 ` [PATCH i-g-t 2/2] kms_pipe_crc_basic: Rename test to kms_pipe_crc Tvrtko Ursulin
  2016-04-22 15:11 ` [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set Chris Wilson
@ 2016-04-25  7:00 ` Jani Nikula
  2 siblings, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2016-04-25  7:00 UTC (permalink / raw)
  To: Tvrtko Ursulin, Intel-gfx

On Fri, 22 Apr 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> I would argue it is enough to test one pipe in the BAT set.

FWIW BYT/CHV DSI may be fixed to port C which is fixed to pipe B,
skipping the test for pipe A.

BR,
Jani.


>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  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))
>  			test_read_crc(&data, i, 0);
>  
> -		igt_subtest_f("read-crc-pipe-%c-frame-sequence", 'A'+i)
> +		igt_subtest_f("%sread-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
>  			test_read_crc(&data, i, TEST_SEQUENCE);
>  
> -		igt_subtest_f("nonblocking-crc-pipe-%c", 'A'+i)
> +		igt_subtest_f("%snonblocking-crc-pipe-%c", test_prefix(i), pipe_name(i))
>  			test_read_crc(&data, i, TEST_NONBLOCK);
>  
> -		igt_subtest_f("nonblocking-crc-pipe-%c-frame-sequence", 'A'+i)
> +		igt_subtest_f("%snonblocking-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
>  			test_read_crc(&data, i, TEST_SEQUENCE | TEST_NONBLOCK);
>  
> -		igt_subtest_f("suspend-read-crc-pipe-%c", 'A'+i) {
> +		igt_subtest_f("%ssuspend-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
>  			igt_skip_on(i >= data.display.n_pipes);
>  			igt_system_suspend_autoresume();
>  
>  			test_read_crc(&data, i, 0);
>  		}
>  
> -		igt_subtest_f("hang-read-crc-pipe-%c", 'A'+i) {
> +		igt_subtest_f("%shang-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
>  			igt_hang_ring_t hang =
>  				igt_hang_ring(data.drm_fd, I915_EXEC_RENDER);
>  			test_read_crc(&data, i, 0);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set
  2016-04-22 15:11 ` [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set Chris Wilson
@ 2016-04-25  8:51   ` Daniel Vetter
  2016-04-25  9:45     ` Tvrtko Ursulin
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2016-04-25  8:51 UTC (permalink / raw)
  To: Chris Wilson, Tvrtko Ursulin, Intel-gfx

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@intel.com>
> > 
> > I would argue it is enough to test one pipe in the BAT set.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  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.

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set
  2016-04-25  8:51   ` Daniel Vetter
@ 2016-04-25  9:45     ` Tvrtko Ursulin
  2016-04-25 20:12       ` Zanoni, Paulo R
  0 siblings, 1 reply; 8+ messages in thread
From: Tvrtko Ursulin @ 2016-04-25  9:45 UTC (permalink / raw)
  To: Daniel Vetter, Chris Wilson, Intel-gfx


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@intel.com>
>>>
>>> I would argue it is enough to test one pipe in the BAT set.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set
  2016-04-25  9:45     ` Tvrtko Ursulin
@ 2016-04-25 20:12       ` Zanoni, Paulo R
  2016-04-26 13:54         ` Daniel Vetter
  0 siblings, 1 reply; 8+ messages in thread
From: Zanoni, Paulo R @ 2016-04-25 20:12 UTC (permalink / raw)
  To: daniel@ffwll.ch, Intel-gfx@lists.freedesktop.org,
	chris@chris-wilson.co.uk, tvrtko.ursulin@linux.intel.com

Em Seg, 2016-04-25 às 10:45 +0100, Tvrtko Ursulin escreveu:
> 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@intel.com>
> > > > 
> > > > I would argue it is enough to test one pipe in the BAT set.
> > > > 
> > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > ---
> > > >   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.

One of the things about our driver is that it tries to be very very
conservative regarding the eDP timings, leading to waits longer than
necessary for pretty much everyone. We tried to optimize this in the
past, but due to the fact that we have a single code base to support
every product released since gen 2, pretty much any sort of
optimization attempt tends to break someone's machine because maybe
they have a bad BIOS or a bad VBT or a bad panel or something else. Now
that this problem is impacting our ability to run tests, maybe it is
time to start thinking about these optimizations again?

If you want to investigate this, you could start by looking at the
patches stored here:

https://cgit.freedesktop.org/~pzanoni/linux/?h=edp-reverts

The first patch adds an option that you could try to play it, and the
other patches contain references to my previous optimization attempts
that ended up being reverted. You'll have to search the mailing list
for the actual regression reports. Feel free to adopt the patches and
change them.

Perhaps we could also think about some local changes that would only
optimize QA's systems, like a module option used by them? The problem
is that this would mean QA wouldn't be testing exactly what's upstream.
Still, it could be worth it, depending on how much time it saves.

> 
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set
  2016-04-25 20:12       ` Zanoni, Paulo R
@ 2016-04-26 13:54         ` Daniel Vetter
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2016-04-26 13:54 UTC (permalink / raw)
  To: Zanoni, Paulo R; +Cc: Intel-gfx@lists.freedesktop.org

On Mon, Apr 25, 2016 at 08:12:41PM +0000, Zanoni, Paulo R wrote:
> One of the things about our driver is that it tries to be very very
> conservative regarding the eDP timings, leading to waits longer than
> necessary for pretty much everyone. We tried to optimize this in the
> past, but due to the fact that we have a single code base to support
> every product released since gen 2, pretty much any sort of
> optimization attempt tends to break someone's machine because maybe
> they have a bad BIOS or a bad VBT or a bad panel or something else. Now
> that this problem is impacting our ability to run tests, maybe it is
> time to start thinking about these optimizations again?
> 
> If you want to investigate this, you could start by looking at the
> patches stored here:
> 
> https://cgit.freedesktop.org/~pzanoni/linux/?h=edp-reverts
> 
> The first patch adds an option that you could try to play it, and the
> other patches contain references to my previous optimization attempts
> that ended up being reverted. You'll have to search the mailing list
> for the actual regression reports. Feel free to adopt the patches and
> change them.
> 
> Perhaps we could also think about some local changes that would only
> optimize QA's systems, like a module option used by them? The problem
> is that this would mean QA wouldn't be testing exactly what's upstream.
> Still, it could be worth it, depending on how much time it saves.

Broken record, but I don't like code disabled by default. What we could
try is to get this merged & enabled, but only for recent-ish vbt version.
That way the support nightmare should be manageable, and hopefully we can
figure out for recent firmware how it's supposed to work.

CI could then still use the option to enable fast timings everywhere (or
at least on those machines where it works).

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-04-26 13:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-22 14:41 [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set Tvrtko Ursulin
2016-04-22 14:41 ` [PATCH i-g-t 2/2] kms_pipe_crc_basic: Rename test to kms_pipe_crc Tvrtko Ursulin
2016-04-22 15:11 ` [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set Chris Wilson
2016-04-25  8:51   ` Daniel Vetter
2016-04-25  9:45     ` Tvrtko Ursulin
2016-04-25 20:12       ` Zanoni, Paulo R
2016-04-26 13:54         ` Daniel Vetter
2016-04-25  7:00 ` Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).