2015-12-03 17:44 GMT-02:00 Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>:
> 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...
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.
>
>> 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
>>
>>
>>
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
