Re: [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests

Gore, Tim
2014-07-09T13:25:08Z

Some comments on Daniels' comments inline

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, July 07, 2014 5:10 PM
> To: Gore, Tim
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init
> for simple tests
>
> On Fri, Jun 27, 2014 at 03:15:37PM +0100, tim.gore@intel.com wrote:
> > From: Tim Gore <tim.gore@intel.com>
> >
> > igt_subtest_init mainly does stuff that we also want for simple/single
> > tests, such as looking for --list-subtests and --help options and
> > calling common_init. So just call this from igt_simple_init and then
> > set tests_with_subtests to false. NOTE that this means that
> > check_igt_exit is now registered as an exit handler for single tests,
> > so need to make sure that ALL tests exit via igt_exit.
> >
> > Signed-off-by: Tim Gore <tim.gore@intel.com>
> > ---
> > lib/igt_core.c | 32 ++++++++++++++++----------------
> > lib/igt_core.h | 4 ++--
> > tests/gem_gtt_hog.c | 2 +-
> > tests/igt_simulation.c | 4 ++--
> > 4 files changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/lib/igt_core.c b/lib/igt_core.c index 7ac7ebe..aaeaa3b
> > 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -458,13 +458,15 @@ void igt_subtest_init(int argc, char **argv)
> > * #igt_simple_main block instead of stitching the tests's main() function
> together
> > * manually.
> > */
> > -void igt_simple_init(void)
> > +void igt_simple_init(int argc, char **argv)
> > {
> > - print_version();
> > -
> > - oom_adjust_for_doom();
> > -
> > - common_init();
> > + /* Use the same init function as is used with subtests - we want most
> of its functionality */
> > + /* Note that this will install the igt_exit_handler so you need to exit
> via igt_exit(), */
> > + /* Dont call exit() */
> > + igt_subtest_init(argc, argv);
>
> Not sure whether forcing the reuse of igt_subtest_init makes a lot of sense
> since it requires a lot of follow-up changes all over. I'd just add a bit of
> argparsing here with the required special cases, i.e.
> - exit without doing anything for --list-subtests
> - exit with 77 when anything is specified with --run-subtests
>
What I wanted to do here was start removing the distinction between single
Tests and multiple test; this distinction seems somewhat artificial and doesn't
seem to add much.
igt_subtest_init does pretty much everything we want for single tests
so I thought it made sense to re-use it. Perhaps the name should change,
although this would lead to more knock on changes.
Tim

> Also I prefer if the piglit changes come together with this patch so that we
> can roll it all out together.

What piglet changes do you refer to? I have not made any piglit changes.
Tim

> -Daniel
>
> > + test_with_subtests = false;
> > + if (list_subtests)
> > + igt_exit();
> > }
> >
> > /*
> > @@ -565,7 +567,7 @@ void igt_skip(const char *f, ...)
> > assert(in_fixture);
> > __igt_fixture_end();
> > } else {
> > - exit(IGT_EXIT_SKIP);
> > + igt_exit();
> > }
> > }
> >
> > @@ -655,7 +657,7 @@ void igt_fail(int exitcode)
> > __igt_fixture_end();
> > }
> >
> > - exit(exitcode);
> > + igt_exit();
> > }
> > }
> >
> > @@ -713,18 +715,16 @@ void igt_exit(void)
> > if (igt_only_list_subtests())
> > exit(IGT_EXIT_SUCCESS);
> >
> > - if (!test_with_subtests)
> > - exit(IGT_EXIT_SUCCESS);
> > -
> > - /* Calling this without calling one of the above is a failure */
> > - assert(skipped_one || succeeded_one || failed_one);
> > + if (test_with_subtests)
> > + /* Calling this without calling one of the above is a failure */
> > + assert(skipped_one || succeeded_one || failed_one);
> >
> > if (failed_one)
> > exit(igt_exitcode);
> > - else if (succeeded_one)
> > - exit(IGT_EXIT_SUCCESS);
> > - else
> > + else if (skipped_one)
> > exit(IGT_EXIT_SKIP);
> > + else
> > + exit(IGT_EXIT_SUCCESS);
> > }
> >
> > /* fork support code */
> > diff --git a/lib/igt_core.h b/lib/igt_core.h index 9853e6b..cabbc3b
> > 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -166,7 +166,7 @@ bool igt_only_list_subtests(void);
> > *
> > * Init for simple tests without subtests
> > */
> > -void igt_simple_init(void);
> > +void igt_simple_init(int argc, char **argv);
> >
> > /**
> > * igt_simple_main:
> > @@ -178,7 +178,7 @@ void igt_simple_init(void); #define
> > igt_simple_main \
> > static void igt_tokencat(__real_main, __LINE__)(int argc, char
> **argv); \
> > int main(int argc, char **argv) { \
> > - igt_simple_init(); \
> > + igt_simple_init(argc, argv); \
> > igt_tokencat(__real_main, __LINE__)(argc, argv); \
> > igt_exit(); \
> > } \
> > diff --git a/tests/gem_gtt_hog.c b/tests/gem_gtt_hog.c index
> > 5d47540..f607ea0 100644
> > --- a/tests/gem_gtt_hog.c
> > +++ b/tests/gem_gtt_hog.c
> > @@ -150,7 +150,7 @@ static void run(data_t *data, int child)
> > munmap(ptr, size);
> >
> > igt_assert(x == canary);
> > - exit(0);
> > + _exit(0);
> > }
> >
> > igt_simple_main
> > diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c index
> > 15cbe64..3048c9e 100644
> > --- a/tests/igt_simulation.c
> > +++ b/tests/igt_simulation.c
> > @@ -53,11 +53,11 @@ static int do_fork(void)
> > assert(0);
> > case 0:
> > if (simple) {
> > - igt_simple_init();
> > + igt_simple_init(1, argv_run);
> >
> > igt_skip_on_simulation();
> >
> > - exit(0);
> > + igt_exit();
> > } else {
> > if (list_subtests)
> > igt_subtest_init(2, argv_list);
> > --
> > 1.9.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch