Re: [Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[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 
> > > > Signed-off-by: Daniel Vetter 
> > > 
> > > Thanks for doing that. Works for me this way:
> > > Reviewed-by: Paulo Zanoni 
> > 
> > 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:) where  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(_, mandatory_sink_crc);
> > > > \
> > > > -   print_crc("Calculated CRC:", _);
> > > > 
> > > > \
> > > > -   
> > > > \
> > > > -   igt_assert(wanted_crc); 
> > > > \
> > > > -   igt_assert_crc_equal(_.pipe, _crc->pipe);
> > > > 
> > > > \
> > > > -   if (mandatory_sink_crc) 
> > > > \
> > > > -   assert_sink_crc_equal(_.sink, _crc-
> > > > > sink);\
> > > > 
> > > > -   else if (sink_crc.reliable &&   
> > > > \
> > > > -    !sink_crc_equal(_.sink, _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");  
> > > >   

Re: [Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[Paulo Zanoni]

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 
> > > Signed-off-by: Daniel Vetter 
> > 
> > Thanks for doing that. Works for me this way:
> > Reviewed-by: Paulo Zanoni 
> 
> 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:) where  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(_, mandatory_sink_crc);
> > > \
> > > - print_crc("Calculated CRC:", _);
> > >   
> > > \
> > > - 
> > > \
> > > - igt_assert(wanted_crc); 
> > >   \
> > > - igt_assert_crc_equal(_.pipe, _crc->pipe);
> > >   
> > > \
> > > - if (mandatory_sink_crc) 
> > >   \
> > > - assert_sink_crc_equal(_.sink, _crc-
> > > > sink);  \
> > > 
> > > - else if (sink_crc.reliable &&   
> > >   \
> > > -  !sink_crc_equal(_.sink, _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");  
> > >   \
> > > - }   

Re: [Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[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 
> > Signed-off-by: Daniel Vetter 
> 
> Thanks for doing that. Works for me this way:
> Reviewed-by: Paulo Zanoni 

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(_, mandatory_sink_crc);
> > \
> > -   print_crc("Calculated CRC:", _);
> > \
> > -   
> > \
> > -   igt_assert(wanted_crc); 
> > \
> > -   igt_assert_crc_equal(_.pipe, _crc->pipe);
> > \
> > -   if (mandatory_sink_crc) 
> > \
> > -   assert_sink_crc_equal(_.sink, _crc-
> > >sink); \
> > -   else if (sink_crc.reliable &&   
> > \
> > -    !sink_crc_equal(_.sink, _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)); 

Re: [Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[Paulo Zanoni]

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 
> Signed-off-by: Daniel Vetter 

Thanks for doing that. Works for me this way:
Reviewed-by: Paulo Zanoni 

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(_, mandatory_sink_crc);
> \
> - print_crc("Calculated CRC:", _);
> \
> - 
> \
> - igt_assert(wanted_crc); 
>   \
> - igt_assert_crc_equal(_.pipe, _crc->pipe);
> \
> - if (mandatory_sink_crc) 
>   \
> - assert_sink_crc_equal(_.sink, _crc-
> >sink);   \
> - else if (sink_crc.reliable &&   
>   \
> -  !sink_crc_equal(_.sink, _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.");  
> \
> -

[Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[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 
Signed-off-by: Daniel Vetter 
---
 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(_, mandatory_sink_crc);\
-   print_crc("Calculated CRC:", _);\
-   \
-   igt_assert(wanted_crc); \
-   igt_assert_crc_equal(_.pipe, _crc->pipe);\
-   if (mandatory_sink_crc) \
-   assert_sink_crc_equal(_.sink, _crc->sink);   \
-   else if (sink_crc.reliable &&   \
-!sink_crc_equal(_.sink, _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);   

[Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[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 
Signed-off-by: Daniel Vetter 
---
 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(_, mandatory_sink_crc);\
-   print_crc("Calculated CRC:", _);\
-   \
-   igt_assert(wanted_crc); \
-   igt_assert_crc_equal(_.pipe, _crc->pipe);\
-   if (mandatory_sink_crc) \
-   assert_sink_crc_equal(_.sink, _crc->sink);   \
-   else if (sink_crc.reliable &&   \
-!sink_crc_equal(_.sink, _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); 

Re: [Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[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 
> > Signed-off-by: Daniel Vetter 
> > ---
> >  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(_, mandatory_sink_crc);
> > \
> > -   print_crc("Calculated CRC:", _);
> > \
> > -   
> > \
> > -   igt_assert(wanted_crc); 
> > \
> > -   igt_assert_crc_equal(_.pipe, _crc->pipe);
> > \
> > -   if (mandatory_sink_crc) 
> > \
> > -   assert_sink_crc_equal(_.sink, _crc-
> > >sink); \
> > -   else if (sink_crc.reliable &&   
> > \
> > -    !sink_crc_equal(_.sink, 

Re: [Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[Paulo Zanoni]

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 
> Signed-off-by: Daniel Vetter 
> ---
>  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(_, mandatory_sink_crc);
> \
> - print_crc("Calculated CRC:", _);
> \
> - 
> \
> - igt_assert(wanted_crc); 
>   \
> - igt_assert_crc_equal(_.pipe, _crc->pipe);
> \
> - if (mandatory_sink_crc) 
>   \
> - assert_sink_crc_equal(_.sink, _crc-
> >sink);   \
> - else if (sink_crc.reliable &&   
>   \
> -  !sink_crc_equal(_.sink, _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");  
>   \
> - }   
> \
> 

[Intel-gfx] [PATCH i-g-t] tests/kms_frontbuffer_tracking: convert macros to functions

[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 
Signed-off-by: Daniel Vetter 
---
 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(_, mandatory_sink_crc);\
-   print_crc("Calculated CRC:", _);\
-   \
-   igt_assert(wanted_crc); \
-   igt_assert_crc_equal(_.pipe, _crc->pipe);\
-   if (mandatory_sink_crc) \
-   assert_sink_crc_equal(_.sink, _crc->sink);   \
-   else if (sink_crc.reliable &&   \
-!sink_crc_equal(_.sink, _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; \
-