* [PATCH 2/8] [media] uvc_video: use ktime_t for timestamps
2017-11-27 13:19 [PATCH 1/8] [media] uvc_video: use ktime_t for stats Arnd Bergmann
@ 2017-11-27 13:19 ` Arnd Bergmann
2017-12-05 0:37 ` Laurent Pinchart
2017-11-27 13:19 ` [PATCH 3/8] [media] solo6x10: use ktime_get_ts64() for time sync Arnd Bergmann
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-11-27 13:19 UTC (permalink / raw)
To: Laurent Pinchart, Mauro Carvalho Chehab
Cc: y2038, Arnd Bergmann, Kieran Bingham, Guennadi Liakhovetski,
Hans Verkuil, linux-media, linux-kernel
uvc_video_get_ts() returns a 'struct timespec', but all its users
really want a nanoseconds variable anyway.
Changing the deprecated ktime_get_ts/ktime_get_real_ts to ktime_get
and ktime_get_real simplifies the code noticeably, while keeping
the resulting numbers unchanged.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/usb/uvc/uvc_video.c | 37 ++++++++++++-------------------------
drivers/media/usb/uvc/uvcvideo.h | 2 +-
2 files changed, 13 insertions(+), 26 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index d6bee37cd1b8..f7a919490b2b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -369,12 +369,12 @@ static int uvc_commit_video(struct uvc_streaming *stream,
* Clocks and timestamps
*/
-static inline void uvc_video_get_ts(struct timespec *ts)
+static inline ktime_t uvc_video_get_time(void)
{
if (uvc_clock_param == CLOCK_MONOTONIC)
- ktime_get_ts(ts);
+ return ktime_get();
else
- ktime_get_real_ts(ts);
+ return ktime_get_real();
}
static void
@@ -386,7 +386,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
bool has_pts = false;
bool has_scr = false;
unsigned long flags;
- struct timespec ts;
+ ktime_t time;
u16 host_sof;
u16 dev_sof;
@@ -436,7 +436,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
stream->clock.last_sof = dev_sof;
host_sof = usb_get_current_frame_number(stream->dev->udev);
- uvc_video_get_ts(&ts);
+ time = uvc_video_get_time();
/* The UVC specification allows device implementations that can't obtain
* the USB frame number to keep their own frame counters as long as they
@@ -473,7 +473,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
sample->dev_stc = get_unaligned_le32(&data[header_size - 6]);
sample->dev_sof = dev_sof;
sample->host_sof = host_sof;
- sample->host_ts = ts;
+ sample->host_time = time;
/* Update the sliding window head and count. */
stream->clock.head = (stream->clock.head + 1) % stream->clock.size;
@@ -613,14 +613,12 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
struct uvc_clock_sample *first;
struct uvc_clock_sample *last;
unsigned long flags;
- struct timespec ts;
+ u64 timestamp;
u32 delta_stc;
u32 y1, y2;
u32 x1, x2;
u32 mean;
u32 sof;
- u32 div;
- u32 rem;
u64 y;
if (!uvc_hw_timestamps_param)
@@ -667,9 +665,8 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
if (x1 == x2)
goto done;
- ts = timespec_sub(last->host_ts, first->host_ts);
y1 = NSEC_PER_SEC;
- y2 = (ts.tv_sec + 1) * NSEC_PER_SEC + ts.tv_nsec;
+ y2 = (u32)ktime_to_ns(ktime_sub(last->host_time, first->host_time)) + y1;
/* Interpolated and host SOF timestamps can wrap around at slightly
* different times. Handle this by adding or removing 2048 to or from
@@ -686,24 +683,18 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
- (u64)y2 * (u64)x1;
y = div_u64(y, x2 - x1);
- div = div_u64_rem(y, NSEC_PER_SEC, &rem);
- ts.tv_sec = first->host_ts.tv_sec - 1 + div;
- ts.tv_nsec = first->host_ts.tv_nsec + rem;
- if (ts.tv_nsec >= NSEC_PER_SEC) {
- ts.tv_sec++;
- ts.tv_nsec -= NSEC_PER_SEC;
- }
+ timestamp = ktime_to_ns(first->host_time) + y - y1;
uvc_trace(UVC_TRACE_CLOCK, "%s: SOF %u.%06llu y %llu ts %llu "
"buf ts %llu (x1 %u/%u/%u x2 %u/%u/%u y1 %u y2 %u)\n",
stream->dev->name,
sof >> 16, div_u64(((u64)sof & 0xffff) * 1000000LLU, 65536),
- y, timespec_to_ns(&ts), vbuf->vb2_buf.timestamp,
+ y, timestamp, vbuf->vb2_buf.timestamp,
x1, first->host_sof, first->dev_sof,
x2, last->host_sof, last->dev_sof, y1, y2);
/* Update the V4L2 buffer. */
- vbuf->vb2_buf.timestamp = timespec_to_ns(&ts);
+ vbuf->vb2_buf.timestamp = timestamp;
done:
spin_unlock_irqrestore(&clock->lock, flags);
@@ -1007,8 +998,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
* when the EOF bit is set to force synchronisation on the next packet.
*/
if (buf->state != UVC_BUF_STATE_ACTIVE) {
- struct timespec ts;
-
if (fid == stream->last_fid) {
uvc_trace(UVC_TRACE_FRAME, "Dropping payload (out of "
"sync).\n");
@@ -1018,11 +1007,9 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
return -ENODATA;
}
- uvc_video_get_ts(&ts);
-
buf->buf.field = V4L2_FIELD_NONE;
buf->buf.sequence = stream->sequence;
- buf->buf.vb2_buf.timestamp = timespec_to_ns(&ts);
+ buf->buf.vb2_buf.timestamp = uvc_video_get_time();
/* TODO: Handle PTS and SCR. */
buf->state = UVC_BUF_STATE_ACTIVE;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index a2c190937067..d7797dfb6468 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -536,8 +536,8 @@ struct uvc_streaming {
struct uvc_clock_sample {
u32 dev_stc;
u16 dev_sof;
- struct timespec host_ts;
u16 host_sof;
+ ktime_t host_time;
} *samples;
unsigned int head;
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/8] [media] uvc_video: use ktime_t for timestamps
2017-11-27 13:19 ` [PATCH 2/8] [media] uvc_video: use ktime_t for timestamps Arnd Bergmann
@ 2017-12-05 0:37 ` Laurent Pinchart
2017-12-05 0:58 ` Laurent Pinchart
0 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2017-12-05 0:37 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mauro Carvalho Chehab, y2038, Kieran Bingham,
Guennadi Liakhovetski, Hans Verkuil, linux-media, linux-kernel
Hi Arnd,
Thank you for the patch.
On Monday, 27 November 2017 15:19:54 EET Arnd Bergmann wrote:
> uvc_video_get_ts() returns a 'struct timespec', but all its users
> really want a nanoseconds variable anyway.
>
> Changing the deprecated ktime_get_ts/ktime_get_real_ts to ktime_get
> and ktime_get_real simplifies the code noticeably, while keeping
> the resulting numbers unchanged.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/media/usb/uvc/uvc_video.c | 37 ++++++++++++-----------------------
> drivers/media/usb/uvc/uvcvideo.h | 2 +-
> 2 files changed, 13 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index d6bee37cd1b8..f7a919490b2b 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -369,12 +369,12 @@ static int uvc_commit_video(struct uvc_streaming
> *stream, * Clocks and timestamps
> */
>
> -static inline void uvc_video_get_ts(struct timespec *ts)
> +static inline ktime_t uvc_video_get_time(void)
> {
> if (uvc_clock_param == CLOCK_MONOTONIC)
> - ktime_get_ts(ts);
> + return ktime_get();
> else
> - ktime_get_real_ts(ts);
> + return ktime_get_real();
> }
>
> static void
> @@ -386,7 +386,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream,
> struct uvc_buffer *buf, bool has_pts = false;
> bool has_scr = false;
> unsigned long flags;
> - struct timespec ts;
> + ktime_t time;
> u16 host_sof;
> u16 dev_sof;
>
> @@ -436,7 +436,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream,
> struct uvc_buffer *buf, stream->clock.last_sof = dev_sof;
>
> host_sof = usb_get_current_frame_number(stream->dev->udev);
> - uvc_video_get_ts(&ts);
> + time = uvc_video_get_time();
>
> /* The UVC specification allows device implementations that can't obtain
> * the USB frame number to keep their own frame counters as long as they
> @@ -473,7 +473,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream,
> struct uvc_buffer *buf, sample->dev_stc =
> get_unaligned_le32(&data[header_size - 6]);
> sample->dev_sof = dev_sof;
> sample->host_sof = host_sof;
> - sample->host_ts = ts;
> + sample->host_time = time;
>
> /* Update the sliding window head and count. */
> stream->clock.head = (stream->clock.head + 1) % stream->clock.size;
> @@ -613,14 +613,12 @@ void uvc_video_clock_update(struct uvc_streaming
> *stream, struct uvc_clock_sample *first;
> struct uvc_clock_sample *last;
> unsigned long flags;
> - struct timespec ts;
> + u64 timestamp;
> u32 delta_stc;
> u32 y1, y2;
> u32 x1, x2;
> u32 mean;
> u32 sof;
> - u32 div;
> - u32 rem;
> u64 y;
>
> if (!uvc_hw_timestamps_param)
> @@ -667,9 +665,8 @@ void uvc_video_clock_update(struct uvc_streaming
> *stream, if (x1 == x2)
> goto done;
>
> - ts = timespec_sub(last->host_ts, first->host_ts);
> y1 = NSEC_PER_SEC;
> - y2 = (ts.tv_sec + 1) * NSEC_PER_SEC + ts.tv_nsec;
> + y2 = (u32)ktime_to_ns(ktime_sub(last->host_time, first->host_time)) + y1;
>
> /* Interpolated and host SOF timestamps can wrap around at slightly
> * different times. Handle this by adding or removing 2048 to or from
> @@ -686,24 +683,18 @@ void uvc_video_clock_update(struct uvc_streaming
> *stream, - (u64)y2 * (u64)x1;
> y = div_u64(y, x2 - x1);
>
> - div = div_u64_rem(y, NSEC_PER_SEC, &rem);
> - ts.tv_sec = first->host_ts.tv_sec - 1 + div;
> - ts.tv_nsec = first->host_ts.tv_nsec + rem;
> - if (ts.tv_nsec >= NSEC_PER_SEC) {
> - ts.tv_sec++;
> - ts.tv_nsec -= NSEC_PER_SEC;
> - }
> + timestamp = ktime_to_ns(first->host_time) + y - y1;
It took me a few minutes to see that the -1 and -y1 were equivalent. And then
more time to re-read the code and the comments to understand what I had done.
I'm impressed that you haven't blindly replaced the +1s and -1s by
+NSEC_PER_SEC and -NSEC_PER_SEC, but used +y1 and -y1 which I think improves
readability.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
with the highest honors :-)
Should I merge this through my tree ?
> uvc_trace(UVC_TRACE_CLOCK, "%s: SOF %u.%06llu y %llu ts %llu "
> "buf ts %llu (x1 %u/%u/%u x2 %u/%u/%u y1 %u y2 %u)\n",
> stream->dev->name,
> sof >> 16, div_u64(((u64)sof & 0xffff) * 1000000LLU, 65536),
> - y, timespec_to_ns(&ts), vbuf->vb2_buf.timestamp,
> + y, timestamp, vbuf->vb2_buf.timestamp,
> x1, first->host_sof, first->dev_sof,
> x2, last->host_sof, last->dev_sof, y1, y2);
>
> /* Update the V4L2 buffer. */
> - vbuf->vb2_buf.timestamp = timespec_to_ns(&ts);
> + vbuf->vb2_buf.timestamp = timestamp;
>
> done:
> spin_unlock_irqrestore(&clock->lock, flags);
> @@ -1007,8 +998,6 @@ static int uvc_video_decode_start(struct uvc_streaming
> *stream, * when the EOF bit is set to force synchronisation on the next
> packet. */
> if (buf->state != UVC_BUF_STATE_ACTIVE) {
> - struct timespec ts;
> -
> if (fid == stream->last_fid) {
> uvc_trace(UVC_TRACE_FRAME, "Dropping payload (out of "
> "sync).\n");
> @@ -1018,11 +1007,9 @@ static int uvc_video_decode_start(struct
> uvc_streaming *stream, return -ENODATA;
> }
>
> - uvc_video_get_ts(&ts);
> -
> buf->buf.field = V4L2_FIELD_NONE;
> buf->buf.sequence = stream->sequence;
> - buf->buf.vb2_buf.timestamp = timespec_to_ns(&ts);
> + buf->buf.vb2_buf.timestamp = uvc_video_get_time();
>
> /* TODO: Handle PTS and SCR. */
> buf->state = UVC_BUF_STATE_ACTIVE;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index a2c190937067..d7797dfb6468 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -536,8 +536,8 @@ struct uvc_streaming {
> struct uvc_clock_sample {
> u32 dev_stc;
> u16 dev_sof;
> - struct timespec host_ts;
> u16 host_sof;
> + ktime_t host_time;
> } *samples;
>
> unsigned int head;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/8] [media] uvc_video: use ktime_t for timestamps
2017-12-05 0:37 ` Laurent Pinchart
@ 2017-12-05 0:58 ` Laurent Pinchart
2017-12-05 11:27 ` Arnd Bergmann
0 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2017-12-05 0:58 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Arnd Bergmann, Mauro Carvalho Chehab, y2038, Kieran Bingham,
Guennadi Liakhovetski, Hans Verkuil, linux-media, linux-kernel
Hi Arnd,
On Tuesday, 5 December 2017 02:37:27 EET Laurent Pinchart wrote:
> On Monday, 27 November 2017 15:19:54 EET Arnd Bergmann wrote:
> > uvc_video_get_ts() returns a 'struct timespec', but all its users
> > really want a nanoseconds variable anyway.
> >
> > Changing the deprecated ktime_get_ts/ktime_get_real_ts to ktime_get
> > and ktime_get_real simplifies the code noticeably, while keeping
> > the resulting numbers unchanged.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >
> > drivers/media/usb/uvc/uvc_video.c | 37 ++++++++++++---------------------
> > drivers/media/usb/uvc/uvcvideo.h | 2 +-
> > 2 files changed, 13 insertions(+), 26 deletions(-)
[snip]
> > - struct timespec ts;
> > + u64 timestamp;
[snip]
> > uvc_trace(UVC_TRACE_CLOCK, "%s: SOF %u.%06llu y %llu ts %llu "
> > "buf ts %llu (x1 %u/%u/%u x2 %u/%u/%u y1 %u y2 %u)\n",
> > stream->dev->name,
> > sof >> 16, div_u64(((u64)sof & 0xffff) * 1000000LLU, 65536),
> > - y, timespec_to_ns(&ts), vbuf->vb2_buf.timestamp,
> > + y, timestamp, vbuf->vb2_buf.timestamp,
> > x1, first->host_sof, first->dev_sof,
> > x2, last->host_sof, last->dev_sof, y1, y2);
As you've done lots of work moving code away from timespec I figured out I
would ask, what is the preferred way to print a ktime in secs.nsecs format ?
Should I use ktime_to_timespec and print ts.tv_sec and ts.tv_nsec, or is there
a better way ?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/8] [media] uvc_video: use ktime_t for timestamps
2017-12-05 0:58 ` Laurent Pinchart
@ 2017-12-05 11:27 ` Arnd Bergmann
0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-12-05 11:27 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, y2038 Mailman List, Kieran Bingham,
Guennadi Liakhovetski, Hans Verkuil, Linux Media Mailing List,
Linux Kernel Mailing List
On Tue, Dec 5, 2017 at 1:58 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Arnd,
>
> On Tuesday, 5 December 2017 02:37:27 EET Laurent Pinchart wrote:
>> On Monday, 27 November 2017 15:19:54 EET Arnd Bergmann wrote:
>> > uvc_video_get_ts() returns a 'struct timespec', but all its users
>> > really want a nanoseconds variable anyway.
>> >
>> > Changing the deprecated ktime_get_ts/ktime_get_real_ts to ktime_get
>> > and ktime_get_real simplifies the code noticeably, while keeping
>> > the resulting numbers unchanged.
>> >
>> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> > ---
>> >
>> > drivers/media/usb/uvc/uvc_video.c | 37 ++++++++++++---------------------
>> > drivers/media/usb/uvc/uvcvideo.h | 2 +-
>> > 2 files changed, 13 insertions(+), 26 deletions(-)
>
> [snip]
>
>> > - struct timespec ts;
>> > + u64 timestamp;
>
> [snip]
>
>> > uvc_trace(UVC_TRACE_CLOCK, "%s: SOF %u.%06llu y %llu ts %llu "
>> > "buf ts %llu (x1 %u/%u/%u x2 %u/%u/%u y1 %u y2 %u)\n",
>> > stream->dev->name,
>> > sof >> 16, div_u64(((u64)sof & 0xffff) * 1000000LLU, 65536),
>> > - y, timespec_to_ns(&ts), vbuf->vb2_buf.timestamp,
>> > + y, timestamp, vbuf->vb2_buf.timestamp,
>> > x1, first->host_sof, first->dev_sof,
>> > x2, last->host_sof, last->dev_sof, y1, y2);
>
> As you've done lots of work moving code away from timespec I figured out I
> would ask, what is the preferred way to print a ktime in secs.nsecs format ?
> Should I use ktime_to_timespec and print ts.tv_sec and ts.tv_nsec, or is there
> a better way ?
We had patches under discussion to add a special printk format string that
would pretty-print a date, but that never got merged. Usually there is a
tradeoff between runtime to convert the nanoseconds into a different format
and how useful you want it to be. ktime_to_timespec() can be a bit slow on
some architectures, since it has to do a 64-bit division, but then again
the sprintf logic also needs to do that. If the output isn't on a time-critical
path, you can use time64_to_tm and print it in years/months/days/hours/
minutes/seconds, but depending on where it gets printed, that may not
be easier to interpret than the seconds/nanoseconds or pure
nanoseconds.
Arnd
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/8] [media] solo6x10: use ktime_get_ts64() for time sync
2017-11-27 13:19 [PATCH 1/8] [media] uvc_video: use ktime_t for stats Arnd Bergmann
2017-11-27 13:19 ` [PATCH 2/8] [media] uvc_video: use ktime_t for timestamps Arnd Bergmann
@ 2017-11-27 13:19 ` Arnd Bergmann
2017-11-27 20:45 ` Ismael Luceno
2017-11-27 13:19 ` [PATCH 5/8] [media] omap3isp: support 64-bit version of omap3isp_stat_data Arnd Bergmann
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-11-27 13:19 UTC (permalink / raw)
To: Bluecherry Maintainers, Anton Sviridenko, Andrey Utkin,
Ismael Luceno, Mauro Carvalho Chehab
Cc: y2038, Arnd Bergmann, linux-media, linux-kernel
solo6x10 correctly deals with time stamps and will never
suffer from overflows, but it uses the deprecated 'struct timespec'
type and 'ktime_get_ts()' interface to read the monotonic clock.
This changes it to use ktime_get_ts64() instead, so we can
eventually remove ktime_get_ts().
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/pci/solo6x10/solo6x10-core.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/media/pci/solo6x10/solo6x10-core.c b/drivers/media/pci/solo6x10/solo6x10-core.c
index ca0873e47bea..19ffd2ed3cc7 100644
--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -47,18 +47,19 @@ MODULE_PARM_DESC(full_eeprom, "Allow access to full 128B EEPROM (dangerous)");
static void solo_set_time(struct solo_dev *solo_dev)
{
- struct timespec ts;
+ struct timespec64 ts;
- ktime_get_ts(&ts);
+ ktime_get_ts64(&ts);
- solo_reg_write(solo_dev, SOLO_TIMER_SEC, ts.tv_sec);
- solo_reg_write(solo_dev, SOLO_TIMER_USEC, ts.tv_nsec / NSEC_PER_USEC);
+ /* no overflow because we use monotonic timestamps */
+ solo_reg_write(solo_dev, SOLO_TIMER_SEC, (u32)ts.tv_sec);
+ solo_reg_write(solo_dev, SOLO_TIMER_USEC, (u32)ts.tv_nsec / NSEC_PER_USEC);
}
static void solo_timer_sync(struct solo_dev *solo_dev)
{
u32 sec, usec;
- struct timespec ts;
+ struct timespec64 ts;
long diff;
if (solo_dev->type != SOLO_DEV_6110)
@@ -72,11 +73,11 @@ static void solo_timer_sync(struct solo_dev *solo_dev)
sec = solo_reg_read(solo_dev, SOLO_TIMER_SEC);
usec = solo_reg_read(solo_dev, SOLO_TIMER_USEC);
- ktime_get_ts(&ts);
+ ktime_get_ts64(&ts);
- diff = (long)ts.tv_sec - (long)sec;
+ diff = (s32)ts.tv_sec - (s32)sec;
diff = (diff * 1000000)
- + ((long)(ts.tv_nsec / NSEC_PER_USEC) - (long)usec);
+ + ((s32)(ts.tv_nsec / NSEC_PER_USEC) - (s32)usec);
if (diff > 1000 || diff < -1000) {
solo_set_time(solo_dev);
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/8] [media] solo6x10: use ktime_get_ts64() for time sync
2017-11-27 13:19 ` [PATCH 3/8] [media] solo6x10: use ktime_get_ts64() for time sync Arnd Bergmann
@ 2017-11-27 20:45 ` Ismael Luceno
0 siblings, 0 replies; 19+ messages in thread
From: Ismael Luceno @ 2017-11-27 20:45 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Bluecherry Maintainers, Anton Sviridenko, Andrey Utkin,
Mauro Carvalho Chehab, y2038, linux-media, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]
On 27/Nov/2017 14:19, Arnd Bergmann wrote:
> solo6x10 correctly deals with time stamps and will never
> suffer from overflows, but it uses the deprecated 'struct timespec'
> type and 'ktime_get_ts()' interface to read the monotonic clock.
>
> This changes it to use ktime_get_ts64() instead, so we can
> eventually remove ktime_get_ts().
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/media/pci/solo6x10/solo6x10-core.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/pci/solo6x10/solo6x10-core.c b/drivers/media/pci/solo6x10/solo6x10-core.c
> index ca0873e47bea..19ffd2ed3cc7 100644
> --- a/drivers/media/pci/solo6x10/solo6x10-core.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-core.c
> @@ -47,18 +47,19 @@ MODULE_PARM_DESC(full_eeprom, "Allow access to full 128B EEPROM (dangerous)");
>
> static void solo_set_time(struct solo_dev *solo_dev)
> {
> - struct timespec ts;
> + struct timespec64 ts;
>
> - ktime_get_ts(&ts);
> + ktime_get_ts64(&ts);
>
> - solo_reg_write(solo_dev, SOLO_TIMER_SEC, ts.tv_sec);
> - solo_reg_write(solo_dev, SOLO_TIMER_USEC, ts.tv_nsec / NSEC_PER_USEC);
> + /* no overflow because we use monotonic timestamps */
> + solo_reg_write(solo_dev, SOLO_TIMER_SEC, (u32)ts.tv_sec);
> + solo_reg_write(solo_dev, SOLO_TIMER_USEC, (u32)ts.tv_nsec / NSEC_PER_USEC);
> }
>
> static void solo_timer_sync(struct solo_dev *solo_dev)
> {
> u32 sec, usec;
> - struct timespec ts;
> + struct timespec64 ts;
> long diff;
>
> if (solo_dev->type != SOLO_DEV_6110)
> @@ -72,11 +73,11 @@ static void solo_timer_sync(struct solo_dev *solo_dev)
> sec = solo_reg_read(solo_dev, SOLO_TIMER_SEC);
> usec = solo_reg_read(solo_dev, SOLO_TIMER_USEC);
>
> - ktime_get_ts(&ts);
> + ktime_get_ts64(&ts);
>
> - diff = (long)ts.tv_sec - (long)sec;
> + diff = (s32)ts.tv_sec - (s32)sec;
> diff = (diff * 1000000)
> - + ((long)(ts.tv_nsec / NSEC_PER_USEC) - (long)usec);
> + + ((s32)(ts.tv_nsec / NSEC_PER_USEC) - (s32)usec);
>
> if (diff > 1000 || diff < -1000) {
> solo_set_time(solo_dev);
> --
> 2.9.0
>
Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/8] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2017-11-27 13:19 [PATCH 1/8] [media] uvc_video: use ktime_t for stats Arnd Bergmann
2017-11-27 13:19 ` [PATCH 2/8] [media] uvc_video: use ktime_t for timestamps Arnd Bergmann
2017-11-27 13:19 ` [PATCH 3/8] [media] solo6x10: use ktime_get_ts64() for time sync Arnd Bergmann
@ 2017-11-27 13:19 ` Arnd Bergmann
2017-12-05 0:41 ` Laurent Pinchart
2017-11-27 13:19 ` [PATCH 6/8] [media] vivid: use ktime_t for timestamp calculation Arnd Bergmann
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-11-27 13:19 UTC (permalink / raw)
To: Laurent Pinchart, Mauro Carvalho Chehab
Cc: y2038, Arnd Bergmann, linux-media, linux-kernel
C libraries with 64-bit time_t use an incompatible format for
struct omap3isp_stat_data. This changes the kernel code to
support either version, by moving over the normal handling
to the 64-bit variant, and adding compatiblity code to handle
the old binary format with the existing ioctl command code.
Fortunately, the command code includes the size of the structure,
so the difference gets handled automatically. In the process of
eliminating the references to 'struct timeval' from the kernel,
I also change the way the timestamp is generated internally,
basically by open-coding the v4l2_get_timestamp() call.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/platform/omap3isp/isph3a_aewb.c | 2 ++
drivers/media/platform/omap3isp/isph3a_af.c | 2 ++
drivers/media/platform/omap3isp/isphist.c | 2 ++
drivers/media/platform/omap3isp/ispstat.c | 21 +++++++++++++++++++--
drivers/media/platform/omap3isp/ispstat.h | 4 +++-
include/uapi/linux/omap3isp.h | 22 ++++++++++++++++++++++
6 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
index d44626f20ac6..3c82dea4d375 100644
--- a/drivers/media/platform/omap3isp/isph3a_aewb.c
+++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
@@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
return omap3isp_stat_config(stat, arg);
case VIDIOC_OMAP3ISP_STAT_REQ:
return omap3isp_stat_request_statistics(stat, arg);
+ case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
+ return omap3isp_stat_request_statistics_time32(stat, arg);
case VIDIOC_OMAP3ISP_STAT_EN: {
unsigned long *en = arg;
return omap3isp_stat_enable(stat, !!*en);
diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
index 99bd6cc21d86..4da25c84f0c6 100644
--- a/drivers/media/platform/omap3isp/isph3a_af.c
+++ b/drivers/media/platform/omap3isp/isph3a_af.c
@@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
return omap3isp_stat_config(stat, arg);
case VIDIOC_OMAP3ISP_STAT_REQ:
return omap3isp_stat_request_statistics(stat, arg);
+ case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
+ return omap3isp_stat_request_statistics_time32(stat, arg);
case VIDIOC_OMAP3ISP_STAT_EN: {
int *en = arg;
return omap3isp_stat_enable(stat, !!*en);
diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c
index a4ed5d140d48..d4be3d0e06f9 100644
--- a/drivers/media/platform/omap3isp/isphist.c
+++ b/drivers/media/platform/omap3isp/isphist.c
@@ -435,6 +435,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
return omap3isp_stat_config(stat, arg);
case VIDIOC_OMAP3ISP_STAT_REQ:
return omap3isp_stat_request_statistics(stat, arg);
+ case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
+ return omap3isp_stat_request_statistics_time32(stat, arg);
case VIDIOC_OMAP3ISP_STAT_EN: {
int *en = arg;
return omap3isp_stat_enable(stat, !!*en);
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 47cbc7e3d825..5967dfd0a9f7 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -18,6 +18,7 @@
#include <linux/dma-mapping.h>
#include <linux/slab.h>
#include <linux/uaccess.h>
+#include <linux/timekeeping.h>
#include "isp.h"
@@ -237,7 +238,7 @@ static int isp_stat_buf_queue(struct ispstat *stat)
if (!stat->active_buf)
return STAT_NO_BUF;
- v4l2_get_timestamp(&stat->active_buf->ts);
+ ktime_get_ts64(&stat->active_buf->ts);
stat->active_buf->buf_size = stat->buf_size;
if (isp_stat_buf_check_magic(stat, stat->active_buf)) {
@@ -500,7 +501,8 @@ int omap3isp_stat_request_statistics(struct ispstat *stat,
return PTR_ERR(buf);
}
- data->ts = buf->ts;
+ data->ts.tv_sec = buf->ts.tv_sec;
+ data->ts.tv_usec = buf->ts.tv_nsec / NSEC_PER_USEC;
data->config_counter = buf->config_counter;
data->frame_number = buf->frame_number;
data->buf_size = buf->buf_size;
@@ -512,6 +514,21 @@ int omap3isp_stat_request_statistics(struct ispstat *stat,
return 0;
}
+int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
+ struct omap3isp_stat_data_time32 *data)
+{
+ struct omap3isp_stat_data data64;
+ int ret;
+
+ ret = omap3isp_stat_request_statistics(stat, &data64);
+
+ data->ts.tv_sec = data64.ts.tv_sec;
+ data->ts.tv_usec = data64.ts.tv_usec;
+ memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
+
+ return ret;
+}
+
/*
* omap3isp_stat_config - Receives new statistic engine configuration.
* @new_conf: Pointer to config structure.
diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h
index 6d9b0244f320..923b38cfc682 100644
--- a/drivers/media/platform/omap3isp/ispstat.h
+++ b/drivers/media/platform/omap3isp/ispstat.h
@@ -39,7 +39,7 @@ struct ispstat_buffer {
struct sg_table sgt;
void *virt_addr;
dma_addr_t dma_addr;
- struct timeval ts;
+ struct timespec64 ts;
u32 buf_size;
u32 frame_number;
u16 config_counter;
@@ -130,6 +130,8 @@ struct ispstat_generic_config {
int omap3isp_stat_config(struct ispstat *stat, void *new_conf);
int omap3isp_stat_request_statistics(struct ispstat *stat,
struct omap3isp_stat_data *data);
+int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
+ struct omap3isp_stat_data_time32 *data);
int omap3isp_stat_init(struct ispstat *stat, const char *name,
const struct v4l2_subdev_ops *sd_ops);
void omap3isp_stat_cleanup(struct ispstat *stat);
diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h
index 1a920145db04..87b55755f4ff 100644
--- a/include/uapi/linux/omap3isp.h
+++ b/include/uapi/linux/omap3isp.h
@@ -55,6 +55,8 @@
_IOWR('V', BASE_VIDIOC_PRIVATE + 5, struct omap3isp_h3a_af_config)
#define VIDIOC_OMAP3ISP_STAT_REQ \
_IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data)
+#define VIDIOC_OMAP3ISP_STAT_REQ_TIME32 \
+ _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data_time32)
#define VIDIOC_OMAP3ISP_STAT_EN \
_IOWR('V', BASE_VIDIOC_PRIVATE + 7, unsigned long)
@@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
* @config_counter: Number of the configuration associated with the data.
*/
struct omap3isp_stat_data {
+#ifdef __KERNEL__
+ struct {
+ __s64 tv_sec;
+ __s64 tv_usec;
+ } ts;
+#else
struct timeval ts;
+#endif
void __user *buf;
__u32 buf_size;
__u16 frame_number;
@@ -173,6 +182,19 @@ struct omap3isp_stat_data {
__u16 config_counter;
};
+#ifdef __KERNEL__
+struct omap3isp_stat_data_time32 {
+ struct {
+ __s32 tv_sec;
+ __s32 tv_usec;
+ } ts;
+ __u32 buf;
+ __u32 buf_size;
+ __u16 frame_number;
+ __u16 cur_frame;
+ __u16 config_counter;
+};
+#endif
/* Histogram related structs */
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/8] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2017-11-27 13:19 ` [PATCH 5/8] [media] omap3isp: support 64-bit version of omap3isp_stat_data Arnd Bergmann
@ 2017-12-05 0:41 ` Laurent Pinchart
2018-01-16 17:10 ` Arnd Bergmann
0 siblings, 1 reply; 19+ messages in thread
From: Laurent Pinchart @ 2017-12-05 0:41 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mauro Carvalho Chehab, y2038, linux-media, linux-kernel, Sakari Ailus
Hi Arnd,
Thank you for the patch.
I'll try to review this without too much delay. In the meantime, I'm CC'ing
Sakari Ailus who might be faster than me :-)
On Monday, 27 November 2017 15:19:57 EET Arnd Bergmann wrote:
> C libraries with 64-bit time_t use an incompatible format for
> struct omap3isp_stat_data. This changes the kernel code to
> support either version, by moving over the normal handling
> to the 64-bit variant, and adding compatiblity code to handle
> the old binary format with the existing ioctl command code.
>
> Fortunately, the command code includes the size of the structure,
> so the difference gets handled automatically. In the process of
> eliminating the references to 'struct timeval' from the kernel,
> I also change the way the timestamp is generated internally,
> basically by open-coding the v4l2_get_timestamp() call.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/media/platform/omap3isp/isph3a_aewb.c | 2 ++
> drivers/media/platform/omap3isp/isph3a_af.c | 2 ++
> drivers/media/platform/omap3isp/isphist.c | 2 ++
> drivers/media/platform/omap3isp/ispstat.c | 21 +++++++++++++++++++--
> drivers/media/platform/omap3isp/ispstat.h | 4 +++-
> include/uapi/linux/omap3isp.h | 22 ++++++++++++++++++++++
> 6 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c
> b/drivers/media/platform/omap3isp/isph3a_aewb.c index
> d44626f20ac6..3c82dea4d375 100644
> --- a/drivers/media/platform/omap3isp/isph3a_aewb.c
> +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
> @@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd,
> unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> unsigned long *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isph3a_af.c
> b/drivers/media/platform/omap3isp/isph3a_af.c index
> 99bd6cc21d86..4da25c84f0c6 100644
> --- a/drivers/media/platform/omap3isp/isph3a_af.c
> +++ b/drivers/media/platform/omap3isp/isph3a_af.c
> @@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd,
> unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> int *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/isphist.c
> b/drivers/media/platform/omap3isp/isphist.c index
> a4ed5d140d48..d4be3d0e06f9 100644
> --- a/drivers/media/platform/omap3isp/isphist.c
> +++ b/drivers/media/platform/omap3isp/isphist.c
> @@ -435,6 +435,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned
> int cmd, void *arg) return omap3isp_stat_config(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_REQ:
> return omap3isp_stat_request_statistics(stat, arg);
> + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
> + return omap3isp_stat_request_statistics_time32(stat, arg);
> case VIDIOC_OMAP3ISP_STAT_EN: {
> int *en = arg;
> return omap3isp_stat_enable(stat, !!*en);
> diff --git a/drivers/media/platform/omap3isp/ispstat.c
> b/drivers/media/platform/omap3isp/ispstat.c index
> 47cbc7e3d825..5967dfd0a9f7 100644
> --- a/drivers/media/platform/omap3isp/ispstat.c
> +++ b/drivers/media/platform/omap3isp/ispstat.c
> @@ -18,6 +18,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <linux/timekeeping.h>
>
> #include "isp.h"
>
> @@ -237,7 +238,7 @@ static int isp_stat_buf_queue(struct ispstat *stat)
> if (!stat->active_buf)
> return STAT_NO_BUF;
>
> - v4l2_get_timestamp(&stat->active_buf->ts);
> + ktime_get_ts64(&stat->active_buf->ts);
>
> stat->active_buf->buf_size = stat->buf_size;
> if (isp_stat_buf_check_magic(stat, stat->active_buf)) {
> @@ -500,7 +501,8 @@ int omap3isp_stat_request_statistics(struct ispstat
> *stat, return PTR_ERR(buf);
> }
>
> - data->ts = buf->ts;
> + data->ts.tv_sec = buf->ts.tv_sec;
> + data->ts.tv_usec = buf->ts.tv_nsec / NSEC_PER_USEC;
> data->config_counter = buf->config_counter;
> data->frame_number = buf->frame_number;
> data->buf_size = buf->buf_size;
> @@ -512,6 +514,21 @@ int omap3isp_stat_request_statistics(struct ispstat
> *stat, return 0;
> }
>
> +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> + struct omap3isp_stat_data_time32 *data)
> +{
> + struct omap3isp_stat_data data64;
> + int ret;
> +
> + ret = omap3isp_stat_request_statistics(stat, &data64);
> +
> + data->ts.tv_sec = data64.ts.tv_sec;
> + data->ts.tv_usec = data64.ts.tv_usec;
> + memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
> +
> + return ret;
> +}
> +
> /*
> * omap3isp_stat_config - Receives new statistic engine configuration.
> * @new_conf: Pointer to config structure.
> diff --git a/drivers/media/platform/omap3isp/ispstat.h
> b/drivers/media/platform/omap3isp/ispstat.h index
> 6d9b0244f320..923b38cfc682 100644
> --- a/drivers/media/platform/omap3isp/ispstat.h
> +++ b/drivers/media/platform/omap3isp/ispstat.h
> @@ -39,7 +39,7 @@ struct ispstat_buffer {
> struct sg_table sgt;
> void *virt_addr;
> dma_addr_t dma_addr;
> - struct timeval ts;
> + struct timespec64 ts;
> u32 buf_size;
> u32 frame_number;
> u16 config_counter;
> @@ -130,6 +130,8 @@ struct ispstat_generic_config {
> int omap3isp_stat_config(struct ispstat *stat, void *new_conf);
> int omap3isp_stat_request_statistics(struct ispstat *stat,
> struct omap3isp_stat_data *data);
> +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> + struct omap3isp_stat_data_time32 *data);
> int omap3isp_stat_init(struct ispstat *stat, const char *name,
> const struct v4l2_subdev_ops *sd_ops);
> void omap3isp_stat_cleanup(struct ispstat *stat);
> diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h
> index 1a920145db04..87b55755f4ff 100644
> --- a/include/uapi/linux/omap3isp.h
> +++ b/include/uapi/linux/omap3isp.h
> @@ -55,6 +55,8 @@
> _IOWR('V', BASE_VIDIOC_PRIVATE + 5, struct omap3isp_h3a_af_config)
> #define VIDIOC_OMAP3ISP_STAT_REQ \
> _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data)
> +#define VIDIOC_OMAP3ISP_STAT_REQ_TIME32 \
> + _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data_time32)
> #define VIDIOC_OMAP3ISP_STAT_EN \
> _IOWR('V', BASE_VIDIOC_PRIVATE + 7, unsigned long)
>
> @@ -165,7 +167,14 @@ struct omap3isp_h3a_aewb_config {
> * @config_counter: Number of the configuration associated with the data.
> */
> struct omap3isp_stat_data {
> +#ifdef __KERNEL__
> + struct {
> + __s64 tv_sec;
> + __s64 tv_usec;
> + } ts;
> +#else
> struct timeval ts;
> +#endif
> void __user *buf;
> __u32 buf_size;
> __u16 frame_number;
> @@ -173,6 +182,19 @@ struct omap3isp_stat_data {
> __u16 config_counter;
> };
>
> +#ifdef __KERNEL__
> +struct omap3isp_stat_data_time32 {
> + struct {
> + __s32 tv_sec;
> + __s32 tv_usec;
> + } ts;
> + __u32 buf;
> + __u32 buf_size;
> + __u16 frame_number;
> + __u16 cur_frame;
> + __u16 config_counter;
> +};
> +#endif
>
> /* Histogram related structs */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 5/8] [media] omap3isp: support 64-bit version of omap3isp_stat_data
2017-12-05 0:41 ` Laurent Pinchart
@ 2018-01-16 17:10 ` Arnd Bergmann
0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2018-01-16 17:10 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, y2038 Mailman List,
Linux Media Mailing List, Linux Kernel Mailing List,
Sakari Ailus
On Tue, Dec 5, 2017 at 1:41 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Arnd,
>
> Thank you for the patch.
>
> I'll try to review this without too much delay. In the meantime, I'm CC'ing
> Sakari Ailus who might be faster than me :-)
Hi Laurent and Sakari,
I stumbled over this while cleaning out my backlog of patches. Any chance one
of you can have a look at this one? It's not needed for the 4.16 merge window,
but we do need it at some point and I would like to not see it again the next
time I clean out my old patches ;-)
Arnd
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/8] [media] vivid: use ktime_t for timestamp calculation
2017-11-27 13:19 [PATCH 1/8] [media] uvc_video: use ktime_t for stats Arnd Bergmann
` (2 preceding siblings ...)
2017-11-27 13:19 ` [PATCH 5/8] [media] omap3isp: support 64-bit version of omap3isp_stat_data Arnd Bergmann
@ 2017-11-27 13:19 ` Arnd Bergmann
2017-11-27 15:14 ` Hans Verkuil
2017-11-27 13:19 ` [PATCH 7/8] [media] staging: atomisp: convert timestamps to ktime_t Arnd Bergmann
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-11-27 13:19 UTC (permalink / raw)
To: Hans Verkuil, Mauro Carvalho Chehab
Cc: y2038, Arnd Bergmann, Javier Martinez Canillas, Vincent ABRIOU,
Ingo Molnar, linux-media, linux-kernel
timespec is generally deprecated because of the y2038 overflow.
In vivid, the usage is fine, since we are dealing with monotonic
timestamps, but we can also simplify the code by going to ktime_t.
Using ktime_divns() should be roughly as efficient as the old code,
since the constant 64-bit division gets turned into a multiplication
on modern platforms, and we save multiple 32-bit divisions that can be
expensive e.g. on ARMv7.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I hope I understood the VIVID_RDS_GEN_BLOCKS calculation right,
please review carefully.
---
drivers/media/platform/vivid/vivid-core.c | 2 +-
drivers/media/platform/vivid/vivid-core.h | 2 +-
drivers/media/platform/vivid/vivid-radio-rx.c | 11 +++++------
drivers/media/platform/vivid/vivid-radio-tx.c | 8 +++-----
drivers/media/platform/vivid/vivid-rds-gen.h | 1 +
5 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 5f316a5e38db..a091cfd93164 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -995,7 +995,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
dev->edid_max_blocks = dev->edid_blocks = 2;
memcpy(dev->edid, vivid_hdmi_edid, sizeof(vivid_hdmi_edid));
- ktime_get_ts(&dev->radio_rds_init_ts);
+ dev->radio_rds_init_time = ktime_get();
/* create all controls */
ret = vivid_create_controls(dev, ccs_cap == -1, ccs_out == -1, no_error_inj,
diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
index 5cdf95bdc4d1..d8bff4dcefb7 100644
--- a/drivers/media/platform/vivid/vivid-core.h
+++ b/drivers/media/platform/vivid/vivid-core.h
@@ -510,7 +510,7 @@ struct vivid_dev {
/* Shared between radio receiver and transmitter */
bool radio_rds_loop;
- struct timespec radio_rds_init_ts;
+ ktime_t radio_rds_init_time;
/* CEC */
struct cec_adapter *cec_rx_adap;
diff --git a/drivers/media/platform/vivid/vivid-radio-rx.c b/drivers/media/platform/vivid/vivid-radio-rx.c
index 47c36c26096b..1b96cbd7f2ea 100644
--- a/drivers/media/platform/vivid/vivid-radio-rx.c
+++ b/drivers/media/platform/vivid/vivid-radio-rx.c
@@ -38,9 +38,9 @@ ssize_t vivid_radio_rx_read(struct file *file, char __user *buf,
size_t size, loff_t *offset)
{
struct vivid_dev *dev = video_drvdata(file);
- struct timespec ts;
struct v4l2_rds_data *data = dev->rds_gen.data;
bool use_alternates;
+ ktime_t timestamp;
unsigned blk;
int perc;
int i;
@@ -64,17 +64,16 @@ ssize_t vivid_radio_rx_read(struct file *file, char __user *buf,
}
retry:
- ktime_get_ts(&ts);
- use_alternates = ts.tv_sec % 10 >= 5;
+ timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
+ blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
+ use_alternates = blk % VIVID_RDS_GEN_BLOCKS;
+
if (dev->radio_rx_rds_last_block == 0 ||
dev->radio_rx_rds_use_alternates != use_alternates) {
dev->radio_rx_rds_use_alternates = use_alternates;
/* Re-init the RDS generator */
vivid_radio_rds_init(dev);
}
- ts = timespec_sub(ts, dev->radio_rds_init_ts);
- blk = ts.tv_sec * 100 + ts.tv_nsec / 10000000;
- blk = (blk * VIVID_RDS_GEN_BLOCKS) / 500;
if (blk >= dev->radio_rx_rds_last_block + VIVID_RDS_GEN_BLOCKS)
dev->radio_rx_rds_last_block = blk - VIVID_RDS_GEN_BLOCKS + 1;
diff --git a/drivers/media/platform/vivid/vivid-radio-tx.c b/drivers/media/platform/vivid/vivid-radio-tx.c
index 0e8025b7b4dd..897b56195ca7 100644
--- a/drivers/media/platform/vivid/vivid-radio-tx.c
+++ b/drivers/media/platform/vivid/vivid-radio-tx.c
@@ -37,7 +37,7 @@ ssize_t vivid_radio_tx_write(struct file *file, const char __user *buf,
{
struct vivid_dev *dev = video_drvdata(file);
struct v4l2_rds_data *data = dev->rds_gen.data;
- struct timespec ts;
+ ktime_t timestamp;
unsigned blk;
int i;
@@ -58,10 +58,8 @@ ssize_t vivid_radio_tx_write(struct file *file, const char __user *buf,
dev->radio_tx_rds_owner = file->private_data;
retry:
- ktime_get_ts(&ts);
- ts = timespec_sub(ts, dev->radio_rds_init_ts);
- blk = ts.tv_sec * 100 + ts.tv_nsec / 10000000;
- blk = (blk * VIVID_RDS_GEN_BLOCKS) / 500;
+ timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
+ blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
if (blk - VIVID_RDS_GEN_BLOCKS >= dev->radio_tx_rds_last_block)
dev->radio_tx_rds_last_block = blk - VIVID_RDS_GEN_BLOCKS + 1;
diff --git a/drivers/media/platform/vivid/vivid-rds-gen.h b/drivers/media/platform/vivid/vivid-rds-gen.h
index eff4bf552ed3..e55e3b22b7ca 100644
--- a/drivers/media/platform/vivid/vivid-rds-gen.h
+++ b/drivers/media/platform/vivid/vivid-rds-gen.h
@@ -29,6 +29,7 @@
#define VIVID_RDS_GEN_GROUPS 57
#define VIVID_RDS_GEN_BLKS_PER_GRP 4
#define VIVID_RDS_GEN_BLOCKS (VIVID_RDS_GEN_BLKS_PER_GRP * VIVID_RDS_GEN_GROUPS)
+#define VIVID_RDS_NSEC_PER_BLK (u32)(5ull * NSEC_PER_SEC / VIVID_RDS_GEN_BLOCKS)
struct vivid_rds_gen {
struct v4l2_rds_data data[VIVID_RDS_GEN_BLOCKS];
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 6/8] [media] vivid: use ktime_t for timestamp calculation
2017-11-27 13:19 ` [PATCH 6/8] [media] vivid: use ktime_t for timestamp calculation Arnd Bergmann
@ 2017-11-27 15:14 ` Hans Verkuil
2017-11-27 15:25 ` Arnd Bergmann
0 siblings, 1 reply; 19+ messages in thread
From: Hans Verkuil @ 2017-11-27 15:14 UTC (permalink / raw)
To: Arnd Bergmann, Mauro Carvalho Chehab
Cc: y2038, Javier Martinez Canillas, Vincent ABRIOU, Ingo Molnar,
linux-media, linux-kernel
Hi Arnd,
On 11/27/2017 02:19 PM, Arnd Bergmann wrote:
> timespec is generally deprecated because of the y2038 overflow.
> In vivid, the usage is fine, since we are dealing with monotonic
> timestamps, but we can also simplify the code by going to ktime_t.
>
> Using ktime_divns() should be roughly as efficient as the old code,
> since the constant 64-bit division gets turned into a multiplication
> on modern platforms, and we save multiple 32-bit divisions that can be
> expensive e.g. on ARMv7.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I hope I understood the VIVID_RDS_GEN_BLOCKS calculation right,
> please review carefully.
> ---
> drivers/media/platform/vivid/vivid-core.c | 2 +-
> drivers/media/platform/vivid/vivid-core.h | 2 +-
> drivers/media/platform/vivid/vivid-radio-rx.c | 11 +++++------
> drivers/media/platform/vivid/vivid-radio-tx.c | 8 +++-----
> drivers/media/platform/vivid/vivid-rds-gen.h | 1 +
> 5 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
> index 5f316a5e38db..a091cfd93164 100644
> --- a/drivers/media/platform/vivid/vivid-core.c
> +++ b/drivers/media/platform/vivid/vivid-core.c
> @@ -995,7 +995,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>
> dev->edid_max_blocks = dev->edid_blocks = 2;
> memcpy(dev->edid, vivid_hdmi_edid, sizeof(vivid_hdmi_edid));
> - ktime_get_ts(&dev->radio_rds_init_ts);
> + dev->radio_rds_init_time = ktime_get();
>
> /* create all controls */
> ret = vivid_create_controls(dev, ccs_cap == -1, ccs_out == -1, no_error_inj,
> diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
> index 5cdf95bdc4d1..d8bff4dcefb7 100644
> --- a/drivers/media/platform/vivid/vivid-core.h
> +++ b/drivers/media/platform/vivid/vivid-core.h
> @@ -510,7 +510,7 @@ struct vivid_dev {
>
> /* Shared between radio receiver and transmitter */
> bool radio_rds_loop;
> - struct timespec radio_rds_init_ts;
> + ktime_t radio_rds_init_time;
>
> /* CEC */
> struct cec_adapter *cec_rx_adap;
> diff --git a/drivers/media/platform/vivid/vivid-radio-rx.c b/drivers/media/platform/vivid/vivid-radio-rx.c
> index 47c36c26096b..1b96cbd7f2ea 100644
> --- a/drivers/media/platform/vivid/vivid-radio-rx.c
> +++ b/drivers/media/platform/vivid/vivid-radio-rx.c
> @@ -38,9 +38,9 @@ ssize_t vivid_radio_rx_read(struct file *file, char __user *buf,
> size_t size, loff_t *offset)
> {
> struct vivid_dev *dev = video_drvdata(file);
> - struct timespec ts;
> struct v4l2_rds_data *data = dev->rds_gen.data;
> bool use_alternates;
> + ktime_t timestamp;
> unsigned blk;
> int perc;
> int i;
> @@ -64,17 +64,16 @@ ssize_t vivid_radio_rx_read(struct file *file, char __user *buf,
> }
>
> retry:
> - ktime_get_ts(&ts);
> - use_alternates = ts.tv_sec % 10 >= 5;
> + timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
> + blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
> + use_alternates = blk % VIVID_RDS_GEN_BLOCKS;
> +
Almost right. This last line should be:
use_alternates = (blk / VIVID_RDS_GEN_BLOCKS) & 1;
With that in place it works and you can add my:
Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
to this patch.
Regards,
Hans
> if (dev->radio_rx_rds_last_block == 0 ||
> dev->radio_rx_rds_use_alternates != use_alternates) {
> dev->radio_rx_rds_use_alternates = use_alternates;
> /* Re-init the RDS generator */
> vivid_radio_rds_init(dev);
> }
> - ts = timespec_sub(ts, dev->radio_rds_init_ts);
> - blk = ts.tv_sec * 100 + ts.tv_nsec / 10000000;
> - blk = (blk * VIVID_RDS_GEN_BLOCKS) / 500;
> if (blk >= dev->radio_rx_rds_last_block + VIVID_RDS_GEN_BLOCKS)
> dev->radio_rx_rds_last_block = blk - VIVID_RDS_GEN_BLOCKS + 1;
>
> diff --git a/drivers/media/platform/vivid/vivid-radio-tx.c b/drivers/media/platform/vivid/vivid-radio-tx.c
> index 0e8025b7b4dd..897b56195ca7 100644
> --- a/drivers/media/platform/vivid/vivid-radio-tx.c
> +++ b/drivers/media/platform/vivid/vivid-radio-tx.c
> @@ -37,7 +37,7 @@ ssize_t vivid_radio_tx_write(struct file *file, const char __user *buf,
> {
> struct vivid_dev *dev = video_drvdata(file);
> struct v4l2_rds_data *data = dev->rds_gen.data;
> - struct timespec ts;
> + ktime_t timestamp;
> unsigned blk;
> int i;
>
> @@ -58,10 +58,8 @@ ssize_t vivid_radio_tx_write(struct file *file, const char __user *buf,
> dev->radio_tx_rds_owner = file->private_data;
>
> retry:
> - ktime_get_ts(&ts);
> - ts = timespec_sub(ts, dev->radio_rds_init_ts);
> - blk = ts.tv_sec * 100 + ts.tv_nsec / 10000000;
> - blk = (blk * VIVID_RDS_GEN_BLOCKS) / 500;
> + timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
> + blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
> if (blk - VIVID_RDS_GEN_BLOCKS >= dev->radio_tx_rds_last_block)
> dev->radio_tx_rds_last_block = blk - VIVID_RDS_GEN_BLOCKS + 1;
>
> diff --git a/drivers/media/platform/vivid/vivid-rds-gen.h b/drivers/media/platform/vivid/vivid-rds-gen.h
> index eff4bf552ed3..e55e3b22b7ca 100644
> --- a/drivers/media/platform/vivid/vivid-rds-gen.h
> +++ b/drivers/media/platform/vivid/vivid-rds-gen.h
> @@ -29,6 +29,7 @@
> #define VIVID_RDS_GEN_GROUPS 57
> #define VIVID_RDS_GEN_BLKS_PER_GRP 4
> #define VIVID_RDS_GEN_BLOCKS (VIVID_RDS_GEN_BLKS_PER_GRP * VIVID_RDS_GEN_GROUPS)
> +#define VIVID_RDS_NSEC_PER_BLK (u32)(5ull * NSEC_PER_SEC / VIVID_RDS_GEN_BLOCKS)
>
> struct vivid_rds_gen {
> struct v4l2_rds_data data[VIVID_RDS_GEN_BLOCKS];
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/8] [media] vivid: use ktime_t for timestamp calculation
2017-11-27 15:14 ` Hans Verkuil
@ 2017-11-27 15:25 ` Arnd Bergmann
0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-11-27 15:25 UTC (permalink / raw)
To: Hans Verkuil
Cc: Mauro Carvalho Chehab, y2038 Mailman List,
Javier Martinez Canillas, Vincent ABRIOU, Ingo Molnar,
Linux Media Mailing List, Linux Kernel Mailing List
On Mon, Nov 27, 2017 at 4:14 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> - ktime_get_ts(&ts);
>> - use_alternates = ts.tv_sec % 10 >= 5;
>> + timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
>> + blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
>> + use_alternates = blk % VIVID_RDS_GEN_BLOCKS;
>> +
>
> Almost right. This last line should be:
>
> use_alternates = (blk / VIVID_RDS_GEN_BLOCKS) & 1;
>
> With that in place it works and you can add my:
>
> Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
Makes sense. Sending a fixed version now, thanks a lot for testing!
Arnd
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 7/8] [media] staging: atomisp: convert timestamps to ktime_t
2017-11-27 13:19 [PATCH 1/8] [media] uvc_video: use ktime_t for stats Arnd Bergmann
` (3 preceding siblings ...)
2017-11-27 13:19 ` [PATCH 6/8] [media] vivid: use ktime_t for timestamp calculation Arnd Bergmann
@ 2017-11-27 13:19 ` Arnd Bergmann
2017-11-27 15:05 ` Andy Shevchenko
2017-11-27 13:20 ` [PATCH 8/8] [media] staging: imx: use ktime_t for timestamps Arnd Bergmann
2017-12-05 0:40 ` [PATCH 1/8] [media] uvc_video: use ktime_t for stats Laurent Pinchart
6 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-11-27 13:19 UTC (permalink / raw)
To: Alan Cox, Sakari Ailus, Mauro Carvalho Chehab, Greg Kroah-Hartman
Cc: y2038, Arnd Bergmann, Andy Shevchenko, linux-media, devel, linux-kernel
timespec overflows in 2038 on 32-bit architectures, and the
getnstimeofday() suffers from possible time jumps, so the
timestamps here are better done using ktime_get(), which has
neither of those problems.
In case of ov2680, we don't seem to use the timestamp at
all, so I just remove it.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/staging/media/atomisp/i2c/ov2680.h | 1 -
drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 15 ++++++---------
drivers/staging/media/atomisp/i2c/ov5693/ov5693.h | 2 +-
3 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index bf4897347df7..03f75dd80f87 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -174,7 +174,6 @@ struct ov2680_format {
struct mutex input_lock;
struct v4l2_ctrl_handler ctrl_handler;
struct camera_sensor_platform_data *platform_data;
- struct timespec timestamp_t_focus_abs;
int vt_pix_clk_freq_mhz;
int fmt_idx;
int run_mode;
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 3e7c3851280f..a715ea0e4230 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -973,7 +973,7 @@ static int ov5693_t_focus_abs(struct v4l2_subdev *sd, s32 value)
if (ret == 0) {
dev->number_of_steps = value - dev->focus;
dev->focus = value;
- getnstimeofday(&(dev->timestamp_t_focus_abs));
+ dev->timestamp_t_focus_abs = ktime_get();
} else
dev_err(&client->dev,
"%s: i2c failed. ret %d\n", __func__, ret);
@@ -993,16 +993,13 @@ static int ov5693_q_focus_status(struct v4l2_subdev *sd, s32 *value)
{
u32 status = 0;
struct ov5693_device *dev = to_ov5693_sensor(sd);
- struct timespec temptime;
- const struct timespec timedelay = {
- 0,
+ ktime_t temptime;
+ ktime_t timedelay = ns_to_ktime(
min((u32)abs(dev->number_of_steps) * DELAY_PER_STEP_NS,
- (u32)DELAY_MAX_PER_STEP_NS),
- };
+ (u32)DELAY_MAX_PER_STEP_NS));
- getnstimeofday(&temptime);
- temptime = timespec_sub(temptime, (dev->timestamp_t_focus_abs));
- if (timespec_compare(&temptime, &timedelay) <= 0) {
+ temptime = ktime_sub(ktime_get(), (dev->timestamp_t_focus_abs));
+ if (ktime_compare(temptime, timedelay) <= 0) {
status |= ATOMISP_FOCUS_STATUS_MOVING;
status |= ATOMISP_FOCUS_HP_IN_PROGRESS;
} else {
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
index 2ea63807c56d..68cfcb4a6c3c 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
+++ b/drivers/staging/media/atomisp/i2c/ov5693/ov5693.h
@@ -221,7 +221,7 @@ struct ov5693_device {
struct v4l2_ctrl_handler ctrl_handler;
struct camera_sensor_platform_data *platform_data;
- struct timespec timestamp_t_focus_abs;
+ ktime_t timestamp_t_focus_abs;
int vt_pix_clk_freq_mhz;
int fmt_idx;
int run_mode;
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 7/8] [media] staging: atomisp: convert timestamps to ktime_t
2017-11-27 13:19 ` [PATCH 7/8] [media] staging: atomisp: convert timestamps to ktime_t Arnd Bergmann
@ 2017-11-27 15:05 ` Andy Shevchenko
2017-11-27 15:20 ` [Y2038] " Arnd Bergmann
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2017-11-27 15:05 UTC (permalink / raw)
To: Arnd Bergmann, Alan Cox, Sakari Ailus, Mauro Carvalho Chehab,
Greg Kroah-Hartman
Cc: y2038, linux-media, devel, linux-kernel
On Mon, 2017-11-27 at 14:19 +0100, Arnd Bergmann wrote:
> timespec overflows in 2038 on 32-bit architectures, and the
> getnstimeofday() suffers from possible time jumps, so the
> timestamps here are better done using ktime_get(), which has
> neither of those problems.
>
> In case of ov2680, we don't seem to use the timestamp at
> all, so I just remove it.
>
> + ktime_t timedelay = ns_to_ktime(
> min((u32)abs(dev->number_of_steps) *
> DELAY_PER_STEP_NS,
> - (u32)DELAY_MAX_PER_STEP_NS),
> - };
> + (u32)DELAY_MAX_PER_STEP_NS));
Since you are touching this, it might make sense to convert to
min_t(u32, ...)
...and locate lines something like:
ktime_t timeday = ns_to_ktime(min_t(u32,
param1,
param2));
>From my pov will make readability better.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [Y2038] [PATCH 7/8] [media] staging: atomisp: convert timestamps to ktime_t
2017-11-27 15:05 ` Andy Shevchenko
@ 2017-11-27 15:20 ` Arnd Bergmann
0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2017-11-27 15:20 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Alan Cox, Sakari Ailus, Mauro Carvalho Chehab,
Greg Kroah-Hartman, y2038 Mailman List, devel,
Linux Kernel Mailing List, Linux Media Mailing List
On Mon, Nov 27, 2017 at 4:05 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2017-11-27 at 14:19 +0100, Arnd Bergmann wrote:
>> timespec overflows in 2038 on 32-bit architectures, and the
>> getnstimeofday() suffers from possible time jumps, so the
>> timestamps here are better done using ktime_get(), which has
>> neither of those problems.
>>
>> In case of ov2680, we don't seem to use the timestamp at
>> all, so I just remove it.
>>
>
>> + ktime_t timedelay = ns_to_ktime(
>> min((u32)abs(dev->number_of_steps) *
>> DELAY_PER_STEP_NS,
>> - (u32)DELAY_MAX_PER_STEP_NS),
>> - };
>> + (u32)DELAY_MAX_PER_STEP_NS));
>
> Since you are touching this, it might make sense to convert to
>
> min_t(u32, ...)
>
> ...and locate lines something like:
>
> ktime_t timeday = ns_to_ktime(min_t(u32,
> param1,
> param2));
>
> From my pov will make readability better.
Yes, good idea. Re-sending the patch now. Thanks for taking a look,
Arnd
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 8/8] [media] staging: imx: use ktime_t for timestamps
2017-11-27 13:19 [PATCH 1/8] [media] uvc_video: use ktime_t for stats Arnd Bergmann
` (4 preceding siblings ...)
2017-11-27 13:19 ` [PATCH 7/8] [media] staging: atomisp: convert timestamps to ktime_t Arnd Bergmann
@ 2017-11-27 13:20 ` Arnd Bergmann
2017-12-01 23:55 ` Steve Longerbeam
2017-12-05 0:40 ` [PATCH 1/8] [media] uvc_video: use ktime_t for stats Laurent Pinchart
6 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2017-11-27 13:20 UTC (permalink / raw)
To: Steve Longerbeam, Philipp Zabel, Mauro Carvalho Chehab,
Greg Kroah-Hartman
Cc: y2038, Arnd Bergmann, Hans Verkuil, Russell King, linux-media,
devel, linux-kernel
The imx media driver passes around monotonic timestamps in the deprecated
'timespec' format. This is not a problem for the driver, as they won't
overflow, but moving to either timespec64 or ktime_t is preferred.
I'm picking ktime_t for simplicity here. frame_interval_monitor() is
the main function that changes, as it tries to compare a time interval
in microseconds. The algorithm slightly changes here, to avoid 64-bit
division. The code previously assumed that the error was at most 32-bit
worth of microseconds here, so I'm making the same assumption but add
an explicit test for it.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/staging/media/imx/imx-media-csi.c | 8 ++------
drivers/staging/media/imx/imx-media-fim.c | 30 +++++++++++++++++-------------
drivers/staging/media/imx/imx-media.h | 2 +-
3 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index bb1d6dafca83..26994b429cf2 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -207,13 +207,9 @@ static irqreturn_t csi_idmac_eof_interrupt(int irq, void *dev_id)
goto unlock;
}
- if (priv->fim) {
- struct timespec cur_ts;
-
- ktime_get_ts(&cur_ts);
+ if (priv->fim)
/* call frame interval monitor */
- imx_media_fim_eof_monitor(priv->fim, &cur_ts);
- }
+ imx_media_fim_eof_monitor(priv->fim, ktime_get());
csi_vb2_buf_done(priv);
diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c
index 47275ef803f3..6df189135db8 100644
--- a/drivers/staging/media/imx/imx-media-fim.c
+++ b/drivers/staging/media/imx/imx-media-fim.c
@@ -66,7 +66,7 @@ struct imx_media_fim {
int icap_flags;
int counter;
- struct timespec last_ts;
+ ktime_t last_ts;
unsigned long sum; /* usec */
unsigned long nominal; /* usec */
@@ -147,22 +147,26 @@ static void send_fim_event(struct imx_media_fim *fim, unsigned long error)
* (presumably random) interrupt latency.
*/
static void frame_interval_monitor(struct imx_media_fim *fim,
- struct timespec *ts)
+ ktime_t timestamp)
{
- unsigned long interval, error, error_avg;
+ long long interval, error;
+ unsigned long error_avg;
bool send_event = false;
- struct timespec diff;
if (!fim->enabled || ++fim->counter <= 0)
goto out_update_ts;
- diff = timespec_sub(*ts, fim->last_ts);
- interval = diff.tv_sec * 1000 * 1000 + diff.tv_nsec / 1000;
- error = abs(interval - fim->nominal);
+ /* max error is less than l00µs, so use 32-bit division or fail */
+ interval = ktime_to_ns(ktime_sub(timestamp, fim->last_ts));
+ error = abs(interval - NSEC_PER_USEC * (u64)fim->nominal);
+ if (error > U32_MAX)
+ error = U32_MAX;
+ else
+ error = abs((u32)error / NSEC_PER_USEC);
if (fim->tolerance_max && error >= fim->tolerance_max) {
dev_dbg(fim->sd->dev,
- "FIM: %lu ignored, out of tolerance bounds\n",
+ "FIM: %llu ignored, out of tolerance bounds\n",
error);
fim->counter--;
goto out_update_ts;
@@ -184,7 +188,7 @@ static void frame_interval_monitor(struct imx_media_fim *fim,
}
out_update_ts:
- fim->last_ts = *ts;
+ fim->last_ts = timestamp;
if (send_event)
send_fim_event(fim, error_avg);
}
@@ -195,14 +199,14 @@ static void frame_interval_monitor(struct imx_media_fim *fim,
* to interrupt latency.
*/
static void fim_input_capture_handler(int channel, void *dev_id,
- struct timespec *ts)
+ ktime_t timestamp)
{
struct imx_media_fim *fim = dev_id;
unsigned long flags;
spin_lock_irqsave(&fim->lock, flags);
- frame_interval_monitor(fim, ts);
+ frame_interval_monitor(fim, timestamp);
if (!completion_done(&fim->icap_first_event))
complete(&fim->icap_first_event);
@@ -405,14 +409,14 @@ static int init_fim_controls(struct imx_media_fim *fim)
* the frame_interval_monitor() is called by the input capture event
* callback handler in that case.
*/
-void imx_media_fim_eof_monitor(struct imx_media_fim *fim, struct timespec *ts)
+void imx_media_fim_eof_monitor(struct imx_media_fim *fim, ktime_t timestamp)
{
unsigned long flags;
spin_lock_irqsave(&fim->lock, flags);
if (!icap_enabled(fim))
- frame_interval_monitor(fim, ts);
+ frame_interval_monitor(fim, timestamp);
spin_unlock_irqrestore(&fim->lock, flags);
}
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index d409170632bd..ac3ab115394f 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -280,7 +280,7 @@ int imx_media_pipeline_set_stream(struct imx_media_dev *imxmd,
/* imx-media-fim.c */
struct imx_media_fim;
-void imx_media_fim_eof_monitor(struct imx_media_fim *fim, struct timespec *ts);
+void imx_media_fim_eof_monitor(struct imx_media_fim *fim, ktime_t timestamp);
int imx_media_fim_set_stream(struct imx_media_fim *fim,
const struct v4l2_fract *frame_interval,
bool on);
--
2.9.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 8/8] [media] staging: imx: use ktime_t for timestamps
2017-11-27 13:20 ` [PATCH 8/8] [media] staging: imx: use ktime_t for timestamps Arnd Bergmann
@ 2017-12-01 23:55 ` Steve Longerbeam
0 siblings, 0 replies; 19+ messages in thread
From: Steve Longerbeam @ 2017-12-01 23:55 UTC (permalink / raw)
To: Arnd Bergmann, Philipp Zabel, Mauro Carvalho Chehab, Greg Kroah-Hartman
Cc: y2038, Hans Verkuil, Russell King, linux-media, devel, linux-kernel
Hi Arnd,
This looks fine to me, except for a couple things...
On 11/27/2017 05:20 AM, Arnd Bergmann wrote:
> The imx media driver passes around monotonic timestamps in the deprecated
> 'timespec' format. This is not a problem for the driver, as they won't
> overflow, but moving to either timespec64 or ktime_t is preferred.
>
> I'm picking ktime_t for simplicity here. frame_interval_monitor() is
> the main function that changes, as it tries to compare a time interval
> in microseconds. The algorithm slightly changes here, to avoid 64-bit
> division. The code previously assumed that the error was at most 32-bit
> worth of microseconds here, so I'm making the same assumption but add
> an explicit test for it.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/staging/media/imx/imx-media-csi.c | 8 ++------
> drivers/staging/media/imx/imx-media-fim.c | 30 +++++++++++++++++-------------
> drivers/staging/media/imx/imx-media.h | 2 +-
> 3 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index bb1d6dafca83..26994b429cf2 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -207,13 +207,9 @@ static irqreturn_t csi_idmac_eof_interrupt(int irq, void *dev_id)
> goto unlock;
> }
>
> - if (priv->fim) {
> - struct timespec cur_ts;
> -
> - ktime_get_ts(&cur_ts);
> + if (priv->fim)
> /* call frame interval monitor */
> - imx_media_fim_eof_monitor(priv->fim, &cur_ts);
> - }
> + imx_media_fim_eof_monitor(priv->fim, ktime_get());
>
> csi_vb2_buf_done(priv);
>
> diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c
> index 47275ef803f3..6df189135db8 100644
> --- a/drivers/staging/media/imx/imx-media-fim.c
> +++ b/drivers/staging/media/imx/imx-media-fim.c
> @@ -66,7 +66,7 @@ struct imx_media_fim {
> int icap_flags;
>
> int counter;
> - struct timespec last_ts;
> + ktime_t last_ts;
> unsigned long sum; /* usec */
> unsigned long nominal; /* usec */
>
> @@ -147,22 +147,26 @@ static void send_fim_event(struct imx_media_fim *fim, unsigned long error)
> * (presumably random) interrupt latency.
> */
> static void frame_interval_monitor(struct imx_media_fim *fim,
> - struct timespec *ts)
> + ktime_t timestamp)
> {
> - unsigned long interval, error, error_avg;
> + long long interval, error;
> + unsigned long error_avg;
> bool send_event = false;
> - struct timespec diff;
>
> if (!fim->enabled || ++fim->counter <= 0)
> goto out_update_ts;
>
> - diff = timespec_sub(*ts, fim->last_ts);
> - interval = diff.tv_sec * 1000 * 1000 + diff.tv_nsec / 1000;
> - error = abs(interval - fim->nominal);
> + /* max error is less than l00µs, so use 32-bit division or fail */
This comment doesn't make sense. By "max error" maybe you are referring
to the tolerance_max control, which is limited to 500 usec, but if set
to zero
(or less than tolerance_min), there is no max error, it is unbounded. Please
just remove this comment.
> + interval = ktime_to_ns(ktime_sub(timestamp, fim->last_ts));
> + error = abs(interval - NSEC_PER_USEC * (u64)fim->nominal);
> + if (error > U32_MAX)
> + error = U32_MAX;
> + else
> + error = abs((u32)error / NSEC_PER_USEC);
Why the second abs() ?
How about the following:
- if (error > U32_MAX)
- error = U32_MAX;
- else
- error = abs((u32)error / NSEC_PER_USEC);
+ /* convert interval error to usec using 32-bit divide */
+ error = (u32)min_t(long long, error, U32_MAX) / NSEC_PER_USEC;
Steve
>
> if (fim->tolerance_max && error >= fim->tolerance_max) {
> dev_dbg(fim->sd->dev,
> - "FIM: %lu ignored, out of tolerance bounds\n",
> + "FIM: %llu ignored, out of tolerance bounds\n",
> error);
> fim->counter--;
> goto out_update_ts;
> @@ -184,7 +188,7 @@ static void frame_interval_monitor(struct imx_media_fim *fim,
> }
>
> out_update_ts:
> - fim->last_ts = *ts;
> + fim->last_ts = timestamp;
> if (send_event)
> send_fim_event(fim, error_avg);
> }
> @@ -195,14 +199,14 @@ static void frame_interval_monitor(struct imx_media_fim *fim,
> * to interrupt latency.
> */
> static void fim_input_capture_handler(int channel, void *dev_id,
> - struct timespec *ts)
> + ktime_t timestamp)
> {
> struct imx_media_fim *fim = dev_id;
> unsigned long flags;
>
> spin_lock_irqsave(&fim->lock, flags);
>
> - frame_interval_monitor(fim, ts);
> + frame_interval_monitor(fim, timestamp);
>
> if (!completion_done(&fim->icap_first_event))
> complete(&fim->icap_first_event);
> @@ -405,14 +409,14 @@ static int init_fim_controls(struct imx_media_fim *fim)
> * the frame_interval_monitor() is called by the input capture event
> * callback handler in that case.
> */
> -void imx_media_fim_eof_monitor(struct imx_media_fim *fim, struct timespec *ts)
> +void imx_media_fim_eof_monitor(struct imx_media_fim *fim, ktime_t timestamp)
> {
> unsigned long flags;
>
> spin_lock_irqsave(&fim->lock, flags);
>
> if (!icap_enabled(fim))
> - frame_interval_monitor(fim, ts);
> + frame_interval_monitor(fim, timestamp);
>
> spin_unlock_irqrestore(&fim->lock, flags);
> }
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index d409170632bd..ac3ab115394f 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -280,7 +280,7 @@ int imx_media_pipeline_set_stream(struct imx_media_dev *imxmd,
>
> /* imx-media-fim.c */
> struct imx_media_fim;
> -void imx_media_fim_eof_monitor(struct imx_media_fim *fim, struct timespec *ts);
> +void imx_media_fim_eof_monitor(struct imx_media_fim *fim, ktime_t timestamp);
> int imx_media_fim_set_stream(struct imx_media_fim *fim,
> const struct v4l2_fract *frame_interval,
> bool on);
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/8] [media] uvc_video: use ktime_t for stats
2017-11-27 13:19 [PATCH 1/8] [media] uvc_video: use ktime_t for stats Arnd Bergmann
` (5 preceding siblings ...)
2017-11-27 13:20 ` [PATCH 8/8] [media] staging: imx: use ktime_t for timestamps Arnd Bergmann
@ 2017-12-05 0:40 ` Laurent Pinchart
6 siblings, 0 replies; 19+ messages in thread
From: Laurent Pinchart @ 2017-12-05 0:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mauro Carvalho Chehab, y2038, Kieran Bingham,
Guennadi Liakhovetski, Hans Verkuil, linux-media, linux-kernel
Hi Arnd,
Thank you for the patch.
On Monday, 27 November 2017 15:19:53 EET Arnd Bergmann wrote:
> 'struct timespec' works fine here, but we try to migrate
> away from it in favor of ktime_t or timespec64. In this
> case, using ktime_t produces the simplest code.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/usb/uvc/uvc_video.c | 11 ++++-------
> drivers/media/usb/uvc/uvcvideo.h | 4 ++--
> 2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index fb86d6af398d..d6bee37cd1b8 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -725,7 +725,7 @@ static void uvc_video_stats_decode(struct uvc_streaming
> *stream,
>
> if (stream->stats.stream.nb_frames == 0 &&
> stream->stats.frame.nb_packets == 0)
> - ktime_get_ts(&stream->stats.stream.start_ts);
> + stream->stats.stream.start_ts = ktime_get();
>
> switch (data[1] & (UVC_STREAM_PTS | UVC_STREAM_SCR)) {
> case UVC_STREAM_PTS | UVC_STREAM_SCR:
> @@ -865,16 +865,13 @@ size_t uvc_video_stats_dump(struct uvc_streaming
> *stream, char *buf, {
> unsigned int scr_sof_freq;
> unsigned int duration;
> - struct timespec ts;
> size_t count = 0;
>
> - ts = timespec_sub(stream->stats.stream.stop_ts,
> - stream->stats.stream.start_ts);
> -
> /* Compute the SCR.SOF frequency estimate. At the nominal 1kHz SOF
> * frequency this will not overflow before more than 1h.
> */
> - duration = ts.tv_sec * 1000 + ts.tv_nsec / 1000000;
> + duration = ktime_ms_delta(stream->stats.stream.stop_ts,
> + stream->stats.stream.start_ts);
> if (duration != 0)
> scr_sof_freq = stream->stats.stream.scr_sof_count * 1000
> / duration;
> @@ -915,7 +912,7 @@ static void uvc_video_stats_start(struct uvc_streaming
> *stream)
>
> static void uvc_video_stats_stop(struct uvc_streaming *stream)
> {
> - ktime_get_ts(&stream->stats.stream.stop_ts);
> + stream->stats.stream.stop_ts = ktime_get();
> }
>
> /* ------------------------------------------------------------------------
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 05398784d1c8..a2c190937067 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -452,8 +452,8 @@ struct uvc_stats_frame {
> };
>
> struct uvc_stats_stream {
> - struct timespec start_ts; /* Stream start timestamp */
> - struct timespec stop_ts; /* Stream stop timestamp */
> + ktime_t start_ts; /* Stream start timestamp */
> + ktime_t stop_ts; /* Stream stop timestamp */
>
> unsigned int nb_frames; /* Number of frames */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 19+ messages in thread