Re: [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main

Daniel Vetter
2014-07-09T14:19:58Z

On Wed, Jul 09, 2014 at 01:50:30PM +0000, Gore, Tim wrote:
> Some inline comments
>
> Tim
>
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Monday, July 07, 2014 5:12 PM
> > To: Gore, Tim
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple
> > main
> >
> > On Fri, Jun 27, 2014 at 03:15:36PM +0100, tim.gore@intel.com wrote:
> > > From: Tim Gore <tim.gore@intel.com>
> > >
> > > Quite a few single tests do not use the igt_simple_main macro because
> > > they want access to argc/argv. So change the igt_simple_main macro to
> > > pass these arguments through to the "__real_mainxxx" function, and
> > > change these tests to use the macro.
> > >
> > > Signed-off-by: Tim Gore <tim.gore@intel.com>
> > > ---
> > > lib/igt_core.h | 8 ++++----
>
>
> > > tests/gem_ctx_basic.c | 6 +-----
> > > tests/gem_exec_blt.c | 5 +----
> > > tests/gem_gtt_speed.c | 5 +----
> > > tests/gem_hang.c | 5 +----
> > > tests/gem_render_copy.c | 4 +---
> > > tests/gem_render_linear_blits.c | 5 +----
> > > tests/gem_render_tiled_blits.c | 5 +----
> > > tests/gem_seqno_wrap.c | 11 ++++-------
> > > tests/gem_stress.c | 5 +----
> > > tests/gen3_mixed_blits.c | 5 +----
> > > tests/gen3_render_linear_blits.c | 5 +----
> > > tests/gen3_render_mixed_blits.c | 5 +----
> > > tests/gen3_render_tiledx_blits.c | 5 +----
> > > tests/gen3_render_tiledy_blits.c | 5 +----
> > > 15 files changed, 21 insertions(+), 63 deletions(-)
> > >
> > > diff --git a/lib/igt_core.h b/lib/igt_core.h index e252eba..9853e6b
> > > 100644
> > > --- a/lib/igt_core.h
> > > +++ b/lib/igt_core.h
> > > @@ -176,13 +176,13 @@ void igt_simple_init(void);
> > > * the test needs to parse additional cmdline arguments of its own.
> > > */
> > > #define igt_simple_main \
> > > - static void igt_tokencat(__real_main, __LINE__)(void); \
> > > + static void igt_tokencat(__real_main, __LINE__)(int argc, char
> > > +**argv); \
> > > int main(int argc, char **argv) { \
> > > igt_simple_init(); \
> > > - igt_tokencat(__real_main, __LINE__)(); \
> > > - exit(0); \
> > > + igt_tokencat(__real_main, __LINE__)(argc, argv); \
> > > + igt_exit(); \
> > > } \
> > > - static void igt_tokencat(__real_main, __LINE__)(void) \
> > > + static void igt_tokencat(__real_main, __LINE__)(int argc, char
> > > +**argv) \
> >
> > Hm, I'd just add argc/argv to igt_simple_init like you do in the next patch and
> > update the relevant tests which have their own main. That would be more in
> > line with subtest-tests which have their own additional arguments, too.
> >
> > Also having functions with magic/predefined arguments is a bit too much
> > magic for my taste. If we keep the signature of real_main as-is we'll avoid
> > (highly unlikely, but still) accidental name clashes.
> >
> > And with my suggestion for patch 2 to just have a bare-bones argv parsing for
> > simple tests we also don't need to call igt_exit.
> > -Daniel
>
> My motivation here is mainly to reduce the amount of differentiation, so that
> we can move away from having a different methodology for tests that want to
> parse argv. Together with the next patch we move towards all tests having the
> same setup/initialisation. I believe that this will make the test suite easier to
> maintain and add features to, such as common Doxygen/--help text.
> I agree that the passing argc/v through to real_main is not ideal, but the whole
> macro thing is not to my taste to be honest, for all the usual reasons. If we can
> reduce the number of test "types" (single/multiple , parses argv/or not) it
> should make the macros easier to maintain etc.

The problem is that simple tests without subtest do work slightly
differently. One option we could pursue instead is to redefine
igt_simple_main as

igt_main
igt_subtest("basic")

but that will look a bit ugly in the test listenings. My proposal for this
patch here was to add (argc, argv) parameters to igt_simple_init and then
adjust just that function in patch 2. I don't see the upside of forcefully
converting the tests to use the igt_simple_main macro - we also have
subtest-tests with their open-coded main() function.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch