[PATCH i-g-t 1/8] kms_frontbuffer_tracking: Increase the time we wait for PSR.

[PATCH i-g-t 1/8] kms_frontbuffer_tracking: Increase the time we wait for PSR.

[Rodrigo Vivi]

With commit (drm/i915: Delay first PSR activation.) in kernel
PSR might take a bit longer to really activate after the modeset.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 38ed662..cd2879d 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -848,7 +848,7 @@ static bool fbc_wait_until_enabled(void)
 
 static bool psr_wait_until_enabled(void)
 {
-	return igt_wait(psr_is_enabled(), 2000, 1);
+	return igt_wait(psr_is_enabled(), 5000, 1);
 }
 
 #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
-- 
2.4.3

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

[PATCH i-g-t 2/8] kms_frontbuffer_tracking: Skip on unreliable CRC.

[Rodrigo Vivi]

Even with all sink crc re-works we still have platforms
where after 6 vblanks it is unable to calculate the
sink crc. But if we don't get the sink crc it isn't true
that test failed, but that we have no ways to say test
passed or failed.

So let's print a message and move forward in case sink crc
cannot help us to know if the screen has been updated.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index cd2879d..606d0a9 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -858,10 +858,17 @@ static bool psr_wait_until_enabled(void)
 
 static void get_sink_crc(sink_crc_t *crc)
 {
+	int rc, errno_;
+
 	lseek(sink_crc.fd, 0, SEEK_SET);
 
-	igt_assert(read(sink_crc.fd, crc->data, SINK_CRC_SIZE) ==
-		   SINK_CRC_SIZE);
+	rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
+	errno_ = errno;
+
+	if (rc == -1 && errno_ == ETIMEDOUT)
+		igt_skip("Sink CRC is unreliable on this machine. Try running this test again individually\n");
+
+	igt_assert(rc == SINK_CRC_SIZE);
 }
 
 static bool sink_crc_equal(sink_crc_t *a, sink_crc_t *b)
-- 
2.4.3

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

[PATCH i-g-t 3/8] kms_frontbuffer_tracking: Allow pipe crc or sink crc individually.

[Rodrigo Vivi]

Sink CRC should be enough by itself to validate PSR. Also sometimes
the error might be on the CRC calculations themselves. So let's give
the flexibility to use either individually.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 606d0a9..d879493 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -232,7 +232,8 @@ struct draw_pattern_info pattern4;
 /* Command line parameters. */
 struct {
 	bool check_status;
-	bool check_crc;
+	bool check_pipe_crc;
+	bool check_sink_crc;
 	bool fbc_check_compression;
 	bool fbc_check_last_action;
 	bool no_edp;
@@ -244,7 +245,8 @@ struct {
 	int shared_fb_y_offset;
 } opt = {
 	.check_status = true,
-	.check_crc = true,
+	.check_pipe_crc = true,
+	.check_sink_crc = true,
 	.fbc_check_compression = true,
 	.fbc_check_last_action = true,
 	.no_edp = false,
@@ -1578,15 +1580,17 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags)
 	int flags__ = (flags);						\
 	struct both_crcs crc_;						\
 									\
-	if (!opt.check_crc || (flags__ & DONT_ASSERT_CRC))		\
+	if (flags__ & DONT_ASSERT_CRC)					\
 		break;							\
 									\
 	collect_crcs(&crc_);						\
 	print_crc("Calculated CRC:", &crc_);				\
 									\
 	igt_assert(wanted_crc);						\
-	igt_assert_crc_equal(&crc_.pipe, &wanted_crc->pipe);		\
-	assert_sink_crc_equal(&crc_.sink, &wanted_crc->sink);		\
+	if (opt.check_pipe_crc)						\
+		igt_assert_crc_equal(&crc_.pipe, &wanted_crc->pipe);	\
+	if (opt.check_sink_crc)						\
+		assert_sink_crc_equal(&crc_.sink, &wanted_crc->sink);	\
 } while (0)
 
 #define do_status_assertions(flags_) do {				\
@@ -2926,7 +2930,16 @@ static int opt_handler(int option, int option_index, void *data)
 		opt.check_status = false;
 		break;
 	case 'c':
-		opt.check_crc = false;
+		opt.check_pipe_crc = false;
+		opt.check_sink_crc = false;
+		break;
+	case 'S':
+		opt.check_pipe_crc = false;
+		opt.check_sink_crc = true;
+		break;
+	case 'P':
+		opt.check_pipe_crc = true;
+		opt.check_sink_crc = false;
 		break;
 	case 'o':
 		opt.fbc_check_compression = false;
@@ -2974,6 +2987,8 @@ static int opt_handler(int option, int option_index, void *data)
 const char *help_str =
 "  --no-status-check           Don't check for enable/disable status\n"
 "  --no-crc-check              Don't check for CRC values\n"
+"  --sink-crc-only             Check for Sink CRC only. Don't check for Pipe CRC value\n"
+"  --pipe-crc-only             Check for Pipe CRC only. Don't check for Sink CRC value\n"
 "  --no-fbc-compression-check  Don't check for the FBC compression status\n"
 "  --no-fbc-action-check       Don't check for the FBC last action\n"
 "  --no-edp                    Don't use eDP monitors\n"
@@ -3097,6 +3112,8 @@ int main(int argc, char *argv[])
 	struct option long_options[] = {
 		{ "no-status-check",          0, 0, 's'},
 		{ "no-crc-check",             0, 0, 'c'},
+		{ "sink-crc-only",            0, 0, 'S'},
+		{ "pipe-crc-only",            0, 0, 'P'},
 		{ "no-fbc-compression-check", 0, 0, 'o'},
 		{ "no-fbc-action-check",      0, 0, 'a'},
 		{ "no-edp",                   0, 0, 'e'},
-- 
2.4.3

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

[PATCH i-g-t 4/8] kms_frontbuffer_tracking: Allow to skip suspend_resume sub test case.

[Rodrigo Vivi]

There are few platforms with other suspend resume bugs that breaks
the full execution. So let's provide a way to skip suspend resume case.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index d879493..1cc1c9e 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -237,6 +237,7 @@ struct {
 	bool fbc_check_compression;
 	bool fbc_check_last_action;
 	bool no_edp;
+	bool no_suspend;
 	bool small_modes;
 	bool show_hidden;
 	int step;
@@ -250,6 +251,7 @@ struct {
 	.fbc_check_compression = true,
 	.fbc_check_last_action = true,
 	.no_edp = false,
+	.no_suspend = false,
 	.small_modes = false,
 	.show_hidden= false,
 	.step = 0,
@@ -2735,6 +2737,8 @@ static void suspend_subtest(const struct test_mode *t)
 {
 	struct modeset_params *params = pick_params(t);
 
+	igt_skip_on(opt.no_suspend);
+
 	prepare_subtest(t, NULL);
 	sleep(5);
 	igt_system_suspend_autoresume();
@@ -2950,6 +2954,9 @@ static int opt_handler(int option, int option_index, void *data)
 	case 'e':
 		opt.no_edp = true;
 		break;
+	case 'r':
+		opt.no_suspend = true;
+		break;
 	case 'm':
 		opt.small_modes = true;
 		break;
@@ -2992,6 +2999,7 @@ const char *help_str =
 "  --no-fbc-compression-check  Don't check for the FBC compression status\n"
 "  --no-fbc-action-check       Don't check for the FBC last action\n"
 "  --no-edp                    Don't use eDP monitors\n"
+"  --no-suspend                Don't run Suspend/Resume test cases\n"
 "  --use-small-modes           Use smaller resolutions for the modes\n"
 "  --show-hidden               Show hidden subtests\n"
 "  --step                      Stop on each step so you can check the screen\n"
@@ -3117,6 +3125,7 @@ int main(int argc, char *argv[])
 		{ "no-fbc-compression-check", 0, 0, 'o'},
 		{ "no-fbc-action-check",      0, 0, 'a'},
 		{ "no-edp",                   0, 0, 'e'},
+		{ "no-suspend",               0, 0, 'r'},
 		{ "use-small-modes",          0, 0, 'm'},
 		{ "show-hidden",              0, 0, 'i'},
 		{ "step",                     0, 0, 't'},
-- 
2.4.3

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

[PATCH i-g-t 5/8] kms_frontbuffer_tracking: Add option to allow running tescases with PSR disabled.

[Rodrigo Vivi]

We need to be able to identify if the issue is feature related
or caused by another bug.

Also this feature allow users to have a visual feedback of what to
expect when running the test case for real.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 1cc1c9e..312c08e 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -238,6 +238,7 @@ struct {
 	bool fbc_check_last_action;
 	bool no_edp;
 	bool no_suspend;
+	bool psr_disabled;
 	bool small_modes;
 	bool show_hidden;
 	int step;
@@ -252,6 +253,7 @@ struct {
 	.fbc_check_last_action = true,
 	.no_edp = false,
 	.no_suspend = false,
+	.psr_disabled = false,
 	.small_modes = false,
 	.show_hidden= false,
 	.step = 0,
@@ -1617,7 +1619,7 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags)
 	}								\
 									\
 	if (flags_ & ASSERT_PSR_ENABLED) {				\
-		if (!psr_wait_until_enabled()) {			\
+		if (!opt.psr_disabled && !psr_wait_until_enabled()) {	\
 			psr_print_status();				\
 			igt_assert_f(false, "PSR disabled\n");		\
 		}							\
@@ -1717,8 +1719,13 @@ static void enable_features_for_test(const struct test_mode *t)
 {
 	if (t->feature & FEATURE_FBC)
 		fbc_enable();
-	if (t->feature & FEATURE_PSR)
-		psr_enable();
+	if (t->feature & FEATURE_PSR) {
+		if (opt.psr_disabled) {
+			igt_info("WARNING: Running with PSR disabled\n");
+			psr_disable();
+		} else
+			psr_enable();
+	}
 }
 
 static void check_test_requirements(const struct test_mode *t)
@@ -2957,6 +2964,9 @@ static int opt_handler(int option, int option_index, void *data)
 	case 'r':
 		opt.no_suspend = true;
 		break;
+	case 'R':
+		opt.psr_disabled = true;
+		break;
 	case 'm':
 		opt.small_modes = true;
 		break;
@@ -3000,6 +3010,7 @@ const char *help_str =
 "  --no-fbc-action-check       Don't check for the FBC last action\n"
 "  --no-edp                    Don't use eDP monitors\n"
 "  --no-suspend                Don't run Suspend/Resume test cases\n"
+"  --psr-disabled              Dry-run. Run tests with PSR feature disabled.\n"
 "  --use-small-modes           Use smaller resolutions for the modes\n"
 "  --show-hidden               Show hidden subtests\n"
 "  --step                      Stop on each step so you can check the screen\n"
@@ -3126,6 +3137,7 @@ int main(int argc, char *argv[])
 		{ "no-fbc-action-check",      0, 0, 'a'},
 		{ "no-edp",                   0, 0, 'e'},
 		{ "no-suspend",               0, 0, 'r'},
+		{ "psr-disabled",             0, 0, 'R'},
 		{ "use-small-modes",          0, 0, 'm'},
 		{ "show-hidden",              0, 0, 'i'},
 		{ "step",                     0, 0, 't'},
-- 
2.4.3

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

[PATCH i-g-t 6/8] kms_frontbuffer_tracking: Add option to allow running tescases with FBC disabled.

[Rodrigo Vivi]

We need to be able to identify if the issue is feature related
or caused by another bug.

Also this feature allow users to have a visual feedback of what to
expect when running the test case for real.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 312c08e..891fab5 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -239,6 +239,7 @@ struct {
 	bool no_edp;
 	bool no_suspend;
 	bool psr_disabled;
+	bool fbc_disabled;
 	bool small_modes;
 	bool show_hidden;
 	int step;
@@ -254,6 +255,7 @@ struct {
 	.no_edp = false,
 	.no_suspend = false,
 	.psr_disabled = false,
+	.fbc_disabled = false,
 	.small_modes = false,
 	.show_hidden= false,
 	.step = 0,
@@ -1717,8 +1719,13 @@ static void set_sprite_for_test(const struct test_mode *t,
 
 static void enable_features_for_test(const struct test_mode *t)
 {
-	if (t->feature & FEATURE_FBC)
-		fbc_enable();
+	if (t->feature & FEATURE_FBC) {
+		if (opt.fbc_disabled) {
+			igt_info("WARNING: Running with PSR disabled\n");
+			fbc_disable();
+		} else
+			fbc_enable();
+	}
 	if (t->feature & FEATURE_PSR) {
 		if (opt.psr_disabled) {
 			igt_info("WARNING: Running with PSR disabled\n");
@@ -2967,6 +2974,9 @@ static int opt_handler(int option, int option_index, void *data)
 	case 'R':
 		opt.psr_disabled = true;
 		break;
+	case 'F':
+		opt.fbc_disabled = true;
+		break;
 	case 'm':
 		opt.small_modes = true;
 		break;
@@ -3011,6 +3021,7 @@ const char *help_str =
 "  --no-edp                    Don't use eDP monitors\n"
 "  --no-suspend                Don't run Suspend/Resume test cases\n"
 "  --psr-disabled              Dry-run. Run tests with PSR feature disabled.\n"
+"  --fbc-disabled              Dry-run. Run tests with FBC feature disabled.\n"
 "  --use-small-modes           Use smaller resolutions for the modes\n"
 "  --show-hidden               Show hidden subtests\n"
 "  --step                      Stop on each step so you can check the screen\n"
@@ -3138,6 +3149,7 @@ int main(int argc, char *argv[])
 		{ "no-edp",                   0, 0, 'e'},
 		{ "no-suspend",               0, 0, 'r'},
 		{ "psr-disabled",             0, 0, 'R'},
+		{ "fbc-disabled",             0, 0, 'F'},
 		{ "use-small-modes",          0, 0, 'm'},
 		{ "show-hidden",              0, 0, 'i'},
 		{ "step",                     0, 0, 't'},
-- 
2.4.3

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

[PATCH i-g-t 7/8] kms_psr_sink_crc: Fix no-psr option.

[Rodrigo Vivi]

commit 75b286e821 ("tests/kms_psr_sink_crc: test even
if PSR is disabled by default")' force PSR enabling without
respecting the no-psr (running-with-psr-disabled) option.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/kms_psr_sink_crc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
index 387c615..b2f88ea 100644
--- a/tests/kms_psr_sink_crc.c
+++ b/tests/kms_psr_sink_crc.c
@@ -557,7 +557,8 @@ int main(int argc, char *argv[])
 		kmstest_set_vt_graphics_mode();
 		data.devid = intel_get_drm_devid(data.drm_fd);
 
-		igt_set_module_param_int("enable_psr", 1);
+		igt_set_module_param_int("enable_psr", running_with_psr_disabled ?
+					 0 : 1);
 
 		igt_skip_on(!psr_possible(&data));
 
-- 
2.4.3

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

[PATCH i-g-t 8/8] kms_psr_sink_crc: Add suspend/resume sub test.

[Rodrigo Vivi]

Also add a option to allow us to skip this test case in machines
where other suspend/resume issues would break the test exectution.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 tests/kms_psr_sink_crc.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
index b2f88ea..e2ca988 100644
--- a/tests/kms_psr_sink_crc.c
+++ b/tests/kms_psr_sink_crc.c
@@ -31,6 +31,7 @@
 #include "intel_bufmgr.h"
 
 bool running_with_psr_disabled;
+bool no_suspend;
 
 #define CRC_BLACK "000000000000"
 
@@ -530,6 +531,9 @@ static int opt_handler(int opt, int opt_index, void *data)
 	case 'n':
 		running_with_psr_disabled = true;
 		break;
+	case 's':
+		no_suspend = true;
+		break;
 	default:
 		igt_assert(0);
 	}
@@ -540,9 +544,11 @@ static int opt_handler(int opt, int opt_index, void *data)
 int main(int argc, char *argv[])
 {
 	const char *help_str =
-	       "  --no-psr\tRun test without PSR to check the CRC test logic.";
+		"  --no-psr\tRun test without PSR to check the CRC test logic."
+		"  --no-suspend\tDon't run suspend resume test cases.";
 	static struct option long_options[] = {
 		{"no-psr", 0, 0, 'n'},
+		{"no-suspend", 0, 0, 's'},
 		{ 0, 0, 0, 0 }
 	};
 	data_t data = {};
@@ -626,6 +632,36 @@ int main(int argc, char *argv[])
 		test_cleanup(&data);
 	}
 
+	igt_subtest_f("suspend_psr_active") {
+
+		igt_skip_on(no_suspend);
+
+		data.test_plane = PRIMARY;
+		data.op = PAGE_FLIP;
+		setup_test_plane(&data);
+		igt_assert(wait_psr_entry(&data));
+
+		igt_system_suspend_autoresume();
+
+		run_test(&data);
+		test_cleanup(&data);
+	}
+
+	igt_subtest_f("suspend_psr_exit") {
+
+		igt_skip_on(no_suspend);
+
+		data.test_plane = CURSOR;
+		data.op = PLANE_ONOFF;
+		setup_test_plane(&data);
+
+		igt_system_suspend_autoresume();
+
+		igt_assert(wait_psr_entry(&data));
+		run_test(&data);
+		test_cleanup(&data);
+	}
+
 	igt_fixture {
 		drm_intel_bufmgr_destroy(data.bufmgr);
 		display_fini(&data);
-- 
2.4.3

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

Re: [PATCH i-g-t 1/8] kms_frontbuffer_tracking: Increase the time we wait for PSR.

[Paulo Zanoni]

2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> With commit (drm/i915: Delay first PSR activation.) in kernel
> PSR might take a bit longer to really activate after the modeset.

Can you please expand this commit message a little bit for Future
Paulo and Future Rodrigo? It would be nice to mention that the timeout
is going to be 5 times the panel power cycle delay, which is XYZms for
your machine, so the timeout needs to be a minimum of Xs, so 5s is the
safe value (we print the panel power cycle delay time on dmesg).

Also, maybe add a notice that since we do a lot of
assert(psr_disabled), this commit is increasing the time it takes to
run the whole set of PSR tests by a few minutes (check commit
f4db3b18841263f8f617a9f7f0aaf14fab7196d1 for an explanation).

Maybe you could also provide patch 9/8 adding a DONT_ASSERT_PSR_STATUS
flag and patch 10/8 that changes the callers of op_disables_psr(),
making them use DONT_ASSERT_PSR_STATUS so we won't eat the full 5s
timeout during every single GTT mmap test. This will make the time it
takes to run the full set of PSR tests even smaller than what it is
today (!!!).

With the improved commit message: Reviewed-by: Paulo Zanoni
<paulo.r.zanoni@intel.com>

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index 38ed662..cd2879d 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -848,7 +848,7 @@ static bool fbc_wait_until_enabled(void)
>
>  static bool psr_wait_until_enabled(void)
>  {
> -       return igt_wait(psr_is_enabled(), 2000, 1);
> +       return igt_wait(psr_is_enabled(), 5000, 1);
>  }
>
>  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH i-g-t 2/8] kms_frontbuffer_tracking: Skip on unreliable CRC.

[Paulo Zanoni]

2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Even with all sink crc re-works we still have platforms
> where after 6 vblanks it is unable to calculate the
> sink crc. But if we don't get the sink crc it isn't true
> that test failed, but that we have no ways to say test
> passed or failed.
>
> So let's print a message and move forward in case sink crc
> cannot help us to know if the screen has been updated.

As much as I understand your reasoning here, "Try running this test
again" will be ignored by our future bots.

Instead of just skipping, isn't there something else we could do, such
as trying again 10 times? 60 frames doesn't seem expensive. If it
works at least sometimes, I'd say it's worth the try.

Besides, did we try the AUX_MUTEX register that was suggested here:
http://patchwork.freedesktop.org/patch/57693/ ? Maybe it would solve
all our sink CRCs problem.

Another comment: FBC doesn't really need sink CRC, but it's currently
checking sink CRC, so it may get SKIPs. Maybe instead of a SKIP for
failed sink CRC on FBC we could just ignore and move on? Maybe we
could pass some flags to collect_crcs() so it can know if sink CRCs
are ignorable.

Another problem is: what if we fail while getting the reference CRC?
We will leave garbage inside crc->data, and the other tests will
compare themselves against the garbage in case reading sink CRCs end
up working for them, so we'll have test failures that are not real
failures. Maybe we should pass some flag to collect_crtcs() signaling
that we're trying a reference CRC, so it can write something to
crtc->data, just like we have the "unsupported!" string. Then we'd
have to check this special string later.

You also probably need to fix setup_sink_crc(), because it currently
doesn't check for ETIMETDOUT.

I'm not blocking the patch, just starting the discussion :)

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index cd2879d..606d0a9 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -858,10 +858,17 @@ static bool psr_wait_until_enabled(void)
>
>  static void get_sink_crc(sink_crc_t *crc)
>  {
> +       int rc, errno_;
> +
>         lseek(sink_crc.fd, 0, SEEK_SET);
>
> -       igt_assert(read(sink_crc.fd, crc->data, SINK_CRC_SIZE) ==
> -                  SINK_CRC_SIZE);
> +       rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
> +       errno_ = errno;
> +
> +       if (rc == -1 && errno_ == ETIMEDOUT)
> +               igt_skip("Sink CRC is unreliable on this machine. Try running this test again individually\n");
> +
> +       igt_assert(rc == SINK_CRC_SIZE);
>  }
>
>  static bool sink_crc_equal(sink_crc_t *a, sink_crc_t *b)
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH i-g-t 4/8] kms_frontbuffer_tracking: Allow to skip suspend_resume sub test case.

[Paulo Zanoni]

2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> There are few platforms with other suspend resume bugs that breaks
> the full execution. So let's provide a way to skip suspend resume case.

Well, I carry a local patch that completely disables suspend subtests
for the tests that I usually run, so I really understand your pain.
Suspend subtests take a long time to run, and they usually don't work
on some of the preproduction machines I still use.

But since this problem is not specific to kms_frontbuffer_tracking,
maybe we could adopt an igt-wide solution here? Thomas, any idea here?

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index d879493..1cc1c9e 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -237,6 +237,7 @@ struct {
>         bool fbc_check_compression;
>         bool fbc_check_last_action;
>         bool no_edp;
> +       bool no_suspend;
>         bool small_modes;
>         bool show_hidden;
>         int step;
> @@ -250,6 +251,7 @@ struct {
>         .fbc_check_compression = true,
>         .fbc_check_last_action = true,
>         .no_edp = false,
> +       .no_suspend = false,
>         .small_modes = false,
>         .show_hidden= false,
>         .step = 0,
> @@ -2735,6 +2737,8 @@ static void suspend_subtest(const struct test_mode *t)
>  {
>         struct modeset_params *params = pick_params(t);
>
> +       igt_skip_on(opt.no_suspend);
> +
>         prepare_subtest(t, NULL);
>         sleep(5);
>         igt_system_suspend_autoresume();
> @@ -2950,6 +2954,9 @@ static int opt_handler(int option, int option_index, void *data)
>         case 'e':
>                 opt.no_edp = true;
>                 break;
> +       case 'r':
> +               opt.no_suspend = true;
> +               break;
>         case 'm':
>                 opt.small_modes = true;
>                 break;
> @@ -2992,6 +2999,7 @@ const char *help_str =
>  "  --no-fbc-compression-check  Don't check for the FBC compression status\n"
>  "  --no-fbc-action-check       Don't check for the FBC last action\n"
>  "  --no-edp                    Don't use eDP monitors\n"
> +"  --no-suspend                Don't run Suspend/Resume test cases\n"
>  "  --use-small-modes           Use smaller resolutions for the modes\n"
>  "  --show-hidden               Show hidden subtests\n"
>  "  --step                      Stop on each step so you can check the screen\n"
> @@ -3117,6 +3125,7 @@ int main(int argc, char *argv[])
>                 { "no-fbc-compression-check", 0, 0, 'o'},
>                 { "no-fbc-action-check",      0, 0, 'a'},
>                 { "no-edp",                   0, 0, 'e'},
> +               { "no-suspend",               0, 0, 'r'},
>                 { "use-small-modes",          0, 0, 'm'},
>                 { "show-hidden",              0, 0, 'i'},
>                 { "step",                     0, 0, 't'},
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH i-g-t 4/8] kms_frontbuffer_tracking: Allow to skip suspend_resume sub test case.

[Ville Syrjälä]

On Thu, Nov 05, 2015 at 06:34:07PM -0200, Paulo Zanoni wrote:
> 2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > There are few platforms with other suspend resume bugs that breaks
> > the full execution. So let's provide a way to skip suspend resume case.
> 
> Well, I carry a local patch that completely disables suspend subtests
> for the tests that I usually run, so I really understand your pain.
> Suspend subtests take a long time to run, and they usually don't work
> on some of the preproduction machines I still use.
> 
> But since this problem is not specific to kms_frontbuffer_tracking,
> maybe we could adopt an igt-wide solution here? Thomas, any idea here?

-x suspend is what I tell piglit on one hsw I have here which hangs on s3.

> 
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  tests/kms_frontbuffer_tracking.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> > index d879493..1cc1c9e 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -237,6 +237,7 @@ struct {
> >         bool fbc_check_compression;
> >         bool fbc_check_last_action;
> >         bool no_edp;
> > +       bool no_suspend;
> >         bool small_modes;
> >         bool show_hidden;
> >         int step;
> > @@ -250,6 +251,7 @@ struct {
> >         .fbc_check_compression = true,
> >         .fbc_check_last_action = true,
> >         .no_edp = false,
> > +       .no_suspend = false,
> >         .small_modes = false,
> >         .show_hidden= false,
> >         .step = 0,
> > @@ -2735,6 +2737,8 @@ static void suspend_subtest(const struct test_mode *t)
> >  {
> >         struct modeset_params *params = pick_params(t);
> >
> > +       igt_skip_on(opt.no_suspend);
> > +
> >         prepare_subtest(t, NULL);
> >         sleep(5);
> >         igt_system_suspend_autoresume();
> > @@ -2950,6 +2954,9 @@ static int opt_handler(int option, int option_index, void *data)
> >         case 'e':
> >                 opt.no_edp = true;
> >                 break;
> > +       case 'r':
> > +               opt.no_suspend = true;
> > +               break;
> >         case 'm':
> >                 opt.small_modes = true;
> >                 break;
> > @@ -2992,6 +2999,7 @@ const char *help_str =
> >  "  --no-fbc-compression-check  Don't check for the FBC compression status\n"
> >  "  --no-fbc-action-check       Don't check for the FBC last action\n"
> >  "  --no-edp                    Don't use eDP monitors\n"
> > +"  --no-suspend                Don't run Suspend/Resume test cases\n"
> >  "  --use-small-modes           Use smaller resolutions for the modes\n"
> >  "  --show-hidden               Show hidden subtests\n"
> >  "  --step                      Stop on each step so you can check the screen\n"
> > @@ -3117,6 +3125,7 @@ int main(int argc, char *argv[])
> >                 { "no-fbc-compression-check", 0, 0, 'o'},
> >                 { "no-fbc-action-check",      0, 0, 'a'},
> >                 { "no-edp",                   0, 0, 'e'},
> > +               { "no-suspend",               0, 0, 'r'},
> >                 { "use-small-modes",          0, 0, 'm'},
> >                 { "show-hidden",              0, 0, 'i'},
> >                 { "step",                     0, 0, 't'},
> > --
> > 2.4.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 5/8] kms_frontbuffer_tracking: Add option to allow running tescases with PSR disabled.

[Paulo Zanoni]

2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> We need to be able to identify if the issue is feature related
> or caused by another bug.
>
> Also this feature allow users to have a visual feedback of what to
> expect when running the test case for real.

We currently have --no-status-check that can be used when you don't
want the enable/disable checks.

We also have the "nop" subtests that appear when you use --show-hidden
(I don't want to waste QA's time with these since they're just for
debugging purposes, that's why they are hidden). So if you want to
debug, for example, subtest psr-1p-primscrn-pri-indfb-draw-mmap-cpu,
you can just run:

sudo ./kms_frontbuffer_tracking --show-hidden --run-subtest
nop-1p-primscrn-pri-indfb-draw-mmap-cpu

(notice that there's a patch on the list proposing to replace the
--show-hidden with --all)

Do any of the solutions above solve the problem that made you write
this patch and patch 6? If not, why?

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index 1cc1c9e..312c08e 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -238,6 +238,7 @@ struct {
>         bool fbc_check_last_action;
>         bool no_edp;
>         bool no_suspend;
> +       bool psr_disabled;
>         bool small_modes;
>         bool show_hidden;
>         int step;
> @@ -252,6 +253,7 @@ struct {
>         .fbc_check_last_action = true,
>         .no_edp = false,
>         .no_suspend = false,
> +       .psr_disabled = false,
>         .small_modes = false,
>         .show_hidden= false,
>         .step = 0,
> @@ -1617,7 +1619,7 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags)
>         }                                                               \
>                                                                         \
>         if (flags_ & ASSERT_PSR_ENABLED) {                              \
> -               if (!psr_wait_until_enabled()) {                        \
> +               if (!opt.psr_disabled && !psr_wait_until_enabled()) {   \
>                         psr_print_status();                             \
>                         igt_assert_f(false, "PSR disabled\n");          \
>                 }                                                       \
> @@ -1717,8 +1719,13 @@ static void enable_features_for_test(const struct test_mode *t)
>  {
>         if (t->feature & FEATURE_FBC)
>                 fbc_enable();
> -       if (t->feature & FEATURE_PSR)
> -               psr_enable();
> +       if (t->feature & FEATURE_PSR) {
> +               if (opt.psr_disabled) {
> +                       igt_info("WARNING: Running with PSR disabled\n");
> +                       psr_disable();
> +               } else
> +                       psr_enable();
> +       }
>  }
>
>  static void check_test_requirements(const struct test_mode *t)
> @@ -2957,6 +2964,9 @@ static int opt_handler(int option, int option_index, void *data)
>         case 'r':
>                 opt.no_suspend = true;
>                 break;
> +       case 'R':
> +               opt.psr_disabled = true;
> +               break;
>         case 'm':
>                 opt.small_modes = true;
>                 break;
> @@ -3000,6 +3010,7 @@ const char *help_str =
>  "  --no-fbc-action-check       Don't check for the FBC last action\n"
>  "  --no-edp                    Don't use eDP monitors\n"
>  "  --no-suspend                Don't run Suspend/Resume test cases\n"
> +"  --psr-disabled              Dry-run. Run tests with PSR feature disabled.\n"
>  "  --use-small-modes           Use smaller resolutions for the modes\n"
>  "  --show-hidden               Show hidden subtests\n"
>  "  --step                      Stop on each step so you can check the screen\n"
> @@ -3126,6 +3137,7 @@ int main(int argc, char *argv[])
>                 { "no-fbc-action-check",      0, 0, 'a'},
>                 { "no-edp",                   0, 0, 'e'},
>                 { "no-suspend",               0, 0, 'r'},
> +               { "psr-disabled",             0, 0, 'R'},
>                 { "use-small-modes",          0, 0, 'm'},
>                 { "show-hidden",              0, 0, 'i'},
>                 { "step",                     0, 0, 't'},
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH i-g-t 3/8] kms_frontbuffer_tracking: Allow pipe crc or sink crc individually.

[Paulo Zanoni]

2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> Sink CRC should be enough by itself to validate PSR. Also sometimes
> the error might be on the CRC calculations themselves. So let's give
> the flexibility to use either individually.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index 606d0a9..d879493 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -232,7 +232,8 @@ struct draw_pattern_info pattern4;
>  /* Command line parameters. */
>  struct {
>         bool check_status;
> -       bool check_crc;
> +       bool check_pipe_crc;
> +       bool check_sink_crc;
>         bool fbc_check_compression;
>         bool fbc_check_last_action;
>         bool no_edp;
> @@ -244,7 +245,8 @@ struct {
>         int shared_fb_y_offset;
>  } opt = {
>         .check_status = true,
> -       .check_crc = true,
> +       .check_pipe_crc = true,
> +       .check_sink_crc = true,
>         .fbc_check_compression = true,
>         .fbc_check_last_action = true,
>         .no_edp = false,
> @@ -1578,15 +1580,17 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags)
>         int flags__ = (flags);                                          \
>         struct both_crcs crc_;                                          \
>                                                                         \
> -       if (!opt.check_crc || (flags__ & DONT_ASSERT_CRC))              \
> +       if (flags__ & DONT_ASSERT_CRC)                                  \
>                 break;                                                  \
>                                                                         \
>         collect_crcs(&crc_);                                            \
>         print_crc("Calculated CRC:", &crc_);                            \
>                                                                         \
>         igt_assert(wanted_crc);                                         \
> -       igt_assert_crc_equal(&crc_.pipe, &wanted_crc->pipe);            \
> -       assert_sink_crc_equal(&crc_.sink, &wanted_crc->sink);           \
> +       if (opt.check_pipe_crc)                                         \
> +               igt_assert_crc_equal(&crc_.pipe, &wanted_crc->pipe);    \
> +       if (opt.check_sink_crc)                                         \
> +               assert_sink_crc_equal(&crc_.sink, &wanted_crc->sink);   \

This, combined with patch 2/8, means that we may still get SKIPs in
case reading the sink CRC fails, even if we specify --pipe-crc-only.
We should avoid even collecting the CRCs in case the flags are passed
so we don't risk that SKIP you introduced.

>  } while (0)
>
>  #define do_status_assertions(flags_) do {                              \
> @@ -2926,7 +2930,16 @@ static int opt_handler(int option, int option_index, void *data)
>                 opt.check_status = false;
>                 break;
>         case 'c':
> -               opt.check_crc = false;
> +               opt.check_pipe_crc = false;
> +               opt.check_sink_crc = false;
> +               break;
> +       case 'S':
> +               opt.check_pipe_crc = false;
> +               opt.check_sink_crc = true;
> +               break;
> +       case 'P':
> +               opt.check_pipe_crc = true;
> +               opt.check_sink_crc = false;
>                 break;
>         case 'o':
>                 opt.fbc_check_compression = false;
> @@ -2974,6 +2987,8 @@ static int opt_handler(int option, int option_index, void *data)
>  const char *help_str =
>  "  --no-status-check           Don't check for enable/disable status\n"
>  "  --no-crc-check              Don't check for CRC values\n"
> +"  --sink-crc-only             Check for Sink CRC only. Don't check for Pipe CRC value\n"
> +"  --pipe-crc-only             Check for Pipe CRC only. Don't check for Sink CRC value\n"

I was trying really hard to keep all descriptions under 80 columns
because OCD :)

By the way, the logic behind these flags seems inverted. Since both
CRCs are enabled by default, the flags should be to prevent them. Why
not just make them "--no-sink-crc" and "--no-pipe-crc"? IMHO it's more
intuitive and clear since each flag only touches one of the knobs
instead of mandating the behavior on two different knobs (i.e.,
passing --sink-crc-only --pipe-crc-only doesn't make sense and only
the last flag will be respected, but passing --no-pipe-crc
--no-sink-crc makes sense since each flag controls only one of the
knobs).

Otherwise, I like the idea: many times I wrote small hacks to ignore
just sink CRCs but not pipe CRC but never ended up writing the option.


>  "  --no-fbc-compression-check  Don't check for the FBC compression status\n"
>  "  --no-fbc-action-check       Don't check for the FBC last action\n"
>  "  --no-edp                    Don't use eDP monitors\n"
> @@ -3097,6 +3112,8 @@ int main(int argc, char *argv[])
>         struct option long_options[] = {
>                 { "no-status-check",          0, 0, 's'},
>                 { "no-crc-check",             0, 0, 'c'},
> +               { "sink-crc-only",            0, 0, 'S'},
> +               { "pipe-crc-only",            0, 0, 'P'},
>                 { "no-fbc-compression-check", 0, 0, 'o'},
>                 { "no-fbc-action-check",      0, 0, 'a'},
>                 { "no-edp",                   0, 0, 'e'},
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH i-g-t 4/8] kms_frontbuffer_tracking: Allow to skip suspend_resume sub test case.

[Paulo Zanoni]

2015-11-05 18:40 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Nov 05, 2015 at 06:34:07PM -0200, Paulo Zanoni wrote:
>> 2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
>> > There are few platforms with other suspend resume bugs that breaks
>> > the full execution. So let's provide a way to skip suspend resume case.
>>
>> Well, I carry a local patch that completely disables suspend subtests
>> for the tests that I usually run, so I really understand your pain.
>> Suspend subtests take a long time to run, and they usually don't work
>> on some of the preproduction machines I still use.
>>
>> But since this problem is not specific to kms_frontbuffer_tracking,
>> maybe we could adopt an igt-wide solution here? Thomas, any idea here?
>
> -x suspend is what I tell piglit on one hsw I have here which hangs on s3.

The problem with piglit is that it runs every single subtest as a
separate test program invocation. For KMS tests this is a huge problem
since it requires generating the reference CRCs every time and it also
requires a modeset to restore fbcon every single time. For non-eDP the
cost is not big, but for eDP it adds up: running a subset of only 70
subtests on kms_frontbuffer_tracking, we go from 3:05 to 6:21 in total
execution time.

>
>>
>> >
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > ---
>> >  tests/kms_frontbuffer_tracking.c | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
>> > index d879493..1cc1c9e 100644
>> > --- a/tests/kms_frontbuffer_tracking.c
>> > +++ b/tests/kms_frontbuffer_tracking.c
>> > @@ -237,6 +237,7 @@ struct {
>> >         bool fbc_check_compression;
>> >         bool fbc_check_last_action;
>> >         bool no_edp;
>> > +       bool no_suspend;
>> >         bool small_modes;
>> >         bool show_hidden;
>> >         int step;
>> > @@ -250,6 +251,7 @@ struct {
>> >         .fbc_check_compression = true,
>> >         .fbc_check_last_action = true,
>> >         .no_edp = false,
>> > +       .no_suspend = false,
>> >         .small_modes = false,
>> >         .show_hidden= false,
>> >         .step = 0,
>> > @@ -2735,6 +2737,8 @@ static void suspend_subtest(const struct test_mode *t)
>> >  {
>> >         struct modeset_params *params = pick_params(t);
>> >
>> > +       igt_skip_on(opt.no_suspend);
>> > +
>> >         prepare_subtest(t, NULL);
>> >         sleep(5);
>> >         igt_system_suspend_autoresume();
>> > @@ -2950,6 +2954,9 @@ static int opt_handler(int option, int option_index, void *data)
>> >         case 'e':
>> >                 opt.no_edp = true;
>> >                 break;
>> > +       case 'r':
>> > +               opt.no_suspend = true;
>> > +               break;
>> >         case 'm':
>> >                 opt.small_modes = true;
>> >                 break;
>> > @@ -2992,6 +2999,7 @@ const char *help_str =
>> >  "  --no-fbc-compression-check  Don't check for the FBC compression status\n"
>> >  "  --no-fbc-action-check       Don't check for the FBC last action\n"
>> >  "  --no-edp                    Don't use eDP monitors\n"
>> > +"  --no-suspend                Don't run Suspend/Resume test cases\n"
>> >  "  --use-small-modes           Use smaller resolutions for the modes\n"
>> >  "  --show-hidden               Show hidden subtests\n"
>> >  "  --step                      Stop on each step so you can check the screen\n"
>> > @@ -3117,6 +3125,7 @@ int main(int argc, char *argv[])
>> >                 { "no-fbc-compression-check", 0, 0, 'o'},
>> >                 { "no-fbc-action-check",      0, 0, 'a'},
>> >                 { "no-edp",                   0, 0, 'e'},
>> > +               { "no-suspend",               0, 0, 'r'},
>> >                 { "use-small-modes",          0, 0, 'm'},
>> >                 { "show-hidden",              0, 0, 'i'},
>> >                 { "step",                     0, 0, 't'},
>> > --
>> > 2.4.3
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC



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

Re: [PATCH i-g-t 2/8] kms_frontbuffer_tracking: Skip on unreliable CRC.

[Daniel Vetter]

On Thu, Nov 05, 2015 at 06:30:30PM -0200, Paulo Zanoni wrote:
> 2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > Even with all sink crc re-works we still have platforms
> > where after 6 vblanks it is unable to calculate the
> > sink crc. But if we don't get the sink crc it isn't true
> > that test failed, but that we have no ways to say test
> > passed or failed.
> >
> > So let's print a message and move forward in case sink crc
> > cannot help us to know if the screen has been updated.
> 
> As much as I understand your reasoning here, "Try running this test
> again" will be ignored by our future bots.
> 
> Instead of just skipping, isn't there something else we could do, such
> as trying again 10 times? 60 frames doesn't seem expensive. If it
> works at least sometimes, I'd say it's worth the try.
> 
> Besides, did we try the AUX_MUTEX register that was suggested here:
> http://patchwork.freedesktop.org/patch/57693/ ? Maybe it would solve
> all our sink CRCs problem.
> 
> Another comment: FBC doesn't really need sink CRC, but it's currently
> checking sink CRC, so it may get SKIPs. Maybe instead of a SKIP for
> failed sink CRC on FBC we could just ignore and move on? Maybe we
> could pass some flags to collect_crcs() so it can know if sink CRCs
> are ignorable.

Yeah, at least we should make FBC tests not depend upon sink crcs.
Otherwise test coverage might artificially go down.
-Daniel

> 
> Another problem is: what if we fail while getting the reference CRC?
> We will leave garbage inside crc->data, and the other tests will
> compare themselves against the garbage in case reading sink CRCs end
> up working for them, so we'll have test failures that are not real
> failures. Maybe we should pass some flag to collect_crtcs() signaling
> that we're trying a reference CRC, so it can write something to
> crtc->data, just like we have the "unsupported!" string. Then we'd
> have to check this special string later.
> 
> You also probably need to fix setup_sink_crc(), because it currently
> doesn't check for ETIMETDOUT.
> 
> I'm not blocking the patch, just starting the discussion :)
> 
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  tests/kms_frontbuffer_tracking.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> > index cd2879d..606d0a9 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -858,10 +858,17 @@ static bool psr_wait_until_enabled(void)
> >
> >  static void get_sink_crc(sink_crc_t *crc)
> >  {
> > +       int rc, errno_;
> > +
> >         lseek(sink_crc.fd, 0, SEEK_SET);
> >
> > -       igt_assert(read(sink_crc.fd, crc->data, SINK_CRC_SIZE) ==
> > -                  SINK_CRC_SIZE);
> > +       rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
> > +       errno_ = errno;
> > +
> > +       if (rc == -1 && errno_ == ETIMEDOUT)
> > +               igt_skip("Sink CRC is unreliable on this machine. Try running this test again individually\n");
> > +
> > +       igt_assert(rc == SINK_CRC_SIZE);
> >  }
> >
> >  static bool sink_crc_equal(sink_crc_t *a, sink_crc_t *b)
> > --
> > 2.4.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

Re: [PATCH i-g-t 4/8] kms_frontbuffer_tracking: Allow to skip suspend_resume sub test case.

[Daniel Vetter]

On Mon, Nov 09, 2015 at 11:52:20AM -0200, Paulo Zanoni wrote:
> 2015-11-05 18:40 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Thu, Nov 05, 2015 at 06:34:07PM -0200, Paulo Zanoni wrote:
> >> 2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> >> > There are few platforms with other suspend resume bugs that breaks
> >> > the full execution. So let's provide a way to skip suspend resume case.
> >>
> >> Well, I carry a local patch that completely disables suspend subtests
> >> for the tests that I usually run, so I really understand your pain.
> >> Suspend subtests take a long time to run, and they usually don't work
> >> on some of the preproduction machines I still use.
> >>
> >> But since this problem is not specific to kms_frontbuffer_tracking,
> >> maybe we could adopt an igt-wide solution here? Thomas, any idea here?
> >
> > -x suspend is what I tell piglit on one hsw I have here which hangs on s3.
> 
> The problem with piglit is that it runs every single subtest as a
> separate test program invocation. For KMS tests this is a huge problem
> since it requires generating the reference CRCs every time and it also
> requires a modeset to restore fbcon every single time. For non-eDP the
> cost is not big, but for eDP it adds up: running a subset of only 70
> subtests on kms_frontbuffer_tracking, we go from 3:05 to 6:21 in total
> execution time.

We need to fix this. Generating the reference crc is hard, but getting rid
of fbcon and other setup overhead should be doable. Improving piglit
runtime means CI can run more tests, which means the test you're writing
might actually be used.
-Daniel
-- 
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

Re: [PATCH i-g-t 2/8] kms_frontbuffer_tracking: Skip on unreliable CRC.

[Rodrigo Vivi]

On Thu, Nov 5, 2015 at 12:30 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
>> Even with all sink crc re-works we still have platforms
>> where after 6 vblanks it is unable to calculate the
>> sink crc. But if we don't get the sink crc it isn't true
>> that test failed, but that we have no ways to say test
>> passed or failed.
>>
>> So let's print a message and move forward in case sink crc
>> cannot help us to know if the screen has been updated.
>
> As much as I understand your reasoning here, "Try running this test
> again" will be ignored by our future bots.
>
> Instead of just skipping, isn't there something else we could do, such
> as trying again 10 times? 60 frames doesn't seem expensive. If it
> works at least sometimes, I'd say it's worth the try.

we would need to redo the test... whenever in that unreliable mode if
there is no screen update the sink crc never gets recovered.. so at
least for now we could skip, then later we can rework the tests to
fully retry whenever makes sense.

>
> Besides, did we try the AUX_MUTEX register that was suggested here:
> http://patchwork.freedesktop.org/patch/57693/ ? Maybe it would solve
> all our sink CRCs problem.

We tried after this... For a moment when running it on KBL I was
excited and believed that it could help, but later when testing again
on another SKL we saw aux mutex is unreliable..  unfortunately :(

>
> Another comment: FBC doesn't really need sink CRC, but it's currently
> checking sink CRC, so it may get SKIPs. Maybe instead of a SKIP for
> failed sink CRC on FBC we could just ignore and move on? Maybe we
> could pass some flags to collect_crcs() so it can know if sink CRCs
> are ignorable.

good point.

>
> Another problem is: what if we fail while getting the reference CRC?
> We will leave garbage inside crc->data, and the other tests will
> compare themselves against the garbage in case reading sink CRCs end
> up working for them, so we'll have test failures that are not real
> failures. Maybe we should pass some flag to collect_crtcs() signaling
> that we're trying a reference CRC, so it can write something to
> crtc->data, just like we have the "unsupported!" string. Then we'd
> have to check this special string later.

This is something that was happening before late sink crc rework...
now sink crc only returns something if it is reliable.

>
> You also probably need to fix setup_sink_crc(), because it currently
> doesn't check for ETIMETDOUT.

oh I see.. I will take a look there as well...

>
> I'm not blocking the patch, just starting the discussion :)

Thanks and sorry for not engaging on this discussion sooner..

>
>>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  tests/kms_frontbuffer_tracking.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
>> index cd2879d..606d0a9 100644
>> --- a/tests/kms_frontbuffer_tracking.c
>> +++ b/tests/kms_frontbuffer_tracking.c
>> @@ -858,10 +858,17 @@ static bool psr_wait_until_enabled(void)
>>
>>  static void get_sink_crc(sink_crc_t *crc)
>>  {
>> +       int rc, errno_;
>> +
>>         lseek(sink_crc.fd, 0, SEEK_SET);
>>
>> -       igt_assert(read(sink_crc.fd, crc->data, SINK_CRC_SIZE) ==
>> -                  SINK_CRC_SIZE);
>> +       rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
>> +       errno_ = errno;
>> +
>> +       if (rc == -1 && errno_ == ETIMEDOUT)
>> +               igt_skip("Sink CRC is unreliable on this machine. Try running this test again individually\n");
>> +
>> +       igt_assert(rc == SINK_CRC_SIZE);
>>  }
>>
>>  static bool sink_crc_equal(sink_crc_t *a, sink_crc_t *b)
>> --
>> 2.4.3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] kms_frontbuffer_tracking: Increase the time we wait for PSR.

[Rodrigo Vivi]

On Thu, Nov 5, 2015 at 12:11 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
>> With commit (drm/i915: Delay first PSR activation.) in kernel
>> PSR might take a bit longer to really activate after the modeset.
>
> Can you please expand this commit message a little bit for Future
> Paulo and Future Rodrigo? It would be nice to mention that the timeout
> is going to be 5 times the panel power cycle delay, which is XYZms for
> your machine, so the timeout needs to be a minimum of Xs, so 5s is the
> safe value (we print the panel power cycle delay time on dmesg).

Sure I will expand it.

>
> Also, maybe add a notice that since we do a lot of
> assert(psr_disabled), this commit is increasing the time it takes to
> run the whole set of PSR tests by a few minutes (check commit
> f4db3b18841263f8f617a9f7f0aaf14fab7196d1 for an explanation).

it shouldn't increase that much since igt_wait returns what ever
happens first: psr getting enabled or the time out
and the PSR delayed start is only after the full modeset when psr gets
enabled and these asserts happens many more times from what I saw.

>
> Maybe you could also provide patch 9/8 adding a DONT_ASSERT_PSR_STATUS
> flag and patch 10/8 that changes the callers of op_disables_psr(),
> making them use DONT_ASSERT_PSR_STATUS so we won't eat the full 5s
> timeout during every single GTT mmap test. This will make the time it
> takes to run the full set of PSR tests even smaller than what it is
> today (!!!).

oh I see...  but I really like the assert enabled/disable even taking longer...

Maybe we should pick few tests that doesn't take so long and call them
basic_ to get included on BAT ;)

>
> With the improved commit message: Reviewed-by: Paulo Zanoni
> <paulo.r.zanoni@intel.com>

Anyway I will double check with you again! ;)
Thanks for the review and comments!


>
>>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  tests/kms_frontbuffer_tracking.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
>> index 38ed662..cd2879d 100644
>> --- a/tests/kms_frontbuffer_tracking.c
>> +++ b/tests/kms_frontbuffer_tracking.c
>> @@ -848,7 +848,7 @@ static bool fbc_wait_until_enabled(void)
>>
>>  static bool psr_wait_until_enabled(void)
>>  {
>> -       return igt_wait(psr_is_enabled(), 2000, 1);
>> +       return igt_wait(psr_is_enabled(), 5000, 1);
>>  }
>>
>>  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
>> --
>> 2.4.3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/8] kms_frontbuffer_tracking: Skip on unreliable CRC.

[Daniel Vetter]

On Thu, Nov 05, 2015 at 10:53:28AM -0800, Rodrigo Vivi wrote:
> Even with all sink crc re-works we still have platforms
> where after 6 vblanks it is unable to calculate the
> sink crc. But if we don't get the sink crc it isn't true
> that test failed, but that we have no ways to say test
> passed or failed.
> 
> So let's print a message and move forward in case sink crc
> cannot help us to know if the screen has been updated.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index cd2879d..606d0a9 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -858,10 +858,17 @@ static bool psr_wait_until_enabled(void)
>  
>  static void get_sink_crc(sink_crc_t *crc)
>  {
> +	int rc, errno_;
> +
>  	lseek(sink_crc.fd, 0, SEEK_SET);
>  
> -	igt_assert(read(sink_crc.fd, crc->data, SINK_CRC_SIZE) ==
> -		   SINK_CRC_SIZE);
> +	rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
> +	errno_ = errno;
> +
> +	if (rc == -1 && errno_ == ETIMEDOUT)
> +		igt_skip("Sink CRC is unreliable on this machine. Try running this test again individually\n");

igt_skip_on, just as an aside.
-Daniel

> +
> +	igt_assert(rc == SINK_CRC_SIZE);
>  }
>  
>  static bool sink_crc_equal(sink_crc_t *a, sink_crc_t *b)
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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