[Intel-gfx] [PATCH i-g-t 3/5] kms_frontbuffer_tracking: Skip on unreliable CRC.
Vivi, Rodrigo
rodrigo.vivi at intel.com
Thu Dec 3 14:32:29 PST 2015
On Thu, 2015-12-03 at 17:59 -0200, Paulo Zanoni wrote:
> 2015-12-03 17:44 GMT-02:00 Vivi, Rodrigo <rodrigo.vivi at 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 at 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 at 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...
>
> If the current failures don't happen 100% of the time, I wonder if it
> wouldn't be better to just re-run the same subtest all over again
> (including the mode unset/sets) until the CRCs work.
Yeah, I agree this is a best approach. Let's stay with this skip for
now and apply this retry on top
>
> >
> > > Maybe someone else could have an idea here?
> > >
> > > >
> > > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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 at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > >
> > >
>
>
>
More information about the Intel-gfx
mailing list