Igt-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC
@ 2018-02-28 13:44 Mika Kahola
  2018-02-28 14:34 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_plane_multiple: Drain pipe before reading CRC (rev2) Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Mika Kahola @ 2018-02-28 13:44 UTC (permalink / raw)
  To: igt-dev

In CI runs we every now and then fail to read correct CRC yielding an error
when comparing reference and grabbed CRC's. Let's first fix the test so that
we drain the pipe first and then read the correct CRC. While at it, let's
simplify the test by combining legacy and atomic tests into a one common
function.

v2: We don't need to drain pipe when we grab first CRC

References: https://bugs.freedesktop.org/show_bug.cgi?id=103166
Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 tests/kms_plane_multiple.c | 142 ++++++++++++---------------------------------
 1 file changed, 38 insertions(+), 104 deletions(-)

diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
index 95b7138..ff8ede3 100644
--- a/tests/kms_plane_multiple.c
+++ b/tests/kms_plane_multiple.c
@@ -32,7 +32,6 @@
 
 IGT_TEST_DESCRIPTION("Test atomic mode setting with multiple planes ");
 
-#define MAX_CRCS          2
 #define SIZE_PLANE      256
 #define SIZE_CURSOR     128
 #define LOOP_FOREVER     -1
@@ -46,6 +45,7 @@ typedef struct {
 typedef struct {
 	int drm_fd;
 	igt_display_t display;
+	igt_crc_t ref_crc;
 	igt_pipe_crc_t *pipe_crc;
 	igt_plane_t **plane;
 	struct igt_fb *fb;
@@ -92,20 +92,23 @@ static void test_fini(data_t *data, igt_output_t *output, int n_planes)
 	igt_output_set_pipe(output, PIPE_ANY);
 
 	igt_pipe_crc_free(data->pipe_crc);
+	data->pipe_crc = NULL;
 
 	free(data->plane);
 	data->plane = NULL;
-	free(data->fb);
-	data->fb = NULL;
+
+	igt_remove_fb(data->drm_fd, data->fb);
+
+	igt_display_reset(&data->display);
 }
 
 static void
-test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe, bool atomic,
-	      color_t *color, uint64_t tiling, igt_crc_t **crc /* out */)
+test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe, int commit,
+	      color_t *color, uint64_t tiling)
 {
 	drmModeModeInfo *mode;
 	igt_plane_t *primary;
-	int ret, n;
+	int ret;
 
 	igt_output_set_pipe(output, pipe);
 
@@ -122,13 +125,16 @@ test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe, bool atomic,
 
 	igt_plane_set_fb(data->plane[primary->index], &data->fb[primary->index]);
 
-	ret = igt_display_try_commit2(&data->display,
-				      atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+	ret = igt_display_try_commit2(&data->display, commit);
 	igt_skip_on(ret != 0);
 
-	igt_pipe_crc_start(data->pipe_crc);
-	n = igt_pipe_crc_get_crcs(data->pipe_crc, 1, crc);
-	igt_assert_eq(n, 1);
+	if (commit == COMMIT_LEGACY) {
+		igt_pipe_crc_collect_crc(data->pipe_crc, &data->ref_crc);
+	} else {
+		igt_pipe_crc_start(data->pipe_crc);
+		igt_pipe_crc_drain(data->pipe_crc);
+		igt_pipe_crc_get_single(data->pipe_crc, &data->ref_crc);
+	}
 }
 
 /*
@@ -239,17 +245,13 @@ prepare_planes(data_t *data, enum pipe pipe_id, color_t *color,
 }
 
 static void
-test_atomic_plane_position_with_output(data_t *data, enum pipe pipe,
-				       igt_output_t *output, int n_planes,
-				       uint64_t tiling)
+test_plane_position_with_output(data_t *data, enum pipe pipe,
+				igt_output_t *output, int n_planes,
+				uint64_t tiling, int commit)
 {
-	char buf[256];
-	struct drm_event *e = (void *)buf;
 	color_t blue  = { 0.0f, 0.0f, 1.0f };
-	igt_crc_t *ref = NULL;
-	igt_crc_t *crc = NULL;
-	unsigned int vblank_start, vblank_stop;
-	int i, n, ret;
+	igt_crc_t crc;
+	int i;
 	int iterations = opt.iterations < 1 ? 1 : opt.iterations;
 	bool loop_forever;
 	char info[256];
@@ -269,89 +271,27 @@ test_atomic_plane_position_with_output(data_t *data, enum pipe pipe,
 
 	test_init(data, pipe, n_planes);
 
-	test_grab_crc(data, output, pipe, true, &blue, tiling, &ref);
+	test_grab_crc(data, output, pipe, commit, &blue, tiling);
 
 	i = 0;
 	while (i < iterations || loop_forever) {
 		prepare_planes(data, pipe, &blue, tiling, n_planes, output);
 
-		vblank_start = kmstest_get_vblank(data->display.drm_fd, pipe,
-						  DRM_VBLANK_NEXTONMISS);
+		igt_display_commit2(&data->display, commit);
 
-		igt_display_commit_atomic(&data->display,
-					  DRM_MODE_PAGE_FLIP_EVENT,
-					  &data->display);
-
-		igt_set_timeout(1, "Stuck on page flip");
-
-		ret = read(data->display.drm_fd, buf, sizeof(buf));
-		igt_assert(ret >= 0);
-
-		vblank_stop = kmstest_get_vblank(data->display.drm_fd, pipe, 0);
-		igt_assert_eq(e->type, DRM_EVENT_FLIP_COMPLETE);
-		igt_reset_timeout();
-
-		n = igt_pipe_crc_get_crcs(data->pipe_crc, vblank_stop - vblank_start, &crc);
-
-		igt_assert(vblank_stop - vblank_start <= MAX_CRCS);
-		igt_assert_eq(n, vblank_stop - vblank_start);
-
-		igt_assert_crc_equal(ref, crc);
-
-		i++;
-	}
-
-	igt_pipe_crc_stop(data->pipe_crc);
-
-	test_fini(data, output, n_planes);
-}
-
-static void
-test_legacy_plane_position_with_output(data_t *data, enum pipe pipe,
-				       igt_output_t *output, int n_planes,
-				       uint64_t tiling)
-{
-	color_t blue  = { 0.0f, 0.0f, 1.0f };
-	igt_crc_t *ref = NULL;
-	igt_crc_t *crc = NULL;
-	int i, n;
-	int iterations = opt.iterations < 1 ? 1 : opt.iterations;
-	bool loop_forever;
-	char info[256];
-
-	if (opt.iterations == LOOP_FOREVER) {
-		loop_forever = true;
-		sprintf(info, "forever");
-	} else {
-		loop_forever = false;
-		sprintf(info, "for %d %s",
-			iterations, iterations > 1 ? "iterations" : "iteration");
-	}
-
-	igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
-		 igt_output_name(output), kmstest_pipe_name(pipe), n_planes,
-		 info, opt.seed);
-
-	test_init(data, pipe, n_planes);
-
-	test_grab_crc(data, output, pipe, false, &blue, tiling, &ref);
-
-	i = 0;
-	while (i < iterations || loop_forever) {
-		prepare_planes(data, pipe, &blue, tiling, n_planes, output);
-
-		igt_display_commit2(&data->display, COMMIT_LEGACY);
-
-		n = igt_pipe_crc_get_crcs(data->pipe_crc, MAX_CRCS, &crc);
-
-		igt_assert_eq(n, MAX_CRCS);
-
-		igt_assert_crc_equal(ref, crc);
+		if (commit == COMMIT_LEGACY) {
+			igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
+		} else {
+			igt_pipe_crc_drain(data->pipe_crc);
+			igt_pipe_crc_get_single(data->pipe_crc, &crc);
+		}
+		igt_assert_crc_equal(&data->ref_crc, &crc);
 
 		i++;
 	}
 
-	igt_pipe_crc_stop(data->pipe_crc);
+	if (commit != COMMIT_LEGACY)
+		igt_pipe_crc_stop(data->pipe_crc);
 
 	test_fini(data, output, n_planes);
 }
@@ -378,17 +318,11 @@ test_plane_position(data_t *data, enum pipe pipe, bool atomic, uint64_t tiling)
 
 	connected_outs = 0;
 	for_each_valid_output_on_pipe(&data->display, pipe, output) {
-		if (atomic)
-			test_atomic_plane_position_with_output(data, pipe,
-							       output,
-							       n_planes,
-							       tiling);
-		else
-			test_legacy_plane_position_with_output(data, pipe,
-							       output,
-							       n_planes,
-							       tiling);
-
+		test_plane_position_with_output(data, pipe,
+						output,
+						n_planes,
+						tiling,
+						atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
 		connected_outs++;
 	}
 
-- 
2.7.4

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_plane_multiple: Drain pipe before reading CRC (rev2)
  2018-02-28 13:44 [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC Mika Kahola
@ 2018-02-28 14:34 ` Patchwork
  2018-02-28 18:47 ` [igt-dev] ✗ Fi.CI.IGT: warning " Patchwork
  2018-03-09  8:40 ` [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC Maarten Lankhorst
  2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2018-02-28 14:34 UTC (permalink / raw)
  To: Mika Kahola; +Cc: igt-dev

== Series Details ==

Series: tests/kms_plane_multiple: Drain pipe before reading CRC (rev2)
URL   : https://patchwork.freedesktop.org/series/38851/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
0d47ec161b4eca1b41c5348604aa05b105e5d1cf tests/perf: Fix build warning

with latest DRM-Tip kernel build CI_DRM_3848
446c2101a788 drm-tip: 2018y-02m-28d-13h-18m-03s UTC integration manifest

No testlist changes.

---- Known issues:

Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> FAIL       (fi-cnl-y3) fdo#103167
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-bdw-5557u) fdo#104162

fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#104162 https://bugs.freedesktop.org/show_bug.cgi?id=104162

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:415s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:375s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:491s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:288s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:481s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:468s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:465s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:395s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:564s
fi-cnl-y3        total:288  pass:261  dwarn:0   dfail:0   fail:1   skip:26  time:584s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:409s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:285s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:509s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:387s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:414s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:453s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:411s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:451s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:494s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:497s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:584s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:434s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:499s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:519s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:491s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:485s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:413s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:433s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:523s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:397s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1023/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* [igt-dev] ✗ Fi.CI.IGT: warning for tests/kms_plane_multiple: Drain pipe before reading CRC (rev2)
  2018-02-28 13:44 [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC Mika Kahola
  2018-02-28 14:34 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_plane_multiple: Drain pipe before reading CRC (rev2) Patchwork
@ 2018-02-28 18:47 ` Patchwork
  2018-03-09  8:40 ` [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC Maarten Lankhorst
  2 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2018-02-28 18:47 UTC (permalink / raw)
  To: Mika Kahola; +Cc: igt-dev

== Series Details ==

Series: tests/kms_plane_multiple: Drain pipe before reading CRC (rev2)
URL   : https://patchwork.freedesktop.org/series/38851/
State : warning

== Summary ==

---- Possible new issues:

Test gem_pwrite:
        Subgroup big-gtt-backwards:
                pass       -> SKIP       (shard-apl)
Test kms_chv_cursor_fail:
        Subgroup pipe-b-256x256-bottom-edge:
                dmesg-warn -> PASS       (shard-snb)
Test pm_rc6_residency:
        Subgroup rc6-accuracy:
                pass       -> SKIP       (shard-snb)

---- Known issues:

Test gem_eio:
        Subgroup in-flight:
                incomplete -> PASS       (shard-apl) fdo#104945
Test kms_chv_cursor_fail:
        Subgroup pipe-b-128x128-right-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185
Test kms_rotation_crc:
        Subgroup primary-rotation-180:
                pass       -> FAIL       (shard-snb) fdo#103925
Test kms_sysfs_edid_timing:
                warn       -> PASS       (shard-apl) fdo#100047
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (shard-apl) fdo#104008

fdo#104945 https://bugs.freedesktop.org/show_bug.cgi?id=104945
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

shard-apl        total:3460 pass:1818 dwarn:1   dfail:0   fail:8   skip:1633 time:12206s
shard-hsw        total:3460 pass:1767 dwarn:1   dfail:0   fail:1   skip:1690 time:11585s
shard-snb        total:3460 pass:1358 dwarn:1   dfail:0   fail:2   skip:2099 time:6675s
Blacklisted hosts:
shard-kbl        total:3460 pass:1944 dwarn:1   dfail:0   fail:7   skip:1508 time:9601s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1023/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC
  2018-02-28 13:44 [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC Mika Kahola
  2018-02-28 14:34 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_plane_multiple: Drain pipe before reading CRC (rev2) Patchwork
  2018-02-28 18:47 ` [igt-dev] ✗ Fi.CI.IGT: warning " Patchwork
@ 2018-03-09  8:40 ` Maarten Lankhorst
  2018-03-09 11:25   ` Mika Kahola
  2 siblings, 1 reply; 7+ messages in thread
From: Maarten Lankhorst @ 2018-03-09  8:40 UTC (permalink / raw)
  To: Mika Kahola, igt-dev

Op 28-02-18 om 14:44 schreef Mika Kahola:
> In CI runs we every now and then fail to read correct CRC yielding an error
> when comparing reference and grabbed CRC's. Let's first fix the test so that
> we drain the pipe first and then read the correct CRC. While at it, let's
> simplify the test by combining legacy and atomic tests into a one common
> function.
>
> v2: We don't need to drain pipe when we grab first CRC
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=103166
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  tests/kms_plane_multiple.c | 142 ++++++++++++---------------------------------
>  1 file changed, 38 insertions(+), 104 deletions(-)
>
> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
> index 95b7138..ff8ede3 100644
> --- a/tests/kms_plane_multiple.c
> +++ b/tests/kms_plane_multiple.c
> @@ -32,7 +32,6 @@
>  
>  IGT_TEST_DESCRIPTION("Test atomic mode setting with multiple planes ");
>  
> -#define MAX_CRCS          2
>  #define SIZE_PLANE      256
>  #define SIZE_CURSOR     128
>  #define LOOP_FOREVER     -1
> @@ -46,6 +45,7 @@ typedef struct {
>  typedef struct {
>  	int drm_fd;
>  	igt_display_t display;
> +	igt_crc_t ref_crc;
>  	igt_pipe_crc_t *pipe_crc;
>  	igt_plane_t **plane;
>  	struct igt_fb *fb;
> @@ -92,20 +92,23 @@ static void test_fini(data_t *data, igt_output_t *output, int n_planes)
>  	igt_output_set_pipe(output, PIPE_ANY);
>  
>  	igt_pipe_crc_free(data->pipe_crc);
> +	data->pipe_crc = NULL;
>  
>  	free(data->plane);
>  	data->plane = NULL;
> -	free(data->fb);
> -	data->fb = NULL;
> +
> +	igt_remove_fb(data->drm_fd, data->fb);
> +
> +	igt_display_reset(&data->display);
>  }
>  
>  static void
> -test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe, bool atomic,
> -	      color_t *color, uint64_t tiling, igt_crc_t **crc /* out */)
> +test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe, int commit,
> +	      color_t *color, uint64_t tiling)
>  {
>  	drmModeModeInfo *mode;
>  	igt_plane_t *primary;
> -	int ret, n;
> +	int ret;
>  
>  	igt_output_set_pipe(output, pipe);
>  
> @@ -122,13 +125,16 @@ test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe, bool atomic,
>  
>  	igt_plane_set_fb(data->plane[primary->index], &data->fb[primary->index]);
>  
> -	ret = igt_display_try_commit2(&data->display,
> -				      atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> +	ret = igt_display_try_commit2(&data->display, commit);
>  	igt_skip_on(ret != 0);
>  
> -	igt_pipe_crc_start(data->pipe_crc);
> -	n = igt_pipe_crc_get_crcs(data->pipe_crc, 1, crc);
> -	igt_assert_eq(n, 1);
> +	if (commit == COMMIT_LEGACY) {
> +		igt_pipe_crc_collect_crc(data->pipe_crc, &data->ref_crc);
> +	} else {
> +		igt_pipe_crc_start(data->pipe_crc);
> +		igt_pipe_crc_drain(data->pipe_crc);
> +		igt_pipe_crc_get_single(data->pipe_crc, &data->ref_crc);
> +	}
Why the 2 different paths?

And again, after pipe_crc_start no _drain() is needed, we already have a good CRC.
>  }
>  
>  /*
> @@ -239,17 +245,13 @@ prepare_planes(data_t *data, enum pipe pipe_id, color_t *color,
>  }
>  
>  static void
> -test_atomic_plane_position_with_output(data_t *data, enum pipe pipe,
> -				       igt_output_t *output, int n_planes,
> -				       uint64_t tiling)
> +test_plane_position_with_output(data_t *data, enum pipe pipe,
> +				igt_output_t *output, int n_planes,
> +				uint64_t tiling, int commit)
>  {
> -	char buf[256];
> -	struct drm_event *e = (void *)buf;
>  	color_t blue  = { 0.0f, 0.0f, 1.0f };
> -	igt_crc_t *ref = NULL;
> -	igt_crc_t *crc = NULL;
> -	unsigned int vblank_start, vblank_stop;
> -	int i, n, ret;
> +	igt_crc_t crc;
> +	int i;
>  	int iterations = opt.iterations < 1 ? 1 : opt.iterations;
>  	bool loop_forever;
>  	char info[256];
> @@ -269,89 +271,27 @@ test_atomic_plane_position_with_output(data_t *data, enum pipe pipe,
>  
>  	test_init(data, pipe, n_planes);
>  
> -	test_grab_crc(data, output, pipe, true, &blue, tiling, &ref);
> +	test_grab_crc(data, output, pipe, commit, &blue, tiling);
>  
>  	i = 0;
>  	while (i < iterations || loop_forever) {
>  		prepare_planes(data, pipe, &blue, tiling, n_planes, output);
>  
> -		vblank_start = kmstest_get_vblank(data->display.drm_fd, pipe,
> -						  DRM_VBLANK_NEXTONMISS);
> +		igt_display_commit2(&data->display, commit);
>  
> -		igt_display_commit_atomic(&data->display,
> -					  DRM_MODE_PAGE_FLIP_EVENT,
> -					  &data->display);
> -
> -		igt_set_timeout(1, "Stuck on page flip");
> -
> -		ret = read(data->display.drm_fd, buf, sizeof(buf));
> -		igt_assert(ret >= 0);
> -
> -		vblank_stop = kmstest_get_vblank(data->display.drm_fd, pipe, 0);
> -		igt_assert_eq(e->type, DRM_EVENT_FLIP_COMPLETE);
> -		igt_reset_timeout();
> -
> -		n = igt_pipe_crc_get_crcs(data->pipe_crc, vblank_stop - vblank_start, &crc);
> -
> -		igt_assert(vblank_stop - vblank_start <= MAX_CRCS);
> -		igt_assert_eq(n, vblank_stop - vblank_start);
> -
> -		igt_assert_crc_equal(ref, crc);
> -
> -		i++;
> -	}
> -
> -	igt_pipe_crc_stop(data->pipe_crc);
> -
> -	test_fini(data, output, n_planes);
> -}
> -
> -static void
> -test_legacy_plane_position_with_output(data_t *data, enum pipe pipe,
> -				       igt_output_t *output, int n_planes,
> -				       uint64_t tiling)
> -{
> -	color_t blue  = { 0.0f, 0.0f, 1.0f };
> -	igt_crc_t *ref = NULL;
> -	igt_crc_t *crc = NULL;
> -	int i, n;
> -	int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> -	bool loop_forever;
> -	char info[256];
> -
> -	if (opt.iterations == LOOP_FOREVER) {
> -		loop_forever = true;
> -		sprintf(info, "forever");
> -	} else {
> -		loop_forever = false;
> -		sprintf(info, "for %d %s",
> -			iterations, iterations > 1 ? "iterations" : "iteration");
> -	}
> -
> -	igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
> -		 igt_output_name(output), kmstest_pipe_name(pipe), n_planes,
> -		 info, opt.seed);
> -
> -	test_init(data, pipe, n_planes);
> -
> -	test_grab_crc(data, output, pipe, false, &blue, tiling, &ref);
> -
> -	i = 0;
> -	while (i < iterations || loop_forever) {
> -		prepare_planes(data, pipe, &blue, tiling, n_planes, output);
> -
> -		igt_display_commit2(&data->display, COMMIT_LEGACY);
> -
> -		n = igt_pipe_crc_get_crcs(data->pipe_crc, MAX_CRCS, &crc);
> -
> -		igt_assert_eq(n, MAX_CRCS);
> -
> -		igt_assert_crc_equal(ref, crc);
> +		if (commit == COMMIT_LEGACY) {
> +			igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
> +		} else {
> +			igt_pipe_crc_drain(data->pipe_crc);
> +			igt_pipe_crc_get_single(data->pipe_crc, &crc);
> +		}
Again why the separate paths?
> +		igt_assert_crc_equal(&data->ref_crc, &crc);
>  
>  		i++;
>  	}
>  
> -	igt_pipe_crc_stop(data->pipe_crc);
> +	if (commit != COMMIT_LEGACY)
> +		igt_pipe_crc_stop(data->pipe_crc);
>  
>  	test_fini(data, output, n_planes);
>  }
> @@ -378,17 +318,11 @@ test_plane_position(data_t *data, enum pipe pipe, bool atomic, uint64_t tiling)
>  
>  	connected_outs = 0;
>  	for_each_valid_output_on_pipe(&data->display, pipe, output) {
> -		if (atomic)
> -			test_atomic_plane_position_with_output(data, pipe,
> -							       output,
> -							       n_planes,
> -							       tiling);
> -		else
> -			test_legacy_plane_position_with_output(data, pipe,
> -							       output,
> -							       n_planes,
> -							       tiling);
> -
> +		test_plane_position_with_output(data, pipe,
> +						output,
> +						n_planes,
> +						tiling,
> +						atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
>  		connected_outs++;
>  	}
>  


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC
  2018-03-09  8:40 ` [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC Maarten Lankhorst
@ 2018-03-09 11:25   ` Mika Kahola
  2018-03-09 11:34     ` Maarten Lankhorst
  0 siblings, 1 reply; 7+ messages in thread
From: Mika Kahola @ 2018-03-09 11:25 UTC (permalink / raw)
  To: Maarten Lankhorst, igt-dev

On Fri, 2018-03-09 at 09:40 +0100, Maarten Lankhorst wrote:
> Op 28-02-18 om 14:44 schreef Mika Kahola:
> > 
> > In CI runs we every now and then fail to read correct CRC yielding
> > an error
> > when comparing reference and grabbed CRC's. Let's first fix the
> > test so that
> > we drain the pipe first and then read the correct CRC. While at it,
> > let's
> > simplify the test by combining legacy and atomic tests into a one
> > common
> > function.
> > 
> > v2: We don't need to drain pipe when we grab first CRC
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=103166
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  tests/kms_plane_multiple.c | 142 ++++++++++++---------------------
> > ------------
> >  1 file changed, 38 insertions(+), 104 deletions(-)
> > 
> > diff --git a/tests/kms_plane_multiple.c
> > b/tests/kms_plane_multiple.c
> > index 95b7138..ff8ede3 100644
> > --- a/tests/kms_plane_multiple.c
> > +++ b/tests/kms_plane_multiple.c
> > @@ -32,7 +32,6 @@
> >  
> >  IGT_TEST_DESCRIPTION("Test atomic mode setting with multiple
> > planes ");
> >  
> > -#define MAX_CRCS          2
> >  #define SIZE_PLANE      256
> >  #define SIZE_CURSOR     128
> >  #define LOOP_FOREVER     -1
> > @@ -46,6 +45,7 @@ typedef struct {
> >  typedef struct {
> >  	int drm_fd;
> >  	igt_display_t display;
> > +	igt_crc_t ref_crc;
> >  	igt_pipe_crc_t *pipe_crc;
> >  	igt_plane_t **plane;
> >  	struct igt_fb *fb;
> > @@ -92,20 +92,23 @@ static void test_fini(data_t *data,
> > igt_output_t *output, int n_planes)
> >  	igt_output_set_pipe(output, PIPE_ANY);
> >  
> >  	igt_pipe_crc_free(data->pipe_crc);
> > +	data->pipe_crc = NULL;
> >  
> >  	free(data->plane);
> >  	data->plane = NULL;
> > -	free(data->fb);
> > -	data->fb = NULL;
> > +
> > +	igt_remove_fb(data->drm_fd, data->fb);
> > +
> > +	igt_display_reset(&data->display);
> >  }
> >  
> >  static void
> > -test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
> > bool atomic,
> > -	      color_t *color, uint64_t tiling, igt_crc_t **crc /*
> > out */)
> > +test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
> > int commit,
> > +	      color_t *color, uint64_t tiling)
> >  {
> >  	drmModeModeInfo *mode;
> >  	igt_plane_t *primary;
> > -	int ret, n;
> > +	int ret;
> >  
> >  	igt_output_set_pipe(output, pipe);
> >  
> > @@ -122,13 +125,16 @@ test_grab_crc(data_t *data, igt_output_t
> > *output, enum pipe pipe, bool atomic,
> >  
> >  	igt_plane_set_fb(data->plane[primary->index], &data-
> > >fb[primary->index]);
> >  
> > -	ret = igt_display_try_commit2(&data->display,
> > -				      atomic ? COMMIT_ATOMIC :
> > COMMIT_LEGACY);
> > +	ret = igt_display_try_commit2(&data->display, commit);
> >  	igt_skip_on(ret != 0);
> >  
> > -	igt_pipe_crc_start(data->pipe_crc);
> > -	n = igt_pipe_crc_get_crcs(data->pipe_crc, 1, crc);
> > -	igt_assert_eq(n, 1);
> > +	if (commit == COMMIT_LEGACY) {
> > +		igt_pipe_crc_collect_crc(data->pipe_crc, &data-
> > >ref_crc);
> > +	} else {
> > +		igt_pipe_crc_start(data->pipe_crc);
> > +		igt_pipe_crc_drain(data->pipe_crc);
> > +		igt_pipe_crc_get_single(data->pipe_crc, &data-
> > >ref_crc);
> > +	}
> Why the 2 different paths?
The *_start *_get_single method didn't work for COMMIT_ATOMIC so I
decided to use legacy way for COMMIT_LEGACY. Maybe we could after all
drop the legacy commits from this test. Originally, this test was all
about atomic tests. 
> 
> And again, after pipe_crc_start no _drain() is needed, we already
> have a good CRC.
Yeah, this thing is an echo from the past. I remember we already
discussed this when previous patch was under review.

Thanks for the review!

> > 
> >  }
> >  
> >  /*
> > @@ -239,17 +245,13 @@ prepare_planes(data_t *data, enum pipe
> > pipe_id, color_t *color,
> >  }
> >  
> >  static void
> > -test_atomic_plane_position_with_output(data_t *data, enum pipe
> > pipe,
> > -				       igt_output_t *output, int
> > n_planes,
> > -				       uint64_t tiling)
> > +test_plane_position_with_output(data_t *data, enum pipe pipe,
> > +				igt_output_t *output, int
> > n_planes,
> > +				uint64_t tiling, int commit)
> >  {
> > -	char buf[256];
> > -	struct drm_event *e = (void *)buf;
> >  	color_t blue  = { 0.0f, 0.0f, 1.0f };
> > -	igt_crc_t *ref = NULL;
> > -	igt_crc_t *crc = NULL;
> > -	unsigned int vblank_start, vblank_stop;
> > -	int i, n, ret;
> > +	igt_crc_t crc;
> > +	int i;
> >  	int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> >  	bool loop_forever;
> >  	char info[256];
> > @@ -269,89 +271,27 @@ test_atomic_plane_position_with_output(data_t
> > *data, enum pipe pipe,
> >  
> >  	test_init(data, pipe, n_planes);
> >  
> > -	test_grab_crc(data, output, pipe, true, &blue, tiling,
> > &ref);
> > +	test_grab_crc(data, output, pipe, commit, &blue, tiling);
> >  
> >  	i = 0;
> >  	while (i < iterations || loop_forever) {
> >  		prepare_planes(data, pipe, &blue, tiling,
> > n_planes, output);
> >  
> > -		vblank_start = kmstest_get_vblank(data-
> > >display.drm_fd, pipe,
> > -						  DRM_VBLANK_NEXTO
> > NMISS);
> > +		igt_display_commit2(&data->display, commit);
> >  
> > -		igt_display_commit_atomic(&data->display,
> > -					  DRM_MODE_PAGE_FLIP_EVENT
> > ,
> > -					  &data->display);
> > -
> > -		igt_set_timeout(1, "Stuck on page flip");
> > -
> > -		ret = read(data->display.drm_fd, buf,
> > sizeof(buf));
> > -		igt_assert(ret >= 0);
> > -
> > -		vblank_stop = kmstest_get_vblank(data-
> > >display.drm_fd, pipe, 0);
> > -		igt_assert_eq(e->type, DRM_EVENT_FLIP_COMPLETE);
> > -		igt_reset_timeout();
> > -
> > -		n = igt_pipe_crc_get_crcs(data->pipe_crc,
> > vblank_stop - vblank_start, &crc);
> > -
> > -		igt_assert(vblank_stop - vblank_start <=
> > MAX_CRCS);
> > -		igt_assert_eq(n, vblank_stop - vblank_start);
> > -
> > -		igt_assert_crc_equal(ref, crc);
> > -
> > -		i++;
> > -	}
> > -
> > -	igt_pipe_crc_stop(data->pipe_crc);
> > -
> > -	test_fini(data, output, n_planes);
> > -}
> > -
> > -static void
> > -test_legacy_plane_position_with_output(data_t *data, enum pipe
> > pipe,
> > -				       igt_output_t *output, int
> > n_planes,
> > -				       uint64_t tiling)
> > -{
> > -	color_t blue  = { 0.0f, 0.0f, 1.0f };
> > -	igt_crc_t *ref = NULL;
> > -	igt_crc_t *crc = NULL;
> > -	int i, n;
> > -	int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> > -	bool loop_forever;
> > -	char info[256];
> > -
> > -	if (opt.iterations == LOOP_FOREVER) {
> > -		loop_forever = true;
> > -		sprintf(info, "forever");
> > -	} else {
> > -		loop_forever = false;
> > -		sprintf(info, "for %d %s",
> > -			iterations, iterations > 1 ? "iterations"
> > : "iteration");
> > -	}
> > -
> > -	igt_info("Testing connector %s using pipe %s with %d
> > planes %s with seed %d\n",
> > -		 igt_output_name(output), kmstest_pipe_name(pipe),
> > n_planes,
> > -		 info, opt.seed);
> > -
> > -	test_init(data, pipe, n_planes);
> > -
> > -	test_grab_crc(data, output, pipe, false, &blue, tiling,
> > &ref);
> > -
> > -	i = 0;
> > -	while (i < iterations || loop_forever) {
> > -		prepare_planes(data, pipe, &blue, tiling,
> > n_planes, output);
> > -
> > -		igt_display_commit2(&data->display,
> > COMMIT_LEGACY);
> > -
> > -		n = igt_pipe_crc_get_crcs(data->pipe_crc,
> > MAX_CRCS, &crc);
> > -
> > -		igt_assert_eq(n, MAX_CRCS);
> > -
> > -		igt_assert_crc_equal(ref, crc);
> > +		if (commit == COMMIT_LEGACY) {
> > +			igt_pipe_crc_collect_crc(data->pipe_crc,
> > &crc);
> > +		} else {
> > +			igt_pipe_crc_drain(data->pipe_crc);
> > +			igt_pipe_crc_get_single(data->pipe_crc,
> > &crc);
> > +		}
> Again why the separate paths?
> > 
> > +		igt_assert_crc_equal(&data->ref_crc, &crc);
> >  
> >  		i++;
> >  	}
> >  
> > -	igt_pipe_crc_stop(data->pipe_crc);
> > +	if (commit != COMMIT_LEGACY)
> > +		igt_pipe_crc_stop(data->pipe_crc);
> >  
> >  	test_fini(data, output, n_planes);
> >  }
> > @@ -378,17 +318,11 @@ test_plane_position(data_t *data, enum pipe
> > pipe, bool atomic, uint64_t tiling)
> >  
> >  	connected_outs = 0;
> >  	for_each_valid_output_on_pipe(&data->display, pipe,
> > output) {
> > -		if (atomic)
> > -			test_atomic_plane_position_with_output(dat
> > a, pipe,
> > -							       out
> > put,
> > -							       n_p
> > lanes,
> > -							       til
> > ing);
> > -		else
> > -			test_legacy_plane_position_with_output(dat
> > a, pipe,
> > -							       out
> > put,
> > -							       n_p
> > lanes,
> > -							       til
> > ing);
> > -
> > +		test_plane_position_with_output(data, pipe,
> > +						output,
> > +						n_planes,
> > +						tiling,
> > +						atomic ?
> > COMMIT_ATOMIC : COMMIT_LEGACY);
> >  		connected_outs++;
> >  	}
> >  
> 
-- 
Mika Kahola - Intel OTC

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC
  2018-03-09 11:25   ` Mika Kahola
@ 2018-03-09 11:34     ` Maarten Lankhorst
  2018-03-09 12:08       ` Mika Kahola
  0 siblings, 1 reply; 7+ messages in thread
From: Maarten Lankhorst @ 2018-03-09 11:34 UTC (permalink / raw)
  To: mika.kahola, igt-dev

Op 09-03-18 om 12:25 schreef Mika Kahola:
> On Fri, 2018-03-09 at 09:40 +0100, Maarten Lankhorst wrote:
>> Op 28-02-18 om 14:44 schreef Mika Kahola:
>>> In CI runs we every now and then fail to read correct CRC yielding
>>> an error
>>> when comparing reference and grabbed CRC's. Let's first fix the
>>> test so that
>>> we drain the pipe first and then read the correct CRC. While at it,
>>> let's
>>> simplify the test by combining legacy and atomic tests into a one
>>> common
>>> function.
>>>
>>> v2: We don't need to drain pipe when we grab first CRC
>>>
>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=103166
>>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>>> ---
>>>  tests/kms_plane_multiple.c | 142 ++++++++++++---------------------
>>> ------------
>>>  1 file changed, 38 insertions(+), 104 deletions(-)
>>>
>>> diff --git a/tests/kms_plane_multiple.c
>>> b/tests/kms_plane_multiple.c
>>> index 95b7138..ff8ede3 100644
>>> --- a/tests/kms_plane_multiple.c
>>> +++ b/tests/kms_plane_multiple.c
>>> @@ -32,7 +32,6 @@
>>>  
>>>  IGT_TEST_DESCRIPTION("Test atomic mode setting with multiple
>>> planes ");
>>>  
>>> -#define MAX_CRCS          2
>>>  #define SIZE_PLANE      256
>>>  #define SIZE_CURSOR     128
>>>  #define LOOP_FOREVER     -1
>>> @@ -46,6 +45,7 @@ typedef struct {
>>>  typedef struct {
>>>  	int drm_fd;
>>>  	igt_display_t display;
>>> +	igt_crc_t ref_crc;
>>>  	igt_pipe_crc_t *pipe_crc;
>>>  	igt_plane_t **plane;
>>>  	struct igt_fb *fb;
>>> @@ -92,20 +92,23 @@ static void test_fini(data_t *data,
>>> igt_output_t *output, int n_planes)
>>>  	igt_output_set_pipe(output, PIPE_ANY);
>>>  
>>>  	igt_pipe_crc_free(data->pipe_crc);
>>> +	data->pipe_crc = NULL;
>>>  
>>>  	free(data->plane);
>>>  	data->plane = NULL;
>>> -	free(data->fb);
>>> -	data->fb = NULL;
>>> +
>>> +	igt_remove_fb(data->drm_fd, data->fb);
>>> +
>>> +	igt_display_reset(&data->display);
>>>  }
>>>  
>>>  static void
>>> -test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
>>> bool atomic,
>>> -	      color_t *color, uint64_t tiling, igt_crc_t **crc /*
>>> out */)
>>> +test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
>>> int commit,
>>> +	      color_t *color, uint64_t tiling)
>>>  {
>>>  	drmModeModeInfo *mode;
>>>  	igt_plane_t *primary;
>>> -	int ret, n;
>>> +	int ret;
>>>  
>>>  	igt_output_set_pipe(output, pipe);
>>>  
>>> @@ -122,13 +125,16 @@ test_grab_crc(data_t *data, igt_output_t
>>> *output, enum pipe pipe, bool atomic,
>>>  
>>>  	igt_plane_set_fb(data->plane[primary->index], &data-
>>>> fb[primary->index]);
>>>  
>>> -	ret = igt_display_try_commit2(&data->display,
>>> -				      atomic ? COMMIT_ATOMIC :
>>> COMMIT_LEGACY);
>>> +	ret = igt_display_try_commit2(&data->display, commit);
>>>  	igt_skip_on(ret != 0);
>>>  
>>> -	igt_pipe_crc_start(data->pipe_crc);
>>> -	n = igt_pipe_crc_get_crcs(data->pipe_crc, 1, crc);
>>> -	igt_assert_eq(n, 1);
>>> +	if (commit == COMMIT_LEGACY) {
>>> +		igt_pipe_crc_collect_crc(data->pipe_crc, &data-
>>>> ref_crc);
>>> +	} else {
>>> +		igt_pipe_crc_start(data->pipe_crc);
>>> +		igt_pipe_crc_drain(data->pipe_crc);
>>> +		igt_pipe_crc_get_single(data->pipe_crc, &data-
>>>> ref_crc);
>>> +	}
>> Why the 2 different paths?
> The *_start *_get_single method didn't work for COMMIT_ATOMIC so I
> decided to use legacy way for COMMIT_LEGACY. Maybe we could after all
> drop the legacy commits from this test. Originally, this test was all
> about atomic tests. 
>> And again, after pipe_crc_start no _drain() is needed, we already
>> have a good CRC.
> Yeah, this thing is an echo from the past. I remember we already
> discussed this when previous patch was under review.
>
> Thanks for the review!
Just nuke the legacy paths then. :)
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC
  2018-03-09 11:34     ` Maarten Lankhorst
@ 2018-03-09 12:08       ` Mika Kahola
  0 siblings, 0 replies; 7+ messages in thread
From: Mika Kahola @ 2018-03-09 12:08 UTC (permalink / raw)
  To: Maarten Lankhorst, igt-dev

On Fri, 2018-03-09 at 12:34 +0100, Maarten Lankhorst wrote:
> Op 09-03-18 om 12:25 schreef Mika Kahola:
> > 
> > On Fri, 2018-03-09 at 09:40 +0100, Maarten Lankhorst wrote:
> > > 
> > > Op 28-02-18 om 14:44 schreef Mika Kahola:
> > > > 
> > > > In CI runs we every now and then fail to read correct CRC
> > > > yielding
> > > > an error
> > > > when comparing reference and grabbed CRC's. Let's first fix the
> > > > test so that
> > > > we drain the pipe first and then read the correct CRC. While at
> > > > it,
> > > > let's
> > > > simplify the test by combining legacy and atomic tests into a
> > > > one
> > > > common
> > > > function.
> > > > 
> > > > v2: We don't need to drain pipe when we grab first CRC
> > > > 
> > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=103166
> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > ---
> > > >  tests/kms_plane_multiple.c | 142 ++++++++++++-----------------
> > > > ----
> > > > ------------
> > > >  1 file changed, 38 insertions(+), 104 deletions(-)
> > > > 
> > > > diff --git a/tests/kms_plane_multiple.c
> > > > b/tests/kms_plane_multiple.c
> > > > index 95b7138..ff8ede3 100644
> > > > --- a/tests/kms_plane_multiple.c
> > > > +++ b/tests/kms_plane_multiple.c
> > > > @@ -32,7 +32,6 @@
> > > >  
> > > >  IGT_TEST_DESCRIPTION("Test atomic mode setting with multiple
> > > > planes ");
> > > >  
> > > > -#define MAX_CRCS          2
> > > >  #define SIZE_PLANE      256
> > > >  #define SIZE_CURSOR     128
> > > >  #define LOOP_FOREVER     -1
> > > > @@ -46,6 +45,7 @@ typedef struct {
> > > >  typedef struct {
> > > >  	int drm_fd;
> > > >  	igt_display_t display;
> > > > +	igt_crc_t ref_crc;
> > > >  	igt_pipe_crc_t *pipe_crc;
> > > >  	igt_plane_t **plane;
> > > >  	struct igt_fb *fb;
> > > > @@ -92,20 +92,23 @@ static void test_fini(data_t *data,
> > > > igt_output_t *output, int n_planes)
> > > >  	igt_output_set_pipe(output, PIPE_ANY);
> > > >  
> > > >  	igt_pipe_crc_free(data->pipe_crc);
> > > > +	data->pipe_crc = NULL;
> > > >  
> > > >  	free(data->plane);
> > > >  	data->plane = NULL;
> > > > -	free(data->fb);
> > > > -	data->fb = NULL;
> > > > +
> > > > +	igt_remove_fb(data->drm_fd, data->fb);
> > > > +
> > > > +	igt_display_reset(&data->display);
> > > >  }
> > > >  
> > > >  static void
> > > > -test_grab_crc(data_t *data, igt_output_t *output, enum pipe
> > > > pipe,
> > > > bool atomic,
> > > > -	      color_t *color, uint64_t tiling, igt_crc_t **crc
> > > > /*
> > > > out */)
> > > > +test_grab_crc(data_t *data, igt_output_t *output, enum pipe
> > > > pipe,
> > > > int commit,
> > > > +	      color_t *color, uint64_t tiling)
> > > >  {
> > > >  	drmModeModeInfo *mode;
> > > >  	igt_plane_t *primary;
> > > > -	int ret, n;
> > > > +	int ret;
> > > >  
> > > >  	igt_output_set_pipe(output, pipe);
> > > >  
> > > > @@ -122,13 +125,16 @@ test_grab_crc(data_t *data, igt_output_t
> > > > *output, enum pipe pipe, bool atomic,
> > > >  
> > > >  	igt_plane_set_fb(data->plane[primary->index], &data-
> > > > > 
> > > > > fb[primary->index]);
> > > >  
> > > > -	ret = igt_display_try_commit2(&data->display,
> > > > -				      atomic ? COMMIT_ATOMIC :
> > > > COMMIT_LEGACY);
> > > > +	ret = igt_display_try_commit2(&data->display, commit);
> > > >  	igt_skip_on(ret != 0);
> > > >  
> > > > -	igt_pipe_crc_start(data->pipe_crc);
> > > > -	n = igt_pipe_crc_get_crcs(data->pipe_crc, 1, crc);
> > > > -	igt_assert_eq(n, 1);
> > > > +	if (commit == COMMIT_LEGACY) {
> > > > +		igt_pipe_crc_collect_crc(data->pipe_crc,
> > > > &data-
> > > > > 
> > > > > ref_crc);
> > > > +	} else {
> > > > +		igt_pipe_crc_start(data->pipe_crc);
> > > > +		igt_pipe_crc_drain(data->pipe_crc);
> > > > +		igt_pipe_crc_get_single(data->pipe_crc, &data-
> > > > > 
> > > > > ref_crc);
> > > > +	}
> > > Why the 2 different paths?
> > The *_start *_get_single method didn't work for COMMIT_ATOMIC so I
> > decided to use legacy way for COMMIT_LEGACY. Maybe we could after
> > all
> > drop the legacy commits from this test. Originally, this test was
> > all
> > about atomic tests. 
> > > 
> > > And again, after pipe_crc_start no _drain() is needed, we already
> > > have a good CRC.
> > Yeah, this thing is an echo from the past. I remember we already
> > discussed this when previous patch was under review.
> > 
> > Thanks for the review!
> Just nuke the legacy paths then. :)
Ok. I'll do that :)

-- 
Mika Kahola - Intel OTC

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

end of thread, other threads:[~2018-03-09 12:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-28 13:44 [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC Mika Kahola
2018-02-28 14:34 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_plane_multiple: Drain pipe before reading CRC (rev2) Patchwork
2018-02-28 18:47 ` [igt-dev] ✗ Fi.CI.IGT: warning " Patchwork
2018-03-09  8:40 ` [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC Maarten Lankhorst
2018-03-09 11:25   ` Mika Kahola
2018-03-09 11:34     ` Maarten Lankhorst
2018-03-09 12:08       ` Mika Kahola

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox