Re: [PATCH v2 1/3] usb: gadget: uvc: Switch to monotonic clock for buffer timestamps

Re: [PATCH v2 1/3] usb: gadget: uvc: Switch to monotonic clock for buffer timestamps

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On Fri, Mar 28, 2014 at 05:02:46PM +0100, Laurent Pinchart wrote:
> The wall time clock isn't useful for applications as it can jump around
> due to time adjustement. Switch to the monotonic clock.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Felipe Balbi <balbi@ti.com>

> ---
>  drivers/usb/gadget/uvc_queue.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> Changes since v1:
> 
> - Replace ktime_get_ts() with v4l2_get_timestamp()
> 
> diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
> index 0bb5d50..9ac4ffe1 100644
> --- a/drivers/usb/gadget/uvc_queue.c
> +++ b/drivers/usb/gadget/uvc_queue.c
> @@ -20,6 +20,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/wait.h>
>  
> +#include <media/v4l2-common.h>
>  #include <media/videobuf2-vmalloc.h>
>  
>  #include "uvc.h"
> @@ -379,14 +380,8 @@ static struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
>  	else
>  		nextbuf = NULL;
>  
> -	/*
> -	 * FIXME: with videobuf2, the sequence number or timestamp fields
> -	 * are valid only for video capture devices and the UVC gadget usually
> -	 * is a video output device. Keeping these until the specs are clear on
> -	 * this aspect.
> -	 */
>  	buf->buf.v4l2_buf.sequence = queue->sequence++;
> -	do_gettimeofday(&buf->buf.v4l2_buf.timestamp);
> +	v4l2_get_timestamp(&buf->buf.v4l2_buf.timestamp);
>  
>  	vb2_set_plane_payload(&buf->buf, 0, buf->bytesused);
>  	vb2_buffer_done(&buf->buf, VB2_BUF_STATE_DONE);
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/3] usb: gadget: uvc: Set the V4L2 buffer field to V4L2_FIELD_NONE

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Fri, Mar 28, 2014 at 05:02:47PM +0100, Laurent Pinchart wrote:
> The UVC gadget driver doesn't support interlaced video but left the
> buffer field uninitialized. Set it to V4L2_FIELD_NONE.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Felipe Balbi <balbi@ti.com>

> ---
>  drivers/usb/gadget/uvc_queue.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
> index 9ac4ffe1..305eb49 100644
> --- a/drivers/usb/gadget/uvc_queue.c
> +++ b/drivers/usb/gadget/uvc_queue.c
> @@ -380,6 +380,7 @@ static struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
>  	else
>  		nextbuf = NULL;
>  
> +	buf->buf.v4l2_buf.field = V4L2_FIELD_NONE;
>  	buf->buf.v4l2_buf.sequence = queue->sequence++;
>  	v4l2_get_timestamp(&buf->buf.v4l2_buf.timestamp);
>  
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 3/3] usb: gadget: uvc: Set the vb2 queue timestamp flags

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1730 bytes --]

On Fri, Mar 28, 2014 at 05:02:48PM +0100, Laurent Pinchart wrote:
> The vb2 queue timestamp_flags field must be set by drivers, as enforced
> by a WARN_ON in vb2_queue_init. The UVC gadget driver failed to do so.
> This resulted in the following warning.
> 
> [    2.104371] g_webcam gadget: uvc_function_bind
> [    2.105567] ------------[ cut here ]------------
> [    2.105567] ------------[ cut here ]------------
> [    2.106779] WARNING: CPU: 0 PID: 1 at drivers/media/v4l2-core/videobuf2-core.c:2207 vb2_queue_init+0xa3/0x113()
> 
> Fix it.
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Felipe Balbi <balbi@ti.com>

> ---
>  drivers/usb/gadget/uvc_queue.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
> index 305eb49..1c29bc9 100644
> --- a/drivers/usb/gadget/uvc_queue.c
> +++ b/drivers/usb/gadget/uvc_queue.c
> @@ -137,6 +137,8 @@ static int uvc_queue_init(struct uvc_video_queue *queue,
>  	queue->queue.buf_struct_size = sizeof(struct uvc_buffer);
>  	queue->queue.ops = &uvc_queue_qops;
>  	queue->queue.mem_ops = &vb2_vmalloc_memops;
> +	queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> +				     | V4L2_BUF_FLAG_TSTAMP_SRC_EOF;
>  	ret = vb2_queue_init(&queue->queue);
>  	if (ret)
>  		return ret;
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 0/3] Miscellaneous fixes for UVC gadget driver

[Laurent Pinchart]

Hello,

These three patches fix miscellaneous issues in the UVC gadget driver. Patches
1 and 3 have already been posted as part of the "Clock fixes for UVC gadget
driver" series, and patch 2 is new.

The series is based on the latest media tree master branch as it depends on
commit 872484ce40881e295b046adf21f7211306477751 ("v4l: Add timestamp source
flags, mask and document them") queued for v3.15. It would thus be easier to
merge it through the media tree. Greg and Mauro, would that be fine ?
Alternatively I can rebase it on top of v3.15-rc1 when that version will be
tagged.

Laurent Pinchart (3):
  usb: gadget: uvc: Switch to monotonic clock for buffer timestamps
  usb: gadget: uvc: Set the V4L2 buffer field to V4L2_FIELD_NONE
  usb: gadget: uvc: Set the vb2 queue timestamp flags

 drivers/usb/gadget/uvc_queue.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/3] usb: gadget: uvc: Switch to monotonic clock for buffer timestamps

[Laurent Pinchart]

The wall time clock isn't useful for applications as it can jump around
due to time adjustement. Switch to the monotonic clock.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/usb/gadget/uvc_queue.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Changes since v1:

- Replace ktime_get_ts() with v4l2_get_timestamp()

diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
index 0bb5d50..9ac4ffe1 100644
--- a/drivers/usb/gadget/uvc_queue.c
+++ b/drivers/usb/gadget/uvc_queue.c
@@ -20,6 +20,7 @@
 #include <linux/vmalloc.h>
 #include <linux/wait.h>
 
+#include <media/v4l2-common.h>
 #include <media/videobuf2-vmalloc.h>
 
 #include "uvc.h"
@@ -379,14 +380,8 @@ static struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 	else
 		nextbuf = NULL;
 
-	/*
-	 * FIXME: with videobuf2, the sequence number or timestamp fields
-	 * are valid only for video capture devices and the UVC gadget usually
-	 * is a video output device. Keeping these until the specs are clear on
-	 * this aspect.
-	 */
 	buf->buf.v4l2_buf.sequence = queue->sequence++;
-	do_gettimeofday(&buf->buf.v4l2_buf.timestamp);
+	v4l2_get_timestamp(&buf->buf.v4l2_buf.timestamp);
 
 	vb2_set_plane_payload(&buf->buf, 0, buf->bytesused);
 	vb2_buffer_done(&buf->buf, VB2_BUF_STATE_DONE);
-- 
1.8.3.2

[PATCH v2 2/3] usb: gadget: uvc: Set the V4L2 buffer field to V4L2_FIELD_NONE

[Laurent Pinchart]

The UVC gadget driver doesn't support interlaced video but left the
buffer field uninitialized. Set it to V4L2_FIELD_NONE.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/usb/gadget/uvc_queue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
index 9ac4ffe1..305eb49 100644
--- a/drivers/usb/gadget/uvc_queue.c
+++ b/drivers/usb/gadget/uvc_queue.c
@@ -380,6 +380,7 @@ static struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 	else
 		nextbuf = NULL;
 
+	buf->buf.v4l2_buf.field = V4L2_FIELD_NONE;
 	buf->buf.v4l2_buf.sequence = queue->sequence++;
 	v4l2_get_timestamp(&buf->buf.v4l2_buf.timestamp);
 
-- 
1.8.3.2

[PATCH v2 3/3] usb: gadget: uvc: Set the vb2 queue timestamp flags

[Laurent Pinchart]

The vb2 queue timestamp_flags field must be set by drivers, as enforced
by a WARN_ON in vb2_queue_init. The UVC gadget driver failed to do so.
This resulted in the following warning.

[    2.104371] g_webcam gadget: uvc_function_bind
[    2.105567] ------------[ cut here ]------------
[    2.105567] ------------[ cut here ]------------
[    2.106779] WARNING: CPU: 0 PID: 1 at drivers/media/v4l2-core/videobuf2-core.c:2207 vb2_queue_init+0xa3/0x113()

Fix it.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/usb/gadget/uvc_queue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
index 305eb49..1c29bc9 100644
--- a/drivers/usb/gadget/uvc_queue.c
+++ b/drivers/usb/gadget/uvc_queue.c
@@ -137,6 +137,8 @@ static int uvc_queue_init(struct uvc_video_queue *queue,
 	queue->queue.buf_struct_size = sizeof(struct uvc_buffer);
 	queue->queue.ops = &uvc_queue_qops;
 	queue->queue.mem_ops = &vb2_vmalloc_memops;
+	queue->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
+				     | V4L2_BUF_FLAG_TSTAMP_SRC_EOF;
 	ret = vb2_queue_init(&queue->queue);
 	if (ret)
 		return ret;
-- 
1.8.3.2

Re: [PATCH v2 1/3] usb: gadget: uvc: Switch to monotonic clock for buffer timestamps

[Hans Verkuil]

On 03/28/2014 05:02 PM, Laurent Pinchart wrote:
> The wall time clock isn't useful for applications as it can jump around
> due to time adjustement. Switch to the monotonic clock.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>  drivers/usb/gadget/uvc_queue.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> Changes since v1:
> 
> - Replace ktime_get_ts() with v4l2_get_timestamp()
> 
> diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
> index 0bb5d50..9ac4ffe1 100644
> --- a/drivers/usb/gadget/uvc_queue.c
> +++ b/drivers/usb/gadget/uvc_queue.c
> @@ -20,6 +20,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/wait.h>
>  
> +#include <media/v4l2-common.h>
>  #include <media/videobuf2-vmalloc.h>
>  
>  #include "uvc.h"
> @@ -379,14 +380,8 @@ static struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
>  	else
>  		nextbuf = NULL;
>  
> -	/*
> -	 * FIXME: with videobuf2, the sequence number or timestamp fields
> -	 * are valid only for video capture devices and the UVC gadget usually
> -	 * is a video output device. Keeping these until the specs are clear on
> -	 * this aspect.
> -	 */
>  	buf->buf.v4l2_buf.sequence = queue->sequence++;
> -	do_gettimeofday(&buf->buf.v4l2_buf.timestamp);
> +	v4l2_get_timestamp(&buf->buf.v4l2_buf.timestamp);
>  
>  	vb2_set_plane_payload(&buf->buf, 0, buf->bytesused);
>  	vb2_buffer_done(&buf->buf, VB2_BUF_STATE_DONE);
> 

Re: [PATCH v2 2/3] usb: gadget: uvc: Set the V4L2 buffer field to V4L2_FIELD_NONE

[Hans Verkuil]

On 03/28/2014 05:02 PM, Laurent Pinchart wrote:
> The UVC gadget driver doesn't support interlaced video but left the
> buffer field uninitialized. Set it to V4L2_FIELD_NONE.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>  drivers/usb/gadget/uvc_queue.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
> index 9ac4ffe1..305eb49 100644
> --- a/drivers/usb/gadget/uvc_queue.c
> +++ b/drivers/usb/gadget/uvc_queue.c
> @@ -380,6 +380,7 @@ static struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
>  	else
>  		nextbuf = NULL;
>  
> +	buf->buf.v4l2_buf.field = V4L2_FIELD_NONE;
>  	buf->buf.v4l2_buf.sequence = queue->sequence++;
>  	v4l2_get_timestamp(&buf->buf.v4l2_buf.timestamp);
>  
>