* [PATCH] Input: Add support for CLOCK_BOOTTIME
@ 2014-12-16 16:56 Aniroop Mathur
2014-12-17 18:20 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Aniroop Mathur @ 2014-12-16 16:56 UTC (permalink / raw)
To: dmitry.torokhov, dtor, linux-input; +Cc: aniroop.mathur, a.mathur
This patch adds support for CLOCK_BOOTTIME for input event timestamp.
CLOCK_BOOTTIME includes suspend time, so it would allow aplications
to get correct time difference between two events even when system
resumes from suspend state.
Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
drivers/input/evdev.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index de05545..7825794 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -28,6 +28,13 @@
#include <linux/cdev.h>
#include "input-compat.h"
+enum clock_type {
+ REAL = 0,
+ MONO,
+ BOOT,
+ CLK_MAX
+};
+
struct evdev {
int open;
struct input_handle handle;
@@ -108,8 +115,9 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
struct input_event ev;
ktime_t time;
- time = (client->clkid == CLOCK_MONOTONIC) ?
- ktime_get() : ktime_get_real();
+ time = (client->clkid == CLOCK_REALTIME) ?
+ ktime_get_real() : (client->clkid == CLOCK_MONOTONIC) ?
+ ktime_get() : ktime_get_boottime();
ev.time = ktime_to_timeval(time);
ev.type = EV_SYN;
@@ -159,7 +167,7 @@ static void __pass_event(struct evdev_client *client,
static void evdev_pass_values(struct evdev_client *client,
const struct input_value *vals, unsigned int count,
- ktime_t mono, ktime_t real)
+ ktime_t *ev_time)
{
struct evdev *evdev = client->evdev;
const struct input_value *v;
@@ -169,8 +177,9 @@ static void evdev_pass_values(struct evdev_client *client,
if (client->revoked)
return;
- event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
- mono : real);
+ event.time = ktime_to_timeval(client->clkid == CLOCK_REALTIME ?
+ ev_time[REAL] : client->clkid == CLOCK_MONOTONIC ?
+ ev_time[MONO] : ev_time[BOOT]);
/* Interrupts are disabled, just acquire the lock. */
spin_lock(&client->buffer_lock);
@@ -198,21 +207,21 @@ static void evdev_events(struct input_handle *handle,
{
struct evdev *evdev = handle->private;
struct evdev_client *client;
- ktime_t time_mono, time_real;
+ ktime_t ev_time[CLK_MAX];
- time_mono = ktime_get();
- time_real = ktime_mono_to_real(time_mono);
+ ev_time[MONO] = ktime_get();
+ ev_time[REAL] = ktime_mono_to_real(ev_time[MONO]);
+ ev_time[BOOT] = ktime_get_boottime();
rcu_read_lock();
client = rcu_dereference(evdev->grab);
if (client)
- evdev_pass_values(client, vals, count, time_mono, time_real);
+ evdev_pass_values(client, vals, count, ev_time);
else
list_for_each_entry_rcu(client, &evdev->client_list, node)
- evdev_pass_values(client, vals, count,
- time_mono, time_real);
+ evdev_pass_values(client, vals, count, ev_time);
rcu_read_unlock();
}
@@ -874,7 +883,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
case EVIOCSCLOCKID:
if (copy_from_user(&i, p, sizeof(unsigned int)))
return -EFAULT;
- if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME)
+ if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME && i != CLOCK_BOOTTIME)
return -EINVAL;
client->clkid = i;
return 0;
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Input: Add support for CLOCK_BOOTTIME
2014-12-16 16:56 [PATCH] Input: Add support for CLOCK_BOOTTIME Aniroop Mathur
@ 2014-12-17 18:20 ` Dmitry Torokhov
2014-12-17 21:33 ` Aniroop Mathur
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2014-12-17 18:20 UTC (permalink / raw)
To: Aniroop Mathur; +Cc: linux-input, a.mathur, Thomas Gleixner, John Stultz
Hi Aniroop,
On Tue, Dec 16, 2014 at 10:26:34PM +0530, Aniroop Mathur wrote:
> This patch adds support for CLOCK_BOOTTIME for input event timestamp.
> CLOCK_BOOTTIME includes suspend time, so it would allow aplications
> to get correct time difference between two events even when system
> resumes from suspend state.
>
> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
> ---
> drivers/input/evdev.c | 33 +++++++++++++++++++++------------
> 1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index de05545..7825794 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -28,6 +28,13 @@
> #include <linux/cdev.h>
> #include "input-compat.h"
>
> +enum clock_type {
> + REAL = 0,
> + MONO,
> + BOOT,
> + CLK_MAX
> +};
> +
> struct evdev {
> int open;
> struct input_handle handle;
> @@ -108,8 +115,9 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
> struct input_event ev;
> ktime_t time;
>
> - time = (client->clkid == CLOCK_MONOTONIC) ?
> - ktime_get() : ktime_get_real();
> + time = (client->clkid == CLOCK_REALTIME) ?
> + ktime_get_real() : (client->clkid == CLOCK_MONOTONIC) ?
> + ktime_get() : ktime_get_boottime();
>
> ev.time = ktime_to_timeval(time);
> ev.type = EV_SYN;
> @@ -159,7 +167,7 @@ static void __pass_event(struct evdev_client *client,
>
> static void evdev_pass_values(struct evdev_client *client,
> const struct input_value *vals, unsigned int count,
> - ktime_t mono, ktime_t real)
> + ktime_t *ev_time)
> {
> struct evdev *evdev = client->evdev;
> const struct input_value *v;
> @@ -169,8 +177,9 @@ static void evdev_pass_values(struct evdev_client *client,
> if (client->revoked)
> return;
>
> - event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
> - mono : real);
> + event.time = ktime_to_timeval(client->clkid == CLOCK_REALTIME ?
> + ev_time[REAL] : client->clkid == CLOCK_MONOTONIC ?
> + ev_time[MONO] : ev_time[BOOT]);
This becomes unwieldy. Should we have client->clk_type instead of
client->clkid and convert CLOCK_* into your EVDEV_CLK_{REAL|MONO|BOOT}
when setting it in ioctl and then do
event.time = ktime_to_timeval(ev_time[client->clk_offset]);
>
> /* Interrupts are disabled, just acquire the lock. */
> spin_lock(&client->buffer_lock);
> @@ -198,21 +207,21 @@ static void evdev_events(struct input_handle *handle,
> {
> struct evdev *evdev = handle->private;
> struct evdev_client *client;
> - ktime_t time_mono, time_real;
> + ktime_t ev_time[CLK_MAX];
>
> - time_mono = ktime_get();
> - time_real = ktime_mono_to_real(time_mono);
> + ev_time[MONO] = ktime_get();
> + ev_time[REAL] = ktime_mono_to_real(ev_time[MONO]);
> + ev_time[BOOT] = ktime_get_boottime();
I do not think we want to use ktime_get_boottime() here; I'd rather you
use ktime_mono_to_anY(ev_time[MONO], TK_OFFS_BOOT) so that all 3 times
are consistent.
>
> rcu_read_lock();
>
> client = rcu_dereference(evdev->grab);
>
> if (client)
> - evdev_pass_values(client, vals, count, time_mono, time_real);
> + evdev_pass_values(client, vals, count, ev_time);
> else
> list_for_each_entry_rcu(client, &evdev->client_list, node)
> - evdev_pass_values(client, vals, count,
> - time_mono, time_real);
> + evdev_pass_values(client, vals, count, ev_time);
>
> rcu_read_unlock();
> }
> @@ -874,7 +883,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> case EVIOCSCLOCKID:
> if (copy_from_user(&i, p, sizeof(unsigned int)))
> return -EFAULT;
> - if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME)
> + if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME && i != CLOCK_BOOTTIME)
> return -EINVAL;
> client->clkid = i;
> return 0;
> --
> 1.9.1
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Input: Add support for CLOCK_BOOTTIME
2014-12-17 18:20 ` Dmitry Torokhov
@ 2014-12-17 21:33 ` Aniroop Mathur
2014-12-17 22:03 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Aniroop Mathur @ 2014-12-17 21:33 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, a.mathur, Thomas Gleixner,
John Stultz
Hello Mr. Torokhov,
On Wed, Dec 17, 2014 at 11:50 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Aniroop,
>
> On Tue, Dec 16, 2014 at 10:26:34PM +0530, Aniroop Mathur wrote:
>> This patch adds support for CLOCK_BOOTTIME for input event timestamp.
>> CLOCK_BOOTTIME includes suspend time, so it would allow aplications
>> to get correct time difference between two events even when system
>> resumes from suspend state.
>>
>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>> ---
>> drivers/input/evdev.c | 33 +++++++++++++++++++++------------
>> 1 file changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index de05545..7825794 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -28,6 +28,13 @@
>> #include <linux/cdev.h>
>> #include "input-compat.h"
>>
>> +enum clock_type {
>> + REAL = 0,
>> + MONO,
>> + BOOT,
>> + CLK_MAX
>> +};
>> +
>> struct evdev {
>> int open;
>> struct input_handle handle;
>> @@ -108,8 +115,9 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
>> struct input_event ev;
>> ktime_t time;
>>
>> - time = (client->clkid == CLOCK_MONOTONIC) ?
>> - ktime_get() : ktime_get_real();
>> + time = (client->clkid == CLOCK_REALTIME) ?
>> + ktime_get_real() : (client->clkid == CLOCK_MONOTONIC) ?
>> + ktime_get() : ktime_get_boottime();
>>
Is this okay ?
To improve readabilty,
how about adding a new funtion to get time depending upon clkid like below ?
time = get_clk_time(client->clkid);
static ktime_t get_clk_time(int id)
{
switch (id) {
case CLOCK_REALTIME: return ktime_get_real();
case CLOCK_MONOTONIC: return ktime_get();
case CLOCK_BOOTTIME: return ktime_get_bootime();
}
}
>> ev.time = ktime_to_timeval(time);
>> ev.type = EV_SYN;
>> @@ -159,7 +167,7 @@ static void __pass_event(struct evdev_client *client,
>>
>> static void evdev_pass_values(struct evdev_client *client,
>> const struct input_value *vals, unsigned int count,
>> - ktime_t mono, ktime_t real)
>> + ktime_t *ev_time)
>> {
>> struct evdev *evdev = client->evdev;
>> const struct input_value *v;
>> @@ -169,8 +177,9 @@ static void evdev_pass_values(struct evdev_client *client,
>> if (client->revoked)
>> return;
>>
>> - event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
>> - mono : real);
>> + event.time = ktime_to_timeval(client->clkid == CLOCK_REALTIME ?
>> + ev_time[REAL] : client->clkid == CLOCK_MONOTONIC ?
>> + ev_time[MONO] : ev_time[BOOT]);
>
> This becomes unwieldy. Should we have client->clk_type instead of
> client->clkid and convert CLOCK_* into your EVDEV_CLK_{REAL|MONO|BOOT}
> when setting it in ioctl and then do
>
> event.time = ktime_to_timeval(ev_time[client->clk_offset]);
>
Sure, using clk_type would be better and improve code readability.
I am afraid how converting CLOCK_* into your
EVDEV_CLK_{REAL|MONO|BOOT} will help
because we cannot have ev_time array size more than CLK_MAX (3).
Did you mean to have following changes:
1. Add clk_type variable
struct evdev_client {
int clkid:
+ int clk_type;
}
2. Add new funtion for setting clk_type to improve readability
static int set_clk_type(struct evdev_client *client, int id)
{
switch (id) {
case CLOCK_REALTIME: client->clk_type = EV_REAL; break;
case CLOCK_MONOTONIC: client->clk_type = EV_MONO; break;
case CLOCK_BOOTTIME: client->clk_type = EV_BOOT; break;
default: return -1
}
return 0;
}
3. Addition in ioctl call
case EVIOCSCLOCKID:
...
if (!set_clk_type())
return -EINVAL;
client->clkid = i;
return 0;
4. Change in evdev_pass_values function
event.time = ktime_to_timeval(ev_time[client->clk_type]);
>>
>> /* Interrupts are disabled, just acquire the lock. */
>> spin_lock(&client->buffer_lock);
>> @@ -198,21 +207,21 @@ static void evdev_events(struct input_handle *handle,
>> {
>> struct evdev *evdev = handle->private;
>> struct evdev_client *client;
>> - ktime_t time_mono, time_real;
>> + ktime_t ev_time[CLK_MAX];
>>
>> - time_mono = ktime_get();
>> - time_real = ktime_mono_to_real(time_mono);
>> + ev_time[MONO] = ktime_get();
>> + ev_time[REAL] = ktime_mono_to_real(ev_time[MONO]);
>> + ev_time[BOOT] = ktime_get_boottime();
>
> I do not think we want to use ktime_get_boottime() here; I'd rather you
> use ktime_mono_to_anY(ev_time[MONO], TK_OFFS_BOOT) so that all 3 times
> are consistent.
>
Sure. I will send the updated patch with this change.
>>
>> rcu_read_lock();
>>
>> client = rcu_dereference(evdev->grab);
>>
>> if (client)
>> - evdev_pass_values(client, vals, count, time_mono, time_real);
>> + evdev_pass_values(client, vals, count, ev_time);
>> else
>> list_for_each_entry_rcu(client, &evdev->client_list, node)
>> - evdev_pass_values(client, vals, count,
>> - time_mono, time_real);
>> + evdev_pass_values(client, vals, count, ev_time);
>>
>> rcu_read_unlock();
>> }
>> @@ -874,7 +883,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>> case EVIOCSCLOCKID:
>> if (copy_from_user(&i, p, sizeof(unsigned int)))
>> return -EFAULT;
>> - if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME)
>> + if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME && i != CLOCK_BOOTTIME)
>> return -EINVAL;
>> client->clkid = i;
>> return 0;
>> --
>> 1.9.1
>>
>
> Thanks.
>
> --
> Dmitry
Thanks,
Aniroop Mathur
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Input: Add support for CLOCK_BOOTTIME
2014-12-17 21:33 ` Aniroop Mathur
@ 2014-12-17 22:03 ` Dmitry Torokhov
2014-12-17 23:11 ` Aniroop Mathur
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2014-12-17 22:03 UTC (permalink / raw)
To: Aniroop Mathur
Cc: linux-input@vger.kernel.org, a.mathur, Thomas Gleixner,
John Stultz
On Thu, Dec 18, 2014 at 03:03:26AM +0530, Aniroop Mathur wrote:
> Hello Mr. Torokhov,
>
> On Wed, Dec 17, 2014 at 11:50 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Aniroop,
> >
> > On Tue, Dec 16, 2014 at 10:26:34PM +0530, Aniroop Mathur wrote:
> >> This patch adds support for CLOCK_BOOTTIME for input event timestamp.
> >> CLOCK_BOOTTIME includes suspend time, so it would allow aplications
> >> to get correct time difference between two events even when system
> >> resumes from suspend state.
> >>
> >> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
> >> ---
> >> drivers/input/evdev.c | 33 +++++++++++++++++++++------------
> >> 1 file changed, 21 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> >> index de05545..7825794 100644
> >> --- a/drivers/input/evdev.c
> >> +++ b/drivers/input/evdev.c
> >> @@ -28,6 +28,13 @@
> >> #include <linux/cdev.h>
> >> #include "input-compat.h"
> >>
> >> +enum clock_type {
> >> + REAL = 0,
> >> + MONO,
> >> + BOOT,
> >> + CLK_MAX
> >> +};
> >> +
> >> struct evdev {
> >> int open;
> >> struct input_handle handle;
> >> @@ -108,8 +115,9 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
> >> struct input_event ev;
> >> ktime_t time;
> >>
> >> - time = (client->clkid == CLOCK_MONOTONIC) ?
> >> - ktime_get() : ktime_get_real();
> >> + time = (client->clkid == CLOCK_REALTIME) ?
> >> + ktime_get_real() : (client->clkid == CLOCK_MONOTONIC) ?
> >> + ktime_get() : ktime_get_boottime();
> >>
>
> Is this okay ?
> To improve readabilty,
> how about adding a new funtion to get time depending upon clkid like below ?
> time = get_clk_time(client->clkid);
>
> static ktime_t get_clk_time(int id)
> {
> switch (id) {
>
> case CLOCK_REALTIME: return ktime_get_real();
> case CLOCK_MONOTONIC: return ktime_get();
> case CLOCK_BOOTTIME: return ktime_get_bootime();
> }
> }
Just open-code it in evdev_queue_syn_dropped() since there is only place
where we call it. And use clk_type instead of clock id-.
>
> >> ev.time = ktime_to_timeval(time);
> >> ev.type = EV_SYN;
> >> @@ -159,7 +167,7 @@ static void __pass_event(struct evdev_client *client,
> >>
> >> static void evdev_pass_values(struct evdev_client *client,
> >> const struct input_value *vals, unsigned int count,
> >> - ktime_t mono, ktime_t real)
> >> + ktime_t *ev_time)
> >> {
> >> struct evdev *evdev = client->evdev;
> >> const struct input_value *v;
> >> @@ -169,8 +177,9 @@ static void evdev_pass_values(struct evdev_client *client,
> >> if (client->revoked)
> >> return;
> >>
> >> - event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
> >> - mono : real);
> >> + event.time = ktime_to_timeval(client->clkid == CLOCK_REALTIME ?
> >> + ev_time[REAL] : client->clkid == CLOCK_MONOTONIC ?
> >> + ev_time[MONO] : ev_time[BOOT]);
> >
> > This becomes unwieldy. Should we have client->clk_type instead of
> > client->clkid and convert CLOCK_* into your EVDEV_CLK_{REAL|MONO|BOOT}
> > when setting it in ioctl and then do
> >
> > event.time = ktime_to_timeval(ev_time[client->clk_offset]);
> >
>
> Sure, using clk_type would be better and improve code readability.
>
> I am afraid how converting CLOCK_* into your
> EVDEV_CLK_{REAL|MONO|BOOT} will help
> because we cannot have ev_time array size more than CLK_MAX (3).
>
> Did you mean to have following changes:
>
> 1. Add clk_type variable
> struct evdev_client {
> int clkid:
> + int clk_type;
> }
I meant replacing clkid with clk_type in evdev_client.
>
> 2. Add new funtion for setting clk_type to improve readability
> static int set_clk_type(struct evdev_client *client, int id)
evdev_set_clk_type() to keep with the style.
> {
> switch (id) {
>
> case CLOCK_REALTIME: client->clk_type = EV_REAL; break;
> case CLOCK_MONOTONIC: client->clk_type = EV_MONO; break;
> case CLOCK_BOOTTIME: client->clk_type = EV_BOOT; break;
> default: return -1
-EINVAL here, otherwise yes.
> }
>
> return 0;
> }
>
> 3. Addition in ioctl call
> case EVIOCSCLOCKID:
> ...
> if (!set_clk_type())
> return -EINVAL;
>
error = evdev_set_clktype(i);
if (error)
return error;
> client->clkid = i;
Drop this.
> return 0;
>
> 4. Change in evdev_pass_values function
> event.time = ktime_to_timeval(ev_time[client->clk_type]);
Yes.
>
> >>
> >> /* Interrupts are disabled, just acquire the lock. */
> >> spin_lock(&client->buffer_lock);
> >> @@ -198,21 +207,21 @@ static void evdev_events(struct input_handle *handle,
> >> {
> >> struct evdev *evdev = handle->private;
> >> struct evdev_client *client;
> >> - ktime_t time_mono, time_real;
> >> + ktime_t ev_time[CLK_MAX];
> >>
> >> - time_mono = ktime_get();
> >> - time_real = ktime_mono_to_real(time_mono);
> >> + ev_time[MONO] = ktime_get();
> >> + ev_time[REAL] = ktime_mono_to_real(ev_time[MONO]);
> >> + ev_time[BOOT] = ktime_get_boottime();
> >
> > I do not think we want to use ktime_get_boottime() here; I'd rather you
> > use ktime_mono_to_anY(ev_time[MONO], TK_OFFS_BOOT) so that all 3 times
> > are consistent.
> >
>
> Sure. I will send the updated patch with this change.
>
> >>
> >> rcu_read_lock();
> >>
> >> client = rcu_dereference(evdev->grab);
> >>
> >> if (client)
> >> - evdev_pass_values(client, vals, count, time_mono, time_real);
> >> + evdev_pass_values(client, vals, count, ev_time);
> >> else
> >> list_for_each_entry_rcu(client, &evdev->client_list, node)
> >> - evdev_pass_values(client, vals, count,
> >> - time_mono, time_real);
> >> + evdev_pass_values(client, vals, count, ev_time);
> >>
> >> rcu_read_unlock();
> >> }
> >> @@ -874,7 +883,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> >> case EVIOCSCLOCKID:
> >> if (copy_from_user(&i, p, sizeof(unsigned int)))
> >> return -EFAULT;
> >> - if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME)
> >> + if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME && i != CLOCK_BOOTTIME)
> >> return -EINVAL;
> >> client->clkid = i;
> >> return 0;
> >> --
> >> 1.9.1
> >>
> >
> > Thanks.
> >
> > --
> > Dmitry
>
> Thanks,
> Aniroop Mathur
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Input: Add support for CLOCK_BOOTTIME
2014-12-17 22:03 ` Dmitry Torokhov
@ 2014-12-17 23:11 ` Aniroop Mathur
0 siblings, 0 replies; 6+ messages in thread
From: Aniroop Mathur @ 2014-12-17 23:11 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, a.mathur, Thomas Gleixner,
John Stultz
Hello Mr. Torokhov,
On Thu, Dec 18, 2014 at 3:33 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Dec 18, 2014 at 03:03:26AM +0530, Aniroop Mathur wrote:
>> Hello Mr. Torokhov,
>>
>> On Wed, Dec 17, 2014 at 11:50 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > Hi Aniroop,
>> >
>> > On Tue, Dec 16, 2014 at 10:26:34PM +0530, Aniroop Mathur wrote:
>> >> This patch adds support for CLOCK_BOOTTIME for input event timestamp.
>> >> CLOCK_BOOTTIME includes suspend time, so it would allow aplications
>> >> to get correct time difference between two events even when system
>> >> resumes from suspend state.
>> >>
>> >> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>> >> ---
>> >> drivers/input/evdev.c | 33 +++++++++++++++++++++------------
>> >> 1 file changed, 21 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> >> index de05545..7825794 100644
>> >> --- a/drivers/input/evdev.c
>> >> +++ b/drivers/input/evdev.c
>> >> @@ -28,6 +28,13 @@
>> >> #include <linux/cdev.h>
>> >> #include "input-compat.h"
>> >>
>> >> +enum clock_type {
>> >> + REAL = 0,
>> >> + MONO,
>> >> + BOOT,
>> >> + CLK_MAX
>> >> +};
>> >> +
>> >> struct evdev {
>> >> int open;
>> >> struct input_handle handle;
>> >> @@ -108,8 +115,9 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
>> >> struct input_event ev;
>> >> ktime_t time;
>> >>
>> >> - time = (client->clkid == CLOCK_MONOTONIC) ?
>> >> - ktime_get() : ktime_get_real();
>> >> + time = (client->clkid == CLOCK_REALTIME) ?
>> >> + ktime_get_real() : (client->clkid == CLOCK_MONOTONIC) ?
>> >> + ktime_get() : ktime_get_boottime();
>> >>
>>
>> Is this okay ?
>> To improve readabilty,
>> how about adding a new funtion to get time depending upon clkid like below ?
>> time = get_clk_time(client->clkid);
>>
>> static ktime_t get_clk_time(int id)
>> {
>> switch (id) {
>>
>> case CLOCK_REALTIME: return ktime_get_real();
>> case CLOCK_MONOTONIC: return ktime_get();
>> case CLOCK_BOOTTIME: return ktime_get_bootime();
>> }
>> }
>
> Just open-code it in evdev_queue_syn_dropped() since there is only place
> where we call it. And use clk_type instead of clock id-.
>
>>
>> >> ev.time = ktime_to_timeval(time);
>> >> ev.type = EV_SYN;
>> >> @@ -159,7 +167,7 @@ static void __pass_event(struct evdev_client *client,
>> >>
>> >> static void evdev_pass_values(struct evdev_client *client,
>> >> const struct input_value *vals, unsigned int count,
>> >> - ktime_t mono, ktime_t real)
>> >> + ktime_t *ev_time)
>> >> {
>> >> struct evdev *evdev = client->evdev;
>> >> const struct input_value *v;
>> >> @@ -169,8 +177,9 @@ static void evdev_pass_values(struct evdev_client *client,
>> >> if (client->revoked)
>> >> return;
>> >>
>> >> - event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
>> >> - mono : real);
>> >> + event.time = ktime_to_timeval(client->clkid == CLOCK_REALTIME ?
>> >> + ev_time[REAL] : client->clkid == CLOCK_MONOTONIC ?
>> >> + ev_time[MONO] : ev_time[BOOT]);
>> >
>> > This becomes unwieldy. Should we have client->clk_type instead of
>> > client->clkid and convert CLOCK_* into your EVDEV_CLK_{REAL|MONO|BOOT}
>> > when setting it in ioctl and then do
>> >
>> > event.time = ktime_to_timeval(ev_time[client->clk_offset]);
>> >
>>
>> Sure, using clk_type would be better and improve code readability.
>>
>> I am afraid how converting CLOCK_* into your
>> EVDEV_CLK_{REAL|MONO|BOOT} will help
>> because we cannot have ev_time array size more than CLK_MAX (3).
>>
>> Did you mean to have following changes:
>>
>> 1. Add clk_type variable
>> struct evdev_client {
>> int clkid:
>> + int clk_type;
>> }
>
> I meant replacing clkid with clk_type in evdev_client.
>
>>
>> 2. Add new funtion for setting clk_type to improve readability
>> static int set_clk_type(struct evdev_client *client, int id)
>
>
> evdev_set_clk_type() to keep with the style.
>
>> {
>> switch (id) {
>>
>> case CLOCK_REALTIME: client->clk_type = EV_REAL; break;
>> case CLOCK_MONOTONIC: client->clk_type = EV_MONO; break;
>> case CLOCK_BOOTTIME: client->clk_type = EV_BOOT; break;
>> default: return -1
>
> -EINVAL here, otherwise yes.
>
>> }
>>
>> return 0;
>> }
>>
>> 3. Addition in ioctl call
>> case EVIOCSCLOCKID:
>> ...
>> if (!set_clk_type())
>> return -EINVAL;
>>
>
> error = evdev_set_clktype(i);
> if (error)
> return error;
>
>> client->clkid = i;
>
> Drop this.
>
>> return 0;
>>
>> 4. Change in evdev_pass_values function
>> event.time = ktime_to_timeval(ev_time[client->clk_type]);
>
> Yes.
>
>>
>> >>
>> >> /* Interrupts are disabled, just acquire the lock. */
>> >> spin_lock(&client->buffer_lock);
>> >> @@ -198,21 +207,21 @@ static void evdev_events(struct input_handle *handle,
>> >> {
>> >> struct evdev *evdev = handle->private;
>> >> struct evdev_client *client;
>> >> - ktime_t time_mono, time_real;
>> >> + ktime_t ev_time[CLK_MAX];
>> >>
>> >> - time_mono = ktime_get();
>> >> - time_real = ktime_mono_to_real(time_mono);
>> >> + ev_time[MONO] = ktime_get();
>> >> + ev_time[REAL] = ktime_mono_to_real(ev_time[MONO]);
>> >> + ev_time[BOOT] = ktime_get_boottime();
>> >
>> > I do not think we want to use ktime_get_boottime() here; I'd rather you
>> > use ktime_mono_to_anY(ev_time[MONO], TK_OFFS_BOOT) so that all 3 times
>> > are consistent.
>> >
>>
>> Sure. I will send the updated patch with this change.
>>
>> >>
>> >> rcu_read_lock();
>> >>
>> >> client = rcu_dereference(evdev->grab);
>> >>
>> >> if (client)
>> >> - evdev_pass_values(client, vals, count, time_mono, time_real);
>> >> + evdev_pass_values(client, vals, count, ev_time);
>> >> else
>> >> list_for_each_entry_rcu(client, &evdev->client_list, node)
>> >> - evdev_pass_values(client, vals, count,
>> >> - time_mono, time_real);
>> >> + evdev_pass_values(client, vals, count, ev_time);
>> >>
>> >> rcu_read_unlock();
>> >> }
>> >> @@ -874,7 +883,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>> >> case EVIOCSCLOCKID:
>> >> if (copy_from_user(&i, p, sizeof(unsigned int)))
>> >> return -EFAULT;
>> >> - if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME)
>> >> + if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME && i != CLOCK_BOOTTIME)
>> >> return -EINVAL;
>> >> client->clkid = i;
>> >> return 0;
>> >> --
>> >> 1.9.1
>> >>
>> >
>> > Thanks.
>> >
>> > --
>> > Dmitry
>>
>> Thanks,
>> Aniroop Mathur
>
> --
> Dmitry
Thanks for the suggestions !!
I have sent you the updated patch with new changes done.
Thanks,
Aniroop
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Input: Add support for CLOCK_BOOTTIME
@ 2014-12-16 8:23 Aniroop, Mathur
0 siblings, 0 replies; 6+ messages in thread
From: Aniroop, Mathur @ 2014-12-16 8:23 UTC (permalink / raw)
To: dmitry.torokhov, dtor, linux-input; +Cc: aniroop.mathur, a.mathur
From: Aniroop Mathur <a.mathur@samsung.com>
This patch adds support for CLOCK_BOOTTIME for input event timestamp.
CLOCK_BOOTTIME includes suspend time, so it would allow applications
to get correct time difference between two events even when system
resumes from suspend state.
Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
drivers/input/evdev.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index de05545..7825794 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -28,6 +28,13 @@
#include <linux/cdev.h>
#include "input-compat.h"
+enum clock_type {
+ REAL = 0,
+ MONO,
+ BOOT,
+ CLK_MAX
+};
+
struct evdev {
int open;
struct input_handle handle;
@@ -108,8 +115,9 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
struct input_event ev;
ktime_t time;
- time = (client->clkid == CLOCK_MONOTONIC) ?
- ktime_get() : ktime_get_real();
+ time = (client->clkid == CLOCK_REALTIME) ?
+ ktime_get_real() : (client->clkid == CLOCK_MONOTONIC) ?
+ ktime_get() : ktime_get_boottime();
ev.time = ktime_to_timeval(time);
ev.type = EV_SYN;
@@ -159,7 +167,7 @@ static void __pass_event(struct evdev_client *client,
static void evdev_pass_values(struct evdev_client *client,
const struct input_value *vals, unsigned int count,
- ktime_t mono, ktime_t real)
+ ktime_t *ev_time)
{
struct evdev *evdev = client->evdev;
const struct input_value *v;
@@ -169,8 +177,9 @@ static void evdev_pass_values(struct evdev_client *client,
if (client->revoked)
return;
- event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
- mono : real);
+ event.time = ktime_to_timeval(client->clkid == CLOCK_REALTIME ?
+ ev_time[REAL] : client->clkid == CLOCK_MONOTONIC ?
+ ev_time[MONO] : ev_time[BOOT]);
/* Interrupts are disabled, just acquire the lock. */
spin_lock(&client->buffer_lock);
@@ -198,21 +207,21 @@ static void evdev_events(struct input_handle *handle,
{
struct evdev *evdev = handle->private;
struct evdev_client *client;
- ktime_t time_mono, time_real;
+ ktime_t ev_time[CLK_MAX];
- time_mono = ktime_get();
- time_real = ktime_mono_to_real(time_mono);
+ ev_time[MONO] = ktime_get();
+ ev_time[REAL] = ktime_mono_to_real(ev_time[MONO]);
+ ev_time[BOOT] = ktime_get_boottime();
rcu_read_lock();
client = rcu_dereference(evdev->grab);
if (client)
- evdev_pass_values(client, vals, count, time_mono, time_real);
+ evdev_pass_values(client, vals, count, ev_time);
else
list_for_each_entry_rcu(client, &evdev->client_list, node)
- evdev_pass_values(client, vals, count,
- time_mono, time_real);
+ evdev_pass_values(client, vals, count, ev_time);
rcu_read_unlock();
}
@@ -874,7 +883,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
case EVIOCSCLOCKID:
if (copy_from_user(&i, p, sizeof(unsigned int)))
return -EFAULT;
- if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME)
+ if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME && i != CLOCK_BOOTTIME)
return -EINVAL;
client->clkid = i;
return 0;
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-17 23:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-16 16:56 [PATCH] Input: Add support for CLOCK_BOOTTIME Aniroop Mathur
2014-12-17 18:20 ` Dmitry Torokhov
2014-12-17 21:33 ` Aniroop Mathur
2014-12-17 22:03 ` Dmitry Torokhov
2014-12-17 23:11 ` Aniroop Mathur
-- strict thread matches above, loose matches on Subject: below --
2014-12-16 8:23 Aniroop, Mathur
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).