| Message ID | 1445600556-14884-3-git-send-email-david.weinehall@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 23, 2015 at 02:42:35PM +0300, David Weinehall wrote: > Some tests should not be run by default, due to their slow, > and sometimes superfluous, nature. > > We still want to be able to run these tests though in some cases. > Until now there's been no unified way of handling this. Remedy > this by introducing the --with-slow-combinatorial option to > igt_core, and use it in gem_concurrent_blit & kms_frontbuffer_tracking. > --- > diff --git a/tests/gem_concurrent_blit.c b/tests/gem_concurrent_blit.c > index 1d2d787202df..311b6829e984 100644 > --- a/tests/gem_concurrent_blit.c > +++ b/tests/gem_concurrent_blit.c > @@ -931,9 +931,6 @@ run_basic_modes(const struct access_mode *mode, > struct buffers buffers; > > for (h = hangs; h->suffix; h++) { > - if (!all && *h->suffix) > - continue; > - > for (p = all ? pipelines : pskip; p->prefix; p++) { > igt_fixture { > batch = buffers_init(&buffers, mode, fd); You didn't update this to skip the first few CPU vs CPU loops. Perhaps if you just kill the all variable that would help. -Chris
2015-10-23 9:42 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 though in some cases. > Until now there's been no unified way of handling this. Remedy > this by introducing the --with-slow-combinatorial option to > igt_core, and use it in gem_concurrent_blit & kms_frontbuffer_tracking. > --- > lib/igt_core.c | 19 ++++++ > lib/igt_core.h | 1 + > tests/gem_concurrent_blit.c | 40 ++++++++---- > tests/kms_frontbuffer_tracking.c | 135 +++++++++++++++++++++++++++------------ > 4 files changed, 142 insertions(+), 53 deletions(-) > > diff --git a/lib/igt_core.c b/lib/igt_core.c > index 59127cafe606..ba40ce0e0ead 100644 > --- a/lib/igt_core.c > +++ b/lib/igt_core.c > @@ -216,6 +216,7 @@ const char *igt_interactive_debug; > > /* subtests helpers */ > static bool list_subtests = false; > +static bool with_slow_combinatorial = false; > static char *run_single_subtest = NULL; > static bool run_single_subtest_found = false; > static const char *in_subtest = NULL; > @@ -235,6 +236,7 @@ bool test_child; > > enum { > OPT_LIST_SUBTESTS, > + OPT_WITH_SLOW_COMBINATORIAL, > OPT_RUN_SUBTEST, > OPT_DESCRIPTION, > OPT_DEBUG, > @@ -478,6 +480,7 @@ static void print_usage(const char *help_str, bool output_on_stderr) > > fprintf(f, "Usage: %s [OPTIONS]\n", command_str); > fprintf(f, " --list-subtests\n" > + " --with-slow-combinatorial\n" > " --run-subtest <pattern>\n" > " --debug[=log-domain]\n" > " --interactive-debug[=domain]\n" > @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv, > int c, option_index = 0, i, x; > static struct option long_options[] = { > {"list-subtests", 0, 0, OPT_LIST_SUBTESTS}, > + {"with-slow-combinatorial", 0, 0, OPT_WITH_SLOW_COMBINATORIAL}, > {"run-subtest", 1, 0, OPT_RUN_SUBTEST}, > {"help-description", 0, 0, OPT_DESCRIPTION}, > {"debug", optional_argument, 0, OPT_DEBUG}, > @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv, > if (!run_single_subtest) > list_subtests = true; > break; > + case OPT_WITH_SLOW_COMBINATORIAL: > + if (!run_single_subtest) > + with_slow_combinatorial = true; > + break; > case OPT_RUN_SUBTEST: > if (!list_subtests) > run_single_subtest = strdup(optarg); > @@ -1629,6 +1637,17 @@ void igt_skip_on_simulation(void) > igt_require(!igt_run_in_simulation()); > } > > +/** > + * igt_slow_combinatorial: > + * > + * This is used to define subtests that should only be listed/run > + * when the "--with-slow-combinatorial" has been specified > + */ > +void igt_slow_combinatorial(void) > +{ > + igt_skip_on(!with_slow_combinatorial); > +} > + > /* structured logging */ > > /** > diff --git a/lib/igt_core.h b/lib/igt_core.h > index 5ae09653fd55..6ddf25563275 100644 > --- a/lib/igt_core.h > +++ b/lib/igt_core.h > @@ -680,6 +680,7 @@ bool igt_run_in_simulation(void); > #define SLOW_QUICK(slow,quick) (igt_run_in_simulation() ? (quick) : (slow)) > > void igt_skip_on_simulation(void); > +void igt_slow_combinatorial(void); > > extern const char *igt_interactive_debug; > > diff --git a/tests/gem_concurrent_blit.c b/tests/gem_concurrent_blit.c > index 1d2d787202df..311b6829e984 100644 > --- a/tests/gem_concurrent_blit.c > +++ b/tests/gem_concurrent_blit.c > @@ -931,9 +931,6 @@ run_basic_modes(const struct access_mode *mode, > struct buffers buffers; > > for (h = hangs; h->suffix; h++) { > - if (!all && *h->suffix) > - continue; > - > for (p = all ? pipelines : pskip; p->prefix; p++) { > igt_fixture { > batch = buffers_init(&buffers, mode, fd); > @@ -941,6 +938,8 @@ run_basic_modes(const struct access_mode *mode, > > /* try to overwrite the source values */ > igt_subtest_f("%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -950,6 +949,8 @@ run_basic_modes(const struct access_mode *mode, > } > > igt_subtest_f("%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -959,6 +960,8 @@ run_basic_modes(const struct access_mode *mode, > } > > igt_subtest_f("%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -968,6 +971,8 @@ run_basic_modes(const struct access_mode *mode, > } > > igt_subtest_f("%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -978,6 +983,8 @@ run_basic_modes(const struct access_mode *mode, > } > > igt_subtest_f("%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -988,6 +995,8 @@ run_basic_modes(const struct access_mode *mode, > > /* try to intermix copies with GPU copies*/ > igt_subtest_f("%s-%s-intermix-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -997,6 +1006,8 @@ run_basic_modes(const struct access_mode *mode, > p->copy, h->hang); > } > igt_subtest_f("%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -1006,6 +1017,8 @@ run_basic_modes(const struct access_mode *mode, > p->copy, h->hang); > } > igt_subtest_f("%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -1017,6 +1030,8 @@ run_basic_modes(const struct access_mode *mode, > > /* try to read the results before the copy completes */ > igt_subtest_f("%s-%s-early-read%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -1027,6 +1042,8 @@ run_basic_modes(const struct access_mode *mode, > > /* concurrent reads */ > igt_subtest_f("%s-%s-read-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -1035,6 +1052,8 @@ run_basic_modes(const struct access_mode *mode, > p->copy, h->hang); > } > igt_subtest_f("%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -1046,6 +1065,8 @@ run_basic_modes(const struct access_mode *mode, > > /* and finally try to trick the kernel into loosing the pending write */ > igt_subtest_f("%s-%s-gpu-read-after-write%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -1064,13 +1085,11 @@ run_basic_modes(const struct access_mode *mode, > static void > run_modes(const struct access_mode *mode) > { > - if (all) { > - run_basic_modes(mode, "", run_single); > + run_basic_modes(mode, "", run_single); > > - igt_fork_signal_helper(); > - run_basic_modes(mode, "-interruptible", run_interruptible); > - igt_stop_signal_helper(); > - } > + igt_fork_signal_helper(); > + run_basic_modes(mode, "-interruptible", run_interruptible); > + igt_stop_signal_helper(); > > igt_fork_signal_helper(); > run_basic_modes(mode, "-forked", run_forked); > @@ -1083,9 +1102,6 @@ igt_main > > igt_skip_on_simulation(); > > - if (strstr(igt_test_name(), "all")) > - all = true; > - > igt_fixture { > fd = drm_open_driver(DRIVER_INTEL); > devid = intel_get_drm_devid(fd); > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c > index d97e148c5073..6f84ef0813d9 100644 > --- a/tests/kms_frontbuffer_tracking.c > +++ b/tests/kms_frontbuffer_tracking.c > @@ -47,8 +47,8 @@ IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and " > * combinations that are somewhat redundant and don't add much value to the > * test. For example, since we already do the offscreen testing with a single > * pipe enabled, there's no much value in doing it again with dual pipes. If you > - * still want to try these redundant tests, you need to use the --show-hidden > - * option. > + * still want to try these redundant tests, you need to use the > + * "--with-slow-combinatorial" option. > * > * The most important hidden thing is the FEATURE_NONE set of tests. Whenever > * you get a failure on any test, it is important to check whether the same test > @@ -116,6 +116,10 @@ struct test_mode { > } format; > > enum igt_draw_method method; > + > + /* The test is slow and/or combinatorial; > + * skip unless otherwise specified */ > + bool slow; > }; > > enum flip_type { > @@ -237,7 +241,6 @@ struct { > bool fbc_check_last_action; > bool no_edp; > bool small_modes; > - bool show_hidden; It's not clear to me, please clarify: now the tests that were previously completely hidden will be listed in --list-subtests and will be shown as skipped during normal runs? For kms_frontbuffer_tracking, hidden tests are supposed to be just for developers who know what they are doing. I hide them behind a special command-line switch that's not used by QA because I don't want QA wasting time running those tests. One third of the kms_frontbuffer_tracking hidden tests only serve the purpose of checking whether there's a bug in kms_frontbuffer_track itself or not. For some other hidden tests, they are there just to help better debug in case some other non-hidden tests fail. Some other hidden tests are 100% useless and superfulous.QA should only run the non-hidden tests. So if some non-hidden test fails, the developers can use the hidden tests to help debugging. Besides, the "if (t.slow)" could have been moved to check_test_requirements(), making the code much simpler :) > int step; > int only_feature; > int only_pipes; > @@ -250,7 +253,6 @@ struct { > .fbc_check_last_action = true, > .no_edp = false, > .small_modes = false, > - .show_hidden= false, > .step = 0, > .only_feature = FEATURE_COUNT, > .only_pipes = PIPE_COUNT, > @@ -2892,9 +2894,6 @@ static int opt_handler(int option, int option_index, void *data) > case 'm': > opt.small_modes = true; > break; > - case 'i': > - opt.show_hidden = true; > - break; > case 't': > opt.step++; > break; > @@ -2942,7 +2941,6 @@ const char *help_str = > " --no-fbc-action-check Don't check for the FBC last action\n" > " --no-edp Don't use eDP monitors\n" > " --use-small-modes Use smaller resolutions for the modes\n" > -" --show-hidden Show hidden subtests\n" > " --step Stop on each step so you can check the screen\n" > " --nop-only Only run the \"nop\" feature subtests\n" > " --fbc-only Only run the \"fbc\" feature subtests\n" > @@ -3036,6 +3034,7 @@ static const char *format_str(enum pixel_format format) > > #define TEST_MODE_ITER_BEGIN(t) \ > t.format = FORMAT_DEFAULT; \ > + t.slow = false; \ > for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) { \ > for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) { \ > for (t.screen = 0; t.screen < SCREEN_COUNT; t.screen++) { \ > @@ -3046,15 +3045,15 @@ static const char *format_str(enum pixel_format format) > continue; \ > if (t.screen == SCREEN_OFFSCREEN && t.plane != PLANE_PRI) \ > continue; \ > - if (!opt.show_hidden && t.pipes == PIPE_DUAL && \ > + if (t.pipes == PIPE_DUAL && \ > t.screen == SCREEN_OFFSCREEN) \ > - continue; \ > - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE) \ > - && t.feature == FEATURE_NONE) \ > - continue; \ > - if (!opt.show_hidden && t.fbs == FBS_SHARED && \ > + t.slow = true; \ > + if (opt.only_feature != FEATURE_NONE && \ > + t.feature == FEATURE_NONE) \ > + t.slow = true; \ > + if (t.fbs == FBS_SHARED && \ > (t.plane == PLANE_CUR || t.plane == PLANE_SPR)) \ > - continue; > + t.slow = true; > > > #define TEST_MODE_ITER_END } } } } } } > @@ -3069,7 +3068,6 @@ int main(int argc, char *argv[]) > { "no-fbc-action-check", 0, 0, 'a'}, > { "no-edp", 0, 0, 'e'}, > { "use-small-modes", 0, 0, 'm'}, > - { "show-hidden", 0, 0, 'i'}, > { "step", 0, 0, 't'}, > { "nop-only", 0, 0, 'n'}, > { "fbc-only", 0, 0, 'f'}, > @@ -3088,9 +3086,11 @@ int main(int argc, char *argv[]) > setup_environment(); > > for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) { > - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE) > - && t.feature == FEATURE_NONE) > - continue; > + bool slow = false; > + > + if (opt.only_feature != FEATURE_NONE && > + t.feature == FEATURE_NONE) > + slow = true; > for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) { > t.screen = SCREEN_PRIM; > t.plane = PLANE_PRI; > @@ -3101,8 +3101,11 @@ int main(int argc, char *argv[]) > > igt_subtest_f("%s-%s-rte", > feature_str(t.feature), > - pipes_str(t.pipes)) > + pipes_str(t.pipes)) { > + if (slow) > + igt_slow_combinatorial(); > rte_subtest(&t); > + } > } > } > > @@ -3113,39 +3116,52 @@ int main(int argc, char *argv[]) > screen_str(t.screen), > plane_str(t.plane), > fbs_str(t.fbs), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > draw_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > if (t.plane != PLANE_PRI || > - t.screen == SCREEN_OFFSCREEN || > - (!opt.show_hidden && t.method != IGT_DRAW_BLT)) > + t.screen == SCREEN_OFFSCREEN) > continue; > + if (t.method != IGT_DRAW_BLT) > + t.slow = true; > > igt_subtest_f("%s-%s-%s-%s-flip-%s", > feature_str(t.feature), > pipes_str(t.pipes), > screen_str(t.screen), > fbs_str(t.fbs), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > flip_subtest(&t, FLIP_PAGEFLIP); > + } > > igt_subtest_f("%s-%s-%s-%s-evflip-%s", > feature_str(t.feature), > pipes_str(t.pipes), > screen_str(t.screen), > fbs_str(t.fbs), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > flip_subtest(&t, FLIP_PAGEFLIP_EVENT); > + } > > igt_subtest_f("%s-%s-%s-%s-msflip-%s", > feature_str(t.feature), > pipes_str(t.pipes), > screen_str(t.screen), > fbs_str(t.fbs), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > flip_subtest(&t, FLIP_MODESET); > + } > > TEST_MODE_ITER_END > > @@ -3159,8 +3175,11 @@ int main(int argc, char *argv[]) > igt_subtest_f("%s-%s-%s-fliptrack", > feature_str(t.feature), > pipes_str(t.pipes), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > fliptrack_subtest(&t, FLIP_PAGEFLIP); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3174,16 +3193,22 @@ int main(int argc, char *argv[]) > pipes_str(t.pipes), > screen_str(t.screen), > plane_str(t.plane), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > move_subtest(&t); > + } > > igt_subtest_f("%s-%s-%s-%s-%s-onoff", > feature_str(t.feature), > pipes_str(t.pipes), > screen_str(t.screen), > plane_str(t.plane), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > onoff_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3197,23 +3222,30 @@ int main(int argc, char *argv[]) > pipes_str(t.pipes), > screen_str(t.screen), > plane_str(t.plane), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > fullscreen_plane_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > if (t.screen != SCREEN_PRIM || > - t.method != IGT_DRAW_BLT || > - (!opt.show_hidden && t.plane != PLANE_PRI) || > - (!opt.show_hidden && t.fbs != FBS_INDIVIDUAL)) > + t.method != IGT_DRAW_BLT) > continue; > + if (t.plane != PLANE_PRI || > + t.fbs != FBS_INDIVIDUAL) > + t.slow = true; > > igt_subtest_f("%s-%s-%s-%s-multidraw", > feature_str(t.feature), > pipes_str(t.pipes), > plane_str(t.plane), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > multidraw_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3224,8 +3256,11 @@ int main(int argc, char *argv[]) > t.method != IGT_DRAW_MMAP_GTT) > continue; > > - igt_subtest_f("%s-farfromfence", feature_str(t.feature)) > + igt_subtest_f("%s-farfromfence", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > farfromfence_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3243,8 +3278,11 @@ int main(int argc, char *argv[]) > igt_subtest_f("%s-%s-draw-%s", > feature_str(t.feature), > format_str(t.format), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > format_draw_subtest(&t); > + } > } > TEST_MODE_ITER_END > > @@ -3256,8 +3294,11 @@ int main(int argc, char *argv[]) > continue; > igt_subtest_f("%s-%s-scaledprimary", > feature_str(t.feature), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > scaledprimary_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3268,19 +3309,31 @@ int main(int argc, char *argv[]) > t.method != IGT_DRAW_MMAP_CPU) > continue; > > - igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature)) > + igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > modesetfrombusy_subtest(&t); > + } > > if (t.feature & FEATURE_FBC) > - igt_subtest_f("%s-badstride", feature_str(t.feature)) > + igt_subtest_f("%s-badstride", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > badstride_subtest(&t); > + } > > if (t.feature & FEATURE_PSR) > - igt_subtest_f("%s-slowdraw", feature_str(t.feature)) > + igt_subtest_f("%s-slowdraw", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > slow_draw_subtest(&t); > + } > > - igt_subtest_f("%s-suspend", feature_str(t.feature)) > + igt_subtest_f("%s-suspend", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > suspend_subtest(&t); > + } > TEST_MODE_ITER_END > > igt_fixture > -- > 2.6.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 23 October 2015 at 12:42, David Weinehall <david.weinehall@linux.intel.com> wrote: > Some tests should not be run by default, due to their slow, > and sometimes superfluous, nature. > > We still want to be able to run these tests though in some cases. > Until now there's been no unified way of handling this. Remedy > this by introducing the --with-slow-combinatorial option to > igt_core, and use it in gem_concurrent_blit & kms_frontbuffer_tracking. > --- > lib/igt_core.c | 19 ++++++ > lib/igt_core.h | 1 + > tests/gem_concurrent_blit.c | 40 ++++++++---- > tests/kms_frontbuffer_tracking.c | 135 +++++++++++++++++++++++++++------------ > 4 files changed, 142 insertions(+), 53 deletions(-) > > diff --git a/lib/igt_core.c b/lib/igt_core.c > index 59127cafe606..ba40ce0e0ead 100644 > --- a/lib/igt_core.c > +++ b/lib/igt_core.c > @@ -216,6 +216,7 @@ const char *igt_interactive_debug; > > /* subtests helpers */ > static bool list_subtests = false; > +static bool with_slow_combinatorial = false; > static char *run_single_subtest = NULL; > static bool run_single_subtest_found = false; > static const char *in_subtest = NULL; > @@ -235,6 +236,7 @@ bool test_child; > > enum { > OPT_LIST_SUBTESTS, > + OPT_WITH_SLOW_COMBINATORIAL, > OPT_RUN_SUBTEST, > OPT_DESCRIPTION, > OPT_DEBUG, > @@ -478,6 +480,7 @@ static void print_usage(const char *help_str, bool output_on_stderr) > > fprintf(f, "Usage: %s [OPTIONS]\n", command_str); > fprintf(f, " --list-subtests\n" > + " --with-slow-combinatorial\n" > " --run-subtest <pattern>\n" > " --debug[=log-domain]\n" > " --interactive-debug[=domain]\n" > @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv, > int c, option_index = 0, i, x; > static struct option long_options[] = { > {"list-subtests", 0, 0, OPT_LIST_SUBTESTS}, > + {"with-slow-combinatorial", 0, 0, OPT_WITH_SLOW_COMBINATORIAL}, > {"run-subtest", 1, 0, OPT_RUN_SUBTEST}, > {"help-description", 0, 0, OPT_DESCRIPTION}, > {"debug", optional_argument, 0, OPT_DEBUG}, > @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv, > if (!run_single_subtest) > list_subtests = true; > break; > + case OPT_WITH_SLOW_COMBINATORIAL: > + if (!run_single_subtest) This will cause piglit (and therefore QA) to unconditionally run all tests marked as slow, since it runs subtests individually. > + with_slow_combinatorial = true; > + break; > case OPT_RUN_SUBTEST: > if (!list_subtests) > run_single_subtest = strdup(optarg); > @@ -1629,6 +1637,17 @@ void igt_skip_on_simulation(void) > igt_require(!igt_run_in_simulation()); > } > > +/** > + * igt_slow_combinatorial: > + * > + * This is used to define subtests that should only be listed/run > + * when the "--with-slow-combinatorial" has been specified This isn't quite correct, as the subtests that use igt_slow_combinatorial will still always be listed. > + */ > +void igt_slow_combinatorial(void) > +{ > + igt_skip_on(!with_slow_combinatorial); Although it is convenient to just skip the tests when the --with-slow-combinatorial flag is passed, it may be useful to be able to classify the subtests before they are run, so that they are filtered out from the test list entirely. An approach that can do this might also be used to mark tests as being part of the basic acceptance tests, so that they can be marked as such without relying on the naming convention. > +} > + > /* structured logging */ > > /** > diff --git a/lib/igt_core.h b/lib/igt_core.h > index 5ae09653fd55..6ddf25563275 100644 > --- a/lib/igt_core.h > +++ b/lib/igt_core.h > @@ -680,6 +680,7 @@ bool igt_run_in_simulation(void); > #define SLOW_QUICK(slow,quick) (igt_run_in_simulation() ? (quick) : (slow)) > > void igt_skip_on_simulation(void); > +void igt_slow_combinatorial(void); > > extern const char *igt_interactive_debug; > > diff --git a/tests/gem_concurrent_blit.c b/tests/gem_concurrent_blit.c > index 1d2d787202df..311b6829e984 100644 > --- a/tests/gem_concurrent_blit.c > +++ b/tests/gem_concurrent_blit.c > @@ -931,9 +931,6 @@ run_basic_modes(const struct access_mode *mode, > struct buffers buffers; > > for (h = hangs; h->suffix; h++) { > - if (!all && *h->suffix) > - continue; > - > for (p = all ? pipelines : pskip; p->prefix; p++) { > igt_fixture { > batch = buffers_init(&buffers, mode, fd); > @@ -941,6 +938,8 @@ run_basic_modes(const struct access_mode *mode, > > /* try to overwrite the source values */ > igt_subtest_f("%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -950,6 +949,8 @@ run_basic_modes(const struct access_mode *mode, > } > > igt_subtest_f("%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -959,6 +960,8 @@ run_basic_modes(const struct access_mode *mode, > } > > igt_subtest_f("%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -968,6 +971,8 @@ run_basic_modes(const struct access_mode *mode, > } > > igt_subtest_f("%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -978,6 +983,8 @@ run_basic_modes(const struct access_mode *mode, > } > > igt_subtest_f("%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -988,6 +995,8 @@ run_basic_modes(const struct access_mode *mode, > > /* try to intermix copies with GPU copies*/ > igt_subtest_f("%s-%s-intermix-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -997,6 +1006,8 @@ run_basic_modes(const struct access_mode *mode, > p->copy, h->hang); > } > igt_subtest_f("%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -1006,6 +1017,8 @@ run_basic_modes(const struct access_mode *mode, > p->copy, h->hang); > } > igt_subtest_f("%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -1017,6 +1030,8 @@ run_basic_modes(const struct access_mode *mode, > > /* try to read the results before the copy completes */ > igt_subtest_f("%s-%s-early-read%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -1027,6 +1042,8 @@ run_basic_modes(const struct access_mode *mode, > > /* concurrent reads */ > igt_subtest_f("%s-%s-read-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -1035,6 +1052,8 @@ run_basic_modes(const struct access_mode *mode, > p->copy, h->hang); > } > igt_subtest_f("%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > igt_require(rendercopy); > @@ -1046,6 +1065,8 @@ run_basic_modes(const struct access_mode *mode, > > /* and finally try to trick the kernel into loosing the pending write */ > igt_subtest_f("%s-%s-gpu-read-after-write%s%s", mode->name, p->prefix, suffix, h->suffix) { > + if (*h->suffix) > + igt_slow_combinatorial(); > h->require(); > p->require(); > buffers_create(&buffers, num_buffers); > @@ -1064,13 +1085,11 @@ run_basic_modes(const struct access_mode *mode, > static void > run_modes(const struct access_mode *mode) > { > - if (all) { > - run_basic_modes(mode, "", run_single); > + run_basic_modes(mode, "", run_single); > > - igt_fork_signal_helper(); > - run_basic_modes(mode, "-interruptible", run_interruptible); > - igt_stop_signal_helper(); > - } > + igt_fork_signal_helper(); > + run_basic_modes(mode, "-interruptible", run_interruptible); > + igt_stop_signal_helper(); > > igt_fork_signal_helper(); > run_basic_modes(mode, "-forked", run_forked); > @@ -1083,9 +1102,6 @@ igt_main > > igt_skip_on_simulation(); > > - if (strstr(igt_test_name(), "all")) > - all = true; > - > igt_fixture { > fd = drm_open_driver(DRIVER_INTEL); > devid = intel_get_drm_devid(fd); > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c > index d97e148c5073..6f84ef0813d9 100644 > --- a/tests/kms_frontbuffer_tracking.c > +++ b/tests/kms_frontbuffer_tracking.c > @@ -47,8 +47,8 @@ IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and " > * combinations that are somewhat redundant and don't add much value to the > * test. For example, since we already do the offscreen testing with a single > * pipe enabled, there's no much value in doing it again with dual pipes. If you > - * still want to try these redundant tests, you need to use the --show-hidden > - * option. > + * still want to try these redundant tests, you need to use the > + * "--with-slow-combinatorial" option. > * > * The most important hidden thing is the FEATURE_NONE set of tests. Whenever > * you get a failure on any test, it is important to check whether the same test > @@ -116,6 +116,10 @@ struct test_mode { > } format; > > enum igt_draw_method method; > + > + /* The test is slow and/or combinatorial; > + * skip unless otherwise specified */ > + bool slow; > }; > > enum flip_type { > @@ -237,7 +241,6 @@ struct { > bool fbc_check_last_action; > bool no_edp; > bool small_modes; > - bool show_hidden; > int step; > int only_feature; > int only_pipes; > @@ -250,7 +253,6 @@ struct { > .fbc_check_last_action = true, > .no_edp = false, > .small_modes = false, > - .show_hidden= false, > .step = 0, > .only_feature = FEATURE_COUNT, > .only_pipes = PIPE_COUNT, > @@ -2892,9 +2894,6 @@ static int opt_handler(int option, int option_index, void *data) > case 'm': > opt.small_modes = true; > break; > - case 'i': > - opt.show_hidden = true; > - break; > case 't': > opt.step++; > break; > @@ -2942,7 +2941,6 @@ const char *help_str = > " --no-fbc-action-check Don't check for the FBC last action\n" > " --no-edp Don't use eDP monitors\n" > " --use-small-modes Use smaller resolutions for the modes\n" > -" --show-hidden Show hidden subtests\n" > " --step Stop on each step so you can check the screen\n" > " --nop-only Only run the \"nop\" feature subtests\n" > " --fbc-only Only run the \"fbc\" feature subtests\n" > @@ -3036,6 +3034,7 @@ static const char *format_str(enum pixel_format format) > > #define TEST_MODE_ITER_BEGIN(t) \ > t.format = FORMAT_DEFAULT; \ > + t.slow = false; \ > for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) { \ > for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) { \ > for (t.screen = 0; t.screen < SCREEN_COUNT; t.screen++) { \ > @@ -3046,15 +3045,15 @@ static const char *format_str(enum pixel_format format) > continue; \ > if (t.screen == SCREEN_OFFSCREEN && t.plane != PLANE_PRI) \ > continue; \ > - if (!opt.show_hidden && t.pipes == PIPE_DUAL && \ > + if (t.pipes == PIPE_DUAL && \ > t.screen == SCREEN_OFFSCREEN) \ > - continue; \ > - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE) \ > - && t.feature == FEATURE_NONE) \ > - continue; \ > - if (!opt.show_hidden && t.fbs == FBS_SHARED && \ > + t.slow = true; \ > + if (opt.only_feature != FEATURE_NONE && \ > + t.feature == FEATURE_NONE) \ > + t.slow = true; \ > + if (t.fbs == FBS_SHARED && \ > (t.plane == PLANE_CUR || t.plane == PLANE_SPR)) \ > - continue; > + t.slow = true; > > > #define TEST_MODE_ITER_END } } } } } } > @@ -3069,7 +3068,6 @@ int main(int argc, char *argv[]) > { "no-fbc-action-check", 0, 0, 'a'}, > { "no-edp", 0, 0, 'e'}, > { "use-small-modes", 0, 0, 'm'}, > - { "show-hidden", 0, 0, 'i'}, > { "step", 0, 0, 't'}, > { "nop-only", 0, 0, 'n'}, > { "fbc-only", 0, 0, 'f'}, > @@ -3088,9 +3086,11 @@ int main(int argc, char *argv[]) > setup_environment(); > > for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) { > - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE) > - && t.feature == FEATURE_NONE) > - continue; > + bool slow = false; > + > + if (opt.only_feature != FEATURE_NONE && > + t.feature == FEATURE_NONE) > + slow = true; > for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) { > t.screen = SCREEN_PRIM; > t.plane = PLANE_PRI; > @@ -3101,8 +3101,11 @@ int main(int argc, char *argv[]) > > igt_subtest_f("%s-%s-rte", > feature_str(t.feature), > - pipes_str(t.pipes)) > + pipes_str(t.pipes)) { > + if (slow) > + igt_slow_combinatorial(); > rte_subtest(&t); > + } > } > } > > @@ -3113,39 +3116,52 @@ int main(int argc, char *argv[]) > screen_str(t.screen), > plane_str(t.plane), > fbs_str(t.fbs), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > draw_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > if (t.plane != PLANE_PRI || > - t.screen == SCREEN_OFFSCREEN || > - (!opt.show_hidden && t.method != IGT_DRAW_BLT)) > + t.screen == SCREEN_OFFSCREEN) > continue; > + if (t.method != IGT_DRAW_BLT) > + t.slow = true; > > igt_subtest_f("%s-%s-%s-%s-flip-%s", > feature_str(t.feature), > pipes_str(t.pipes), > screen_str(t.screen), > fbs_str(t.fbs), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > flip_subtest(&t, FLIP_PAGEFLIP); > + } > > igt_subtest_f("%s-%s-%s-%s-evflip-%s", > feature_str(t.feature), > pipes_str(t.pipes), > screen_str(t.screen), > fbs_str(t.fbs), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > flip_subtest(&t, FLIP_PAGEFLIP_EVENT); > + } > > igt_subtest_f("%s-%s-%s-%s-msflip-%s", > feature_str(t.feature), > pipes_str(t.pipes), > screen_str(t.screen), > fbs_str(t.fbs), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > flip_subtest(&t, FLIP_MODESET); > + } > > TEST_MODE_ITER_END > > @@ -3159,8 +3175,11 @@ int main(int argc, char *argv[]) > igt_subtest_f("%s-%s-%s-fliptrack", > feature_str(t.feature), > pipes_str(t.pipes), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > fliptrack_subtest(&t, FLIP_PAGEFLIP); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3174,16 +3193,22 @@ int main(int argc, char *argv[]) > pipes_str(t.pipes), > screen_str(t.screen), > plane_str(t.plane), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > move_subtest(&t); > + } > > igt_subtest_f("%s-%s-%s-%s-%s-onoff", > feature_str(t.feature), > pipes_str(t.pipes), > screen_str(t.screen), > plane_str(t.plane), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > onoff_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3197,23 +3222,30 @@ int main(int argc, char *argv[]) > pipes_str(t.pipes), > screen_str(t.screen), > plane_str(t.plane), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > fullscreen_plane_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > if (t.screen != SCREEN_PRIM || > - t.method != IGT_DRAW_BLT || > - (!opt.show_hidden && t.plane != PLANE_PRI) || > - (!opt.show_hidden && t.fbs != FBS_INDIVIDUAL)) > + t.method != IGT_DRAW_BLT) > continue; > + if (t.plane != PLANE_PRI || > + t.fbs != FBS_INDIVIDUAL) > + t.slow = true; > > igt_subtest_f("%s-%s-%s-%s-multidraw", > feature_str(t.feature), > pipes_str(t.pipes), > plane_str(t.plane), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > multidraw_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3224,8 +3256,11 @@ int main(int argc, char *argv[]) > t.method != IGT_DRAW_MMAP_GTT) > continue; > > - igt_subtest_f("%s-farfromfence", feature_str(t.feature)) > + igt_subtest_f("%s-farfromfence", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > farfromfence_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3243,8 +3278,11 @@ int main(int argc, char *argv[]) > igt_subtest_f("%s-%s-draw-%s", > feature_str(t.feature), > format_str(t.format), > - igt_draw_get_method_name(t.method)) > + igt_draw_get_method_name(t.method)) { > + if (t.slow) > + igt_slow_combinatorial(); > format_draw_subtest(&t); > + } > } > TEST_MODE_ITER_END > > @@ -3256,8 +3294,11 @@ int main(int argc, char *argv[]) > continue; > igt_subtest_f("%s-%s-scaledprimary", > feature_str(t.feature), > - fbs_str(t.fbs)) > + fbs_str(t.fbs)) { > + if (t.slow) > + igt_slow_combinatorial(); > scaledprimary_subtest(&t); > + } > TEST_MODE_ITER_END > > TEST_MODE_ITER_BEGIN(t) > @@ -3268,19 +3309,31 @@ int main(int argc, char *argv[]) > t.method != IGT_DRAW_MMAP_CPU) > continue; > > - igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature)) > + igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > modesetfrombusy_subtest(&t); > + } > > if (t.feature & FEATURE_FBC) > - igt_subtest_f("%s-badstride", feature_str(t.feature)) > + igt_subtest_f("%s-badstride", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > badstride_subtest(&t); > + } > > if (t.feature & FEATURE_PSR) > - igt_subtest_f("%s-slowdraw", feature_str(t.feature)) > + igt_subtest_f("%s-slowdraw", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > slow_draw_subtest(&t); > + } > > - igt_subtest_f("%s-suspend", feature_str(t.feature)) > + igt_subtest_f("%s-suspend", feature_str(t.feature)) { > + if (t.slow) > + igt_slow_combinatorial(); > suspend_subtest(&t); > + } > TEST_MODE_ITER_END > > igt_fixture > -- > 2.6.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 23, 2015 at 11:50:46AM -0200, Paulo Zanoni wrote: [snip] > It's not clear to me, please clarify: now the tests that were > previously completely hidden will be listed in --list-subtests and > will be shown as skipped during normal runs? Yes. Daniel and I discussed this and he thought listing all test cases, even the slow ones, would not be an issue, since QA should be running the default set not the full list (and for that matter, shouldn't QA know what they are doing too? :P). > For kms_frontbuffer_tracking, hidden tests are supposed to be just for > developers who know what they are doing. I hide them behind a special > command-line switch that's not used by QA because I don't want QA > wasting time running those tests. One third of the > kms_frontbuffer_tracking hidden tests only serve the purpose of > checking whether there's a bug in kms_frontbuffer_track itself or not. > For some other hidden tests, they are there just to help better debug > in case some other non-hidden tests fail. Some other hidden tests are > 100% useless and superfluous. Shouldn't 100% useless and superfluous tests be excised completely? > QA should only run the non-hidden tests. Which is the default behaviour, AFAICT. > So if some non-hidden test fails, the developers can use the hidden > tests to help debugging. > > Besides, the "if (t.slow)" could have been moved to > check_test_requirements(), making the code much simpler :) Thanks for the suggestion. Will modify the code accordingly. That change does indeed simplify things quite a bit! Kind regards, David
On Fri, Oct 23, 2015 at 03:55:23PM +0100, Thomas Wood wrote: > On 23 October 2015 at 12:42, David Weinehall > <david.weinehall@linux.intel.com> wrote: > > Some tests should not be run by default, due to their slow, > > and sometimes superfluous, nature. > > > > We still want to be able to run these tests though in some cases. > > Until now there's been no unified way of handling this. Remedy > > this by introducing the --with-slow-combinatorial option to > > igt_core, and use it in gem_concurrent_blit & kms_frontbuffer_tracking. > > --- > > lib/igt_core.c | 19 ++++++ > > lib/igt_core.h | 1 + > > tests/gem_concurrent_blit.c | 40 ++++++++---- > > tests/kms_frontbuffer_tracking.c | 135 +++++++++++++++++++++++++++------------ > > 4 files changed, 142 insertions(+), 53 deletions(-) > > > > diff --git a/lib/igt_core.c b/lib/igt_core.c > > index 59127cafe606..ba40ce0e0ead 100644 > > --- a/lib/igt_core.c > > +++ b/lib/igt_core.c > > @@ -216,6 +216,7 @@ const char *igt_interactive_debug; > > > > /* subtests helpers */ > > static bool list_subtests = false; > > +static bool with_slow_combinatorial = false; > > static char *run_single_subtest = NULL; > > static bool run_single_subtest_found = false; > > static const char *in_subtest = NULL; > > @@ -235,6 +236,7 @@ bool test_child; > > > > enum { > > OPT_LIST_SUBTESTS, > > + OPT_WITH_SLOW_COMBINATORIAL, > > OPT_RUN_SUBTEST, > > OPT_DESCRIPTION, > > OPT_DEBUG, > > @@ -478,6 +480,7 @@ static void print_usage(const char *help_str, bool output_on_stderr) > > > > fprintf(f, "Usage: %s [OPTIONS]\n", command_str); > > fprintf(f, " --list-subtests\n" > > + " --with-slow-combinatorial\n" > > " --run-subtest <pattern>\n" > > " --debug[=log-domain]\n" > > " --interactive-debug[=domain]\n" > > @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv, > > int c, option_index = 0, i, x; > > static struct option long_options[] = { > > {"list-subtests", 0, 0, OPT_LIST_SUBTESTS}, > > + {"with-slow-combinatorial", 0, 0, OPT_WITH_SLOW_COMBINATORIAL}, > > {"run-subtest", 1, 0, OPT_RUN_SUBTEST}, > > {"help-description", 0, 0, OPT_DESCRIPTION}, > > {"debug", optional_argument, 0, OPT_DEBUG}, > > @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv, > > if (!run_single_subtest) > > list_subtests = true; > > break; > > + case OPT_WITH_SLOW_COMBINATORIAL: > > + if (!run_single_subtest) > > This will cause piglit (and therefore QA) to unconditionally run all > tests marked as slow, since it runs subtests individually. Why doesn't piglit run the default set of tests instead? > > > + with_slow_combinatorial = true; > > + break; > > case OPT_RUN_SUBTEST: > > if (!list_subtests) > > run_single_subtest = strdup(optarg); > > @@ -1629,6 +1637,17 @@ void igt_skip_on_simulation(void) > > igt_require(!igt_run_in_simulation()); > > } > > > > +/** > > + * igt_slow_combinatorial: > > + * > > + * This is used to define subtests that should only be listed/run > > + * when the "--with-slow-combinatorial" has been specified > > This isn't quite correct, as the subtests that use > igt_slow_combinatorial will still always be listed. Yeah, I agree that the comment is incorrect; it should say "be run", or alternatively the code altered to not list them unless "--all" is passed. > > + */ > > +void igt_slow_combinatorial(void) > > +{ > > + igt_skip_on(!with_slow_combinatorial); > > Although it is convenient to just skip the tests when the > --with-slow-combinatorial flag is passed, it may be useful to be able > to classify the subtests before they are run, so that they are > filtered out from the test list entirely. An approach that can do this > might also be used to mark tests as being part of the basic acceptance > tests, so that they can be marked as such without relying on the > naming convention. If the list is how piglit gets its list of tests, doign classification won't be feasible, since only "testname" or "testname (SKIP)" are valid, TTBOMK.
On 26 October 2015 at 15:28, David Weinehall <david.weinehall@linux.intel.com> wrote: > On Fri, Oct 23, 2015 at 03:55:23PM +0100, Thomas Wood wrote: >> On 23 October 2015 at 12:42, David Weinehall >> <david.weinehall@linux.intel.com> wrote: >> > Some tests should not be run by default, due to their slow, >> > and sometimes superfluous, nature. >> > >> > We still want to be able to run these tests though in some cases. >> > Until now there's been no unified way of handling this. Remedy >> > this by introducing the --with-slow-combinatorial option to >> > igt_core, and use it in gem_concurrent_blit & kms_frontbuffer_tracking. >> > --- >> > lib/igt_core.c | 19 ++++++ >> > lib/igt_core.h | 1 + >> > tests/gem_concurrent_blit.c | 40 ++++++++---- >> > tests/kms_frontbuffer_tracking.c | 135 +++++++++++++++++++++++++++------------ >> > 4 files changed, 142 insertions(+), 53 deletions(-) >> > >> > diff --git a/lib/igt_core.c b/lib/igt_core.c >> > index 59127cafe606..ba40ce0e0ead 100644 >> > --- a/lib/igt_core.c >> > +++ b/lib/igt_core.c >> > @@ -216,6 +216,7 @@ const char *igt_interactive_debug; >> > >> > /* subtests helpers */ >> > static bool list_subtests = false; >> > +static bool with_slow_combinatorial = false; >> > static char *run_single_subtest = NULL; >> > static bool run_single_subtest_found = false; >> > static const char *in_subtest = NULL; >> > @@ -235,6 +236,7 @@ bool test_child; >> > >> > enum { >> > OPT_LIST_SUBTESTS, >> > + OPT_WITH_SLOW_COMBINATORIAL, >> > OPT_RUN_SUBTEST, >> > OPT_DESCRIPTION, >> > OPT_DEBUG, >> > @@ -478,6 +480,7 @@ static void print_usage(const char *help_str, bool output_on_stderr) >> > >> > fprintf(f, "Usage: %s [OPTIONS]\n", command_str); >> > fprintf(f, " --list-subtests\n" >> > + " --with-slow-combinatorial\n" >> > " --run-subtest <pattern>\n" >> > " --debug[=log-domain]\n" >> > " --interactive-debug[=domain]\n" >> > @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv, >> > int c, option_index = 0, i, x; >> > static struct option long_options[] = { >> > {"list-subtests", 0, 0, OPT_LIST_SUBTESTS}, >> > + {"with-slow-combinatorial", 0, 0, OPT_WITH_SLOW_COMBINATORIAL}, >> > {"run-subtest", 1, 0, OPT_RUN_SUBTEST}, >> > {"help-description", 0, 0, OPT_DESCRIPTION}, >> > {"debug", optional_argument, 0, OPT_DEBUG}, >> > @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv, >> > if (!run_single_subtest) >> > list_subtests = true; >> > break; >> > + case OPT_WITH_SLOW_COMBINATORIAL: >> > + if (!run_single_subtest) >> >> This will cause piglit (and therefore QA) to unconditionally run all >> tests marked as slow, since it runs subtests individually. > > Why doesn't piglit run the default set of tests instead? What is the default set of tests? Each subtest is executed by piglit using --run-subtest to ensure information can be collected per-subtest (return code, error messages, dmesg logs, timings, etc.). > >> >> > + with_slow_combinatorial = true; >> > + break; >> > case OPT_RUN_SUBTEST: >> > if (!list_subtests) >> > run_single_subtest = strdup(optarg); >> > @@ -1629,6 +1637,17 @@ void igt_skip_on_simulation(void) >> > igt_require(!igt_run_in_simulation()); >> > } >> > >> > +/** >> > + * igt_slow_combinatorial: >> > + * >> > + * This is used to define subtests that should only be listed/run >> > + * when the "--with-slow-combinatorial" has been specified >> >> This isn't quite correct, as the subtests that use >> igt_slow_combinatorial will still always be listed. > > Yeah, I agree that the comment is incorrect; it should say "be run", > or alternatively the code altered to not list them unless "--all" > is passed. > >> > + */ >> > +void igt_slow_combinatorial(void) >> > +{ >> > + igt_skip_on(!with_slow_combinatorial); >> >> Although it is convenient to just skip the tests when the >> --with-slow-combinatorial flag is passed, it may be useful to be able >> to classify the subtests before they are run, so that they are >> filtered out from the test list entirely. An approach that can do this >> might also be used to mark tests as being part of the basic acceptance >> tests, so that they can be marked as such without relying on the >> naming convention. > > If the list is how piglit gets its list of tests, doign classification > won't be feasible, since only "testname" or "testname (SKIP)" are > valid, TTBOMK. Test and subtest names for i-g-t are collected and parsed in piglit/tests/igt.py, which could always be updated include classification parsing.
2015-10-26 12:59 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: > On Fri, Oct 23, 2015 at 11:50:46AM -0200, Paulo Zanoni wrote: > > [snip] > >> It's not clear to me, please clarify: now the tests that were >> previously completely hidden will be listed in --list-subtests and >> will be shown as skipped during normal runs? > > Yes. Daniel and I discussed this and he thought listing all test > cases, even the slow ones, would not be an issue, since QA should > be running the default set not the full list > (and for that matter, shouldn't QA know what they are doing too? :P). If that's the case, I really think your patch should not touch kms_frontbuffer_tracking.c. The hidden subtests should not appear on the list. People shouldn't even have to ask themselves why they are getting 800 skips from a single testcase. Those are only for debugging purposes. > >> For kms_frontbuffer_tracking, hidden tests are supposed to be just for >> developers who know what they are doing. I hide them behind a special >> command-line switch that's not used by QA because I don't want QA >> wasting time running those tests. One third of the >> kms_frontbuffer_tracking hidden tests only serve the purpose of >> checking whether there's a bug in kms_frontbuffer_track itself or not. >> For some other hidden tests, they are there just to help better debug >> in case some other non-hidden tests fail. Some other hidden tests are >> 100% useless and superfluous. > > Shouldn't 100% useless and superfluous tests be excised completely? The change would be from "if (case && hidden) continue;" to "if (case) continue;". But that's not the focus. There are still tests that are useful for debugging but useless for QA. > >> QA should only run the non-hidden tests. > > Which is the default behaviour, AFAICT. Then why do you want to expose those tests that you're not even planning to run?? You're kinda implying that QA - or someone else - will run those tests at some point, and I say that, for kms_frontbuffer_tracking, that's a waste of time. Maybe this is the case for the other tests you're touching, but not here. > >> So if some non-hidden test fails, the developers can use the hidden >> tests to help debugging. >> >> Besides, the "if (t.slow)" could have been moved to >> check_test_requirements(), making the code much simpler :) > > Thanks for the suggestion. Will modify the code accordingly. > That change does indeed simplify things quite a bit! > > > Kind regards, David
On Mon, Oct 26, 2015 at 02:44:18PM -0200, Paulo Zanoni wrote: > 2015-10-26 12:59 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: > > On Fri, Oct 23, 2015 at 11:50:46AM -0200, Paulo Zanoni wrote: > > > > [snip] > > > >> It's not clear to me, please clarify: now the tests that were > >> previously completely hidden will be listed in --list-subtests and > >> will be shown as skipped during normal runs? > > > > Yes. Daniel and I discussed this and he thought listing all test > > cases, even the slow ones, would not be an issue, since QA should > > be running the default set not the full list > > (and for that matter, shouldn't QA know what they are doing too? :P). > > If that's the case, I really think your patch should not touch > kms_frontbuffer_tracking.c. The hidden subtests should not appear on > the list. People shouldn't even have to ask themselves why they are > getting 800 skips from a single testcase. Those are only for debugging > purposes. Fair enough. I'll try to come up with a resonable way to exclude them from the list in a generic manner. Because that's the whole point of this exercise -- to standardise this rather than have every test case implement its own method of choosing whether or not to run all tests. > > > >> For kms_frontbuffer_tracking, hidden tests are supposed to be just for > >> developers who know what they are doing. I hide them behind a special > >> command-line switch that's not used by QA because I don't want QA > >> wasting time running those tests. One third of the > >> kms_frontbuffer_tracking hidden tests only serve the purpose of > >> checking whether there's a bug in kms_frontbuffer_track itself or not. > >> For some other hidden tests, they are there just to help better debug > >> in case some other non-hidden tests fail. Some other hidden tests are > >> 100% useless and superfluous. > > > > Shouldn't 100% useless and superfluous tests be excised completely? > > The change would be from "if (case && hidden) continue;" to "if (case) > continue;". But that's not the focus. There are still tests that are > useful for debugging but useless for QA. It's not the focus of my change, no. But if there are tests that are useless and/or superfluous, then they should be dropped. Note that I'm not suggesting that all non-default tests be dropped, just that if there indeed are tests that don't make sense, they shouldn't be in the test case in the first place. > > > >> QA should only run the non-hidden tests. > > > > Which is the default behaviour, AFAICT. > > Then why do you want to expose those tests that you're not even > planning to run?? To allow developers to see the options they have? > You're kinda implying that QA - or someone else - > will run those tests at some point, and I say that, for > kms_frontbuffer_tracking, that's a waste of time. Maybe this is the > case for the other tests you're touching, but not here. No, I'm not implying that -- you're putting those words in my mouth. Anyway, the choice to expose all cases, not just those run without specifying --all, was a suggestion by Daniel -- you'll have to prod him to hear what his reasoning was. Regards, David
On Mon, Oct 26, 2015 at 04:28:15PM +0000, Thomas Wood wrote: > On 26 October 2015 at 15:28, David Weinehall > <david.weinehall@linux.intel.com> wrote: > > On Fri, Oct 23, 2015 at 03:55:23PM +0100, Thomas Wood wrote: > >> On 23 October 2015 at 12:42, David Weinehall > >> <david.weinehall@linux.intel.com> wrote: > >> > Some tests should not be run by default, due to their slow, > >> > and sometimes superfluous, nature. > >> > > >> > We still want to be able to run these tests though in some cases. > >> > Until now there's been no unified way of handling this. Remedy > >> > this by introducing the --with-slow-combinatorial option to > >> > igt_core, and use it in gem_concurrent_blit & kms_frontbuffer_tracking. > >> > --- > >> > lib/igt_core.c | 19 ++++++ > >> > lib/igt_core.h | 1 + > >> > tests/gem_concurrent_blit.c | 40 ++++++++---- > >> > tests/kms_frontbuffer_tracking.c | 135 +++++++++++++++++++++++++++------------ > >> > 4 files changed, 142 insertions(+), 53 deletions(-) > >> > > >> > diff --git a/lib/igt_core.c b/lib/igt_core.c > >> > index 59127cafe606..ba40ce0e0ead 100644 > >> > --- a/lib/igt_core.c > >> > +++ b/lib/igt_core.c > >> > @@ -216,6 +216,7 @@ const char *igt_interactive_debug; > >> > > >> > /* subtests helpers */ > >> > static bool list_subtests = false; > >> > +static bool with_slow_combinatorial = false; > >> > static char *run_single_subtest = NULL; > >> > static bool run_single_subtest_found = false; > >> > static const char *in_subtest = NULL; > >> > @@ -235,6 +236,7 @@ bool test_child; > >> > > >> > enum { > >> > OPT_LIST_SUBTESTS, > >> > + OPT_WITH_SLOW_COMBINATORIAL, > >> > OPT_RUN_SUBTEST, > >> > OPT_DESCRIPTION, > >> > OPT_DEBUG, > >> > @@ -478,6 +480,7 @@ static void print_usage(const char *help_str, bool output_on_stderr) > >> > > >> > fprintf(f, "Usage: %s [OPTIONS]\n", command_str); > >> > fprintf(f, " --list-subtests\n" > >> > + " --with-slow-combinatorial\n" > >> > " --run-subtest <pattern>\n" > >> > " --debug[=log-domain]\n" > >> > " --interactive-debug[=domain]\n" > >> > @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv, > >> > int c, option_index = 0, i, x; > >> > static struct option long_options[] = { > >> > {"list-subtests", 0, 0, OPT_LIST_SUBTESTS}, > >> > + {"with-slow-combinatorial", 0, 0, OPT_WITH_SLOW_COMBINATORIAL}, > >> > {"run-subtest", 1, 0, OPT_RUN_SUBTEST}, > >> > {"help-description", 0, 0, OPT_DESCRIPTION}, > >> > {"debug", optional_argument, 0, OPT_DEBUG}, > >> > @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv, > >> > if (!run_single_subtest) > >> > list_subtests = true; > >> > break; > >> > + case OPT_WITH_SLOW_COMBINATORIAL: > >> > + if (!run_single_subtest) > >> > >> This will cause piglit (and therefore QA) to unconditionally run all > >> tests marked as slow, since it runs subtests individually. > > > > Why doesn't piglit run the default set of tests instead? > > What is the default set of tests? Each subtest is executed by piglit > using --run-subtest to ensure information can be collected per-subtest > (return code, error messages, dmesg logs, timings, etc.). The default set would be the tests that are run when running gem_concurrent_blit or kms_frontbuffer_tracking without specifying --all. > > > >> > >> > + with_slow_combinatorial = true; > >> > + break; > >> > case OPT_RUN_SUBTEST: > >> > if (!list_subtests) > >> > run_single_subtest = strdup(optarg); > >> > @@ -1629,6 +1637,17 @@ void igt_skip_on_simulation(void) > >> > igt_require(!igt_run_in_simulation()); > >> > } > >> > > >> > +/** > >> > + * igt_slow_combinatorial: > >> > + * > >> > + * This is used to define subtests that should only be listed/run > >> > + * when the "--with-slow-combinatorial" has been specified > >> > >> This isn't quite correct, as the subtests that use > >> igt_slow_combinatorial will still always be listed. > > > > Yeah, I agree that the comment is incorrect; it should say "be run", > > or alternatively the code altered to not list them unless "--all" > > is passed. > > > >> > + */ > >> > +void igt_slow_combinatorial(void) > >> > +{ > >> > + igt_skip_on(!with_slow_combinatorial); > >> > >> Although it is convenient to just skip the tests when the > >> --with-slow-combinatorial flag is passed, it may be useful to be able > >> to classify the subtests before they are run, so that they are > >> filtered out from the test list entirely. An approach that can do this > >> might also be used to mark tests as being part of the basic acceptance > >> tests, so that they can be marked as such without relying on the > >> naming convention. > > > > If the list is how piglit gets its list of tests, doing classification > > won't be feasible, since only "testname" or "testname (SKIP)" are > > valid, TTBOMK. > > Test and subtest names for i-g-t are collected and parsed in > piglit/tests/igt.py, which could always be updated include > classification parsing. It would probably make sense to do this, but considering that I neither have proper python-fu nor know enough about the various test cases to classify them properly, I think that's orthogonal to this changeset. Kind regards, David
2015-10-26 15:30 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: > On Mon, Oct 26, 2015 at 02:44:18PM -0200, Paulo Zanoni wrote: >> 2015-10-26 12:59 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: >> > On Fri, Oct 23, 2015 at 11:50:46AM -0200, Paulo Zanoni wrote: >> > >> > [snip] >> > >> >> It's not clear to me, please clarify: now the tests that were >> >> previously completely hidden will be listed in --list-subtests and >> >> will be shown as skipped during normal runs? >> > >> > Yes. Daniel and I discussed this and he thought listing all test >> > cases, even the slow ones, would not be an issue, since QA should >> > be running the default set not the full list >> > (and for that matter, shouldn't QA know what they are doing too? :P). >> >> If that's the case, I really think your patch should not touch >> kms_frontbuffer_tracking.c. The hidden subtests should not appear on >> the list. People shouldn't even have to ask themselves why they are >> getting 800 skips from a single testcase. Those are only for debugging >> purposes. > > Fair enough. I'll try to come up with a resonable way to exclude them > from the list in a generic manner. Because that's the whole point of > this exercise -- to standardise this rather than have every test case > implement its own method of choosing whether or not to run all tests. Maybe instead of marking these tests as SKIP we could use some other flag. That would avoid the confusion between "skipped because some condition was not match but the test is useful" vs "skipped because the test is unnecessary". > >> > >> >> For kms_frontbuffer_tracking, hidden tests are supposed to be just for >> >> developers who know what they are doing. I hide them behind a special >> >> command-line switch that's not used by QA because I don't want QA >> >> wasting time running those tests. One third of the >> >> kms_frontbuffer_tracking hidden tests only serve the purpose of >> >> checking whether there's a bug in kms_frontbuffer_track itself or not. >> >> For some other hidden tests, they are there just to help better debug >> >> in case some other non-hidden tests fail. Some other hidden tests are >> >> 100% useless and superfluous. >> > >> > Shouldn't 100% useless and superfluous tests be excised completely? >> >> The change would be from "if (case && hidden) continue;" to "if (case) >> continue;". But that's not the focus. There are still tests that are >> useful for debugging but useless for QA. > > It's not the focus of my change, no. But if there are tests that are > useless and/or superfluous, then they should be dropped. > Note that > I'm not suggesting that all non-default tests be dropped, just that > if there indeed are tests that don't make sense, they shouldn't be > in the test case in the first place. > >> > >> >> QA should only run the non-hidden tests. >> > >> > Which is the default behaviour, AFAICT. >> >> Then why do you want to expose those tests that you're not even >> planning to run?? > > To allow developers to see the options they have? > >> You're kinda implying that QA - or someone else - >> will run those tests at some point, and I say that, for >> kms_frontbuffer_tracking, that's a waste of time. Maybe this is the >> case for the other tests you're touching, but not here. > > No, I'm not implying that -- you're putting those words in my mouth. > > Anyway, the choice to expose all cases, not just those run without > specifying --all, was a suggestion by Daniel -- you'll have to prod him > to hear what his reasoning was. CC'ing Daniel. > > > Regards, David
2015-10-23 12:55 GMT-02:00 Thomas Wood <thomas.wood@intel.com>: > On 23 October 2015 at 12:42, David Weinehall > <david.weinehall@linux.intel.com> wrote: >> Some tests should not be run by default, due to their slow, >> and sometimes superfluous, nature. >> >> We still want to be able to run these tests though in some cases. >> Until now there's been no unified way of handling this. Remedy >> this by introducing the --with-slow-combinatorial option to >> igt_core, and use it in gem_concurrent_blit & kms_frontbuffer_tracking. >> --- >> lib/igt_core.c | 19 ++++++ >> lib/igt_core.h | 1 + >> tests/gem_concurrent_blit.c | 40 ++++++++---- >> tests/kms_frontbuffer_tracking.c | 135 +++++++++++++++++++++++++++------------ >> 4 files changed, 142 insertions(+), 53 deletions(-) >> >> diff --git a/lib/igt_core.c b/lib/igt_core.c >> index 59127cafe606..ba40ce0e0ead 100644 >> --- a/lib/igt_core.c >> +++ b/lib/igt_core.c >> @@ -216,6 +216,7 @@ const char *igt_interactive_debug; >> >> /* subtests helpers */ >> static bool list_subtests = false; >> +static bool with_slow_combinatorial = false; >> static char *run_single_subtest = NULL; >> static bool run_single_subtest_found = false; >> static const char *in_subtest = NULL; >> @@ -235,6 +236,7 @@ bool test_child; >> >> enum { >> OPT_LIST_SUBTESTS, >> + OPT_WITH_SLOW_COMBINATORIAL, >> OPT_RUN_SUBTEST, >> OPT_DESCRIPTION, >> OPT_DEBUG, >> @@ -478,6 +480,7 @@ static void print_usage(const char *help_str, bool output_on_stderr) >> >> fprintf(f, "Usage: %s [OPTIONS]\n", command_str); >> fprintf(f, " --list-subtests\n" >> + " --with-slow-combinatorial\n" >> " --run-subtest <pattern>\n" >> " --debug[=log-domain]\n" >> " --interactive-debug[=domain]\n" >> @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv, >> int c, option_index = 0, i, x; >> static struct option long_options[] = { >> {"list-subtests", 0, 0, OPT_LIST_SUBTESTS}, >> + {"with-slow-combinatorial", 0, 0, OPT_WITH_SLOW_COMBINATORIAL}, >> {"run-subtest", 1, 0, OPT_RUN_SUBTEST}, >> {"help-description", 0, 0, OPT_DESCRIPTION}, >> {"debug", optional_argument, 0, OPT_DEBUG}, >> @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv, >> if (!run_single_subtest) >> list_subtests = true; >> break; >> + case OPT_WITH_SLOW_COMBINATORIAL: >> + if (!run_single_subtest) > > This will cause piglit (and therefore QA) to unconditionally run all > tests marked as slow, since it runs subtests individually. > > >> + with_slow_combinatorial = true; >> + break; >> case OPT_RUN_SUBTEST: >> if (!list_subtests) >> run_single_subtest = strdup(optarg); >> @@ -1629,6 +1637,17 @@ void igt_skip_on_simulation(void) >> igt_require(!igt_run_in_simulation()); >> } >> >> +/** >> + * igt_slow_combinatorial: >> + * >> + * This is used to define subtests that should only be listed/run >> + * when the "--with-slow-combinatorial" has been specified > > This isn't quite correct, as the subtests that use > igt_slow_combinatorial will still always be listed. > >> + */ >> +void igt_slow_combinatorial(void) >> +{ >> + igt_skip_on(!with_slow_combinatorial); > > Although it is convenient to just skip the tests when the > --with-slow-combinatorial flag is passed, it may be useful to be able > to classify the subtests before they are run, so that they are > filtered out from the test list entirely. Maybe we could make --list-subtests not list these subtests unless you also pass --with-slow-combinatorial too? That should help solving the biggest problem for my scripts. So we'd have "./test --list-subtests" for normal usage, and "./test --list-subtests --with-slow-combinatorial" for the full set. This should make these tests remain 100% transparent to QA (given they don't use --with-slow-combinatorial). They won't start showing up as SKIPs on our dashboards. > An approach that can do this > might also be used to mark tests as being part of the basic acceptance > tests, so that they can be marked as such without relying on the > naming convention. > > >> +} >> + >> /* structured logging */ >> >> /** >> diff --git a/lib/igt_core.h b/lib/igt_core.h >> index 5ae09653fd55..6ddf25563275 100644 >> --- a/lib/igt_core.h >> +++ b/lib/igt_core.h >> @@ -680,6 +680,7 @@ bool igt_run_in_simulation(void); >> #define SLOW_QUICK(slow,quick) (igt_run_in_simulation() ? (quick) : (slow)) >> >> void igt_skip_on_simulation(void); >> +void igt_slow_combinatorial(void); >> >> extern const char *igt_interactive_debug; >> >> diff --git a/tests/gem_concurrent_blit.c b/tests/gem_concurrent_blit.c >> index 1d2d787202df..311b6829e984 100644 >> --- a/tests/gem_concurrent_blit.c >> +++ b/tests/gem_concurrent_blit.c >> @@ -931,9 +931,6 @@ run_basic_modes(const struct access_mode *mode, >> struct buffers buffers; >> >> for (h = hangs; h->suffix; h++) { >> - if (!all && *h->suffix) >> - continue; >> - >> for (p = all ? pipelines : pskip; p->prefix; p++) { >> igt_fixture { >> batch = buffers_init(&buffers, mode, fd); >> @@ -941,6 +938,8 @@ run_basic_modes(const struct access_mode *mode, >> >> /* try to overwrite the source values */ >> igt_subtest_f("%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> buffers_create(&buffers, num_buffers); >> @@ -950,6 +949,8 @@ run_basic_modes(const struct access_mode *mode, >> } >> >> igt_subtest_f("%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> buffers_create(&buffers, num_buffers); >> @@ -959,6 +960,8 @@ run_basic_modes(const struct access_mode *mode, >> } >> >> igt_subtest_f("%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> buffers_create(&buffers, num_buffers); >> @@ -968,6 +971,8 @@ run_basic_modes(const struct access_mode *mode, >> } >> >> igt_subtest_f("%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> igt_require(rendercopy); >> @@ -978,6 +983,8 @@ run_basic_modes(const struct access_mode *mode, >> } >> >> igt_subtest_f("%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> buffers_create(&buffers, num_buffers); >> @@ -988,6 +995,8 @@ run_basic_modes(const struct access_mode *mode, >> >> /* try to intermix copies with GPU copies*/ >> igt_subtest_f("%s-%s-intermix-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> igt_require(rendercopy); >> @@ -997,6 +1006,8 @@ run_basic_modes(const struct access_mode *mode, >> p->copy, h->hang); >> } >> igt_subtest_f("%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> igt_require(rendercopy); >> @@ -1006,6 +1017,8 @@ run_basic_modes(const struct access_mode *mode, >> p->copy, h->hang); >> } >> igt_subtest_f("%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> igt_require(rendercopy); >> @@ -1017,6 +1030,8 @@ run_basic_modes(const struct access_mode *mode, >> >> /* try to read the results before the copy completes */ >> igt_subtest_f("%s-%s-early-read%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> buffers_create(&buffers, num_buffers); >> @@ -1027,6 +1042,8 @@ run_basic_modes(const struct access_mode *mode, >> >> /* concurrent reads */ >> igt_subtest_f("%s-%s-read-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> buffers_create(&buffers, num_buffers); >> @@ -1035,6 +1052,8 @@ run_basic_modes(const struct access_mode *mode, >> p->copy, h->hang); >> } >> igt_subtest_f("%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> igt_require(rendercopy); >> @@ -1046,6 +1065,8 @@ run_basic_modes(const struct access_mode *mode, >> >> /* and finally try to trick the kernel into loosing the pending write */ >> igt_subtest_f("%s-%s-gpu-read-after-write%s%s", mode->name, p->prefix, suffix, h->suffix) { >> + if (*h->suffix) >> + igt_slow_combinatorial(); >> h->require(); >> p->require(); >> buffers_create(&buffers, num_buffers); >> @@ -1064,13 +1085,11 @@ run_basic_modes(const struct access_mode *mode, >> static void >> run_modes(const struct access_mode *mode) >> { >> - if (all) { >> - run_basic_modes(mode, "", run_single); >> + run_basic_modes(mode, "", run_single); >> >> - igt_fork_signal_helper(); >> - run_basic_modes(mode, "-interruptible", run_interruptible); >> - igt_stop_signal_helper(); >> - } >> + igt_fork_signal_helper(); >> + run_basic_modes(mode, "-interruptible", run_interruptible); >> + igt_stop_signal_helper(); >> >> igt_fork_signal_helper(); >> run_basic_modes(mode, "-forked", run_forked); >> @@ -1083,9 +1102,6 @@ igt_main >> >> igt_skip_on_simulation(); >> >> - if (strstr(igt_test_name(), "all")) >> - all = true; >> - >> igt_fixture { >> fd = drm_open_driver(DRIVER_INTEL); >> devid = intel_get_drm_devid(fd); >> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c >> index d97e148c5073..6f84ef0813d9 100644 >> --- a/tests/kms_frontbuffer_tracking.c >> +++ b/tests/kms_frontbuffer_tracking.c >> @@ -47,8 +47,8 @@ IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and " >> * combinations that are somewhat redundant and don't add much value to the >> * test. For example, since we already do the offscreen testing with a single >> * pipe enabled, there's no much value in doing it again with dual pipes. If you >> - * still want to try these redundant tests, you need to use the --show-hidden >> - * option. >> + * still want to try these redundant tests, you need to use the >> + * "--with-slow-combinatorial" option. >> * >> * The most important hidden thing is the FEATURE_NONE set of tests. Whenever >> * you get a failure on any test, it is important to check whether the same test >> @@ -116,6 +116,10 @@ struct test_mode { >> } format; >> >> enum igt_draw_method method; >> + >> + /* The test is slow and/or combinatorial; >> + * skip unless otherwise specified */ >> + bool slow; >> }; >> >> enum flip_type { >> @@ -237,7 +241,6 @@ struct { >> bool fbc_check_last_action; >> bool no_edp; >> bool small_modes; >> - bool show_hidden; >> int step; >> int only_feature; >> int only_pipes; >> @@ -250,7 +253,6 @@ struct { >> .fbc_check_last_action = true, >> .no_edp = false, >> .small_modes = false, >> - .show_hidden= false, >> .step = 0, >> .only_feature = FEATURE_COUNT, >> .only_pipes = PIPE_COUNT, >> @@ -2892,9 +2894,6 @@ static int opt_handler(int option, int option_index, void *data) >> case 'm': >> opt.small_modes = true; >> break; >> - case 'i': >> - opt.show_hidden = true; >> - break; >> case 't': >> opt.step++; >> break; >> @@ -2942,7 +2941,6 @@ const char *help_str = >> " --no-fbc-action-check Don't check for the FBC last action\n" >> " --no-edp Don't use eDP monitors\n" >> " --use-small-modes Use smaller resolutions for the modes\n" >> -" --show-hidden Show hidden subtests\n" >> " --step Stop on each step so you can check the screen\n" >> " --nop-only Only run the \"nop\" feature subtests\n" >> " --fbc-only Only run the \"fbc\" feature subtests\n" >> @@ -3036,6 +3034,7 @@ static const char *format_str(enum pixel_format format) >> >> #define TEST_MODE_ITER_BEGIN(t) \ >> t.format = FORMAT_DEFAULT; \ >> + t.slow = false; \ >> for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) { \ >> for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) { \ >> for (t.screen = 0; t.screen < SCREEN_COUNT; t.screen++) { \ >> @@ -3046,15 +3045,15 @@ static const char *format_str(enum pixel_format format) >> continue; \ >> if (t.screen == SCREEN_OFFSCREEN && t.plane != PLANE_PRI) \ >> continue; \ >> - if (!opt.show_hidden && t.pipes == PIPE_DUAL && \ >> + if (t.pipes == PIPE_DUAL && \ >> t.screen == SCREEN_OFFSCREEN) \ >> - continue; \ >> - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE) \ >> - && t.feature == FEATURE_NONE) \ >> - continue; \ >> - if (!opt.show_hidden && t.fbs == FBS_SHARED && \ >> + t.slow = true; \ >> + if (opt.only_feature != FEATURE_NONE && \ >> + t.feature == FEATURE_NONE) \ >> + t.slow = true; \ >> + if (t.fbs == FBS_SHARED && \ >> (t.plane == PLANE_CUR || t.plane == PLANE_SPR)) \ >> - continue; >> + t.slow = true; >> >> >> #define TEST_MODE_ITER_END } } } } } } >> @@ -3069,7 +3068,6 @@ int main(int argc, char *argv[]) >> { "no-fbc-action-check", 0, 0, 'a'}, >> { "no-edp", 0, 0, 'e'}, >> { "use-small-modes", 0, 0, 'm'}, >> - { "show-hidden", 0, 0, 'i'}, >> { "step", 0, 0, 't'}, >> { "nop-only", 0, 0, 'n'}, >> { "fbc-only", 0, 0, 'f'}, >> @@ -3088,9 +3086,11 @@ int main(int argc, char *argv[]) >> setup_environment(); >> >> for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) { >> - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE) >> - && t.feature == FEATURE_NONE) >> - continue; >> + bool slow = false; >> + >> + if (opt.only_feature != FEATURE_NONE && >> + t.feature == FEATURE_NONE) >> + slow = true; >> for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) { >> t.screen = SCREEN_PRIM; >> t.plane = PLANE_PRI; >> @@ -3101,8 +3101,11 @@ int main(int argc, char *argv[]) >> >> igt_subtest_f("%s-%s-rte", >> feature_str(t.feature), >> - pipes_str(t.pipes)) >> + pipes_str(t.pipes)) { >> + if (slow) >> + igt_slow_combinatorial(); >> rte_subtest(&t); >> + } >> } >> } >> >> @@ -3113,39 +3116,52 @@ int main(int argc, char *argv[]) >> screen_str(t.screen), >> plane_str(t.plane), >> fbs_str(t.fbs), >> - igt_draw_get_method_name(t.method)) >> + igt_draw_get_method_name(t.method)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> draw_subtest(&t); >> + } >> TEST_MODE_ITER_END >> >> TEST_MODE_ITER_BEGIN(t) >> if (t.plane != PLANE_PRI || >> - t.screen == SCREEN_OFFSCREEN || >> - (!opt.show_hidden && t.method != IGT_DRAW_BLT)) >> + t.screen == SCREEN_OFFSCREEN) >> continue; >> + if (t.method != IGT_DRAW_BLT) >> + t.slow = true; >> >> igt_subtest_f("%s-%s-%s-%s-flip-%s", >> feature_str(t.feature), >> pipes_str(t.pipes), >> screen_str(t.screen), >> fbs_str(t.fbs), >> - igt_draw_get_method_name(t.method)) >> + igt_draw_get_method_name(t.method)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> flip_subtest(&t, FLIP_PAGEFLIP); >> + } >> >> igt_subtest_f("%s-%s-%s-%s-evflip-%s", >> feature_str(t.feature), >> pipes_str(t.pipes), >> screen_str(t.screen), >> fbs_str(t.fbs), >> - igt_draw_get_method_name(t.method)) >> + igt_draw_get_method_name(t.method)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> flip_subtest(&t, FLIP_PAGEFLIP_EVENT); >> + } >> >> igt_subtest_f("%s-%s-%s-%s-msflip-%s", >> feature_str(t.feature), >> pipes_str(t.pipes), >> screen_str(t.screen), >> fbs_str(t.fbs), >> - igt_draw_get_method_name(t.method)) >> + igt_draw_get_method_name(t.method)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> flip_subtest(&t, FLIP_MODESET); >> + } >> >> TEST_MODE_ITER_END >> >> @@ -3159,8 +3175,11 @@ int main(int argc, char *argv[]) >> igt_subtest_f("%s-%s-%s-fliptrack", >> feature_str(t.feature), >> pipes_str(t.pipes), >> - fbs_str(t.fbs)) >> + fbs_str(t.fbs)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> fliptrack_subtest(&t, FLIP_PAGEFLIP); >> + } >> TEST_MODE_ITER_END >> >> TEST_MODE_ITER_BEGIN(t) >> @@ -3174,16 +3193,22 @@ int main(int argc, char *argv[]) >> pipes_str(t.pipes), >> screen_str(t.screen), >> plane_str(t.plane), >> - fbs_str(t.fbs)) >> + fbs_str(t.fbs)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> move_subtest(&t); >> + } >> >> igt_subtest_f("%s-%s-%s-%s-%s-onoff", >> feature_str(t.feature), >> pipes_str(t.pipes), >> screen_str(t.screen), >> plane_str(t.plane), >> - fbs_str(t.fbs)) >> + fbs_str(t.fbs)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> onoff_subtest(&t); >> + } >> TEST_MODE_ITER_END >> >> TEST_MODE_ITER_BEGIN(t) >> @@ -3197,23 +3222,30 @@ int main(int argc, char *argv[]) >> pipes_str(t.pipes), >> screen_str(t.screen), >> plane_str(t.plane), >> - fbs_str(t.fbs)) >> + fbs_str(t.fbs)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> fullscreen_plane_subtest(&t); >> + } >> TEST_MODE_ITER_END >> >> TEST_MODE_ITER_BEGIN(t) >> if (t.screen != SCREEN_PRIM || >> - t.method != IGT_DRAW_BLT || >> - (!opt.show_hidden && t.plane != PLANE_PRI) || >> - (!opt.show_hidden && t.fbs != FBS_INDIVIDUAL)) >> + t.method != IGT_DRAW_BLT) >> continue; >> + if (t.plane != PLANE_PRI || >> + t.fbs != FBS_INDIVIDUAL) >> + t.slow = true; >> >> igt_subtest_f("%s-%s-%s-%s-multidraw", >> feature_str(t.feature), >> pipes_str(t.pipes), >> plane_str(t.plane), >> - fbs_str(t.fbs)) >> + fbs_str(t.fbs)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> multidraw_subtest(&t); >> + } >> TEST_MODE_ITER_END >> >> TEST_MODE_ITER_BEGIN(t) >> @@ -3224,8 +3256,11 @@ int main(int argc, char *argv[]) >> t.method != IGT_DRAW_MMAP_GTT) >> continue; >> >> - igt_subtest_f("%s-farfromfence", feature_str(t.feature)) >> + igt_subtest_f("%s-farfromfence", feature_str(t.feature)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> farfromfence_subtest(&t); >> + } >> TEST_MODE_ITER_END >> >> TEST_MODE_ITER_BEGIN(t) >> @@ -3243,8 +3278,11 @@ int main(int argc, char *argv[]) >> igt_subtest_f("%s-%s-draw-%s", >> feature_str(t.feature), >> format_str(t.format), >> - igt_draw_get_method_name(t.method)) >> + igt_draw_get_method_name(t.method)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> format_draw_subtest(&t); >> + } >> } >> TEST_MODE_ITER_END >> >> @@ -3256,8 +3294,11 @@ int main(int argc, char *argv[]) >> continue; >> igt_subtest_f("%s-%s-scaledprimary", >> feature_str(t.feature), >> - fbs_str(t.fbs)) >> + fbs_str(t.fbs)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> scaledprimary_subtest(&t); >> + } >> TEST_MODE_ITER_END >> >> TEST_MODE_ITER_BEGIN(t) >> @@ -3268,19 +3309,31 @@ int main(int argc, char *argv[]) >> t.method != IGT_DRAW_MMAP_CPU) >> continue; >> >> - igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature)) >> + igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> modesetfrombusy_subtest(&t); >> + } >> >> if (t.feature & FEATURE_FBC) >> - igt_subtest_f("%s-badstride", feature_str(t.feature)) >> + igt_subtest_f("%s-badstride", feature_str(t.feature)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> badstride_subtest(&t); >> + } >> >> if (t.feature & FEATURE_PSR) >> - igt_subtest_f("%s-slowdraw", feature_str(t.feature)) >> + igt_subtest_f("%s-slowdraw", feature_str(t.feature)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> slow_draw_subtest(&t); >> + } >> >> - igt_subtest_f("%s-suspend", feature_str(t.feature)) >> + igt_subtest_f("%s-suspend", feature_str(t.feature)) { >> + if (t.slow) >> + igt_slow_combinatorial(); >> suspend_subtest(&t); >> + } >> TEST_MODE_ITER_END >> >> igt_fixture >> -- >> 2.6.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Oct 26, 2015 at 03:59:24PM -0200, Paulo Zanoni wrote: > 2015-10-26 15:30 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: > > On Mon, Oct 26, 2015 at 02:44:18PM -0200, Paulo Zanoni wrote: > >> 2015-10-26 12:59 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: > >> > On Fri, Oct 23, 2015 at 11:50:46AM -0200, Paulo Zanoni wrote: > >> > > >> > [snip] > >> > > >> >> It's not clear to me, please clarify: now the tests that were > >> >> previously completely hidden will be listed in --list-subtests and > >> >> will be shown as skipped during normal runs? > >> > > >> > Yes. Daniel and I discussed this and he thought listing all test > >> > cases, even the slow ones, would not be an issue, since QA should > >> > be running the default set not the full list > >> > (and for that matter, shouldn't QA know what they are doing too? :P). > >> > >> If that's the case, I really think your patch should not touch > >> kms_frontbuffer_tracking.c. The hidden subtests should not appear on > >> the list. People shouldn't even have to ask themselves why they are > >> getting 800 skips from a single testcase. Those are only for debugging > >> purposes. > > > > Fair enough. I'll try to come up with a resonable way to exclude them > > from the list in a generic manner. Because that's the whole point of > > this exercise -- to standardise this rather than have every test case > > implement its own method of choosing whether or not to run all tests. > > Maybe instead of marking these tests as SKIP we could use some other > flag. That would avoid the confusion between "skipped because some > condition was not match but the test is useful" vs "skipped because > the test is unnecessary". I'd prefer a method that wouldn't require patching piglit. Regards, David
On Tue, Oct 27, 2015 at 08:47:28AM +0200, David Weinehall wrote: > On Mon, Oct 26, 2015 at 03:59:24PM -0200, Paulo Zanoni wrote: > > 2015-10-26 15:30 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: > > > On Mon, Oct 26, 2015 at 02:44:18PM -0200, Paulo Zanoni wrote: > > >> 2015-10-26 12:59 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: > > >> > On Fri, Oct 23, 2015 at 11:50:46AM -0200, Paulo Zanoni wrote: > > >> > > > >> > [snip] > > >> > > > >> >> It's not clear to me, please clarify: now the tests that were > > >> >> previously completely hidden will be listed in --list-subtests and > > >> >> will be shown as skipped during normal runs? > > >> > > > >> > Yes. Daniel and I discussed this and he thought listing all test > > >> > cases, even the slow ones, would not be an issue, since QA should > > >> > be running the default set not the full list > > >> > (and for that matter, shouldn't QA know what they are doing too? :P). > > >> > > >> If that's the case, I really think your patch should not touch > > >> kms_frontbuffer_tracking.c. The hidden subtests should not appear on > > >> the list. People shouldn't even have to ask themselves why they are > > >> getting 800 skips from a single testcase. Those are only for debugging > > >> purposes. > > > > > > Fair enough. I'll try to come up with a resonable way to exclude them > > > from the list in a generic manner. Because that's the whole point of > > > this exercise -- to standardise this rather than have every test case > > > implement its own method of choosing whether or not to run all tests. > > > > Maybe instead of marking these tests as SKIP we could use some other > > flag. That would avoid the confusion between "skipped because some > > condition was not match but the test is useful" vs "skipped because > > the test is unnecessary". > > I'd prefer a method that wouldn't require patching piglit. The entire "why was this skipped" question is currently unsolved, since there's also "skipped because old kernel" and "skipped because wrong platform" and "skipped because not enough ram" and "skipped because wrong outputs connected" and "skipped because ...". In short, it's a much bigger problem imo. -Daniel
On Mon, Oct 26, 2015 at 03:59:24PM -0200, Paulo Zanoni wrote: > 2015-10-26 15:30 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: > > On Mon, Oct 26, 2015 at 02:44:18PM -0200, Paulo Zanoni wrote: > >> 2015-10-26 12:59 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: > >> > On Fri, Oct 23, 2015 at 11:50:46AM -0200, Paulo Zanoni wrote: > >> > > >> > [snip] > >> > > >> >> It's not clear to me, please clarify: now the tests that were > >> >> previously completely hidden will be listed in --list-subtests and > >> >> will be shown as skipped during normal runs? > >> > > >> > Yes. Daniel and I discussed this and he thought listing all test > >> > cases, even the slow ones, would not be an issue, since QA should > >> > be running the default set not the full list > >> > (and for that matter, shouldn't QA know what they are doing too? :P). > >> > >> If that's the case, I really think your patch should not touch > >> kms_frontbuffer_tracking.c. The hidden subtests should not appear on > >> the list. People shouldn't even have to ask themselves why they are > >> getting 800 skips from a single testcase. Those are only for debugging > >> purposes. > > > > Fair enough. I'll try to come up with a resonable way to exclude them > > from the list in a generic manner. Because that's the whole point of > > this exercise -- to standardise this rather than have every test case > > implement its own method of choosing whether or not to run all tests. > > Maybe instead of marking these tests as SKIP we could use some other > flag. That would avoid the confusion between "skipped because some > condition was not match but the test is useful" vs "skipped because > the test is unnecessary". > > > > >> > > >> >> For kms_frontbuffer_tracking, hidden tests are supposed to be just for > >> >> developers who know what they are doing. I hide them behind a special > >> >> command-line switch that's not used by QA because I don't want QA > >> >> wasting time running those tests. One third of the > >> >> kms_frontbuffer_tracking hidden tests only serve the purpose of > >> >> checking whether there's a bug in kms_frontbuffer_track itself or not. > >> >> For some other hidden tests, they are there just to help better debug > >> >> in case some other non-hidden tests fail. Some other hidden tests are > >> >> 100% useless and superfluous. > >> > > >> > Shouldn't 100% useless and superfluous tests be excised completely? > >> > >> The change would be from "if (case && hidden) continue;" to "if (case) > >> continue;". But that's not the focus. There are still tests that are > >> useful for debugging but useless for QA. > > > > It's not the focus of my change, no. But if there are tests that are > > useless and/or superfluous, then they should be dropped. > > Note that > > I'm not suggesting that all non-default tests be dropped, just that > > if there indeed are tests that don't make sense, they shouldn't be > > in the test case in the first place. > > > >> > > >> >> QA should only run the non-hidden tests. > >> > > >> > Which is the default behaviour, AFAICT. > >> > >> Then why do you want to expose those tests that you're not even > >> planning to run?? > > > > To allow developers to see the options they have? > > > >> You're kinda implying that QA - or someone else - > >> will run those tests at some point, and I say that, for > >> kms_frontbuffer_tracking, that's a waste of time. Maybe this is the > >> case for the other tests you're touching, but not here. > > > > No, I'm not implying that -- you're putting those words in my mouth. > > > > Anyway, the choice to expose all cases, not just those run without > > specifying --all, was a suggestion by Daniel -- you'll have to prod him > > to hear what his reasoning was. > > CC'ing Daniel. I thought the hidden tests in kms_frontbuffer_tracking would be useful, just really slow, but seems I'm mistaken. In general we have a bunch of stress tests which we want to run, but at a lower priority. The idea is to eventually use this knob to resurface them, but right now with only BAT igt running in our CI we can't do that and still need to do a lot of other things first. -Daniel
2015-11-17 13:34 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Oct 26, 2015 at 03:59:24PM -0200, Paulo Zanoni wrote: >> 2015-10-26 15:30 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: >> > On Mon, Oct 26, 2015 at 02:44:18PM -0200, Paulo Zanoni wrote: >> >> 2015-10-26 12:59 GMT-02:00 David Weinehall <david.weinehall@linux.intel.com>: >> >> > On Fri, Oct 23, 2015 at 11:50:46AM -0200, Paulo Zanoni wrote: >> >> > >> >> > [snip] >> >> > >> >> >> It's not clear to me, please clarify: now the tests that were >> >> >> previously completely hidden will be listed in --list-subtests and >> >> >> will be shown as skipped during normal runs? >> >> > >> >> > Yes. Daniel and I discussed this and he thought listing all test >> >> > cases, even the slow ones, would not be an issue, since QA should >> >> > be running the default set not the full list >> >> > (and for that matter, shouldn't QA know what they are doing too? :P). >> >> >> >> If that's the case, I really think your patch should not touch >> >> kms_frontbuffer_tracking.c. The hidden subtests should not appear on >> >> the list. People shouldn't even have to ask themselves why they are >> >> getting 800 skips from a single testcase. Those are only for debugging >> >> purposes. >> > >> > Fair enough. I'll try to come up with a resonable way to exclude them >> > from the list in a generic manner. Because that's the whole point of >> > this exercise -- to standardise this rather than have every test case >> > implement its own method of choosing whether or not to run all tests. >> >> Maybe instead of marking these tests as SKIP we could use some other >> flag. That would avoid the confusion between "skipped because some >> condition was not match but the test is useful" vs "skipped because >> the test is unnecessary". >> >> > >> >> > >> >> >> For kms_frontbuffer_tracking, hidden tests are supposed to be just for >> >> >> developers who know what they are doing. I hide them behind a special >> >> >> command-line switch that's not used by QA because I don't want QA >> >> >> wasting time running those tests. One third of the >> >> >> kms_frontbuffer_tracking hidden tests only serve the purpose of >> >> >> checking whether there's a bug in kms_frontbuffer_track itself or not. >> >> >> For some other hidden tests, they are there just to help better debug >> >> >> in case some other non-hidden tests fail. Some other hidden tests are >> >> >> 100% useless and superfluous. >> >> > >> >> > Shouldn't 100% useless and superfluous tests be excised completely? >> >> >> >> The change would be from "if (case && hidden) continue;" to "if (case) >> >> continue;". But that's not the focus. There are still tests that are >> >> useful for debugging but useless for QA. >> > >> > It's not the focus of my change, no. But if there are tests that are >> > useless and/or superfluous, then they should be dropped. >> > Note that >> > I'm not suggesting that all non-default tests be dropped, just that >> > if there indeed are tests that don't make sense, they shouldn't be >> > in the test case in the first place. >> > >> >> > >> >> >> QA should only run the non-hidden tests. >> >> > >> >> > Which is the default behaviour, AFAICT. >> >> >> >> Then why do you want to expose those tests that you're not even >> >> planning to run?? >> > >> > To allow developers to see the options they have? >> > >> >> You're kinda implying that QA - or someone else - >> >> will run those tests at some point, and I say that, for >> >> kms_frontbuffer_tracking, that's a waste of time. Maybe this is the >> >> case for the other tests you're touching, but not here. >> > >> > No, I'm not implying that -- you're putting those words in my mouth. >> > >> > Anyway, the choice to expose all cases, not just those run without >> > specifying --all, was a suggestion by Daniel -- you'll have to prod him >> > to hear what his reasoning was. >> >> CC'ing Daniel. > > I thought the hidden tests in kms_frontbuffer_tracking would be useful, > just really slow, but seems I'm mistaken. In general we have a bunch of > stress tests which we want to run, but at a lower priority. So it doesn't sound good to put both the kms_frontbuffer_trackign and the slow-but-useful behind the same knob. Anyway, I think the "flags" idea can solve the problem. > The idea is to > eventually use this knob to resurface them, but right now with only BAT > igt running in our CI we can't do that and still need to do a lot of other > things first. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Tue, Nov 17, 2015 at 01:49:06PM -0200, Paulo Zanoni wrote: > 2015-11-17 13:34 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: [snip] > > I thought the hidden tests in kms_frontbuffer_tracking would be useful, > > just really slow, but seems I'm mistaken. In general we have a bunch of > > stress tests which we want to run, but at a lower priority. > > So it doesn't sound good to put both the kms_frontbuffer_trackign and > the slow-but-useful behind the same knob. Anyway, I think the "flags" > idea can solve the problem. Indeed it should be able to solve that problem. Obviously it cannot solve the "skipped because the feature isn't available on this platform", "blacklisted because this feature hasn't been implemented yet", and what not, but that is, I think, out of scope here. So, does anyone have any objections (philosophical, colour of bikeshed, or technical) against the current "flags" implementation? Kind regards, David
diff --git a/lib/igt_core.c b/lib/igt_core.c index 59127cafe606..ba40ce0e0ead 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -216,6 +216,7 @@ const char *igt_interactive_debug; /* subtests helpers */ static bool list_subtests = false; +static bool with_slow_combinatorial = false; static char *run_single_subtest = NULL; static bool run_single_subtest_found = false; static const char *in_subtest = NULL; @@ -235,6 +236,7 @@ bool test_child; enum { OPT_LIST_SUBTESTS, + OPT_WITH_SLOW_COMBINATORIAL, OPT_RUN_SUBTEST, OPT_DESCRIPTION, OPT_DEBUG, @@ -478,6 +480,7 @@ static void print_usage(const char *help_str, bool output_on_stderr) fprintf(f, "Usage: %s [OPTIONS]\n", command_str); fprintf(f, " --list-subtests\n" + " --with-slow-combinatorial\n" " --run-subtest <pattern>\n" " --debug[=log-domain]\n" " --interactive-debug[=domain]\n" @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv, int c, option_index = 0, i, x; static struct option long_options[] = { {"list-subtests", 0, 0, OPT_LIST_SUBTESTS}, + {"with-slow-combinatorial", 0, 0, OPT_WITH_SLOW_COMBINATORIAL}, {"run-subtest", 1, 0, OPT_RUN_SUBTEST}, {"help-description", 0, 0, OPT_DESCRIPTION}, {"debug", optional_argument, 0, OPT_DEBUG}, @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv, if (!run_single_subtest) list_subtests = true; break; + case OPT_WITH_SLOW_COMBINATORIAL: + if (!run_single_subtest) + with_slow_combinatorial = true; + break; case OPT_RUN_SUBTEST: if (!list_subtests) run_single_subtest = strdup(optarg); @@ -1629,6 +1637,17 @@ void igt_skip_on_simulation(void) igt_require(!igt_run_in_simulation()); } +/** + * igt_slow_combinatorial: + * + * This is used to define subtests that should only be listed/run + * when the "--with-slow-combinatorial" has been specified + */ +void igt_slow_combinatorial(void) +{ + igt_skip_on(!with_slow_combinatorial); +} + /* structured logging */ /** diff --git a/lib/igt_core.h b/lib/igt_core.h index 5ae09653fd55..6ddf25563275 100644 --- a/lib/igt_core.h +++ b/lib/igt_core.h @@ -680,6 +680,7 @@ bool igt_run_in_simulation(void); #define SLOW_QUICK(slow,quick) (igt_run_in_simulation() ? (quick) : (slow)) void igt_skip_on_simulation(void); +void igt_slow_combinatorial(void); extern const char *igt_interactive_debug; diff --git a/tests/gem_concurrent_blit.c b/tests/gem_concurrent_blit.c index 1d2d787202df..311b6829e984 100644 --- a/tests/gem_concurrent_blit.c +++ b/tests/gem_concurrent_blit.c @@ -931,9 +931,6 @@ run_basic_modes(const struct access_mode *mode, struct buffers buffers; for (h = hangs; h->suffix; h++) { - if (!all && *h->suffix) - continue; - for (p = all ? pipelines : pskip; p->prefix; p++) { igt_fixture { batch = buffers_init(&buffers, mode, fd); @@ -941,6 +938,8 @@ run_basic_modes(const struct access_mode *mode, /* try to overwrite the source values */ igt_subtest_f("%s-%s-overwrite-source-one%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); buffers_create(&buffers, num_buffers); @@ -950,6 +949,8 @@ run_basic_modes(const struct access_mode *mode, } igt_subtest_f("%s-%s-overwrite-source%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); buffers_create(&buffers, num_buffers); @@ -959,6 +960,8 @@ run_basic_modes(const struct access_mode *mode, } igt_subtest_f("%s-%s-overwrite-source-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); buffers_create(&buffers, num_buffers); @@ -968,6 +971,8 @@ run_basic_modes(const struct access_mode *mode, } igt_subtest_f("%s-%s-overwrite-source-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); igt_require(rendercopy); @@ -978,6 +983,8 @@ run_basic_modes(const struct access_mode *mode, } igt_subtest_f("%s-%s-overwrite-source-rev%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); buffers_create(&buffers, num_buffers); @@ -988,6 +995,8 @@ run_basic_modes(const struct access_mode *mode, /* try to intermix copies with GPU copies*/ igt_subtest_f("%s-%s-intermix-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); igt_require(rendercopy); @@ -997,6 +1006,8 @@ run_basic_modes(const struct access_mode *mode, p->copy, h->hang); } igt_subtest_f("%s-%s-intermix-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); igt_require(rendercopy); @@ -1006,6 +1017,8 @@ run_basic_modes(const struct access_mode *mode, p->copy, h->hang); } igt_subtest_f("%s-%s-intermix-both%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); igt_require(rendercopy); @@ -1017,6 +1030,8 @@ run_basic_modes(const struct access_mode *mode, /* try to read the results before the copy completes */ igt_subtest_f("%s-%s-early-read%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); buffers_create(&buffers, num_buffers); @@ -1027,6 +1042,8 @@ run_basic_modes(const struct access_mode *mode, /* concurrent reads */ igt_subtest_f("%s-%s-read-read-bcs%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); buffers_create(&buffers, num_buffers); @@ -1035,6 +1052,8 @@ run_basic_modes(const struct access_mode *mode, p->copy, h->hang); } igt_subtest_f("%s-%s-read-read-rcs%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); igt_require(rendercopy); @@ -1046,6 +1065,8 @@ run_basic_modes(const struct access_mode *mode, /* and finally try to trick the kernel into loosing the pending write */ igt_subtest_f("%s-%s-gpu-read-after-write%s%s", mode->name, p->prefix, suffix, h->suffix) { + if (*h->suffix) + igt_slow_combinatorial(); h->require(); p->require(); buffers_create(&buffers, num_buffers); @@ -1064,13 +1085,11 @@ run_basic_modes(const struct access_mode *mode, static void run_modes(const struct access_mode *mode) { - if (all) { - run_basic_modes(mode, "", run_single); + run_basic_modes(mode, "", run_single); - igt_fork_signal_helper(); - run_basic_modes(mode, "-interruptible", run_interruptible); - igt_stop_signal_helper(); - } + igt_fork_signal_helper(); + run_basic_modes(mode, "-interruptible", run_interruptible); + igt_stop_signal_helper(); igt_fork_signal_helper(); run_basic_modes(mode, "-forked", run_forked); @@ -1083,9 +1102,6 @@ igt_main igt_skip_on_simulation(); - if (strstr(igt_test_name(), "all")) - all = true; - igt_fixture { fd = drm_open_driver(DRIVER_INTEL); devid = intel_get_drm_devid(fd); diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c index d97e148c5073..6f84ef0813d9 100644 --- a/tests/kms_frontbuffer_tracking.c +++ b/tests/kms_frontbuffer_tracking.c @@ -47,8 +47,8 @@ IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and " * combinations that are somewhat redundant and don't add much value to the * test. For example, since we already do the offscreen testing with a single * pipe enabled, there's no much value in doing it again with dual pipes. If you - * still want to try these redundant tests, you need to use the --show-hidden - * option. + * still want to try these redundant tests, you need to use the + * "--with-slow-combinatorial" option. * * The most important hidden thing is the FEATURE_NONE set of tests. Whenever * you get a failure on any test, it is important to check whether the same test @@ -116,6 +116,10 @@ struct test_mode { } format; enum igt_draw_method method; + + /* The test is slow and/or combinatorial; + * skip unless otherwise specified */ + bool slow; }; enum flip_type { @@ -237,7 +241,6 @@ struct { bool fbc_check_last_action; bool no_edp; bool small_modes; - bool show_hidden; int step; int only_feature; int only_pipes; @@ -250,7 +253,6 @@ struct { .fbc_check_last_action = true, .no_edp = false, .small_modes = false, - .show_hidden= false, .step = 0, .only_feature = FEATURE_COUNT, .only_pipes = PIPE_COUNT, @@ -2892,9 +2894,6 @@ static int opt_handler(int option, int option_index, void *data) case 'm': opt.small_modes = true; break; - case 'i': - opt.show_hidden = true; - break; case 't': opt.step++; break; @@ -2942,7 +2941,6 @@ const char *help_str = " --no-fbc-action-check Don't check for the FBC last action\n" " --no-edp Don't use eDP monitors\n" " --use-small-modes Use smaller resolutions for the modes\n" -" --show-hidden Show hidden subtests\n" " --step Stop on each step so you can check the screen\n" " --nop-only Only run the \"nop\" feature subtests\n" " --fbc-only Only run the \"fbc\" feature subtests\n" @@ -3036,6 +3034,7 @@ static const char *format_str(enum pixel_format format) #define TEST_MODE_ITER_BEGIN(t) \ t.format = FORMAT_DEFAULT; \ + t.slow = false; \ for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) { \ for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) { \ for (t.screen = 0; t.screen < SCREEN_COUNT; t.screen++) { \ @@ -3046,15 +3045,15 @@ static const char *format_str(enum pixel_format format) continue; \ if (t.screen == SCREEN_OFFSCREEN && t.plane != PLANE_PRI) \ continue; \ - if (!opt.show_hidden && t.pipes == PIPE_DUAL && \ + if (t.pipes == PIPE_DUAL && \ t.screen == SCREEN_OFFSCREEN) \ - continue; \ - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE) \ - && t.feature == FEATURE_NONE) \ - continue; \ - if (!opt.show_hidden && t.fbs == FBS_SHARED && \ + t.slow = true; \ + if (opt.only_feature != FEATURE_NONE && \ + t.feature == FEATURE_NONE) \ + t.slow = true; \ + if (t.fbs == FBS_SHARED && \ (t.plane == PLANE_CUR || t.plane == PLANE_SPR)) \ - continue; + t.slow = true; #define TEST_MODE_ITER_END } } } } } } @@ -3069,7 +3068,6 @@ int main(int argc, char *argv[]) { "no-fbc-action-check", 0, 0, 'a'}, { "no-edp", 0, 0, 'e'}, { "use-small-modes", 0, 0, 'm'}, - { "show-hidden", 0, 0, 'i'}, { "step", 0, 0, 't'}, { "nop-only", 0, 0, 'n'}, { "fbc-only", 0, 0, 'f'}, @@ -3088,9 +3086,11 @@ int main(int argc, char *argv[]) setup_environment(); for (t.feature = 0; t.feature < FEATURE_COUNT; t.feature++) { - if ((!opt.show_hidden && opt.only_feature != FEATURE_NONE) - && t.feature == FEATURE_NONE) - continue; + bool slow = false; + + if (opt.only_feature != FEATURE_NONE && + t.feature == FEATURE_NONE) + slow = true; for (t.pipes = 0; t.pipes < PIPE_COUNT; t.pipes++) { t.screen = SCREEN_PRIM; t.plane = PLANE_PRI; @@ -3101,8 +3101,11 @@ int main(int argc, char *argv[]) igt_subtest_f("%s-%s-rte", feature_str(t.feature), - pipes_str(t.pipes)) + pipes_str(t.pipes)) { + if (slow) + igt_slow_combinatorial(); rte_subtest(&t); + } } } @@ -3113,39 +3116,52 @@ int main(int argc, char *argv[]) screen_str(t.screen), plane_str(t.plane), fbs_str(t.fbs), - igt_draw_get_method_name(t.method)) + igt_draw_get_method_name(t.method)) { + if (t.slow) + igt_slow_combinatorial(); draw_subtest(&t); + } TEST_MODE_ITER_END TEST_MODE_ITER_BEGIN(t) if (t.plane != PLANE_PRI || - t.screen == SCREEN_OFFSCREEN || - (!opt.show_hidden && t.method != IGT_DRAW_BLT)) + t.screen == SCREEN_OFFSCREEN) continue; + if (t.method != IGT_DRAW_BLT) + t.slow = true; igt_subtest_f("%s-%s-%s-%s-flip-%s", feature_str(t.feature), pipes_str(t.pipes), screen_str(t.screen), fbs_str(t.fbs), - igt_draw_get_method_name(t.method)) + igt_draw_get_method_name(t.method)) { + if (t.slow) + igt_slow_combinatorial(); flip_subtest(&t, FLIP_PAGEFLIP); + } igt_subtest_f("%s-%s-%s-%s-evflip-%s", feature_str(t.feature), pipes_str(t.pipes), screen_str(t.screen), fbs_str(t.fbs), - igt_draw_get_method_name(t.method)) + igt_draw_get_method_name(t.method)) { + if (t.slow) + igt_slow_combinatorial(); flip_subtest(&t, FLIP_PAGEFLIP_EVENT); + } igt_subtest_f("%s-%s-%s-%s-msflip-%s", feature_str(t.feature), pipes_str(t.pipes), screen_str(t.screen), fbs_str(t.fbs), - igt_draw_get_method_name(t.method)) + igt_draw_get_method_name(t.method)) { + if (t.slow) + igt_slow_combinatorial(); flip_subtest(&t, FLIP_MODESET); + } TEST_MODE_ITER_END @@ -3159,8 +3175,11 @@ int main(int argc, char *argv[]) igt_subtest_f("%s-%s-%s-fliptrack", feature_str(t.feature), pipes_str(t.pipes), - fbs_str(t.fbs)) + fbs_str(t.fbs)) { + if (t.slow) + igt_slow_combinatorial(); fliptrack_subtest(&t, FLIP_PAGEFLIP); + } TEST_MODE_ITER_END TEST_MODE_ITER_BEGIN(t) @@ -3174,16 +3193,22 @@ int main(int argc, char *argv[]) pipes_str(t.pipes), screen_str(t.screen), plane_str(t.plane), - fbs_str(t.fbs)) + fbs_str(t.fbs)) { + if (t.slow) + igt_slow_combinatorial(); move_subtest(&t); + } igt_subtest_f("%s-%s-%s-%s-%s-onoff", feature_str(t.feature), pipes_str(t.pipes), screen_str(t.screen), plane_str(t.plane), - fbs_str(t.fbs)) + fbs_str(t.fbs)) { + if (t.slow) + igt_slow_combinatorial(); onoff_subtest(&t); + } TEST_MODE_ITER_END TEST_MODE_ITER_BEGIN(t) @@ -3197,23 +3222,30 @@ int main(int argc, char *argv[]) pipes_str(t.pipes), screen_str(t.screen), plane_str(t.plane), - fbs_str(t.fbs)) + fbs_str(t.fbs)) { + if (t.slow) + igt_slow_combinatorial(); fullscreen_plane_subtest(&t); + } TEST_MODE_ITER_END TEST_MODE_ITER_BEGIN(t) if (t.screen != SCREEN_PRIM || - t.method != IGT_DRAW_BLT || - (!opt.show_hidden && t.plane != PLANE_PRI) || - (!opt.show_hidden && t.fbs != FBS_INDIVIDUAL)) + t.method != IGT_DRAW_BLT) continue; + if (t.plane != PLANE_PRI || + t.fbs != FBS_INDIVIDUAL) + t.slow = true; igt_subtest_f("%s-%s-%s-%s-multidraw", feature_str(t.feature), pipes_str(t.pipes), plane_str(t.plane), - fbs_str(t.fbs)) + fbs_str(t.fbs)) { + if (t.slow) + igt_slow_combinatorial(); multidraw_subtest(&t); + } TEST_MODE_ITER_END TEST_MODE_ITER_BEGIN(t) @@ -3224,8 +3256,11 @@ int main(int argc, char *argv[]) t.method != IGT_DRAW_MMAP_GTT) continue; - igt_subtest_f("%s-farfromfence", feature_str(t.feature)) + igt_subtest_f("%s-farfromfence", feature_str(t.feature)) { + if (t.slow) + igt_slow_combinatorial(); farfromfence_subtest(&t); + } TEST_MODE_ITER_END TEST_MODE_ITER_BEGIN(t) @@ -3243,8 +3278,11 @@ int main(int argc, char *argv[]) igt_subtest_f("%s-%s-draw-%s", feature_str(t.feature), format_str(t.format), - igt_draw_get_method_name(t.method)) + igt_draw_get_method_name(t.method)) { + if (t.slow) + igt_slow_combinatorial(); format_draw_subtest(&t); + } } TEST_MODE_ITER_END @@ -3256,8 +3294,11 @@ int main(int argc, char *argv[]) continue; igt_subtest_f("%s-%s-scaledprimary", feature_str(t.feature), - fbs_str(t.fbs)) + fbs_str(t.fbs)) { + if (t.slow) + igt_slow_combinatorial(); scaledprimary_subtest(&t); + } TEST_MODE_ITER_END TEST_MODE_ITER_BEGIN(t) @@ -3268,19 +3309,31 @@ int main(int argc, char *argv[]) t.method != IGT_DRAW_MMAP_CPU) continue; - igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature)) + igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature)) { + if (t.slow) + igt_slow_combinatorial(); modesetfrombusy_subtest(&t); + } if (t.feature & FEATURE_FBC) - igt_subtest_f("%s-badstride", feature_str(t.feature)) + igt_subtest_f("%s-badstride", feature_str(t.feature)) { + if (t.slow) + igt_slow_combinatorial(); badstride_subtest(&t); + } if (t.feature & FEATURE_PSR) - igt_subtest_f("%s-slowdraw", feature_str(t.feature)) + igt_subtest_f("%s-slowdraw", feature_str(t.feature)) { + if (t.slow) + igt_slow_combinatorial(); slow_draw_subtest(&t); + } - igt_subtest_f("%s-suspend", feature_str(t.feature)) + igt_subtest_f("%s-suspend", feature_str(t.feature)) { + if (t.slow) + igt_slow_combinatorial(); suspend_subtest(&t); + } TEST_MODE_ITER_END igt_fixture