[igt-dev] [PATH i-g-t 1/2] intel: Be consistent with test results on simulation

[igt-dev] [PATH i-g-t 1/2] intel: Be consistent with test results on simulation

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Tests which call intel_require_memory currently always skip on simulation,
unless they fail first due insufficient memory. This can create different
outcomes depending on the simulation environment so move the simulation
skip to the start of the function for 100% consistency in results.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
---
 lib/intel_os.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/intel_os.c b/lib/intel_os.c
index 29a27272e782..e1e31e23069b 100644
--- a/lib/intel_os.c
+++ b/lib/intel_os.c
@@ -375,6 +375,8 @@ void intel_require_memory(uint64_t count, uint64_t size, unsigned mode)
 	uint64_t required, total;
 	bool sufficient_memory;
 
+	igt_skip_on_simulation();
+
 	sufficient_memory = __intel_check_memory(count, size, mode,
 						 &required, &total);
 	if (!sufficient_memory) {
@@ -406,8 +408,6 @@ void intel_require_memory(uint64_t count, uint64_t size, unsigned mode)
 		      mode & (CHECK_RAM | CHECK_SWAP) ? "RAM" : "",
 		      mode & CHECK_SWAP ? " + swap": "",
 		      (long long)vfs_file_max());
-
-	igt_skip_on_simulation();
 }
 
 void intel_purge_vm_caches(int drm_fd)
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[Intel-gfx] [PATH i-g-t 2/2] core: Show backtrace from igt_skip_on_simulation

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

igt_skip_on_simulation is called both directly from tests but also from
library helpers. In the latter case especially the logged caller name is
useless since it is always the helper itself. What we instead want to know
is who is the caller.

Trivial approach would be to move the helper to a header as static inline,
but due the longjmp in it it can never be inlined. Alternative option is
to print a backtrace from it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
---
 lib/igt_core.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 23bb858fd886..990abc5a36b3 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void)
  */
 void igt_skip_on_simulation(void)
 {
+	bool in_simulation;
+
 	if (igt_only_list_subtests())
 		return;
 
+	in_simulation = igt_run_in_simulation();
+
 	if (!igt_can_fail()) {
-		igt_fixture
-			igt_require(!igt_run_in_simulation());
-	} else
-		igt_require(!igt_run_in_simulation());
+		igt_fixture {
+			if (in_simulation) {
+				print_backtrace();
+				igt_require(!in_simulation);
+			}
+		}
+	} else {
+		if (in_simulation) {
+			print_backtrace();
+			igt_require(!in_simulation);
+		}
+	}
 }
 
 /* structured logging */
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [PATH,i-g-t,1/2] intel: Be consistent with test results on simulation

[Patchwork]

== Series Details ==

Series: series starting with [PATH,i-g-t,1/2] intel: Be consistent with test results on simulation
URL   : https://patchwork.freedesktop.org/series/49542/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4807 -> IGTPW_1829 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49542/revisions/1/mbox/

== Known issues ==

  Here are the changes found in IGTPW_1829 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-kbl-guc:         PASS -> DMESG-FAIL (fdo#107710, fdo#107860)

    igt@gem_exec_suspend@basic-s3:
      fi-kbl-soraka:      NOTRUN -> INCOMPLETE (fdo#107774, fdo#107556)

    igt@gem_mmap@basic-small-bo:
      fi-glk-dsi:         PASS -> INCOMPLETE (fdo#103359, k.org#198133)

    igt@kms_psr@primary_mmap_gtt:
      {fi-cnl-u}:         NOTRUN -> FAIL (fdo#107383) +3

    igt@kms_psr@primary_page_flip:
      fi-kbl-r:           PASS -> FAIL (fdo#107336)

    igt@kms_psr@sprite_plane_onoff:
      fi-bdw-samus:       NOTRUN -> FAIL (fdo#107383, fdo#107360)

    igt@pm_rpm@module-reload:
      fi-hsw-peppy:       NOTRUN -> DMESG-WARN (fdo#107603)
      fi-bdw-samus:       NOTRUN -> DMESG-WARN (fdo#107603)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-cfl-8109u:       INCOMPLETE (fdo#106070) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107360 https://bugs.freedesktop.org/show_bug.cgi?id=107360
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107383 https://bugs.freedesktop.org/show_bug.cgi?id=107383
  fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
  fdo#107603 https://bugs.freedesktop.org/show_bug.cgi?id=107603
  fdo#107710 https://bugs.freedesktop.org/show_bug.cgi?id=107710
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107774 https://bugs.freedesktop.org/show_bug.cgi?id=107774
  fdo#107860 https://bugs.freedesktop.org/show_bug.cgi?id=107860
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (46 -> 46) ==

  Additional (5): fi-kbl-soraka fi-cnl-u fi-icl-u fi-hsw-peppy fi-bdw-samus 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * IGT: IGT_4639 -> IGTPW_1829

  CI_DRM_4807: 55b148b84b254f61adbfeb89c4f6674664f01c46 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1829: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1829/
  IGT_4639: c7fa2ea9fbce87206474748100b825558eebe08e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1829/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [Intel-gfx] [PATH i-g-t 1/2] intel: Be consistent with test results on simulation

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-09-12 10:33:05)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Tests which call intel_require_memory currently always skip on simulation,
> unless they fail first due insufficient memory. This can create different
> outcomes depending on the simulation environment so move the simulation
> skip to the start of the function for 100% consistency in results.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>

Makes sense.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for series starting with [PATH,i-g-t,1/2] intel: Be consistent with test results on simulation

[Patchwork]

== Series Details ==

Series: series starting with [PATH,i-g-t,1/2] intel: Be consistent with test results on simulation
URL   : https://patchwork.freedesktop.org/series/49542/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4639_full -> IGTPW_1829_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1829_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1829_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49542/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1829_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in IGTPW_1829_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
      shard-glk:          PASS -> FAIL (fdo#103167)
      shard-kbl:          PASS -> FAIL (fdo#103167)

    igt@kms_setmode@basic:
      shard-glk:          PASS -> FAIL (fdo#99912)

    igt@perf_pmu@rc6-runtime-pm:
      shard-apl:          PASS -> FAIL (fdo#105010)

    
    ==== Possible fixes ====

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
      shard-hsw:          FAIL (fdo#105767) -> PASS

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-snb:          FAIL (fdo#103166) -> PASS

    igt@perf_pmu@all-busy-check-all:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105010 https://bugs.freedesktop.org/show_bug.cgi?id=105010
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * IGT: IGT_4639 -> IGTPW_1829
    * Linux: CI_DRM_4806 -> CI_DRM_4807

  CI_DRM_4806: feeccde66999c5e87be3550f2159e5d7eeb61c67 @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4807: 55b148b84b254f61adbfeb89c4f6674664f01c46 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1829: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1829/
  IGT_4639: c7fa2ea9fbce87206474748100b825558eebe08e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1829/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATH i-g-t 2/2] core: Show backtrace from igt_skip_on_simulation

[Daniel Vetter]

On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> igt_skip_on_simulation is called both directly from tests but also from
> library helpers. In the latter case especially the logged caller name is
> useless since it is always the helper itself. What we instead want to know
> is who is the caller.
> 
> Trivial approach would be to move the helper to a header as static inline,
> but due the longjmp in it it can never be inlined. Alternative option is
> to print a backtrace from it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
> ---
>  lib/igt_core.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 23bb858fd886..990abc5a36b3 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void)
>   */
>  void igt_skip_on_simulation(void)
>  {
> +	bool in_simulation;
> +
>  	if (igt_only_list_subtests())
>  		return;
>  
> +	in_simulation = igt_run_in_simulation();
> +
>  	if (!igt_can_fail()) {
> -		igt_fixture
> -			igt_require(!igt_run_in_simulation());
> -	} else
> -		igt_require(!igt_run_in_simulation());
> +		igt_fixture {
> +			if (in_simulation) {
> +				print_backtrace();
> +				igt_require(!in_simulation);
> +			}
> +		}
> +	} else {
> +		if (in_simulation) {
> +			print_backtrace();
> +			igt_require(!in_simulation);

Hm, why don't we go right ahead and push this into igt_skip()? There's a
pile of other igt_require, and we tend to push a lot of these into the
library. So they have all the same problem.
-Daniel

> +		}
> +	}
>  }
>  
>  /* structured logging */
> -- 
> 2.17.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATH i-g-t 2/2] core: Show backtrace from igt_skip_on_simulation

[Tvrtko Ursulin]

On 14/09/2018 10:12, Daniel Vetter wrote:
> On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> igt_skip_on_simulation is called both directly from tests but also from
>> library helpers. In the latter case especially the logged caller name is
>> useless since it is always the helper itself. What we instead want to know
>> is who is the caller.
>>
>> Trivial approach would be to move the helper to a header as static inline,
>> but due the longjmp in it it can never be inlined. Alternative option is
>> to print a backtrace from it.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
>> ---
>>   lib/igt_core.c | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/igt_core.c b/lib/igt_core.c
>> index 23bb858fd886..990abc5a36b3 100644
>> --- a/lib/igt_core.c
>> +++ b/lib/igt_core.c
>> @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void)
>>    */
>>   void igt_skip_on_simulation(void)
>>   {
>> +	bool in_simulation;
>> +
>>   	if (igt_only_list_subtests())
>>   		return;
>>   
>> +	in_simulation = igt_run_in_simulation();
>> +
>>   	if (!igt_can_fail()) {
>> -		igt_fixture
>> -			igt_require(!igt_run_in_simulation());
>> -	} else
>> -		igt_require(!igt_run_in_simulation());
>> +		igt_fixture {
>> +			if (in_simulation) {
>> +				print_backtrace();
>> +				igt_require(!in_simulation);
>> +			}
>> +		}
>> +	} else {
>> +		if (in_simulation) {
>> +			print_backtrace();
>> +			igt_require(!in_simulation);
> 
> Hm, why don't we go right ahead and push this into igt_skip()? There's a
> pile of other igt_require, and we tend to push a lot of these into the
> library. So they have all the same problem.

Maybe.. I wasn't 100% this was a good idea to start with, or in another 
words, that other people would consider it a problem. Since the downside 
is test output gets more verbose on skips, or some could say more noisy.

So I basically floated the patch to see if it will provoke some 
responses. :)

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATH i-g-t 2/2] core: Show backtrace from igt_skip_on_simulation

[Daniel Vetter]

On Fri, Sep 14, 2018 at 10:19:29AM +0100, Tvrtko Ursulin wrote:
> 
> On 14/09/2018 10:12, Daniel Vetter wrote:
> > On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > igt_skip_on_simulation is called both directly from tests but also from
> > > library helpers. In the latter case especially the logged caller name is
> > > useless since it is always the helper itself. What we instead want to know
> > > is who is the caller.
> > > 
> > > Trivial approach would be to move the helper to a header as static inline,
> > > but due the longjmp in it it can never be inlined. Alternative option is
> > > to print a backtrace from it.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
> > > ---
> > >   lib/igt_core.c | 20 ++++++++++++++++----
> > >   1 file changed, 16 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > index 23bb858fd886..990abc5a36b3 100644
> > > --- a/lib/igt_core.c
> > > +++ b/lib/igt_core.c
> > > @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void)
> > >    */
> > >   void igt_skip_on_simulation(void)
> > >   {
> > > +	bool in_simulation;
> > > +
> > >   	if (igt_only_list_subtests())
> > >   		return;
> > > +	in_simulation = igt_run_in_simulation();
> > > +
> > >   	if (!igt_can_fail()) {
> > > -		igt_fixture
> > > -			igt_require(!igt_run_in_simulation());
> > > -	} else
> > > -		igt_require(!igt_run_in_simulation());
> > > +		igt_fixture {
> > > +			if (in_simulation) {
> > > +				print_backtrace();
> > > +				igt_require(!in_simulation);
> > > +			}
> > > +		}
> > > +	} else {
> > > +		if (in_simulation) {
> > > +			print_backtrace();
> > > +			igt_require(!in_simulation);
> > 
> > Hm, why don't we go right ahead and push this into igt_skip()? There's a
> > pile of other igt_require, and we tend to push a lot of these into the
> > library. So they have all the same problem.
> 
> Maybe.. I wasn't 100% this was a good idea to start with, or in another
> words, that other people would consider it a problem. Since the downside is
> test output gets more verbose on skips, or some could say more noisy.
> 
> So I basically floated the patch to see if it will provoke some responses.
> :)

We have the backtrace already in igt_fail, makes total sense to add it to
igt_skip too. Has my ack at least.

Aside: I kinda wonder whether we need an __igt_require, which embeds the
if (igt_can_fail) igt_require() else igt_fixture igt_require() thing. But
that's just an aside.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [Intel-gfx] [PATH i-g-t 2/2] core: Show backtrace from igt_skip_on_simulation

[Chris Wilson]

Quoting Daniel Vetter (2018-09-14 10:46:25)
> On Fri, Sep 14, 2018 at 10:19:29AM +0100, Tvrtko Ursulin wrote:
> > 
> > On 14/09/2018 10:12, Daniel Vetter wrote:
> > > On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote:
> > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > 
> > > > igt_skip_on_simulation is called both directly from tests but also from
> > > > library helpers. In the latter case especially the logged caller name is
> > > > useless since it is always the helper itself. What we instead want to know
> > > > is who is the caller.
> > > > 
> > > > Trivial approach would be to move the helper to a header as static inline,
> > > > but due the longjmp in it it can never be inlined. Alternative option is
> > > > to print a backtrace from it.
> > > > 
> > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
> > > > ---
> > > >   lib/igt_core.c | 20 ++++++++++++++++----
> > > >   1 file changed, 16 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > > index 23bb858fd886..990abc5a36b3 100644
> > > > --- a/lib/igt_core.c
> > > > +++ b/lib/igt_core.c
> > > > @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void)
> > > >    */
> > > >   void igt_skip_on_simulation(void)
> > > >   {
> > > > + bool in_simulation;
> > > > +
> > > >           if (igt_only_list_subtests())
> > > >                   return;
> > > > + in_simulation = igt_run_in_simulation();
> > > > +
> > > >           if (!igt_can_fail()) {
> > > > -         igt_fixture
> > > > -                 igt_require(!igt_run_in_simulation());
> > > > - } else
> > > > -         igt_require(!igt_run_in_simulation());
> > > > +         igt_fixture {
> > > > +                 if (in_simulation) {
> > > > +                         print_backtrace();
> > > > +                         igt_require(!in_simulation);
> > > > +                 }
> > > > +         }
> > > > + } else {
> > > > +         if (in_simulation) {
> > > > +                 print_backtrace();
> > > > +                 igt_require(!in_simulation);
> > > 
> > > Hm, why don't we go right ahead and push this into igt_skip()? There's a
> > > pile of other igt_require, and we tend to push a lot of these into the
> > > library. So they have all the same problem.
> > 
> > Maybe.. I wasn't 100% this was a good idea to start with, or in another
> > words, that other people would consider it a problem. Since the downside is
> > test output gets more verbose on skips, or some could say more noisy.
> > 
> > So I basically floated the patch to see if it will provoke some responses.
> > :)
> 
> We have the backtrace already in igt_fail, makes total sense to add it to
> igt_skip too. Has my ack at least.

Skip is rather more frequent than fail, and more often than not,
expected. So I am not entirely enjoying the prospect of a lot more noise,
the requirement message was supposed to be sufficient to understand why
we skipped. Maybe enforce that we have no igt_skip without a message?
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [Intel-gfx] [PATH i-g-t 2/2] core: Show backtrace from igt_skip_on_simulation

[Daniel Vetter]

On Fri, Sep 14, 2018 at 10:49:47AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-09-14 10:46:25)
> > On Fri, Sep 14, 2018 at 10:19:29AM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 14/09/2018 10:12, Daniel Vetter wrote:
> > > > On Wed, Sep 12, 2018 at 10:33:06AM +0100, Tvrtko Ursulin wrote:
> > > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > > 
> > > > > igt_skip_on_simulation is called both directly from tests but also from
> > > > > library helpers. In the latter case especially the logged caller name is
> > > > > useless since it is always the helper itself. What we instead want to know
> > > > > is who is the caller.
> > > > > 
> > > > > Trivial approach would be to move the helper to a header as static inline,
> > > > > but due the longjmp in it it can never be inlined. Alternative option is
> > > > > to print a backtrace from it.
> > > > > 
> > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
> > > > > ---
> > > > >   lib/igt_core.c | 20 ++++++++++++++++----
> > > > >   1 file changed, 16 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > > > index 23bb858fd886..990abc5a36b3 100644
> > > > > --- a/lib/igt_core.c
> > > > > +++ b/lib/igt_core.c
> > > > > @@ -2065,14 +2065,26 @@ bool igt_run_in_simulation(void)
> > > > >    */
> > > > >   void igt_skip_on_simulation(void)
> > > > >   {
> > > > > + bool in_simulation;
> > > > > +
> > > > >           if (igt_only_list_subtests())
> > > > >                   return;
> > > > > + in_simulation = igt_run_in_simulation();
> > > > > +
> > > > >           if (!igt_can_fail()) {
> > > > > -         igt_fixture
> > > > > -                 igt_require(!igt_run_in_simulation());
> > > > > - } else
> > > > > -         igt_require(!igt_run_in_simulation());
> > > > > +         igt_fixture {
> > > > > +                 if (in_simulation) {
> > > > > +                         print_backtrace();
> > > > > +                         igt_require(!in_simulation);
> > > > > +                 }
> > > > > +         }
> > > > > + } else {
> > > > > +         if (in_simulation) {
> > > > > +                 print_backtrace();
> > > > > +                 igt_require(!in_simulation);
> > > > 
> > > > Hm, why don't we go right ahead and push this into igt_skip()? There's a
> > > > pile of other igt_require, and we tend to push a lot of these into the
> > > > library. So they have all the same problem.
> > > 
> > > Maybe.. I wasn't 100% this was a good idea to start with, or in another
> > > words, that other people would consider it a problem. Since the downside is
> > > test output gets more verbose on skips, or some could say more noisy.
> > > 
> > > So I basically floated the patch to see if it will provoke some responses.
> > > :)
> > 
> > We have the backtrace already in igt_fail, makes total sense to add it to
> > igt_skip too. Has my ack at least.
> 
> Skip is rather more frequent than fail, and more often than not,
> expected. So I am not entirely enjoying the prospect of a lot more noise,
> the requirement message was supposed to be sufficient to understand why
> we skipped. Maybe enforce that we have no igt_skip without a message?

Hm yeah, adding a const char * to igt_skip_on_simulation that explains why
we're skipping could be the pretty solution here. Would also serve as some
self-documentation.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev