[igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require

[igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require

[Daniel Vetter]

Need to extract into a test subgroup to make sure we only skip the
tests that need display support.

Cc: Antonio Argenziano <antonio.argenziano@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 tests/debugfs_test.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/tests/debugfs_test.c b/tests/debugfs_test.c
index 2e87e4420b15..eb32932ed686 100644
--- a/tests/debugfs_test.c
+++ b/tests/debugfs_test.c
@@ -87,23 +87,14 @@ static void read_and_discard_sysfs_entries(int path_fd, int indent)
 	closedir(dir);
 }
 
-igt_main
+static void kms_tests(int fd, int debugfs)
 {
-	int fd = -1, debugfs;
 	igt_display_t display;
 	struct igt_fb fb[IGT_MAX_PIPES];
 	enum pipe pipe;
 
-	igt_skip_on_simulation();
-
-	igt_fixture {
-		fd = drm_open_driver_master(DRIVER_INTEL);
-		igt_require_gem(fd);
-		debugfs = igt_debugfs_dir(fd);
-
-		kmstest_set_vt_graphics_mode();
-		igt_display_init(&display, fd);
-	}
+	igt_fixture
+		igt_display_require(&display, fd);
 
 	igt_subtest("read_all_entries") {
 		/* try to light all pipes */
@@ -152,6 +143,27 @@ igt_main
 		read_and_discard_sysfs_entries(debugfs, 0);
 	}
 
+	igt_fixture
+		igt_display_fini(&display);
+}
+
+igt_main
+{
+	int fd = -1, debugfs;
+
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		fd = drm_open_driver_master(DRIVER_INTEL);
+		igt_require_gem(fd);
+		debugfs = igt_debugfs_dir(fd);
+
+		kmstest_set_vt_graphics_mode();
+	}
+
+	igt_subtest_group
+		kms_tests(fd, debugfs);
+
 	igt_subtest("emon_crash") {
 		int i;
 		/*
@@ -174,7 +186,6 @@ igt_main
 	}
 
 	igt_fixture {
-		igt_display_fini(&display);
 		close(debugfs);
 		close(fd);
 	}
-- 
2.19.0.rc2

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

[igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require

[Daniel Vetter]

Remaining tests that have been overlooked and don't need any
invasive changes to limit the skipping to only the relevant parts.

Cc: Antonio Argenziano <antonio.argenziano@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 lib/igt_chamelium.c               | 2 +-
 tests/kms_atomic_interruptible.c  | 4 ++--
 tests/kms_force_connector_basic.c | 2 +-
 tests/kms_getfb.c                 | 2 +-
 tests/kms_plane_alpha_blend.c     | 2 +-
 tests/perf_pmu.c                  | 2 +-
 tests/pm_backlight.c              | 2 +-
 tests/prime_mmap_kms.c            | 2 +-
 8 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index b8418e13e7bf..f7e9f851a964 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -1544,7 +1544,7 @@ static void chamelium_exit_handler(int sig)
 /**
  * chamelium_init:
  * @chamelium: The Chamelium instance to use
- * @drm_fd: a display initialized with #igt_display_init
+ * @drm_fd: a display initialized with #igt_display_require
  *
  * Sets up a connection with a chamelium, using the URL specified in the
  * Chamelium configuration. This must be called first before trying to use the
diff --git a/tests/kms_atomic_interruptible.c b/tests/kms_atomic_interruptible.c
index 8e9d4cb69c4d..be688638973f 100644
--- a/tests/kms_atomic_interruptible.c
+++ b/tests/kms_atomic_interruptible.c
@@ -85,8 +85,8 @@ static void run_plane_test(igt_display_t *display, enum pipe pipe, igt_output_t
 
 	/*
 	 * Make sure we start with everything disabled to force a real modeset.
-	 * igt_display_init only sets sw state, and assumes the first test doesn't care
-	 * about hw state.
+	 * igt_display_require only sets sw state, and assumes the first test
+	 * doesn't care about hw state.
 	 */
 	igt_display_commit2(display, COMMIT_ATOMIC);
 
diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
index e9325dec9305..b8246e669939 100644
--- a/tests/kms_force_connector_basic.c
+++ b/tests/kms_force_connector_basic.c
@@ -217,7 +217,7 @@ int main(int argc, char **argv)
 
 		/* attempt to use the display */
 		kmstest_set_vt_graphics_mode();
-		igt_display_init(&display, drm_fd);
+		igt_display_require(&display, drm_fd);
 		igt_display_commit(&display);
 		igt_display_fini(&display);
 
diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
index 07ffd79c4613..ca0b01c05e5c 100644
--- a/tests/kms_getfb.c
+++ b/tests/kms_getfb.c
@@ -116,7 +116,7 @@ static uint32_t get_any_prop_id(int fd)
 {
 	igt_display_t display;
 
-	igt_display_init(&display, fd);
+	igt_display_require(&display, fd);
 	for (int i = 0; i < display.n_outputs; i++) {
 		igt_output_t *output = &display.outputs[i];
 		if (output->props[IGT_CONNECTOR_DPMS] != 0)
diff --git a/tests/kms_plane_alpha_blend.c b/tests/kms_plane_alpha_blend.c
index 81c8cb916a63..2194e26d536f 100644
--- a/tests/kms_plane_alpha_blend.c
+++ b/tests/kms_plane_alpha_blend.c
@@ -558,7 +558,7 @@ igt_main
 
 		data.gfx_fd = drm_open_driver(DRIVER_ANY);
 		igt_require_pipe_crc(data.gfx_fd);
-		igt_display_init(&data.display, data.gfx_fd);
+		igt_display_require(&data.display, data.gfx_fd);
 		igt_require(data.display.is_atomic);
 	}
 
diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
index b34bc66ce2c4..21292bf3a2fe 100644
--- a/tests/perf_pmu.c
+++ b/tests/perf_pmu.c
@@ -811,7 +811,7 @@ event_wait(int gem_fd, const struct intel_execution_engine2 *e)
 	igt_skip_on(IS_VALLEYVIEW(devid) || IS_CHERRYVIEW(devid));
 
 	kmstest_set_vt_graphics_mode();
-	igt_display_init(&data.display, gem_fd);
+	igt_display_require(&data.display, gem_fd);
 
 	/**
 	 * We will use the display to render event forwarind so need to
diff --git a/tests/pm_backlight.c b/tests/pm_backlight.c
index 32808cdf6ca4..054300f6e2e1 100644
--- a/tests/pm_backlight.c
+++ b/tests/pm_backlight.c
@@ -214,7 +214,7 @@ igt_main
 		 * try to enable all.
 		 */
 		kmstest_set_vt_graphics_mode();
-		igt_display_init(&display, drm_open_driver(DRIVER_INTEL));
+		igt_display_require(&display, drm_open_driver(DRIVER_INTEL));
 
 		/* should be ../../cardX-$output */
 		igt_assert_lt(12, readlink(BACKLIGHT_PATH "/device", full_name, sizeof(full_name) - 1));
diff --git a/tests/prime_mmap_kms.c b/tests/prime_mmap_kms.c
index faace4afd478..fdc37214d96d 100644
--- a/tests/prime_mmap_kms.c
+++ b/tests/prime_mmap_kms.c
@@ -248,7 +248,7 @@ igt_main
 
 		igt_require_pipe_crc(gpu.drm_fd);
 
-		igt_display_init(&gpu.display, gpu.drm_fd);
+		igt_display_require(&gpu.display, gpu.drm_fd);
 	}
 
 	igt_subtest("buffer-sharing")
-- 
2.19.0.rc2

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

[igt-dev] [PATCH i-g-t 3/4] lib/kms: Drop igt_display_init

[Daniel Vetter]

If you need the high-level functions, then you probably need a
full display. Unexport the non-requiring version, and adjust the
documentation. This also gives us proper docs for the recently
added igt_display_require.

Cc: Antonio Argenziano <antonio.argenziano@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 lib/igt_kms.c | 14 +++++---------
 lib/igt_kms.h |  1 -
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index b2cbaa114664..454ab7481cde 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1838,16 +1838,17 @@ static void igt_fill_plane_format_mod(igt_display_t *display, igt_plane_t *plane
 static void igt_fill_display_format_mod(igt_display_t *display);
 
 /**
- * igt_display_init:
+ * igt_display_require:
  * @display: a pointer to an #igt_display_t structure
  * @drm_fd: a drm file descriptor
  *
  * Initialize @display and allocate the various resources required. Use
  * #igt_display_fini to release the resources when they are no longer required.
  *
- * Returns: true if the display has outputs and pipes available, false otherwise
+ * This function automatically skips if the kernel driver doesn't support any
+ * CRTC or outputs.
  */
-bool igt_display_init(igt_display_t *display, int drm_fd)
+void igt_display_require(igt_display_t *display, int drm_fd)
 {
 	drmModeRes *resources;
 	drmModePlaneRes *plane_resources;
@@ -2011,12 +2012,7 @@ bool igt_display_init(igt_display_t *display, int drm_fd)
 out:
 	LOG_UNINDENT(display);
 
-	return display->n_pipes && display->n_outputs;
-}
-
-void igt_display_require(igt_display_t *display, int drm_fd)
-{
-	igt_require(igt_display_init(display, drm_fd));
+	igt_require(display->n_pipes && display->n_outputs);
 }
 
 /**
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 38fa944ef156..44e2090ca445 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -380,7 +380,6 @@ struct igt_display {
 	int format_mod_count;
 };
 
-bool igt_display_init(igt_display_t *display, int drm_fd);
 void igt_display_require(igt_display_t *display, int drm_fd);
 void igt_display_fini(igt_display_t *display);
 void igt_display_reset(igt_display_t *display);
-- 
2.19.0.rc2

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

[igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Daniel Vetter]

With the high-level helpers requiring outputs there's not point
in silently ignoring issues anymore. Complain about that if it
ever happens.

Cc: Antonio Argenziano <antonio.argenziano@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 lib/igt_kms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 454ab7481cde..867eaa7a971c 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -3269,7 +3269,7 @@ static int do_display_commit(igt_display_t *display,
 	enum pipe pipe;
 	LOG_INDENT(display, "commit");
 
-	if (!display->n_pipes || !display->n_outputs)
+	if (igt_warn_on(!display->n_pipes || !display->n_outputs))
 		return 0; /* nothing to do */
 
 	igt_display_refresh(display);
@@ -3322,7 +3322,7 @@ int igt_display_try_commit_atomic(igt_display_t *display, uint32_t flags, void *
 {
 	int ret;
 
-	if (!display->n_pipes || !display->n_outputs)
+	if (igt_warn_on(!display->n_pipes || !display->n_outputs))
 		return 0; /* nothing to do */
 
 	LOG_INDENT(display, "commit");
-- 
2.19.0.rc2

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

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/4] tests/debugfs: use igt_display_require

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/4] tests/debugfs: use igt_display_require
URL   : https://patchwork.freedesktop.org/series/50554/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4931 -> IGTPW_1908 =

== Summary - WARNING ==

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@debugfs_test@read_all_entries:
      fi-kbl-8809g:       PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-bdw-samus:       NOTRUN -> INCOMPLETE (fdo#107773)
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

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

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

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

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#108126 https://bugs.freedesktop.org/show_bug.cgi?id=108126


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

  Additional (2): fi-icl-u fi-bdw-samus 
  Missing    (9): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600 fi-gdg-551 fi-pnv-d510 fi-skl-6600u 


== Build changes ==

    * IGT: IGT_4667 -> IGTPW_1908

  CI_DRM_4931: 826702bf60ae2b37841c051ed769b44af194fbb1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1908: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1908/
  IGT_4667: 596f48dcd59fd2f8c16671514f3e69d4a2891374 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1908/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 1/4] tests/debugfs: use igt_display_require

[Chris Wilson]

Quoting Daniel Vetter (2018-10-04 14:21:25)
> Need to extract into a test subgroup to make sure we only skip the
> tests that need display support.
> 
> Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  tests/debugfs_test.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/debugfs_test.c b/tests/debugfs_test.c
> index 2e87e4420b15..eb32932ed686 100644
> --- a/tests/debugfs_test.c
> +++ b/tests/debugfs_test.c
> @@ -87,23 +87,14 @@ static void read_and_discard_sysfs_entries(int path_fd, int indent)
>         closedir(dir);
>  }
>  
> -igt_main
> +static void kms_tests(int fd, int debugfs)
>  {
> -       int fd = -1, debugfs;
>         igt_display_t display;
>         struct igt_fb fb[IGT_MAX_PIPES];
>         enum pipe pipe;
>  
> -       igt_skip_on_simulation();
> -
> -       igt_fixture {
> -               fd = drm_open_driver_master(DRIVER_INTEL);
> -               igt_require_gem(fd);
> -               debugfs = igt_debugfs_dir(fd);
> -
> -               kmstest_set_vt_graphics_mode();
> -               igt_display_init(&display, fd);
> -       }
> +       igt_fixture
> +               igt_display_require(&display, fd);
>  
>         igt_subtest("read_all_entries") {
>                 /* try to light all pipes */
> @@ -152,6 +143,27 @@ igt_main
>                 read_and_discard_sysfs_entries(debugfs, 0);
>         }
>  
> +       igt_fixture
> +               igt_display_fini(&display);
> +}
> +
> +igt_main
> +{
> +       int fd = -1, debugfs;
> +
> +       igt_skip_on_simulation();
> +
> +       igt_fixture {
> +               fd = drm_open_driver_master(DRIVER_INTEL);
> +               igt_require_gem(fd);
> +               debugfs = igt_debugfs_dir(fd);
> +
> +               kmstest_set_vt_graphics_mode();
> +       }
> +
> +       igt_subtest_group
> +               kms_tests(fd, debugfs);

Not quite. read_all_entries is the original *non-KMS* test.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require

[Chris Wilson]

Quoting Daniel Vetter (2018-10-04 14:21:26)
> diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
> index e9325dec9305..b8246e669939 100644
> --- a/tests/kms_force_connector_basic.c
> +++ b/tests/kms_force_connector_basic.c
> @@ -217,7 +217,7 @@ int main(int argc, char **argv)
>  
>                 /* attempt to use the display */
>                 kmstest_set_vt_graphics_mode();
> -               igt_display_init(&display, drm_fd);
> +               igt_display_require(&display, drm_fd);
>                 igt_display_commit(&display);
>                 igt_display_fini(&display);

Where is the requirement here? I'd buy that this should be an
igt_assert() since we did the forcing earlier.

> diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
> index 07ffd79c4613..ca0b01c05e5c 100644
> --- a/tests/kms_getfb.c
> +++ b/tests/kms_getfb.c
> @@ -116,7 +116,7 @@ static uint32_t get_any_prop_id(int fd)
>  {
>         igt_display_t display;
>  
> -       igt_display_init(&display, fd);
> +       igt_display_require(&display, fd);

Not required, as we describe the requirement of having the property
as an output of this function.

>         for (int i = 0; i < display.n_outputs; i++) {
>                 igt_output_t *output = &display.outputs[i];
>                 if (output->props[IGT_CONNECTOR_DPMS] != 0)
> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> index b34bc66ce2c4..21292bf3a2fe 100644
> --- a/tests/perf_pmu.c
> +++ b/tests/perf_pmu.c
> @@ -811,7 +811,7 @@ event_wait(int gem_fd, const struct intel_execution_engine2 *e)
>         igt_skip_on(IS_VALLEYVIEW(devid) || IS_CHERRYVIEW(devid));
>  
>         kmstest_set_vt_graphics_mode();
> -       igt_display_init(&data.display, gem_fd);
> +       igt_display_require(&data.display, gem_fd);

We do a search for our requirements in the loop below.

>         /**
>          * We will use the display to render event forwarind so need to
> diff --git a/tests/pm_backlight.c b/tests/pm_backlight.c
> index 32808cdf6ca4..054300f6e2e1 100644
> --- a/tests/pm_backlight.c
> +++ b/tests/pm_backlight.c
> @@ -214,7 +214,7 @@ igt_main
>                  * try to enable all.
>                  */
>                 kmstest_set_vt_graphics_mode();
> -               igt_display_init(&display, drm_open_driver(DRIVER_INTEL));
> +               igt_display_require(&display, drm_open_driver(DRIVER_INTEL));

The exact requirement of having a connected backlight is spelled out
below.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require

[Daniel Vetter]

On Thu, Oct 04, 2018 at 04:06:53PM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-10-04 14:21:26)
> > diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
> > index e9325dec9305..b8246e669939 100644
> > --- a/tests/kms_force_connector_basic.c
> > +++ b/tests/kms_force_connector_basic.c
> > @@ -217,7 +217,7 @@ int main(int argc, char **argv)
> >  
> >                 /* attempt to use the display */
> >                 kmstest_set_vt_graphics_mode();
> > -               igt_display_init(&display, drm_fd);
> > +               igt_display_require(&display, drm_fd);
> >                 igt_display_commit(&display);
> >                 igt_display_fini(&display);
> 
> Where is the requirement here? I'd buy that this should be an
> igt_assert() since we did the forcing earlier.
> 
> > diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
> > index 07ffd79c4613..ca0b01c05e5c 100644
> > --- a/tests/kms_getfb.c
> > +++ b/tests/kms_getfb.c
> > @@ -116,7 +116,7 @@ static uint32_t get_any_prop_id(int fd)
> >  {
> >         igt_display_t display;
> >  
> > -       igt_display_init(&display, fd);
> > +       igt_display_require(&display, fd);
> 
> Not required, as we describe the requirement of having the property
> as an output of this function.
> 
> >         for (int i = 0; i < display.n_outputs; i++) {
> >                 igt_output_t *output = &display.outputs[i];
> >                 if (output->props[IGT_CONNECTOR_DPMS] != 0)
> > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> > index b34bc66ce2c4..21292bf3a2fe 100644
> > --- a/tests/perf_pmu.c
> > +++ b/tests/perf_pmu.c
> > @@ -811,7 +811,7 @@ event_wait(int gem_fd, const struct intel_execution_engine2 *e)
> >         igt_skip_on(IS_VALLEYVIEW(devid) || IS_CHERRYVIEW(devid));
> >  
> >         kmstest_set_vt_graphics_mode();
> > -       igt_display_init(&data.display, gem_fd);
> > +       igt_display_require(&data.display, gem_fd);
> 
> We do a search for our requirements in the loop below.
> 
> >         /**
> >          * We will use the display to render event forwarind so need to
> > diff --git a/tests/pm_backlight.c b/tests/pm_backlight.c
> > index 32808cdf6ca4..054300f6e2e1 100644
> > --- a/tests/pm_backlight.c
> > +++ b/tests/pm_backlight.c
> > @@ -214,7 +214,7 @@ igt_main
> >                  * try to enable all.
> >                  */
> >                 kmstest_set_vt_graphics_mode();
> > -               igt_display_init(&display, drm_open_driver(DRIVER_INTEL));
> > +               igt_display_require(&display, drm_open_driver(DRIVER_INTEL));
> 
> The exact requirement of having a connected backlight is spelled out
> below.

Answering to all 4, since they boil down to the same I think:

If I understand this correctly, you want to have only 1 igt_require, at
the very end, and keeping initializing everything? Even if it's pointless
to keep on going, because we know that if there's no output, you can't
possibly have a backlight on an output (as an example)?

My preferred approach is to bail out as soon as we know we can't
reasonably proceed. Since that tends to give us the option of more
abuse-proof apis - if I can remove the igt_display_init, then I think
that's a good goal.
-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

[igt-dev] [PATCH i-g-t] tests/debugfs: use igt_display_require

[Daniel Vetter]

Need to extract into a test subgroup to make sure we only skip the
tests that need display support.

Cc: Antonio Argenziano <antonio.argenziano@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 tests/debugfs_test.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/tests/debugfs_test.c b/tests/debugfs_test.c
index 2e87e4420b15..eb32932ed686 100644
--- a/tests/debugfs_test.c
+++ b/tests/debugfs_test.c
@@ -87,23 +87,14 @@ static void read_and_discard_sysfs_entries(int path_fd, int indent)
 	closedir(dir);
 }
 
-igt_main
+static void kms_tests(int fd, int debugfs)
 {
-	int fd = -1, debugfs;
 	igt_display_t display;
 	struct igt_fb fb[IGT_MAX_PIPES];
 	enum pipe pipe;
 
-	igt_skip_on_simulation();
-
-	igt_fixture {
-		fd = drm_open_driver_master(DRIVER_INTEL);
-		igt_require_gem(fd);
-		debugfs = igt_debugfs_dir(fd);
-
-		kmstest_set_vt_graphics_mode();
-		igt_display_init(&display, fd);
-	}
+	igt_fixture
+		igt_display_require(&display, fd);
 
 	igt_subtest("read_all_entries") {
 		/* try to light all pipes */
@@ -152,6 +143,27 @@ igt_main
 		read_and_discard_sysfs_entries(debugfs, 0);
 	}
 
+	igt_fixture
+		igt_display_fini(&display);
+}
+
+igt_main
+{
+	int fd = -1, debugfs;
+
+	igt_skip_on_simulation();
+
+	igt_fixture {
+		fd = drm_open_driver_master(DRIVER_INTEL);
+		igt_require_gem(fd);
+		debugfs = igt_debugfs_dir(fd);
+
+		kmstest_set_vt_graphics_mode();
+	}
+
+	igt_subtest_group
+		kms_tests(fd, debugfs);
+
 	igt_subtest("emon_crash") {
 		int i;
 		/*
@@ -174,7 +186,6 @@ igt_main
 	}
 
 	igt_fixture {
-		igt_display_fini(&display);
 		close(debugfs);
 		close(fd);
 	}
-- 
2.19.0.rc2

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

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2)
URL   : https://patchwork.freedesktop.org/series/50554/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4931 -> IGTPW_1911 =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1911 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1911, 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/50554/revisions/2/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@debugfs_test@read_all_entries:
      fi-kbl-8809g:       PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

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

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

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

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

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

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#108126 https://bugs.freedesktop.org/show_bug.cgi?id=108126


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

  Missing    (9): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-snb-2520m fi-ctg-p8600 fi-pnv-d510 fi-kbl-7560u 


== Build changes ==

    * IGT: IGT_4667 -> IGTPW_1911

  CI_DRM_4931: 826702bf60ae2b37841c051ed769b44af194fbb1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1911: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1911/
  IGT_4667: 596f48dcd59fd2f8c16671514f3e69d4a2891374 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1911/issues.html
_______________________________________________
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 [i-g-t,1/4] tests/debugfs: use igt_display_require

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/4] tests/debugfs: use igt_display_require
URL   : https://patchwork.freedesktop.org/series/50554/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4667_full -> IGTPW_1908_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1908_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1908_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/50554/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

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

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-snb:          PASS -> FAIL (fdo#106641)

    igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
      shard-glk:          PASS -> FAIL (fdo#108145)

    igt@kms_cursor_crc@cursor-256x256-sliding:
      shard-glk:          PASS -> FAIL (fdo#103232) +2

    igt@kms_cursor_crc@cursor-256x85-sliding:
      shard-apl:          PASS -> FAIL (fdo#103232) +1

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-apl:          PASS -> FAIL (fdo#103232, fdo#103191)

    igt@kms_cursor_legacy@cursorb-vs-flipb-toggle:
      shard-glk:          PASS -> DMESG-WARN (fdo#105763, fdo#106538) +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
      shard-kbl:          PASS -> FAIL (fdo#103167)
      shard-apl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-mmap-cpu:
      shard-glk:          PASS -> FAIL (fdo#103167) +11

    igt@kms_plane@pixel-format-pipe-a-planes:
      shard-snb:          PASS -> FAIL (fdo#103166, fdo#107749)

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-glk:          PASS -> FAIL (fdo#103166) +4
      shard-kbl:          PASS -> FAIL (fdo#103166)

    igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
      shard-apl:          PASS -> FAIL (fdo#103166)

    igt@kms_rotation_crc@exhaust-fences:
      shard-snb:          SKIP -> INCOMPLETE (fdo#105411)

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

    
    ==== Possible fixes ====

    igt@drv_suspend@fence-restore-untiled:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@gem_exec_big:
      shard-hsw:          TIMEOUT (fdo#107937) -> PASS

    igt@gem_mmap_gtt@medium-copy:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    igt@kms_ccs@pipe-a-bad-pixel-format:
      shard-apl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS +6

    igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
      shard-kbl:          FAIL (fdo#108145) -> PASS

    igt@kms_cursor_crc@cursor-128x128-dpms:
      shard-kbl:          FAIL (fdo#103232) -> PASS

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-apl:          FAIL (fdo#103232, fdo#103191) -> PASS
      shard-kbl:          FAIL (fdo#103232, fdo#103191) -> PASS

    igt@kms_cursor_crc@cursor-128x42-onscreen:
      shard-glk:          FAIL (fdo#103232) -> PASS +4

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-apl:          DMESG-FAIL (fdo#103558, fdo#105602) -> PASS

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +7

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

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363, fdo#102887) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
      shard-apl:          FAIL (fdo#103167) -> PASS +3
      shard-glk:          FAIL (fdo#103167) -> PASS
      shard-kbl:          FAIL (fdo#103167) -> PASS

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-apl:          FAIL (fdo#103166) -> PASS +4

    {igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max}:
      shard-glk:          FAIL (fdo#108145) -> PASS +1

    igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
      shard-glk:          FAIL (fdo#103166) -> PASS +1

    igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
      shard-kbl:          FAIL (fdo#103166) -> PASS +1

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

  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#107749 https://bugs.freedesktop.org/show_bug.cgi?id=107749
  fdo#107937 https://bugs.freedesktop.org/show_bug.cgi?id=107937
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


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

  Missing    (1): shard-skl 


== Build changes ==

    * IGT: IGT_4667 -> IGTPW_1908
    * Linux: CI_DRM_4930 -> CI_DRM_4931

  CI_DRM_4930: bf1bd5e86f267d58ac68c342fcfff70e8ef1fd34 @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4931: 826702bf60ae2b37841c051ed769b44af194fbb1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1908: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1908/
  IGT_4667: 596f48dcd59fd2f8c16671514f3e69d4a2891374 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

[igt-dev] ✗ Fi.CI.IGT: failure for series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2)
URL   : https://patchwork.freedesktop.org/series/50554/
State : failure

== Summary ==

= CI Bug Log - changes from IGT_4667_full -> IGTPW_1911_full =

== Summary - FAILURE ==

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_color@pipe-a-ctm-max:
      shard-kbl:          PASS -> FAIL

    
    ==== Warnings ====

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

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-glk:          PASS -> INCOMPLETE (k.org#198133, fdo#103359, fdo#106886)

    igt@gem_linear_blits@interruptible:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
      shard-glk:          PASS -> FAIL (fdo#108145)

    igt@kms_color@pipe-a-ctm-max:
      shard-apl:          PASS -> FAIL (fdo#108147)

    igt@kms_cursor_crc@cursor-256x256-sliding:
      shard-glk:          PASS -> FAIL (fdo#103232) +2

    igt@kms_cursor_crc@cursor-64x64-sliding:
      shard-apl:          PASS -> FAIL (fdo#103232)
      shard-kbl:          PASS -> FAIL (fdo#103232)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
      shard-apl:          PASS -> FAIL (fdo#103167)

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-apl:          PASS -> FAIL (fdo#103166)

    igt@kms_rmfb@rmfb-ioctl:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +8

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

    
    ==== Possible fixes ====

    igt@drv_suspend@fence-restore-untiled:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@gem_exec_big:
      shard-hsw:          TIMEOUT (fdo#107937) -> PASS

    igt@gem_mmap_gtt@medium-copy:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    igt@kms_ccs@pipe-a-bad-pixel-format:
      shard-apl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS +5

    igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
      shard-kbl:          FAIL (fdo#108145) -> PASS

    igt@kms_cursor_crc@cursor-128x128-dpms:
      shard-kbl:          FAIL (fdo#103232) -> PASS

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-apl:          FAIL (fdo#103191, fdo#103232) -> PASS
      shard-kbl:          FAIL (fdo#103191, fdo#103232) -> PASS

    igt@kms_cursor_crc@cursor-128x42-onscreen:
      shard-glk:          FAIL (fdo#103232) -> PASS +4

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-apl:          DMESG-FAIL (fdo#103558, fdo#105602) -> PASS

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +3

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

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#102887, fdo#105363) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
      shard-apl:          FAIL (fdo#103167) -> PASS +5

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

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-glk:          FAIL (fdo#103167) -> PASS +3

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-glk:          FAIL (fdo#103166) -> PASS +3

    {igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb}:
      shard-glk:          FAIL (fdo#108145) -> PASS +1

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-apl:          FAIL (fdo#103166) -> PASS +4

    igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
      shard-kbl:          FAIL (fdo#103166) -> PASS +1

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

  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107937 https://bugs.freedesktop.org/show_bug.cgi?id=107937
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108147 https://bugs.freedesktop.org/show_bug.cgi?id=108147
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


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

  Missing    (1): shard-skl 


== Build changes ==

    * IGT: IGT_4667 -> IGTPW_1911
    * Linux: CI_DRM_4930 -> CI_DRM_4931

  CI_DRM_4930: bf1bd5e86f267d58ac68c342fcfff70e8ef1fd34 @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4931: 826702bf60ae2b37841c051ed769b44af194fbb1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1911: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1911/
  IGT_4667: 596f48dcd59fd2f8c16671514f3e69d4a2891374 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1911/shards.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 2/4] tests: Use igt_display_require

[Daniel Vetter]

On Thu, Oct 04, 2018 at 09:04:09PM +0200, Daniel Vetter wrote:
> On Thu, Oct 04, 2018 at 04:06:53PM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2018-10-04 14:21:26)
> > > diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
> > > index e9325dec9305..b8246e669939 100644
> > > --- a/tests/kms_force_connector_basic.c
> > > +++ b/tests/kms_force_connector_basic.c
> > > @@ -217,7 +217,7 @@ int main(int argc, char **argv)
> > >  
> > >                 /* attempt to use the display */
> > >                 kmstest_set_vt_graphics_mode();
> > > -               igt_display_init(&display, drm_fd);
> > > +               igt_display_require(&display, drm_fd);
> > >                 igt_display_commit(&display);
> > >                 igt_display_fini(&display);
> > 
> > Where is the requirement here? I'd buy that this should be an
> > igt_assert() since we did the forcing earlier.
> > 
> > > diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
> > > index 07ffd79c4613..ca0b01c05e5c 100644
> > > --- a/tests/kms_getfb.c
> > > +++ b/tests/kms_getfb.c
> > > @@ -116,7 +116,7 @@ static uint32_t get_any_prop_id(int fd)
> > >  {
> > >         igt_display_t display;
> > >  
> > > -       igt_display_init(&display, fd);
> > > +       igt_display_require(&display, fd);
> > 
> > Not required, as we describe the requirement of having the property
> > as an output of this function.
> > 
> > >         for (int i = 0; i < display.n_outputs; i++) {
> > >                 igt_output_t *output = &display.outputs[i];
> > >                 if (output->props[IGT_CONNECTOR_DPMS] != 0)
> > > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> > > index b34bc66ce2c4..21292bf3a2fe 100644
> > > --- a/tests/perf_pmu.c
> > > +++ b/tests/perf_pmu.c
> > > @@ -811,7 +811,7 @@ event_wait(int gem_fd, const struct intel_execution_engine2 *e)
> > >         igt_skip_on(IS_VALLEYVIEW(devid) || IS_CHERRYVIEW(devid));
> > >  
> > >         kmstest_set_vt_graphics_mode();
> > > -       igt_display_init(&data.display, gem_fd);
> > > +       igt_display_require(&data.display, gem_fd);
> > 
> > We do a search for our requirements in the loop below.
> > 
> > >         /**
> > >          * We will use the display to render event forwarind so need to
> > > diff --git a/tests/pm_backlight.c b/tests/pm_backlight.c
> > > index 32808cdf6ca4..054300f6e2e1 100644
> > > --- a/tests/pm_backlight.c
> > > +++ b/tests/pm_backlight.c
> > > @@ -214,7 +214,7 @@ igt_main
> > >                  * try to enable all.
> > >                  */
> > >                 kmstest_set_vt_graphics_mode();
> > > -               igt_display_init(&display, drm_open_driver(DRIVER_INTEL));
> > > +               igt_display_require(&display, drm_open_driver(DRIVER_INTEL));
> > 
> > The exact requirement of having a connected backlight is spelled out
> > below.
> 
> Answering to all 4, since they boil down to the same I think:
> 
> If I understand this correctly, you want to have only 1 igt_require, at
> the very end, and keeping initializing everything? Even if it's pointless
> to keep on going, because we know that if there's no output, you can't
> possibly have a backlight on an output (as an example)?
> 
> My preferred approach is to bail out as soon as we know we can't
> reasonably proceed. Since that tends to give us the option of more
> abuse-proof apis - if I can remove the igt_display_init, then I think
> that's a good goal.

Another upside of checking early (even if you might not catch the full
conditions all in one go): We waste less time on skipped tests with
pointless setup. E.g. on the gem side we also don't open the drm fd,
initialize the batch manager, set up the batch and everything only to then
realize that we're not running on top of i915.ko. I think this here is
similar.

If you want, I could move them a lot further up in the code so they catch
stuff earlier.
-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] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Daniel Vetter]

On Thu, Oct 04, 2018 at 03:21:28PM +0200, Daniel Vetter wrote:
> With the high-level helpers requiring outputs there's not point
> in silently ignoring issues anymore. Complain about that if it
> ever happens.
> 
> Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

I guess my motivation with fumbling around with the igt_display_* api
wasn't entirely clear: It's this patch here, respectively Chris' patch
which added the silent short circuit.

Imo that's very brittle api, asking for trouble, and me not recognizing
right away what's happening in the debugfs is kinda proving the point.
Silently throwing a request away from the testcase is at least very
surprising. And inconsistent with both more explicit igt_require/assert
checks in drivers, and (the style I favour personally, but really not the
issue here) putting igt_require/assert into the helper library.

Now my proposal to get us back some robustness seems not met with huge
enthusiams, so what's it going to be? I'd be ok with sprinkling more
explicit checks over tests (and probably more explicit control flow), plus
proper documentation for igt_display_require, too. But this here doesn't
look like a good idea long-term, and that's why I'm not happy with how
that bugfixing was rushed in. Fixing disable_display=1 isn't _that_ high
of a priority that we can't take the time to get the igt library api
somewhat right.

Cheers, Daniel

> ---
>  lib/igt_kms.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 454ab7481cde..867eaa7a971c 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -3269,7 +3269,7 @@ static int do_display_commit(igt_display_t *display,
>  	enum pipe pipe;
>  	LOG_INDENT(display, "commit");
>  
> -	if (!display->n_pipes || !display->n_outputs)
> +	if (igt_warn_on(!display->n_pipes || !display->n_outputs))
>  		return 0; /* nothing to do */
>  
>  	igt_display_refresh(display);
> @@ -3322,7 +3322,7 @@ int igt_display_try_commit_atomic(igt_display_t *display, uint32_t flags, void *
>  {
>  	int ret;
>  
> -	if (!display->n_pipes || !display->n_outputs)
> +	if (igt_warn_on(!display->n_pipes || !display->n_outputs))
>  		return 0; /* nothing to do */
>  
>  	LOG_INDENT(display, "commit");
> -- 
> 2.19.0.rc2
> 

-- 
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] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Chris Wilson]

Quoting Daniel Vetter (2018-10-05 16:07:38)
> On Thu, Oct 04, 2018 at 03:21:28PM +0200, Daniel Vetter wrote:
> > With the high-level helpers requiring outputs there's not point
> > in silently ignoring issues anymore. Complain about that if it
> > ever happens.
> > 
> > Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> I guess my motivation with fumbling around with the igt_display_* api
> wasn't entirely clear: It's this patch here, respectively Chris' patch
> which added the silent short circuit.
> 
> Imo that's very brittle api, asking for trouble, and me not recognizing
> right away what's happening in the debugfs is kinda proving the point.
> Silently throwing a request away from the testcase is at least very
> surprising. And inconsistent with both more explicit igt_require/assert
> checks in drivers, and (the style I favour personally, but really not the
> issue here) putting igt_require/assert into the helper library.

I'd argue the opposite. The test case requested us to configure 0
outputs across 0 pipes, and we are obeying it. Not second guessing what
the test case intended.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Daniel Vetter]

On Fri, Oct 05, 2018 at 04:40:14PM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-10-05 16:07:38)
> > On Thu, Oct 04, 2018 at 03:21:28PM +0200, Daniel Vetter wrote:
> > > With the high-level helpers requiring outputs there's not point
> > > in silently ignoring issues anymore. Complain about that if it
> > > ever happens.
> > > 
> > > Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > 
> > I guess my motivation with fumbling around with the igt_display_* api
> > wasn't entirely clear: It's this patch here, respectively Chris' patch
> > which added the silent short circuit.
> > 
> > Imo that's very brittle api, asking for trouble, and me not recognizing
> > right away what's happening in the debugfs is kinda proving the point.
> > Silently throwing a request away from the testcase is at least very
> > surprising. And inconsistent with both more explicit igt_require/assert
> > checks in drivers, and (the style I favour personally, but really not the
> > issue here) putting igt_require/assert into the helper library.
> 
> I'd argue the opposite. The test case requested us to configure 0
> outputs across 0 pipes, and we are obeying it. Not second guessing what
> the test case intended.

We have 8 cases of igt_display_init now, and slightly shy of 70 for
igt_display_require. There's almost 10x testcases you can silently break
for the handful that don't break with this.

Furthermore, there seems exactly 1 testcase which actually asked for this,
and I'm argueing that that testcase is much cleaner if we'd split the
no-display test from the all-displays-off test.

So we have 1 vs. ~70 testcases. If your api makes the exceedingly common
case brittle, then I really don't see why that's good api design.

And there's tons other options, without any brittleness. E.g. we could add
this igt_warn_on here, but add explicit control flow to the debugfs test.
That:
- Keeps all the igt require checks exactly where you want them.
- Gives us a non-brittle api that doesn't hide surprises.

There's probably metric piles of other approaches, with varying shades of
paint applied. I don't see any reason why the api should silently eat
errors - and errors does it eat, since that's why you pushed the patch.

All I'm arguing for is to make that more explicit.
-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] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Arkadiusz Hiler]

On Fri, Oct 05, 2018 at 08:48:40PM +0200, Daniel Vetter wrote:
> On Fri, Oct 05, 2018 at 04:40:14PM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2018-10-05 16:07:38)
> > > On Thu, Oct 04, 2018 at 03:21:28PM +0200, Daniel Vetter wrote:
> > > > With the high-level helpers requiring outputs there's not point
> > > > in silently ignoring issues anymore. Complain about that if it
> > > > ever happens.
> > > > 
> > > > Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > 
> > > I guess my motivation with fumbling around with the igt_display_* api
> > > wasn't entirely clear: It's this patch here, respectively Chris' patch
> > > which added the silent short circuit.
> > > 
> > > Imo that's very brittle api, asking for trouble, and me not recognizing
> > > right away what's happening in the debugfs is kinda proving the point.
> > > Silently throwing a request away from the testcase is at least very
> > > surprising. And inconsistent with both more explicit igt_require/assert
> > > checks in drivers, and (the style I favour personally, but really not the
> > > issue here) putting igt_require/assert into the helper library.

So basically what you want to do here:

1. make sure that tests are using the correct one:
 * igt_display_init()    - display disabled safe/aware tests
 * igt_display_require() - if you need a functioning display

2. Make sure that we don't hide/silence errors coming out of kms
   unnecessarily.

Sounds sensible to me.

> > I'd argue the opposite. The test case requested us to configure 0
> > outputs across 0 pipes, and we are obeying it. Not second guessing what
> > the test case intended.

| if (!display->n_pipes || !display->n_outputs)
|	return 0; /* nothing to do */

Then if the test does requests that, why do we return 0 here, instead of
doing the IOCTL, especially that the doc states:
"This function should be used to commit changes that are expected to fail"?

How is that less second guessing? TBH I would not even warn here.

> We have 8 cases of igt_display_init now, and slightly shy of 70 for
> igt_display_require. There's almost 10x testcases you can silently break
> for the handful that don't break with this.
> 
> Furthermore, there seems exactly 1 testcase which actually asked for this,
> and I'm argueing that that testcase is much cleaner if we'd split the
> no-display test from the all-displays-off test.
> 
> So we have 1 vs. ~70 testcases. If your api makes the exceedingly common
> case brittle, then I really don't see why that's good api design.
> 
> And there's tons other options, without any brittleness. E.g. we could add
> this igt_warn_on here, but add explicit control flow to the debugfs test.
> That:
> - Keeps all the igt require checks exactly where you want them.
> - Gives us a non-brittle api that doesn't hide surprises.
> 
> There's probably metric piles of other approaches, with varying shades of
> paint applied. I don't see any reason why the api should silently eat
> errors - and errors does it eat, since that's why you pushed the patch.
> 
> All I'm arguing for is to make that more explicit.

Again, sounds like a sensible thing to do and being explicit helps with
making the codebase more digestible. This is less of a two-liner hax and
more of a proper rethinking. I agree that we are at the point where it
has to be done, so we won't let our APIs rot any further.

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

Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Daniel Vetter]

On Fri, Oct 12, 2018 at 2:02 PM Arkadiusz Hiler
<arkadiusz.hiler@intel.com> wrote:
>
> On Fri, Oct 05, 2018 at 08:48:40PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 05, 2018 at 04:40:14PM +0100, Chris Wilson wrote:
> > > Quoting Daniel Vetter (2018-10-05 16:07:38)
> > > > On Thu, Oct 04, 2018 at 03:21:28PM +0200, Daniel Vetter wrote:
> > > > > With the high-level helpers requiring outputs there's not point
> > > > > in silently ignoring issues anymore. Complain about that if it
> > > > > ever happens.
> > > > >
> > > > > Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > >
> > > > I guess my motivation with fumbling around with the igt_display_* api
> > > > wasn't entirely clear: It's this patch here, respectively Chris' patch
> > > > which added the silent short circuit.
> > > >
> > > > Imo that's very brittle api, asking for trouble, and me not recognizing
> > > > right away what's happening in the debugfs is kinda proving the point.
> > > > Silently throwing a request away from the testcase is at least very
> > > > surprising. And inconsistent with both more explicit igt_require/assert
> > > > checks in drivers, and (the style I favour personally, but really not the
> > > > issue here) putting igt_require/assert into the helper library.
>
> So basically what you want to do here:
>
> 1. make sure that tests are using the correct one:
>  * igt_display_init()    - display disabled safe/aware tests
>  * igt_display_require() - if you need a functioning display
>
> 2. Make sure that we don't hide/silence errors coming out of kms
>    unnecessarily.
>
> Sounds sensible to me.
>
> > > I'd argue the opposite. The test case requested us to configure 0
> > > outputs across 0 pipes, and we are obeying it. Not second guessing what
> > > the test case intended.
>
> | if (!display->n_pipes || !display->n_outputs)
> |       return 0; /* nothing to do */
>
> Then if the test does requests that, why do we return 0 here, instead of
> doing the IOCTL, especially that the doc states:
> "This function should be used to commit changes that are expected to fail"?
>
> How is that less second guessing? TBH I would not even warn here.

Afaiui this fails for the tests (like the debugfs one) that want to
keep running even if no display is present. That's at least my
understanding. Just committing is what we originally had, and would
amount to a revert of

commit 212b71372bfbb73663d872df31118d6b396ada4f
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Sep 14 21:03:38 2018 +0100

    lib/kms: Skip no-op display updates

which would break some tests with i915.disable_display=1 I think (but
the commit message isn't terribly clear on what exactly). I'm also ok
with reverting that one instead of the igt_warn_on patch here, both
are imo fine options.

> > We have 8 cases of igt_display_init now, and slightly shy of 70 for
> > igt_display_require. There's almost 10x testcases you can silently break
> > for the handful that don't break with this.
> >
> > Furthermore, there seems exactly 1 testcase which actually asked for this,
> > and I'm argueing that that testcase is much cleaner if we'd split the
> > no-display test from the all-displays-off test.
> >
> > So we have 1 vs. ~70 testcases. If your api makes the exceedingly common
> > case brittle, then I really don't see why that's good api design.
> >
> > And there's tons other options, without any brittleness. E.g. we could add
> > this igt_warn_on here, but add explicit control flow to the debugfs test.
> > That:
> > - Keeps all the igt require checks exactly where you want them.
> > - Gives us a non-brittle api that doesn't hide surprises.
> >
> > There's probably metric piles of other approaches, with varying shades of
> > paint applied. I don't see any reason why the api should silently eat
> > errors - and errors does it eat, since that's why you pushed the patch.
> >
> > All I'm arguing for is to make that more explicit.
>
> Again, sounds like a sensible thing to do and being explicit helps with
> making the codebase more digestible. This is less of a two-liner hax and
> more of a proper rethinking. I agree that we are at the point where it
> has to be done, so we won't let our APIs rot any further.

So the question is, what do do. We have 2 options for this function
here (revert or igt_warn), and we have a bunch of options for how to
make things more explicit in the tests (either my patch series here,
with v2 of patch 1, or maybe more explicit control flow with a
__must_check annotation for igt_display_init). Imo all combinations
would give us a solid api in the end, I just prefer not to implement
them all first, but decide first :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Arkadiusz Hiler]

On Fri, Oct 12, 2018 at 02:32:44PM +0200, Daniel Vetter wrote:
> On Fri, Oct 12, 2018 at 2:02 PM Arkadiusz Hiler
> <arkadiusz.hiler@intel.com> wrote:
> >
> > On Fri, Oct 05, 2018 at 08:48:40PM +0200, Daniel Vetter wrote:
> > > On Fri, Oct 05, 2018 at 04:40:14PM +0100, Chris Wilson wrote:
> > > > Quoting Daniel Vetter (2018-10-05 16:07:38)
> > > > > On Thu, Oct 04, 2018 at 03:21:28PM +0200, Daniel Vetter wrote:
> > > > > > With the high-level helpers requiring outputs there's not point
> > > > > > in silently ignoring issues anymore. Complain about that if it
> > > > > > ever happens.
> > > > > >
> > > > > > Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > >
> > > > > I guess my motivation with fumbling around with the igt_display_* api
> > > > > wasn't entirely clear: It's this patch here, respectively Chris' patch
> > > > > which added the silent short circuit.
> > > > >
> > > > > Imo that's very brittle api, asking for trouble, and me not recognizing
> > > > > right away what's happening in the debugfs is kinda proving the point.
> > > > > Silently throwing a request away from the testcase is at least very
> > > > > surprising. And inconsistent with both more explicit igt_require/assert
> > > > > checks in drivers, and (the style I favour personally, but really not the
> > > > > issue here) putting igt_require/assert into the helper library.
> >
> > So basically what you want to do here:
> >
> > 1. make sure that tests are using the correct one:
> >  * igt_display_init()    - display disabled safe/aware tests
> >  * igt_display_require() - if you need a functioning display
> >
> > 2. Make sure that we don't hide/silence errors coming out of kms
> >    unnecessarily.
> >
> > Sounds sensible to me.
> >
> > > > I'd argue the opposite. The test case requested us to configure 0
> > > > outputs across 0 pipes, and we are obeying it. Not second guessing what
> > > > the test case intended.
> >
> > | if (!display->n_pipes || !display->n_outputs)
> > |       return 0; /* nothing to do */
> >
> > Then if the test does requests that, why do we return 0 here, instead of
> > doing the IOCTL, especially that the doc states:
> > "This function should be used to commit changes that are expected to fail"?
> >
> > How is that less second guessing? TBH I would not even warn here.
> 
> Afaiui this fails for the tests (like the debugfs one) that want to
> keep running even if no display is present. That's at least my
> understanding. Just committing is what we originally had, and would
> amount to a revert of
> 
> commit 212b71372bfbb73663d872df31118d6b396ada4f
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Sep 14 21:03:38 2018 +0100
> 
>     lib/kms: Skip no-op display updates
> 
> which would break some tests with i915.disable_display=1 I think (but
> the commit message isn't terribly clear on what exactly). I'm also ok
> with reverting that one instead of the igt_warn_on patch here, both
> are imo fine options.

I wonder what breaks in there.

If we have some hard asserts inside that are failing us with display
disabled, maybe this is the thing that needs fixing.

I think I have a NUC I can use for testing somewhere around.
Let me find it.

> > > We have 8 cases of igt_display_init now, and slightly shy of 70 for
> > > igt_display_require. There's almost 10x testcases you can silently break
> > > for the handful that don't break with this.
> > >
> > > Furthermore, there seems exactly 1 testcase which actually asked for this,
> > > and I'm argueing that that testcase is much cleaner if we'd split the
> > > no-display test from the all-displays-off test.
> > >
> > > So we have 1 vs. ~70 testcases. If your api makes the exceedingly common
> > > case brittle, then I really don't see why that's good api design.
> > >
> > > And there's tons other options, without any brittleness. E.g. we could add
> > > this igt_warn_on here, but add explicit control flow to the debugfs test.
> > > That:
> > > - Keeps all the igt require checks exactly where you want them.
> > > - Gives us a non-brittle api that doesn't hide surprises.
> > >
> > > There's probably metric piles of other approaches, with varying shades of
> > > paint applied. I don't see any reason why the api should silently eat
> > > errors - and errors does it eat, since that's why you pushed the patch.
> > >
> > > All I'm arguing for is to make that more explicit.
> >
> > Again, sounds like a sensible thing to do and being explicit helps with
> > making the codebase more digestible. This is less of a two-liner hax and
> > more of a proper rethinking. I agree that we are at the point where it
> > has to be done, so we won't let our APIs rot any further.
> 
> So the question is, what do do. We have 2 options for this function
> here (revert or igt_warn), and we have a bunch of options for how to
> make things more explicit in the tests (either my patch series here,
> with v2 of patch 1, or maybe more explicit control flow with a
> __must_check annotation for igt_display_init). Imo all combinations
> would give us a solid api in the end, I just prefer not to implement
> them all first, but decide first :-)
> -Daniel

IMO:

1. revert and either:
 a) fixing try_commit so that it does not assert
 b) moving the if n_pipes == 0 || n_outputs == 0 logic to debugfs tests
    (or a block igt_with_display_on(display) { } ?)
2. respin of your patch series

But let's see what others have to say.

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

Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Arkadiusz Hiler]

On Fri, Oct 12, 2018 at 03:57:59PM +0300, Arkadiusz Hiler wrote:
> On Fri, Oct 12, 2018 at 02:32:44PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 12, 2018 at 2:02 PM Arkadiusz Hiler
> > So the question is, what do do. We have 2 options for this function
> > here (revert or igt_warn), and we have a bunch of options for how to
> > make things more explicit in the tests (either my patch series here,
> > with v2 of patch 1, or maybe more explicit control flow with a
> > __must_check annotation for igt_display_init). Imo all combinations
> > would give us a solid api in the end, I just prefer not to implement
> > them all first, but decide first :-)
> > -Daniel
> 
> IMO:
> 
> 1. revert and either:
>  a) fixing try_commit so that it does not assert
>  b) moving the if n_pipes == 0 || n_outputs == 0 logic to debugfs tests
>     (or a block igt_with_display_on(display) { } ?)
> 2. respin of your patch series
> 
> But let's see what others have to say.
> 
> -Arek

I reverted the patch locally to see whether we stumble upon any of the
internal asserts while running debugfs tests with display disabled.
Tested with today's drm-tip and igt.

To my surprise - everything went just fine:

--------------------------------------------------------------------------------
$ dmesg | tail -n 9
[  166.643232] [drm] Display disabled (module parameter)
[  166.643407] [drm] Replacing VGA console driver
[  166.655672] i915 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
[  166.668362] [drm] Initialized i915 1.6.0 20180921 for 0000:00:02.0 on minor 0
[  166.670518] [drm] DRM_I915_DEBUG enabled
[  166.670520] [drm] DRM_I915_DEBUG_GEM enabled
[  166.670522] [drm] DRM_I915_DEBUG_RUNTIME_PM enabled
[  188.818475] random: crng init done
[  188.818484] random: 7 urandom warning(s) missed due to ratelimiting

$ git show
commit ff89f7bce0111404ae480b7e901b827dedecf9fc (HEAD -> master)
Author: testrunner <testrunner@dev-bdw>
Date:   Wed Oct 17 11:42:35 2018 +0300

    Revert "lib/kms: Skip no-op display updates"

    This reverts commit 212b71372bfbb73663d872df31118d6b396ada4f.

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 6e499f48..e4165065 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -3295,9 +3295,6 @@ static int do_display_commit(igt_display_t *display,
        enum pipe pipe;
        LOG_INDENT(display, "commit");

-       if (!display->n_pipes || !display->n_outputs)
-               return 0; /* nothing to do */
-
        igt_display_refresh(display);

        if (s == COMMIT_ATOMIC) {
@@ -3348,9 +3345,6 @@ int igt_display_try_commit_atomic(igt_display_t *display, uint32_t flags, void *
 {
        int ret;

-       if (!display->n_pipes || !display->n_outputs)
-               return 0; /* nothing to do */
-
        LOG_INDENT(display, "commit");

        igt_display_refresh(display);

$ git diff
diff --git a/tests/debugfs_test.c b/tests/debugfs_test.c
index 2e87e442..01f3422f 100644
--- a/tests/debugfs_test.c
+++ b/tests/debugfs_test.c
@@ -131,7 +131,8 @@ igt_main
                        }
                }

-               igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+               int ret = igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+               printf("igt_display_commit2: %d\n", ret);

                read_and_discard_sysfs_entries(debugfs, 0);
        }
@@ -147,7 +148,8 @@ igt_main
                        for_each_plane_on_pipe(&display, pipe, plane)
                                igt_plane_set_fb(plane, NULL);

-               igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+               int ret = igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+               printf("igt_display_commit2: %d\n", ret);

                read_and_discard_sysfs_entries(debugfs, 0);
        }

$ ninja
[1/342] Generating version.h with a custom command.

$ sudo ./tests/debugfs_test
IGT-Version: 1.23-gff89f7bc (x86_64) (Linux: 4.19.0-rc8-CI-CI_DRM_4994+ x86_64)
Starting subtest: read_all_entries
igt_display_commit2: 0
Subtest read_all_entries: SUCCESS (0,017s)
Starting subtest: read_all_entries_display_off
igt_display_commit2: 0
Subtest read_all_entries_display_off: SUCCESS (0,000s)
Starting subtest: emon_crash
Test requirement not met in function __real_main90, file ../tests/debugfs_test.c:168:
Test requirement: !(!buf && !i)
i915_emon_status could not be read
Last errno: 9, Bad file descriptor
Subtest emon_crash: SKIP (0,000s)
--------------------------------------------------------------------------------

@Chris: What am I missing?

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