* [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
@ 2017-08-04 16:21 Daniel Vetter
2017-08-04 18:30 ` Paulo Zanoni
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2017-08-04 16:21 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Paulo Zanoni, Daniel Vetter
I guess this was done to have a better indication of which testcase
and function failed, but igt nowadays dumps an entire stacktrace. And
macros of this magnitude mean the line number is entirely meaningless,
since it doesn't point at a specific check.
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:
(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.
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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)
{
--
2.5.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
2017-08-04 16:21 [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions Daniel Vetter
@ 2017-08-04 18:30 ` Paulo Zanoni
2017-08-07 16:19 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Paulo Zanoni @ 2017-08-04 18:30 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development; +Cc: Daniel Vetter
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@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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)
> {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
2017-08-04 18:30 ` Paulo Zanoni
@ 2017-08-07 16:19 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-08-07 16:19 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter
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@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
@ 2017-08-14 9:25 Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-08-14 9:25 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Paulo Zanoni, Daniel Vetter
Macros that should be C functions but aren't are really hard to
read and confusing. Convert them over.
v2: Clean up commit message and keep printing the line numbers (Paulo).
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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)
{
--
2.5.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
@ 2017-08-14 9:25 Daniel Vetter
2017-09-01 19:44 ` Paulo Zanoni
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2017-08-14 9:25 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Paulo Zanoni, Daniel Vetter
Macros that should be C functions but aren't are really hard to
read and confusing. Convert them over.
v2: Clean up commit message and keep printing the line numbers (Paulo).
v3: Actually git add (silly me).
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
tests/kms_frontbuffer_tracking.c | 171 ++++++++++++++++++++-------------------
1 file changed, 89 insertions(+), 82 deletions(-)
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index e03524f1c45b..a068c8afb6d8 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1677,88 +1677,95 @@ 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(const struct test_mode *t, int flags,
+ int line)
+{
+ flags = adjust_assertion_flags(t, flags);
+ bool mandatory_sink_crc = t->feature & FEATURE_PSR;
+
+ igt_debug("checking asserts in line %i\n", line);
+
+ 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.");
+}
+
+#define do_assertions(__flags) __do_assertions(t, (__flags), __LINE__)
static void enable_prim_screen_and_wait(const struct test_mode *t)
{
--
2.5.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
2017-08-14 9:25 Daniel Vetter
@ 2017-09-01 19:44 ` Paulo Zanoni
2017-09-04 8:00 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Paulo Zanoni @ 2017-09-01 19:44 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development; +Cc: Daniel Vetter
Em Seg, 2017-08-14 às 11:25 +0200, Daniel Vetter escreveu:
> Macros that should be C functions but aren't are really hard to
> read and confusing. Convert them over.
>
> v2: Clean up commit message and keep printing the line numbers
> (Paulo).
>
> v3: Actually git add (silly me).
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Thanks for doing that. Works for me this way:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
But I'm still waiting for the patch that removes those bogus line
numbers in every error message we print :).
> ---
> tests/kms_frontbuffer_tracking.c | 171 ++++++++++++++++++++---------
> ----------
> 1 file changed, 89 insertions(+), 82 deletions(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index e03524f1c45b..a068c8afb6d8 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -1677,88 +1677,95 @@ 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(const struct test_mode *t, int flags,
> + int line)
> +{
> + flags = adjust_assertion_flags(t, flags);
> + bool mandatory_sink_crc = t->feature & FEATURE_PSR;
> +
> + igt_debug("checking asserts in line %i\n", line);
> +
> + 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.");
> +}
> +
> +#define do_assertions(__flags) __do_assertions(t, (__flags),
> __LINE__)
>
> static void enable_prim_screen_and_wait(const struct test_mode *t)
> {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
2017-09-01 19:44 ` Paulo Zanoni
@ 2017-09-04 8:00 ` Daniel Vetter
2017-09-04 18:09 ` Paulo Zanoni
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2017-09-04 8:00 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter
On Fri, Sep 01, 2017 at 04:44:38PM -0300, Paulo Zanoni wrote:
> Em Seg, 2017-08-14 às 11:25 +0200, Daniel Vetter escreveu:
> > Macros that should be C functions but aren't are really hard to
> > read and confusing. Convert them over.
> >
> > v2: Clean up commit message and keep printing the line numbers
> > (Paulo).
> >
> > v3: Actually git add (silly me).
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Thanks for doing that. Works for me this way:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Thanks, pushed.
> But I'm still waiting for the patch that removes those bogus line
> numbers in every error message we print :).
Hm, which bogus line numbers where?
-Daniel
>
> > ---
> > tests/kms_frontbuffer_tracking.c | 171 ++++++++++++++++++++---------
> > ----------
> > 1 file changed, 89 insertions(+), 82 deletions(-)
> >
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index e03524f1c45b..a068c8afb6d8 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1677,88 +1677,95 @@ 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(const struct test_mode *t, int flags,
> > + int line)
> > +{
> > + flags = adjust_assertion_flags(t, flags);
> > + bool mandatory_sink_crc = t->feature & FEATURE_PSR;
> > +
> > + igt_debug("checking asserts in line %i\n", line);
> > +
> > + 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.");
> > +}
> > +
> > +#define do_assertions(__flags) __do_assertions(t, (__flags),
> > __LINE__)
> >
> > static void enable_prim_screen_and_wait(const struct test_mode *t)
> > {
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
2017-09-04 8:00 ` Daniel Vetter
@ 2017-09-04 18:09 ` Paulo Zanoni
2017-09-05 11:18 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Paulo Zanoni @ 2017-09-04 18:09 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter
Em Seg, 2017-09-04 às 10:00 +0200, Daniel Vetter escreveu:
> On Fri, Sep 01, 2017 at 04:44:38PM -0300, Paulo Zanoni wrote:
> > Em Seg, 2017-08-14 às 11:25 +0200, Daniel Vetter escreveu:
> > > Macros that should be C functions but aren't are really hard to
> > > read and confusing. Convert them over.
> > >
> > > v2: Clean up commit message and keep printing the line numbers
> > > (Paulo).
> > >
> > > v3: Actually git add (silly me).
> > >
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >
> > Thanks for doing that. Works for me this way:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Thanks, pushed.
>
> > But I'm still waiting for the patch that removes those bogus line
> > numbers in every error message we print :).
>
> Hm, which bogus line numbers where?
Every igt_log message is starts with:
(kms_frontbuffer_tracking:XXXX) where XXXX is a number that means
nothing but looks like a line number, except that that line usually has
nothing to do with the message.
> -Daniel
> >
> > > ---
> > > tests/kms_frontbuffer_tracking.c | 171 ++++++++++++++++++++-----
> > > ----
> > > ----------
> > > 1 file changed, 89 insertions(+), 82 deletions(-)
> > >
> > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > b/tests/kms_frontbuffer_tracking.c
> > > index e03524f1c45b..a068c8afb6d8 100644
> > > --- a/tests/kms_frontbuffer_tracking.c
> > > +++ b/tests/kms_frontbuffer_tracking.c
> > > @@ -1677,88 +1677,95 @@ 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(const struct test_mode *t, int
> > > flags,
> > > + int line)
> > > +{
> > > + flags = adjust_assertion_flags(t, flags);
> > > + bool mandatory_sink_crc = t->feature & FEATURE_PSR;
> > > +
> > > + igt_debug("checking asserts in line %i\n", line);
> > > +
> > > + 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.");
> > > +}
> > > +
> > > +#define do_assertions(__flags) __do_assertions(t, (__flags),
> > > __LINE__)
> > >
> > > static void enable_prim_screen_and_wait(const struct test_mode
> > > *t)
> > > {
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions
2017-09-04 18:09 ` Paulo Zanoni
@ 2017-09-05 11:18 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-09-05 11:18 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter
On Mon, Sep 04, 2017 at 03:09:10PM -0300, Paulo Zanoni wrote:
> Em Seg, 2017-09-04 às 10:00 +0200, Daniel Vetter escreveu:
> > On Fri, Sep 01, 2017 at 04:44:38PM -0300, Paulo Zanoni wrote:
> > > Em Seg, 2017-08-14 às 11:25 +0200, Daniel Vetter escreveu:
> > > > Macros that should be C functions but aren't are really hard to
> > > > read and confusing. Convert them over.
> > > >
> > > > v2: Clean up commit message and keep printing the line numbers
> > > > (Paulo).
> > > >
> > > > v3: Actually git add (silly me).
> > > >
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > >
> > > Thanks for doing that. Works for me this way:
> > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Thanks, pushed.
> >
> > > But I'm still waiting for the patch that removes those bogus line
> > > numbers in every error message we print :).
> >
> > Hm, which bogus line numbers where?
>
> Every igt_log message is starts with:
> (kms_frontbuffer_tracking:XXXX) where XXXX is a number that means
> nothing but looks like a line number, except that that line usually has
> nothing to do with the message.
It's the process id. Not idea how you came up with the idea it's a line
number, executable:pid is pretty standard format for logfiles. Note how
it's the executable file name, not the source code file name.
And pid matters if you have a testcase that does lots of forking.
Should we improve the documentation about this? Care to type a patch
please?
Thanks, Daniel
>
> > -Daniel
> > >
> > > > ---
> > > > tests/kms_frontbuffer_tracking.c | 171 ++++++++++++++++++++-----
> > > > ----
> > > > ----------
> > > > 1 file changed, 89 insertions(+), 82 deletions(-)
> > > >
> > > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > > b/tests/kms_frontbuffer_tracking.c
> > > > index e03524f1c45b..a068c8afb6d8 100644
> > > > --- a/tests/kms_frontbuffer_tracking.c
> > > > +++ b/tests/kms_frontbuffer_tracking.c
> > > > @@ -1677,88 +1677,95 @@ 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(const struct test_mode *t, int
> > > > flags,
> > > > + int line)
> > > > +{
> > > > + flags = adjust_assertion_flags(t, flags);
> > > > + bool mandatory_sink_crc = t->feature & FEATURE_PSR;
> > > > +
> > > > + igt_debug("checking asserts in line %i\n", line);
> > > > +
> > > > + 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.");
> > > > +}
> > > > +
> > > > +#define do_assertions(__flags) __do_assertions(t, (__flags),
> > > > __LINE__)
> > > >
> > > > static void enable_prim_screen_and_wait(const struct test_mode
> > > > *t)
> > > > {
> >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-05 11:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-04 16:21 [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions Daniel Vetter
2017-08-04 18:30 ` Paulo Zanoni
2017-08-07 16:19 ` Daniel Vetter
-- strict thread matches above, loose matches on Subject: below --
2017-08-14 9:25 Daniel Vetter
2017-08-14 9:25 Daniel Vetter
2017-09-01 19:44 ` Paulo Zanoni
2017-09-04 8:00 ` Daniel Vetter
2017-09-04 18:09 ` Paulo Zanoni
2017-09-05 11:18 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox