[Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
Daniel Vetter
daniel at ffwll.ch
Mon Aug 7 16:19:32 UTC 2017
On Fri, Aug 04, 2017 at 03:30:30PM -0300, Paulo Zanoni wrote:
> Em Sex, 2017-08-04 às 18:21 +0200, Daniel Vetter escreveu:
> > I guess this was done to have a better indication of which testcase
> > and function failed, but igt nowadays dumps an entire stacktrace.
>
> But we may have multiple do_assertions() calls in a single function.
>
> > And
> > macros of this magnitude mean the line number is entirely
> > meaningless,
> > since it doesn't point at a specific check.
>
> False. It always points to a do_assertions() call, which is what
> matters.
>
> >
> > Reason I've started to looking into this is that in our full igt CI
> > runs we have a serious problem with the fbc testcases randomly
> > failing with
> >
> > (kms_frontbuffer_tracking:1609) CRITICAL: Test assertion failure
> > function enable_prim_screen_and_wait, file
> > kms_frontbuffer_tracking.c:1771:
>
> https://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tests/kms_fr
> ontbuffer_tracking.c#n1771
>
> See?
Yeah, but why did it fall over? There's an "Assertion failed: false" and
everything else in your test is non-standard by usual igt test patterns.
You know how to look at this, I had absolutely no idea at all what's going
on here.
> > (kms_frontbuffer_tracking:1609) CRITICAL: Failed assertion: false
> > (kms_frontbuffer_tracking:1609) CRITICAL: FBC disabled
> >
> > And that's not entirely helpful. Also, macros of this magnitude are
> > just horrible to read.
>
> NAK. Being a macro instead of a function is extremely helpful and the
> line number always points me to the correct do_assertions() call, at
> least when I run this locally.
>
> If the line number in the CI system doesn't match what you see in your
> file, then it's a problem either on your side or on the CI side. But I
> don't think that's your problem. I think your problem is that we print
> two different line numbers (1609 and 1771), and you're looking at the
> wrong one. I would totally ACK a patch removing the 1609 one... But I
> don't think that would require patching kms_frontbuffuer_tracking.c.
>
> If you really really want to change this to a function, can't you try
> to find a way to pass a __LINE__ argument that corresponds to the exact
> line of the do_assertions() call and print it somewhere? Maybe another
> wrapper macro could auto-include __LINE__? But seriously, patch IGT to
> not print those bogus numbers, so you won't be confused next time.
Imo other people than you need to be able to understand tests. This much
macro abuse really doesn't help. I agree that I've been thrown off by
what's really going on here, but the underlying problem stands.
If you're typing macros instead of C functions and the macro very much
looks like a C function, something is wrong and needs to be fixed.
Note that you can sprinkle igt_debug log calls into your test, which would
a) help even more understanding it.
b) also make it clear which assert blew up, because we dump the entire
debug log upon failure.
E.g. you could dump __LINE__ in there too if you want that.
As is this is a mess no one but you understands, and some way we need to
improve this.
Please reconsider your nack.
Thanks, Daniel
>
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> > tests/kms_frontbuffer_tracking.c | 166 ++++++++++++++++++++---------
> > ----------
> > 1 file changed, 84 insertions(+), 82 deletions(-)
> >
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index e03524f1c45b..8d11dc065623 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1677,88 +1677,90 @@ static int adjust_assertion_flags(const
> > struct test_mode *t, int flags)
> > return flags;
> > }
> >
> > -#define do_crc_assertions(flags, mandatory_sink_crc) do {
> > \
> > - int flags__ = (flags);
> > \
> > - struct both_crcs crc_;
> > \
> > -
> > \
> > - if (!opt.check_crc || (flags__ & DONT_ASSERT_CRC))
> > \
> > - break;
> > \
> > -
> > \
> > - collect_crcs(&crc_, mandatory_sink_crc);
> > \
> > - print_crc("Calculated CRC:", &crc_);
> > \
> > -
> > \
> > - igt_assert(wanted_crc);
> > \
> > - igt_assert_crc_equal(&crc_.pipe, &wanted_crc->pipe);
> > \
> > - if (mandatory_sink_crc)
> > \
> > - assert_sink_crc_equal(&crc_.sink, &wanted_crc-
> > >sink); \
> > - else if (sink_crc.reliable &&
> > \
> > - !sink_crc_equal(&crc_.sink, &wanted_crc->sink))
> > \
> > - igt_info("Sink CRC differ, but not required\n");
> > \
> > -} while (0)
> > -
> > -#define do_status_assertions(flags_) do {
> > \
> > - if (!opt.check_status) {
> > \
> > - /* Make sure we settle before continuing. */
> > \
> > - sleep(1);
> > \
> > - break;
> > \
> > - }
> > \
> > -
> > \
> > - if (flags_ & ASSERT_FBC_ENABLED) {
> > \
> > - igt_require(!fbc_not_enough_stolen());
> > \
> > - igt_require(!fbc_stride_not_supported());
> > \
> > - if (!fbc_wait_until_enabled()) {
> > \
> > - fbc_print_status();
> > \
> > - igt_assert_f(false, "FBC disabled\n");
> > \
> > - }
> > \
> > -
> > \
> > - if (opt.fbc_check_compression)
> > \
> > - igt_assert(fbc_wait_for_compression());
> > \
> > - } else if (flags_ & ASSERT_FBC_DISABLED) {
> > \
> > - igt_assert(!fbc_wait_until_enabled());
> > \
> > - }
> > \
> > -
> > \
> > - if (flags_ & ASSERT_PSR_ENABLED) {
> > \
> > - if (!psr_wait_until_enabled()) {
> > \
> > - psr_print_status();
> > \
> > - igt_assert_f(false, "PSR disabled\n");
> > \
> > - }
> > \
> > - } else if (flags_ & ASSERT_PSR_DISABLED) {
> > \
> > - igt_assert(!psr_wait_until_enabled());
> > \
> > - }
> > \
> > -} while (0)
> > -
> > -#define do_assertions(flags) do {
> > \
> > - int flags_ = adjust_assertion_flags(t, (flags));
> > \
> > - bool mandatory_sink_crc = t->feature & FEATURE_PSR;
> > \
> > -
> > \
> > - wait_user(2, "Paused before assertions.");
> > \
> > -
> > \
> > - /* Check the CRC to make sure the drawing operations work
> > \
> > - * immediately, independently of the features being enabled.
> > */ \
> > - do_crc_assertions(flags_, mandatory_sink_crc);
> > \
> > -
> > \
> > - /* Now we can flush things to make the test faster. */
> > \
> > - do_flush(t);
> > \
> > -
> > \
> > - do_status_assertions(flags_);
> > \
> > -
> > \
> > - /* Check CRC again to make sure the compressed screen is ok,
> > \
> > - * except if we're not drawing on the primary screen. On
> > this \
> > - * case, the first check should be enough and a new CRC
> > check \
> > - * would only delay the test suite while adding no value to
> > the \
> > - * test suite. */
> > \
> > - if (t->screen == SCREEN_PRIM)
> > \
> > - do_crc_assertions(flags_, mandatory_sink_crc);
> > \
> > -
> > \
> > - if (fbc.supports_last_action && opt.fbc_check_last_action) {
> > \
> > - if (flags_ & ASSERT_LAST_ACTION_CHANGED)
> > \
> > - igt_assert(fbc_last_action_changed());
> > \
> > - else if (flags_ & ASSERT_NO_ACTION_CHANGE)
> > \
> > - igt_assert(!fbc_last_action_changed());
> > \
> > - }
> > \
> > -
> > \
> > - wait_user(1, "Paused after assertions.");
> > \
> > -} while (0)
> > +static void do_crc_assertions(int flags, bool mandatory_sink_crc)
> > +{
> > + struct both_crcs crc;
> > +
> > + if (!opt.check_crc || (flags & DONT_ASSERT_CRC))
> > + return;
> > +
> > + collect_crcs(&crc, mandatory_sink_crc);
> > + print_crc("Calculated CRC:", &crc);
> > +
> > + igt_assert(wanted_crc);
> > + igt_assert_crc_equal(&crc.pipe, &wanted_crc->pipe);
> > + if (mandatory_sink_crc)
> > + assert_sink_crc_equal(&crc.sink, &wanted_crc->sink);
> > + else if (sink_crc.reliable &&
> > + !sink_crc_equal(&crc.sink, &wanted_crc->sink))
> > + igt_info("Sink CRC differ, but not required\n");
> > +}
> > +
> > +static void do_status_assertions(int flags)
> > +{
> > + if (!opt.check_status) {
> > + /* Make sure we settle before continuing. */
> > + sleep(1);
> > + return;
> > + }
> > +
> > + if (flags & ASSERT_FBC_ENABLED) {
> > + igt_require(!fbc_not_enough_stolen());
> > + igt_require(!fbc_stride_not_supported());
> > + if (!fbc_wait_until_enabled()) {
> > + fbc_print_status();
> > + igt_assert_f(false, "FBC disabled\n");
> > + }
> > +
> > + if (opt.fbc_check_compression)
> > + igt_assert(fbc_wait_for_compression());
> > + } else if (flags & ASSERT_FBC_DISABLED) {
> > + igt_assert(!fbc_wait_until_enabled());
> > + }
> > +
> > + if (flags & ASSERT_PSR_ENABLED) {
> > + if (!psr_wait_until_enabled()) {
> > + psr_print_status();
> > + igt_assert_f(false, "PSR disabled\n");
> > + }
> > + } else if (flags & ASSERT_PSR_DISABLED) {
> > + igt_assert(!psr_wait_until_enabled());
> > + }
> > +}
> > +
> > +static void do_assertions(int flags)
> > +{
> > + flags = adjust_assertion_flags(t, flags);
> > + bool mandatory_sink_crc = t->feature & FEATURE_PSR;
> > +
> > + wait_user(2, "Paused before assertions.");
> > +
> > + /* Check the CRC to make sure the drawing operations work
> > + * immediately, independently of the features being enabled.
> > */
> > + do_crc_assertions(flags, mandatory_sink_crc);
> > +
> > + /* Now we can flush things to make the test faster. */
> > + do_flush(t);
> > +
> > + do_status_assertions(flags);
> > +
> > + /* Check CRC again to make sure the compressed screen is ok,
> > + * except if we're not drawing on the primary screen. On
> > this
> > + * case, the first check should be enough and a new CRC
> > check
> > + * would only delay the test suite while adding no value to
> > the
> > + * test suite. */
> > + if (t->screen == SCREEN_PRIM)
> > + do_crc_assertions(flags, mandatory_sink_crc);
> > +
> > + if (fbc.supports_last_action && opt.fbc_check_last_action) {
> > + if (flags & ASSERT_LAST_ACTION_CHANGED)
> > + igt_assert(fbc_last_action_changed());
> > + else if (flags_ & ASSERT_NO_ACTION_CHANGE)
> > + igt_assert(!fbc_last_action_changed());
> > + }
> > +
> > + wait_user(1, "Paused after assertions.");
> > +}
> >
> > static void enable_prim_screen_and_wait(const struct test_mode *t)
> > {
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list