[igt-dev] [PATCH i-g-t v4 2/2] tests: Check and skip unhandled tests that needs display
Souza, Jose
jose.souza at intel.com
Thu Sep 13 23:15:47 UTC 2018
On Thu, 2018-09-13 at 22:02 +0100, Chris Wilson wrote:
> Quoting José Roberto de Souza (2018-09-13 21:39:35)
> > 'lib: Require drmModeResources()' take care of all tests that calls
> > igt_display_init(), here handling other tests that needs display
> > and
> > do not call it.
> >
> > v2:
> > - Not skipping all tests in debugfs_test, perf_pmu and perf_pmu,
> > now skiping only the required subtests
> >
> > v3:
> > - Renamed igt_require_display to igt_display_required, to keep
> > naming
> > consistent
> > - Added igt_display_available()
> > - Checking if display is available by quering a modeset capability
> > instead of drmModeGetResources()
> > - Not skiping read_all_entries tests when display is disabled
> >
> > v4:
> > - Using 'lib: Require drmModeResources()', so all tests already
> > handled by that patch had the igt_display_require() call removed.
> >
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> > lib/igt_kms.c | 30 +++++++++++++-
> > lib/igt_kms.h | 2 +
> > tests/debugfs_test.c | 15 +++++--
> > tests/kms_3d.c | 1 +
> > tests/kms_addfb_basic.c | 4 +-
> > tests/kms_draw_crc.c | 1 +
> > tests/kms_fbcon_fbt.c | 1 +
> > tests/kms_force_connector_basic.c | 1 +
> > tests/kms_getfb.c | 4 +-
> > tests/kms_hdmi_inject.c | 1 +
> > tests/kms_setmode.c | 1 +
> > tests/kms_sysfs_edid_timing.c | 5 +++
> > tests/kms_tv_load_detect.c | 1 +
> > tests/perf_pmu.c | 16 +++----
> > tests/pm_lpsp.c | 1 +
> > tests/pm_rpm.c | 69 ++++++++++++++++++++++++---
> > ----
> > tests/prime_vgem.c | 2 +
> > tests/testdisplay.c | 1 +
> > 18 files changed, 129 insertions(+), 27 deletions(-)
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index c20cf1eb..2bc92c81 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -2018,6 +2018,34 @@ int igt_display_get_n_pipes(igt_display_t
> > *display)
> > return display->n_pipes;
> > }
> >
> > +/**
> > + * igt_display_require:
> > + * @drm_fd: a drm file descriptor
> > + *
> > + * Checks if driver supports modeset and have display enabled.
> > + */
> > +void igt_display_require(int fd)
> > +{
> > + bool available = igt_display_available(fd);
> > + igt_require_f(available, "drm driver do not support
> > modeset\n");
> > +}
> > +
> > +/**
> > + * igt_display_available:
> > + * @drm_fd: a drm file descriptor
> > + *
> > + * Return true if driver supports modeset and have display
> > enabled.
> > + */
> > +bool igt_display_available(int fd)
> > +{
> > + uint64_t cap;
> > +
> > + /* Check if driver support modeset by checking one of the
> > modeset
> > + * specific capabilities, in case of error it is not
> > supported.
> > + */
> > + return !drmGetCap(fd, DRM_CAP_DUMB_BUFFER, &cap);
> > +}
> > +
> > /**
> > * igt_display_require_output:
> > * @display: A pointer to an #igt_display_t structure
> > @@ -3895,7 +3923,7 @@ void igt_enable_connectors(void)
> > drm_fd = drm_open_driver(DRIVER_ANY);
> >
> > res = drmModeGetResources(drm_fd);
> > - igt_assert(res != NULL);
> > + igt_require(res != NULL);
> >
> > for (int i = 0; i < res->count_connectors; i++) {
> > drmModeConnector *c;
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 3862efa2..feafeda9 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -388,6 +388,8 @@ void igt_display_commit_atomic(igt_display_t
> > *display, uint32_t flags, void *use
> > int igt_display_try_commit2(igt_display_t *display, enum
> > igt_commit_style s);
> > int igt_display_drop_events(igt_display_t *display);
> > int igt_display_get_n_pipes(igt_display_t *display);
> > +void igt_display_require(int fd);
> > +bool igt_display_available(int fd);
> > void igt_display_require_output(igt_display_t *display);
> > void igt_display_require_output_on_pipe(igt_display_t *display,
> > enum pipe pipe);
> >
> > diff --git a/tests/debugfs_test.c b/tests/debugfs_test.c
> > index 2e87e442..79243b1a 100644
> > --- a/tests/debugfs_test.c
> > +++ b/tests/debugfs_test.c
> > @@ -102,10 +102,16 @@ igt_main
> > debugfs = igt_debugfs_dir(fd);
> >
> > kmstest_set_vt_graphics_mode();
> > - igt_display_init(&display, fd);
> > + if (igt_display_available(fd))
> > + igt_display_init(&display, fd);
> > + else
> > + display.n_pipes = 0;
>
> Bleurgh. That's why I wasn't so keen on display_init including the
> require.
>
> Could we compromise by igt_require(igt_display_init(()); for tests
> that
> need it?
You mean zero igt_display_t when KMS is not suported and return false?
If so, it will add igt_require(igt_display_init(()); to several tests,
I don't think it is worthy to just make debugfs_test more pretty.
>
> > }
> >
> > igt_subtest("read_all_entries") {
> > + if (!display.n_pipes)
> > + goto skip_modeset;
> > +
> > /* try to light all pipes */
> > for_each_pipe(&display, pipe) {
>
> But why isn't for_each_pipe() a no-op if !n_pipes?
>
> It seems to be for(pipe = 0; pipe < display.n_pipes; pipe++)
As you got bellow it fails in the igt_display_commit2, here just
skiping both before hand but I'm fine with both ways.
>
> > igt_output_t *output;
> > @@ -132,7 +138,7 @@ igt_main
> > }
> >
> > igt_display_commit2(&display, display.is_atomic ?
> > COMMIT_ATOMIC : COMMIT_LEGACY);
>
> So presumably its ^. Something like
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 99d4c37..549d4e0 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -3260,6 +3260,10 @@ static int do_display_commit(igt_display_t
> *display,
> {
> int i, ret = 0;
> enum pipe pipe;
> +
> + if (!display->n_pipes)
> + return 0;
> +
> LOG_INDENT(display, "commit");
>
> igt_display_refresh(display);
> @@ -3312,6 +3316,9 @@ int igt_display_try_commit_atomic(igt_display_t
> *display, uint32_t flags, void *
> {
> int ret;
>
> + if (!display->n_pipes)
> + return 0;
> +
> LOG_INDENT(display, "commit");
>
> igt_display_refresh(display);
>
> as suitable defense.
not sure, it may be misleading return 0 in those functions, if
DRIVER_MODESET is unset user would not even be able to initialize
igt_display_t anyways unless we go by your
'igt_require(igt_display_init(())' suggestion.
>
> > +skip_modeset:
> > read_and_discard_sysfs_entries(debugfs, 0);
> > }
> >
> > @@ -140,6 +146,9 @@ igt_main
> > igt_output_t *output;
> > igt_plane_t *plane;
> >
> > + if (!display.n_pipes)
> > + goto skip_modeset2;
> > +
> > for_each_connected_output(&display, output)
> > igt_output_set_pipe(output, PIPE_NONE);
> >
> > @@ -148,7 +157,7 @@ igt_main
> > igt_plane_set_fb(plane, NULL);
> >
> > igt_display_commit2(&display, display.is_atomic ?
> > COMMIT_ATOMIC : COMMIT_LEGACY);
> > -
> > +skip_modeset2:
> > read_and_discard_sysfs_entries(debugfs, 0);
> > }
> >
> > diff --git a/tests/kms_3d.c b/tests/kms_3d.c
> > index bfc981ee..18764c41 100644
> > --- a/tests/kms_3d.c
> > +++ b/tests/kms_3d.c
> > @@ -36,6 +36,7 @@ igt_simple_main
> > int mode_count, connector_id;
> >
> > drm_fd = drm_open_driver_master(DRIVER_INTEL);
> > + igt_display_require(drm_fd);
> > res = drmModeGetResources(drm_fd);
> >
> > igt_assert(drmSetClientCap(drm_fd,
> > DRM_CLIENT_CAP_STEREO_3D, 1) >= 0);
>
> s/igt_assert/igt_require/
drm_setclientcap() is returning 0 for every client capability even with
DRIVER_MODESET unset. I sent a patch fixing it in kernel so we can use
igt_require here.
>
> > diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
> > index ce48d24f..5d3f9b85 100644
> > --- a/tests/kms_addfb_basic.c
> > +++ b/tests/kms_addfb_basic.c
> > @@ -671,8 +671,10 @@ int fd;
> >
> > igt_main
> > {
> > - igt_fixture
> > + igt_fixture {
> > fd = drm_open_driver_master(DRIVER_ANY);
> > + igt_display_require(fd);
>
> igt_require(has_addfb2()); is how we do it normally.
I saw your patch adding that but as you are still waiting for comments
in kernel patches, so skiping it for now.
>
> > + }
> >
> > invalid_tests(fd);
> >
> > diff --git a/tests/kms_draw_crc.c b/tests/kms_draw_crc.c
> > index 86dcf392..51c92b71 100644
> > --- a/tests/kms_draw_crc.c
> > +++ b/tests/kms_draw_crc.c
> > @@ -251,6 +251,7 @@ static void setup_environment(void)
> >
> > drm_fd = drm_open_driver_master(DRIVER_INTEL);
> > igt_require(drm_fd >= 0);
> > + igt_display_require(drm_fd);
> >
> > drm_res = drmModeGetResources(drm_fd);
>
> igt_require(drm_res)?
Sounds good, applying this to the other similar cases.
There is now only 4 users of igt_display_require()
- kms_addfb_basic
- kms_getfb
both will die when you patches are merged
- prime_vgem
it uses DRM_IOCTL_MODE_ADDFB2 and drmModePageFlip() the second
one returns -EINVAL with KMS disbaled and for other error cases, so I
guess is better just skip in the begiging of test_flip() with
igt_display_require()
- pm_rpm
discussed above
And igt_display_available() have:
- debugfs_test
discussed above
>
> > diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
> > index 81d796a4..67088883 100644
> > --- a/tests/kms_getfb.c
> > +++ b/tests/kms_getfb.c
> > @@ -198,8 +198,10 @@ igt_main
> > {
> > int fd;
> >
> > - igt_fixture
> > + igt_fixture {
> > fd = drm_open_driver_master(DRIVER_ANY);
> > + igt_display_require(fd);
>
> This should be another igt_require(has_getfb());
>
> So this looks like it becomes a lot simpler if we take the path of
> disallowing DRIVER_MODESET for INTEL_INFO()->num_pipes == 0 and
> suggest
> that all other drivers that allow disabling their KMS interface also
> drop the MODESET feature.
> -Chris
More information about the igt-dev
mailing list