| Message ID | 1501245916-3719-1-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 28, 2017 at 2:45 PM, Mika Kahola <mika.kahola@intel.com> 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? -Daniel > > The problem was caught by CI system and at least affects on HSW platform. > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > --- > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of > Daniel Vetter > Sent: Monday, July 31, 2017 11:13 AM > To: Kahola, Mika <mika.kahola@intel.com> > Cc: intel-gfx <intel-gfx@lists.freedesktop.org> > Subject: Re: [Intel-gfx] [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.com> 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. Cheers, Mika > -Daniel > > > > > The problem was caught by CI system and at least affects on HSW platform. > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > --- > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Jul 31, 2017 at 09:04:50AM +0000, Kahola, Mika wrote: > > -----Original Message----- > > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of > > Daniel Vetter > > Sent: Monday, July 31, 2017 11:13 AM > > To: Kahola, Mika <mika.kahola@intel.com> > > Cc: intel-gfx <intel-gfx@lists.freedesktop.org> > > Subject: Re: [Intel-gfx] [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.com> 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? 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@intel.com> > > > --- > > > 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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
FYI, bug 101907 > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf > Of Daniel Vetter > Sent: Wednesday, August 2, 2017 2:37 PM > To: Kahola, Mika <mika.kahola@intel.com> > Cc: intel-gfx <intel-gfx@lists.freedesktop.org> > Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/kms_plane_multiple: Fix > reference CRC > > On Mon, Jul 31, 2017 at 09:04:50AM +0000, Kahola, Mika wrote: > > > -----Original Message----- > > > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On > > > Behalf Of Daniel Vetter > > > Sent: Monday, July 31, 2017 11:13 AM > > > To: Kahola, Mika <mika.kahola@intel.com> > > > Cc: intel-gfx <intel-gfx@lists.freedesktop.org> > > > Subject: Re: [Intel-gfx] [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.com> > 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? > > 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@intel.com> > > > > --- > > > > 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@lists.freedesktop.org > > > > 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 > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On > > > Behalf Of > > > Daniel Vetter > > > Sent: Monday, July 31, 2017 11:13 AM > > > To: Kahola, Mika <mika.kahola@intel.com> > > > Cc: intel-gfx <intel-gfx@lists.freedesktop.org> > > > Subject: Re: [Intel-gfx] [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@intel.com> > > > > --- > > > > 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@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Aug 2, 2017 at 2:06 PM, Mika Kahola <mika.kahola@intel.com> 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@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On >> > > Behalf Of >> > > Daniel Vetter >> > > Sent: Monday, July 31, 2017 11:13 AM >> > > To: Kahola, Mika <mika.kahola@intel.com> >> > > Cc: intel-gfx <intel-gfx@lists.freedesktop.org> >> > > Subject: Re: [Intel-gfx] [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@intel.com> >> > > > --- >> > > > 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@lists.freedesktop.org >> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > > >> > > >> > > -- >> > > Daniel Vetter >> > > Software Engineer, Intel Corporation >> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, 2017-08-02 at 14:16 +0200, Daniel Vetter wrote: > On Wed, Aug 2, 2017 at 2:06 PM, Mika Kahola <mika.kahola@intel.com> > 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@ffwll.ch [mailto:daniel.vetter@ffwll.ch] > > > > > On > > > > > Behalf Of > > > > > Daniel Vetter > > > > > Sent: Monday, July 31, 2017 11:13 AM > > > > > To: Kahola, Mika <mika.kahola@intel.com> > > > > > Cc: intel-gfx <intel-gfx@lists.freedesktop.org> > > > > > Subject: Re: [Intel-gfx] [PATCH i-g-t] > > > > > tests/kms_plane_multiple: > > > > > Fix reference > > > > > CRC > > > > > > > > > > On Fri, Jul 28, 2017 at 2:45 PM, Mika Kahola <mika.kahola@int > > > > > el.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. OK, thanks! I'll change the commit message and push the patch. Cheers, Mika > > > > > 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@intel.com> > > > > > > --- > > > > > > 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@lists.freedesktop.org > > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > -- > > > > > Daniel Vetter > > > > > Software Engineer, Intel Corporation > > > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > >
After a bit of digging into this n_words conundrum that I also could not understand, I found the issue Mika described. kms_plane_multiple et al basically do igt_crc_t crc; and then they pass &crc to test-internal helper functions, which pass the igt_crc_t* pointer to various igt lib functions. The problem is, those lib functions take igt_crc_t** so that they can give you the correct pointer, kms_plane* wrongly expect them to modify the passed object. Mika, can you fix this while you're at it? -- Petri Latvala
On Wed, 2017-08-02 at 15:42 +0300, Petri Latvala wrote: > After a bit of digging into this n_words conundrum that I also could > not understand, I found the issue Mika described. > > kms_plane_multiple et al basically do > > igt_crc_t crc; > > > and then they pass &crc to test-internal helper functions, which pass > the igt_crc_t* pointer to various igt lib functions. The problem is, > those lib functions take igt_crc_t** so that they can give you the > correct pointer, kms_plane* wrongly expect them to modify the passed > object. > > Mika, can you fix this while you're at it? Right, we can do this without changing the original function call. I'll throw a patch shortly. Cheers, Mika > > > > -- > Petri Latvala
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);
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. The problem was caught by CI system and at least affects on HSW platform. Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- tests/kms_plane_multiple.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)