All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Single tests to respond to --list-subtests
@ 2014-06-27 14:15 tim.gore
  2014-06-27 14:15 ` [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main tim.gore
  2014-06-27 14:15 ` [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests tim.gore
  0 siblings, 2 replies; 12+ messages in thread
From: tim.gore @ 2014-06-27 14:15 UTC (permalink / raw)
  To: intel-gfx

From: Tim Gore <tim.gore@intel.com>

A step towards towards removing the distinction between
single and multiple tests. The first step is to change
the igt_simple_main macro to pass argc/v through to the
real_main function, so that several simple tests that want
argc/v can still use this macro.
Once this is done, all the simple tests are using this
macro and we then modify this macro and the igt_simple_init
function so that argc/v are available to igt_simple_init which
can then call the igt_subtest_init function to parse the
cmdline args for --list-subtest and help etc.
There are some subtleties are introduced by the fact that
igt_subtest_init registers the check_igt_exit exit handler,
so now single as well as multiple tests must always exit via
igt_exit.


Tim Gore (2):
  intel-gpu-tools: pass argc/argv to simple main
  intel-gpu-tools: Re-use igt_subtest_init for simple tests

 lib/igt_core.c                   | 32 ++++++++++++++++----------------
 lib/igt_core.h                   | 12 ++++++------
 tests/gem_ctx_basic.c            |  6 +-----
 tests/gem_exec_blt.c             |  5 +----
 tests/gem_gtt_hog.c              |  2 +-
 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 +----
 tests/igt_simulation.c           |  4 ++--
 18 files changed, 42 insertions(+), 84 deletions(-)

-- 
1.9.2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main
  2014-06-27 14:15 [PATCH 0/2] Single tests to respond to --list-subtests tim.gore
@ 2014-06-27 14:15 ` tim.gore
  2014-07-01  9:19   ` Thomas Wood
  2014-07-07 16:12   ` Daniel Vetter
  2014-06-27 14:15 ` [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests tim.gore
  1 sibling, 2 replies; 12+ messages in thread
From: tim.gore @ 2014-06-27 14:15 UTC (permalink / raw)
  To: intel-gfx

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) \
 
 __attribute__((format(printf, 1, 2)))
 void igt_skip(const char *f, ...) __attribute__((noreturn));
diff --git a/tests/gem_ctx_basic.c b/tests/gem_ctx_basic.c
index 3e9b688..fe770ea 100644
--- a/tests/gem_ctx_basic.c
+++ b/tests/gem_ctx_basic.c
@@ -145,12 +145,10 @@ static void parse(int argc, char *argv[])
 	}
 }
 
-int main(int argc, char *argv[])
+igt_simple_main
 {
 	int i;
 
-	igt_simple_init();
-
 	fd = drm_open_any_render();
 	devid = intel_get_drm_devid(fd);
 
@@ -173,6 +171,4 @@ int main(int argc, char *argv[])
 
 	free(threads);
 	close(fd);
-
-	return 0;
 }
diff --git a/tests/gem_exec_blt.c b/tests/gem_exec_blt.c
index 3bcef18..3d092fe 100644
--- a/tests/gem_exec_blt.c
+++ b/tests/gem_exec_blt.c
@@ -253,12 +253,10 @@ static void run(int object_size)
 	close(fd);
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	int i;
 
-	igt_simple_init();
-
 	igt_skip_on_simulation();
 
 	if (argc > 1) {
@@ -270,5 +268,4 @@ int main(int argc, char **argv)
 	} else
 		run(OBJECT_SIZE);
 
-	return 0;
 }
diff --git a/tests/gem_gtt_speed.c b/tests/gem_gtt_speed.c
index 385eeb7..fa20de0 100644
--- a/tests/gem_gtt_speed.c
+++ b/tests/gem_gtt_speed.c
@@ -50,7 +50,7 @@ static double elapsed(const struct timeval *start,
 	return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_usec - start->tv_usec))/loop;
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	struct timeval start, end;
 	uint8_t *buf;
@@ -59,8 +59,6 @@ int main(int argc, char **argv)
 	int loop, i, tiling;
 	int fd;
 
-	igt_simple_init();
-
 	igt_skip_on_simulation();
 
 	if (argc > 1)
@@ -329,5 +327,4 @@ int main(int argc, char **argv)
 	gem_close(fd, handle);
 	close(fd);
 
-	return 0;
 }
diff --git a/tests/gem_hang.c b/tests/gem_hang.c
index 6248244..a4f4d10 100644
--- a/tests/gem_hang.c
+++ b/tests/gem_hang.c
@@ -68,12 +68,10 @@ gpu_hang(void)
 	intel_batchbuffer_flush(batch);
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	int fd;
 
-	igt_simple_init();
-
 	igt_assert_f(argc == 2,
 		     "usage: %s <disabled pipe number>\n",
 		     argv[0]);
@@ -93,5 +91,4 @@ int main(int argc, char **argv)
 
 	close(fd);
 
-	return 0;
 }
diff --git a/tests/gem_render_copy.c b/tests/gem_render_copy.c
index fd26b43..12dd90d 100644
--- a/tests/gem_render_copy.c
+++ b/tests/gem_render_copy.c
@@ -117,7 +117,7 @@ scratch_buf_check(data_t *data, struct igt_buf *buf, int x, int y,
 		     color, val, x, y);
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	data_t data = {0, };
 	struct intel_batchbuffer *batch = NULL;
@@ -127,7 +127,6 @@ int main(int argc, char **argv)
 	int opt_dump_png = false;
 	int opt_dump_aub = igt_aub_dump_enabled();
 
-	igt_simple_init();
 
 	while ((opt = getopt(argc, argv, "d")) != -1) {
 		switch (opt) {
@@ -189,5 +188,4 @@ int main(int argc, char **argv)
 		scratch_buf_check(&data, &dst, WIDTH - 10, HEIGHT - 10, SRC_COLOR);
 	}
 
-	return 0;
 }
diff --git a/tests/gem_render_linear_blits.c b/tests/gem_render_linear_blits.c
index f847486..7c6081e 100644
--- a/tests/gem_render_linear_blits.c
+++ b/tests/gem_render_linear_blits.c
@@ -81,7 +81,7 @@ check_bo(int fd, uint32_t handle, uint32_t val)
 	}
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	drm_intel_bufmgr *bufmgr;
 	struct intel_batchbuffer *batch;
@@ -90,8 +90,6 @@ int main(int argc, char **argv)
 	uint32_t start = 0;
 	int i, j, fd, count;
 
-	igt_simple_init();
-
 	fd = drm_open_any();
 
 	render_copy = igt_get_render_copyfunc(intel_get_drm_devid(fd));
@@ -201,5 +199,4 @@ int main(int argc, char **argv)
 	for (i = 0; i < count; i++)
 		check_bo(fd, bo[i]->handle, start_val[i]);
 
-	return 0;
 }
diff --git a/tests/gem_render_tiled_blits.c b/tests/gem_render_tiled_blits.c
index f63c57e..ec8e8de 100644
--- a/tests/gem_render_tiled_blits.c
+++ b/tests/gem_render_tiled_blits.c
@@ -95,7 +95,7 @@ check_bo(struct intel_batchbuffer *batch, struct igt_buf *buf, uint32_t val)
 		dri_bo_unmap(linear);
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	drm_intel_bufmgr *bufmgr;
 	struct intel_batchbuffer *batch;
@@ -105,8 +105,6 @@ int main(int argc, char **argv)
 	int i, j, fd, count;
 	uint32_t devid;
 
-	igt_simple_init();
-
 	igt_skip_on_simulation();
 
 	fd = drm_open_any();
@@ -212,5 +210,4 @@ int main(int argc, char **argv)
 	for (i = 0; i < count; i++)
 		check_bo(batch, &buf[i], start_val[i]);
 
-	return 0;
 }
diff --git a/tests/gem_seqno_wrap.c b/tests/gem_seqno_wrap.c
index beda28b..8a54c2e 100644
--- a/tests/gem_seqno_wrap.c
+++ b/tests/gem_seqno_wrap.c
@@ -533,12 +533,9 @@ static void parse_options(int argc, char **argv)
 	}
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	int wcount = 0;
-	int r = -1;
-
-	igt_simple_init();
 
 	parse_options(argc, argv);
 
@@ -563,8 +560,8 @@ int main(int argc, char **argv)
 
 	if (options.rounds == wcount) {
 		igt_debug("done %d wraps successfully\n", wcount);
-		return 0;
 	}
-
-	return r;
+	else {
+	    igt_fail(-1);
+	}
 }
diff --git a/tests/gem_stress.c b/tests/gem_stress.c
index 2ccb6fc..35ed32f 100644
--- a/tests/gem_stress.c
+++ b/tests/gem_stress.c
@@ -860,13 +860,11 @@ static void check_render_copyfunc(void)
 }
 
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	int i, j;
 	unsigned *current_permutation, *tmp_permutation;
 
-	igt_simple_init();
-
 	drm_fd = drm_open_any();
 	devid = intel_get_drm_devid(drm_fd);
 
@@ -925,5 +923,4 @@ int main(int argc, char **argv)
 
 	igt_stop_signal_helper();
 
-	return 0;
 }
diff --git a/tests/gen3_mixed_blits.c b/tests/gen3_mixed_blits.c
index 75d61a5..f0a6b64 100644
--- a/tests/gen3_mixed_blits.c
+++ b/tests/gen3_mixed_blits.c
@@ -457,14 +457,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
 	munmap(v, WIDTH*HEIGHT*4);
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	uint32_t *handle, *tiling, *start_val;
 	uint32_t start = 0;
 	int i, fd, count;
 
-	igt_simple_init();
-
 	fd = drm_open_any();
 
 	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
@@ -533,5 +531,4 @@ int main(int argc, char **argv)
 		check_bo(fd, handle[i], start_val[i]);
 	igt_info("done\n");
 
-	return 0;
 }
diff --git a/tests/gen3_render_linear_blits.c b/tests/gen3_render_linear_blits.c
index 7fe368d..60c0d0b 100644
--- a/tests/gen3_render_linear_blits.c
+++ b/tests/gen3_render_linear_blits.c
@@ -325,14 +325,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
 	}
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	uint32_t *handle, *start_val;
 	uint32_t start = 0;
 	int i, fd, count;
 
-	igt_simple_init();
-
 	fd = drm_open_any();
 
 	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
@@ -393,5 +391,4 @@ int main(int argc, char **argv)
 	for (i = 0; i < count; i++)
 		check_bo(fd, handle[i], start_val[i]);
 
-	return 0;
 }
diff --git a/tests/gen3_render_mixed_blits.c b/tests/gen3_render_mixed_blits.c
index 77ac0e2..68660a3 100644
--- a/tests/gen3_render_mixed_blits.c
+++ b/tests/gen3_render_mixed_blits.c
@@ -345,14 +345,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
 	munmap(v, WIDTH*HEIGHT*4);
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	uint32_t *handle, *tiling, *start_val;
 	uint32_t start = 0;
 	int i, fd, count;
 
-	igt_simple_init();
-
 	fd = drm_open_any();
 
 	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
@@ -421,5 +419,4 @@ int main(int argc, char **argv)
 		check_bo(fd, handle[i], start_val[i]);
 	igt_info("done\n");
 
-	return 0;
 }
diff --git a/tests/gen3_render_tiledx_blits.c b/tests/gen3_render_tiledx_blits.c
index 95c0c96..d54d714 100644
--- a/tests/gen3_render_tiledx_blits.c
+++ b/tests/gen3_render_tiledx_blits.c
@@ -332,14 +332,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
 	munmap(v, WIDTH*HEIGHT*4);
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	uint32_t *handle, *start_val;
 	uint32_t start = 0;
 	int i, fd, count;
 
-	igt_simple_init();
-
 	fd = drm_open_any();
 
 	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
@@ -400,5 +398,4 @@ int main(int argc, char **argv)
 	for (i = 0; i < count; i++)
 		check_bo(fd, handle[i], start_val[i]);
 
-	return 0;
 }
diff --git a/tests/gen3_render_tiledy_blits.c b/tests/gen3_render_tiledy_blits.c
index 1b9a419..3ef08c8 100644
--- a/tests/gen3_render_tiledy_blits.c
+++ b/tests/gen3_render_tiledy_blits.c
@@ -332,14 +332,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
 	munmap(v, WIDTH*HEIGHT*4);
 }
 
-int main(int argc, char **argv)
+igt_simple_main
 {
 	uint32_t *handle, *start_val;
 	uint32_t start = 0;
 	int i, fd, count;
 
-	igt_simple_init();
-
 	fd = drm_open_any();
 
 	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
@@ -407,5 +405,4 @@ int main(int argc, char **argv)
 		check_bo(fd, handle[i], start_val[i]);
 	igt_info("done\n");
 
-	return 0;
 }
-- 
1.9.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests
  2014-06-27 14:15 [PATCH 0/2] Single tests to respond to --list-subtests tim.gore
  2014-06-27 14:15 ` [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main tim.gore
@ 2014-06-27 14:15 ` tim.gore
  2014-07-01  9:26   ` Thomas Wood
  2014-07-07 16:09   ` Daniel Vetter
  1 sibling, 2 replies; 12+ messages in thread
From: tim.gore @ 2014-06-27 14:15 UTC (permalink / raw)
  To: intel-gfx

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);
+	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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main
  2014-06-27 14:15 ` [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main tim.gore
@ 2014-07-01  9:19   ` Thomas Wood
  2014-07-07 16:12   ` Daniel Vetter
  1 sibling, 0 replies; 12+ messages in thread
From: Thomas Wood @ 2014-07-01  9:19 UTC (permalink / raw)
  To: Tim Gore; +Cc: Intel Graphics Development

On 27 June 2014 15:15,  <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.

It might also be worth marking igt_simple_init as an internal function
as after this change it is only used in one of the internal framework
tests.

>
> 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.

This documentation comment needs updating to mention that argc and
argv are available.


>   */
>  #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) \
>
>  __attribute__((format(printf, 1, 2)))
>  void igt_skip(const char *f, ...) __attribute__((noreturn));
> diff --git a/tests/gem_ctx_basic.c b/tests/gem_ctx_basic.c
> index 3e9b688..fe770ea 100644
> --- a/tests/gem_ctx_basic.c
> +++ b/tests/gem_ctx_basic.c
> @@ -145,12 +145,10 @@ static void parse(int argc, char *argv[])
>         }
>  }
>
> -int main(int argc, char *argv[])
> +igt_simple_main
>  {
>         int i;
>
> -       igt_simple_init();
> -
>         fd = drm_open_any_render();
>         devid = intel_get_drm_devid(fd);
>
> @@ -173,6 +171,4 @@ int main(int argc, char *argv[])
>
>         free(threads);
>         close(fd);
> -
> -       return 0;
>  }
> diff --git a/tests/gem_exec_blt.c b/tests/gem_exec_blt.c
> index 3bcef18..3d092fe 100644
> --- a/tests/gem_exec_blt.c
> +++ b/tests/gem_exec_blt.c
> @@ -253,12 +253,10 @@ static void run(int object_size)
>         close(fd);
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         int i;
>
> -       igt_simple_init();
> -
>         igt_skip_on_simulation();
>
>         if (argc > 1) {
> @@ -270,5 +268,4 @@ int main(int argc, char **argv)
>         } else
>                 run(OBJECT_SIZE);
>
> -       return 0;
>  }
> diff --git a/tests/gem_gtt_speed.c b/tests/gem_gtt_speed.c
> index 385eeb7..fa20de0 100644
> --- a/tests/gem_gtt_speed.c
> +++ b/tests/gem_gtt_speed.c
> @@ -50,7 +50,7 @@ static double elapsed(const struct timeval *start,
>         return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_usec - start->tv_usec))/loop;
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         struct timeval start, end;
>         uint8_t *buf;
> @@ -59,8 +59,6 @@ int main(int argc, char **argv)
>         int loop, i, tiling;
>         int fd;
>
> -       igt_simple_init();
> -
>         igt_skip_on_simulation();
>
>         if (argc > 1)
> @@ -329,5 +327,4 @@ int main(int argc, char **argv)
>         gem_close(fd, handle);
>         close(fd);
>
> -       return 0;


There are a couple of other returns that need to be replaced in this
test and in a few others, since the function igt_simple_main creates
returns void.


>  }
> diff --git a/tests/gem_hang.c b/tests/gem_hang.c
> index 6248244..a4f4d10 100644
> --- a/tests/gem_hang.c
> +++ b/tests/gem_hang.c
> @@ -68,12 +68,10 @@ gpu_hang(void)
>         intel_batchbuffer_flush(batch);
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         int fd;
>
> -       igt_simple_init();
> -
>         igt_assert_f(argc == 2,
>                      "usage: %s <disabled pipe number>\n",
>                      argv[0]);
> @@ -93,5 +91,4 @@ int main(int argc, char **argv)
>
>         close(fd);
>
> -       return 0;
>  }
> diff --git a/tests/gem_render_copy.c b/tests/gem_render_copy.c
> index fd26b43..12dd90d 100644
> --- a/tests/gem_render_copy.c
> +++ b/tests/gem_render_copy.c
> @@ -117,7 +117,7 @@ scratch_buf_check(data_t *data, struct igt_buf *buf, int x, int y,
>                      color, val, x, y);
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         data_t data = {0, };
>         struct intel_batchbuffer *batch = NULL;
> @@ -127,7 +127,6 @@ int main(int argc, char **argv)
>         int opt_dump_png = false;
>         int opt_dump_aub = igt_aub_dump_enabled();
>
> -       igt_simple_init();
>
>         while ((opt = getopt(argc, argv, "d")) != -1) {
>                 switch (opt) {
> @@ -189,5 +188,4 @@ int main(int argc, char **argv)
>                 scratch_buf_check(&data, &dst, WIDTH - 10, HEIGHT - 10, SRC_COLOR);
>         }
>
> -       return 0;
>  }
> diff --git a/tests/gem_render_linear_blits.c b/tests/gem_render_linear_blits.c
> index f847486..7c6081e 100644
> --- a/tests/gem_render_linear_blits.c
> +++ b/tests/gem_render_linear_blits.c
> @@ -81,7 +81,7 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>         }
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         drm_intel_bufmgr *bufmgr;
>         struct intel_batchbuffer *batch;
> @@ -90,8 +90,6 @@ int main(int argc, char **argv)
>         uint32_t start = 0;
>         int i, j, fd, count;
>
> -       igt_simple_init();
> -
>         fd = drm_open_any();
>
>         render_copy = igt_get_render_copyfunc(intel_get_drm_devid(fd));
> @@ -201,5 +199,4 @@ int main(int argc, char **argv)
>         for (i = 0; i < count; i++)
>                 check_bo(fd, bo[i]->handle, start_val[i]);
>
> -       return 0;
>  }
> diff --git a/tests/gem_render_tiled_blits.c b/tests/gem_render_tiled_blits.c
> index f63c57e..ec8e8de 100644
> --- a/tests/gem_render_tiled_blits.c
> +++ b/tests/gem_render_tiled_blits.c
> @@ -95,7 +95,7 @@ check_bo(struct intel_batchbuffer *batch, struct igt_buf *buf, uint32_t val)
>                 dri_bo_unmap(linear);
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         drm_intel_bufmgr *bufmgr;
>         struct intel_batchbuffer *batch;
> @@ -105,8 +105,6 @@ int main(int argc, char **argv)
>         int i, j, fd, count;
>         uint32_t devid;
>
> -       igt_simple_init();
> -
>         igt_skip_on_simulation();
>
>         fd = drm_open_any();
> @@ -212,5 +210,4 @@ int main(int argc, char **argv)
>         for (i = 0; i < count; i++)
>                 check_bo(batch, &buf[i], start_val[i]);
>
> -       return 0;
>  }
> diff --git a/tests/gem_seqno_wrap.c b/tests/gem_seqno_wrap.c
> index beda28b..8a54c2e 100644
> --- a/tests/gem_seqno_wrap.c
> +++ b/tests/gem_seqno_wrap.c
> @@ -533,12 +533,9 @@ static void parse_options(int argc, char **argv)
>         }
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         int wcount = 0;
> -       int r = -1;
> -
> -       igt_simple_init();
>
>         parse_options(argc, argv);
>
> @@ -563,8 +560,8 @@ int main(int argc, char **argv)
>
>         if (options.rounds == wcount) {
>                 igt_debug("done %d wraps successfully\n", wcount);
> -               return 0;
>         }
> -
> -       return r;
> +       else {
> +           igt_fail(-1);
> +       }
>  }
> diff --git a/tests/gem_stress.c b/tests/gem_stress.c
> index 2ccb6fc..35ed32f 100644
> --- a/tests/gem_stress.c
> +++ b/tests/gem_stress.c
> @@ -860,13 +860,11 @@ static void check_render_copyfunc(void)
>  }
>
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         int i, j;
>         unsigned *current_permutation, *tmp_permutation;
>
> -       igt_simple_init();
> -
>         drm_fd = drm_open_any();
>         devid = intel_get_drm_devid(drm_fd);
>
> @@ -925,5 +923,4 @@ int main(int argc, char **argv)
>
>         igt_stop_signal_helper();
>
> -       return 0;
>  }
> diff --git a/tests/gen3_mixed_blits.c b/tests/gen3_mixed_blits.c
> index 75d61a5..f0a6b64 100644
> --- a/tests/gen3_mixed_blits.c
> +++ b/tests/gen3_mixed_blits.c
> @@ -457,14 +457,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>         munmap(v, WIDTH*HEIGHT*4);
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         uint32_t *handle, *tiling, *start_val;
>         uint32_t start = 0;
>         int i, fd, count;
>
> -       igt_simple_init();
> -
>         fd = drm_open_any();
>
>         igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -533,5 +531,4 @@ int main(int argc, char **argv)
>                 check_bo(fd, handle[i], start_val[i]);
>         igt_info("done\n");
>
> -       return 0;
>  }
> diff --git a/tests/gen3_render_linear_blits.c b/tests/gen3_render_linear_blits.c
> index 7fe368d..60c0d0b 100644
> --- a/tests/gen3_render_linear_blits.c
> +++ b/tests/gen3_render_linear_blits.c
> @@ -325,14 +325,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>         }
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         uint32_t *handle, *start_val;
>         uint32_t start = 0;
>         int i, fd, count;
>
> -       igt_simple_init();
> -
>         fd = drm_open_any();
>
>         igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -393,5 +391,4 @@ int main(int argc, char **argv)
>         for (i = 0; i < count; i++)
>                 check_bo(fd, handle[i], start_val[i]);
>
> -       return 0;
>  }
> diff --git a/tests/gen3_render_mixed_blits.c b/tests/gen3_render_mixed_blits.c
> index 77ac0e2..68660a3 100644
> --- a/tests/gen3_render_mixed_blits.c
> +++ b/tests/gen3_render_mixed_blits.c
> @@ -345,14 +345,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>         munmap(v, WIDTH*HEIGHT*4);
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         uint32_t *handle, *tiling, *start_val;
>         uint32_t start = 0;
>         int i, fd, count;
>
> -       igt_simple_init();
> -
>         fd = drm_open_any();
>
>         igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -421,5 +419,4 @@ int main(int argc, char **argv)
>                 check_bo(fd, handle[i], start_val[i]);
>         igt_info("done\n");
>
> -       return 0;
>  }
> diff --git a/tests/gen3_render_tiledx_blits.c b/tests/gen3_render_tiledx_blits.c
> index 95c0c96..d54d714 100644
> --- a/tests/gen3_render_tiledx_blits.c
> +++ b/tests/gen3_render_tiledx_blits.c
> @@ -332,14 +332,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>         munmap(v, WIDTH*HEIGHT*4);
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         uint32_t *handle, *start_val;
>         uint32_t start = 0;
>         int i, fd, count;
>
> -       igt_simple_init();
> -
>         fd = drm_open_any();
>
>         igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -400,5 +398,4 @@ int main(int argc, char **argv)
>         for (i = 0; i < count; i++)
>                 check_bo(fd, handle[i], start_val[i]);
>
> -       return 0;
>  }
> diff --git a/tests/gen3_render_tiledy_blits.c b/tests/gen3_render_tiledy_blits.c
> index 1b9a419..3ef08c8 100644
> --- a/tests/gen3_render_tiledy_blits.c
> +++ b/tests/gen3_render_tiledy_blits.c
> @@ -332,14 +332,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>         munmap(v, WIDTH*HEIGHT*4);
>  }
>
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>         uint32_t *handle, *start_val;
>         uint32_t start = 0;
>         int i, fd, count;
>
> -       igt_simple_init();
> -
>         fd = drm_open_any();
>
>         igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -407,5 +405,4 @@ int main(int argc, char **argv)
>                 check_bo(fd, handle[i], start_val[i]);
>         igt_info("done\n");
>
> -       return 0;
>  }
> --
> 1.9.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests
  2014-06-27 14:15 ` [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests tim.gore
@ 2014-07-01  9:26   ` Thomas Wood
  2014-07-07 16:09   ` Daniel Vetter
  1 sibling, 0 replies; 12+ messages in thread
From: Thomas Wood @ 2014-07-01  9:26 UTC (permalink / raw)
  To: Tim Gore; +Cc: Intel Graphics Development

On 27 June 2014 15:15,  <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()                                                                        */

This note needs to be added to the documentation comment for
igt_simple_init so that it appears in the documentation.


> +       igt_subtest_init(argc, argv);
> +       test_with_subtests = false;
> +       if (list_subtests)
> +               igt_exit();

This should also exit with an error code if --run-subtest is used,
since there are no subtests.


>  }
>
>  /*
> @@ -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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests
  2014-06-27 14:15 ` [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests tim.gore
  2014-07-01  9:26   ` Thomas Wood
@ 2014-07-07 16:09   ` Daniel Vetter
  2014-07-09 13:23     ` Gore, Tim
  1 sibling, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2014-07-07 16:09 UTC (permalink / raw)
  To: tim.gore; +Cc: intel-gfx

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

Also I prefer if the piglit changes come together with this patch so that
we can roll it all out together.
-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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main
  2014-06-27 14:15 ` [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main tim.gore
  2014-07-01  9:19   ` Thomas Wood
@ 2014-07-07 16:12   ` Daniel Vetter
  2014-07-09 13:50     ` Gore, Tim
  1 sibling, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2014-07-07 16:12 UTC (permalink / raw)
  To: tim.gore; +Cc: intel-gfx

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

>  
>  __attribute__((format(printf, 1, 2)))
>  void igt_skip(const char *f, ...) __attribute__((noreturn));
> diff --git a/tests/gem_ctx_basic.c b/tests/gem_ctx_basic.c
> index 3e9b688..fe770ea 100644
> --- a/tests/gem_ctx_basic.c
> +++ b/tests/gem_ctx_basic.c
> @@ -145,12 +145,10 @@ static void parse(int argc, char *argv[])
>  	}
>  }
>  
> -int main(int argc, char *argv[])
> +igt_simple_main
>  {
>  	int i;
>  
> -	igt_simple_init();
> -
>  	fd = drm_open_any_render();
>  	devid = intel_get_drm_devid(fd);
>  
> @@ -173,6 +171,4 @@ int main(int argc, char *argv[])
>  
>  	free(threads);
>  	close(fd);
> -
> -	return 0;
>  }
> diff --git a/tests/gem_exec_blt.c b/tests/gem_exec_blt.c
> index 3bcef18..3d092fe 100644
> --- a/tests/gem_exec_blt.c
> +++ b/tests/gem_exec_blt.c
> @@ -253,12 +253,10 @@ static void run(int object_size)
>  	close(fd);
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	int i;
>  
> -	igt_simple_init();
> -
>  	igt_skip_on_simulation();
>  
>  	if (argc > 1) {
> @@ -270,5 +268,4 @@ int main(int argc, char **argv)
>  	} else
>  		run(OBJECT_SIZE);
>  
> -	return 0;
>  }
> diff --git a/tests/gem_gtt_speed.c b/tests/gem_gtt_speed.c
> index 385eeb7..fa20de0 100644
> --- a/tests/gem_gtt_speed.c
> +++ b/tests/gem_gtt_speed.c
> @@ -50,7 +50,7 @@ static double elapsed(const struct timeval *start,
>  	return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_usec - start->tv_usec))/loop;
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	struct timeval start, end;
>  	uint8_t *buf;
> @@ -59,8 +59,6 @@ int main(int argc, char **argv)
>  	int loop, i, tiling;
>  	int fd;
>  
> -	igt_simple_init();
> -
>  	igt_skip_on_simulation();
>  
>  	if (argc > 1)
> @@ -329,5 +327,4 @@ int main(int argc, char **argv)
>  	gem_close(fd, handle);
>  	close(fd);
>  
> -	return 0;
>  }
> diff --git a/tests/gem_hang.c b/tests/gem_hang.c
> index 6248244..a4f4d10 100644
> --- a/tests/gem_hang.c
> +++ b/tests/gem_hang.c
> @@ -68,12 +68,10 @@ gpu_hang(void)
>  	intel_batchbuffer_flush(batch);
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	int fd;
>  
> -	igt_simple_init();
> -
>  	igt_assert_f(argc == 2,
>  		     "usage: %s <disabled pipe number>\n",
>  		     argv[0]);
> @@ -93,5 +91,4 @@ int main(int argc, char **argv)
>  
>  	close(fd);
>  
> -	return 0;
>  }
> diff --git a/tests/gem_render_copy.c b/tests/gem_render_copy.c
> index fd26b43..12dd90d 100644
> --- a/tests/gem_render_copy.c
> +++ b/tests/gem_render_copy.c
> @@ -117,7 +117,7 @@ scratch_buf_check(data_t *data, struct igt_buf *buf, int x, int y,
>  		     color, val, x, y);
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	data_t data = {0, };
>  	struct intel_batchbuffer *batch = NULL;
> @@ -127,7 +127,6 @@ int main(int argc, char **argv)
>  	int opt_dump_png = false;
>  	int opt_dump_aub = igt_aub_dump_enabled();
>  
> -	igt_simple_init();
>  
>  	while ((opt = getopt(argc, argv, "d")) != -1) {
>  		switch (opt) {
> @@ -189,5 +188,4 @@ int main(int argc, char **argv)
>  		scratch_buf_check(&data, &dst, WIDTH - 10, HEIGHT - 10, SRC_COLOR);
>  	}
>  
> -	return 0;
>  }
> diff --git a/tests/gem_render_linear_blits.c b/tests/gem_render_linear_blits.c
> index f847486..7c6081e 100644
> --- a/tests/gem_render_linear_blits.c
> +++ b/tests/gem_render_linear_blits.c
> @@ -81,7 +81,7 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>  	}
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	drm_intel_bufmgr *bufmgr;
>  	struct intel_batchbuffer *batch;
> @@ -90,8 +90,6 @@ int main(int argc, char **argv)
>  	uint32_t start = 0;
>  	int i, j, fd, count;
>  
> -	igt_simple_init();
> -
>  	fd = drm_open_any();
>  
>  	render_copy = igt_get_render_copyfunc(intel_get_drm_devid(fd));
> @@ -201,5 +199,4 @@ int main(int argc, char **argv)
>  	for (i = 0; i < count; i++)
>  		check_bo(fd, bo[i]->handle, start_val[i]);
>  
> -	return 0;
>  }
> diff --git a/tests/gem_render_tiled_blits.c b/tests/gem_render_tiled_blits.c
> index f63c57e..ec8e8de 100644
> --- a/tests/gem_render_tiled_blits.c
> +++ b/tests/gem_render_tiled_blits.c
> @@ -95,7 +95,7 @@ check_bo(struct intel_batchbuffer *batch, struct igt_buf *buf, uint32_t val)
>  		dri_bo_unmap(linear);
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	drm_intel_bufmgr *bufmgr;
>  	struct intel_batchbuffer *batch;
> @@ -105,8 +105,6 @@ int main(int argc, char **argv)
>  	int i, j, fd, count;
>  	uint32_t devid;
>  
> -	igt_simple_init();
> -
>  	igt_skip_on_simulation();
>  
>  	fd = drm_open_any();
> @@ -212,5 +210,4 @@ int main(int argc, char **argv)
>  	for (i = 0; i < count; i++)
>  		check_bo(batch, &buf[i], start_val[i]);
>  
> -	return 0;
>  }
> diff --git a/tests/gem_seqno_wrap.c b/tests/gem_seqno_wrap.c
> index beda28b..8a54c2e 100644
> --- a/tests/gem_seqno_wrap.c
> +++ b/tests/gem_seqno_wrap.c
> @@ -533,12 +533,9 @@ static void parse_options(int argc, char **argv)
>  	}
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	int wcount = 0;
> -	int r = -1;
> -
> -	igt_simple_init();
>  
>  	parse_options(argc, argv);
>  
> @@ -563,8 +560,8 @@ int main(int argc, char **argv)
>  
>  	if (options.rounds == wcount) {
>  		igt_debug("done %d wraps successfully\n", wcount);
> -		return 0;
>  	}
> -
> -	return r;
> +	else {
> +	    igt_fail(-1);
> +	}
>  }
> diff --git a/tests/gem_stress.c b/tests/gem_stress.c
> index 2ccb6fc..35ed32f 100644
> --- a/tests/gem_stress.c
> +++ b/tests/gem_stress.c
> @@ -860,13 +860,11 @@ static void check_render_copyfunc(void)
>  }
>  
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	int i, j;
>  	unsigned *current_permutation, *tmp_permutation;
>  
> -	igt_simple_init();
> -
>  	drm_fd = drm_open_any();
>  	devid = intel_get_drm_devid(drm_fd);
>  
> @@ -925,5 +923,4 @@ int main(int argc, char **argv)
>  
>  	igt_stop_signal_helper();
>  
> -	return 0;
>  }
> diff --git a/tests/gen3_mixed_blits.c b/tests/gen3_mixed_blits.c
> index 75d61a5..f0a6b64 100644
> --- a/tests/gen3_mixed_blits.c
> +++ b/tests/gen3_mixed_blits.c
> @@ -457,14 +457,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>  	munmap(v, WIDTH*HEIGHT*4);
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	uint32_t *handle, *tiling, *start_val;
>  	uint32_t start = 0;
>  	int i, fd, count;
>  
> -	igt_simple_init();
> -
>  	fd = drm_open_any();
>  
>  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -533,5 +531,4 @@ int main(int argc, char **argv)
>  		check_bo(fd, handle[i], start_val[i]);
>  	igt_info("done\n");
>  
> -	return 0;
>  }
> diff --git a/tests/gen3_render_linear_blits.c b/tests/gen3_render_linear_blits.c
> index 7fe368d..60c0d0b 100644
> --- a/tests/gen3_render_linear_blits.c
> +++ b/tests/gen3_render_linear_blits.c
> @@ -325,14 +325,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>  	}
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	uint32_t *handle, *start_val;
>  	uint32_t start = 0;
>  	int i, fd, count;
>  
> -	igt_simple_init();
> -
>  	fd = drm_open_any();
>  
>  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -393,5 +391,4 @@ int main(int argc, char **argv)
>  	for (i = 0; i < count; i++)
>  		check_bo(fd, handle[i], start_val[i]);
>  
> -	return 0;
>  }
> diff --git a/tests/gen3_render_mixed_blits.c b/tests/gen3_render_mixed_blits.c
> index 77ac0e2..68660a3 100644
> --- a/tests/gen3_render_mixed_blits.c
> +++ b/tests/gen3_render_mixed_blits.c
> @@ -345,14 +345,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>  	munmap(v, WIDTH*HEIGHT*4);
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	uint32_t *handle, *tiling, *start_val;
>  	uint32_t start = 0;
>  	int i, fd, count;
>  
> -	igt_simple_init();
> -
>  	fd = drm_open_any();
>  
>  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -421,5 +419,4 @@ int main(int argc, char **argv)
>  		check_bo(fd, handle[i], start_val[i]);
>  	igt_info("done\n");
>  
> -	return 0;
>  }
> diff --git a/tests/gen3_render_tiledx_blits.c b/tests/gen3_render_tiledx_blits.c
> index 95c0c96..d54d714 100644
> --- a/tests/gen3_render_tiledx_blits.c
> +++ b/tests/gen3_render_tiledx_blits.c
> @@ -332,14 +332,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>  	munmap(v, WIDTH*HEIGHT*4);
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	uint32_t *handle, *start_val;
>  	uint32_t start = 0;
>  	int i, fd, count;
>  
> -	igt_simple_init();
> -
>  	fd = drm_open_any();
>  
>  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -400,5 +398,4 @@ int main(int argc, char **argv)
>  	for (i = 0; i < count; i++)
>  		check_bo(fd, handle[i], start_val[i]);
>  
> -	return 0;
>  }
> diff --git a/tests/gen3_render_tiledy_blits.c b/tests/gen3_render_tiledy_blits.c
> index 1b9a419..3ef08c8 100644
> --- a/tests/gen3_render_tiledy_blits.c
> +++ b/tests/gen3_render_tiledy_blits.c
> @@ -332,14 +332,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
>  	munmap(v, WIDTH*HEIGHT*4);
>  }
>  
> -int main(int argc, char **argv)
> +igt_simple_main
>  {
>  	uint32_t *handle, *start_val;
>  	uint32_t start = 0;
>  	int i, fd, count;
>  
> -	igt_simple_init();
> -
>  	fd = drm_open_any();
>  
>  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> @@ -407,5 +405,4 @@ int main(int argc, char **argv)
>  		check_bo(fd, handle[i], start_val[i]);
>  	igt_info("done\n");
>  
> -	return 0;
>  }
> -- 
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests
  2014-07-07 16:09   ` Daniel Vetter
@ 2014-07-09 13:23     ` Gore, Tim
  2014-07-09 14:23       ` Daniel Vetter
  0 siblings, 1 reply; 12+ messages in thread
From: Gore, Tim @ 2014-07-09 13:23 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main
  2014-07-07 16:12   ` Daniel Vetter
@ 2014-07-09 13:50     ` Gore, Tim
  2014-07-09 14:20       ` Daniel Vetter
  0 siblings, 1 reply; 12+ messages in thread
From: Gore, Tim @ 2014-07-09 13:50 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

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.

       Tim
> 
> >
> >  __attribute__((format(printf, 1, 2)))  void igt_skip(const char *f,
> > ...) __attribute__((noreturn)); diff --git a/tests/gem_ctx_basic.c
> > b/tests/gem_ctx_basic.c index 3e9b688..fe770ea 100644
> > --- a/tests/gem_ctx_basic.c
> > +++ b/tests/gem_ctx_basic.c
> > @@ -145,12 +145,10 @@ static void parse(int argc, char *argv[])
> >  	}
> >  }
> >
> > -int main(int argc, char *argv[])
> > +igt_simple_main
> >  {
> >  	int i;
> >
> > -	igt_simple_init();
> > -
> >  	fd = drm_open_any_render();
> >  	devid = intel_get_drm_devid(fd);
> >
> > @@ -173,6 +171,4 @@ int main(int argc, char *argv[])
> >
> >  	free(threads);
> >  	close(fd);
> > -
> > -	return 0;
> >  }
> > diff --git a/tests/gem_exec_blt.c b/tests/gem_exec_blt.c index
> > 3bcef18..3d092fe 100644
> > --- a/tests/gem_exec_blt.c
> > +++ b/tests/gem_exec_blt.c
> > @@ -253,12 +253,10 @@ static void run(int object_size)
> >  	close(fd);
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	int i;
> >
> > -	igt_simple_init();
> > -
> >  	igt_skip_on_simulation();
> >
> >  	if (argc > 1) {
> > @@ -270,5 +268,4 @@ int main(int argc, char **argv)
> >  	} else
> >  		run(OBJECT_SIZE);
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gem_gtt_speed.c b/tests/gem_gtt_speed.c index
> > 385eeb7..fa20de0 100644
> > --- a/tests/gem_gtt_speed.c
> > +++ b/tests/gem_gtt_speed.c
> > @@ -50,7 +50,7 @@ static double elapsed(const struct timeval *start,
> >  	return (1e6*(end->tv_sec - start->tv_sec) + (end->tv_usec -
> > start->tv_usec))/loop;  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	struct timeval start, end;
> >  	uint8_t *buf;
> > @@ -59,8 +59,6 @@ int main(int argc, char **argv)
> >  	int loop, i, tiling;
> >  	int fd;
> >
> > -	igt_simple_init();
> > -
> >  	igt_skip_on_simulation();
> >
> >  	if (argc > 1)
> > @@ -329,5 +327,4 @@ int main(int argc, char **argv)
> >  	gem_close(fd, handle);
> >  	close(fd);
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gem_hang.c b/tests/gem_hang.c index
> > 6248244..a4f4d10 100644
> > --- a/tests/gem_hang.c
> > +++ b/tests/gem_hang.c
> > @@ -68,12 +68,10 @@ gpu_hang(void)
> >  	intel_batchbuffer_flush(batch);
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	int fd;
> >
> > -	igt_simple_init();
> > -
> >  	igt_assert_f(argc == 2,
> >  		     "usage: %s <disabled pipe number>\n",
> >  		     argv[0]);
> > @@ -93,5 +91,4 @@ int main(int argc, char **argv)
> >
> >  	close(fd);
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gem_render_copy.c b/tests/gem_render_copy.c index
> > fd26b43..12dd90d 100644
> > --- a/tests/gem_render_copy.c
> > +++ b/tests/gem_render_copy.c
> > @@ -117,7 +117,7 @@ scratch_buf_check(data_t *data, struct igt_buf
> *buf, int x, int y,
> >  		     color, val, x, y);
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	data_t data = {0, };
> >  	struct intel_batchbuffer *batch = NULL; @@ -127,7 +127,6 @@ int
> > main(int argc, char **argv)
> >  	int opt_dump_png = false;
> >  	int opt_dump_aub = igt_aub_dump_enabled();
> >
> > -	igt_simple_init();
> >
> >  	while ((opt = getopt(argc, argv, "d")) != -1) {
> >  		switch (opt) {
> > @@ -189,5 +188,4 @@ int main(int argc, char **argv)
> >  		scratch_buf_check(&data, &dst, WIDTH - 10, HEIGHT - 10,
> SRC_COLOR);
> >  	}
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gem_render_linear_blits.c
> > b/tests/gem_render_linear_blits.c index f847486..7c6081e 100644
> > --- a/tests/gem_render_linear_blits.c
> > +++ b/tests/gem_render_linear_blits.c
> > @@ -81,7 +81,7 @@ check_bo(int fd, uint32_t handle, uint32_t val)
> >  	}
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	drm_intel_bufmgr *bufmgr;
> >  	struct intel_batchbuffer *batch;
> > @@ -90,8 +90,6 @@ int main(int argc, char **argv)
> >  	uint32_t start = 0;
> >  	int i, j, fd, count;
> >
> > -	igt_simple_init();
> > -
> >  	fd = drm_open_any();
> >
> >  	render_copy = igt_get_render_copyfunc(intel_get_drm_devid(fd));
> > @@ -201,5 +199,4 @@ int main(int argc, char **argv)
> >  	for (i = 0; i < count; i++)
> >  		check_bo(fd, bo[i]->handle, start_val[i]);
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gem_render_tiled_blits.c
> > b/tests/gem_render_tiled_blits.c index f63c57e..ec8e8de 100644
> > --- a/tests/gem_render_tiled_blits.c
> > +++ b/tests/gem_render_tiled_blits.c
> > @@ -95,7 +95,7 @@ check_bo(struct intel_batchbuffer *batch, struct
> igt_buf *buf, uint32_t val)
> >  		dri_bo_unmap(linear);
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	drm_intel_bufmgr *bufmgr;
> >  	struct intel_batchbuffer *batch;
> > @@ -105,8 +105,6 @@ int main(int argc, char **argv)
> >  	int i, j, fd, count;
> >  	uint32_t devid;
> >
> > -	igt_simple_init();
> > -
> >  	igt_skip_on_simulation();
> >
> >  	fd = drm_open_any();
> > @@ -212,5 +210,4 @@ int main(int argc, char **argv)
> >  	for (i = 0; i < count; i++)
> >  		check_bo(batch, &buf[i], start_val[i]);
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gem_seqno_wrap.c b/tests/gem_seqno_wrap.c index
> > beda28b..8a54c2e 100644
> > --- a/tests/gem_seqno_wrap.c
> > +++ b/tests/gem_seqno_wrap.c
> > @@ -533,12 +533,9 @@ static void parse_options(int argc, char **argv)
> >  	}
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	int wcount = 0;
> > -	int r = -1;
> > -
> > -	igt_simple_init();
> >
> >  	parse_options(argc, argv);
> >
> > @@ -563,8 +560,8 @@ int main(int argc, char **argv)
> >
> >  	if (options.rounds == wcount) {
> >  		igt_debug("done %d wraps successfully\n", wcount);
> > -		return 0;
> >  	}
> > -
> > -	return r;
> > +	else {
> > +	    igt_fail(-1);
> > +	}
> >  }
> > diff --git a/tests/gem_stress.c b/tests/gem_stress.c index
> > 2ccb6fc..35ed32f 100644
> > --- a/tests/gem_stress.c
> > +++ b/tests/gem_stress.c
> > @@ -860,13 +860,11 @@ static void check_render_copyfunc(void)  }
> >
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	int i, j;
> >  	unsigned *current_permutation, *tmp_permutation;
> >
> > -	igt_simple_init();
> > -
> >  	drm_fd = drm_open_any();
> >  	devid = intel_get_drm_devid(drm_fd);
> >
> > @@ -925,5 +923,4 @@ int main(int argc, char **argv)
> >
> >  	igt_stop_signal_helper();
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gen3_mixed_blits.c b/tests/gen3_mixed_blits.c index
> > 75d61a5..f0a6b64 100644
> > --- a/tests/gen3_mixed_blits.c
> > +++ b/tests/gen3_mixed_blits.c
> > @@ -457,14 +457,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
> >  	munmap(v, WIDTH*HEIGHT*4);
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	uint32_t *handle, *tiling, *start_val;
> >  	uint32_t start = 0;
> >  	int i, fd, count;
> >
> > -	igt_simple_init();
> > -
> >  	fd = drm_open_any();
> >
> >  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> > @@ -533,5 +531,4 @@ int main(int argc, char **argv)
> >  		check_bo(fd, handle[i], start_val[i]);
> >  	igt_info("done\n");
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gen3_render_linear_blits.c
> > b/tests/gen3_render_linear_blits.c
> > index 7fe368d..60c0d0b 100644
> > --- a/tests/gen3_render_linear_blits.c
> > +++ b/tests/gen3_render_linear_blits.c
> > @@ -325,14 +325,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
> >  	}
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	uint32_t *handle, *start_val;
> >  	uint32_t start = 0;
> >  	int i, fd, count;
> >
> > -	igt_simple_init();
> > -
> >  	fd = drm_open_any();
> >
> >  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> > @@ -393,5 +391,4 @@ int main(int argc, char **argv)
> >  	for (i = 0; i < count; i++)
> >  		check_bo(fd, handle[i], start_val[i]);
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gen3_render_mixed_blits.c
> > b/tests/gen3_render_mixed_blits.c index 77ac0e2..68660a3 100644
> > --- a/tests/gen3_render_mixed_blits.c
> > +++ b/tests/gen3_render_mixed_blits.c
> > @@ -345,14 +345,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
> >  	munmap(v, WIDTH*HEIGHT*4);
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	uint32_t *handle, *tiling, *start_val;
> >  	uint32_t start = 0;
> >  	int i, fd, count;
> >
> > -	igt_simple_init();
> > -
> >  	fd = drm_open_any();
> >
> >  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> > @@ -421,5 +419,4 @@ int main(int argc, char **argv)
> >  		check_bo(fd, handle[i], start_val[i]);
> >  	igt_info("done\n");
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gen3_render_tiledx_blits.c
> > b/tests/gen3_render_tiledx_blits.c
> > index 95c0c96..d54d714 100644
> > --- a/tests/gen3_render_tiledx_blits.c
> > +++ b/tests/gen3_render_tiledx_blits.c
> > @@ -332,14 +332,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
> >  	munmap(v, WIDTH*HEIGHT*4);
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	uint32_t *handle, *start_val;
> >  	uint32_t start = 0;
> >  	int i, fd, count;
> >
> > -	igt_simple_init();
> > -
> >  	fd = drm_open_any();
> >
> >  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> > @@ -400,5 +398,4 @@ int main(int argc, char **argv)
> >  	for (i = 0; i < count; i++)
> >  		check_bo(fd, handle[i], start_val[i]);
> >
> > -	return 0;
> >  }
> > diff --git a/tests/gen3_render_tiledy_blits.c
> > b/tests/gen3_render_tiledy_blits.c
> > index 1b9a419..3ef08c8 100644
> > --- a/tests/gen3_render_tiledy_blits.c
> > +++ b/tests/gen3_render_tiledy_blits.c
> > @@ -332,14 +332,12 @@ check_bo(int fd, uint32_t handle, uint32_t val)
> >  	munmap(v, WIDTH*HEIGHT*4);
> >  }
> >
> > -int main(int argc, char **argv)
> > +igt_simple_main
> >  {
> >  	uint32_t *handle, *start_val;
> >  	uint32_t start = 0;
> >  	int i, fd, count;
> >
> > -	igt_simple_init();
> > -
> >  	fd = drm_open_any();
> >
> >  	igt_require(IS_GEN3(intel_get_drm_devid(fd)));
> > @@ -407,5 +405,4 @@ int main(int argc, char **argv)
> >  		check_bo(fd, handle[i], start_val[i]);
> >  	igt_info("done\n");
> >
> > -	return 0;
> >  }
> > --
> > 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main
  2014-07-09 13:50     ` Gore, Tim
@ 2014-07-09 14:20       ` Daniel Vetter
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2014-07-09 14:20 UTC (permalink / raw)
  To: Gore, Tim; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests
  2014-07-09 13:23     ` Gore, Tim
@ 2014-07-09 14:23       ` Daniel Vetter
  2014-07-09 16:42         ` Damien Lespiau
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2014-07-09 14:23 UTC (permalink / raw)
  To: Gore, Tim; +Cc: intel-gfx

On Wed, Jul 09, 2014 at 01:23:46PM +0000, Gore, Tim wrote:
> Some comments on Daniels' comments inline
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > 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.

Well under the hood subtests work with longjmps when skipping/failing
tests while simple tests directly call exit. The reason for that is that
historically we've started with simple binaries and slowly grew the igt
infrastructure on top of them. I don't really see much benefits in
converting the last stragglers.

> > 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.

For me the only trouble with this disdinction is that people consistently
place tests in the wrong Makefile target. Hence I'd like to see those two
Makefile targets being unified (which requires adjustements in piglit,
too) to validate this work. Iirc Thomas is also working on this.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests
  2014-07-09 14:23       ` Daniel Vetter
@ 2014-07-09 16:42         ` Damien Lespiau
  0 siblings, 0 replies; 12+ messages in thread
From: Damien Lespiau @ 2014-07-09 16:42 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Wed, Jul 09, 2014 at 04:23:50PM +0200, Daniel Vetter wrote:
> For me the only trouble with this disdinction is that people consistently
> place tests in the wrong Makefile target. Hence I'd like to see those two
> Makefile targets being unified (which requires adjustements in piglit,
> too) to validate this work. Iirc Thomas is also working on this.

In all generalities, I think it makes things a lot simpler if we only
one case to care about. In this instance:

  "A test is a set of one or more subtests"

Seems a good invariant to me. Not sure I understand what the objections
are.

-- 
Damien

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2014-07-09 16:43 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-27 14:15 [PATCH 0/2] Single tests to respond to --list-subtests tim.gore
2014-06-27 14:15 ` [PATCH 1/2] intel-gpu-tools: pass argc/argv to simple main tim.gore
2014-07-01  9:19   ` Thomas Wood
2014-07-07 16:12   ` Daniel Vetter
2014-07-09 13:50     ` Gore, Tim
2014-07-09 14:20       ` Daniel Vetter
2014-06-27 14:15 ` [PATCH 2/2] intel-gpu-tools: Re-use igt_subtest_init for simple tests tim.gore
2014-07-01  9:26   ` Thomas Wood
2014-07-07 16:09   ` Daniel Vetter
2014-07-09 13:23     ` Gore, Tim
2014-07-09 14:23       ` Daniel Vetter
2014-07-09 16:42         ` Damien Lespiau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.