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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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