[i-g-t,v2] tests/kms_plane_multiple: Drain pipe before reading CRC

Submitted by Kahola, Mika on Feb. 28, 2018, 1:44 p.m.

Details

Message ID 1519825460-30141-1-git-send-email-mika.kahola@intel.com
State New
Headers show
Series "tests/kms_plane_multiple: Drain pipe before reading CRC" ( rev: 2 ) in IGT

Not browsing as part of any series.

Commit Message

Kahola, Mika Feb. 28, 2018, 1:44 p.m.
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(-)

Patch hide | download patch | download mbox

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++;
 	}
 

Comments

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++;
>  	}
>
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++;
> >  	}
> >  
>
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. :)
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 :)