[igt-dev] [PATCH i-g-t] tests: Check and skip tests when driver don't have display enabled
Souza, Jose
jose.souza at intel.com
Fri Sep 7 23:31:47 UTC 2018
On Fri, 2018-09-07 at 15:57 +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-09-07 15:48:28)
> > On Thu, Sep 06, 2018 at 05:44:54PM -0700, José Roberto de Souza
> > wrote:
> > > Right now i915 is not doing the full job to complete disable
> > > display
> > > when i915.disable_display parameters is set, when that is
> > > completed
> > > it will cause several tests to fail, so here skiping all the
> > > tests
> > > that depends on modeset or display block when those are not
> > > available.
>
> <rant>
> This changelog is a pile of excuses.
>
> > > Cc: Jani Nikula <jani.nikula at intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > ---
> > > lib/igt_kms.c | 20 ++++++++++++++++++++
> > > lib/igt_kms.h | 1 +
> > > tests/debugfs_test.c | 1 +
> > > tests/gem_exec_nop.c | 1 +
> > > tests/kms_3d.c | 1 +
> > > tests/kms_addfb_basic.c | 4 +++-
> > > tests/kms_atomic.c | 1 +
> > > tests/kms_atomic_interruptible.c | 1 +
> > > tests/kms_available_modes_crc.c | 1 +
> > > tests/kms_busy.c | 1 +
> > > tests/kms_ccs.c | 1 +
> > > tests/kms_chamelium.c | 1 +
> > > tests/kms_chv_cursor_fail.c | 1 +
> > > tests/kms_color.c | 1 +
> > > tests/kms_concurrent.c | 1 +
> > > tests/kms_crtc_background_color.c | 1 +
> > > tests/kms_cursor_crc.c | 1 +
> > > tests/kms_cursor_legacy.c | 1 +
> > > tests/kms_draw_crc.c | 1 +
> > > tests/kms_fbcon_fbt.c | 1 +
> > > tests/kms_fence_pin_leak.c | 1 +
> > > tests/kms_flip.c | 1 +
> > > tests/kms_flip_event_leak.c | 1 +
> > > tests/kms_flip_tiling.c | 1 +
> > > tests/kms_force_connector_basic.c | 1 +
> > > tests/kms_frontbuffer_tracking.c | 1 +
> > > tests/kms_getfb.c | 4 +++-
> > > tests/kms_hdmi_inject.c | 1 +
> > > tests/kms_invalid_dotclock.c | 1 +
> > > tests/kms_legacy_colorkey.c | 1 +
> > > tests/kms_mmap_write_crc.c | 1 +
> > > tests/kms_panel_fitting.c | 1 +
> > > tests/kms_pipe_b_c_ivb.c | 1 +
> > > tests/kms_pipe_crc_basic.c | 1 +
> > > tests/kms_plane.c | 1 +
> > > tests/kms_plane_lowres.c | 1 +
> > > tests/kms_plane_multiple.c | 1 +
> > > tests/kms_plane_scaling.c | 1 +
> > > tests/kms_properties.c | 1 +
> > > tests/kms_psr.c | 1 +
> > > tests/kms_pwrite_crc.c | 1 +
> > > tests/kms_rmfb.c | 1 +
> > > tests/kms_rotation_crc.c | 1 +
> > > tests/kms_setmode.c | 1 +
> > > tests/kms_sysfs_edid_timing.c | 5 +++++
> > > tests/kms_tv_load_detect.c | 1 +
> > > tests/kms_universal_plane.c | 1 +
> > > tests/kms_vblank.c | 1 +
> > > tests/perf_pmu.c | 1 +
> > > tests/pm_backlight.c | 5 ++++-
> > > tests/pm_lpsp.c | 1 +
> > > tests/pm_rpm.c | 1 +
> > > tests/prime_vgem.c | 2 ++
> > > tests/testdisplay.c | 1 +
> > > 54 files changed, 85 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > index 62d84684..7578a10b 100644
> > > --- a/lib/igt_kms.c
> > > +++ b/lib/igt_kms.c
> > > @@ -2018,6 +2018,26 @@ int igt_display_get_n_pipes(igt_display_t
> > > *display)
> > > return display->n_pipes;
> > > }
> > >
> > > +/**
> > > + * igt_display_require_output:
> > > + * @drm_fd: a drm file descriptor
> > > + *
> > > + * Checks if driver supports modeset and have display enabled.
> > > + */
> > > +void igt_require_display(int fd)
> > > +{
> > > + drmModeRes *resources;
> > > + int pipes;
> > > +
> > > + resources = drmModeGetResources(fd);
>
> You don't need to allocate anything, so don't.
So what is the alternative here? I have sent a patch adding
DRM_CAP_MODESET, so we could check if modeset is supported by calling
drm_getcap() but you turn that down and said that we should
use drmModeGetResources().
https://patchwork.freedesktop.org/patch/243554/
>
> > > + igt_require_f(resources, "drm driver do not support
> > > modeset\n");
> > > +
> > > + pipes = resources->count_crtcs;
> > > + drmModeFreeResources(resources);
> > > +
> > > + igt_require_f(pipes > 0, "drm driver without display
> > > hardware\n");
> > > +}
> > > +
> > > /**
> > > * igt_display_require_output:
> > > * @display: A pointer to an #igt_display_t structure
> > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > > index 3a12f278..037bcb23 100644
> > > --- a/lib/igt_kms.h
> > > +++ b/lib/igt_kms.h
> > > @@ -388,6 +388,7 @@ 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_require_display(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..6d77ee72 100644
> > > --- a/tests/debugfs_test.c
> > > +++ b/tests/debugfs_test.c
> > > @@ -99,6 +99,7 @@ igt_main
> > > igt_fixture {
> > > fd = drm_open_driver_master(DRIVER_INTEL);
> > > igt_require_gem(fd);
> > > + igt_require_display(fd);
> > > debugfs = igt_debugfs_dir(fd);
> > >
> > > kmstest_set_vt_graphics_mode();
> >
> > I guess we lose the emon subtest here. But that's ilk only so meh.
>
> Skipping the entire set of tests just because there's no display
> attached, no.
Okay, this was easily fixed.
>
> > <snip>
> > > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> > > index 6ab2595b..6c347acc 100644
> > > --- a/tests/perf_pmu.c
> > > +++ b/tests/perf_pmu.c
> > > @@ -1397,6 +1397,7 @@ test_rc6(int gem_fd, unsigned int flags)
> > > if (flags & TEST_RUNTIME_PM) {
> > > drmModeRes *res;
> > >
> > > + igt_require_display(gem_fd);
> >
> > Might we want to just skip the display interaction here but still
> > execute the rest of the subtest? Or is that redundant when there is
> > no
> > display anyway?
>
> Nope. Should work without.
Okay, this was fixed too.
>
> > > res = drmModeGetResources(gem_fd);
> > > igt_assert(res);
> > >
> >
> > <snip>
> > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> > > index c24fd95b..627b1a1a 100644
> > > --- a/tests/pm_rpm.c
> > > +++ b/tests/pm_rpm.c
> > > @@ -719,6 +719,7 @@ static bool setup_environment(void)
> > >
> > > debugfs = igt_debugfs_dir(drm_fd);
> > > igt_require(debugfs != -1);
> > > + igt_require_display(drm_fd);
> >
> > Somewhat the same question here. Although this test looks like it's
> > expecting display stuff all over, so probably can't easily exclude
> > it.
>
> Nah, this is meant to work (and even tests) i915.disable_display.
Okay, if I do 'if (ms_data.res) igt_require(dmc_loaded());' in
setup_environment()(because I'm not loading DMC when display is
disabled in my kernel patches) and do some changes noted bellow I can
make some tests here run but some tests becomes not so much useful when
display is disabled, please check below:
- basic-rte
skip: as it only calls and disable_all_screens_and_wait() and
enable_one_screen_and_wait()
- drm-resources-equal
skip: as it calls enable_one_screen_and_wait() and
disable_all_screens_and_wait() to get drm info, between those calls and
compare if they are equal
- basic-pci-d3-state
skip: as it calls disable_all_screens_and_wait() and
enable_one_screen_and_wait() to check device is in D3, with display off
it will ways be in D3 I can change it to check if it is D3 in that case
but with no toggle.
- modeset-lpsp
- modeset-non-lpsp
skip: because it uses modeset
- dpms-lpsp
- dpms-non-lpsp
skip: as it calls enable_one_screen_with_type() that calls
set_mode_for_params()
- gem-mmap-cpu
- gem-mmap-gtt
- gem-pread
- gem-execbuf
can run: it do some gem operations with
enable_one_screen_and_wait() and then others with
disable_all_screens_and_wait(), I can make it run by adding a 'if
(!data->res) return;' in those 2 functions but not sure if it will be a
useful test
- gem-idle
run: a simple 'if (!data->res) return;' in
disable_all_screens() make this run
- gem-evict-pwrite
run: a simple 'if (!data->res) return;' in
disable_all_screens() make this run
- cursor
- cursor-dpms
- legacy-planes
- legacy-planes-dpms
- universal-planes
- universal-planes-dpms
skip: because it uses modeset
- reg-read-ioctl
run: a simple 'if (!data->res) return;' in
disable_all_screens() make this run
- i2c
skip: when the work to complete disable displays is finished
I2C will not be initialized
- pc8-residency
can run: it disable_all_screens() then check pc8 residence then
enable_one_screen() check if pc8 residence stoped but not sure if it
will be a useful test
- debugfs-read
run
- debugfs-forcewake-user
run: a simple 'if (!data->res) return;' in
disable_all_screens() make this run
- sysfs-read
run
- dpms-mode-unset-lpsp
skip: as it calls enable_one_screen_with_type() that calls
set_mode_for_params()
- dpms-mode-unset-non-lpsp
skip: as it calls enable_one_screen_with_type() that calls
set_mode_for_params()
- fences
- fences-dpms
- modeset-lpsp-stress
- modeset-non-lpsp-stress
- modeset-lpsp-stress-no-wait
- modeset-non-lpsp-stress-no-wait
- modeset-pc8-residency-stress
- modeset-stress-extra-wait
skip: because it uses modeset
- system-suspend-devices
- system-suspend
- system-suspend-execbuf
run: a simple 'if (!data->res) return;' in
disable_all_screens() make this run
- system-suspend-modeset
run: a simple 'if (!data->res) return;' in
disable_all_screens() and enable_one_screen_and_wait() make this run,
but I guess it will not be useful anymore
- system-hibernate-devices
- system-hibernate
- gem-execbuf-stress
- gem-execbuf-stress-pc8
- gem-execbuf-stress-extra-wait
- pm-tiling
run: a simple 'if (!data->res) return;' in
disable_all_screens() and enable_one_screen_and_wait() make this run
- pm-caching
run: a simple 'if (!data->res) return;' in
disable_all_screens() make this run
- module-reload
run
>
> Nacked-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris
More information about the igt-dev
mailing list