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

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

On Mon, 2018-02-26 at 13:16 +0100, Maarten Lankhorst wrote:
> Op 26-02-18 om 12:10 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.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103166
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> This won't fix 103166, so I think this should be a references.
True, this doesn't fully fix the bug, only partially. I'll change to
references.

> > 
> > ---
> >  tests/kms_plane_lowres.c | 30 ++++++++++--------------------
> >  1 file changed, 10 insertions(+), 20 deletions(-)
> > 
> > diff --git a/tests/kms_plane_lowres.c b/tests/kms_plane_lowres.c
> > index c224a1b..b89904f 100644
> > --- a/tests/kms_plane_lowres.c
> > +++ b/tests/kms_plane_lowres.c
> > @@ -126,23 +126,6 @@ test_fini(data_t *data, igt_output_t *output,
> > enum pipe pipe)
> >  }
> >  
> >  static void
> > -display_commit_mode(igt_display_t *display, igt_pipe_crc_t
> > *pipe_crc,
> > -		    enum pipe pipe, int flags, igt_crc_t *out_crc)
> > -{
> > -	char buf[256];
> > -	int ret;
> > -
> > -	ret = igt_display_try_commit_atomic(display, flags, NULL);
> > -	igt_skip_on(ret != 0);
> > -
> > -	igt_set_timeout(1, "Stuck on page flip");
> > -	ret = read(display->drm_fd, buf, sizeof(buf));
> > -	igt_assert(ret >= 0);
> > -
> > -	igt_pipe_crc_collect_crc(pipe_crc, out_crc);
> > -}
> > -
> > -static void
> >  check_mode(drmModeModeInfo *mode1, drmModeModeInfo *mode2)
> >  {
> >  	igt_assert_eq(mode1->hdisplay, mode2->hdisplay);
> > @@ -225,8 +208,10 @@ test_plane_position_with_output(data_t *data,
> > enum pipe pipe,
> >  	igt_skip_on(ret != 0);
> >  
> >  	pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
> > INTEL_PIPE_CRC_SOURCE_AUTO);
> > +	igt_pipe_crc_start(pipe_crc);
> >  
> > -	igt_pipe_crc_collect_crc(pipe_crc, &crc_hires1);
> > +	igt_pipe_crc_drain(pipe_crc);
> > +	igt_pipe_crc_get_single(pipe_crc, &crc_hires1);
> First drain isn't required. You're not having any stale CRCs at this
> point. :)
I'll keep the original here.
> 
> flags can also be removed.
Yep. I forgot to remove those.

Thanks for the review!
> 
> With that fixed:
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > 
> >  
> >  	igt_assert_plane_visible(data->drm_fd, pipe, true);
> >  
> > @@ -238,7 +223,9 @@ test_plane_position_with_output(data_t *data,
> > enum pipe pipe,
> >  
> >  	check_mode(&mode_lowres, mode2);
> >  
> > -	display_commit_mode(&data->display, pipe_crc, pipe, flags,
> > &crc_lowres);
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > +	igt_pipe_crc_drain(pipe_crc);
> > +	igt_pipe_crc_get_single(pipe_crc, &crc_lowres);
> >  
> >  	igt_assert_plane_visible(data->drm_fd, pipe, false);
> >  
> > @@ -250,7 +237,10 @@ test_plane_position_with_output(data_t *data,
> > enum pipe pipe,
> >  
> >  	check_mode(mode1, mode3);
> >  
> > -	display_commit_mode(&data->display, pipe_crc, pipe, flags,
> > &crc_hires2);
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > +
> > +	igt_pipe_crc_drain(pipe_crc);
> > +	igt_pipe_crc_get_single(pipe_crc, &crc_hires2);
> >  
> >  	igt_assert_plane_visible(data->drm_fd, pipe, true);
> >  
> 
-- 
Mika Kahola - Intel OTC

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