Re: [PATCH i-g-t 3/5] kms_frontbuffer_tracking: Skip on unreliable CRC.

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Thu, 2015-12-03 at 15:05 -0200, Paulo Zanoni wrote:
> 2015-12-03 14:39 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > Even with all sink crc re-works we still have platforms
> > where after 6 vblanks it is unable to calculate the
> > sink crc. But if we don't get the sink crc it isn't true
> > that test failed, but that we have no ways to say test
> > passed or failed.
> > 
> > So let's print a message and move forward in case sink crc
> > cannot help us to know if the screen has been updated.
> > 
> > v2: Also include a message on setup_sink_crc and also
> > only skip when it is mandatory, i.e. when running for PSR.
> 
> If I assume that the idea you're implementing is what we want, then
> the patch looks correct and Acked-by: Paulo Zanoni
> <paulo.r.zanoni@intel.com> .
> 
> My problem is the whole idea of "silently" skipping the tests in case
> the sink CRC fails. What if QA's only machines always have this
> problem with sink CRCs and always skip every single PSR test during
> automatic testing? We won't discover that it's skipping, and we'll 
> end
> up assuming PSR works due to no test failures, while in fact it's not
> even being tested. That said, I don't really have a solution for 
> this.

Yeah, this is a good point. This is one of the reasons I want to add
that aux fix soon as well. With that in place the sink CRC is not that
unreliable and if skip will be rare and random.
I run the tests in many different platforms here and many times and it
is really rare to skip so I prefer to skip when that happens than have
new bug entry for every false positive that might randomly happen.
Also we know it is a sink CRC issue and not a PSR one, but people
looking to the test will believe that PSR failed and not sink crc...

> Maybe someone else could have an idea here?
> 
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  tests/kms_frontbuffer_tracking.c | 37 +++++++++++++++++++++++++---
> > ---------
> >  1 file changed, 25 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tests/kms_frontbuffer_tracking.c 
> > b/tests/kms_frontbuffer_tracking.c
> > index ddcec75..e200975 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -859,12 +859,22 @@ static bool psr_wait_until_enabled(void)
> >  #define psr_enable() igt_set_module_param_int("enable_psr", 1)
> >  #define psr_disable() igt_set_module_param_int("enable_psr", 0)
> > 
> > -static void get_sink_crc(sink_crc_t *crc)
> > +static void get_sink_crc(sink_crc_t *crc, bool mandatory)
> >  {
> > +       int rc, errno_;
> > +
> >         lseek(sink_crc.fd, 0, SEEK_SET);
> > 
> > -       igt_assert(read(sink_crc.fd, crc->data, SINK_CRC_SIZE) ==
> > -                  SINK_CRC_SIZE);
> > +       rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
> > +       errno_ = errno;
> > +
> > +       if (rc == -1 && errno_ == ETIMEDOUT) {
> > +               if (mandatory)
> > +                       igt_skip("Sink CRC is unreliable on this 
> > machine. Try running this test again individually\n");
> > +               else
> > +                       igt_info("Sink CRC is unreliable on this 
> > machine. Try running this test again individually\n");
> > +       }
> > +       igt_assert(rc == SINK_CRC_SIZE);
> >  }
> > 
> >  static bool sink_crc_equal(sink_crc_t *a, sink_crc_t *b)
> > @@ -1148,17 +1158,17 @@ static void print_crc(const char *str, 
> > struct both_crcs *crc)
> >         free(pipe_str);
> >  }
> > 
> > -static void collect_crcs(struct both_crcs *crcs)
> > +static void collect_crcs(struct both_crcs *crcs, bool 
> > mandatory_sink_crc)
> >  {
> >         igt_pipe_crc_collect_crc(pipe_crc, &crcs->pipe);
> > 
> >         if (sink_crc.supported)
> > -               get_sink_crc(&crcs->sink);
> > +               get_sink_crc(&crcs->sink, mandatory_sink_crc);
> >         else
> >                 memcpy(&crcs->sink, "unsupported!", SINK_CRC_SIZE);
> >  }
> > 
> > -static void init_blue_crc(enum pixel_format format)
> > +static void init_blue_crc(enum pixel_format format, bool 
> > mandatory_sink_crc)
> >  {
> >         struct igt_fb blue;
> >         int rc;
> > @@ -1176,7 +1186,7 @@ static void init_blue_crc(enum pixel_format 
> > format)
> >                             blue.fb_id, 0, 0, 
> > &prim_mode_params.connector_id, 1,
> >                             prim_mode_params.mode);
> >         igt_assert_eq(rc, 0);
> > -       collect_crcs(&blue_crcs[format].crc);
> > +       collect_crcs(&blue_crcs[format].crc, mandatory_sink_crc);
> > 
> >         print_crc("Blue CRC:  ", &blue_crcs[format].crc);
> > 
> > @@ -1188,7 +1198,8 @@ static void init_blue_crc(enum pixel_format 
> > format)
> >  }
> > 
> >  static void init_crcs(enum pixel_format format,
> > -                     struct draw_pattern_info *pattern)
> > +                     struct draw_pattern_info *pattern,
> > +                     bool mandatory_sink_crc)
> >  {
> >         int r, r_, rc;
> >         struct igt_fb tmp_fbs[pattern->n_rects];
> > @@ -1224,7 +1235,7 @@ static void init_crcs(enum pixel_format 
> > format,
> >                                    &prim_mode_params.connector_id, 
> > 1,
> >                                    prim_mode_params.mode);
> >                 igt_assert_eq(rc, 0);
> > -               collect_crcs(&pattern->crcs[format][r]);
> > +               collect_crcs(&pattern->crcs[format][r], 
> > mandatory_sink_crc);
> >         }
> > 
> >         for (r = 0; r < pattern->n_rects; r++) {
> > @@ -1345,6 +1356,8 @@ static void setup_sink_crc(void)
> >         errno_ = errno;
> >         if (rc == -1 && errno_ == ENOTTY)
> >                 igt_info("Sink CRC not supported: panel doesn't 
> > support it\n");
> > +       if (rc == -1 && errno_ == ETIMEDOUT)
> > +               igt_info("Sink CRC not reliable on this panel: 
> > skipping it\n");
> >         else if (rc == SINK_CRC_SIZE)
> >                 sink_crc.supported = true;
> >         else
> > @@ -1577,7 +1590,7 @@ static int adjust_assertion_flags(const 
> > struct test_mode *t, int flags)
> >         if (!opt.check_crc || (flags__ & DONT_ASSERT_CRC))         
> >      \
> >                 break;                                             
> >      \
> >                                                                    
> >      \
> > -       collect_crcs(&crc_);                                       
> >      \
> > +       collect_crcs(&crc_, mandatory_sink_crc);                   
> >      \
> >         print_crc("Calculated CRC:", &crc_);                       
> >      \
> >                                                                    
> >      \
> >         igt_assert(wanted_crc);                                    
> >      \
> > @@ -1797,9 +1810,9 @@ static void prepare_subtest_data(const struct 
> > test_mode *t,
> > 
> >         unset_all_crtcs();
> > 
> > -       init_blue_crc(t->format);
> > +       init_blue_crc(t->format, t->feature & FEATURE_PSR);
> >         if (pattern)
> > -               init_crcs(t->format, pattern);
> > +               init_crcs(t->format, pattern, t->feature & 
> > FEATURE_PSR);
> > 
> >         enable_features_for_test(t);
> >  }
> > --
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx