[PATCH i-g-t 1/8] kms_force_connector: use comparison macros to make debug output clearer

[PATCH i-g-t 1/8] kms_force_connector: use comparison macros to make debug output clearer

[Thomas Wood]

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 tests/kms_force_connector.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/tests/kms_force_connector.c b/tests/kms_force_connector.c
index 7485ca8..4ba1e0b 100644
--- a/tests/kms_force_connector.c
+++ b/tests/kms_force_connector.c
@@ -27,8 +27,9 @@
 IGT_TEST_DESCRIPTION("Check the debugfs force connector/edid features work"
 		     " correctly.");
 
-#define CHECK_MODE(m, h, w, r) igt_assert(m.hdisplay == h && m.vdisplay == w \
-					  && m.vrefresh == r)
+#define CHECK_MODE(m, h, w, r) \
+	igt_assert_eq(m.hdisplay, h); igt_assert_eq(m.vdisplay, w); \
+	igt_assert_eq(m.vrefresh, r);
 
 igt_main
 {
@@ -63,8 +64,8 @@ igt_main
 		/* force the connector on and check the reported values */
 		kmstest_force_connector(drm_fd, vga_connector, FORCE_CONNECTOR_ON);
 		temp = drmModeGetConnector(drm_fd, vga_connector->connector_id);
-		igt_assert(temp->connection == DRM_MODE_CONNECTED);
-		igt_assert(temp->count_modes > 0);
+		igt_assert_eq(temp->connection, DRM_MODE_CONNECTED);
+		igt_assert_lt(0, temp->count_modes);
 		drmModeFreeConnector(temp);
 
 		/* attempt to use the display */
@@ -77,15 +78,15 @@ igt_main
 		kmstest_force_connector(drm_fd, vga_connector,
 					FORCE_CONNECTOR_OFF);
 		temp = drmModeGetConnector(drm_fd, vga_connector->connector_id);
-		igt_assert(temp->connection == DRM_MODE_DISCONNECTED);
-		igt_assert(temp->count_modes == 0);
+		igt_assert_eq(temp->connection, DRM_MODE_DISCONNECTED);
+		igt_assert_lt(0, temp->count_modes);
 		drmModeFreeConnector(temp);
 
 		/* check that the previous state is restored */
 		kmstest_force_connector(drm_fd, vga_connector,
 					FORCE_CONNECTOR_UNSPECIFIED);
 		temp = drmModeGetConnector(drm_fd, vga_connector->connector_id);
-		igt_assert(temp->connection == vga_connector->connection);
+		igt_assert_eq(temp->connection, vga_connector->connection);
 		drmModeFreeConnector(temp);
 	}
 
@@ -115,7 +116,7 @@ igt_main
 		temp = drmModeGetConnector(drm_fd, vga_connector->connector_id);
 		/* the connector should now have the same number of modes that
 		 * it started with */
-		igt_assert(temp->count_modes == start_n_modes);
+		igt_assert_eq(temp->count_modes, start_n_modes);
 		drmModeFreeConnector(temp);
 
 		kmstest_force_connector(drm_fd, vga_connector,
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 2/8] lib: highlight subtest results on terminals

[Thomas Wood]

Make subtest results easier to identify by making them bold when the output
is a terminal.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 59127ca..7123455 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -779,9 +779,12 @@ bool __igt_run_subtest(const char *subtest_name)
 	}
 
 	if (skip_subtests_henceforth) {
-		printf("Subtest %s: %s\n", subtest_name,
+		bool istty = isatty(STDOUT_FILENO);
+
+		printf("%sSubtest %s: %s%s\n",
+		       (istty) ? "\x1b[1m" : "", subtest_name,
 		       skip_subtests_henceforth == SKIP ?
-		       "SKIP" : "FAIL");
+		       "SKIP" : "FAIL", (istty) ? "\x1b[0m" : "");
 		return false;
 	}
 
@@ -825,12 +828,14 @@ static void exit_subtest(const char *result)
 {
 	struct timespec now;
 	double elapsed;
+	bool istty = isatty(STDOUT_FILENO);
 
 	gettime(&now);
 	elapsed = now.tv_sec - subtest_time.tv_sec;
 	elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
 
-	printf("Subtest %s: %s (%.3fs)\n", in_subtest, result, elapsed);
+	printf("%sSubtest %s: %s (%.3fs)%s\n", (istty) ? "\x1b[1m" : "",
+	       in_subtest, result, elapsed, (istty) ? "\x1b[0m" : "");
 	fflush(stdout);
 
 	in_subtest = NULL;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 3/8] Add missing noreturn attribute to various functions

[Thomas Wood]

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 debugger/eudb.c              | 2 +-
 lib/igt_core.c               | 2 +-
 tests/gem_madvise.c          | 2 +-
 tests/pm_rpm.c               | 2 +-
 tests/testdisplay.c          | 2 +-
 tools/intel_display_poller.c | 2 +-
 tools/intel_gpu_frequency.c  | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/debugger/eudb.c b/debugger/eudb.c
index e015e2e..47d5d92 100644
--- a/debugger/eudb.c
+++ b/debugger/eudb.c
@@ -326,7 +326,7 @@ db_shutdown(int sig) {
 	printf("Shutting down...\n");
 }
 
-static void
+static void __attribute__((noreturn))
 die(int reason) {
 	int i = 0;
 
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 7123455..7e99b24 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1755,7 +1755,7 @@ out:
 }
 
 static const char *timeout_op;
-static void igt_alarm_handler(int signal)
+static void __attribute__((noreturn)) igt_alarm_handler(int signal)
 {
 	if (timeout_op)
 		igt_info("Timed out: %s\n", timeout_op);
diff --git a/tests/gem_madvise.c b/tests/gem_madvise.c
index 093d78a..36408fe 100644
--- a/tests/gem_madvise.c
+++ b/tests/gem_madvise.c
@@ -49,7 +49,7 @@ IGT_TEST_DESCRIPTION("Checks that the kernel reports EFAULT when trying to use"
 
 static jmp_buf jmp;
 
-static void sigtrap(int sig)
+static void __attribute__((noreturn)) sigtrap(int sig)
 {
 	longjmp(jmp, sig);
 }
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index d43cc06..c4fb19c 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1386,7 +1386,7 @@ static void pci_d3_state_subtest(void)
 	igt_assert(!device_in_pci_d3());
 }
 
-static void stay_subtest(void)
+static void __attribute__((noreturn)) stay_subtest(void)
 {
 	disable_all_screens_and_wait(&ms_data);
 
diff --git a/tests/testdisplay.c b/tests/testdisplay.c
index 4efcb59..28875b2 100644
--- a/tests/testdisplay.c
+++ b/tests/testdisplay.c
@@ -552,7 +552,7 @@ static void __attribute__((noreturn)) usage(char *name, char opt)
 
 #define dump_resource(res) if (res) dump_##res()
 
-static void cleanup_and_exit(int ret)
+static void __attribute__((noreturn)) cleanup_and_exit(int ret)
 {
 	close(drm_fd);
 	exit(ret);
diff --git a/tools/intel_display_poller.c b/tools/intel_display_poller.c
index 56dcd44..eab17c5 100644
--- a/tools/intel_display_poller.c
+++ b/tools/intel_display_poller.c
@@ -946,7 +946,7 @@ static const char *test_name(enum test test, int pipe, int bit, bool test_pixel_
 	}
 }
 
-static void usage(const char *name)
+static void __attribute__((noreturn)) usage(const char *name)
 {
 	fprintf(stderr, "Usage: %s [options]\n"
 		" -t,--test <pipestat|iir|framecount|flipcount|pan|flip|surflive|wrap|field>\n"
diff --git a/tools/intel_gpu_frequency.c b/tools/intel_gpu_frequency.c
index 5ae47e7..cb758b0 100644
--- a/tools/intel_gpu_frequency.c
+++ b/tools/intel_gpu_frequency.c
@@ -142,7 +142,7 @@ static int get_frequency(struct freq_info *freq_info)
 	return val;
 }
 
-static void
+static void __attribute__((noreturn))
 usage(const char *prog)
 {
 	printf("%s A program to manipulate Intel GPU frequencies.\n\n", prog);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 4/8] tests: remove unnecessary igt_exit calls

[Thomas Wood]

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 tests/gem_render_linear_blits.c | 2 --
 tests/gem_render_tiled_blits.c  | 2 --
 tests/kms_3d.c                  | 2 --
 tests/kms_force_connector.c     | 2 --
 4 files changed, 8 deletions(-)

diff --git a/tests/gem_render_linear_blits.c b/tests/gem_render_linear_blits.c
index 8df180a..13f76a5 100644
--- a/tests/gem_render_linear_blits.c
+++ b/tests/gem_render_linear_blits.c
@@ -210,6 +210,4 @@ igt_main
 		intel_require_memory(count, SIZE, CHECK_RAM | CHECK_SWAP);
 		run_test(fd, count);
 	}
-
-	igt_exit();
 }
diff --git a/tests/gem_render_tiled_blits.c b/tests/gem_render_tiled_blits.c
index 9f6cbd5..fb2f39d 100644
--- a/tests/gem_render_tiled_blits.c
+++ b/tests/gem_render_tiled_blits.c
@@ -223,6 +223,4 @@ igt_main
 		intel_require_memory(count, SIZE, CHECK_RAM | CHECK_SWAP);
 		run_test(fd, count);
 	}
-
-	igt_exit();
 }
diff --git a/tests/kms_3d.c b/tests/kms_3d.c
index 642a3d6..7484940 100644
--- a/tests/kms_3d.c
+++ b/tests/kms_3d.c
@@ -115,6 +115,4 @@ igt_simple_main
 
 	drmModeFreeConnector(connector);
 	free(edid);
-
-	igt_exit();
 }
diff --git a/tests/kms_force_connector.c b/tests/kms_force_connector.c
index 4ba1e0b..6b0cb34 100644
--- a/tests/kms_force_connector.c
+++ b/tests/kms_force_connector.c
@@ -126,6 +126,4 @@ igt_main
 	igt_fixture {
 		drmModeFreeConnector(vga_connector);
 	}
-
-	igt_exit();
 }
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 5/8] tests: remove duplicate struct member initializers

[Thomas Wood]

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 1 -
 tests/pm_lpsp.c                  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 15707b9..13b91ad 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -305,7 +305,6 @@ drmModeModeInfo std_1024_mode = {
 	.hsync_start = 1048,
 	.hsync_end = 1184,
 	.htotal = 1344,
-	.vtotal = 806,
 	.hskew = 0,
 	.vdisplay = 768,
 	.vsync_start = 771,
diff --git a/tests/pm_lpsp.c b/tests/pm_lpsp.c
index 2badb5c..b62876c 100644
--- a/tests/pm_lpsp.c
+++ b/tests/pm_lpsp.c
@@ -103,7 +103,6 @@ static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
 		.hsync_start = 1048,
 		.hsync_end = 1184,
 		.htotal = 1344,
-		.vtotal = 806,
 		.hskew = 0,
 		.vdisplay = 768,
 		.vsync_start = 771,
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 6/8] Fix comparison of unsigned integers

[Thomas Wood]

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 benchmarks/gem_exec_reloc.c | 2 --
 overlay/gem-interrupts.c    | 7 +++++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/benchmarks/gem_exec_reloc.c b/benchmarks/gem_exec_reloc.c
index 5be482a..2ef6df5 100644
--- a/benchmarks/gem_exec_reloc.c
+++ b/benchmarks/gem_exec_reloc.c
@@ -249,8 +249,6 @@ int main(int argc, char **argv)
 
 		case 'r':
 			num_relocs = atoi(optarg);
-			if (num_relocs < 0)
-				num_relocs = 0;
 			break;
 		}
 	}
diff --git a/overlay/gem-interrupts.c b/overlay/gem-interrupts.c
index 48a36b8..0150a1d 100644
--- a/overlay/gem-interrupts.c
+++ b/overlay/gem-interrupts.c
@@ -142,9 +142,12 @@ int gem_interrupts_update(struct gem_interrupts *irqs)
 		return irqs->error;
 
 	if (irqs->fd < 0) {
-		val = interrupts_read();
-		if (val < 0)
+		long long ret;
+		ret = interrupts_read();
+		if (ret < 0)
 			return irqs->error = ENODEV;
+		else
+			val = ret;
 	} else {
 		if (read(irqs->fd, &val, sizeof(val)) < 0)
 			return irqs->error = errno;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 7/8] lib: add PIPE_ANY to the pipe enum

[Thomas Wood]

This avoids compiler warnings about invalid enum values.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_kms.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 09c08aa..965c47c 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -40,6 +40,7 @@
 /* Low-level helpers with kmstest_ prefix */
 
 enum pipe {
+        PIPE_ANY = -1,
         PIPE_A = 0,
         PIPE_B,
         PIPE_C,
@@ -278,12 +279,6 @@ void igt_wait_for_vblank(int drm_fd, enum pipe pipe);
 	for (int i__ = 0; (plane) = &(display)->pipes[(pipe)].planes[i__], \
 		     i__ < (display)->pipes[(pipe)].n_planes; i__++)
 
-/*
- * Can be used with igt_output_set_pipe() to mean we don't care about the pipe
- * that should drive this output
- */
-#define PIPE_ANY	(-1)
-
 #define IGT_FIXED(i,f)	((i) << 16 | (f))
 
 void igt_enable_connectors(void);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 8/8] tests/kms_fbc_crc: ensure context is initialized correctly

[Thomas Wood]

Initialization was included in commit a976d7e (tests/kms_fbc_crc: refactor context
handling code), but won't be executed since it is declared before the first
label within a switch statement.

kms_fbc_crc.c:178:2: warning: ‘context’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  rendercopy(batch, context,
  ^
kms_fbc_crc.c:271:22: note: ‘context’ was declared here
   drm_intel_context *context = NULL;

                      ^
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 tests/kms_fbc_crc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
index d580a94..02e95e5 100644
--- a/tests/kms_fbc_crc.c
+++ b/tests/kms_fbc_crc.c
@@ -255,6 +255,7 @@ static void test_crc(data_t *data, enum test_mode mode)
 {
 	uint32_t crtc_id = data->output->config.crtc->crtc_id;
 	uint32_t handle = data->fb[0].gem_handle;
+	drm_intel_context *context = NULL;
 
 	igt_assert(fbc_enabled(data));
 
@@ -268,7 +269,6 @@ static void test_crc(data_t *data, enum test_mode mode)
 	}
 
 	switch (mode) {
-		drm_intel_context *context = NULL;
 	case TEST_PAGE_FLIP:
 		break;
 	case TEST_MMAP_CPU:
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] kms_force_connector: use comparison macros to make debug output clearer

[Thomas Wood]

On 2 November 2015 at 11:48, Thomas Wood <thomas.wood@intel.com> wrote:
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  tests/kms_force_connector.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/tests/kms_force_connector.c b/tests/kms_force_connector.c
> index 7485ca8..4ba1e0b 100644
> --- a/tests/kms_force_connector.c
> +++ b/tests/kms_force_connector.c
> @@ -27,8 +27,9 @@
>  IGT_TEST_DESCRIPTION("Check the debugfs force connector/edid features work"
>                      " correctly.");
>
> -#define CHECK_MODE(m, h, w, r) igt_assert(m.hdisplay == h && m.vdisplay == w \
> -                                         && m.vrefresh == r)
> +#define CHECK_MODE(m, h, w, r) \
> +       igt_assert_eq(m.hdisplay, h); igt_assert_eq(m.vdisplay, w); \
> +       igt_assert_eq(m.vrefresh, r);
>
>  igt_main
>  {
> @@ -63,8 +64,8 @@ igt_main
>                 /* force the connector on and check the reported values */
>                 kmstest_force_connector(drm_fd, vga_connector, FORCE_CONNECTOR_ON);
>                 temp = drmModeGetConnector(drm_fd, vga_connector->connector_id);
> -               igt_assert(temp->connection == DRM_MODE_CONNECTED);
> -               igt_assert(temp->count_modes > 0);
> +               igt_assert_eq(temp->connection, DRM_MODE_CONNECTED);
> +               igt_assert_lt(0, temp->count_modes);
>                 drmModeFreeConnector(temp);
>
>                 /* attempt to use the display */
> @@ -77,15 +78,15 @@ igt_main
>                 kmstest_force_connector(drm_fd, vga_connector,
>                                         FORCE_CONNECTOR_OFF);
>                 temp = drmModeGetConnector(drm_fd, vga_connector->connector_id);
> -               igt_assert(temp->connection == DRM_MODE_DISCONNECTED);
> -               igt_assert(temp->count_modes == 0);
> +               igt_assert_eq(temp->connection, DRM_MODE_DISCONNECTED);
> +               igt_assert_lt(0, temp->count_modes);

This should have been igt_assert_eq.


>                 drmModeFreeConnector(temp);
>
>                 /* check that the previous state is restored */
>                 kmstest_force_connector(drm_fd, vga_connector,
>                                         FORCE_CONNECTOR_UNSPECIFIED);
>                 temp = drmModeGetConnector(drm_fd, vga_connector->connector_id);
> -               igt_assert(temp->connection == vga_connector->connection);
> +               igt_assert_eq(temp->connection, vga_connector->connection);
>                 drmModeFreeConnector(temp);
>         }
>
> @@ -115,7 +116,7 @@ igt_main
>                 temp = drmModeGetConnector(drm_fd, vga_connector->connector_id);
>                 /* the connector should now have the same number of modes that
>                  * it started with */
> -               igt_assert(temp->count_modes == start_n_modes);
> +               igt_assert_eq(temp->count_modes, start_n_modes);
>                 drmModeFreeConnector(temp);
>
>                 kmstest_force_connector(drm_fd, vga_connector,
> --
> 1.9.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 8/8] tests/kms_fbc_crc: ensure context is initialized correctly

[Zanoni, Paulo R]

Em Seg, 2015-11-02 às 11:48 +0000, Thomas Wood escreveu:
> Initialization was included in commit a976d7e (tests/kms_fbc_crc:
> refactor context
> handling code), but won't be executed since it is declared before the
> first
> label within a switch statement.
> 
> kms_fbc_crc.c:178:2: warning: ‘context’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
>   rendercopy(batch, context,
>   ^
> kms_fbc_crc.c:271:22: note: ‘context’ was declared here
>    drm_intel_context *context = NULL;

I don't see these warnings here. Which compiler do you use?

pzanoni@panetone:~/nfs/intel-gpu-tools/tests$ gcc --version
gcc (Debian 5.2.1-22) 5.2.1 20151010                       

The weird part is that it was initialized during the declaration, so
the declaration is valid yet the initialization is not? Anyway, your
change removes the ambiguity of the situation and avoid citations of
the C standard, so:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
>                       ^
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  tests/kms_fbc_crc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
> index d580a94..02e95e5 100644
> --- a/tests/kms_fbc_crc.c
> +++ b/tests/kms_fbc_crc.c
> @@ -255,6 +255,7 @@ static void test_crc(data_t *data, enum test_mode
> mode)
>  {
>  	uint32_t crtc_id = data->output->config.crtc->crtc_id;
>  	uint32_t handle = data->fb[0].gem_handle;
> +	drm_intel_context *context = NULL;
>  
>  	igt_assert(fbc_enabled(data));
>  
> @@ -268,7 +269,6 @@ static void test_crc(data_t *data, enum test_mode
> mode)
>  	}
>  
>  	switch (mode) {
> -		drm_intel_context *context = NULL;
>  	case TEST_PAGE_FLIP:
>  		break;
>  	case TEST_MMAP_CPU:
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/8] lib: highlight subtest results on terminals

[Paulo Zanoni]

2015-11-02 9:48 GMT-02:00 Thomas Wood <thomas.wood@intel.com>:
> Make subtest results easier to identify by making them bold when the output
> is a terminal.

Cool!

A long time ago I suggested using different colors when writing
SUCCESS/FAIL/SKIP/CRASH (green, red, yellow, orange) and I was told
this wouldn't get merged since I should be using piglit. Since you're
creating the precedent here, can I also volunteer you to give colors
to those words on a next commit?

Thanks,
Paulo

>
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  lib/igt_core.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 59127ca..7123455 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -779,9 +779,12 @@ bool __igt_run_subtest(const char *subtest_name)
>         }
>
>         if (skip_subtests_henceforth) {
> -               printf("Subtest %s: %s\n", subtest_name,
> +               bool istty = isatty(STDOUT_FILENO);
> +
> +               printf("%sSubtest %s: %s%s\n",
> +                      (istty) ? "\x1b[1m" : "", subtest_name,
>                        skip_subtests_henceforth == SKIP ?
> -                      "SKIP" : "FAIL");
> +                      "SKIP" : "FAIL", (istty) ? "\x1b[0m" : "");
>                 return false;
>         }
>
> @@ -825,12 +828,14 @@ static void exit_subtest(const char *result)
>  {
>         struct timespec now;
>         double elapsed;
> +       bool istty = isatty(STDOUT_FILENO);
>
>         gettime(&now);
>         elapsed = now.tv_sec - subtest_time.tv_sec;
>         elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
>
> -       printf("Subtest %s: %s (%.3fs)\n", in_subtest, result, elapsed);
> +       printf("%sSubtest %s: %s (%.3fs)%s\n", (istty) ? "\x1b[1m" : "",
> +              in_subtest, result, elapsed, (istty) ? "\x1b[0m" : "");
>         fflush(stdout);
>
>         in_subtest = NULL;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 8/8] tests/kms_fbc_crc: ensure context is initialized correctly

[Ville Syrjälä]

On Wed, Nov 04, 2015 at 07:36:04PM +0000, Zanoni, Paulo R wrote:
> Em Seg, 2015-11-02 às 11:48 +0000, Thomas Wood escreveu:
> > Initialization was included in commit a976d7e (tests/kms_fbc_crc:
> > refactor context
> > handling code), but won't be executed since it is declared before the
> > first
> > label within a switch statement.
> > 
> > kms_fbc_crc.c:178:2: warning: ‘context’ may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
> >   rendercopy(batch, context,
> >   ^
> > kms_fbc_crc.c:271:22: note: ‘context’ was declared here
> >    drm_intel_context *context = NULL;
> 
> I don't see these warnings here. Which compiler do you use?
> 
> pzanoni@panetone:~/nfs/intel-gpu-tools/tests$ gcc --version
> gcc (Debian 5.2.1-22) 5.2.1 20151010                       
> 
> The weird part is that it was initialized during the declaration, so
> the declaration is valid yet the initialization is not?

Speaking as someone who was bitten by this once, yes that is the case.
Would be nice if gcc would flag it as unreachable code.

> Anyway, your
> change removes the ambiguity of the situation and avoid citations of
> the C standard, so:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > 
> >                       ^
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> > ---
> >  tests/kms_fbc_crc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
> > index d580a94..02e95e5 100644
> > --- a/tests/kms_fbc_crc.c
> > +++ b/tests/kms_fbc_crc.c
> > @@ -255,6 +255,7 @@ static void test_crc(data_t *data, enum test_mode
> > mode)
> >  {
> >  	uint32_t crtc_id = data->output->config.crtc->crtc_id;
> >  	uint32_t handle = data->fb[0].gem_handle;
> > +	drm_intel_context *context = NULL;
> >  
> >  	igt_assert(fbc_enabled(data));
> >  
> > @@ -268,7 +269,6 @@ static void test_crc(data_t *data, enum test_mode
> > mode)
> >  	}
> >  
> >  	switch (mode) {
> > -		drm_intel_context *context = NULL;
> >  	case TEST_PAGE_FLIP:
> >  		break;
> >  	case TEST_MMAP_CPU:
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/8] lib: highlight subtest results on terminals

[Morton, Derek J]

Hi Thomas,

This causes all the formatting characters to appear in our logs on Android. The problem I think is because the ADB connection to the device is always seen as a terminal and any redirection to log files is being done on the host.

Would it be possible to add a command line option to explicitly disable any pretty text formatting regardless of the isatty() call (--plain)?

If it could also apply to the isatty() call in igt_core.c/common_init() as well I could be persuaded to drop the formatting change I currently have in another patch to gem_exec_nop :-)
e.g. Have a library function that always returns false if --plain is specified otherwise the return value of isatty()?

//Derek

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Paulo Zanoni
Sent: Wednesday, November 4, 2015 7:58 PM
To: Wood, Thomas
Cc: Intel Graphics Development
Subject: Re: [Intel-gfx] [PATCH i-g-t 2/8] lib: highlight subtest results on terminals

2015-11-02 9:48 GMT-02:00 Thomas Wood <thomas.wood@intel.com>:
> Make subtest results easier to identify by making them bold when the 
> output is a terminal.

Cool!

A long time ago I suggested using different colors when writing SUCCESS/FAIL/SKIP/CRASH (green, red, yellow, orange) and I was told this wouldn't get merged since I should be using piglit. Since you're creating the precedent here, can I also volunteer you to give colors to those words on a next commit?

Thanks,
Paulo

>
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  lib/igt_core.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c index 59127ca..7123455 
> 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -779,9 +779,12 @@ bool __igt_run_subtest(const char *subtest_name)
>         }
>
>         if (skip_subtests_henceforth) {
> -               printf("Subtest %s: %s\n", subtest_name,
> +               bool istty = isatty(STDOUT_FILENO);
> +
> +               printf("%sSubtest %s: %s%s\n",
> +                      (istty) ? "\x1b[1m" : "", subtest_name,
>                        skip_subtests_henceforth == SKIP ?
> -                      "SKIP" : "FAIL");
> +                      "SKIP" : "FAIL", (istty) ? "\x1b[0m" : "");
>                 return false;
>         }
>
> @@ -825,12 +828,14 @@ static void exit_subtest(const char *result)  {
>         struct timespec now;
>         double elapsed;
> +       bool istty = isatty(STDOUT_FILENO);
>
>         gettime(&now);
>         elapsed = now.tv_sec - subtest_time.tv_sec;
>         elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
>
> -       printf("Subtest %s: %s (%.3fs)\n", in_subtest, result, elapsed);
> +       printf("%sSubtest %s: %s (%.3fs)%s\n", (istty) ? "\x1b[1m" : "",
> +              in_subtest, result, elapsed, (istty) ? "\x1b[0m" : "");
>         fflush(stdout);
>
>         in_subtest = NULL;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 8/8] tests/kms_fbc_crc: ensure context is initialized correctly

[Thomas Wood]

On 4 November 2015 at 19:36, Zanoni, Paulo R <paulo.r.zanoni@intel.com> wrote:
> Em Seg, 2015-11-02 às 11:48 +0000, Thomas Wood escreveu:
>> Initialization was included in commit a976d7e (tests/kms_fbc_crc:
>> refactor context
>> handling code), but won't be executed since it is declared before the
>> first
>> label within a switch statement.
>>
>> kms_fbc_crc.c:178:2: warning: ‘context’ may be used uninitialized in
>> this function [-Wmaybe-uninitialized]
>>   rendercopy(batch, context,
>>   ^
>> kms_fbc_crc.c:271:22: note: ‘context’ was declared here
>>    drm_intel_context *context = NULL;
>
> I don't see these warnings here. Which compiler do you use?

The warning requires -Wmaybe-uninitialized and an appropriate
optimisation level to be enabled. -Wmaybe-uninitialized is currently
disabled by the debug cflags in intel-gpu-tools due to previous false
positives, but it looks like the situation with regards to this has
improved and so it may be time to re-enable it.


>
> pzanoni@panetone:~/nfs/intel-gpu-tools/tests$ gcc --version
> gcc (Debian 5.2.1-22) 5.2.1 20151010
>
> The weird part is that it was initialized during the declaration, so
> the declaration is valid yet the initialization is not? Anyway, your
> change removes the ambiguity of the situation and avoid citations of
> the C standard, so:
>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
>>
>>                       ^
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
>> ---
>>  tests/kms_fbc_crc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
>> index d580a94..02e95e5 100644
>> --- a/tests/kms_fbc_crc.c
>> +++ b/tests/kms_fbc_crc.c
>> @@ -255,6 +255,7 @@ static void test_crc(data_t *data, enum test_mode
>> mode)
>>  {
>>       uint32_t crtc_id = data->output->config.crtc->crtc_id;
>>       uint32_t handle = data->fb[0].gem_handle;
>> +     drm_intel_context *context = NULL;
>>
>>       igt_assert(fbc_enabled(data));
>>
>> @@ -268,7 +269,6 @@ static void test_crc(data_t *data, enum test_mode
>> mode)
>>       }
>>
>>       switch (mode) {
>> -             drm_intel_context *context = NULL;
>>       case TEST_PAGE_FLIP:
>>               break;
>>       case TEST_MMAP_CPU:
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t] lib: add a environment variable to control output

[Thomas Wood]

Disable output of terminal control characters and progress meters when
IGT_PLAIN_OUTPUT is set in the environment.

Cc: Derek Morton <derek.j.morton@intel.com>
Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_aux.c  |  2 +-
 lib/igt_core.c | 23 ++++++++++++++---------
 lib/igt_core.h |  2 ++
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index f3c76ae..4d08d68 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -231,7 +231,7 @@ static void igt_interactive_info(const char *format, ...)
 {
 	va_list args;
 
-	if (!isatty(STDERR_FILENO))
+	if (!isatty(STDERR_FILENO) || __igt_plain_output)
 		return;
 
 	if (igt_log_level > IGT_LOG_INFO)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 7123455..ea9a68b 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -227,6 +227,8 @@ static enum {
 	CONT = 0, SKIP, FAIL
 } skip_subtests_henceforth = CONT;
 
+bool __igt_plain_output = false;
+
 /* fork support state */
 pid_t *test_children;
 int num_test_children;
@@ -523,11 +525,15 @@ static int common_init(int *argc, char **argv,
 	int extra_opt_count;
 	int all_opt_count;
 	int ret = 0;
-	char *env = getenv("IGT_LOG_LEVEL");
+	const char *env;
+
+	if (!isatty(STDOUT_FILENO) || getenv("IGT_PLAIN_OUTPUT"))
+		__igt_plain_output = true;
 
-	if (isatty(STDOUT_FILENO))
+	if (!__igt_plain_output)
 		setlocale(LC_ALL, "");
 
+	env = getenv("IGT_LOG_LEVEL");
 	if (env) {
 		if (strcmp(env, "debug") == 0)
 			igt_log_level = IGT_LOG_DEBUG;
@@ -779,12 +785,10 @@ bool __igt_run_subtest(const char *subtest_name)
 	}
 
 	if (skip_subtests_henceforth) {
-		bool istty = isatty(STDOUT_FILENO);
-
 		printf("%sSubtest %s: %s%s\n",
-		       (istty) ? "\x1b[1m" : "", subtest_name,
+		       (!__igt_plain_output) ? "\x1b[1m" : "", subtest_name,
 		       skip_subtests_henceforth == SKIP ?
-		       "SKIP" : "FAIL", (istty) ? "\x1b[0m" : "");
+		       "SKIP" : "FAIL", (!__igt_plain_output) ? "\x1b[0m" : "");
 		return false;
 	}
 
@@ -828,14 +832,15 @@ static void exit_subtest(const char *result)
 {
 	struct timespec now;
 	double elapsed;
-	bool istty = isatty(STDOUT_FILENO);
 
 	gettime(&now);
 	elapsed = now.tv_sec - subtest_time.tv_sec;
 	elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
 
-	printf("%sSubtest %s: %s (%.3fs)%s\n", (istty) ? "\x1b[1m" : "",
-	       in_subtest, result, elapsed, (istty) ? "\x1b[0m" : "");
+	printf("%sSubtest %s: %s (%.3fs)%s\n",
+	       (!__igt_plain_output) ? "\x1b[1m" : "",
+	       in_subtest, result, elapsed,
+	       (!__igt_plain_output) ? "\x1b[0m" : "");
 	fflush(stdout);
 
 	in_subtest = NULL;
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 5ae0965..a244fc3 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -45,6 +45,8 @@
 
 
 extern const char* __igt_test_description __attribute__((weak));
+extern bool __igt_plain_output;
+
 
 /**
  * IGT_TEST_DESCRIPTION:
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] lib: add a environment variable to control output

[Chris Wilson]

On Mon, Nov 09, 2015 at 05:17:13PM +0000, Thomas Wood wrote:
> Disable output of terminal control characters and progress meters when
> IGT_PLAIN_OUTPUT is set in the environment.
> 
> Cc: Derek Morton <derek.j.morton@intel.com>
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  lib/igt_aux.c  |  2 +-
>  lib/igt_core.c | 23 ++++++++++++++---------
>  lib/igt_core.h |  2 ++
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index f3c76ae..4d08d68 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -231,7 +231,7 @@ static void igt_interactive_info(const char *format, ...)
>  {
>  	va_list args;
>  
> -	if (!isatty(STDERR_FILENO))
> +	if (!isatty(STDERR_FILENO) || __igt_plain_output)

Hmm, would it not be a bug if we reach this point and we haven't
initialised __igt_plain_output from common_init()? 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] lib: add a environment variable to control output

[Thomas Wood]

On 9 November 2015 at 20:58, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Nov 09, 2015 at 05:17:13PM +0000, Thomas Wood wrote:
>> Disable output of terminal control characters and progress meters when
>> IGT_PLAIN_OUTPUT is set in the environment.
>>
>> Cc: Derek Morton <derek.j.morton@intel.com>
>> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
>> ---
>>  lib/igt_aux.c  |  2 +-
>>  lib/igt_core.c | 23 ++++++++++++++---------
>>  lib/igt_core.h |  2 ++
>>  3 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
>> index f3c76ae..4d08d68 100644
>> --- a/lib/igt_aux.c
>> +++ b/lib/igt_aux.c
>> @@ -231,7 +231,7 @@ static void igt_interactive_info(const char *format, ...)
>>  {
>>       va_list args;
>>
>> -     if (!isatty(STDERR_FILENO))
>> +     if (!isatty(STDERR_FILENO) || __igt_plain_output)
>
> Hmm, would it not be a bug if we reach this point and we haven't
> initialised __igt_plain_output from common_init()?

Yes, except that common_init is called from within the igt_main or
_init functions, so calling igt_interactive_info before one of these
functions would be incorrect anyway.


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] lib: add a environment variable to control output

[Morton, Derek J]

Hi Thomas,
I have ran with your patches on android. The IGT_PLAIN_OUTPUT environment variable works fine for me. My only comment would be that it should be documented somewhere.

I will remove the formatting change from my gem_exec_nop patch as with this I don't think it is needed.

//Derek 

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Thomas Wood
Sent: Tuesday, November 10, 2015 10:43 AM
To: Chris Wilson; Wood, Thomas; Intel Graphics Development
Subject: Re: [Intel-gfx] [PATCH i-g-t] lib: add a environment variable to control output

On 9 November 2015 at 20:58, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Nov 09, 2015 at 05:17:13PM +0000, Thomas Wood wrote:
>> Disable output of terminal control characters and progress meters 
>> when IGT_PLAIN_OUTPUT is set in the environment.
>>
>> Cc: Derek Morton <derek.j.morton@intel.com>
>> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
>> ---
>>  lib/igt_aux.c  |  2 +-
>>  lib/igt_core.c | 23 ++++++++++++++---------  lib/igt_core.h |  2 ++
>>  3 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/lib/igt_aux.c b/lib/igt_aux.c index f3c76ae..4d08d68 
>> 100644
>> --- a/lib/igt_aux.c
>> +++ b/lib/igt_aux.c
>> @@ -231,7 +231,7 @@ static void igt_interactive_info(const char 
>> *format, ...)  {
>>       va_list args;
>>
>> -     if (!isatty(STDERR_FILENO))
>> +     if (!isatty(STDERR_FILENO) || __igt_plain_output)
>
> Hmm, would it not be a bug if we reach this point and we haven't 
> initialised __igt_plain_output from common_init()?

Yes, except that common_init is called from within the igt_main or _init functions, so calling igt_interactive_info before one of these functions would be incorrect anyway.


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx