Re: [PATCH i-g-t] tests/kms_plane_multiple: Fix reference CRC

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

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
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx