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? > (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. > > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > --- > 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) > { _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
