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

[David Weinehall]

On Wed, Oct 28, 2015 at 02:12:15PM -0200, Paulo Zanoni wrote:
> 2015-10-28 9:29 GMT-02:00 David Weinehall <david.weinehall at linux.intel.com>:
> > Some tests should not be run by default, due to their slow,
> > and sometimes superfluous, nature.
> >
> > We still want to be able to run these tests in some cases.
> > Until now there's been no unified way of handling this. Remedy
> > this by introducing the --all option to igt_core,
> > and use it in gem_concurrent_blit & kms_frontbuffer_tracking.
> 
> I really think you should explain both your plan and its
> implementation in more details here.

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

> >
> > Signed-off-by: David Weinehall <david.weinehall at linux.intel.com>
> > ---
> >  lib/igt_core.c                   |  24 +++++
> >  lib/igt_core.h                   |   7 ++
> >  tests/gem_concurrent_blit.c      |  44 ++++-----
> >  tests/kms_frontbuffer_tracking.c | 208 ++++++++++++++++++++++-----------------
> >  4 files changed, 165 insertions(+), 118 deletions(-)
> >
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 59127cafe606..6575b9d6bf0d 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -216,6 +216,7 @@ const char *igt_interactive_debug;
> >
> >  /* subtests helpers */
> >  static bool list_subtests = false;
> > +static bool with_slow_combinatorial = false;
> 
> The option is called --all, the new subtest macro is _slow and the
> variables and enums are called with_slow_combinatorial. Is this
> intentional?

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

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

> 
> >  static char *run_single_subtest = NULL;
> >  static bool run_single_subtest_found = false;
> >  static const char *in_subtest = NULL;
> > @@ -235,6 +236,7 @@ bool test_child;
> >
> >  enum {
> >   OPT_LIST_SUBTESTS,
> > + OPT_WITH_SLOW_COMBINATORIAL,
> >   OPT_RUN_SUBTEST,
> >   OPT_DESCRIPTION,
> >   OPT_DEBUG,
> > @@ -478,6 +480,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> >
> >         fprintf(f, "Usage: %s [OPTIONS]\n", command_str);
> >         fprintf(f, "  --list-subtests\n"
> > +                  "  --all\n"
> >                    "  --run-subtest <pattern>\n"
> >                    "  --debug[=log-domain]\n"
> >                    "  --interactive-debug[=domain]\n"
> > @@ -510,6 +513,7 @@ static int common_init(int *argc, char **argv,
> >         int c, option_index = 0, i, x;
> >         static struct option long_options[] = {
> >                 {"list-subtests", 0, 0, OPT_LIST_SUBTESTS},
> > +               {"all", 0, 0, OPT_WITH_SLOW_COMBINATORIAL},
> >                 {"run-subtest", 1, 0, OPT_RUN_SUBTEST},
> >                 {"help-description", 0, 0, OPT_DESCRIPTION},
> >                 {"debug", optional_argument, 0, OPT_DEBUG},
> > @@ -617,6 +621,10 @@ static int common_init(int *argc, char **argv,
> >                         if (!run_single_subtest)
> >                                 list_subtests = true;
> >                         break;
> > +               case OPT_WITH_SLOW_COMBINATORIAL:
> > +                       if (!run_single_subtest)
> > +                               with_slow_combinatorial = true;
> > +                       break;
> >                 case OPT_RUN_SUBTEST:
> >                         if (!list_subtests)
> >                                 run_single_subtest = strdup(optarg);
> > @@ -1629,6 +1637,22 @@ void igt_skip_on_simulation(void)
> >                 igt_require(!igt_run_in_simulation());
> >  }
> >
> > +/**
> > + * __igt_slow_combinatorial:
> > + *
> > + * This is used to skip subtests that should only be included
> > + * when the "--all" command line option has been specified.  This version
> > + * is intended as a test.
> > + *
> > + * @slow_test: true if the subtest is part of the slow/combinatorial set
> > + *
> > + * Returns: true if the test should be run, false if the test should be skipped
> > + */
> > +bool __igt_slow_combinatorial(bool slow_test)
> > +{
> > +       return !slow_test || with_slow_combinatorial;
> > +}
> > +
> >  /* structured logging */
> >
> >  /**
> > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > index 5ae09653fd55..7b592278bf6c 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -191,6 +191,12 @@ bool __igt_run_subtest(const char *subtest_name);
> >  #define igt_subtest_f(f...) \
> >         __igt_subtest_f(igt_tokencat(__tmpchar, __LINE__), f)
> >
> > +bool __igt_slow_combinatorial(bool slow_test);
> > +
> 
> We also need a igt_subtest_slow() version (without "_f") and some
> comments explaining what's the real difference between them and the
> other macros, like the other igt_subtest_* macros.

OK, fair enough.

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

Indeed, thanks for spotting.  Will fix.

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

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

[snip]

> > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> > index 15707b9b9040..86fd7ca08692 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -47,8 +47,7 @@ IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and "
> >   * combinations that are somewhat redundant and don't add much value to the
> >   * test. For example, since we already do the offscreen testing with a single
> >   * pipe enabled, there's no much value in doing it again with dual pipes. If you
> > - * still want to try these redundant tests, you need to use the --show-hidden
> > - * option.
> > + * still want to try these redundant tests, you need to use the --all option.
> >   *
> >   * The most important hidden thing is the FEATURE_NONE set of tests. Whenever
> >   * you get a failure on any test, it is important to check whether the same test
> > @@ -116,6 +115,10 @@ struct test_mode {
> >         } format;
> >
> >         enum igt_draw_method method;
> > +
> > +       /* The test is slow and/or combinatorial;
> > +        * skip unless otherwise specified */
> > +       bool slow;
> 
> My problem with this is that exactly none of the tests marked as
> "slow" are actually slow here... They're either redudant or for debug
> purposes, not slow.

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

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


Regards, David