diff mbox

[i-g-t,2/3] Unify handling of slow/combinatorial tests

Message ID 1446031792-22881-3-git-send-email-david.weinehall@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Weinehall Oct. 28, 2015, 11:29 a.m. UTC 
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 in some cases.
Until now there's been no unified way of handling this. Remedy
this by introducing the --all option to igt_core,
and use it in gem_concurrent_blit & kms_frontbuffer_tracking.

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
---
 lib/igt_core.c                   |  24 +++++
 lib/igt_core.h                   |   7 ++
 tests/gem_concurrent_blit.c      |  44 ++++-----
 tests/kms_frontbuffer_tracking.c | 208 ++++++++++++++++++++++-----------------
 4 files changed, 165 insertions(+), 118 deletions(-)

Comments

Paulo Zanoni Oct. 28, 2015, 4:12 p.m. UTC | #1 
2015-10-28 9:29 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>:
> 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 in some cases.
> Until now there's been no unified way of handling this. Remedy
> this by introducing the --all option to igt_core,
> and use it in gem_concurrent_blit & kms_frontbuffer_tracking.

I really think you should explain both your plan and its
implementation in more details here.

>
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> ---
>  lib/igt_core.c                   |  24 +++++
>  lib/igt_core.h                   |   7 ++
>  tests/gem_concurrent_blit.c      |  44 ++++-----
>  tests/kms_frontbuffer_tracking.c | 208 ++++++++++++++++++++++-----------------
>  4 files changed, 165 insertions(+), 118 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 59127cafe606..6575b9d6bf0d 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;

The option is called --all, the new subtest macro is _slow and the
variables and enums are called with_slow_combinatorial. Is this
intentional?

>  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"
> +                  "  --all\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},
> +               {"all", 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)
> +                               with_slow_combinatorial = true;
> +                       break;
>                 case OPT_RUN_SUBTEST:
>                         if (!list_subtests)
>                                 run_single_subtest = strdup(optarg);
> @@ -1629,6 +1637,22 @@ void igt_skip_on_simulation(void)
>                 igt_require(!igt_run_in_simulation());
>  }
>
> +/**
> + * __igt_slow_combinatorial:
> + *
> + * This is used to skip subtests that should only be included
> + * when the "--all" command line option has been specified.  This version
> + * is intended as a test.
> + *
> + * @slow_test: true if the subtest is part of the slow/combinatorial set
> + *
> + * Returns: true if the test should be run, false if the test should be skipped
> + */
> +bool __igt_slow_combinatorial(bool slow_test)
> +{
> +       return !slow_test || with_slow_combinatorial;
> +}
> +
>  /* structured logging */
>
>  /**
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 5ae09653fd55..7b592278bf6c 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -191,6 +191,12 @@ bool __igt_run_subtest(const char *subtest_name);
>  #define igt_subtest_f(f...) \
>         __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
>
> +bool __igt_slow_combinatorial(bool slow_test);
> +

We also need a igt_subtest_slow() version (without "_f") and some
comments explaining what's the real difference between them and the
other macros, like the other igt_subtest_* macros.

> +#define igt_subtest_slow_f(__slow, f...) \
> +       if (__igt_slow_combinatorial(__slow)) \
> +       __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)

Missing tab in the line above.

> +
>  const char *igt_subtest_name(void);
>  bool igt_only_list_subtests(void);
>
> @@ -669,6 +675,7 @@ void igt_disable_exit_handler(void);
>
>  /* helpers to automatically reduce test runtime in simulation */
>  bool igt_run_in_simulation(void);
> +

Bad chunk.

>  /**
>   * SLOW_QUICK:
>   * @slow: value in simulation mode
> diff --git a/tests/gem_concurrent_blit.c b/tests/gem_concurrent_blit.c
> index 1d2d787202df..fe37cc707583 100644
> --- a/tests/gem_concurrent_blit.c
> +++ b/tests/gem_concurrent_blit.c
> @@ -55,7 +55,6 @@ IGT_TEST_DESCRIPTION("Test of pread/pwrite/mmap behavior when writing to active"
>
>  int fd, devid, gen;
>  struct intel_batchbuffer *batch;
> -int all;
>
>  static void
>  nop_release_bo(drm_intel_bo *bo)
> @@ -931,16 +930,14 @@ 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++) {
> +               for (p = __igt_slow_combinatorial(true) ? pipelines : pskip;
> +                    p->prefix; p++) {
>                         igt_fixture {
>                                 batch = buffers_init(&buffers, mode, fd);
>                         }
>
>                         /* try to overwrite the source values */
> -                       igt_subtest_f("%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -949,7 +946,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               p->copy, h->hang);
>                         }
>
> -                       igt_subtest_f("%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -958,7 +955,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               p->copy, h->hang);
>                         }
>
> -                       igt_subtest_f("%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -967,7 +964,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               p->copy, h->hang);
>                         }
>
> -                       igt_subtest_f("%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -977,7 +974,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               p->copy, h->hang);
>                         }
>
> -                       igt_subtest_f("%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -987,7 +984,7 @@ 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) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-intermix-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -996,7 +993,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               do_intermix_rcs,
>                                               p->copy, h->hang);
>                         }
> -                       igt_subtest_f("%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -1005,7 +1002,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               do_intermix_bcs,
>                                               p->copy, h->hang);
>                         }
> -                       igt_subtest_f("%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -1016,7 +1013,7 @@ 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) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-early-read%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -1026,7 +1023,7 @@ 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) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-read-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -1034,7 +1031,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               do_read_read_bcs,
>                                               p->copy, h->hang);
>                         }
> -                       igt_subtest_f("%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -1045,7 +1042,7 @@ 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) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-gpu-read-after-write%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -1064,13 +1061,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 +1078,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 15707b9b9040..86fd7ca08692 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -47,8 +47,7 @@ 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 --all 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 +115,10 @@ struct test_mode {
>         } format;
>
>         enum igt_draw_method method;
> +
> +       /* The test is slow and/or combinatorial;
> +        * skip unless otherwise specified */
> +       bool slow;

My problem with this is that exactly none of the tests marked as
"slow" are actually slow here... They're either redudant or for debug
purposes, not slow.

>  };
>
>  enum flip_type {
> @@ -237,7 +240,6 @@ struct {
>         bool fbc_check_last_action;
>         bool no_edp;
>         bool small_modes;
> -       bool show_hidden;
>         int step;
>         int only_pipes;
>         int shared_fb_x_offset;
> @@ -249,7 +251,6 @@ struct {
>         .fbc_check_last_action = true,
>         .no_edp = false,
>         .small_modes = false,
> -       .show_hidden= false,
>         .step = 0,
>         .only_pipes = PIPE_COUNT,
>         .shared_fb_x_offset = 500,
> @@ -2933,9 +2934,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;
> @@ -2971,7 +2969,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"
>  "  --shared-fb-x offset        Use 'offset' as the X offset for the shared FB\n"
>  "  --shared-fb-y offset        Use 'offset' as the Y offset for the shared FB\n"
> @@ -3068,18 +3065,19 @@ static const char *format_str(enum pixel_format format)
>         for (t.plane = 0; t.plane < PLANE_COUNT; t.plane++) {              \
>         for (t.fbs = 0; t.fbs < FBS_COUNT; t.fbs++) {                      \
>         for (t.method = 0; t.method < IGT_DRAW_METHOD_COUNT; t.method++) { \
> +               t.slow = false;                                            \
>                 if (t.pipes == PIPE_SINGLE && t.screen == SCREEN_SCND)     \
>                         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 && t.feature == FEATURE_NONE)         \
> -                       continue;                                          \
> -               if (!opt.show_hidden && t.fbs == FBS_SHARED &&             \
> +                       t.slow = true;                                     \
> +               if (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 } } } } } }
> @@ -3094,7 +3092,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'},
>                 { "shared-fb-x",              1, 0, 'x'},
>                 { "shared-fb-y",              1, 0, 'y'},
> @@ -3110,8 +3107,9 @@ int main(int argc, char *argv[])
>                 setup_environment();
>
>         for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) {
> -               if (!opt.show_hidden && t.feature == FEATURE_NONE)
> -                       continue;
> +               t.slow = false;
> +               if (t.feature == FEATURE_NONE)
> +                       t.slow = true;
>                 for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) {
>                         t.screen = SCREEN_PRIM;
>                         t.plane = PLANE_PRI;
> @@ -3120,52 +3118,58 @@ int main(int argc, char *argv[])
>                         /* Make sure nothing is using this value. */
>                         t.method = -1;
>
> -                       igt_subtest_f("%s-%s-rte",
> -                                     feature_str(t.feature),
> -                                     pipes_str(t.pipes))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-%s-rte",
> +                                          feature_str(t.feature),
> +                                          pipes_str(t.pipes))
>                                 rte_subtest(&t);
>                 }
>         }
>
>         TEST_MODE_ITER_BEGIN(t)
> -               igt_subtest_f("%s-%s-%s-%s-%s-draw-%s",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             screen_str(t.screen),
> -                             plane_str(t.plane),
> -                             fbs_str(t.fbs),
> -                             igt_draw_get_method_name(t.method))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-%s-%s-draw-%s",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  screen_str(t.screen),
> +                                  plane_str(t.plane),
> +                                  fbs_str(t.fbs),
> +                                  igt_draw_get_method_name(t.method))
>                         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;
> -
> -               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))
> +               if (t.method != IGT_DRAW_BLT)
> +                       t.slow = true;
> +
> +               igt_subtest_slow_f(t.slow,
> +                                  "%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))
>                         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_subtest_slow_f(t.slow,
> +                                  "%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))
>                         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_subtest_slow_f(t.slow,
> +                                  "%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))
>                         flip_subtest(&t, FLIP_MODESET);
>
>         TEST_MODE_ITER_END
> @@ -3177,10 +3181,11 @@ int main(int argc, char *argv[])
>                     (t.feature & FEATURE_FBC) == 0)
>                         continue;
>
> -               igt_subtest_f("%s-%s-%s-fliptrack",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             fbs_str(t.fbs))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-fliptrack",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  fbs_str(t.fbs))
>                         fliptrack_subtest(&t, FLIP_PAGEFLIP);
>         TEST_MODE_ITER_END
>
> @@ -3190,20 +3195,22 @@ int main(int argc, char *argv[])
>                     t.plane == PLANE_PRI)
>                         continue;
>
> -               igt_subtest_f("%s-%s-%s-%s-%s-move",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             screen_str(t.screen),
> -                             plane_str(t.plane),
> -                             fbs_str(t.fbs))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-%s-%s-move",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  screen_str(t.screen),
> +                                  plane_str(t.plane),
> +                                  fbs_str(t.fbs))
>                         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))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%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))
>                         onoff_subtest(&t);
>         TEST_MODE_ITER_END
>
> @@ -3213,27 +3220,30 @@ int main(int argc, char *argv[])
>                     t.plane != PLANE_SPR)
>                         continue;
>
> -               igt_subtest_f("%s-%s-%s-%s-%s-fullscreen",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             screen_str(t.screen),
> -                             plane_str(t.plane),
> -                             fbs_str(t.fbs))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-%s-%s-fullscreen",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  screen_str(t.screen),
> +                                  plane_str(t.plane),
> +                                  fbs_str(t.fbs))
>                         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;
> -
> -               igt_subtest_f("%s-%s-%s-%s-multidraw",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             plane_str(t.plane),
> -                             fbs_str(t.fbs))
> +               if (t.plane != PLANE_PRI ||
> +                   t.fbs != FBS_INDIVIDUAL)
> +                       t.slow = true;
> +
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-%s-multidraw",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  plane_str(t.plane),
> +                                  fbs_str(t.fbs))
>                         multidraw_subtest(&t);
>         TEST_MODE_ITER_END
>
> @@ -3245,7 +3255,9 @@ int main(int argc, char *argv[])
>                     t.method != IGT_DRAW_MMAP_GTT)
>                         continue;
>
> -               igt_subtest_f("%s-farfromfence", feature_str(t.feature))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-farfromfence",
> +                                  feature_str(t.feature))
>                         farfromfence_subtest(&t);
>         TEST_MODE_ITER_END
>
> @@ -3261,10 +3273,11 @@ int main(int argc, char *argv[])
>                         if (t.format == FORMAT_DEFAULT)
>                                 continue;
>
> -                       igt_subtest_f("%s-%s-draw-%s",
> -                                     feature_str(t.feature),
> -                                     format_str(t.format),
> -                                     igt_draw_get_method_name(t.method))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-%s-draw-%s",
> +                                          feature_str(t.feature),
> +                                          format_str(t.format),
> +                                          igt_draw_get_method_name(t.method))
>                                 format_draw_subtest(&t);
>                 }
>         TEST_MODE_ITER_END
> @@ -3275,9 +3288,10 @@ int main(int argc, char *argv[])
>                     t.plane != PLANE_PRI ||
>                     t.method != IGT_DRAW_MMAP_CPU)
>                         continue;
> -               igt_subtest_f("%s-%s-scaledprimary",
> -                             feature_str(t.feature),
> -                             fbs_str(t.fbs))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-scaledprimary",
> +                                  feature_str(t.feature),
> +                                  fbs_str(t.fbs))
>                         scaledprimary_subtest(&t);
>         TEST_MODE_ITER_END
>
> @@ -3289,22 +3303,32 @@ int main(int argc, char *argv[])
>                     t.method != IGT_DRAW_MMAP_CPU)
>                         continue;
>
> -               igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-modesetfrombusy",
> +                                  feature_str(t.feature))
>                         modesetfrombusy_subtest(&t);
>
>                 if (t.feature & FEATURE_FBC) {
> -                       igt_subtest_f("%s-badstride", feature_str(t.feature))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-badstride",
> +                                          feature_str(t.feature))
>                                 badstride_subtest(&t);
>
> -                       igt_subtest_f("%s-stridechange", feature_str(t.feature))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-stridechange",
> +                                          feature_str(t.feature))
>                                 stridechange_subtest(&t);
>                 }
>
>                 if (t.feature & FEATURE_PSR)
> -                       igt_subtest_f("%s-slowdraw", feature_str(t.feature))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-slowdraw",
> +                                          feature_str(t.feature))
>                                 slow_draw_subtest(&t);
>
> -               igt_subtest_f("%s-suspend", feature_str(t.feature))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-suspend",
> +                                  feature_str(t.feature))
>                         suspend_subtest(&t);
>         TEST_MODE_ITER_END
>
> --
> 2.6.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Thomas Wood Oct. 28, 2015, 5:14 p.m. UTC | #2 
On 28 October 2015 at 11:29, 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 in some cases.
> Until now there's been no unified way of handling this. Remedy
> this by introducing the --all option to igt_core,
> and use it in gem_concurrent_blit & kms_frontbuffer_tracking.
>
> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> ---
>  lib/igt_core.c                   |  24 +++++
>  lib/igt_core.h                   |   7 ++
>  tests/gem_concurrent_blit.c      |  44 ++++-----
>  tests/kms_frontbuffer_tracking.c | 208 ++++++++++++++++++++++-----------------
>  4 files changed, 165 insertions(+), 118 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 59127cafe606..6575b9d6bf0d 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"
> +                  "  --all\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},
> +               {"all", 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)
> +                               with_slow_combinatorial = true;
> +                       break;
>                 case OPT_RUN_SUBTEST:
>                         if (!list_subtests)
>                                 run_single_subtest = strdup(optarg);
> @@ -1629,6 +1637,22 @@ void igt_skip_on_simulation(void)
>                 igt_require(!igt_run_in_simulation());
>  }
>
> +/**
> + * __igt_slow_combinatorial:

If this is intended to be documented and used in tests, then it should
be included in the public API (i.e. without the underscore prefix).


> + *
> + * This is used to skip subtests that should only be included
> + * when the "--all" command line option has been specified.  This version
> + * is intended as a test.
> + *
> + * @slow_test: true if the subtest is part of the slow/combinatorial set

If this is used to test if a slow subtest should be run, shouldn't
slow_test always be true?


> + *
> + * Returns: true if the test should be run, false if the test should be skipped
> + */
> +bool __igt_slow_combinatorial(bool slow_test)
> +{
> +       return !slow_test || with_slow_combinatorial;
> +}
> +
>  /* structured logging */
>
>  /**
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 5ae09653fd55..7b592278bf6c 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -191,6 +191,12 @@ bool __igt_run_subtest(const char *subtest_name);
>  #define igt_subtest_f(f...) \
>         __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
>
> +bool __igt_slow_combinatorial(bool slow_test);
> +


Documentation for igt_subtest_slow_f is needed here. If __slow is
false, this macro just defines a normal subtest, which is
contradictory to its name. Perhaps igt_subtest_with_flags_f (or
similar) would be better and would also allow for future expansion
with other categories.

> +#define igt_subtest_slow_f(__slow, f...) \
> +       if (__igt_slow_combinatorial(__slow)) \
> +       __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
> +
>  const char *igt_subtest_name(void);
>  bool igt_only_list_subtests(void);
>
> @@ -669,6 +675,7 @@ void igt_disable_exit_handler(void);
>
>  /* helpers to automatically reduce test runtime in simulation */
>  bool igt_run_in_simulation(void);
> +
>  /**
>   * SLOW_QUICK:
>   * @slow: value in simulation mode
> diff --git a/tests/gem_concurrent_blit.c b/tests/gem_concurrent_blit.c
> index 1d2d787202df..fe37cc707583 100644
> --- a/tests/gem_concurrent_blit.c
> +++ b/tests/gem_concurrent_blit.c
> @@ -55,7 +55,6 @@ IGT_TEST_DESCRIPTION("Test of pread/pwrite/mmap behavior when writing to active"
>
>  int fd, devid, gen;
>  struct intel_batchbuffer *batch;
> -int all;
>
>  static void
>  nop_release_bo(drm_intel_bo *bo)
> @@ -931,16 +930,14 @@ 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++) {
> +               for (p = __igt_slow_combinatorial(true) ? pipelines : pskip;
> +                    p->prefix; p++) {
>                         igt_fixture {
>                                 batch = buffers_init(&buffers, mode, fd);
>                         }
>
>                         /* try to overwrite the source values */
> -                       igt_subtest_f("%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -949,7 +946,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               p->copy, h->hang);
>                         }
>
> -                       igt_subtest_f("%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -958,7 +955,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               p->copy, h->hang);
>                         }
>
> -                       igt_subtest_f("%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -967,7 +964,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               p->copy, h->hang);
>                         }
>
> -                       igt_subtest_f("%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -977,7 +974,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               p->copy, h->hang);
>                         }
>
> -                       igt_subtest_f("%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -987,7 +984,7 @@ 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) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-intermix-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -996,7 +993,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               do_intermix_rcs,
>                                               p->copy, h->hang);
>                         }
> -                       igt_subtest_f("%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -1005,7 +1002,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               do_intermix_bcs,
>                                               p->copy, h->hang);
>                         }
> -                       igt_subtest_f("%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -1016,7 +1013,7 @@ 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) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-early-read%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -1026,7 +1023,7 @@ 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) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-read-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -1034,7 +1031,7 @@ run_basic_modes(const struct access_mode *mode,
>                                               do_read_read_bcs,
>                                               p->copy, h->hang);
>                         }
> -                       igt_subtest_f("%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 igt_require(rendercopy);
> @@ -1045,7 +1042,7 @@ 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) {
> +                       igt_subtest_slow_f(*h->suffix, "%s-%s-gpu-read-after-write%s%s", mode->name, p->prefix, suffix, h->suffix) {
>                                 h->require();
>                                 p->require();
>                                 buffers_create(&buffers, num_buffers);
> @@ -1064,13 +1061,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 +1078,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 15707b9b9040..86fd7ca08692 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -47,8 +47,7 @@ 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 --all 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 +115,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 +240,6 @@ struct {
>         bool fbc_check_last_action;
>         bool no_edp;
>         bool small_modes;
> -       bool show_hidden;
>         int step;
>         int only_pipes;
>         int shared_fb_x_offset;
> @@ -249,7 +251,6 @@ struct {
>         .fbc_check_last_action = true,
>         .no_edp = false,
>         .small_modes = false,
> -       .show_hidden= false,
>         .step = 0,
>         .only_pipes = PIPE_COUNT,
>         .shared_fb_x_offset = 500,
> @@ -2933,9 +2934,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;
> @@ -2971,7 +2969,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"
>  "  --shared-fb-x offset        Use 'offset' as the X offset for the shared FB\n"
>  "  --shared-fb-y offset        Use 'offset' as the Y offset for the shared FB\n"
> @@ -3068,18 +3065,19 @@ static const char *format_str(enum pixel_format format)
>         for (t.plane = 0; t.plane < PLANE_COUNT; t.plane++) {              \
>         for (t.fbs = 0; t.fbs < FBS_COUNT; t.fbs++) {                      \
>         for (t.method = 0; t.method < IGT_DRAW_METHOD_COUNT; t.method++) { \
> +               t.slow = false;                                            \
>                 if (t.pipes == PIPE_SINGLE && t.screen == SCREEN_SCND)     \
>                         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 && t.feature == FEATURE_NONE)         \
> -                       continue;                                          \
> -               if (!opt.show_hidden && t.fbs == FBS_SHARED &&             \
> +                       t.slow = true;                                     \
> +               if (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 } } } } } }
> @@ -3094,7 +3092,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'},
>                 { "shared-fb-x",              1, 0, 'x'},
>                 { "shared-fb-y",              1, 0, 'y'},
> @@ -3110,8 +3107,9 @@ int main(int argc, char *argv[])
>                 setup_environment();
>
>         for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) {
> -               if (!opt.show_hidden && t.feature == FEATURE_NONE)
> -                       continue;
> +               t.slow = false;
> +               if (t.feature == FEATURE_NONE)
> +                       t.slow = true;
>                 for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) {
>                         t.screen = SCREEN_PRIM;
>                         t.plane = PLANE_PRI;
> @@ -3120,52 +3118,58 @@ int main(int argc, char *argv[])
>                         /* Make sure nothing is using this value. */
>                         t.method = -1;
>
> -                       igt_subtest_f("%s-%s-rte",
> -                                     feature_str(t.feature),
> -                                     pipes_str(t.pipes))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-%s-rte",
> +                                          feature_str(t.feature),
> +                                          pipes_str(t.pipes))
>                                 rte_subtest(&t);
>                 }
>         }
>
>         TEST_MODE_ITER_BEGIN(t)
> -               igt_subtest_f("%s-%s-%s-%s-%s-draw-%s",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             screen_str(t.screen),
> -                             plane_str(t.plane),
> -                             fbs_str(t.fbs),
> -                             igt_draw_get_method_name(t.method))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-%s-%s-draw-%s",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  screen_str(t.screen),
> +                                  plane_str(t.plane),
> +                                  fbs_str(t.fbs),
> +                                  igt_draw_get_method_name(t.method))
>                         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;
> -
> -               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))
> +               if (t.method != IGT_DRAW_BLT)
> +                       t.slow = true;
> +
> +               igt_subtest_slow_f(t.slow,
> +                                  "%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))
>                         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_subtest_slow_f(t.slow,
> +                                  "%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))
>                         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_subtest_slow_f(t.slow,
> +                                  "%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))
>                         flip_subtest(&t, FLIP_MODESET);
>
>         TEST_MODE_ITER_END
> @@ -3177,10 +3181,11 @@ int main(int argc, char *argv[])
>                     (t.feature & FEATURE_FBC) == 0)
>                         continue;
>
> -               igt_subtest_f("%s-%s-%s-fliptrack",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             fbs_str(t.fbs))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-fliptrack",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  fbs_str(t.fbs))
>                         fliptrack_subtest(&t, FLIP_PAGEFLIP);
>         TEST_MODE_ITER_END
>
> @@ -3190,20 +3195,22 @@ int main(int argc, char *argv[])
>                     t.plane == PLANE_PRI)
>                         continue;
>
> -               igt_subtest_f("%s-%s-%s-%s-%s-move",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             screen_str(t.screen),
> -                             plane_str(t.plane),
> -                             fbs_str(t.fbs))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-%s-%s-move",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  screen_str(t.screen),
> +                                  plane_str(t.plane),
> +                                  fbs_str(t.fbs))
>                         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))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%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))
>                         onoff_subtest(&t);
>         TEST_MODE_ITER_END
>
> @@ -3213,27 +3220,30 @@ int main(int argc, char *argv[])
>                     t.plane != PLANE_SPR)
>                         continue;
>
> -               igt_subtest_f("%s-%s-%s-%s-%s-fullscreen",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             screen_str(t.screen),
> -                             plane_str(t.plane),
> -                             fbs_str(t.fbs))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-%s-%s-fullscreen",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  screen_str(t.screen),
> +                                  plane_str(t.plane),
> +                                  fbs_str(t.fbs))
>                         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;
> -
> -               igt_subtest_f("%s-%s-%s-%s-multidraw",
> -                             feature_str(t.feature),
> -                             pipes_str(t.pipes),
> -                             plane_str(t.plane),
> -                             fbs_str(t.fbs))
> +               if (t.plane != PLANE_PRI ||
> +                   t.fbs != FBS_INDIVIDUAL)
> +                       t.slow = true;
> +
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-%s-%s-multidraw",
> +                                  feature_str(t.feature),
> +                                  pipes_str(t.pipes),
> +                                  plane_str(t.plane),
> +                                  fbs_str(t.fbs))
>                         multidraw_subtest(&t);
>         TEST_MODE_ITER_END
>
> @@ -3245,7 +3255,9 @@ int main(int argc, char *argv[])
>                     t.method != IGT_DRAW_MMAP_GTT)
>                         continue;
>
> -               igt_subtest_f("%s-farfromfence", feature_str(t.feature))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-farfromfence",
> +                                  feature_str(t.feature))
>                         farfromfence_subtest(&t);
>         TEST_MODE_ITER_END
>
> @@ -3261,10 +3273,11 @@ int main(int argc, char *argv[])
>                         if (t.format == FORMAT_DEFAULT)
>                                 continue;
>
> -                       igt_subtest_f("%s-%s-draw-%s",
> -                                     feature_str(t.feature),
> -                                     format_str(t.format),
> -                                     igt_draw_get_method_name(t.method))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-%s-draw-%s",
> +                                          feature_str(t.feature),
> +                                          format_str(t.format),
> +                                          igt_draw_get_method_name(t.method))
>                                 format_draw_subtest(&t);
>                 }
>         TEST_MODE_ITER_END
> @@ -3275,9 +3288,10 @@ int main(int argc, char *argv[])
>                     t.plane != PLANE_PRI ||
>                     t.method != IGT_DRAW_MMAP_CPU)
>                         continue;
> -               igt_subtest_f("%s-%s-scaledprimary",
> -                             feature_str(t.feature),
> -                             fbs_str(t.fbs))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-%s-scaledprimary",
> +                                  feature_str(t.feature),
> +                                  fbs_str(t.fbs))
>                         scaledprimary_subtest(&t);
>         TEST_MODE_ITER_END
>
> @@ -3289,22 +3303,32 @@ int main(int argc, char *argv[])
>                     t.method != IGT_DRAW_MMAP_CPU)
>                         continue;
>
> -               igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-modesetfrombusy",
> +                                  feature_str(t.feature))
>                         modesetfrombusy_subtest(&t);
>
>                 if (t.feature & FEATURE_FBC) {
> -                       igt_subtest_f("%s-badstride", feature_str(t.feature))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-badstride",
> +                                          feature_str(t.feature))
>                                 badstride_subtest(&t);
>
> -                       igt_subtest_f("%s-stridechange", feature_str(t.feature))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-stridechange",
> +                                          feature_str(t.feature))
>                                 stridechange_subtest(&t);
>                 }
>
>                 if (t.feature & FEATURE_PSR)
> -                       igt_subtest_f("%s-slowdraw", feature_str(t.feature))
> +                       igt_subtest_slow_f(t.slow,
> +                                          "%s-slowdraw",
> +                                          feature_str(t.feature))
>                                 slow_draw_subtest(&t);
>
> -               igt_subtest_f("%s-suspend", feature_str(t.feature))
> +               igt_subtest_slow_f(t.slow,
> +                                  "%s-suspend",
> +                                  feature_str(t.feature))
>                         suspend_subtest(&t);
>         TEST_MODE_ITER_END
>
> --
> 2.6.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
David Weinehall Oct. 30, 2015, 7:44 a.m. UTC | #3 
On Wed, Oct 28, 2015 at 05:14:28PM +0000, Thomas Wood wrote:
> If this is intended to be documented and used in tests, then it should
> be included in the public API (i.e. without the underscore prefix).

True.  Will fix.

> > + *
> > + * This is used to skip subtests that should only be included
> > + * when the "--all" command line option has been specified.  This version
> > + * is intended as a test.
> > + *
> > + * @slow_test: true if the subtest is part of the slow/combinatorial set
> 
> If this is used to test if a slow subtest should be run, shouldn't
> slow_test always be true?

The test is written such that igt_subtest_slow_f() can always be used
both for fast and slow cases -- the slow flag will decide whether or not
it should bail out (combined with the --all flag, obviously).

So slow_test isn't always true.

> Documentation for igt_subtest_slow_f is needed here. If __slow is
> false, this macro just defines a normal subtest, which is
> contradictory to its name. Perhaps igt_subtest_with_flags_f (or
> similar) would be better and would also allow for future expansion
> with other categories.

Yeah, that could be a workable solution; in discussion with Daniel
earlier we agreed not to do a "flags" implementation, to keep things
simple for now, but it might indeed be better to do things right
from the start.

As we get to the point where we call igt_subtest with several different
flags things will probably get quite complex though; at that point
I suspect that the macro might start looking really hairy...


Kind regards, David
David Weinehall Oct. 30, 2015, 7:56 a.m. UTC | #4 
On Wed, Oct 28, 2015 at 02:12:15PM -0200, Paulo Zanoni wrote:
> 2015-10-28 9:29 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>:
> > 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 in some cases.
> > Until now there's been no unified way of handling this. Remedy
> > this by introducing the --all option to igt_core,
> > and use it in gem_concurrent_blit & kms_frontbuffer_tracking.
> 
> I really think you should explain both your plan and its
> implementation in more details here.

Well, I don't see how much more there is to explain; the idea is simply
that different tests shouldn't implement similar behaviour in different
manners (current kms_frontbuffer_tracking uses a command line option,
gem_concurrent_blit changes behaviour depending on the file name it's
called with).

> >
> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> > ---
> >  lib/igt_core.c                   |  24 +++++
> >  lib/igt_core.h                   |   7 ++
> >  tests/gem_concurrent_blit.c      |  44 ++++-----
> >  tests/kms_frontbuffer_tracking.c | 208 ++++++++++++++++++++++-----------------
> >  4 files changed, 165 insertions(+), 118 deletions(-)
> >
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 59127cafe606..6575b9d6bf0d 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;
> 
> The option is called --all, the new subtest macro is _slow and the
> variables and enums are called with_slow_combinatorial. Is this
> intentional?

The option is called --all because "--with-slow-combinatorial" was
considered to be too much of a mouthful.  The variables & enums are
still retaining these names because they're much more descriptive.

The macro is called _slow because I wanted to keep it a bit shorter,
but I can rename it to _slow_combinatorial if that's preferred.

> 
> >  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"
> > +                  "  --all\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},
> > +               {"all", 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)
> > +                               with_slow_combinatorial = true;
> > +                       break;
> >                 case OPT_RUN_SUBTEST:
> >                         if (!list_subtests)
> >                                 run_single_subtest = strdup(optarg);
> > @@ -1629,6 +1637,22 @@ void igt_skip_on_simulation(void)
> >                 igt_require(!igt_run_in_simulation());
> >  }
> >
> > +/**
> > + * __igt_slow_combinatorial:
> > + *
> > + * This is used to skip subtests that should only be included
> > + * when the "--all" command line option has been specified.  This version
> > + * is intended as a test.
> > + *
> > + * @slow_test: true if the subtest is part of the slow/combinatorial set
> > + *
> > + * Returns: true if the test should be run, false if the test should be skipped
> > + */
> > +bool __igt_slow_combinatorial(bool slow_test)
> > +{
> > +       return !slow_test || with_slow_combinatorial;
> > +}
> > +
> >  /* structured logging */
> >
> >  /**
> > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > index 5ae09653fd55..7b592278bf6c 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -191,6 +191,12 @@ bool __igt_run_subtest(const char *subtest_name);
> >  #define igt_subtest_f(f...) \
> >         __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
> >
> > +bool __igt_slow_combinatorial(bool slow_test);
> > +
> 
> We also need a igt_subtest_slow() version (without "_f") and some
> comments explaining what's the real difference between them and the
> other macros, like the other igt_subtest_* macros.

OK, fair enough.

> > +#define igt_subtest_slow_f(__slow, f...) \
> > +       if (__igt_slow_combinatorial(__slow)) \
> > +       __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
> 
> Missing tab in the line above.

Indeed, thanks for spotting.  Will fix.

> > +
> >  const char *igt_subtest_name(void);
> >  bool igt_only_list_subtests(void);
> >
> > @@ -669,6 +675,7 @@ void igt_disable_exit_handler(void);
> >
> >  /* helpers to automatically reduce test runtime in simulation */
> >  bool igt_run_in_simulation(void);
> > +
> 
> Bad chunk.

Doh, that's a remnant from moving things around.  Will fix.

[snip]

> > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> > index 15707b9b9040..86fd7ca08692 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -47,8 +47,7 @@ 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 --all 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 +115,10 @@ struct test_mode {
> >         } format;
> >
> >         enum igt_draw_method method;
> > +
> > +       /* The test is slow and/or combinatorial;
> > +        * skip unless otherwise specified */
> > +       bool slow;
> 
> My problem with this is that exactly none of the tests marked as
> "slow" are actually slow here... They're either redudant or for debug
> purposes, not slow.

If they're redundant they should be removed (but that should be done by
you or someone else who know that they are indeed redundant), as I
already mentioned.  They definitely are "slow" though, in the sense that
running with them is slower than not running with them (admittedly the
difference isn't comparable to that of gem_concurrent_blit, where a full
run on my test machine took 30x as long...).

If you'd like to categorise them into more categories than just
slow/non-slow (or slow_combinatorial/non-slow_combinatorial), then by
all means, I'll go for Thomas Wood's proposal to use the  _flags
approach instead, but for that you need to provide a patch that actually
categorises them.


Regards, David
Paulo Zanoni Oct. 30, 2015, 11:55 a.m. UTC | #5 
2015-10-30 5:56 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>:
> On Wed, Oct 28, 2015 at 02:12:15PM -0200, Paulo Zanoni wrote:
>> 2015-10-28 9:29 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>:
>> > 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 in some cases.
>> > Until now there's been no unified way of handling this. Remedy
>> > this by introducing the --all option to igt_core,
>> > and use it in gem_concurrent_blit & kms_frontbuffer_tracking.
>>
>> I really think you should explain both your plan and its
>> implementation in more details here.
>
> Well, I don't see how much more there is to explain; the idea is simply
> that different tests shouldn't implement similar behaviour in different
> manners (current kms_frontbuffer_tracking uses a command line option,
> gem_concurrent_blit changes behaviour depending on the file name it's
> called with).

What made me write that is that I noticed that now --all is required
during --list-subtests (thanks for doing this!) but it was not easy to
notice this in the commit message or in the code. So I was thinking
something simple, such as a description of how to use the new option
both when running IGT and when writing subtests:

"These tests will only appear in --list-subtests if you also specify
--all. Same for --run-subtest calls. There's this new macro
igt_subtest_slow_f (maybe igt_subtest_flags_f now?) which should be
used in case the subtest can be slow/combinatorial."

>
>> >
>> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
>> > ---
>> >  lib/igt_core.c                   |  24 +++++
>> >  lib/igt_core.h                   |   7 ++
>> >  tests/gem_concurrent_blit.c      |  44 ++++-----
>> >  tests/kms_frontbuffer_tracking.c | 208 ++++++++++++++++++++++-----------------
>> >  4 files changed, 165 insertions(+), 118 deletions(-)
>> >
>> > diff --git a/lib/igt_core.c b/lib/igt_core.c
>> > index 59127cafe606..6575b9d6bf0d 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;
>>
>> The option is called --all, the new subtest macro is _slow and the
>> variables and enums are called with_slow_combinatorial. Is this
>> intentional?
>
> The option is called --all because "--with-slow-combinatorial" was
> considered to be too much of a mouthful.  The variables & enums are
> still retaining these names because they're much more descriptive.

Ok, let's keep is like this then (in case they don't change with
_flags suggestion).

>
> The macro is called _slow because I wanted to keep it a bit shorter,
> but I can rename it to _slow_combinatorial if that's preferred.

I agree _slow_combinatorial is too big, so let's keep it like this.
Maybe the new version is going to be _flags or something?

>
>>
>> >  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"
>> > +                  "  --all\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},
>> > +               {"all", 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)
>> > +                               with_slow_combinatorial = true;
>> > +                       break;
>> >                 case OPT_RUN_SUBTEST:
>> >                         if (!list_subtests)
>> >                                 run_single_subtest = strdup(optarg);
>> > @@ -1629,6 +1637,22 @@ void igt_skip_on_simulation(void)
>> >                 igt_require(!igt_run_in_simulation());
>> >  }
>> >
>> > +/**
>> > + * __igt_slow_combinatorial:
>> > + *
>> > + * This is used to skip subtests that should only be included
>> > + * when the "--all" command line option has been specified.  This version
>> > + * is intended as a test.
>> > + *
>> > + * @slow_test: true if the subtest is part of the slow/combinatorial set
>> > + *
>> > + * Returns: true if the test should be run, false if the test should be skipped
>> > + */
>> > +bool __igt_slow_combinatorial(bool slow_test)
>> > +{
>> > +       return !slow_test || with_slow_combinatorial;
>> > +}
>> > +
>> >  /* structured logging */
>> >
>> >  /**
>> > diff --git a/lib/igt_core.h b/lib/igt_core.h
>> > index 5ae09653fd55..7b592278bf6c 100644
>> > --- a/lib/igt_core.h
>> > +++ b/lib/igt_core.h
>> > @@ -191,6 +191,12 @@ bool __igt_run_subtest(const char *subtest_name);
>> >  #define igt_subtest_f(f...) \
>> >         __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
>> >
>> > +bool __igt_slow_combinatorial(bool slow_test);
>> > +
>>
>> We also need a igt_subtest_slow() version (without "_f") and some
>> comments explaining what's the real difference between them and the
>> other macros, like the other igt_subtest_* macros.
>
> OK, fair enough.
>
>> > +#define igt_subtest_slow_f(__slow, f...) \
>> > +       if (__igt_slow_combinatorial(__slow)) \
>> > +       __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
>>
>> Missing tab in the line above.
>
> Indeed, thanks for spotting.  Will fix.
>
>> > +
>> >  const char *igt_subtest_name(void);
>> >  bool igt_only_list_subtests(void);
>> >
>> > @@ -669,6 +675,7 @@ void igt_disable_exit_handler(void);
>> >
>> >  /* helpers to automatically reduce test runtime in simulation */
>> >  bool igt_run_in_simulation(void);
>> > +
>>
>> Bad chunk.
>
> Doh, that's a remnant from moving things around.  Will fix.
>
> [snip]
>
>> > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
>> > index 15707b9b9040..86fd7ca08692 100644
>> > --- a/tests/kms_frontbuffer_tracking.c
>> > +++ b/tests/kms_frontbuffer_tracking.c
>> > @@ -47,8 +47,7 @@ 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 --all 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 +115,10 @@ struct test_mode {
>> >         } format;
>> >
>> >         enum igt_draw_method method;
>> > +
>> > +       /* The test is slow and/or combinatorial;
>> > +        * skip unless otherwise specified */
>> > +       bool slow;
>>
>> My problem with this is that exactly none of the tests marked as
>> "slow" are actually slow here... They're either redudant or for debug
>> purposes, not slow.
>
> If they're redundant they should be removed (but that should be done by
> you or someone else who know that they are indeed redundant), as I
> already mentioned.  They definitely are "slow" though, in the sense that
> running with them is slower than not running with them (admittedly the
> difference isn't comparable to that of gem_concurrent_blit, where a full
> run on my test machine took 30x as long...).
>
> If you'd like to categorise them into more categories than just
> slow/non-slow (or slow_combinatorial/non-slow_combinatorial), then by
> all means, I'll go for Thomas Wood's proposal to use the  _flags
> approach instead, but for that you need to provide a patch that actually
> categorises them.

I actually like the _flags idea. It's easy to expand in case we want
to create more categories. I'm not sure if this is actually going to
be done, but I'd support it.

Bonus points if --all could become
--flags=slow,combinatorial,useless,stress,blacklisted,debug,todo,etc.
But let's not block your current patches on these things, we can leave
this for later if you want.

>
>
> Regards, David
Chris Wilson Oct. 30, 2015, 11:59 a.m. UTC | #6 
On Fri, Oct 30, 2015 at 09:55:03AM -0200, Paulo Zanoni wrote:
> 2015-10-30 5:56 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>:
> > On Wed, Oct 28, 2015 at 02:12:15PM -0200, Paulo Zanoni wrote:
> >> 2015-10-28 9:29 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>:
> >> > 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 in some cases.
> >> > Until now there's been no unified way of handling this. Remedy
> >> > this by introducing the --all option to igt_core,
> >> > and use it in gem_concurrent_blit & kms_frontbuffer_tracking.
> >>
> >> I really think you should explain both your plan and its
> >> implementation in more details here.
> >
> > Well, I don't see how much more there is to explain; the idea is simply
> > that different tests shouldn't implement similar behaviour in different
> > manners (current kms_frontbuffer_tracking uses a command line option,
> > gem_concurrent_blit changes behaviour depending on the file name it's
> > called with).
> 
> What made me write that is that I noticed that now --all is required
> during --list-subtests (thanks for doing this!) but it was not easy to
> notice this in the commit message or in the code. So I was thinking
> something simple, such as a description of how to use the new option
> both when running IGT and when writing subtests:
> 
> "These tests will only appear in --list-subtests if you also specify
> --all. Same for --run-subtest calls. There's this new macro
> igt_subtest_slow_f (maybe igt_subtest_flags_f now?) which should be
> used in case the subtest can be slow/combinatorial."
> 
> >
> >> >
> >> > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
> >> > ---
> >> >  lib/igt_core.c                   |  24 +++++
> >> >  lib/igt_core.h                   |   7 ++
> >> >  tests/gem_concurrent_blit.c      |  44 ++++-----
> >> >  tests/kms_frontbuffer_tracking.c | 208 ++++++++++++++++++++++-----------------
> >> >  4 files changed, 165 insertions(+), 118 deletions(-)
> >> >
> >> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> >> > index 59127cafe606..6575b9d6bf0d 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;
> >>
> >> The option is called --all, the new subtest macro is _slow and the
> >> variables and enums are called with_slow_combinatorial. Is this
> >> intentional?
> >
> > The option is called --all because "--with-slow-combinatorial" was
> > considered to be too much of a mouthful.  The variables & enums are
> > still retaining these names because they're much more descriptive.
> 
> Ok, let's keep is like this then (in case they don't change with
> _flags suggestion).
> 
> >
> > The macro is called _slow because I wanted to keep it a bit shorter,
> > but I can rename it to _slow_combinatorial if that's preferred.
> 
> I agree _slow_combinatorial is too big, so let's keep it like this.
> Maybe the new version is going to be _flags or something?

igt_subtest_cond_f().

The subtest is included in the test lists if and only if the conditional
experession is true.

And run with igt_subtest_flags.
-Chris
diff mbox

Patch

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 59127cafe606..6575b9d6bf0d 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"
+		   "  --all\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},
+		{"all", 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)
+				with_slow_combinatorial = true;
+			break;
 		case OPT_RUN_SUBTEST:
 			if (!list_subtests)
 				run_single_subtest = strdup(optarg);
@@ -1629,6 +1637,22 @@  void igt_skip_on_simulation(void)
 		igt_require(!igt_run_in_simulation());
 }
 
+/**
+ * __igt_slow_combinatorial:
+ *
+ * This is used to skip subtests that should only be included
+ * when the "--all" command line option has been specified.  This version
+ * is intended as a test.
+ *
+ * @slow_test: true if the subtest is part of the slow/combinatorial set
+ *
+ * Returns: true if the test should be run, false if the test should be skipped
+ */
+bool __igt_slow_combinatorial(bool slow_test)
+{
+	return !slow_test || with_slow_combinatorial;
+}
+
 /* structured logging */
 
 /**
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 5ae09653fd55..7b592278bf6c 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -191,6 +191,12 @@  bool __igt_run_subtest(const char *subtest_name);
 #define igt_subtest_f(f...) \
 	__igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
 
+bool __igt_slow_combinatorial(bool slow_test);
+
+#define igt_subtest_slow_f(__slow, f...) \
+	if (__igt_slow_combinatorial(__slow)) \
+	__igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
+
 const char *igt_subtest_name(void);
 bool igt_only_list_subtests(void);
 
@@ -669,6 +675,7 @@  void igt_disable_exit_handler(void);
 
 /* helpers to automatically reduce test runtime in simulation */
 bool igt_run_in_simulation(void);
+
 /**
  * SLOW_QUICK:
  * @slow: value in simulation mode
diff --git a/tests/gem_concurrent_blit.c b/tests/gem_concurrent_blit.c
index 1d2d787202df..fe37cc707583 100644
--- a/tests/gem_concurrent_blit.c
+++ b/tests/gem_concurrent_blit.c
@@ -55,7 +55,6 @@  IGT_TEST_DESCRIPTION("Test of pread/pwrite/mmap behavior when writing to active"
 
 int fd, devid, gen;
 struct intel_batchbuffer *batch;
-int all;
 
 static void
 nop_release_bo(drm_intel_bo *bo)
@@ -931,16 +930,14 @@  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++) {
+		for (p = __igt_slow_combinatorial(true) ? pipelines : pskip;
+		     p->prefix; p++) {
 			igt_fixture {
 				batch = buffers_init(&buffers, mode, fd);
 			}
 
 			/* try to overwrite the source values */
-			igt_subtest_f("%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				buffers_create(&buffers, num_buffers);
@@ -949,7 +946,7 @@  run_basic_modes(const struct access_mode *mode,
 					      p->copy, h->hang);
 			}
 
-			igt_subtest_f("%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				buffers_create(&buffers, num_buffers);
@@ -958,7 +955,7 @@  run_basic_modes(const struct access_mode *mode,
 					      p->copy, h->hang);
 			}
 
-			igt_subtest_f("%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				buffers_create(&buffers, num_buffers);
@@ -967,7 +964,7 @@  run_basic_modes(const struct access_mode *mode,
 					      p->copy, h->hang);
 			}
 
-			igt_subtest_f("%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				igt_require(rendercopy);
@@ -977,7 +974,7 @@  run_basic_modes(const struct access_mode *mode,
 					      p->copy, h->hang);
 			}
 
-			igt_subtest_f("%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				buffers_create(&buffers, num_buffers);
@@ -987,7 +984,7 @@  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) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-intermix-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				igt_require(rendercopy);
@@ -996,7 +993,7 @@  run_basic_modes(const struct access_mode *mode,
 					      do_intermix_rcs,
 					      p->copy, h->hang);
 			}
-			igt_subtest_f("%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				igt_require(rendercopy);
@@ -1005,7 +1002,7 @@  run_basic_modes(const struct access_mode *mode,
 					      do_intermix_bcs,
 					      p->copy, h->hang);
 			}
-			igt_subtest_f("%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				igt_require(rendercopy);
@@ -1016,7 +1013,7 @@  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) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-early-read%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				buffers_create(&buffers, num_buffers);
@@ -1026,7 +1023,7 @@  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) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-read-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				buffers_create(&buffers, num_buffers);
@@ -1034,7 +1031,7 @@  run_basic_modes(const struct access_mode *mode,
 					      do_read_read_bcs,
 					      p->copy, h->hang);
 			}
-			igt_subtest_f("%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				igt_require(rendercopy);
@@ -1045,7 +1042,7 @@  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) {
+			igt_subtest_slow_f(*h->suffix, "%s-%s-gpu-read-after-write%s%s", mode->name, p->prefix, suffix, h->suffix) {
 				h->require();
 				p->require();
 				buffers_create(&buffers, num_buffers);
@@ -1064,13 +1061,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 +1078,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 15707b9b9040..86fd7ca08692 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -47,8 +47,7 @@  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 --all 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 +115,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 +240,6 @@  struct {
 	bool fbc_check_last_action;
 	bool no_edp;
 	bool small_modes;
-	bool show_hidden;
 	int step;
 	int only_pipes;
 	int shared_fb_x_offset;
@@ -249,7 +251,6 @@  struct {
 	.fbc_check_last_action = true,
 	.no_edp = false,
 	.small_modes = false,
-	.show_hidden= false,
 	.step = 0,
 	.only_pipes = PIPE_COUNT,
 	.shared_fb_x_offset = 500,
@@ -2933,9 +2934,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;
@@ -2971,7 +2969,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"
 "  --shared-fb-x offset        Use 'offset' as the X offset for the shared FB\n"
 "  --shared-fb-y offset        Use 'offset' as the Y offset for the shared FB\n"
@@ -3068,18 +3065,19 @@  static const char *format_str(enum pixel_format format)
 	for (t.plane = 0; t.plane < PLANE_COUNT; t.plane++) {		   \
 	for (t.fbs = 0; t.fbs < FBS_COUNT; t.fbs++) {			   \
 	for (t.method = 0; t.method < IGT_DRAW_METHOD_COUNT; t.method++) { \
+		t.slow = false;						   \
 		if (t.pipes == PIPE_SINGLE && t.screen == SCREEN_SCND)	   \
 			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 && t.feature == FEATURE_NONE)	   \
-			continue;					   \
-		if (!opt.show_hidden && t.fbs == FBS_SHARED &&		   \
+			t.slow = true;					   \
+		if (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 } } } } } }
@@ -3094,7 +3092,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'},
 		{ "shared-fb-x",              1, 0, 'x'},
 		{ "shared-fb-y",              1, 0, 'y'},
@@ -3110,8 +3107,9 @@  int main(int argc, char *argv[])
 		setup_environment();
 
 	for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) {
-		if (!opt.show_hidden && t.feature == FEATURE_NONE)
-			continue;
+		t.slow = false;
+		if (t.feature == FEATURE_NONE)
+			t.slow = true;
 		for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) {
 			t.screen = SCREEN_PRIM;
 			t.plane = PLANE_PRI;
@@ -3120,52 +3118,58 @@  int main(int argc, char *argv[])
 			/* Make sure nothing is using this value. */
 			t.method = -1;
 
-			igt_subtest_f("%s-%s-rte",
-				      feature_str(t.feature),
-				      pipes_str(t.pipes))
+			igt_subtest_slow_f(t.slow,
+					   "%s-%s-rte",
+					   feature_str(t.feature),
+					   pipes_str(t.pipes))
 				rte_subtest(&t);
 		}
 	}
 
 	TEST_MODE_ITER_BEGIN(t)
-		igt_subtest_f("%s-%s-%s-%s-%s-draw-%s",
-			      feature_str(t.feature),
-			      pipes_str(t.pipes),
-			      screen_str(t.screen),
-			      plane_str(t.plane),
-			      fbs_str(t.fbs),
-			      igt_draw_get_method_name(t.method))
+		igt_subtest_slow_f(t.slow,
+				   "%s-%s-%s-%s-%s-draw-%s",
+				   feature_str(t.feature),
+				   pipes_str(t.pipes),
+				   screen_str(t.screen),
+				   plane_str(t.plane),
+				   fbs_str(t.fbs),
+				   igt_draw_get_method_name(t.method))
 			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;
-
-		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))
+		if (t.method != IGT_DRAW_BLT)
+			t.slow = true;
+
+		igt_subtest_slow_f(t.slow,
+				   "%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))
 			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_subtest_slow_f(t.slow,
+				   "%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))
 			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_subtest_slow_f(t.slow,
+				   "%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))
 			flip_subtest(&t, FLIP_MODESET);
 
 	TEST_MODE_ITER_END
@@ -3177,10 +3181,11 @@  int main(int argc, char *argv[])
 		    (t.feature & FEATURE_FBC) == 0)
 			continue;
 
-		igt_subtest_f("%s-%s-%s-fliptrack",
-			      feature_str(t.feature),
-			      pipes_str(t.pipes),
-			      fbs_str(t.fbs))
+		igt_subtest_slow_f(t.slow,
+				   "%s-%s-%s-fliptrack",
+				   feature_str(t.feature),
+				   pipes_str(t.pipes),
+				   fbs_str(t.fbs))
 			fliptrack_subtest(&t, FLIP_PAGEFLIP);
 	TEST_MODE_ITER_END
 
@@ -3190,20 +3195,22 @@  int main(int argc, char *argv[])
 		    t.plane == PLANE_PRI)
 			continue;
 
-		igt_subtest_f("%s-%s-%s-%s-%s-move",
-			      feature_str(t.feature),
-			      pipes_str(t.pipes),
-			      screen_str(t.screen),
-			      plane_str(t.plane),
-			      fbs_str(t.fbs))
+		igt_subtest_slow_f(t.slow,
+				   "%s-%s-%s-%s-%s-move",
+				   feature_str(t.feature),
+				   pipes_str(t.pipes),
+				   screen_str(t.screen),
+				   plane_str(t.plane),
+				   fbs_str(t.fbs))
 			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))
+		igt_subtest_slow_f(t.slow,
+				   "%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))
 			onoff_subtest(&t);
 	TEST_MODE_ITER_END
 
@@ -3213,27 +3220,30 @@  int main(int argc, char *argv[])
 		    t.plane != PLANE_SPR)
 			continue;
 
-		igt_subtest_f("%s-%s-%s-%s-%s-fullscreen",
-			      feature_str(t.feature),
-			      pipes_str(t.pipes),
-			      screen_str(t.screen),
-			      plane_str(t.plane),
-			      fbs_str(t.fbs))
+		igt_subtest_slow_f(t.slow,
+				   "%s-%s-%s-%s-%s-fullscreen",
+				   feature_str(t.feature),
+				   pipes_str(t.pipes),
+				   screen_str(t.screen),
+				   plane_str(t.plane),
+				   fbs_str(t.fbs))
 			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;
-
-		igt_subtest_f("%s-%s-%s-%s-multidraw",
-			      feature_str(t.feature),
-			      pipes_str(t.pipes),
-			      plane_str(t.plane),
-			      fbs_str(t.fbs))
+		if (t.plane != PLANE_PRI ||
+		    t.fbs != FBS_INDIVIDUAL)
+			t.slow = true;
+
+		igt_subtest_slow_f(t.slow,
+				   "%s-%s-%s-%s-multidraw",
+				   feature_str(t.feature),
+				   pipes_str(t.pipes),
+				   plane_str(t.plane),
+				   fbs_str(t.fbs))
 			multidraw_subtest(&t);
 	TEST_MODE_ITER_END
 
@@ -3245,7 +3255,9 @@  int main(int argc, char *argv[])
 		    t.method != IGT_DRAW_MMAP_GTT)
 			continue;
 
-		igt_subtest_f("%s-farfromfence", feature_str(t.feature))
+		igt_subtest_slow_f(t.slow,
+				   "%s-farfromfence",
+				   feature_str(t.feature))
 			farfromfence_subtest(&t);
 	TEST_MODE_ITER_END
 
@@ -3261,10 +3273,11 @@  int main(int argc, char *argv[])
 			if (t.format == FORMAT_DEFAULT)
 				continue;
 
-			igt_subtest_f("%s-%s-draw-%s",
-				      feature_str(t.feature),
-				      format_str(t.format),
-				      igt_draw_get_method_name(t.method))
+			igt_subtest_slow_f(t.slow,
+					   "%s-%s-draw-%s",
+					   feature_str(t.feature),
+					   format_str(t.format),
+					   igt_draw_get_method_name(t.method))
 				format_draw_subtest(&t);
 		}
 	TEST_MODE_ITER_END
@@ -3275,9 +3288,10 @@  int main(int argc, char *argv[])
 		    t.plane != PLANE_PRI ||
 		    t.method != IGT_DRAW_MMAP_CPU)
 			continue;
-		igt_subtest_f("%s-%s-scaledprimary",
-			      feature_str(t.feature),
-			      fbs_str(t.fbs))
+		igt_subtest_slow_f(t.slow,
+				   "%s-%s-scaledprimary",
+				   feature_str(t.feature),
+				   fbs_str(t.fbs))
 			scaledprimary_subtest(&t);
 	TEST_MODE_ITER_END
 
@@ -3289,22 +3303,32 @@  int main(int argc, char *argv[])
 		    t.method != IGT_DRAW_MMAP_CPU)
 			continue;
 
-		igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature))
+		igt_subtest_slow_f(t.slow,
+				   "%s-modesetfrombusy",
+				   feature_str(t.feature))
 			modesetfrombusy_subtest(&t);
 
 		if (t.feature & FEATURE_FBC) {
-			igt_subtest_f("%s-badstride", feature_str(t.feature))
+			igt_subtest_slow_f(t.slow,
+					   "%s-badstride",
+					   feature_str(t.feature))
 				badstride_subtest(&t);
 
-			igt_subtest_f("%s-stridechange", feature_str(t.feature))
+			igt_subtest_slow_f(t.slow,
+					   "%s-stridechange",
+					   feature_str(t.feature))
 				stridechange_subtest(&t);
 		}
 
 		if (t.feature & FEATURE_PSR)
-			igt_subtest_f("%s-slowdraw", feature_str(t.feature))
+			igt_subtest_slow_f(t.slow,
+					   "%s-slowdraw",
+					   feature_str(t.feature))
 				slow_draw_subtest(&t);
 
-		igt_subtest_f("%s-suspend", feature_str(t.feature))
+		igt_subtest_slow_f(t.slow,
+				   "%s-suspend",
+				   feature_str(t.feature))
 			suspend_subtest(&t);
 	TEST_MODE_ITER_END