On Wed, 2017-08-02 at 13:36 +0200, Daniel Vetter wrote: > On Mon, Jul 31, 2017 at 09:04:50AM +0000, Kahola, Mika wrote: > > > > > > > > -----Original Message----- > > > From: daniel.vetter@xxxxxxxx [mailto:daniel.vetter@xxxxxxxx] On > > > Behalf Of > > > Daniel Vetter > > > Sent: Monday, July 31, 2017 11:13 AM > > > To: Kahola, Mika <mika.kahola@xxxxxxxxx> > > > Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx> > > > Subject: Re: [PATCH i-g-t] tests/kms_plane_multiple: > > > Fix reference > > > CRC > > > > > > On Fri, Jul 28, 2017 at 2:45 PM, Mika Kahola <mika.kahola@intel.c > > > om> wrote: > > > > > > > > When grabbing reference CRC with igt_pipe_crc_get_crcs() the > > > > number of > > > > words in igt_crc_t structure was incorrectly collected. The fix > > > > here > > > > is to switch to igt_pipe_crc_collect_crc() function when > > > > collecting > > > > CRC for reference frame. > > > So there's also a bug in the core library that this patch papers > > > over? > > > Do you have a patch for that one too? > > The core library got updated and that actually revealed a bug in > > the > > test itself. igt_crc_t struct member n_words was not correctly > > copied > > and the updated core function checks if this is 0 and if so the crc > > check fails. All we need to do is have a fix on this test. > Yeah, but there's no n_words in kms_plane_multiple.c. How exactly > does > switching from igt_pipe_crc_get_crcs to igt_pipe_crc_collect_crc fix > this? > And if it does, why do we expose a broken function to testcases? > I switched to use igt_pipe_crc_collect() for the following reason. This one time function is used only for grabbing reference CRC and therefore for that purpose we can use this one time CRC collection function. So the real fix in the test is switching to use igt_pipe_crc_collect() function instead of igt_pipe_crc_get_crcs(). That is also the reason why we need to place igt_pipe_crc_start() function call before we enter the main loop. For this purpose, I think it is more feasible to start and stop pipe and get the reference CRC before entering the actual test. I also discovered that the the pointer containing igt_crc_t struct lost its member n_words data when the function returned. The fix here was to read crc in temporary variable and copy that data to crc variable. Instead of doing this, I decided to switch to igt_pipe_crc_collect() function as in my opinion suits better for this purpose. Cheers, Mika > aka pls explain more, I don't get what's going on here. > -Daniel > > > > > > > Cheers, > > Mika > > > > > > > > -Daniel > > > > > > > > > > > > > > > The problem was caught by CI system and at least affects on HSW > > > > platform. > > > > > > > > Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx> > > > > --- > > > > tests/kms_plane_multiple.c | 10 ++++++---- > > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/tests/kms_plane_multiple.c > > > > b/tests/kms_plane_multiple.c > > > > index f6c6223..08f184a 100644 > > > > --- a/tests/kms_plane_multiple.c > > > > +++ b/tests/kms_plane_multiple.c > > > > @@ -110,7 +110,7 @@ test_grab_crc(data_t *data, igt_output_t > > > > *output, > > > > enum pipe pipe, bool atomic, { > > > > drmModeModeInfo *mode; > > > > igt_plane_t *primary; > > > > - int ret, n; > > > > + int ret; > > > > > > > > igt_output_set_pipe(output, pipe); > > > > > > > > @@ -131,9 +131,7 @@ test_grab_crc(data_t *data, igt_output_t > > > > *output, > > > enum pipe pipe, bool atomic, > > > > > > > > atomic ? COMMIT_ATOMIC : > > > > COMMIT_LEGACY); > > > > 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); > > > > + igt_pipe_crc_collect_crc(data->pipe_crc, crc); > > > > } > > > > > > > > /* > > > > @@ -278,6 +276,8 @@ > > > > test_atomic_plane_position_with_output(data_t > > > *data, enum pipe pipe, > > > > > > > > test_grab_crc(data, output, pipe, true, &blue, tiling, > > > > &test.reference_crc); > > > > > > > > + igt_pipe_crc_start(data->pipe_crc); > > > > + > > > > i = 0; > > > > while (i < iterations || loop_forever) { > > > > prepare_planes(data, pipe, &blue, tiling, > > > > n_planes, > > > > output); @@ -344,6 +344,8 @@ > > > test_legacy_plane_position_with_output(data_t *data, enum pipe > > > pipe, > > > > > > > > test_grab_crc(data, output, pipe, false, &blue, tiling, > > > > &test.reference_crc); > > > > > > > > + igt_pipe_crc_start(data->pipe_crc); > > > > + > > > > i = 0; > > > > while (i < iterations || loop_forever) { > > > > prepare_planes(data, pipe, &blue, tiling, > > > > n_planes, > > > > output); > > > > -- > > > > 2.7.4 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
