[igt-dev] [PATCH i-g-t] igt/drv_module_reload: Don't reload on exit

[igt-dev] [PATCH i-g-t] igt/drv_module_reload: Don't reload on exit

[Chris Wilson]

The next test will happily load whatever module it requires.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/drv_module_reload.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c
index 34f55eab1..4c06e4caf 100644
--- a/tests/drv_module_reload.c
+++ b/tests/drv_module_reload.c
@@ -340,21 +340,21 @@ hda_dynamic_debug(bool enable)
 
 igt_main
 {
-	int err;
-
-	igt_fixture
+	igt_subtest("basic-reload") {
 		hda_dynamic_debug(true);
 
-	igt_subtest("basic-reload") {
-		if ((err = reload(NULL)))
-			igt_fail(err);
+		igt_assert_eq(reload(NULL), 0);
 
 		gem_sanitycheck();
 		gem_exec_store();
+
+		hda_dynamic_debug(false);
 	}
 
-	igt_subtest("basic-no-display")
+	igt_subtest("basic-no-display") {
 		igt_assert_eq(reload("disable_display=1"), 0);
+		igt_i915_driver_unload();
+	}
 
 	igt_subtest("basic-reload-inject") {
 		int i = 0;
@@ -367,13 +367,4 @@ igt_main
 		/* We expect to hit at least one fault! */
 		igt_assert(i > 1);
 	}
-
-	igt_fixture {
-		if ((err = reload(NULL)))
-			igt_fail(err);
-
-		gem_sanitycheck();
-		gem_exec_store();
-		hda_dynamic_debug(false);
-	}
 }
-- 
2.18.0

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

[igt-dev] ✗ Fi.CI.BAT: failure for igt/drv_module_reload: Don't reload on exit

[Patchwork]

== Series Details ==

Series: igt/drv_module_reload: Don't reload on exit
URL   : https://patchwork.freedesktop.org/series/47326/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4547 -> IGTPW_1656 =

== Summary - FAILURE ==

  Serious unknown changes coming with IGTPW_1656 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1656, 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/47326/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@debugfs_test@read_all_entries:
      {fi-icl-u}:         NOTRUN -> DMESG-WARN

    igt@drv_module_reload@basic-reload-inject:
      fi-pnv-d510:        NOTRUN -> INCOMPLETE

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-no-display:
      fi-cnl-psr:         NOTRUN -> DMESG-WARN (fdo#105395) +2

    igt@gem_exec_suspend@basic-s3:
      {fi-skl-caroline}:  NOTRUN -> INCOMPLETE (fdo#104108)

    igt@gem_workarounds@basic-read:
      {fi-icl-u}:         NOTRUN -> FAIL (fdo#107338)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      {fi-icl-u}:         NOTRUN -> DMESG-WARN (fdo#107382) +4

    {igt@kms_psr@cursor_plane_move}:
      fi-cnl-psr:         NOTRUN -> DMESG-FAIL (fdo#107372) +1

    {igt@kms_psr@primary_mmap_gtt}:
      fi-cnl-psr:         NOTRUN -> DMESG-WARN (fdo#107372)

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

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#106248, fdo#106725) -> PASS

    igt@gem_exec_create@basic:
      fi-glk-j4005:       DMESG-WARN (fdo#106745) -> PASS

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

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

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105395 https://bugs.freedesktop.org/show_bug.cgi?id=105395
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#106745 https://bugs.freedesktop.org/show_bug.cgi?id=106745
  fdo#107338 https://bugs.freedesktop.org/show_bug.cgi?id=107338
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
  fdo#107382 https://bugs.freedesktop.org/show_bug.cgi?id=107382
  fdo#107383 https://bugs.freedesktop.org/show_bug.cgi?id=107383


== Participating hosts (48 -> 45) ==

  Additional (5): fi-byt-j1900 fi-skl-caroline fi-icl-u fi-cnl-psr fi-pnv-d510 
  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-skl-guc fi-byt-squawks fi-bsw-cyan fi-kbl-guc fi-ctg-p8600 fi-byt-clapper 


== Build changes ==

    * IGT: IGT_4575 -> IGTPW_1656

  CI_DRM_4547: 0a7ab192a697e951b2404f3c1ce42c5fa74f9ed1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1656: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1656/
  IGT_4575: fe908a01012c9daafafb3410b9407725ca9d4f21 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

Re: [igt-dev] [PATCH i-g-t] igt/drv_module_reload: Don't reload on exit

[Petri Latvala]

On Fri, Jul 27, 2018 at 09:06:57AM +0100, Chris Wilson wrote:
> The next test will happily load whatever module it requires.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/drv_module_reload.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)


I was going to comment that leaving a driver loaded with whatever
errorly parameters the tests use is bad... But you changed the
no-display subtest to also ensure the driver is unloaded when it's
done. I tend to forget conventions like that easily, can we have
comments in there stating that subtests should make sure to leave the
driver in either a valid reloaded state or unloaded?


> 
> diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c
> index 34f55eab1..4c06e4caf 100644
> --- a/tests/drv_module_reload.c
> +++ b/tests/drv_module_reload.c
> @@ -340,21 +340,21 @@ hda_dynamic_debug(bool enable)
>  
>  igt_main
>  {
> -	int err;
> -
> -	igt_fixture
> +	igt_subtest("basic-reload") {
>  		hda_dynamic_debug(true);
>  
> -	igt_subtest("basic-reload") {
> -		if ((err = reload(NULL)))
> -			igt_fail(err);
> +		igt_assert_eq(reload(NULL), 0);
>  
>  		gem_sanitycheck();
>  		gem_exec_store();
> +
> +		hda_dynamic_debug(false);
>  	}


hda_dynamic_debug is no longer active for other subtests then. Is it
not useful for them?




>  
> -	igt_subtest("basic-no-display")
> +	igt_subtest("basic-no-display") {
>  		igt_assert_eq(reload("disable_display=1"), 0);
> +		igt_i915_driver_unload();
> +	}
>  
>  	igt_subtest("basic-reload-inject") {
>  		int i = 0;
> @@ -367,13 +367,4 @@ igt_main
>  		/* We expect to hit at least one fault! */
>  		igt_assert(i > 1);


Tautological assert here btw.



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

Re: [igt-dev] [PATCH i-g-t] igt/drv_module_reload: Don't reload on exit

[Petri Latvala]

On Fri, Jul 27, 2018 at 12:23:28PM +0300, Petri Latvala wrote:
> >  
> >  	igt_subtest("basic-reload-inject") {
> >  		int i = 0;
> > @@ -367,13 +367,4 @@ igt_main
> >  		/* We expect to hit at least one fault! */
> >  		igt_assert(i > 1);
> 
> 
> Tautological assert here btw.


Okay, scratch that. I had a sip of coffee and managed to read the code
right this time.



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

Re: [igt-dev] [PATCH i-g-t] igt/drv_module_reload: Don't reload on exit

[Chris Wilson]

Quoting Petri Latvala (2018-07-27 10:23:28)
> > diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c
> > index 34f55eab1..4c06e4caf 100644
> > --- a/tests/drv_module_reload.c
> > +++ b/tests/drv_module_reload.c
> > @@ -340,21 +340,21 @@ hda_dynamic_debug(bool enable)
> >  
> >  igt_main
> >  {
> > -     int err;
> > -
> > -     igt_fixture
> > +     igt_subtest("basic-reload") {
> >               hda_dynamic_debug(true);
> >  
> > -     igt_subtest("basic-reload") {
> > -             if ((err = reload(NULL)))
> > -                     igt_fail(err);
> > +             igt_assert_eq(reload(NULL), 0);
> >  
> >               gem_sanitycheck();
> >               gem_exec_store();
> > +
> > +             hda_dynamic_debug(false);
> >       }
> 
> 
> hda_dynamic_debug is no longer active for other subtests then. Is it
> not useful for them?

Tbh, not sure what value hda_dynamic_debug provides. My presumption was
that it helps show the module ordering on load, in which case we do only
need it once around the basic reload. The purpose of the other tests is
not so much checking the ordinary module load but to investigate other
side-effects. Since it is not the purpose of the other tests to debug
hda, I'd rather not have any extra noise.

> > -     igt_subtest("basic-no-display")
> > +     igt_subtest("basic-no-display") {
> >               igt_assert_eq(reload("disable_display=1"), 0);
> > +             igt_i915_driver_unload();
> > +     }
> >  
> >       igt_subtest("basic-reload-inject") {
> >               int i = 0;
> > @@ -367,13 +367,4 @@ igt_main
> >               /* We expect to hit at least one fault! */
> >               igt_assert(i > 1);
> 
> 
> Tautological assert here btw.

Is it? If inject_load() breaks on the first iteration (no module, or no
fault injection modparam), i is left at 1.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t] igt/drv_module_reload: Don't reload on exit

[Chris Wilson]

The next test will happily load whatever module it requires.

v2: Unload at start, mandate tests cleanup after themselves.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Petri Latvala <petri.latvala@intel.com>
---
 tests/drv_module_reload.c | 48 ++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c
index 34f55eab1..cf4aea80d 100644
--- a/tests/drv_module_reload.c
+++ b/tests/drv_module_reload.c
@@ -220,20 +220,6 @@ static void store_all(int fd)
 	igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
 }
 
-static int
-reload(const char *opts_i915)
-{
-	int err = IGT_EXIT_SUCCESS;
-
-	if ((err = igt_i915_driver_unload()))
-		return err;
-
-	if ((err = igt_i915_driver_load(opts_i915)))
-		return err;
-
-	return err;
-}
-
 static int open_parameters(const char *module_name)
 {
 	char path[256];
@@ -340,21 +326,30 @@ hda_dynamic_debug(bool enable)
 
 igt_main
 {
-	int err;
+	igt_subtest("basic-reload") {
+		int load_error;
+
+		igt_i915_driver_unload();
 
-	igt_fixture
 		hda_dynamic_debug(true);
+		load_error = igt_i915_driver_load(NULL);
+		hda_dynamic_debug(false);
 
-	igt_subtest("basic-reload") {
-		if ((err = reload(NULL)))
-			igt_fail(err);
+		igt_assert_eq(load_error, 0);
 
 		gem_sanitycheck();
 		gem_exec_store();
+
+		/* only default modparams, can leave module loaded */
 	}
 
-	igt_subtest("basic-no-display")
-		igt_assert_eq(reload("disable_display=1"), 0);
+	igt_subtest("basic-no-display") {
+		igt_i915_driver_unload();
+
+		igt_assert_eq(igt_i915_driver_load("disable_display=1"), 0);
+
+		igt_i915_driver_unload();
+	}
 
 	igt_subtest("basic-reload-inject") {
 		int i = 0;
@@ -366,14 +361,9 @@ igt_main
 
 		/* We expect to hit at least one fault! */
 		igt_assert(i > 1);
-	}
-
-	igt_fixture {
-		if ((err = reload(NULL)))
-			igt_fail(err);
 
-		gem_sanitycheck();
-		gem_exec_store();
-		hda_dynamic_debug(false);
+		/* inject_fault() leaves the module unloaded */
 	}
+
+	/* Subtests should unload the module themselves if they use modparams */
 }
-- 
2.18.0

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

Re: [igt-dev] [PATCH i-g-t] igt/drv_module_reload: Don't reload on exit

[Petri Latvala]

On Fri, Jul 27, 2018 at 11:04:35AM +0100, Chris Wilson wrote:
> The next test will happily load whatever module it requires.
> 
> v2: Unload at start, mandate tests cleanup after themselves.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Petri Latvala <petri.latvala@intel.com>


Reviewed-by: Petri Latvala <petri.latvala@intel.com>


> ---
>  tests/drv_module_reload.c | 48 ++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 29 deletions(-)
> 
> diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c
> index 34f55eab1..cf4aea80d 100644
> --- a/tests/drv_module_reload.c
> +++ b/tests/drv_module_reload.c
> @@ -220,20 +220,6 @@ static void store_all(int fd)
>  	igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
>  }
>  
> -static int
> -reload(const char *opts_i915)
> -{
> -	int err = IGT_EXIT_SUCCESS;
> -
> -	if ((err = igt_i915_driver_unload()))
> -		return err;
> -
> -	if ((err = igt_i915_driver_load(opts_i915)))
> -		return err;
> -
> -	return err;
> -}
> -
>  static int open_parameters(const char *module_name)
>  {
>  	char path[256];
> @@ -340,21 +326,30 @@ hda_dynamic_debug(bool enable)
>  
>  igt_main
>  {
> -	int err;
> +	igt_subtest("basic-reload") {
> +		int load_error;
> +
> +		igt_i915_driver_unload();
>  
> -	igt_fixture
>  		hda_dynamic_debug(true);
> +		load_error = igt_i915_driver_load(NULL);
> +		hda_dynamic_debug(false);
>  
> -	igt_subtest("basic-reload") {
> -		if ((err = reload(NULL)))
> -			igt_fail(err);
> +		igt_assert_eq(load_error, 0);
>  
>  		gem_sanitycheck();
>  		gem_exec_store();
> +
> +		/* only default modparams, can leave module loaded */
>  	}
>  
> -	igt_subtest("basic-no-display")
> -		igt_assert_eq(reload("disable_display=1"), 0);
> +	igt_subtest("basic-no-display") {
> +		igt_i915_driver_unload();
> +
> +		igt_assert_eq(igt_i915_driver_load("disable_display=1"), 0);
> +
> +		igt_i915_driver_unload();
> +	}
>  
>  	igt_subtest("basic-reload-inject") {
>  		int i = 0;
> @@ -366,14 +361,9 @@ igt_main
>  
>  		/* We expect to hit at least one fault! */
>  		igt_assert(i > 1);
> -	}
> -
> -	igt_fixture {
> -		if ((err = reload(NULL)))
> -			igt_fail(err);
>  
> -		gem_sanitycheck();
> -		gem_exec_store();
> -		hda_dynamic_debug(false);
> +		/* inject_fault() leaves the module unloaded */
>  	}
> +
> +	/* Subtests should unload the module themselves if they use modparams */
>  }
> -- 
> 2.18.0
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev