Re: [PATCH i-g-t 2/3] Unify handling of slow/combinatorial tests

Thomas Wood
2015-10-23T14:55:44Z

On 23 October 2015 at 12:42, David Weinehall
<david.weinehall@linux.intel.com> wrote:
> Some tests should not be run by default, due to their slow,
> and sometimes superfluous, nature.
>
> We still want to be able to run these tests though in some cases.
> Until now there's been no unified way of handling this. Remedy
> this by introducing the --with-slow-combinatorial option to
> igt_core, and use it in gem_concurrent_blit & kms_frontbuffer_tracking.
> ---
> lib/igt_core.c | 19 ++++++
> lib/igt_core.h | 1 +
> tests/gem_concurrent_blit.c | 40 ++++++++----
> tests/kms_frontbuffer_tracking.c | 135 +++++++++++++++++++++++++++------------
> 4 files changed, 142 insertions(+), 53 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 59127cafe606..ba40ce0e0ead 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -216,6 +216,7 @@ const char *igt_interactive_debug;
>
> /* subtests helpers */
> static bool list_subtests = false;
> +static bool with_slow_combinatorial = false;
> static char *run_single_subtest = NULL;
> static bool run_single_subtest_found = false;
> static const char *in_subtest = NULL;
> @@ -235,6 +236,7 @@ bool test_child;
>
> enum {
> OPT_LIST_SUBTESTS,
> + OPT_WITH_SLOW_COMBINATORIAL,
> OPT_RUN_SUBTEST,
> OPT_DESCRIPTION,
> OPT_DEBUG,
> @@ -478,6 +480,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
>
> fprintf(f, "Usage: %s [OPTIONS]\n", command_str);
> fprintf(f, " --list-subtests\n"
> + " --with-slow-combinatorial\n"
> " --run-subtest <pattern>\n"
> " --debug[=log-domain]\n"
> " --interactive-debug[=domain]\n"
> @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv,
> int c, option_index = 0, i, x;
> static struct option long_options[] = {
> {"list-subtests", 0, 0, OPT_LIST_SUBTESTS},
> + {"with-slow-combinatorial", 0, 0, OPT_WITH_SLOW_COMBINATORIAL},
> {"run-subtest", 1, 0, OPT_RUN_SUBTEST},
> {"help-description", 0, 0, OPT_DESCRIPTION},
> {"debug", optional_argument, 0, OPT_DEBUG},
> @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv,
> if (!run_single_subtest)
> list_subtests = true;
> break;
> + case OPT_WITH_SLOW_COMBINATORIAL:
> + if (!run_single_subtest)

This will cause piglit (and therefore QA) to unconditionally run all
tests marked as slow, since it runs subtests individually.

> + with_slow_combinatorial = true;
> + break;
> case OPT_RUN_SUBTEST:
> if (!list_subtests)
> run_single_subtest = strdup(optarg);
> @@ -1629,6 +1637,17 @@ void igt_skip_on_simulation(void)
> igt_require(!igt_run_in_simulation());
> }
>
> +/**
> + * igt_slow_combinatorial:
> + *
> + * This is used to define subtests that should only be listed/run
> + * when the "--with-slow-combinatorial" has been specified

This isn't quite correct, as the subtests that use
igt_slow_combinatorial will still always be listed.

> + */
> +void igt_slow_combinatorial(void)
> +{
> + igt_skip_on(!with_slow_combinatorial);

Although it is convenient to just skip the tests when the
--with-slow-combinatorial flag is passed, it may be useful to be able
to classify the subtests before they are run, so that they are
filtered out from the test list entirely. An approach that can do this
might also be used to mark tests as being part of the basic acceptance
tests, so that they can be marked as such without relying on the
naming convention.

> +}
> +
> /* structured logging */
>
> /**
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 5ae09653fd55..6ddf25563275 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -680,6 +680,7 @@ bool igt_run_in_simulation(void);
> #define SLOW_QUICK(slow,quick) (igt_run_in_simulation() ? (quick) : (slow))
>
> void igt_skip_on_simulation(void);
> +void igt_slow_combinatorial(void);
>
> extern const char *igt_interactive_debug;
>
> diff --git a/tests/gem_concurrent_blit.c b/tests/gem_concurrent_blit.c
> index 1d2d787202df..311b6829e984 100644
> --- a/tests/gem_concurrent_blit.c
> +++ b/tests/gem_concurrent_blit.c
> @@ -931,9 +931,6 @@ run_basic_modes(const struct access_mode *mode,
> struct buffers buffers;
>
> for (h = hangs; h->suffix; h++) {
> - if (!all && *h->suffix)
> - continue;
> -
> for (p = all ? pipelines : pskip; p->prefix; p++) {
> igt_fixture {
> batch = buffers_init(&buffers, mode, fd);
> @@ -941,6 +938,8 @@ run_basic_modes(const struct access_mode *mode,
>
> /* try to overwrite the source values */
> igt_subtest_f("%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> buffers_create(&buffers, num_buffers);
> @@ -950,6 +949,8 @@ run_basic_modes(const struct access_mode *mode,
> }
>
> igt_subtest_f("%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> buffers_create(&buffers, num_buffers);
> @@ -959,6 +960,8 @@ run_basic_modes(const struct access_mode *mode,
> }
>
> igt_subtest_f("%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> buffers_create(&buffers, num_buffers);
> @@ -968,6 +971,8 @@ run_basic_modes(const struct access_mode *mode,
> }
>
> igt_subtest_f("%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> igt_require(rendercopy);
> @@ -978,6 +983,8 @@ run_basic_modes(const struct access_mode *mode,
> }
>
> igt_subtest_f("%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> buffers_create(&buffers, num_buffers);
> @@ -988,6 +995,8 @@ run_basic_modes(const struct access_mode *mode,
>
> /* try to intermix copies with GPU copies*/
> igt_subtest_f("%s-%s-intermix-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> igt_require(rendercopy);
> @@ -997,6 +1006,8 @@ run_basic_modes(const struct access_mode *mode,
> p->copy, h->hang);
> }
> igt_subtest_f("%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> igt_require(rendercopy);
> @@ -1006,6 +1017,8 @@ run_basic_modes(const struct access_mode *mode,
> p->copy, h->hang);
> }
> igt_subtest_f("%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> igt_require(rendercopy);
> @@ -1017,6 +1030,8 @@ run_basic_modes(const struct access_mode *mode,
>
> /* try to read the results before the copy completes */
> igt_subtest_f("%s-%s-early-read%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> buffers_create(&buffers, num_buffers);
> @@ -1027,6 +1042,8 @@ run_basic_modes(const struct access_mode *mode,
>
> /* concurrent reads */
> igt_subtest_f("%s-%s-read-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> buffers_create(&buffers, num_buffers);
> @@ -1035,6 +1052,8 @@ run_basic_modes(const struct access_mode *mode,
> p->copy, h->hang);
> }
> igt_subtest_f("%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> igt_require(rendercopy);
> @@ -1046,6 +1065,8 @@ run_basic_modes(const struct access_mode *mode,
>
> /* and finally try to trick the kernel into loosing the pending write */
> igt_subtest_f("%s-%s-gpu-read-after-write%s%s", mode->name, p->prefix, suffix, h->suffix) {
> + if (*h->suffix)
> + igt_slow_combinatorial();
> h->require();
> p->require();
> buffers_create(&buffers, num_buffers);
> @@ -1064,13 +1085,11 @@ run_basic_modes(const struct access_mode *mode,
> static void
> run_modes(const struct access_mode *mode)
> {
> - if (all) {
> - run_basic_modes(mode, "", run_single);
> + run_basic_modes(mode, "", run_single);
>
> - igt_fork_signal_helper();
> - run_basic_modes(mode, "-interruptible", run_interruptible);
> - igt_stop_signal_helper();
> - }
> + igt_fork_signal_helper();
> + run_basic_modes(mode, "-interruptible", run_interruptible);
> + igt_stop_signal_helper();
>
> igt_fork_signal_helper();
> run_basic_modes(mode, "-forked", run_forked);
> @@ -1083,9 +1102,6 @@ igt_main
>
> igt_skip_on_simulation();
>
> - if (strstr(igt_test_name(), "all"))
> - all = true;
> -
> igt_fixture {
> fd = drm_open_driver(DRIVER_INTEL);
> devid = intel_get_drm_devid(fd);
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index d97e148c5073..6f84ef0813d9 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -47,8 +47,8 @@ IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and "
> * combinations that are somewhat redundant and don't add much value to the
> * test. For example, since we already do the offscreen testing with a single
> * pipe enabled, there's no much value in doing it again with dual pipes. If you
> - * still want to try these redundant tests, you need to use the --show-hidden
> - * option.
> + * still want to try these redundant tests, you need to use the
> + * "--with-slow-combinatorial" option.
> *
> * The most important hidden thing is the FEATURE_NONE set of tests. Whenever
> * you get a failure on any test, it is important to check whether the same test
> @@ -116,6 +116,10 @@ struct test_mode {
> } format;
>
> enum igt_draw_method method;
> +
> + /* The test is slow and/or combinatorial;
> + * skip unless otherwise specified */
> + bool slow;
> };
>
> enum flip_type {
> @@ -237,7 +241,6 @@ struct {
> bool fbc_check_last_action;
> bool no_edp;
> bool small_modes;
> - bool show_hidden;
> int step;
> int only_feature;
> int only_pipes;
> @@ -250,7 +253,6 @@ struct {
> .fbc_check_last_action = true,
> .no_edp = false,
> .small_modes = false,
> - .show_hidden= false,
> .step = 0,
> .only_feature = FEATURE_COUNT,
> .only_pipes = PIPE_COUNT,
> @@ -2892,9 +2894,6 @@ static int opt_handler(int option, int option_index, void *data)
> case 'm':
> opt.small_modes = true;
> break;
> - case 'i':
> - opt.show_hidden = true;
> - break;
> case 't':
> opt.step++;
> break;
> @@ -2942,7 +2941,6 @@ const char *help_str =
> " --no-fbc-action-check Don't check for the FBC last action\n"
> " --no-edp Don't use eDP monitors\n"
> " --use-small-modes Use smaller resolutions for the modes\n"
> -" --show-hidden Show hidden subtests\n"
> " --step Stop on each step so you can check the screen\n"
> " --nop-only Only run the \"nop\" feature subtests\n"
> " --fbc-only Only run the \"fbc\" feature subtests\n"
> @@ -3036,6 +3034,7 @@ static const char *format_str(enum pixel_format format)
>
> #define TEST_MODE_ITER_BEGIN(t) \
> t.format = FORMAT_DEFAULT; \
> + t.slow = false; \
> for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) { \
> for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) { \
> for (t.screen = 0; t.screen < SCREEN_COUNT; t.screen++) { \
> @@ -3046,15 +3045,15 @@ static const char *format_str(enum pixel_format format)
> continue; \
> if (t.screen == SCREEN_OFFSCREEN && t.plane != PLANE_PRI) \
> continue; \
> - if (!opt.show_hidden && t.pipes == PIPE_DUAL && \
> + if (t.pipes == PIPE_DUAL && \
> t.screen == SCREEN_OFFSCREEN) \
> - continue; \
> - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE) \
> - && t.feature == FEATURE_NONE) \
> - continue; \
> - if (!opt.show_hidden && t.fbs == FBS_SHARED && \
> + t.slow = true; \
> + if (opt.only_feature != FEATURE_NONE && \
> + t.feature == FEATURE_NONE) \
> + t.slow = true; \
> + if (t.fbs == FBS_SHARED && \
> (t.plane == PLANE_CUR || t.plane == PLANE_SPR)) \
> - continue;
> + t.slow = true;
>
>
> #define TEST_MODE_ITER_END } } } } } }
> @@ -3069,7 +3068,6 @@ int main(int argc, char *argv[])
> { "no-fbc-action-check", 0, 0, 'a'},
> { "no-edp", 0, 0, 'e'},
> { "use-small-modes", 0, 0, 'm'},
> - { "show-hidden", 0, 0, 'i'},
> { "step", 0, 0, 't'},
> { "nop-only", 0, 0, 'n'},
> { "fbc-only", 0, 0, 'f'},
> @@ -3088,9 +3086,11 @@ int main(int argc, char *argv[])
> setup_environment();
>
> for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) {
> - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE)
> - && t.feature == FEATURE_NONE)
> - continue;
> + bool slow = false;
> +
> + if (opt.only_feature != FEATURE_NONE &&
> + t.feature == FEATURE_NONE)
> + slow = true;
> for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) {
> t.screen = SCREEN_PRIM;
> t.plane = PLANE_PRI;
> @@ -3101,8 +3101,11 @@ int main(int argc, char *argv[])
>
> igt_subtest_f("%s-%s-rte",
> feature_str(t.feature),
> - pipes_str(t.pipes))
> + pipes_str(t.pipes)) {
> + if (slow)
> + igt_slow_combinatorial();
> rte_subtest(&t);
> + }
> }
> }
>
> @@ -3113,39 +3116,52 @@ int main(int argc, char *argv[])
> screen_str(t.screen),
> plane_str(t.plane),
> fbs_str(t.fbs),
> - igt_draw_get_method_name(t.method))
> + igt_draw_get_method_name(t.method)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> draw_subtest(&t);
> + }
> TEST_MODE_ITER_END
>
> TEST_MODE_ITER_BEGIN(t)
> if (t.plane != PLANE_PRI ||
> - t.screen == SCREEN_OFFSCREEN ||
> - (!opt.show_hidden && t.method != IGT_DRAW_BLT))
> + t.screen == SCREEN_OFFSCREEN)
> continue;
> + if (t.method != IGT_DRAW_BLT)
> + t.slow = true;
>
> igt_subtest_f("%s-%s-%s-%s-flip-%s",
> feature_str(t.feature),
> pipes_str(t.pipes),
> screen_str(t.screen),
> fbs_str(t.fbs),
> - igt_draw_get_method_name(t.method))
> + igt_draw_get_method_name(t.method)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> flip_subtest(&t, FLIP_PAGEFLIP);
> + }
>
> igt_subtest_f("%s-%s-%s-%s-evflip-%s",
> feature_str(t.feature),
> pipes_str(t.pipes),
> screen_str(t.screen),
> fbs_str(t.fbs),
> - igt_draw_get_method_name(t.method))
> + igt_draw_get_method_name(t.method)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> flip_subtest(&t, FLIP_PAGEFLIP_EVENT);
> + }
>
> igt_subtest_f("%s-%s-%s-%s-msflip-%s",
> feature_str(t.feature),
> pipes_str(t.pipes),
> screen_str(t.screen),
> fbs_str(t.fbs),
> - igt_draw_get_method_name(t.method))
> + igt_draw_get_method_name(t.method)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> flip_subtest(&t, FLIP_MODESET);
> + }
>
> TEST_MODE_ITER_END
>
> @@ -3159,8 +3175,11 @@ int main(int argc, char *argv[])
> igt_subtest_f("%s-%s-%s-fliptrack",
> feature_str(t.feature),
> pipes_str(t.pipes),
> - fbs_str(t.fbs))
> + fbs_str(t.fbs)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> fliptrack_subtest(&t, FLIP_PAGEFLIP);
> + }
> TEST_MODE_ITER_END
>
> TEST_MODE_ITER_BEGIN(t)
> @@ -3174,16 +3193,22 @@ int main(int argc, char *argv[])
> pipes_str(t.pipes),
> screen_str(t.screen),
> plane_str(t.plane),
> - fbs_str(t.fbs))
> + fbs_str(t.fbs)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> move_subtest(&t);
> + }
>
> igt_subtest_f("%s-%s-%s-%s-%s-onoff",
> feature_str(t.feature),
> pipes_str(t.pipes),
> screen_str(t.screen),
> plane_str(t.plane),
> - fbs_str(t.fbs))
> + fbs_str(t.fbs)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> onoff_subtest(&t);
> + }
> TEST_MODE_ITER_END
>
> TEST_MODE_ITER_BEGIN(t)
> @@ -3197,23 +3222,30 @@ int main(int argc, char *argv[])
> pipes_str(t.pipes),
> screen_str(t.screen),
> plane_str(t.plane),
> - fbs_str(t.fbs))
> + fbs_str(t.fbs)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> fullscreen_plane_subtest(&t);
> + }
> TEST_MODE_ITER_END
>
> TEST_MODE_ITER_BEGIN(t)
> if (t.screen != SCREEN_PRIM ||
> - t.method != IGT_DRAW_BLT ||
> - (!opt.show_hidden && t.plane != PLANE_PRI) ||
> - (!opt.show_hidden && t.fbs != FBS_INDIVIDUAL))
> + t.method != IGT_DRAW_BLT)
> continue;
> + if (t.plane != PLANE_PRI ||
> + t.fbs != FBS_INDIVIDUAL)
> + t.slow = true;
>
> igt_subtest_f("%s-%s-%s-%s-multidraw",
> feature_str(t.feature),
> pipes_str(t.pipes),
> plane_str(t.plane),
> - fbs_str(t.fbs))
> + fbs_str(t.fbs)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> multidraw_subtest(&t);
> + }
> TEST_MODE_ITER_END
>
> TEST_MODE_ITER_BEGIN(t)
> @@ -3224,8 +3256,11 @@ int main(int argc, char *argv[])
> t.method != IGT_DRAW_MMAP_GTT)
> continue;
>
> - igt_subtest_f("%s-farfromfence", feature_str(t.feature))
> + igt_subtest_f("%s-farfromfence", feature_str(t.feature)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> farfromfence_subtest(&t);
> + }
> TEST_MODE_ITER_END
>
> TEST_MODE_ITER_BEGIN(t)
> @@ -3243,8 +3278,11 @@ int main(int argc, char *argv[])
> igt_subtest_f("%s-%s-draw-%s",
> feature_str(t.feature),
> format_str(t.format),
> - igt_draw_get_method_name(t.method))
> + igt_draw_get_method_name(t.method)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> format_draw_subtest(&t);
> + }
> }
> TEST_MODE_ITER_END
>
> @@ -3256,8 +3294,11 @@ int main(int argc, char *argv[])
> continue;
> igt_subtest_f("%s-%s-scaledprimary",
> feature_str(t.feature),
> - fbs_str(t.fbs))
> + fbs_str(t.fbs)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> scaledprimary_subtest(&t);
> + }
> TEST_MODE_ITER_END
>
> TEST_MODE_ITER_BEGIN(t)
> @@ -3268,19 +3309,31 @@ int main(int argc, char *argv[])
> t.method != IGT_DRAW_MMAP_CPU)
> continue;
>
> - igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature))
> + igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> modesetfrombusy_subtest(&t);
> + }
>
> if (t.feature & FEATURE_FBC)
> - igt_subtest_f("%s-badstride", feature_str(t.feature))
> + igt_subtest_f("%s-badstride", feature_str(t.feature)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> badstride_subtest(&t);
> + }
>
> if (t.feature & FEATURE_PSR)
> - igt_subtest_f("%s-slowdraw", feature_str(t.feature))
> + igt_subtest_f("%s-slowdraw", feature_str(t.feature)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> slow_draw_subtest(&t);
> + }
>
> - igt_subtest_f("%s-suspend", feature_str(t.feature))
> + igt_subtest_f("%s-suspend", feature_str(t.feature)) {
> + if (t.slow)
> + igt_slow_combinatorial();
> suspend_subtest(&t);
> + }
> TEST_MODE_ITER_END
>
> igt_fixture
> --
> 2.6.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx