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

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

[Mika Kahola]

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(-)

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

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

[Daniel Vetter]

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



-- 
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

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

[Kahola, Mika]

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

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

[Daniel Vetter]

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

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

[Lofstedt, Marta]

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

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

[Mika Kahola]

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

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

[Daniel Vetter]

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



-- 
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

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

[Mika Kahola]

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

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

[Petri Latvala]

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

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

[Mika Kahola]

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