On Wed, Aug 2, 2017 at 2:06 PM, Mika Kahola <mika.kahola@xxxxxxxxx> wrote: > 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. Ok, with that as the commit message the patch is r-b'ed: me. Please push. > 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. I still don't get this, and it still sounds like this is a bug in the library code ... Where exactly is n_words lost? Thanks, Daniel > > 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 -- 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
