[Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
Paulo Zanoni
paulo.r.zanoni at intel.com
Fri Aug 4 18:30:30 UTC 2017
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 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)
> {
More information about the Intel-gfx
mailing list