* [igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require
2018-10-04 13:21 [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require Daniel Vetter
@ 2018-10-04 13:21 ` Daniel Vetter
2018-10-04 15:06 ` Chris Wilson
2018-10-04 13:21 ` [igt-dev] [PATCH i-g-t 3/4] lib/kms: Drop igt_display_init Daniel Vetter
` (7 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2018-10-04 13:21 UTC (permalink / raw)
To: IGT development
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require
2018-10-04 13:21 ` [igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require Daniel Vetter
@ 2018-10-04 15:06 ` Chris Wilson
2018-10-04 19:04 ` Daniel Vetter
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2018-10-04 15:06 UTC (permalink / raw)
To: Daniel Vetter, IGT development
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require
2018-10-04 15:06 ` Chris Wilson
@ 2018-10-04 19:04 ` Daniel Vetter
2018-10-05 8:30 ` Daniel Vetter
0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2018-10-04 19:04 UTC (permalink / raw)
To: Chris Wilson; +Cc: IGT development
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require
2018-10-04 19:04 ` Daniel Vetter
@ 2018-10-05 8:30 ` Daniel Vetter
0 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2018-10-05 8:30 UTC (permalink / raw)
To: Chris Wilson; +Cc: IGT development
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [igt-dev] [PATCH i-g-t 3/4] lib/kms: Drop igt_display_init
2018-10-04 13:21 [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require Daniel Vetter
2018-10-04 13:21 ` [igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require Daniel Vetter
@ 2018-10-04 13:21 ` Daniel Vetter
2018-10-04 13:21 ` [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs Daniel Vetter
` (6 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2018-10-04 13:21 UTC (permalink / raw)
To: IGT development; +Cc: 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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs
2018-10-04 13:21 [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require Daniel Vetter
2018-10-04 13:21 ` [igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require Daniel Vetter
2018-10-04 13:21 ` [igt-dev] [PATCH i-g-t 3/4] lib/kms: Drop igt_display_init Daniel Vetter
@ 2018-10-04 13:21 ` Daniel Vetter
2018-10-05 15:07 ` Daniel Vetter
2018-10-04 14:47 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/4] tests/debugfs: use igt_display_require Patchwork
` (5 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2018-10-04 13:21 UTC (permalink / raw)
To: IGT development; +Cc: 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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs
2018-10-04 13:21 ` [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs Daniel Vetter
@ 2018-10-05 15:07 ` Daniel Vetter
2018-10-05 15:40 ` Chris Wilson
0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2018-10-05 15:07 UTC (permalink / raw)
To: IGT development; +Cc: 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs
2018-10-05 15:07 ` Daniel Vetter
@ 2018-10-05 15:40 ` Chris Wilson
2018-10-05 18:48 ` Daniel Vetter
0 siblings, 1 reply; 20+ messages in thread
From: Chris Wilson @ 2018-10-05 15:40 UTC (permalink / raw)
To: Daniel Vetter, IGT development; +Cc: Daniel Vetter
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs
2018-10-05 15:40 ` Chris Wilson
@ 2018-10-05 18:48 ` Daniel Vetter
2018-10-12 12:02 ` Arkadiusz Hiler
0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2018-10-05 18:48 UTC (permalink / raw)
To: Chris Wilson; +Cc: IGT development, Daniel Vetter, 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs
2018-10-05 18:48 ` Daniel Vetter
@ 2018-10-12 12:02 ` Arkadiusz Hiler
2018-10-12 12:32 ` Daniel Vetter
0 siblings, 1 reply; 20+ messages in thread
From: Arkadiusz Hiler @ 2018-10-12 12:02 UTC (permalink / raw)
To: Daniel Vetter; +Cc: IGT development, Daniel Vetter, Petri Latvala
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs
2018-10-12 12:02 ` Arkadiusz Hiler
@ 2018-10-12 12:32 ` Daniel Vetter
2018-10-12 12:57 ` Arkadiusz Hiler
0 siblings, 1 reply; 20+ messages in thread
From: Daniel Vetter @ 2018-10-12 12:32 UTC (permalink / raw)
To: Arkadiusz Hiler; +Cc: IGT development, Daniel Vetter, Petri Latvala
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs
2018-10-12 12:32 ` Daniel Vetter
@ 2018-10-12 12:57 ` Arkadiusz Hiler
2018-10-17 9:57 ` Arkadiusz Hiler
0 siblings, 1 reply; 20+ messages in thread
From: Arkadiusz Hiler @ 2018-10-12 12:57 UTC (permalink / raw)
To: Daniel Vetter; +Cc: IGT development, Daniel Vetter, Petri Latvala
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs
2018-10-12 12:57 ` Arkadiusz Hiler
@ 2018-10-17 9:57 ` Arkadiusz Hiler
0 siblings, 0 replies; 20+ messages in thread
From: Arkadiusz Hiler @ 2018-10-17 9:57 UTC (permalink / raw)
To: Daniel Vetter; +Cc: IGT development, Daniel Vetter, Petri Latvala
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/4] tests/debugfs: use igt_display_require
2018-10-04 13:21 [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require Daniel Vetter
` (2 preceding siblings ...)
2018-10-04 13:21 ` [igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs Daniel Vetter
@ 2018-10-04 14:47 ` Patchwork
2018-10-04 15:03 ` [igt-dev] [PATCH i-g-t 1/4] " Chris Wilson
` (4 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-10-04 14:47 UTC (permalink / raw)
To: Daniel Vetter; +Cc: igt-dev
== 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require
2018-10-04 13:21 [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require Daniel Vetter
` (3 preceding siblings ...)
2018-10-04 14:47 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/4] tests/debugfs: use igt_display_require Patchwork
@ 2018-10-04 15:03 ` Chris Wilson
2018-10-04 19:07 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
` (3 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Chris Wilson @ 2018-10-04 15:03 UTC (permalink / raw)
To: Daniel Vetter, IGT development
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [igt-dev] [PATCH i-g-t] tests/debugfs: use igt_display_require
2018-10-04 13:21 [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require Daniel Vetter
` (4 preceding siblings ...)
2018-10-04 15:03 ` [igt-dev] [PATCH i-g-t 1/4] " Chris Wilson
@ 2018-10-04 19:07 ` Daniel Vetter
2018-10-04 19:55 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2) Patchwork
` (2 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Daniel Vetter @ 2018-10-04 19:07 UTC (permalink / raw)
To: IGT development
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2)
2018-10-04 13:21 [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require Daniel Vetter
` (5 preceding siblings ...)
2018-10-04 19:07 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
@ 2018-10-04 19:55 ` Patchwork
2018-10-04 23:43 ` [igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/4] tests/debugfs: use igt_display_require Patchwork
2018-10-05 3:09 ` [igt-dev] ✗ Fi.CI.IGT: failure for series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2) Patchwork
8 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-10-04 19:55 UTC (permalink / raw)
To: Daniel Vetter; +Cc: igt-dev
== 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/4] tests/debugfs: use igt_display_require
2018-10-04 13:21 [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require Daniel Vetter
` (6 preceding siblings ...)
2018-10-04 19:55 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2) Patchwork
@ 2018-10-04 23:43 ` Patchwork
2018-10-05 3:09 ` [igt-dev] ✗ Fi.CI.IGT: failure for series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2) Patchwork
8 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-10-04 23:43 UTC (permalink / raw)
To: Daniel Vetter; +Cc: igt-dev
== 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [igt-dev] ✗ Fi.CI.IGT: failure for series starting with [i-g-t] tests/debugfs: use igt_display_require (rev2)
2018-10-04 13:21 [igt-dev] [PATCH i-g-t 1/4] tests/debugfs: use igt_display_require Daniel Vetter
` (7 preceding siblings ...)
2018-10-04 23:43 ` [igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/4] tests/debugfs: use igt_display_require Patchwork
@ 2018-10-05 3:09 ` Patchwork
8 siblings, 0 replies; 20+ messages in thread
From: Patchwork @ 2018-10-05 3:09 UTC (permalink / raw)
To: Daniel Vetter; +Cc: igt-dev
== 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
^ permalink raw reply [flat|nested] 20+ messages in thread