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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux