On Thu, 2015-12-03 at 15:05 -0200, Paulo Zanoni wrote:
> 2015-12-03 14:39 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>:
> > 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@xxxxxxxxx> .
>
> 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@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
