[PATCH i-g-t] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[PATCH i-g-t] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Joonas Lahtinen]

CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
used for timing execution of tests.

Cc: Thomas Wood <thomas.wood@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 lib/igt_core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 04a0ab2..5a5b1a9 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -341,10 +341,15 @@ static void gettime(struct timespec *ts)
 {
 	memset(ts, 0, sizeof(*ts));
 
+#ifdef CLOCK_MONOTONIC_RAW
+	if (!clock_gettime(CLOCK_MONOTONIC_RAW, ts))
+		return;
+#endif
 #ifdef CLOCK_MONOTONIC_COARSE
-	if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
+	if (!clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
+		return;
 #endif
-		clock_gettime(CLOCK_MONOTONIC, ts);
+	clock_gettime(CLOCK_MONOTONIC, ts);
 }
 
 bool __igt_fixture(void)
-- 
2.4.3

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

Re: [PATCH i-g-t] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Chris Wilson]

On Tue, Nov 17, 2015 at 12:24:13PM +0200, Joonas Lahtinen wrote:
> CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
> used for timing execution of tests.
> 
> Cc: Thomas Wood <thomas.wood@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  lib/igt_core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 04a0ab2..5a5b1a9 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -341,10 +341,15 @@ static void gettime(struct timespec *ts)
>  {
>  	memset(ts, 0, sizeof(*ts));
>  
> +#ifdef CLOCK_MONOTONIC_RAW
> +	if (!clock_gettime(CLOCK_MONOTONIC_RAW, ts))
> +		return;
> +#endif
>  #ifdef CLOCK_MONOTONIC_COARSE
> -	if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
> +	if (!clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
> +		return;
>  #endif
> -		clock_gettime(CLOCK_MONOTONIC, ts);
> +	clock_gettime(CLOCK_MONOTONIC, ts);

Would it be worth doing

static clock_t clock = -1;

if (clock_gettime(clock, ts) == 0)
	return;

#ifdef CLOCK_MONOTONIC_RAW
	if (clock_gettime(clock = CLOCK_MONOTONIC_RAW, ts) == 0)
		return;
#endif
...

?

Anyway,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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

[PATCH i-g-t v2] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Joonas Lahtinen]

CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
used for timing execution of tests.

v2:
- Cache the used clock (Chris)
- Do not change the clock during execution
- Spit out and error if monotonic time can not be read

Cc: Thomas Wood <thomas.wood@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 lib/igt_core.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 04a0ab2..c56bc15 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -220,6 +220,7 @@ static char *run_single_subtest = NULL;
 static bool run_single_subtest_found = false;
 static const char *in_subtest = NULL;
 static struct timespec subtest_time;
+static clockid_t igt_clock = (clockid_t)-1;
 static bool in_fixture = false;
 static bool test_with_subtests = false;
 static bool in_atexit_handler = false;
@@ -341,10 +342,25 @@ static void gettime(struct timespec *ts)
 {
 	memset(ts, 0, sizeof(*ts));
 
+	// Stay on the same clock for consistency.
+	if (igt_clock != (clockid_t)-1) {
+		clock_gettime(igt_clock, ts);
+		return;
+	}
+
+#ifdef CLOCK_MONOTONIC_RAW
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_RAW, ts))
+		return;
+#endif
 #ifdef CLOCK_MONOTONIC_COARSE
-	if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_COARSE, ts))
+		return;
 #endif
-		clock_gettime(CLOCK_MONOTONIC, ts);
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC, ts))
+		return;
+
+	igt_warn("Unable to get monotonic time!\n");
+	exit(IGT_EXIT_FAILURE);
 }
 
 bool __igt_fixture(void)
-- 
2.4.3

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

Re: [PATCH i-g-t v2] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Chris Wilson]

On Tue, Nov 17, 2015 at 02:18:09PM +0200, Joonas Lahtinen wrote:
> CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
> used for timing execution of tests.
> 
> v2:
> - Cache the used clock (Chris)
> - Do not change the clock during execution
> - Spit out and error if monotonic time can not be read
> 
> Cc: Thomas Wood <thomas.wood@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  lib/igt_core.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 04a0ab2..c56bc15 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -220,6 +220,7 @@ static char *run_single_subtest = NULL;
>  static bool run_single_subtest_found = false;
>  static const char *in_subtest = NULL;
>  static struct timespec subtest_time;
> +static clockid_t igt_clock = (clockid_t)-1;
>  static bool in_fixture = false;
>  static bool test_with_subtests = false;
>  static bool in_atexit_handler = false;
> @@ -341,10 +342,25 @@ static void gettime(struct timespec *ts)
>  {
>  	memset(ts, 0, sizeof(*ts));
>  
> +	// Stay on the same clock for consistency.
> +	if (igt_clock != (clockid_t)-1) {
> +		clock_gettime(igt_clock, ts);
> +		return;
> +	}
> +
> +#ifdef CLOCK_MONOTONIC_RAW
> +	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_RAW, ts))
> +		return;
> +#endif
>  #ifdef CLOCK_MONOTONIC_COARSE
> -	if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
> +	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_COARSE, ts))
> +		return;
>  #endif
> -		clock_gettime(CLOCK_MONOTONIC, ts);
> +	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC, ts))
> +		return;
> +
> +	igt_warn("Unable to get monotonic time!\n");
> +	exit(IGT_EXIT_FAILURE);

Hmm. gettime is used inside the subtest execution, so I'm not too sure
what should be done here, but definitely not call exit() itself!

I think probably return -errno is best, and move the igt_assert() into
the callers (as whatever is appropriate).

A second patch to then export

void igt_gettime(struct timespec *ts)
{
	igt_assert_f(gettime(ts) == 0,
		     "Unable to get monotonic time\n");
}

and replace all of the mismash in the tests would be nice.
-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

[PATCH i-g-t v3] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Joonas Lahtinen]

CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
used for timing execution of tests.

When fetching either the starting or ending time of a test, show the
time as -1.000s.

v3:
- Do not exit directly from handler (Chris)
- Show elapsed time as -1 if it is not calculable

v2:
- Cache the used clock (Chris)
- Do not change the clock during execution
- Spit out and error if monotonic time can not be read

Cc: Thomas Wood <thomas.wood@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 lib/igt_core.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 10 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 04a0ab2..b4cab42 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -220,6 +220,7 @@ static char *run_single_subtest = NULL;
 static bool run_single_subtest_found = false;
 static const char *in_subtest = NULL;
 static struct timespec subtest_time;
+static clockid_t igt_clock = (clockid_t)-1;
 static bool in_fixture = false;
 static bool test_with_subtests = false;
 static bool in_atexit_handler = false;
@@ -337,14 +338,32 @@ static void kmsg(const char *format, ...)
 	fclose(file);
 }
 
-static void gettime(struct timespec *ts)
+static int gettime(struct timespec *ts)
 {
 	memset(ts, 0, sizeof(*ts));
+	errno = 0;
 
+	// Stay on the same clock for consistency.
+	if (igt_clock != (clockid_t)-1) {
+		if (clock_gettime(igt_clock, ts))
+			goto error;
+		return 0;
+	}
+
+#ifdef CLOCK_MONOTONIC_RAW
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_RAW, ts))
+		return 0;
+#endif
 #ifdef CLOCK_MONOTONIC_COARSE
-	if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_COARSE, ts))
+		return 0;
 #endif
-		clock_gettime(CLOCK_MONOTONIC, ts);
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC, ts))
+		return 0;
+error:
+	igt_warn("Could not read monotonic time: %s\n",
+		 strerror(errno));
+	return -errno;
 }
 
 bool __igt_fixture(void)
@@ -832,10 +851,16 @@ static void exit_subtest(const char *result)
 {
 	struct timespec now;
 	double elapsed;
+	int err;
 
-	gettime(&now);
-	elapsed = now.tv_sec - subtest_time.tv_sec;
-	elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
+	err = gettime(&now);
+	if (!err && subtest_time.tv_sec != 0 &&
+	    subtest_time.tv_nsec != 0) {
+		elapsed = now.tv_sec - subtest_time.tv_sec;
+		elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
+	} else {
+		elapsed = -1.;
+	}
 
 	printf("%sSubtest %s: %s (%.3fs)%s\n",
 	       (!__igt_plain_output) ? "\x1b[1m" : "",
@@ -1090,10 +1115,17 @@ void igt_exit(void)
 		struct timespec now;
 		double elapsed;
 		const char *result;
+		int err;
 
-		gettime(&now);
-		elapsed = now.tv_sec - subtest_time.tv_sec;
-		elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
+		err = gettime(&now);
+
+		if (!err && subtest_time.tv_sec != 0 &&
+		    subtest_time.tv_nsec != 0) {
+			elapsed = now.tv_sec - subtest_time.tv_sec;
+			elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
+		} else {
+			elapsed = -1.;
+		}
 
 		switch (igt_exitcode) {
 			case IGT_EXIT_SUCCESS:
@@ -1109,7 +1141,6 @@ void igt_exit(void)
 				result = "FAIL";
 		}
 
-
 		printf("%s (%.3fs)\n", result, elapsed);
 		exit(igt_exitcode);
 	}
-- 
2.4.3

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

Re: [PATCH i-g-t v3] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Chris Wilson]

On Wed, Nov 18, 2015 at 02:18:52PM +0200, Joonas Lahtinen wrote:
> CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
> used for timing execution of tests.
> 
> When fetching either the starting or ending time of a test, show the
> time as -1.000s.
> 
> v3:
> - Do not exit directly from handler (Chris)
> - Show elapsed time as -1 if it is not calculable

Aye, that's better for the subtest handling.

> @@ -832,10 +851,16 @@ static void exit_subtest(const char *result)
>  {
>  	struct timespec now;
>  	double elapsed;
> +	int err;
>  
> -	gettime(&now);
> -	elapsed = now.tv_sec - subtest_time.tv_sec;
> -	elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
> +	err = gettime(&now);
> +	if (!err && subtest_time.tv_sec != 0 &&
> +	    subtest_time.tv_nsec != 0) {

A little paranoid? If we want the paranoia perhaps move it to gettime
and return an error?
-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 v3] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Joonas Lahtinen]

On ke, 2015-11-18 at 12:28 +0000, Chris Wilson wrote:
> On Wed, Nov 18, 2015 at 02:18:52PM +0200, Joonas Lahtinen wrote:
> > CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE
> > clock
> > used for timing execution of tests.
> > 
> > When fetching either the starting or ending time of a test, show
> > the
> > time as -1.000s.
> > 
> > v3:
> > - Do not exit directly from handler (Chris)
> > - Show elapsed time as -1 if it is not calculable
> 
> Aye, that's better for the subtest handling.
> 
> > @@ -832,10 +851,16 @@ static void exit_subtest(const char *result)
> >  {
> >  	struct timespec now;
> >  	double elapsed;
> > +	int err;
> >  
> > -	gettime(&now);
> > -	elapsed = now.tv_sec - subtest_time.tv_sec;
> > -	elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
> > +	err = gettime(&now);
> > +	if (!err && subtest_time.tv_sec != 0 &&
> > +	    subtest_time.tv_nsec != 0) {
> 
> A little paranoid? If we want the paranoia perhaps move it to gettime
> and return an error?
> 

I'm just checking that the starting time which is in the subtest_time
did not fail (which leads to both being zero). I'm not looking at the
now values :)

Regards, Joonas

> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH i-g-t v3] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Chris Wilson]

On Wed, Nov 18, 2015 at 02:54:20PM +0200, Joonas Lahtinen wrote:
> On ke, 2015-11-18 at 12:28 +0000, Chris Wilson wrote:
> > On Wed, Nov 18, 2015 at 02:18:52PM +0200, Joonas Lahtinen wrote:
> > > CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE
> > > clock
> > > used for timing execution of tests.
> > > 
> > > When fetching either the starting or ending time of a test, show
> > > the
> > > time as -1.000s.
> > > 
> > > v3:
> > > - Do not exit directly from handler (Chris)
> > > - Show elapsed time as -1 if it is not calculable
> > 
> > Aye, that's better for the subtest handling.
> > 
> > > @@ -832,10 +851,16 @@ static void exit_subtest(const char *result)
> > >  {
> > >  	struct timespec now;
> > >  	double elapsed;
> > > +	int err;
> > >  
> > > -	gettime(&now);
> > > -	elapsed = now.tv_sec - subtest_time.tv_sec;
> > > -	elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
> > > +	err = gettime(&now);
> > > +	if (!err && subtest_time.tv_sec != 0 &&
> > > +	    subtest_time.tv_nsec != 0) {
> > 
> > A little paranoid? If we want the paranoia perhaps move it to gettime
> > and return an error?
> > 
> 
> I'm just checking that the starting time which is in the subtest_time
> did not fail (which leads to both being zero). I'm not looking at the
> now values :)

Ok, now I see.

#define time_valid(ts) ((ts)->tv_sec || (ts)->tv_nsec)

gettime(&now);
if (time_valid(&now) && time_valid(&subtest_time)) {
	elapsed = ...;
} else {
	elapsed = -1;
}

Perhaps?
-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

[PATCH i-g-t v4] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Joonas Lahtinen]

CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
used for timing execution of tests.

When fetching either the starting or ending time of a test, show the
time as -1.000s.

v4:
- Introduce time_valid macro (Chris)
- Reduce amount of boilerplate code for calculating elapsed time

v3:
- Do not exit directly from handler (Chris)
- Show elapsed time as -1 if it is not calculable

v2:
- Cache the used clock (Chris)
- Do not change the clock during execution
- Spit out and error if monotonic time can not be read

Cc: Thomas Wood <thomas.wood@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 lib/igt_core.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 04a0ab2..3269804 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -220,6 +220,7 @@ static char *run_single_subtest = NULL;
 static bool run_single_subtest_found = false;
 static const char *in_subtest = NULL;
 static struct timespec subtest_time;
+static clockid_t igt_clock = (clockid_t)-1;
 static bool in_fixture = false;
 static bool test_with_subtests = false;
 static bool in_atexit_handler = false;
@@ -337,14 +338,48 @@ static void kmsg(const char *format, ...)
 	fclose(file);
 }
 
-static void gettime(struct timespec *ts)
+#define time_valid(ts) ((ts)->tv_sec || (ts)->tv_nsec)
+
+static double
+time_elapsed(struct timespec *then,
+	     struct timespec* now) {
+	double elapsed = -1.;
+
+	if (time_valid(then) && time_valid(now)) {
+		elapsed = now->tv_sec - then->tv_sec;
+		elapsed += (now->tv_nsec - then->tv_nsec) * 1e-9;
+	}
+
+	return elapsed;
+}
+
+static int gettime(struct timespec *ts)
 {
 	memset(ts, 0, sizeof(*ts));
+	errno = 0;
 
+	// Stay on the same clock for consistency.
+	if (igt_clock != (clockid_t)-1) {
+		if (clock_gettime(igt_clock, ts))
+			goto error;
+		return 0;
+	}
+
+#ifdef CLOCK_MONOTONIC_RAW
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_RAW, ts))
+		return 0;
+#endif
 #ifdef CLOCK_MONOTONIC_COARSE
-	if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_COARSE, ts))
+		return 0;
 #endif
-		clock_gettime(CLOCK_MONOTONIC, ts);
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC, ts))
+		return 0;
+error:
+	igt_warn("Could not read monotonic time: %s\n",
+		 strerror(errno));
+
+	return -errno;
 }
 
 bool __igt_fixture(void)
@@ -831,15 +866,11 @@ static void exit_subtest(const char *) __attribute__((noreturn));
 static void exit_subtest(const char *result)
 {
 	struct timespec now;
-	double elapsed;
 
 	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",
 	       (!__igt_plain_output) ? "\x1b[1m" : "",
-	       in_subtest, result, elapsed,
+	       in_subtest, result, time_elapsed(&subtest_time, &now),
 	       (!__igt_plain_output) ? "\x1b[0m" : "");
 	fflush(stdout);
 
@@ -1088,12 +1119,9 @@ void igt_exit(void)
 
 	if (!test_with_subtests) {
 		struct timespec now;
-		double elapsed;
 		const char *result;
 
 		gettime(&now);
-		elapsed = now.tv_sec - subtest_time.tv_sec;
-		elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
 
 		switch (igt_exitcode) {
 			case IGT_EXIT_SUCCESS:
@@ -1109,8 +1137,7 @@ void igt_exit(void)
 				result = "FAIL";
 		}
 
-
-		printf("%s (%.3fs)\n", result, elapsed);
+		printf("%s (%.3fs)\n", result, time_elapsed(&subtest_time, &now));
 		exit(igt_exitcode);
 	}
 
-- 
2.4.3

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

Re: [PATCH i-g-t v4] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Chris Wilson]

On Thu, Nov 19, 2015 at 01:00:07PM +0200, Joonas Lahtinen wrote:
> CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
> used for timing execution of tests.
> 
> When fetching either the starting or ending time of a test, show the
> time as -1.000s.
> 
> v4:
> - Introduce time_valid macro (Chris)
> - Reduce amount of boilerplate code for calculating elapsed time
> 
> v3:
> - Do not exit directly from handler (Chris)
> - Show elapsed time as -1 if it is not calculable
> 
> v2:
> - Cache the used clock (Chris)
> - Do not change the clock during execution
> - Spit out and error if monotonic time can not be read
> 
> Cc: Thomas Wood <thomas.wood@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> +static int gettime(struct timespec *ts)
>  {
>  	memset(ts, 0, sizeof(*ts));
> +	errno = 0;
>  
> +	// Stay on the same clock for consistency.

We avoid using "//" for comments, even for single lines.
-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

[PATCH i-g-t v5] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Joonas Lahtinen]

CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
used for timing execution of tests.

When fetching either the starting or ending time of a test, show the
time as -1.000s.

v5:
- Do not use C99 style comments (Chris)

v4:
- Introduce time_valid macro (Chris)
- Reduce amount of boilerplate code for calculating elapsed time

v3:
- Do not exit directly from handler (Chris)
- Show elapsed time as -1 if it is not calculable

v2:
- Cache the used clock (Chris)
- Do not change the clock during execution
- Spit out and error if monotonic time can not be read

Cc: Thomas Wood <thomas.wood@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 lib/igt_core.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 04a0ab2..2106883 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -220,6 +220,7 @@ static char *run_single_subtest = NULL;
 static bool run_single_subtest_found = false;
 static const char *in_subtest = NULL;
 static struct timespec subtest_time;
+static clockid_t igt_clock = (clockid_t)-1;
 static bool in_fixture = false;
 static bool test_with_subtests = false;
 static bool in_atexit_handler = false;
@@ -337,14 +338,48 @@ static void kmsg(const char *format, ...)
 	fclose(file);
 }
 
-static void gettime(struct timespec *ts)
+#define time_valid(ts) ((ts)->tv_sec || (ts)->tv_nsec)
+
+static double
+time_elapsed(struct timespec *then,
+	     struct timespec* now) {
+	double elapsed = -1.;
+
+	if (time_valid(then) && time_valid(now)) {
+		elapsed = now->tv_sec - then->tv_sec;
+		elapsed += (now->tv_nsec - then->tv_nsec) * 1e-9;
+	}
+
+	return elapsed;
+}
+
+static int gettime(struct timespec *ts)
 {
 	memset(ts, 0, sizeof(*ts));
+	errno = 0;
 
+	/* Stay on the same clock for consistency. */
+	if (igt_clock != (clockid_t)-1) {
+		if (clock_gettime(igt_clock, ts))
+			goto error;
+		return 0;
+	}
+
+#ifdef CLOCK_MONOTONIC_RAW
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_RAW, ts))
+		return 0;
+#endif
 #ifdef CLOCK_MONOTONIC_COARSE
-	if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_COARSE, ts))
+		return 0;
 #endif
-		clock_gettime(CLOCK_MONOTONIC, ts);
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC, ts))
+		return 0;
+error:
+	igt_warn("Could not read monotonic time: %s\n",
+		 strerror(errno));
+
+	return -errno;
 }
 
 bool __igt_fixture(void)
@@ -831,15 +866,11 @@ static void exit_subtest(const char *) __attribute__((noreturn));
 static void exit_subtest(const char *result)
 {
 	struct timespec now;
-	double elapsed;
 
 	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",
 	       (!__igt_plain_output) ? "\x1b[1m" : "",
-	       in_subtest, result, elapsed,
+	       in_subtest, result, time_elapsed(&subtest_time, &now),
 	       (!__igt_plain_output) ? "\x1b[0m" : "");
 	fflush(stdout);
 
@@ -1088,12 +1119,9 @@ void igt_exit(void)
 
 	if (!test_with_subtests) {
 		struct timespec now;
-		double elapsed;
 		const char *result;
 
 		gettime(&now);
-		elapsed = now.tv_sec - subtest_time.tv_sec;
-		elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
 
 		switch (igt_exitcode) {
 			case IGT_EXIT_SUCCESS:
@@ -1109,8 +1137,7 @@ void igt_exit(void)
 				result = "FAIL";
 		}
 
-
-		printf("%s (%.3fs)\n", result, elapsed);
+		printf("%s (%.3fs)\n", result, time_elapsed(&subtest_time, &now));
 		exit(igt_exitcode);
 	}
 
-- 
2.4.3

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

Re: [PATCH i-g-t v5] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Chris Wilson]

On Fri, Nov 20, 2015 at 01:26:23PM +0200, Joonas Lahtinen wrote:
> +static double
> +time_elapsed(struct timespec *then,
> +	     struct timespec* now) {

I can't stop myself...             ^
-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

[PATCH i-g-t v6] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Joonas Lahtinen]

CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
used for timing execution of tests.

When fetching either the starting or ending time of a test, show the
time as -1.000s.

v6:
- Whitespace corrections (Chris)

v5:
- Do not use C99 style comments (Chris)

v4:
- Introduce time_valid macro (Chris)
- Reduce amount of boilerplate code for calculating elapsed time

v3:
- Do not exit directly from handler (Chris)
- Show elapsed time as -1 if it is not calculable

v2:
- Cache the used clock (Chris)
- Do not change the clock during execution
- Spit out and error if monotonic time can not be read

Cc: Thomas Wood <thomas.wood@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 lib/igt_core.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 04a0ab2..84cf8d2 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -220,6 +220,7 @@ static char *run_single_subtest = NULL;
 static bool run_single_subtest_found = false;
 static const char *in_subtest = NULL;
 static struct timespec subtest_time;
+static clockid_t igt_clock = (clockid_t)-1;
 static bool in_fixture = false;
 static bool test_with_subtests = false;
 static bool in_atexit_handler = false;
@@ -337,14 +338,49 @@ static void kmsg(const char *format, ...)
 	fclose(file);
 }
 
-static void gettime(struct timespec *ts)
+#define time_valid(ts) ((ts)->tv_sec || (ts)->tv_nsec)
+
+static double
+time_elapsed(struct timespec *then,
+	     struct timespec* now)
+{
+	double elapsed = -1.;
+
+	if (time_valid(then) && time_valid(now)) {
+		elapsed = now->tv_sec - then->tv_sec;
+		elapsed += (now->tv_nsec - then->tv_nsec) * 1e-9;
+	}
+
+	return elapsed;
+}
+
+static int gettime(struct timespec *ts)
 {
 	memset(ts, 0, sizeof(*ts));
+	errno = 0;
 
+	/* Stay on the same clock for consistency. */
+	if (igt_clock != (clockid_t)-1) {
+		if (clock_gettime(igt_clock, ts))
+			goto error;
+		return 0;
+	}
+
+#ifdef CLOCK_MONOTONIC_RAW
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_RAW, ts))
+		return 0;
+#endif
 #ifdef CLOCK_MONOTONIC_COARSE
-	if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_COARSE, ts))
+		return 0;
 #endif
-		clock_gettime(CLOCK_MONOTONIC, ts);
+	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC, ts))
+		return 0;
+error:
+	igt_warn("Could not read monotonic time: %s\n",
+		 strerror(errno));
+
+	return -errno;
 }
 
 bool __igt_fixture(void)
@@ -831,15 +867,11 @@ static void exit_subtest(const char *) __attribute__((noreturn));
 static void exit_subtest(const char *result)
 {
 	struct timespec now;
-	double elapsed;
 
 	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",
 	       (!__igt_plain_output) ? "\x1b[1m" : "",
-	       in_subtest, result, elapsed,
+	       in_subtest, result, time_elapsed(&subtest_time, &now),
 	       (!__igt_plain_output) ? "\x1b[0m" : "");
 	fflush(stdout);
 
@@ -1088,12 +1120,9 @@ void igt_exit(void)
 
 	if (!test_with_subtests) {
 		struct timespec now;
-		double elapsed;
 		const char *result;
 
 		gettime(&now);
-		elapsed = now.tv_sec - subtest_time.tv_sec;
-		elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
 
 		switch (igt_exitcode) {
 			case IGT_EXIT_SUCCESS:
@@ -1109,8 +1138,7 @@ void igt_exit(void)
 				result = "FAIL";
 		}
 
-
-		printf("%s (%.3fs)\n", result, elapsed);
+		printf("%s (%.3fs)\n", result, time_elapsed(&subtest_time, &now));
 		exit(igt_exitcode);
 	}
 
-- 
2.4.3

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

Re: [PATCH i-g-t v6] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW

[Joonas Lahtinen]

This could be merged?

On pe, 2015-11-20 at 13:57 +0200, Joonas Lahtinen wrote:
> CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock
> used for timing execution of tests.
> 
> When fetching either the starting or ending time of a test, show the
> time as -1.000s.
> 
> v6:
> - Whitespace corrections (Chris)
> 
> v5:
> - Do not use C99 style comments (Chris)
> 
> v4:
> - Introduce time_valid macro (Chris)
> - Reduce amount of boilerplate code for calculating elapsed time
> 
> v3:
> - Do not exit directly from handler (Chris)
> - Show elapsed time as -1 if it is not calculable
> 
> v2:
> - Cache the used clock (Chris)
> - Do not change the clock during execution
> - Spit out and error if monotonic time can not be read
> 
> Cc: Thomas Wood <thomas.wood@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  lib/igt_core.c | 54 +++++++++++++++++++++++++++++++++++++++++-------
> ------
>  1 file changed, 41 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 04a0ab2..84cf8d2 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -220,6 +220,7 @@ static char *run_single_subtest = NULL;
>  static bool run_single_subtest_found = false;
>  static const char *in_subtest = NULL;
>  static struct timespec subtest_time;
> +static clockid_t igt_clock = (clockid_t)-1;
>  static bool in_fixture = false;
>  static bool test_with_subtests = false;
>  static bool in_atexit_handler = false;
> @@ -337,14 +338,49 @@ static void kmsg(const char *format, ...)
>  	fclose(file);
>  }
>  
> -static void gettime(struct timespec *ts)
> +#define time_valid(ts) ((ts)->tv_sec || (ts)->tv_nsec)
> +
> +static double
> +time_elapsed(struct timespec *then,
> +	     struct timespec* now)
> +{
> +	double elapsed = -1.;
> +
> +	if (time_valid(then) && time_valid(now)) {
> +		elapsed = now->tv_sec - then->tv_sec;
> +		elapsed += (now->tv_nsec - then->tv_nsec) * 1e-9;
> +	}
> +
> +	return elapsed;
> +}
> +
> +static int gettime(struct timespec *ts)
>  {
>  	memset(ts, 0, sizeof(*ts));
> +	errno = 0;
>  
> +	/* Stay on the same clock for consistency. */
> +	if (igt_clock != (clockid_t)-1) {
> +		if (clock_gettime(igt_clock, ts))
> +			goto error;
> +		return 0;
> +	}
> +
> +#ifdef CLOCK_MONOTONIC_RAW
> +	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_RAW, ts))
> +		return 0;
> +#endif
>  #ifdef CLOCK_MONOTONIC_COARSE
> -	if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts))
> +	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_COARSE, ts))
> +		return 0;
>  #endif
> -		clock_gettime(CLOCK_MONOTONIC, ts);
> +	if (!clock_gettime(igt_clock = CLOCK_MONOTONIC, ts))
> +		return 0;
> +error:
> +	igt_warn("Could not read monotonic time: %s\n",
> +		 strerror(errno));
> +
> +	return -errno;
>  }
>  
>  bool __igt_fixture(void)
> @@ -831,15 +867,11 @@ static void exit_subtest(const char *)
> __attribute__((noreturn));
>  static void exit_subtest(const char *result)
>  {
>  	struct timespec now;
> -	double elapsed;
>  
>  	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",
>  	       (!__igt_plain_output) ? "\x1b[1m" : "",
> -	       in_subtest, result, elapsed,
> +	       in_subtest, result, time_elapsed(&subtest_time,
> &now),
>  	       (!__igt_plain_output) ? "\x1b[0m" : "");
>  	fflush(stdout);
>  
> @@ -1088,12 +1120,9 @@ void igt_exit(void)
>  
>  	if (!test_with_subtests) {
>  		struct timespec now;
> -		double elapsed;
>  		const char *result;
>  
>  		gettime(&now);
> -		elapsed = now.tv_sec - subtest_time.tv_sec;
> -		elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e
> -9;
>  
>  		switch (igt_exitcode) {
>  			case IGT_EXIT_SUCCESS:
> @@ -1109,8 +1138,7 @@ void igt_exit(void)
>  				result = "FAIL";
>  		}
>  
> -
> -		printf("%s (%.3fs)\n", result, elapsed);
> +		printf("%s (%.3fs)\n", result,
> time_elapsed(&subtest_time, &now));
>  		exit(igt_exitcode);
>  	}
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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