Input: Use monotonic time for event time stamps.

RFC PATCH In review john stultz 3 participants 10 messages Dec 21, 2011

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

john stultz

author

2011-12-21 01:02 UTC #0 lore raw

RFC PATCH 1/1 Input: Use monotonic time for event time stamps.

Hi
In reviewing the Android patch set, I noticed the following patch from
Arve which looks like it resolves a reasonable issue (events using CLOCK_REALTIME timestamps, which can jump forwards or backwards via settimeofday()).

I'm not very familiar with the evdev interface, so I'm not sure if changing the timestamps to CLOCK_MONOTONIC could cause an ABI problem with legacy apps. Even so, I wanted to send the patch out for review and consideration as it seems like a reasonable fix.

thanks
-john

From 20950b728f120cd808965c1a20b024aa79706d8d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=3D=3FUTF-8=3Fq=3FArve=3D20Hj=3DC3=3DB8nnev=3DC3=3DA5g=3F=3D?= <[email protected]>
Date: Fri, 2 Dec 2011 13:59:05 +0800
Subject: [PATCH] Input: Use monotonic time for event time stamps.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since wall time can jump backwards, it cannot be used to determine if one event occured before another or for how long a key was pressed.

CC Dmitry Torokhov <[email protected]>

CC [email protected]

Signed-off-by Arve Hjønnevåg <[email protected]>

Signed-off-by John Stultz <[email protected]>

---
 drivers/input/evdev.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

drivers/input/evdev.c +4 -1

index 4cf2534..ec329b4 100644
@@ -94,8 +94,11 @@ static void evdev_event(struct input_handle *handle,
 	struct evdev *evdev = handle->private;
 	struct evdev_client *client;
 	struct input_event event;
+	struct timespec ts;
 
-	do_gettimeofday(&event.time);
+	ktime_get_ts(&ts);
+	event.time.tv_sec = ts.tv_sec;
+	event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
 	event.type = type;
 	event.code = code;
 	event.value = value;

Signature · 8 lines

1.7.3.2.146.gca209



--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wanlong Gao

2011-12-21 01:43 UTC #1 lore raw

Quoted 24 lines · john stultz

Hi
In reviewing the Android patch set, I noticed the following patch from
Arve which looks like it resolves a reasonable issue (events using CLOCK_REALTIME timestamps, which can jump forwards or backwards via settimeofday()).
I'm not very familiar with the evdev interface, so I'm not sure if changing the timestamps to CLOCK_MONOTONIC could cause an ABI problem with legacy apps. Even so, I wanted to send the patch out for review and consideration as it seems like a reasonable fix.
thanks
-john
Quoted 1 line
From 20950b728f120cd808965c1a20b024aa79706d8d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=3D=3FUTF-8=3Fq=3FArve=3D20Hj=3DC3=3DB8nnev=3DC3=3DA5g=3F=3D?= <[email protected]>
Date: Fri, 2 Dec 2011 13:59:05 +0800
Subject: [PATCH] Input: Use monotonic time for event time stamps.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Quoted 2 lines

Since wall time can jump backwards, it cannot be used to determine if one event occured before another or for how long a key was pressed.

seems reasonable.

Quoted 16 lines

CC Dmitry Torokhov <[email protected]>
CC [email protected]
Signed-off-by Arve Hjønnevåg <[email protected]>
Signed-off-by John Stultz <[email protected]>
---
 drivers/input/evdev.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/input/evdev.c +0 -0
index 4cf2534..ec329b4 100644
@@ -94,8 +94,11 @@ static void evdev_event(struct input_handle *handle,
 	struct evdev *evdev = handle->private;
 	struct evdev_client *client;

drivers/input/evdev.c static void evdev_event(struct input_handle *handle, · line 104

Quoted 10 lines

drivers/input/evdev.c +4 -1

 	struct input_event event;
+	struct timespec ts;
 
-	do_gettimeofday(&event.time);
+	ktime_get_ts(&ts);
+	event.time.tv_sec = ts.tv_sec;
+	event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
 	event.type = type;
 	event.code = code;
 	event.value = value;

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Daniel Kurtz

2011-12-21 02:11 UTC #2 lore raw

Quoted 8 lines · Wanlong Gao

Quoted 6 lines · john stultz
Hi
In reviewing the Android patch set, I noticed the following patch from Arve which looks like it resolves a reasonable issue (events using CLOCK_REALTIME timestamps, which can jump forwards or backwards via settimeofday()).

Quoted 4 lines

Quoted 4 lines
I'm not very familiar with the evdev interface, so I'm not sure if changing the timestamps to CLOCK_MONOTONIC could cause an ABI problem with legacy apps. Even so, I wanted to send the patch out for review and consideration as it seems like a reasonable fix.

Maybe you will have more luck this time. You can read the previous thread on this exact topic, here:
https://lkml.org/lkml/2011/10/3/37

The previous attempt got bogged down when people wanted more data on use cases and how this patch would promote world peace. I strongly support the concept, but we found other ways to address our major concern at the time, so didn't invest more effort to get that patch accepted.

Just a question though, why ktime_get_ts() and not getrawmonotonic()?

Thanks!

Quoted 38 lines

Quoted 14 lines
thanks
-john
Quoted 1 line
From 20950b728f120cd808965c1a20b024aa79706d8d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=3D=3FUTF-8=3Fq=3FArve=3D20Hj=3DC3=3DB8nnev=3DC3=3DA5g=3F=3D?= <[email protected]>
Date: Fri, 2 Dec 2011 13:59:05 +0800
Subject: [PATCH] Input: Use monotonic time for event time stamps.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Quoted 2 lines
Since wall time can jump backwards, it cannot be used to determine if one event occured before another or for how long a key was pressed.
seems reasonable.
Quoted 19 lines
CC Dmitry Torokhov <[email protected]>
CC [email protected]
Signed-off-by Arve Hjønnevåg <[email protected]>
Signed-off-by John Stultz <[email protected]>
---
drivers/input/evdev.c | 5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/input/evdev.c +1 -0
index 4cf2534..ec329b4 100644
@@ -94,8 +94,11 @@ static void evdev_event(struct input_handle *handle,
       struct evdev *evdev = handle->private;
       struct evdev_client *client;
       struct input_event event;
+     struct timespec ts;
 

Quoted 7 lines

drivers/input/evdev.c static void evdev_event(struct input_handle *handle, · line 104

Quoted 7 lines

drivers/input/evdev.c +3 -1

-     do_gettimeofday(&event.time);
+     ktime_get_ts(&ts);
+     event.time.tv_sec = ts.tv_sec;
+     event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
       event.type = type;
       event.code = code;
       event.value = value;

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

john stultz

author

2011-12-21 02:23 UTC #3 lore raw

Quoted 9 lines · Daniel Kurtz

Quoted 8 lines · Wanlong Gao
Quoted 6 lines · john stultz
Hi
In reviewing the Android patch set, I noticed the following patch from
Arve which looks like it resolves a reasonable issue (events using CLOCK_REALTIME timestamps, which can jump forwards or backwards via settimeofday()).

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
I'm not very familiar with the evdev interface, so I'm not sure if changing the timestamps to CLOCK_MONOTONIC could cause an ABI problem with legacy apps. Even so, I wanted to send the patch out for review and consideration as it seems like a reasonable fix.
Maybe you will have more luck this time. You can read the previous thread on this exact topic, here:
https://lkml.org/lkml/2011/10/3/37

Interesting! Thanks for the link!

Quoted 7 lines

The previous attempt got bogged down when people wanted more data on use cases and how this patch would promote world peace. I strongly support the concept, but we found other ways to address our major concern at the time, so didn't invest more effort to get that patch accepted.
Just a question though, why ktime_get_ts() and not getrawmonotonic()?

So rawmonotonic isn't frequency corrected via NTP, while the monotonic clock is. So if you're calculating intervals, you will get more accurate times (where a second is a second) w/ ktime_get_ts().

The raw monotonic clock is really only useful for time correction applications (being able to accurately measure how much of a correction was applied), or when you really want a abstracted sense of hardware cycles.

thanks
-john

Daniel Kurtz

2011-12-21 03:34 UTC #4 lore raw

Quoted 29 lines · john stultz

Quoted 9 lines · Daniel Kurtz
Quoted 8 lines · Wanlong Gao
Quoted 6 lines · john stultz
Hi
In reviewing the Android patch set, I noticed the following patch from Arve which looks like it resolves a reasonable issue (events using CLOCK_REALTIME timestamps, which can jump forwards or backwards via settimeofday()).
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
I'm not very familiar with the evdev interface, so I'm not sure if changing the timestamps to CLOCK_MONOTONIC could cause an ABI problem with legacy apps. Even so, I wanted to send the patch out for review and consideration as it seems like a reasonable fix.
Maybe you will have more luck this time. You can read the previous thread on this exact topic, here:
https://lkml.org/lkml/2011/10/3/37
Interesting! Thanks for the link!
Quoted 7 lines
The previous attempt got bogged down when people wanted more data on use cases and how this patch would promote world peace. I strongly support the concept, but we found other ways to address our major concern at the time, so didn't invest more effort to get that patch accepted.
Just a question though, why ktime_get_ts() and not getrawmonotonic()?

Quoted 8 lines

So rawmonotonic isn't frequency corrected via NTP, while the monotonic clock is. So if you're calculating intervals, you will get more accurate times (where a second is a second) w/ ktime_get_ts().
The raw monotonic clock is really only useful for time correction applications (being able to accurately measure how much of a correction was applied), or when you really want a abstracted sense of hardware cycles.

And here is where it gets complicated. I think it depends on what you want from your timestamps.

Do you care that the actual timestamps are accurate (wrt wall clock = MONOTONIC), or that inter-event timetamp spacing is consistent (MONOTONIC_RAW)?

For our use case, we don't really care about the absolute timestamp itself, just relative timestamp differences. We wanted to make sure that the inter-event times were always consistent, and didn't wobble about in response to cock adjustments. The affect of clock skew on timestamp deltas is miniscule.

-Dan

Quoted 9 lines

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

john stultz

author

2011-12-21 07:53 UTC #5 lore raw

Quoted 39 lines · Daniel Kurtz

Quoted 29 lines · john stultz
Quoted 9 lines · Daniel Kurtz
Quoted 8 lines · Wanlong Gao
Quoted 6 lines · john stultz
Hi
In reviewing the Android patch set, I noticed the following patch from
Arve which looks like it resolves a reasonable issue (events using CLOCK_REALTIME timestamps, which can jump forwards or backwards via settimeofday()).
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
I'm not very familiar with the evdev interface, so I'm not sure if changing the timestamps to CLOCK_MONOTONIC could cause an ABI problem with legacy apps. Even so, I wanted to send the patch out for review and consideration as it seems like a reasonable fix.
Maybe you will have more luck this time. You can read the previous thread on this exact topic, here:
https://lkml.org/lkml/2011/10/3/37
Interesting! Thanks for the link!
Quoted 7 lines
The previous attempt got bogged down when people wanted more data on use cases and how this patch would promote world peace. I strongly support the concept, but we found other ways to address our major concern at the time, so didn't invest more effort to get that patch accepted.
Just a question though, why ktime_get_ts() and not getrawmonotonic()?
Quoted 8 lines
So rawmonotonic isn't frequency corrected via NTP, while the monotonic clock is. So if you're calculating intervals, you will get more accurate times (where a second is a second) w/ ktime_get_ts().
The raw monotonic clock is really only useful for time correction applications (being able to accurately measure how much of a correction was applied), or when you really want a abstracted sense of hardware cycles.

Quoted 6 lines

And here is where it gets complicated. I think it depends on what you want from your timestamps.
Do you care that the actual timestamps are accurate (wrt wall clock = MONOTONIC), or that inter-event timetamp spacing is consistent (MONOTONIC_RAW)?

Although, hardware fluctuates as well (I've watched clock skew along with AC cycles). So neither is surely to be totally consistent.

Quoted 5 lines

For our use case, we don't really care about the absolute timestamp itself, just relative timestamp differences. We wanted to make sure that the inter-event times were always consistent, and didn't wobble about in response to cock adjustments. The affect of clock skew on timestamp deltas is miniscule.

Hrm. I'm curious. If clock skew is miniscule, any correction should be as well. Have you seen actual issues with clock freq correction here that necessitated using the raw-monotonic clock? Any details if so?

thanks
-john

Daniel Kurtz

2011-12-21 08:55 UTC #6 lore raw

Quoted 56 lines · john stultz

Quoted 39 lines · Daniel Kurtz
Quoted 29 lines · john stultz
Quoted 9 lines · Daniel Kurtz
Quoted 8 lines · Wanlong Gao
Quoted 6 lines · john stultz
Hi
In reviewing the Android patch set, I noticed the following patch from Arve which looks like it resolves a reasonable issue (events using CLOCK_REALTIME timestamps, which can jump forwards or backwards via settimeofday()).
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
I'm not very familiar with the evdev interface, so I'm not sure if changing the timestamps to CLOCK_MONOTONIC could cause an ABI problem with legacy apps. Even so, I wanted to send the patch out for review and consideration as it seems like a reasonable fix.
Maybe you will have more luck this time. You can read the previous thread on this exact topic, here:
https://lkml.org/lkml/2011/10/3/37
Interesting! Thanks for the link!
Quoted 7 lines
The previous attempt got bogged down when people wanted more data on use cases and how this patch would promote world peace. I strongly support the concept, but we found other ways to address our major concern at the time, so didn't invest more effort to get that patch accepted.
Just a question though, why ktime_get_ts() and not getrawmonotonic()?
Quoted 8 lines
So rawmonotonic isn't frequency corrected via NTP, while the monotonic clock is. So if you're calculating intervals, you will get more accurate times (where a second is a second) w/ ktime_get_ts().
The raw monotonic clock is really only useful for time correction applications (being able to accurately measure how much of a correction was applied), or when you really want a abstracted sense of hardware cycles.
Quoted 6 lines
And here is where it gets complicated. I think it depends on what you want from your timestamps.
Do you care that the actual timestamps are accurate (wrt wall clock = MONOTONIC), or that inter-event timetamp spacing is consistent (MONOTONIC_RAW)?
Although, hardware fluctuates as well (I've watched clock skew along with AC cycles). So neither is surely to be totally consistent.
Quoted 5 lines
For our use case, we don't really care about the absolute timestamp itself, just relative timestamp differences. We wanted to make sure that the inter-event times were always consistent, and didn't wobble about in response to cock adjustments. The affect of clock skew on timestamp deltas is miniscule.

Quoted 3 lines

Hrm. I'm curious. If clock skew is miniscule, any correction should be as well. Have you seen actual issues with clock freq correction here that necessitated using the raw-monotonic clock? Any details if so?

Let's say a really bad clock is 1% slow. It will be off (relative to wallclock time) by 3 seconds every 5 minutes. Such a clock is really bad and will need frequent adjustments.
* Those individual frequent clock adjustments, will probably happen
in chunks on the order of several milliseconds. If such an adjustment happens between two input events, the resulting timestamp delta error is on the same order of magnitude as the timestamp delta itself, if not orders of magnitude larger.
* However, when looking at inter-event arrival times, the affect of
this huge clock skew is still only 1%. So a 10ms event arrival time will be off by 100 us. Even for this horrible clock, this is really very small.

Real clocks have much less skew than this, however, the ntp clock adjustments would still be on the order of magnitude as the input event inter-arrival times, just much less frequent. But, the affect on an individual inter-event time is proportional to the skew.

Sorry, I don't have real data to back this up, but this is my understanding of how it will work. Hopefully, I'm explaining it clearly enough to make my point.

-Dan

Quoted 8 lines

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

john stultz

author

2011-12-21 02:29 UTC #7 lore raw

In reply to Daniel Kurtz

Quoted 9 lines · Daniel Kurtz

Maybe you will have more luck this time. You can read the previous thread on this exact topic, here:
https://lkml.org/lkml/2011/10/3/37
The previous attempt got bogged down when people wanted more data on use cases and how this patch would promote world peace. I strongly support the concept, but we found other ways to address our major concern at the time, so didn't invest more effort to get that patch accepted.

Also, are you able to discuss the alternative way you addressed the issue?

thanks
-john

Daniel Kurtz

2011-12-21 03:23 UTC #8 lore raw

Quoted 5 lines · john stultz

Quoted 4 lines · Daniel Kurtz
Maybe you will have more luck this time. You can read the previous thread on this exact topic, here:
https://lkml.org/lkml/2011/10/3/37

Quoted 8 lines

Quoted 5 lines
The previous attempt got bogged down when people wanted more data on use cases and how this patch would promote world peace. I strongly support the concept, but we found other ways to address our major concern at the time, so didn't invest more effort to get that patch accepted.
Also, are you able to discuss the alternative way you addressed the issue?

By ignoring big / backwards time jumps in time in userspace, and just ignoring smaller, forward adjustments. Admittedly inelegant, but got us past a crisis.

Quoted 4 lines

thanks
-john

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

john stultz

author

2011-12-21 03:12 UTC #9 lore raw

In reply to Daniel Kurtz

Quoted 18 lines · Daniel Kurtz

Quoted 8 lines · Wanlong Gao
Quoted 6 lines · john stultz
Hi
In reviewing the Android patch set, I noticed the following patch from
Arve which looks like it resolves a reasonable issue (events using CLOCK_REALTIME timestamps, which can jump forwards or backwards via settimeofday()).
Quoted 4 lines
Quoted 4 lines
I'm not very familiar with the evdev interface, so I'm not sure if changing the timestamps to CLOCK_MONOTONIC could cause an ABI problem with legacy apps. Even so, I wanted to send the patch out for review and consideration as it seems like a reasonable fix.
Maybe you will have more luck this time. You can read the previous thread on this exact topic, here:
https://lkml.org/lkml/2011/10/3/37

Quoted 5 lines

The previous attempt got bogged down when people wanted more data on use cases and how this patch would promote world peace. I strongly support the concept, but we found other ways to address our major concern at the time, so didn't invest more effort to get that patch accepted.

Would something more like the following be an acceptable solution?

NOTE: This is completely untested and off the cuff (all I know is it builds). Just wanted to post it here to get some feedback and I'll try to clean it up and make sure it actually works tomorrow if folks like it.

Arve: Do you think having an IOCTL switch to set the evdev to monotonic time would be acceptable for Android?

thanks
-john

As noted by Arve and others, since wall time can jump backwards, it is difficult to use for input because one cannot determine if one event occurred before another or for how long a key was pressed.

However, the timestamp field is part of the kernel ABI, and cannot be changed without possibly breaking existing users.

This patch adds a new IOCTL that sets a flag in the evdev_client struct that will switch the timestamps to CLOCK_MONOTONIC instead of CLOCK_REALTIME. This allows users of the evdev to specify which clock id they want the timestamps to use.

The default remains CLOCK_REALTIME.

CC Dmitry Torokhov <[email protected]>

CC [email protected]

CC Arve Hjønnevåg <[email protected]>

Signed-off-by John Stultz <[email protected]>

---
 drivers/input/evdev.c |   21 ++++++++++++++++++++-
 include/linux/input.h |    2 ++
 2 files changed, 22 insertions(+), 1 deletions(-)

drivers/input/evdev.c +20 -1

index 4cf2534..9069fab 100644
@@ -46,6 +46,7 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
+	bool use_monotonic;
 	unsigned int bufsize;
 	struct input_event buffer[];
 };

@@ -94,8 +95,12 @@ static void evdev_event(struct input_handle *handle,
 	struct evdev *evdev = handle->private;
 	struct evdev_client *client;
 	struct input_event event;
+	ktime_t time_mono, time_real;
+	struct timespec ts;
+
+	time_mono = ktime_get();
+	time_real = ktime_sub(time_mono, ktime_get_monotonic_offset());
 
-	do_gettimeofday(&event.time);
 	event.type = type;
 	event.code = code;
 	event.value = value;

@@ -103,6 +108,14 @@ static void evdev_event(struct input_handle *handle,
 	rcu_read_lock();
 
 	client = rcu_dereference(evdev->grab);
+
+	if (client->use_monotonic)
+		ts = ktime_to_timespec(time_mono);
+	else
+		ts = ktime_to_timespec(time_real);
+	event.time.tv_sec = ts.tv_sec;
+	event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+
 	if (client)
 		evdev_pass_event(client, &event);
 	else

@@ -683,6 +696,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCMONTIME:
+		if (copy_from_user(&i, p, sizeof(unsigned int)))
+			return -EFAULT;
+		client->use_monotonic = i;
+		return 0;
+
 	case EVIOCGKEYCODE:
 		return evdev_handle_get_keycode(dev, p);

include/linux/input.h +2 -0

index 3862e32..245bfcc 100644
@@ -129,6 +129,8 @@ struct input_keymap_entry {
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
 
+#define EVIOCMONTIME		_IOW('E', 0xA0, int)			/* Set CLOCK_MONOTONIC Timestamps */
+
 /*
  * Device properties and quirks
  */

Signature · 8 lines

1.7.3.2.146.gca209



--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html