Re: [igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC

[Mika Kahola <mika.kahola@intel.com>]

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