[igt-dev] [PATCH i-g-t v4 2/2] tests: Check and skip unhandled tests that needs display
Chris Wilson
chris at chris-wilson.co.uk
Thu Sep 13 21:02:40 UTC 2018
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?
> }
>
> 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++)
> 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.
> +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/
> 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.
> + }
>
> 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)?
> 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