diff mbox series

[2/8,media] uvc_video: use ktime_t for timestamps

Message ID 20171127132027.1734806-2-arnd@arndb.de
State Accepted
Commit 828ee8c719501551d4cdd1ace8425284951dfb82
Headers show
Series None | expand

Commit Message

Arnd Bergmann Nov. 27, 2017, 1:19 p.m. UTC 
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(-)

-- 
2.9.0

Comments

Laurent Pinchart Dec. 5, 2017, 12:37 a.m. UTC | #1 
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
Laurent Pinchart Dec. 5, 2017, 12:58 a.m. UTC | #2 
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
Arnd Bergmann Dec. 5, 2017, 11:27 a.m. UTC | #3 
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
diff mbox series

Patch

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;