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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux