[PATCH v5 0/7] Fix buffer timestamp documentation, add new timestamp flags

[PATCH v5 0/7] Fix buffer timestamp documentation, add new timestamp flags

[Sakari Ailus]

Hi all,

This is the fifth version of the set after a long break. v4 (including
v4.[12]) can be found here:

<URL:http://www.spinics.net/lists/linux-media/msg67445.html>

since v4.2:

- In a few places in documentation it was stated that setting timestamp for
  output devices will affect the time the frame is displayed. The patch now
  removes that statement. Patch 1/7.

- SOF timestamp was changed into SOE timestamp to signify start of exposure.
  This corresponds to what the UVC devices do according to the spec. SOE is
  only valid for CAPTURE queues. 

- Timestamp is always copied from source to destination, not the other way
  around. Drivers affected were exynos-gsc, m2m-deinterlace and mx2_emmaprp.
  Patch 5/7. Kamil: could you check especially this one, please?

- Timestamp source flags are copied but not the timestamp type (which, well,
  is always "COPY". Change all m2m drivers to do so.

-- 
Kind regards,
Sakari

[PATCH v5 1/7] v4l: Document timestamp behaviour to correspond to reality

[Sakari Ailus]

Document that monotonic timestamps are taken after the corresponding frame
has been received, not when the reception has begun. This corresponds to the
reality of current drivers: the timestamp is naturally taken when the
hardware triggers an interrupt to tell the driver to handle the received
frame.

Remove the note on timestamp accuracy as it is fairly subjective what is
actually an unstable timestamp.

Also remove explanation that output buffer timestamps can be used to delay
outputting a frame.

Remove the footnote saying we always use realtime clock.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/DocBook/media/v4l/io.xml |   56 +++++++-------------------------
 1 file changed, 12 insertions(+), 44 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 2c4c068..8facac4 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -339,8 +339,8 @@ returns immediately with an &EAGAIN; when no buffer is available. The
 queues as a side effect. Since there is no notion of doing anything
 "now" on a multitasking system, if an application needs to synchronize
 with another event it should examine the &v4l2-buffer;
-<structfield>timestamp</structfield> of captured buffers, or set the
-field before enqueuing buffers for output.</para>
+<structfield>timestamp</structfield> of captured or outputted buffers.
+</para>
 
     <para>Drivers implementing memory mapping I/O must
 support the <constant>VIDIOC_REQBUFS</constant>,
@@ -457,7 +457,7 @@ queues and unlocks all buffers as a side effect. Since there is no
 notion of doing anything "now" on a multitasking system, if an
 application needs to synchronize with another event it should examine
 the &v4l2-buffer; <structfield>timestamp</structfield> of captured
-buffers, or set the field before enqueuing buffers for output.</para>
+or outputted buffers.</para>
 
     <para>Drivers implementing user pointer I/O must
 support the <constant>VIDIOC_REQBUFS</constant>,
@@ -620,8 +620,7 @@ returns immediately with an &EAGAIN; when no buffer is available. The
 unlocks all buffers as a side effect. Since there is no notion of doing
 anything "now" on a multitasking system, if an application needs to synchronize
 with another event it should examine the &v4l2-buffer;
-<structfield>timestamp</structfield> of captured buffers, or set the field
-before enqueuing buffers for output.</para>
+<structfield>timestamp</structfield> of captured or outputted buffers.</para>
 
     <para>Drivers implementing DMABUF importing I/O must support the
 <constant>VIDIOC_REQBUFS</constant>, <constant>VIDIOC_QBUF</constant>,
@@ -654,38 +653,11 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
 In that case, struct <structname>v4l2_buffer</structname> contains an array of
 plane structures.</para>
 
-      <para>Nominally timestamps refer to the first data byte transmitted.
-In practice however the wide range of hardware covered by the V4L2 API
-limits timestamp accuracy. Often an interrupt routine will
-sample the system clock shortly after the field or frame was stored
-completely in memory. So applications must expect a constant
-difference up to one field or frame period plus a small (few scan
-lines) random error. The delay and error can be much
-larger due to compression or transmission over an external bus when
-the frames are not properly stamped by the sender. This is frequently
-the case with USB cameras. Here timestamps refer to the instant the
-field or frame was received by the driver, not the capture time. These
-devices identify by not enumerating any video standards, see <xref
-linkend="standard" />.</para>
-
-      <para>Similar limitations apply to output timestamps. Typically
-the video hardware locks to a clock controlling the video timing, the
-horizontal and vertical synchronization pulses. At some point in the
-line sequence, possibly the vertical blanking, an interrupt routine
-samples the system clock, compares against the timestamp and programs
-the hardware to repeat the previous field or frame, or to display the
-buffer contents.</para>
-
-      <para>Apart of limitations of the video device and natural
-inaccuracies of all clocks, it should be noted system time itself is
-not perfectly stable. It can be affected by power saving cycles,
-warped to insert leap seconds, or even turned back or forth by the
-system administrator affecting long term measurements. <footnote>
-	  <para>Since no other Linux multimedia
-API supports unadjusted time it would be foolish to introduce here. We
-must use a universally supported clock to synchronize different media,
-hence time of day.</para>
-	</footnote></para>
+      <para>For timestamp types that are sampled from the system clock
+(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp is
+taken after the complete frame has been received (or transmitted in
+case of video output devices). For other kinds of
+timestamps this may vary depending on the driver.</para>
 
     <table frame="none" pgwide="1" id="v4l2-buffer">
       <title>struct <structname>v4l2_buffer</structname></title>
@@ -745,13 +717,9 @@ applications when an output stream.</entry>
 	    byte was captured, as returned by the
 	    <function>clock_gettime()</function> function for the relevant
 	    clock id; see <constant>V4L2_BUF_FLAG_TIMESTAMP_*</constant> in
-	    <xref linkend="buffer-flags" />. For output streams the data
-	    will not be displayed before this time, secondary to the nominal
-	    frame rate determined by the current video standard in enqueued
-	    order. Applications can for example zero this field to display
-	    frames as soon as possible. The driver stores the time at which
-	    the first data byte was actually sent out in the
-	    <structfield>timestamp</structfield> field. This permits
+	    <xref linkend="buffer-flags" />. For output streams the driver
+	    stores the time at which the last data byte was actually sent out
+	    in the  <structfield>timestamp</structfield> field. This permits
 	    applications to monitor the drift between the video and system
 	    clock.</para></entry>
 	  </row>
-- 
1.7.10.4

[PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

[Sakari Ailus]

The buffer flags field is 32 bits but the defined only used 16. This is
fine, but as more than 16 bits will be used in the very near future, define
them as 32-bit numbers for consistency.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++-------------
 include/uapi/linux/videodev2.h         |   38 +++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 8facac4..46d24b3 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -984,7 +984,7 @@ should set this to 0.</entry>
 	<tbody valign="top">
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_MAPPED</constant></entry>
-	    <entry>0x0001</entry>
+	    <entry>0x00000001</entry>
 	    <entry>The buffer resides in device memory and has been mapped
 into the application's address space, see <xref linkend="mmap" /> for details.
 Drivers set or clear this flag when the
@@ -994,7 +994,7 @@ Drivers set or clear this flag when the
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_QUEUED</constant></entry>
-	    <entry>0x0002</entry>
+	    <entry>0x00000002</entry>
 	  <entry>Internally drivers maintain two buffer queues, an
 incoming and outgoing queue. When this flag is set, the buffer is
 currently on the incoming queue. It automatically moves to the
@@ -1007,7 +1007,7 @@ cleared.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_DONE</constant></entry>
-	    <entry>0x0004</entry>
+	    <entry>0x00000004</entry>
 	    <entry>When this flag is set, the buffer is currently on
 the outgoing queue, ready to be dequeued from the driver. Drivers set
 or clear this flag when the <constant>VIDIOC_QUERYBUF</constant> ioctl
@@ -1021,7 +1021,7 @@ state, in the application domain to say so.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_ERROR</constant></entry>
-	    <entry>0x0040</entry>
+	    <entry>0x00000040</entry>
 	    <entry>When this flag is set, the buffer has been dequeued
 	    successfully, although the data might have been corrupted.
 	    This is recoverable, streaming may continue as normal and
@@ -1031,7 +1031,7 @@ state, in the application domain to say so.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_KEYFRAME</constant></entry>
-	    <entry>0x0008</entry>
+	    <entry>0x00000008</entry>
 	  <entry>Drivers set or clear this flag when calling the
 <constant>VIDIOC_DQBUF</constant> ioctl. It may be set by video
 capture devices when the buffer contains a compressed image which is a
@@ -1039,27 +1039,27 @@ key frame (or field), &ie; can be decompressed on its own.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_PFRAME</constant></entry>
-	    <entry>0x0010</entry>
+	    <entry>0x00000010</entry>
 	    <entry>Similar to <constant>V4L2_BUF_FLAG_KEYFRAME</constant>
 this flags predicted frames or fields which contain only differences to a
 previous key frame.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_BFRAME</constant></entry>
-	    <entry>0x0020</entry>
+	    <entry>0x00000020</entry>
 	    <entry>Similar to <constant>V4L2_BUF_FLAG_PFRAME</constant>
 	this is a bidirectional predicted frame or field. [ooc tbd]</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_TIMECODE</constant></entry>
-	    <entry>0x0100</entry>
+	    <entry>0x00000100</entry>
 	    <entry>The <structfield>timecode</structfield> field is valid.
 Drivers set or clear this flag when the <constant>VIDIOC_DQBUF</constant>
 ioctl is called.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_PREPARED</constant></entry>
-	    <entry>0x0400</entry>
+	    <entry>0x00000400</entry>
 	    <entry>The buffer has been prepared for I/O and can be queued by the
 application. Drivers set or clear this flag when the
 <link linkend="vidioc-querybuf">VIDIOC_QUERYBUF</link>, <link
@@ -1069,7 +1069,7 @@ application. Drivers set or clear this flag when the
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant></entry>
-	    <entry>0x0800</entry>
+	    <entry>0x00000800</entry>
 	    <entry>Caches do not have to be invalidated for this buffer.
 Typically applications shall use this flag if the data captured in the buffer
 is not going to be touched by the CPU, instead the buffer will, probably, be
@@ -1078,7 +1078,7 @@ passed on to a DMA-capable hardware unit for further processing or output.
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant></entry>
-	    <entry>0x1000</entry>
+	    <entry>0x00001000</entry>
 	    <entry>Caches do not have to be cleaned for this buffer.
 Typically applications shall use this flag for output buffers if the data
 in this buffer has not been created by the CPU but by some DMA-capable unit,
@@ -1086,7 +1086,7 @@ in which case caches have not been used.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant></entry>
-	    <entry>0xe000</entry>
+	    <entry>0x0000e000</entry>
 	    <entry>Mask for timestamp types below. To test the
 	    timestamp type, mask out bits not belonging to timestamp
 	    type by performing a logical and operation with buffer
@@ -1094,7 +1094,7 @@ in which case caches have not been used.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN</constant></entry>
-	    <entry>0x0000</entry>
+	    <entry>0x00000000</entry>
 	    <entry>Unknown timestamp type. This type is used by
 	    drivers before Linux 3.9 and may be either monotonic (see
 	    below) or realtime (wall clock). Monotonic clock has been
@@ -1107,7 +1107,7 @@ in which case caches have not been used.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC</constant></entry>
-	    <entry>0x2000</entry>
+	    <entry>0x00002000</entry>
 	    <entry>The buffer timestamp has been taken from the
 	    <constant>CLOCK_MONOTONIC</constant> clock. To access the
 	    same clock outside V4L2, use
@@ -1115,7 +1115,7 @@ in which case caches have not been used.</entry>
 	  </row>
 	  <row>
 	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
-	    <entry>0x4000</entry>
+	    <entry>0x00004000</entry>
 	    <entry>The CAPTURE buffer timestamp has been taken from the
 	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
 	  </row>
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 6ae7bbe..e9ee444 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -669,24 +669,32 @@ struct v4l2_buffer {
 };
 
 /*  Flags for 'flags' field */
-#define V4L2_BUF_FLAG_MAPPED	0x0001  /* Buffer is mapped (flag) */
-#define V4L2_BUF_FLAG_QUEUED	0x0002	/* Buffer is queued for processing */
-#define V4L2_BUF_FLAG_DONE	0x0004	/* Buffer is ready */
-#define V4L2_BUF_FLAG_KEYFRAME	0x0008	/* Image is a keyframe (I-frame) */
-#define V4L2_BUF_FLAG_PFRAME	0x0010	/* Image is a P-frame */
-#define V4L2_BUF_FLAG_BFRAME	0x0020	/* Image is a B-frame */
+/* Buffer is mapped (flag) */
+#define V4L2_BUF_FLAG_MAPPED			0x00000001
+/* Buffer is queued for processing */
+#define V4L2_BUF_FLAG_QUEUED			0x00000002
+/* Buffer is ready */
+#define V4L2_BUF_FLAG_DONE			0x00000004
+/* Image is a keyframe (I-frame) */
+#define V4L2_BUF_FLAG_KEYFRAME			0x00000008
+/* Image is a P-frame */
+#define V4L2_BUF_FLAG_PFRAME			0x00000010
+/* Image is a B-frame */
+#define V4L2_BUF_FLAG_BFRAME			0x00000020
 /* Buffer is ready, but the data contained within is corrupted. */
-#define V4L2_BUF_FLAG_ERROR	0x0040
-#define V4L2_BUF_FLAG_TIMECODE	0x0100	/* timecode field is valid */
-#define V4L2_BUF_FLAG_PREPARED	0x0400	/* Buffer is prepared for queuing */
+#define V4L2_BUF_FLAG_ERROR			0x00000040
+/* timecode field is valid */
+#define V4L2_BUF_FLAG_TIMECODE			0x00000100
+/* Buffer is prepared for queuing */
+#define V4L2_BUF_FLAG_PREPARED			0x00000400
 /* Cache handling flags */
-#define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x0800
-#define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x1000
+#define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x00000800
+#define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x00001000
 /* Timestamp type */
-#define V4L2_BUF_FLAG_TIMESTAMP_MASK		0xe000
-#define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x0000
-#define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x2000
-#define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x4000
+#define V4L2_BUF_FLAG_TIMESTAMP_MASK		0x0000e000
+#define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
+#define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
+#define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
 
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
-- 
1.7.10.4

[PATCH v5 3/7] v4l: Add timestamp source flags, mask and document them

[Sakari Ailus]

Some devices do not produce timestamps that correspond to the end of the
frame. The user space should be informed on the matter. This patch achieves
that by adding buffer flags (and a mask) for timestamp sources since more
possible timestamping points are expected than just two.

A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end of
frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure (next
value).

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 Documentation/DocBook/media/v4l/io.xml   |   31 ++++++++++++++++++++++++------
 drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
 include/media/videobuf2-core.h           |    2 ++
 include/uapi/linux/videodev2.h           |    4 ++++
 4 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 46d24b3..fbd0c6e 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -653,12 +653,6 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
 In that case, struct <structname>v4l2_buffer</structname> contains an array of
 plane structures.</para>
 
-      <para>For timestamp types that are sampled from the system clock
-(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp is
-taken after the complete frame has been received (or transmitted in
-case of video output devices). For other kinds of
-timestamps this may vary depending on the driver.</para>
-
     <table frame="none" pgwide="1" id="v4l2-buffer">
       <title>struct <structname>v4l2_buffer</structname></title>
       <tgroup cols="4">
@@ -1119,6 +1113,31 @@ in which case caches have not been used.</entry>
 	    <entry>The CAPTURE buffer timestamp has been taken from the
 	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
 	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
+	    <entry>0x00070000</entry>
+	    <entry>Mask for timestamp sources below. The timestamp source
+	    defines the point of time the timestamp is taken in relation to
+	    the frame. Logical and operation between the
+	    <structfield>flags</structfield> field and
+	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
+	    value of the timestamp source.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
+	    <entry>0x00000000</entry>
+	    <entry>"End of frame." The buffer timestamp has been taken
+	    when the last pixel of the frame has been received or the
+	    last pixel of the frame has been transmitted.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
+	    <entry>0x00010000</entry>
+	    <entry>"Start of exposure." The buffer timestamp has been taken
+	    when the exposure of the frame has begun. This is only
+	    valid for buffer type
+	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 5a5fb7f..6e314b0 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2195,7 +2195,9 @@ int vb2_queue_init(struct vb2_queue *q)
 	    WARN_ON(!q->io_modes)	  ||
 	    WARN_ON(!q->ops->queue_setup) ||
 	    WARN_ON(!q->ops->buf_queue)   ||
-	    WARN_ON(q->timestamp_type & ~V4L2_BUF_FLAG_TIMESTAMP_MASK))
+	    WARN_ON(q->timestamp_type &
+		    ~(V4L2_BUF_FLAG_TIMESTAMP_MASK |
+		      V4L2_BUF_FLAG_TSTAMP_SRC_MASK)))
 		return -EINVAL;
 
 	/* Warn that the driver should choose an appropriate timestamp type */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bef53ce..b6b992d 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -312,6 +312,8 @@ struct v4l2_fh;
  * @buf_struct_size: size of the driver-specific buffer structure;
  *		"0" indicates the driver doesn't want to use a custom buffer
  *		structure type, so sizeof(struct vb2_buffer) will is used
+ * @timestamp_type: Timestamp flags; V4L2_BUF_FLAGS_TIMESTAMP_* and
+ *		V4L2_BUF_FLAGS_TSTAMP_SRC_*
  * @gfp_flags:	additional gfp flags used when allocating the buffers.
  *		Typically this is 0, but it may be e.g. GFP_DMA or __GFP_DMA32
  *		to force the buffer allocation to a specific memory zone.
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index e9ee444..82e8661 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -695,6 +695,10 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
 #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
 #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
+/* Timestamp sources. */
+#define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
+#define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
+#define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
 
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
-- 
1.7.10.4

[PATCH v5 4/7] uvcvideo: Tell the user space we're using start-of-exposure timestamps

[Sakari Ailus]

The UVC device provided timestamps are taken from the clock once the
exposure of the frame has begun, not when the reception of the frame would
have been finished as almost anywhere else. Show this to the user space by
using V4L2_BUF_FLAG_TSTAMP_SRC_SOE buffer flag.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/usb/uvc/uvc_queue.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index cd962be..a9292d2 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -149,7 +149,8 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
 	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_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	queue->queue.timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
+		| V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
 	ret = vb2_queue_init(&queue->queue);
 	if (ret)
 		return ret;
-- 
1.7.10.4

[PATCH v5 5/7] exynos-gsc, m2m-deinterlace, mx2_emmaprp: Copy v4l2_buffer data from src to dst

[Sakari Ailus]

The timestamp and timecode fields were copied from destination to source,
not the other way around as they should. Fix it.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/platform/exynos-gsc/gsc-m2m.c |    4 ++--
 drivers/media/platform/m2m-deinterlace.c    |    4 ++--
 drivers/media/platform/mx2_emmaprp.c        |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 810c3e1..62c84d5 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -88,8 +88,8 @@ void gsc_m2m_job_finish(struct gsc_ctx *ctx, int vb_state)
 	dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
 
 	if (src_vb && dst_vb) {
-		src_vb->v4l2_buf.timestamp = dst_vb->v4l2_buf.timestamp;
-		src_vb->v4l2_buf.timecode = dst_vb->v4l2_buf.timecode;
+		dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
+		dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;
 
 		v4l2_m2m_buf_done(src_vb, vb_state);
 		v4l2_m2m_buf_done(dst_vb, vb_state);
diff --git a/drivers/media/platform/m2m-deinterlace.c b/drivers/media/platform/m2m-deinterlace.c
index 6bb86b5..1f272d3 100644
--- a/drivers/media/platform/m2m-deinterlace.c
+++ b/drivers/media/platform/m2m-deinterlace.c
@@ -207,8 +207,8 @@ static void dma_callback(void *data)
 	src_vb = v4l2_m2m_src_buf_remove(curr_ctx->m2m_ctx);
 	dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->m2m_ctx);
 
-	src_vb->v4l2_buf.timestamp = dst_vb->v4l2_buf.timestamp;
-	src_vb->v4l2_buf.timecode = dst_vb->v4l2_buf.timecode;
+	dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
+	dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;
 
 	v4l2_m2m_buf_done(src_vb, VB2_BUF_STATE_DONE);
 	v4l2_m2m_buf_done(dst_vb, VB2_BUF_STATE_DONE);
diff --git a/drivers/media/platform/mx2_emmaprp.c b/drivers/media/platform/mx2_emmaprp.c
index c690435..91056ac0 100644
--- a/drivers/media/platform/mx2_emmaprp.c
+++ b/drivers/media/platform/mx2_emmaprp.c
@@ -377,8 +377,8 @@ static irqreturn_t emmaprp_irq(int irq_emma, void *data)
 			src_vb = v4l2_m2m_src_buf_remove(curr_ctx->m2m_ctx);
 			dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->m2m_ctx);
 
-			src_vb->v4l2_buf.timestamp = dst_vb->v4l2_buf.timestamp;
-			src_vb->v4l2_buf.timecode = dst_vb->v4l2_buf.timecode;
+			dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
+			dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;
 
 			spin_lock_irqsave(&pcdev->irqlock, flags);
 			v4l2_m2m_buf_done(src_vb, VB2_BUF_STATE_DONE);
-- 
1.7.10.4

[PATCH v5 6/7] v4l: Copy timestamp source flags to destination on m2m devices

[Sakari Ailus]

Copy the flags containing the timestamp source from source buffer flags to
the destination buffer flags on memory-to-memory devices. This is analogous
to copying the timestamp field from source to destination.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/platform/coda.c                |    3 +++
 drivers/media/platform/exynos-gsc/gsc-m2m.c  |    4 ++++
 drivers/media/platform/exynos4-is/fimc-m2m.c |    3 +++
 drivers/media/platform/m2m-deinterlace.c     |    3 +++
 drivers/media/platform/mem2mem_testdev.c     |    3 +++
 drivers/media/platform/mx2_emmaprp.c         |    5 +++++
 drivers/media/platform/s5p-g2d/g2d.c         |    3 +++
 drivers/media/platform/s5p-jpeg/jpeg-core.c  |    3 +++
 drivers/media/platform/s5p-mfc/s5p_mfc.c     |    5 +++++
 drivers/media/platform/ti-vpe/vpe.c          |    2 ++
 10 files changed, 34 insertions(+)

diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c
index 61f3dbc..fe6dee6 100644
--- a/drivers/media/platform/coda.c
+++ b/drivers/media/platform/coda.c
@@ -2829,6 +2829,9 @@ static void coda_finish_encode(struct coda_ctx *ctx)
 	}
 
 	dst_buf->v4l2_buf.timestamp = src_buf->v4l2_buf.timestamp;
+	dst_buf->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	dst_buf->v4l2_buf.flags |=
+		src_buf->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 	dst_buf->v4l2_buf.timecode = src_buf->v4l2_buf.timecode;
 
 	v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 62c84d5..4260ea5 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -90,6 +90,10 @@ void gsc_m2m_job_finish(struct gsc_ctx *ctx, int vb_state)
 	if (src_vb && dst_vb) {
 		dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
 		dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;
+		dst_vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+		dst_vb->v4l2_buf.flags |=
+			src_vb->v4l2_buf.flags
+			& V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 
 		v4l2_m2m_buf_done(src_vb, vb_state);
 		v4l2_m2m_buf_done(dst_vb, vb_state);
diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c b/drivers/media/platform/exynos4-is/fimc-m2m.c
index 9da95bd..a4249a1 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -134,6 +134,9 @@ static void fimc_device_run(void *priv)
 		goto dma_unlock;
 
 	dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
+	dst_vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	dst_vb->v4l2_buf.flags |=
+		src_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 
 	/* Reconfigure hardware if the context has changed. */
 	if (fimc->m2m.ctx != ctx) {
diff --git a/drivers/media/platform/m2m-deinterlace.c b/drivers/media/platform/m2m-deinterlace.c
index 1f272d3..79ffdab 100644
--- a/drivers/media/platform/m2m-deinterlace.c
+++ b/drivers/media/platform/m2m-deinterlace.c
@@ -208,6 +208,9 @@ static void dma_callback(void *data)
 	dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->m2m_ctx);
 
 	dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
+	dst_vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	dst_vb->v4l2_buf.flags |=
+		src_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 	dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;
 
 	v4l2_m2m_buf_done(src_vb, VB2_BUF_STATE_DONE);
diff --git a/drivers/media/platform/mem2mem_testdev.c b/drivers/media/platform/mem2mem_testdev.c
index 08e2437..b91da7f 100644
--- a/drivers/media/platform/mem2mem_testdev.c
+++ b/drivers/media/platform/mem2mem_testdev.c
@@ -239,6 +239,9 @@ static int device_process(struct m2mtest_ctx *ctx,
 	memcpy(&out_vb->v4l2_buf.timestamp,
 			&in_vb->v4l2_buf.timestamp,
 			sizeof(struct timeval));
+	out_vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	out_vb->v4l2_buf.flags |=
+		in_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 
 	switch (ctx->mode) {
 	case MEM2MEM_HFLIP | MEM2MEM_VFLIP:
diff --git a/drivers/media/platform/mx2_emmaprp.c b/drivers/media/platform/mx2_emmaprp.c
index 91056ac0..0f59082 100644
--- a/drivers/media/platform/mx2_emmaprp.c
+++ b/drivers/media/platform/mx2_emmaprp.c
@@ -378,6 +378,11 @@ static irqreturn_t emmaprp_irq(int irq_emma, void *data)
 			dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->m2m_ctx);
 
 			dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
+			dst_vb->v4l2_buf.flags &=
+				~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+			dst_vb->v4l2_buf.flags |=
+				src_vb->v4l2_buf.flags
+				& V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 			dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;
 
 			spin_lock_irqsave(&pcdev->irqlock, flags);
diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
index 0fcf7d7..48fe6ea 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -560,6 +560,9 @@ static irqreturn_t g2d_isr(int irq, void *prv)
 
 	dst->v4l2_buf.timecode = src->v4l2_buf.timecode;
 	dst->v4l2_buf.timestamp = src->v4l2_buf.timestamp;
+	dst->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	dst->v4l2_buf.flags |=
+		src->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 
 	v4l2_m2m_buf_done(src, VB2_BUF_STATE_DONE);
 	v4l2_m2m_buf_done(dst, VB2_BUF_STATE_DONE);
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index a1c78c8..7b10120 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1766,6 +1766,9 @@ static irqreturn_t s5p_jpeg_irq(int irq, void *dev_id)
 
 	dst_buf->v4l2_buf.timecode = src_buf->v4l2_buf.timecode;
 	dst_buf->v4l2_buf.timestamp = src_buf->v4l2_buf.timestamp;
+	dst_buf->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	dst_buf->v4l2_buf.flags |=
+		src_buf->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 
 	v4l2_m2m_buf_done(src_buf, state);
 	if (curr_ctx->mode == S5P_JPEG_ENCODE)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index e2aac59..702ca1b 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -232,6 +232,11 @@ static void s5p_mfc_handle_frame_copy_time(struct s5p_mfc_ctx *ctx)
 						src_buf->b->v4l2_buf.timecode;
 			dst_buf->b->v4l2_buf.timestamp =
 						src_buf->b->v4l2_buf.timestamp;
+			dst_buf->b->v4l2_buf.flags &=
+				~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+			dst_buf->b->v4l2_buf.flags |=
+				src_buf->b->v4l2_buf.flags
+				& V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 			switch (frame_type) {
 			case S5P_FIMV_DECODE_FRAME_I_FRAME:
 				dst_buf->b->v4l2_buf.flags |=
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 1296c53..d67c467 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1278,6 +1278,8 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
 	d_buf = &d_vb->v4l2_buf;
 
 	d_buf->timestamp = s_buf->timestamp;
+	d_buf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+	d_buf->flags |= s_buf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
 	if (s_buf->flags & V4L2_BUF_FLAG_TIMECODE) {
 		d_buf->flags |= V4L2_BUF_FLAG_TIMECODE;
 		d_buf->timecode = s_buf->timecode;
-- 
1.7.10.4

[PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Sakari Ailus]

Timestamp buffer flags are constant at the moment. Document them so that 1)
they're always valid and 2) not changed by the drivers. This leaves room to
extend the functionality later on if needed.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index fbd0c6e..4f76565 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
 In that case, struct <structname>v4l2_buffer</structname> contains an array of
 plane structures.</para>
 
+    <para>Dequeued video buffers come with timestamps. These
+    timestamps can be taken from different clocks and at different
+    part of the frame, depending on the driver. Please see flags in
+    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
+    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
+    linkend="buffer-flags">. These flags are guaranteed to be always
+    valid and will not be changed by the driver autonomously. Changes
+    in these flags may take place due as a side effect of
+    &VIDIOC-S-INPUT; or &VIDIOC-S-OUTPUT; however.</para>
+
     <table frame="none" pgwide="1" id="v4l2-buffer">
       <title>struct <structname>v4l2_buffer</structname></title>
       <tgroup cols="4">
-- 
1.7.10.4

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Hans Verkuil]

On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> Timestamp buffer flags are constant at the moment. Document them so that 1)
> they're always valid and 2) not changed by the drivers. This leaves room to
> extend the functionality later on if needed.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index fbd0c6e..4f76565 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
>  In that case, struct <structname>v4l2_buffer</structname> contains an array of
>  plane structures.</para>
>  
> +    <para>Dequeued video buffers come with timestamps. These
> +    timestamps can be taken from different clocks and at different
> +    part of the frame, depending on the driver. Please see flags in

s/part/parts/

But I think I would write it somewhat differently:

"The driver decides at which part of the frame and with which clock
the timestamp is taken."

> +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
> +    linkend="buffer-flags">. These flags are guaranteed to be always
> +    valid and will not be changed by the driver autonomously. Changes
> +    in these flags may take place due as a side effect of

s/due//

> +    &VIDIOC-S-INPUT; or &VIDIOC-S-OUTPUT; however.</para>
> +
>      <table frame="none" pgwide="1" id="v4l2-buffer">
>        <title>struct <structname>v4l2_buffer</structname></title>
>        <tgroup cols="4">
> 

Regards,

	Hans

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Sakari Ailus]

Hi Hans,

On Sat, Feb 15, 2014 at 10:03:07PM +0100, Hans Verkuil wrote:
> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> > Timestamp buffer flags are constant at the moment. Document them so that 1)
> > they're always valid and 2) not changed by the drivers. This leaves room to
> > extend the functionality later on if needed.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> > index fbd0c6e..4f76565 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
> >  In that case, struct <structname>v4l2_buffer</structname> contains an array of
> >  plane structures.</para>
> >  
> > +    <para>Dequeued video buffers come with timestamps. These
> > +    timestamps can be taken from different clocks and at different
> > +    part of the frame, depending on the driver. Please see flags in
> 
> s/part/parts/
> 
> But I think I would write it somewhat differently:
> 
> "The driver decides at which part of the frame and with which clock
> the timestamp is taken."

I'll fix both for the next version.

> > +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
> > +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
> > +    linkend="buffer-flags">. These flags are guaranteed to be always
> > +    valid and will not be changed by the driver autonomously. Changes
> > +    in these flags may take place due as a side effect of
> 
> s/due//

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v5 4/7] uvcvideo: Tell the user space we're using start-of-exposure timestamps

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Saturday 15 February 2014 22:53:02 Sakari Ailus wrote:
> The UVC device provided timestamps are taken from the clock once the
> exposure of the frame has begun, not when the reception of the frame would
> have been finished as almost anywhere else. Show this to the user space by
> using V4L2_BUF_FLAG_TSTAMP_SRC_SOE buffer flag.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Strictly speaking that's not entirely true, as some devices don't bother 
reporting a hardware timestamp. However, in practice, most devices should 
behave correctly, so that flag is definitely better.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_queue.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c index cd962be..a9292d2 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -149,7 +149,8 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum
> v4l2_buf_type type, 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_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +	queue->queue.timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> +		| V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
>  	ret = vb2_queue_init(&queue->queue);
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Laurent Pinchart]

Hello,

On Saturday 15 February 2014 22:03:07 Hans Verkuil wrote:
> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> > Timestamp buffer flags are constant at the moment. Document them so that
> > 1) they're always valid and 2) not changed by the drivers. This leaves
> > room to extend the functionality later on if needed.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> > 
> >  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/io.xml
> > b/Documentation/DocBook/media/v4l/io.xml index fbd0c6e..4f76565 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -653,6 +653,16 @@ plane, are stored in struct
> > <structname>v4l2_plane</structname> instead.> 
> >  In that case, struct <structname>v4l2_buffer</structname> contains an
> >  array of plane structures.</para>
> > 
> > +    <para>Dequeued video buffers come with timestamps. These
> > +    timestamps can be taken from different clocks and at different
> > +    part of the frame, depending on the driver. Please see flags in
> 
> s/part/parts/
> 
> But I think I would write it somewhat differently:
> 
> "The driver decides at which part of the frame and with which clock
> the timestamp is taken."
> 
> > +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
> > +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
> > +    linkend="buffer-flags">. These flags are guaranteed to be always
> > +    valid and will not be changed by the driver autonomously.

This sentence sounds a bit confusing to me. What about

"These flags are always valid and are constant across all buffers during the 
whole video stream."

> > Changes
> > +    in these flags may take place due as a side effect of
> 
> s/due//
> 
> > +    &VIDIOC-S-INPUT; or &VIDIOC-S-OUTPUT; however.</para>
> > +
> >      <table frame="none" pgwide="1" id="v4l2-buffer">
> >        <title>struct <structname>v4l2_buffer</structname></title>
> >        <tgroup cols="4">

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Hans Verkuil]

On 02/17/2014 01:56 AM, Laurent Pinchart wrote:
> Hello,
> 
> On Saturday 15 February 2014 22:03:07 Hans Verkuil wrote:
>> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
>>> Timestamp buffer flags are constant at the moment. Document them so that
>>> 1) they're always valid and 2) not changed by the drivers. This leaves
>>> room to extend the functionality later on if needed.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>> ---
>>>
>>>  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/DocBook/media/v4l/io.xml
>>> b/Documentation/DocBook/media/v4l/io.xml index fbd0c6e..4f76565 100644
>>> --- a/Documentation/DocBook/media/v4l/io.xml
>>> +++ b/Documentation/DocBook/media/v4l/io.xml
>>> @@ -653,6 +653,16 @@ plane, are stored in struct
>>> <structname>v4l2_plane</structname> instead.> 
>>>  In that case, struct <structname>v4l2_buffer</structname> contains an
>>>  array of plane structures.</para>
>>>
>>> +    <para>Dequeued video buffers come with timestamps. These
>>> +    timestamps can be taken from different clocks and at different
>>> +    part of the frame, depending on the driver. Please see flags in
>>
>> s/part/parts/
>>
>> But I think I would write it somewhat differently:
>>
>> "The driver decides at which part of the frame and with which clock
>> the timestamp is taken."
>>
>>> +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
>>> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
>>> +    linkend="buffer-flags">. These flags are guaranteed to be always
>>> +    valid and will not be changed by the driver autonomously.
> 
> This sentence sounds a bit confusing to me. What about
> 
> "These flags are always valid and are constant across all buffers during the 
> whole video stream."

I like this.

Regards,

	Hans

Re: [PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

[Hans Verkuil]

On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> The buffer flags field is 32 bits but the defined only used 16. This is
> fine, but as more than 16 bits will be used in the very near future, define
> them as 32-bit numbers for consistency.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

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

Regards,

	Hans

> ---
>  Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++-------------
>  include/uapi/linux/videodev2.h         |   38 +++++++++++++++++++-------------
>  2 files changed, 38 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 8facac4..46d24b3 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -984,7 +984,7 @@ should set this to 0.</entry>
>  	<tbody valign="top">
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_MAPPED</constant></entry>
> -	    <entry>0x0001</entry>
> +	    <entry>0x00000001</entry>
>  	    <entry>The buffer resides in device memory and has been mapped
>  into the application's address space, see <xref linkend="mmap" /> for details.
>  Drivers set or clear this flag when the
> @@ -994,7 +994,7 @@ Drivers set or clear this flag when the
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_QUEUED</constant></entry>
> -	    <entry>0x0002</entry>
> +	    <entry>0x00000002</entry>
>  	  <entry>Internally drivers maintain two buffer queues, an
>  incoming and outgoing queue. When this flag is set, the buffer is
>  currently on the incoming queue. It automatically moves to the
> @@ -1007,7 +1007,7 @@ cleared.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_DONE</constant></entry>
> -	    <entry>0x0004</entry>
> +	    <entry>0x00000004</entry>
>  	    <entry>When this flag is set, the buffer is currently on
>  the outgoing queue, ready to be dequeued from the driver. Drivers set
>  or clear this flag when the <constant>VIDIOC_QUERYBUF</constant> ioctl
> @@ -1021,7 +1021,7 @@ state, in the application domain to say so.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_ERROR</constant></entry>
> -	    <entry>0x0040</entry>
> +	    <entry>0x00000040</entry>
>  	    <entry>When this flag is set, the buffer has been dequeued
>  	    successfully, although the data might have been corrupted.
>  	    This is recoverable, streaming may continue as normal and
> @@ -1031,7 +1031,7 @@ state, in the application domain to say so.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_KEYFRAME</constant></entry>
> -	    <entry>0x0008</entry>
> +	    <entry>0x00000008</entry>
>  	  <entry>Drivers set or clear this flag when calling the
>  <constant>VIDIOC_DQBUF</constant> ioctl. It may be set by video
>  capture devices when the buffer contains a compressed image which is a
> @@ -1039,27 +1039,27 @@ key frame (or field), &ie; can be decompressed on its own.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_PFRAME</constant></entry>
> -	    <entry>0x0010</entry>
> +	    <entry>0x00000010</entry>
>  	    <entry>Similar to <constant>V4L2_BUF_FLAG_KEYFRAME</constant>
>  this flags predicted frames or fields which contain only differences to a
>  previous key frame.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_BFRAME</constant></entry>
> -	    <entry>0x0020</entry>
> +	    <entry>0x00000020</entry>
>  	    <entry>Similar to <constant>V4L2_BUF_FLAG_PFRAME</constant>
>  	this is a bidirectional predicted frame or field. [ooc tbd]</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_TIMECODE</constant></entry>
> -	    <entry>0x0100</entry>
> +	    <entry>0x00000100</entry>
>  	    <entry>The <structfield>timecode</structfield> field is valid.
>  Drivers set or clear this flag when the <constant>VIDIOC_DQBUF</constant>
>  ioctl is called.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_PREPARED</constant></entry>
> -	    <entry>0x0400</entry>
> +	    <entry>0x00000400</entry>
>  	    <entry>The buffer has been prepared for I/O and can be queued by the
>  application. Drivers set or clear this flag when the
>  <link linkend="vidioc-querybuf">VIDIOC_QUERYBUF</link>, <link
> @@ -1069,7 +1069,7 @@ application. Drivers set or clear this flag when the
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant></entry>
> -	    <entry>0x0800</entry>
> +	    <entry>0x00000800</entry>
>  	    <entry>Caches do not have to be invalidated for this buffer.
>  Typically applications shall use this flag if the data captured in the buffer
>  is not going to be touched by the CPU, instead the buffer will, probably, be
> @@ -1078,7 +1078,7 @@ passed on to a DMA-capable hardware unit for further processing or output.
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant></entry>
> -	    <entry>0x1000</entry>
> +	    <entry>0x00001000</entry>
>  	    <entry>Caches do not have to be cleaned for this buffer.
>  Typically applications shall use this flag for output buffers if the data
>  in this buffer has not been created by the CPU but by some DMA-capable unit,
> @@ -1086,7 +1086,7 @@ in which case caches have not been used.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant></entry>
> -	    <entry>0xe000</entry>
> +	    <entry>0x0000e000</entry>
>  	    <entry>Mask for timestamp types below. To test the
>  	    timestamp type, mask out bits not belonging to timestamp
>  	    type by performing a logical and operation with buffer
> @@ -1094,7 +1094,7 @@ in which case caches have not been used.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN</constant></entry>
> -	    <entry>0x0000</entry>
> +	    <entry>0x00000000</entry>
>  	    <entry>Unknown timestamp type. This type is used by
>  	    drivers before Linux 3.9 and may be either monotonic (see
>  	    below) or realtime (wall clock). Monotonic clock has been
> @@ -1107,7 +1107,7 @@ in which case caches have not been used.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC</constant></entry>
> -	    <entry>0x2000</entry>
> +	    <entry>0x00002000</entry>
>  	    <entry>The buffer timestamp has been taken from the
>  	    <constant>CLOCK_MONOTONIC</constant> clock. To access the
>  	    same clock outside V4L2, use
> @@ -1115,7 +1115,7 @@ in which case caches have not been used.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
> -	    <entry>0x4000</entry>
> +	    <entry>0x00004000</entry>
>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>  	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
>  	  </row>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 6ae7bbe..e9ee444 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -669,24 +669,32 @@ struct v4l2_buffer {
>  };
>  
>  /*  Flags for 'flags' field */
> -#define V4L2_BUF_FLAG_MAPPED	0x0001  /* Buffer is mapped (flag) */
> -#define V4L2_BUF_FLAG_QUEUED	0x0002	/* Buffer is queued for processing */
> -#define V4L2_BUF_FLAG_DONE	0x0004	/* Buffer is ready */
> -#define V4L2_BUF_FLAG_KEYFRAME	0x0008	/* Image is a keyframe (I-frame) */
> -#define V4L2_BUF_FLAG_PFRAME	0x0010	/* Image is a P-frame */
> -#define V4L2_BUF_FLAG_BFRAME	0x0020	/* Image is a B-frame */
> +/* Buffer is mapped (flag) */
> +#define V4L2_BUF_FLAG_MAPPED			0x00000001
> +/* Buffer is queued for processing */
> +#define V4L2_BUF_FLAG_QUEUED			0x00000002
> +/* Buffer is ready */
> +#define V4L2_BUF_FLAG_DONE			0x00000004
> +/* Image is a keyframe (I-frame) */
> +#define V4L2_BUF_FLAG_KEYFRAME			0x00000008
> +/* Image is a P-frame */
> +#define V4L2_BUF_FLAG_PFRAME			0x00000010
> +/* Image is a B-frame */
> +#define V4L2_BUF_FLAG_BFRAME			0x00000020
>  /* Buffer is ready, but the data contained within is corrupted. */
> -#define V4L2_BUF_FLAG_ERROR	0x0040
> -#define V4L2_BUF_FLAG_TIMECODE	0x0100	/* timecode field is valid */
> -#define V4L2_BUF_FLAG_PREPARED	0x0400	/* Buffer is prepared for queuing */
> +#define V4L2_BUF_FLAG_ERROR			0x00000040
> +/* timecode field is valid */
> +#define V4L2_BUF_FLAG_TIMECODE			0x00000100
> +/* Buffer is prepared for queuing */
> +#define V4L2_BUF_FLAG_PREPARED			0x00000400
>  /* Cache handling flags */
> -#define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x0800
> -#define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x1000
> +#define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x00000800
> +#define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x00001000
>  /* Timestamp type */
> -#define V4L2_BUF_FLAG_TIMESTAMP_MASK		0xe000
> -#define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x0000
> -#define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x2000
> -#define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x4000
> +#define V4L2_BUF_FLAG_TIMESTAMP_MASK		0x0000e000
> +#define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
> +#define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
> +#define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
>  
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
> 

Re: [PATCH v5 3/7] v4l: Add timestamp source flags, mask and document them

[Hans Verkuil]

Hi Sakari,

Some minor comments:

On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> Some devices do not produce timestamps that correspond to the end of the
> frame. The user space should be informed on the matter. This patch achieves
> that by adding buffer flags (and a mask) for timestamp sources since more
> possible timestamping points are expected than just two.
> 
> A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
> eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end of
> frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure (next
> value).
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  Documentation/DocBook/media/v4l/io.xml   |   31 ++++++++++++++++++++++++------
>  drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>  include/media/videobuf2-core.h           |    2 ++
>  include/uapi/linux/videodev2.h           |    4 ++++
>  4 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 46d24b3..fbd0c6e 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -653,12 +653,6 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
>  In that case, struct <structname>v4l2_buffer</structname> contains an array of
>  plane structures.</para>
>  
> -      <para>For timestamp types that are sampled from the system clock
> -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp is
> -taken after the complete frame has been received (or transmitted in
> -case of video output devices). For other kinds of
> -timestamps this may vary depending on the driver.</para>
> -
>      <table frame="none" pgwide="1" id="v4l2-buffer">
>        <title>struct <structname>v4l2_buffer</structname></title>
>        <tgroup cols="4">
> @@ -1119,6 +1113,31 @@ in which case caches have not been used.</entry>
>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>  	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
>  	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
> +	    <entry>0x00070000</entry>
> +	    <entry>Mask for timestamp sources below. The timestamp source
> +	    defines the point of time the timestamp is taken in relation to
> +	    the frame. Logical and operation between the
> +	    <structfield>flags</structfield> field and
> +	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
> +	    value of the timestamp source.</entry>
> +	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
> +	    <entry>0x00000000</entry>
> +	    <entry>"End of frame." The buffer timestamp has been taken

More a typographical thing than anything else: I prefer this:

"End Of Frame": the buffer...

The capitalization links back to the EOF abbreviation more directly.

> +	    when the last pixel of the frame has been received or the

I would say: "after the last pixel of the frame has been received or after the"

The "when" word suggests that it is exactly "when", which is not true in
practice.

> +	    last pixel of the frame has been transmitted.</entry>
> +	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
> +	    <entry>0x00010000</entry>
> +	    <entry>"Start of exposure." The buffer timestamp has been taken

Start Of Exposure: the buffer...

> +	    when the exposure of the frame has begun. This is only

Should 'when' be replaced with 'after' here as well? I think Laurent has to decide
that.

Regards,

	Hans

> +	    valid for buffer type
> +	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
> +	  </row>
>  	</tbody>
>        </tgroup>
>      </table>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 5a5fb7f..6e314b0 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2195,7 +2195,9 @@ int vb2_queue_init(struct vb2_queue *q)
>  	    WARN_ON(!q->io_modes)	  ||
>  	    WARN_ON(!q->ops->queue_setup) ||
>  	    WARN_ON(!q->ops->buf_queue)   ||
> -	    WARN_ON(q->timestamp_type & ~V4L2_BUF_FLAG_TIMESTAMP_MASK))
> +	    WARN_ON(q->timestamp_type &
> +		    ~(V4L2_BUF_FLAG_TIMESTAMP_MASK |
> +		      V4L2_BUF_FLAG_TSTAMP_SRC_MASK)))
>  		return -EINVAL;
>  
>  	/* Warn that the driver should choose an appropriate timestamp type */
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index bef53ce..b6b992d 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -312,6 +312,8 @@ struct v4l2_fh;
>   * @buf_struct_size: size of the driver-specific buffer structure;
>   *		"0" indicates the driver doesn't want to use a custom buffer
>   *		structure type, so sizeof(struct vb2_buffer) will is used
> + * @timestamp_type: Timestamp flags; V4L2_BUF_FLAGS_TIMESTAMP_* and
> + *		V4L2_BUF_FLAGS_TSTAMP_SRC_*
>   * @gfp_flags:	additional gfp flags used when allocating the buffers.
>   *		Typically this is 0, but it may be e.g. GFP_DMA or __GFP_DMA32
>   *		to force the buffer allocation to a specific memory zone.
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index e9ee444..82e8661 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -695,6 +695,10 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
>  #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
>  #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
> +/* Timestamp sources. */
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
>  
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
> 

Re: [PATCH v5 3/7] v4l: Add timestamp source flags, mask and document them

[Sakari Ailus]

Hi Hans,

Thanks for your comments.

On Mon, Feb 17, 2014 at 09:54:53AM +0100, Hans Verkuil wrote:
...
> > @@ -1119,6 +1113,31 @@ in which case caches have not been used.</entry>
> >  	    <entry>The CAPTURE buffer timestamp has been taken from the
> >  	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
> >  	  </row>
> > +	  <row>
> > +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
> > +	    <entry>0x00070000</entry>
> > +	    <entry>Mask for timestamp sources below. The timestamp source
> > +	    defines the point of time the timestamp is taken in relation to
> > +	    the frame. Logical and operation between the
> > +	    <structfield>flags</structfield> field and
> > +	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
> > +	    value of the timestamp source.</entry>
> > +	  </row>
> > +	  <row>
> > +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
> > +	    <entry>0x00000000</entry>
> > +	    <entry>"End of frame." The buffer timestamp has been taken
> 
> More a typographical thing than anything else: I prefer this:
> 
> "End Of Frame": the buffer...
> 
> The capitalization links back to the EOF abbreviation more directly.

Fixed, same for the similar one below.

> > +	    when the last pixel of the frame has been received or the
> 
> I would say: "after the last pixel of the frame has been received or after the"
> 
> The "when" word suggests that it is exactly "when", which is not true in
> practice.

That's the intent nonetheless: to take the timestamp at the end of the
frame, not an unspecified time after the event has taken place. I'd rather
add a note that there's a level of impreciseness in taking the timestamp,
such as:

"In practice, software generated timestamp will typically be read from the
clock a small amount of time after the last pixel has been received,
depending on the system and other activity in it." That would probably be
best put somewhere else in the document, though.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Sakari Ailus]

On Mon, Feb 17, 2014 at 09:43:22AM +0100, Hans Verkuil wrote:
> >>> +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
> >>> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
> >>> +    linkend="buffer-flags">. These flags are guaranteed to be always
> >>> +    valid and will not be changed by the driver autonomously.
> > 
> > This sentence sounds a bit confusing to me. What about
> > 
> > "These flags are always valid and are constant across all buffers during the 
> > whole video stream."
> 
> I like this.

I'll put that to the next version.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Sakari Ailus]

On Tue, Feb 18, 2014 at 01:32:03AM +0200, Sakari Ailus wrote:
> On Mon, Feb 17, 2014 at 09:43:22AM +0100, Hans Verkuil wrote:
> > >>> +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
> > >>> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
> > >>> +    linkend="buffer-flags">. These flags are guaranteed to be always
> > >>> +    valid and will not be changed by the driver autonomously.
> > > 
> > > This sentence sounds a bit confusing to me. What about
> > > 
> > > "These flags are always valid and are constant across all buffers during the 
> > > whole video stream."
> > 
> > I like this.
> 
> I'll put that to the next version.

Well, after removing the second "are ". :-)

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Sakari Ailus]

Some devices do not produce timestamps that correspond to the end of the
frame. The user space should be informed on the matter. This patch achieves
that by adding buffer flags (and a mask) for timestamp sources since more
possible timestamping points are expected than just two.

A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end of
frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure (next
value).

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
since v5:
- Add a note on software generated timestamp inaccuracy.

 Documentation/DocBook/media/v4l/io.xml   |   38 +++++++++++++++++++++++++-----
 drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
 include/media/videobuf2-core.h           |    2 ++
 include/uapi/linux/videodev2.h           |    4 ++++
 4 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 46d24b3..22b87bc 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -653,12 +653,6 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
 In that case, struct <structname>v4l2_buffer</structname> contains an array of
 plane structures.</para>
 
-      <para>For timestamp types that are sampled from the system clock
-(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp is
-taken after the complete frame has been received (or transmitted in
-case of video output devices). For other kinds of
-timestamps this may vary depending on the driver.</para>
-
     <table frame="none" pgwide="1" id="v4l2-buffer">
       <title>struct <structname>v4l2_buffer</structname></title>
       <tgroup cols="4">
@@ -1119,6 +1113,38 @@ in which case caches have not been used.</entry>
 	    <entry>The CAPTURE buffer timestamp has been taken from the
 	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
 	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
+	    <entry>0x00070000</entry>
+	    <entry>Mask for timestamp sources below. The timestamp source
+	    defines the point of time the timestamp is taken in relation to
+	    the frame. Logical and operation between the
+	    <structfield>flags</structfield> field and
+	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
+	    value of the timestamp source.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
+	    <entry>0x00000000</entry>
+	    <entry>End Of Frame. The buffer timestamp has been taken
+	    when the last pixel of the frame has been received or the
+	    last pixel of the frame has been transmitted. In practice,
+	    software generated timestamps will typically be read from
+	    the clock a small amount of time after the last pixel has
+	    been received, depending on the system and other activity
+	    in it.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
+	    <entry>0x00010000</entry>
+	    <entry>Start Of Exposure. The buffer timestamp has been
+	    taken when the exposure of the frame has begun. In
+	    practice, software generated timestamps will typically be
+	    read from the clock a small amount of time after the last
+	    pixel has been received, depending on the system and other
+	    activity in it. This is only valid for buffer type
+	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 5a5fb7f..6e314b0 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2195,7 +2195,9 @@ int vb2_queue_init(struct vb2_queue *q)
 	    WARN_ON(!q->io_modes)	  ||
 	    WARN_ON(!q->ops->queue_setup) ||
 	    WARN_ON(!q->ops->buf_queue)   ||
-	    WARN_ON(q->timestamp_type & ~V4L2_BUF_FLAG_TIMESTAMP_MASK))
+	    WARN_ON(q->timestamp_type &
+		    ~(V4L2_BUF_FLAG_TIMESTAMP_MASK |
+		      V4L2_BUF_FLAG_TSTAMP_SRC_MASK)))
 		return -EINVAL;
 
 	/* Warn that the driver should choose an appropriate timestamp type */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bef53ce..b6b992d 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -312,6 +312,8 @@ struct v4l2_fh;
  * @buf_struct_size: size of the driver-specific buffer structure;
  *		"0" indicates the driver doesn't want to use a custom buffer
  *		structure type, so sizeof(struct vb2_buffer) will is used
+ * @timestamp_type: Timestamp flags; V4L2_BUF_FLAGS_TIMESTAMP_* and
+ *		V4L2_BUF_FLAGS_TSTAMP_SRC_*
  * @gfp_flags:	additional gfp flags used when allocating the buffers.
  *		Typically this is 0, but it may be e.g. GFP_DMA or __GFP_DMA32
  *		to force the buffer allocation to a specific memory zone.
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index e9ee444..82e8661 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -695,6 +695,10 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
 #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
 #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
+/* Timestamp sources. */
+#define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
+#define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
+#define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
 
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
-- 
1.7.10.4

[PATCH v5.1 7/7] v4l: Document timestamp buffer flag behaviour

[Sakari Ailus]

Timestamp buffer flags are constant at the moment. Document them so that 1)
they're always valid and 2) not changed by the drivers. This leaves room to
extend the functionality later on if needed.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
since v5:
- Clarify timestamp source flag behaviour.

 Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 22b87bc..a69e12a 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
 In that case, struct <structname>v4l2_buffer</structname> contains an array of
 plane structures.</para>
 
+    <para>Dequeued video buffers come with timestamps. The driver
+    decides at which part of the frame and with which clock the
+    timestamp is taken. Please see flags in the masks
+    <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
+    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
+    linkend="buffer-flags">. These flags are always valid and constant
+    across all buffers during the whole video stream. Changes in these
+    flags may take place as a side effect of &VIDIOC-S-INPUT; or
+    &VIDIOC-S-OUTPUT; however.</para>
+
     <table frame="none" pgwide="1" id="v4l2-buffer">
       <title>struct <structname>v4l2_buffer</structname></title>
       <tgroup cols="4">
-- 
1.7.10.4

Re: [PATCH v5.1 7/7] v4l: Document timestamp buffer flag behaviour

[Hans Verkuil]

On 02/20/2014 08:42 PM, Sakari Ailus wrote:
> Timestamp buffer flags are constant at the moment. Document them so that 1)
> they're always valid and 2) not changed by the drivers. This leaves room to
> extend the functionality later on if needed.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

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

> ---
> since v5:
> - Clarify timestamp source flag behaviour.
> 
>  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 22b87bc..a69e12a 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
>  In that case, struct <structname>v4l2_buffer</structname> contains an array of
>  plane structures.</para>
>  
> +    <para>Dequeued video buffers come with timestamps. The driver
> +    decides at which part of the frame and with which clock the
> +    timestamp is taken. Please see flags in the masks
> +    <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
> +    linkend="buffer-flags">. These flags are always valid and constant
> +    across all buffers during the whole video stream. Changes in these
> +    flags may take place as a side effect of &VIDIOC-S-INPUT; or
> +    &VIDIOC-S-OUTPUT; however.</para>
> +
>      <table frame="none" pgwide="1" id="v4l2-buffer">
>        <title>struct <structname>v4l2_buffer</structname></title>
>        <tgroup cols="4">
> 

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Hans Verkuil]

On 02/20/2014 08:41 PM, Sakari Ailus wrote:
> Some devices do not produce timestamps that correspond to the end of the
> frame. The user space should be informed on the matter. This patch achieves
> that by adding buffer flags (and a mask) for timestamp sources since more
> possible timestamping points are expected than just two.
> 
> A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
> eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end of
> frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure (next
> value).

Sorry, but I still have two small notes:

> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
> since v5:
> - Add a note on software generated timestamp inaccuracy.
> 
>  Documentation/DocBook/media/v4l/io.xml   |   38 +++++++++++++++++++++++++-----
>  drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>  include/media/videobuf2-core.h           |    2 ++
>  include/uapi/linux/videodev2.h           |    4 ++++
>  4 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 46d24b3..22b87bc 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -653,12 +653,6 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
>  In that case, struct <structname>v4l2_buffer</structname> contains an array of
>  plane structures.</para>
>  
> -      <para>For timestamp types that are sampled from the system clock
> -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp is
> -taken after the complete frame has been received (or transmitted in
> -case of video output devices). For other kinds of
> -timestamps this may vary depending on the driver.</para>
> -
>      <table frame="none" pgwide="1" id="v4l2-buffer">
>        <title>struct <structname>v4l2_buffer</structname></title>
>        <tgroup cols="4">
> @@ -1119,6 +1113,38 @@ in which case caches have not been used.</entry>
>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>  	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
>  	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
> +	    <entry>0x00070000</entry>
> +	    <entry>Mask for timestamp sources below. The timestamp source
> +	    defines the point of time the timestamp is taken in relation to
> +	    the frame. Logical and operation between the
> +	    <structfield>flags</structfield> field and
> +	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
> +	    value of the timestamp source.</entry>
> +	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
> +	    <entry>0x00000000</entry>
> +	    <entry>End Of Frame. The buffer timestamp has been taken
> +	    when the last pixel of the frame has been received or the
> +	    last pixel of the frame has been transmitted. In practice,
> +	    software generated timestamps will typically be read from
> +	    the clock a small amount of time after the last pixel has
> +	    been received, depending on the system and other activity

s/been received/been received or transmitted/

> +	    in it.</entry>
> +	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
> +	    <entry>0x00010000</entry>
> +	    <entry>Start Of Exposure. The buffer timestamp has been
> +	    taken when the exposure of the frame has begun. In
> +	    practice, software generated timestamps will typically be
> +	    read from the clock a small amount of time after the last
> +	    pixel has been received, depending on the system and other
> +	    activity in it. This is only valid for buffer type
> +	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>

I would move the last sentence up to just before "In practice...". The
way it is now it looks like an afterthought.

I am also not sure whether the whole "In practice" sentence is valid
here. Certainly the bit about "the last pixel" isn't since this is the
"SOE" case and not the End Of Frame. In the case of the UVC driver (and that's
the only one using this timestamp source) the timestamps come from the
hardware as I understand it, so the "software generated" bit doesn't
apply.

I would actually be inclined to drop it altogether for this particular
timestamp source. But it's up to Laurent.

Regards,

	Hans

> +	  </row>
>  	</tbody>
>        </tgroup>
>      </table>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 5a5fb7f..6e314b0 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2195,7 +2195,9 @@ int vb2_queue_init(struct vb2_queue *q)
>  	    WARN_ON(!q->io_modes)	  ||
>  	    WARN_ON(!q->ops->queue_setup) ||
>  	    WARN_ON(!q->ops->buf_queue)   ||
> -	    WARN_ON(q->timestamp_type & ~V4L2_BUF_FLAG_TIMESTAMP_MASK))
> +	    WARN_ON(q->timestamp_type &
> +		    ~(V4L2_BUF_FLAG_TIMESTAMP_MASK |
> +		      V4L2_BUF_FLAG_TSTAMP_SRC_MASK)))
>  		return -EINVAL;
>  
>  	/* Warn that the driver should choose an appropriate timestamp type */
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index bef53ce..b6b992d 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -312,6 +312,8 @@ struct v4l2_fh;
>   * @buf_struct_size: size of the driver-specific buffer structure;
>   *		"0" indicates the driver doesn't want to use a custom buffer
>   *		structure type, so sizeof(struct vb2_buffer) will is used
> + * @timestamp_type: Timestamp flags; V4L2_BUF_FLAGS_TIMESTAMP_* and
> + *		V4L2_BUF_FLAGS_TSTAMP_SRC_*
>   * @gfp_flags:	additional gfp flags used when allocating the buffers.
>   *		Typically this is 0, but it may be e.g. GFP_DMA or __GFP_DMA32
>   *		to force the buffer allocation to a specific memory zone.
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index e9ee444..82e8661 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -695,6 +695,10 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
>  #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
>  #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
> +/* Timestamp sources. */
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
>  
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
> 

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Sylwester Nawrocki]

Hi Sakari,

On 02/20/2014 09:36 PM, Hans Verkuil wrote:
> On 02/20/2014 08:41 PM, Sakari Ailus wrote:
>> Some devices do not produce timestamps that correspond to the end of the
>> frame. The user space should be informed on the matter. This patch achieves
>> that by adding buffer flags (and a mask) for timestamp sources since more
>> possible timestamping points are expected than just two.
>>
>> A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
>> eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end of
>> frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure (next
>> value).
>
> Sorry, but I still have two small notes:
>
>> Signed-off-by: Sakari Ailus<sakari.ailus@iki.fi>
>> ---
>> since v5:
>> - Add a note on software generated timestamp inaccuracy.
>>
>>   Documentation/DocBook/media/v4l/io.xml   |   38 +++++++++++++++++++++++++-----
>>   drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>>   include/media/videobuf2-core.h           |    2 ++
>>   include/uapi/linux/videodev2.h           |    4 ++++
>>   4 files changed, 41 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
>> index 46d24b3..22b87bc 100644
>> --- a/Documentation/DocBook/media/v4l/io.xml
>> +++ b/Documentation/DocBook/media/v4l/io.xml
>> @@ -653,12 +653,6 @@ plane, are stored in struct<structname>v4l2_plane</structname>  instead.
>>   In that case, struct<structname>v4l2_buffer</structname>  contains an array of
>>   plane structures.</para>
>>
>> -<para>For timestamp types that are sampled from the system clock
>> -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp is
>> -taken after the complete frame has been received (or transmitted in
>> -case of video output devices). For other kinds of
>> -timestamps this may vary depending on the driver.</para>
>> -
>>       <table frame="none" pgwide="1" id="v4l2-buffer">
>>         <title>struct<structname>v4l2_buffer</structname></title>
>>         <tgroup cols="4">
>> @@ -1119,6 +1113,38 @@ in which case caches have not been used.</entry>
>>   	<entry>The CAPTURE buffer timestamp has been taken from the
>>   	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
>>   	</row>
>> +	<row>
>> +	<entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
>> +	<entry>0x00070000</entry>
>> +	<entry>Mask for timestamp sources below. The timestamp source
>> +	    defines the point of time the timestamp is taken in relation to
>> +	    the frame. Logical and operation between the

Perhaps s/and/AND ?

>> +	<structfield>flags</structfield>  field and
>> +	<constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant>  produces the
>> +	    value of the timestamp source.</entry>
>> +	</row>
>> +	<row>
>> +	<entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
>> +	<entry>0x00000000</entry>
>> +	<entry>End Of Frame. The buffer timestamp has been taken
>> +	    when the last pixel of the frame has been received or the
>> +	    last pixel of the frame has been transmitted. In practice,
>> +	    software generated timestamps will typically be read from
>> +	    the clock a small amount of time after the last pixel has
>> +	    been received, depending on the system and other activity
>
> s/been received/been received or transmitted/
>
>> +	    in it.</entry>
>> +	</row>
>> +	<row>
>> +	<entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>

V4L2_BUF_FLAG_TSTAMP_SRC_SOF (Start Of Frame) wouldn't fit here ?

>> +	<entry>0x00010000</entry>
>> +	<entry>Start Of Exposure. The buffer timestamp has been
>> +	    taken when the exposure of the frame has begun. In
>> +	    practice, software generated timestamps will typically be
>> +	    read from the clock a small amount of time after the last
>> +	    pixel has been received, depending on the system and other
>> +	    activity in it. This is only valid for buffer type
>> +	<constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
>
> I would move the last sentence up to just before "In practice...". The
> way it is now it looks like an afterthought.
>
> I am also not sure whether the whole "In practice" sentence is valid
> here. Certainly the bit about "the last pixel" isn't since this is the
> "SOE" case and not the End Of Frame. In the case of the UVC driver (and that's
> the only one using this timestamp source) the timestamps come from the
> hardware as I understand it, so the "software generated" bit doesn't
> apply.

I agree, not it looks like a copy & paste from the "End Of Frame"
paragraph. I guess for SOE it should have been, e.g.

"...read from the clock a small amount of time after the _first_
     pixel has been received" ?

> I would actually be inclined to drop it altogether for this particular
> timestamp source. But it's up to Laurent.

Yup, the "a small amount of time" concept seems a bit vague here.
It's not clear how long period it could be and the tolerance would like
very across the hardware.

> Regards,
>
> 	Hans
>
>> +	</row>
>>   	</tbody>
>>         </tgroup>
>>       </table>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 5a5fb7f..6e314b0 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -2195,7 +2195,9 @@ int vb2_queue_init(struct vb2_queue *q)
>>   	    WARN_ON(!q->io_modes)	  ||
>>   	    WARN_ON(!q->ops->queue_setup) ||
>>   	    WARN_ON(!q->ops->buf_queue)   ||
>> -	    WARN_ON(q->timestamp_type&  ~V4L2_BUF_FLAG_TIMESTAMP_MASK))
>> +	    WARN_ON(q->timestamp_type&
>> +		    ~(V4L2_BUF_FLAG_TIMESTAMP_MASK |
>> +		      V4L2_BUF_FLAG_TSTAMP_SRC_MASK)))
>>   		return -EINVAL;
>>
>>   	/* Warn that the driver should choose an appropriate timestamp type */
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index bef53ce..b6b992d 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -312,6 +312,8 @@ struct v4l2_fh;
>>    * @buf_struct_size: size of the driver-specific buffer structure;
>>    *		"0" indicates the driver doesn't want to use a custom buffer
>>    *		structure type, so sizeof(struct vb2_buffer) will is used
>> + * @timestamp_type: Timestamp flags; V4L2_BUF_FLAGS_TIMESTAMP_* and

nit: s/Timestamp/timestamp ?

>> + *		V4L2_BUF_FLAGS_TSTAMP_SRC_*
>>    * @gfp_flags:	additional gfp flags used when allocating the buffers.
>>    *		Typically this is 0, but it may be e.g. GFP_DMA or __GFP_DMA32
>>    *		to force the buffer allocation to a specific memory zone.
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index e9ee444..82e8661 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -695,6 +695,10 @@ struct v4l2_buffer {
>>   #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
>>   #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
>>   #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
>> +/* Timestamp sources. */
>> +#define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
>> +#define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
>> +#define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
>>
>>   /**
>>    * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor

--
Regards,
Sylwester

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Sylwester Nawrocki]

On 02/20/2014 10:10 PM, Sylwester Nawrocki wrote:
>> I would actually be inclined to drop it altogether for this particular
>> timestamp source. But it's up to Laurent.
>
> Yup, the "a small amount of time" concept seems a bit vague here.
> It's not clear how long period it could be and the tolerance would like
> very across the hardware.

Sorry, I meant "would likely vary".

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Laurent Pinchart]

Hi Hans,

On Thursday 20 February 2014 21:36:51 Hans Verkuil wrote:
> On 02/20/2014 08:41 PM, Sakari Ailus wrote:
> > Some devices do not produce timestamps that correspond to the end of the
> > frame. The user space should be informed on the matter. This patch
> > achieves
> > that by adding buffer flags (and a mask) for timestamp sources since more
> > possible timestamping points are expected than just two.
> > 
> > A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
> > eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end
> > of frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure
> > (next value).
> 
> Sorry, but I still have two small notes:
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> > since v5:
> > - Add a note on software generated timestamp inaccuracy.
> > 
> >  Documentation/DocBook/media/v4l/io.xml   |   38 +++++++++++++++++++++----
> >  drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
> >  include/media/videobuf2-core.h           |    2 ++
> >  include/uapi/linux/videodev2.h           |    4 ++++
> >  4 files changed, 41 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/io.xml
> > b/Documentation/DocBook/media/v4l/io.xml index 46d24b3..22b87bc 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -653,12 +653,6 @@ plane, are stored in struct
> > <structname>v4l2_plane</structname> instead.> 
> >  In that case, struct <structname>v4l2_buffer</structname> contains an
> >  array of plane structures.</para>
> > 
> > -      <para>For timestamp types that are sampled from the system clock
> > -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp
> > is
> > -taken after the complete frame has been received (or transmitted in
> > -case of video output devices). For other kinds of
> > -timestamps this may vary depending on the driver.</para>
> > -
> > 
> >      <table frame="none" pgwide="1" id="v4l2-buffer">
> >      
> >        <title>struct <structname>v4l2_buffer</structname></title>
> >        <tgroup cols="4">
> > 
> > @@ -1119,6 +1113,38 @@ in which case caches have not been used.</entry>
> >  	    <entry>The CAPTURE buffer timestamp has been taken from the
> >  	    corresponding OUTPUT buffer. This flag applies only to mem2mem
> >  	    devices.</entry>
> >  	  </row>
> > +	  <row>
> > +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
> > +	    <entry>0x00070000</entry>
> > +	    <entry>Mask for timestamp sources below. The timestamp source
> > +	    defines the point of time the timestamp is taken in relation to
> > +	    the frame. Logical and operation between the
> > +	    <structfield>flags</structfield> field and
> > +	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
> > +	    value of the timestamp source.</entry>
> > +	  </row>
> > +	  <row>
> > +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
> > +	    <entry>0x00000000</entry>
> > +	    <entry>End Of Frame. The buffer timestamp has been taken
> > +	    when the last pixel of the frame has been received or the
> > +	    last pixel of the frame has been transmitted. In practice,
> > +	    software generated timestamps will typically be read from
> > +	    the clock a small amount of time after the last pixel has
> > +	    been received, depending on the system and other activity
> 
> s/been received/been received or transmitted/
> 
> > +	    in it.</entry>
> > +	  </row>
> > +	  <row>
> > +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
> > +	    <entry>0x00010000</entry>
> > +	    <entry>Start Of Exposure. The buffer timestamp has been
> > +	    taken when the exposure of the frame has begun. In
> > +	    practice, software generated timestamps will typically be
> > +	    read from the clock a small amount of time after the last
> > +	    pixel has been received, depending on the system and other
> > +	    activity in it. This is only valid for buffer type
> > +	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
> 
> I would move the last sentence up to just before "In practice...". The
> way it is now it looks like an afterthought.
> 
> I am also not sure whether the whole "In practice" sentence is valid
> here. Certainly the bit about "the last pixel" isn't since this is the
> "SOE" case and not the End Of Frame. In the case of the UVC driver (and
> that's the only one using this timestamp source) the timestamps come from
> the hardware as I understand it, so the "software generated" bit doesn't
> apply.
> 
> I would actually be inclined to drop it altogether for this particular
> timestamp source. But it's up to Laurent.

What do you mean, drop what altogether ?

> > +	  </row>
> >  	</tbody>
> >        </tgroup>
> >      </table>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Hans Verkuil]

On 02/21/2014 12:30 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 20 February 2014 21:36:51 Hans Verkuil wrote:
>> On 02/20/2014 08:41 PM, Sakari Ailus wrote:
>>> Some devices do not produce timestamps that correspond to the end of the
>>> frame. The user space should be informed on the matter. This patch
>>> achieves
>>> that by adding buffer flags (and a mask) for timestamp sources since more
>>> possible timestamping points are expected than just two.
>>>
>>> A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
>>> eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end
>>> of frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure
>>> (next value).
>>
>> Sorry, but I still have two small notes:
>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>> ---
>>> since v5:
>>> - Add a note on software generated timestamp inaccuracy.
>>>
>>>  Documentation/DocBook/media/v4l/io.xml   |   38 +++++++++++++++++++++----
>>>  drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>>>  include/media/videobuf2-core.h           |    2 ++
>>>  include/uapi/linux/videodev2.h           |    4 ++++
>>>  4 files changed, 41 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/DocBook/media/v4l/io.xml
>>> b/Documentation/DocBook/media/v4l/io.xml index 46d24b3..22b87bc 100644
>>> --- a/Documentation/DocBook/media/v4l/io.xml
>>> +++ b/Documentation/DocBook/media/v4l/io.xml
>>> @@ -653,12 +653,6 @@ plane, are stored in struct
>>> <structname>v4l2_plane</structname> instead.> 
>>>  In that case, struct <structname>v4l2_buffer</structname> contains an
>>>  array of plane structures.</para>
>>>
>>> -      <para>For timestamp types that are sampled from the system clock
>>> -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp
>>> is
>>> -taken after the complete frame has been received (or transmitted in
>>> -case of video output devices). For other kinds of
>>> -timestamps this may vary depending on the driver.</para>
>>> -
>>>
>>>      <table frame="none" pgwide="1" id="v4l2-buffer">
>>>      
>>>        <title>struct <structname>v4l2_buffer</structname></title>
>>>        <tgroup cols="4">
>>>
>>> @@ -1119,6 +1113,38 @@ in which case caches have not been used.</entry>
>>>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>>>  	    corresponding OUTPUT buffer. This flag applies only to mem2mem
>>>  	    devices.</entry>
>>>  	  </row>
>>> +	  <row>
>>> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
>>> +	    <entry>0x00070000</entry>
>>> +	    <entry>Mask for timestamp sources below. The timestamp source
>>> +	    defines the point of time the timestamp is taken in relation to
>>> +	    the frame. Logical and operation between the
>>> +	    <structfield>flags</structfield> field and
>>> +	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
>>> +	    value of the timestamp source.</entry>
>>> +	  </row>
>>> +	  <row>
>>> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
>>> +	    <entry>0x00000000</entry>
>>> +	    <entry>End Of Frame. The buffer timestamp has been taken
>>> +	    when the last pixel of the frame has been received or the
>>> +	    last pixel of the frame has been transmitted. In practice,
>>> +	    software generated timestamps will typically be read from
>>> +	    the clock a small amount of time after the last pixel has
>>> +	    been received, depending on the system and other activity
>>
>> s/been received/been received or transmitted/
>>
>>> +	    in it.</entry>
>>> +	  </row>
>>> +	  <row>
>>> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
>>> +	    <entry>0x00010000</entry>
>>> +	    <entry>Start Of Exposure. The buffer timestamp has been
>>> +	    taken when the exposure of the frame has begun. In
>>> +	    practice, software generated timestamps will typically be
>>> +	    read from the clock a small amount of time after the last
>>> +	    pixel has been received, depending on the system and other
>>> +	    activity in it. This is only valid for buffer type
>>> +	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
>>
>> I would move the last sentence up to just before "In practice...". The
>> way it is now it looks like an afterthought.
>>
>> I am also not sure whether the whole "In practice" sentence is valid
>> here. Certainly the bit about "the last pixel" isn't since this is the
>> "SOE" case and not the End Of Frame. In the case of the UVC driver (and
>> that's the only one using this timestamp source) the timestamps come from
>> the hardware as I understand it, so the "software generated" bit doesn't
>> apply.
>>
>> I would actually be inclined to drop it altogether for this particular
>> timestamp source. But it's up to Laurent.
> 
> What do you mean, drop what altogether ?

The "In practice ... activity in it." sentence.

Regards,

	Hans

> 
>>> +	  </row>
>>>  	</tbody>
>>>        </tgroup>
>>>      </table>
> 

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Sakari Ailus]

Hi Hans,

Hans Verkuil wrote:
> On 02/20/2014 08:41 PM, Sakari Ailus wrote:
>> Some devices do not produce timestamps that correspond to the end of the
>> frame. The user space should be informed on the matter. This patch achieves
>> that by adding buffer flags (and a mask) for timestamp sources since more
>> possible timestamping points are expected than just two.
>>
>> A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
>> eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end of
>> frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure (next
>> value).
>
> Sorry, but I still have two small notes:
>
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>> ---
>> since v5:
>> - Add a note on software generated timestamp inaccuracy.
>>
>>   Documentation/DocBook/media/v4l/io.xml   |   38 +++++++++++++++++++++++++-----
>>   drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>>   include/media/videobuf2-core.h           |    2 ++
>>   include/uapi/linux/videodev2.h           |    4 ++++
>>   4 files changed, 41 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
>> index 46d24b3..22b87bc 100644
>> --- a/Documentation/DocBook/media/v4l/io.xml
>> +++ b/Documentation/DocBook/media/v4l/io.xml
>> @@ -653,12 +653,6 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
>>   In that case, struct <structname>v4l2_buffer</structname> contains an array of
>>   plane structures.</para>
>>
>> -      <para>For timestamp types that are sampled from the system clock
>> -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp is
>> -taken after the complete frame has been received (or transmitted in
>> -case of video output devices). For other kinds of
>> -timestamps this may vary depending on the driver.</para>
>> -
>>       <table frame="none" pgwide="1" id="v4l2-buffer">
>>         <title>struct <structname>v4l2_buffer</structname></title>
>>         <tgroup cols="4">
>> @@ -1119,6 +1113,38 @@ in which case caches have not been used.</entry>
>>   	    <entry>The CAPTURE buffer timestamp has been taken from the
>>   	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
>>   	  </row>
>> +	  <row>
>> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
>> +	    <entry>0x00070000</entry>
>> +	    <entry>Mask for timestamp sources below. The timestamp source
>> +	    defines the point of time the timestamp is taken in relation to
>> +	    the frame. Logical and operation between the
>> +	    <structfield>flags</structfield> field and
>> +	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
>> +	    value of the timestamp source.</entry>
>> +	  </row>
>> +	  <row>
>> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
>> +	    <entry>0x00000000</entry>
>> +	    <entry>End Of Frame. The buffer timestamp has been taken
>> +	    when the last pixel of the frame has been received or the
>> +	    last pixel of the frame has been transmitted. In practice,
>> +	    software generated timestamps will typically be read from
>> +	    the clock a small amount of time after the last pixel has
>> +	    been received, depending on the system and other activity
>
> s/been received/been received or transmitted/

I'll fix that.

>> +	    in it.</entry>
>> +	  </row>
>> +	  <row>
>> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
>> +	    <entry>0x00010000</entry>
>> +	    <entry>Start Of Exposure. The buffer timestamp has been
>> +	    taken when the exposure of the frame has begun. In
>> +	    practice, software generated timestamps will typically be
>> +	    read from the clock a small amount of time after the last
>> +	    pixel has been received, depending on the system and other
>> +	    activity in it. This is only valid for buffer type
>> +	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
>
> I would move the last sentence up to just before "In practice...". The
> way it is now it looks like an afterthought.

Same here.

> I am also not sure whether the whole "In practice" sentence is valid
> here. Certainly the bit about "the last pixel" isn't since this is the
> "SOE" case and not the End Of Frame. In the case of the UVC driver (and that's
> the only one using this timestamp source) the timestamps come from the
> hardware as I understand it, so the "software generated" bit doesn't
> apply.

Indeed. I don't know how the timestamp is even produced by the hardware. 
It's possible to calculate it (decrementing the readout time + exposure 
time from the end of frame timestamp) and that's what the devices 
supposedly do. The pre-frame exposure time isn't available to the host, 
so the end of frame timestamp cannot be calculated by the host from the 
camera generated timestamp.

However the link to the host is USB which has a lot more latency than 
almost anything else which makes even hardware generated timestamps a 
little imprecise.

-- 
Regards,

Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Sakari Ailus]

Hi Sylwester,

Thanks for the comments.

Sylwester Nawrocki wrote:
...
> On 02/20/2014 09:36 PM, Hans Verkuil wrote:
>> On 02/20/2014 08:41 PM, Sakari Ailus wrote:
>>> Some devices do not produce timestamps that correspond to the end of the
>>> frame. The user space should be informed on the matter. This patch
>>> achieves
>>> that by adding buffer flags (and a mask) for timestamp sources since
>>> more
>>> possible timestamping points are expected than just two.
>>>
>>> A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two
>>> of the
>>> eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for
>>> end of
>>> frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure
>>> (next
>>> value).
>>
>> Sorry, but I still have two small notes:
>>
>>> Signed-off-by: Sakari Ailus<sakari.ailus@iki.fi>
>>> ---
>>> since v5:
>>> - Add a note on software generated timestamp inaccuracy.
>>>
>>>   Documentation/DocBook/media/v4l/io.xml   |   38
>>> +++++++++++++++++++++++++-----
>>>   drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>>>   include/media/videobuf2-core.h           |    2 ++
>>>   include/uapi/linux/videodev2.h           |    4 ++++
>>>   4 files changed, 41 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/DocBook/media/v4l/io.xml
>>> b/Documentation/DocBook/media/v4l/io.xml
>>> index 46d24b3..22b87bc 100644
>>> --- a/Documentation/DocBook/media/v4l/io.xml
>>> +++ b/Documentation/DocBook/media/v4l/io.xml
>>> @@ -653,12 +653,6 @@ plane, are stored in
>>> struct<structname>v4l2_plane</structname>  instead.
>>>   In that case, struct<structname>v4l2_buffer</structname>  contains
>>> an array of
>>>   plane structures.</para>
>>>
>>> -<para>For timestamp types that are sampled from the system clock
>>> -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the
>>> timestamp is
>>> -taken after the complete frame has been received (or transmitted in
>>> -case of video output devices). For other kinds of
>>> -timestamps this may vary depending on the driver.</para>
>>> -
>>>       <table frame="none" pgwide="1" id="v4l2-buffer">
>>>         <title>struct<structname>v4l2_buffer</structname></title>
>>>         <tgroup cols="4">
>>> @@ -1119,6 +1113,38 @@ in which case caches have not been used.</entry>
>>>       <entry>The CAPTURE buffer timestamp has been taken from the
>>>           corresponding OUTPUT buffer. This flag applies only to
>>> mem2mem devices.</entry>
>>>       </row>
>>> +    <row>
>>> +    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
>>> +    <entry>0x00070000</entry>
>>> +    <entry>Mask for timestamp sources below. The timestamp source
>>> +        defines the point of time the timestamp is taken in relation to
>>> +        the frame. Logical and operation between the
>
> Perhaps s/and/AND ?

Is there a particular reason why? :-)

>>> +    <structfield>flags</structfield>  field and
>>> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant>  produces the
>>> +        value of the timestamp source.</entry>
>>> +    </row>
>>> +    <row>
>>> +    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
>>> +    <entry>0x00000000</entry>
>>> +    <entry>End Of Frame. The buffer timestamp has been taken
>>> +        when the last pixel of the frame has been received or the
>>> +        last pixel of the frame has been transmitted. In practice,
>>> +        software generated timestamps will typically be read from
>>> +        the clock a small amount of time after the last pixel has
>>> +        been received, depending on the system and other activity
>>
>> s/been received/been received or transmitted/
>>
>>> +        in it.</entry>
>>> +    </row>
>>> +    <row>
>>> +    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
>
> V4L2_BUF_FLAG_TSTAMP_SRC_SOF (Start Of Frame) wouldn't fit here ?

That's different. Uvc devices provide a special start of exposure 
timestamp which is generated in camera itself. This is start of frame - 
exposure time (which, if AE is enabled, is a per-frame value).

>>> +    <entry>0x00010000</entry>
>>> +    <entry>Start Of Exposure. The buffer timestamp has been
>>> +        taken when the exposure of the frame has begun. In
>>> +        practice, software generated timestamps will typically be
>>> +        read from the clock a small amount of time after the last
>>> +        pixel has been received, depending on the system and other
>>> +        activity in it. This is only valid for buffer type
>>> +    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
>>
>> I would move the last sentence up to just before "In practice...". The
>> way it is now it looks like an afterthought.
>>
>> I am also not sure whether the whole "In practice" sentence is valid
>> here. Certainly the bit about "the last pixel" isn't since this is the
>> "SOE" case and not the End Of Frame. In the case of the UVC driver
>> (and that's
>> the only one using this timestamp source) the timestamps come from the
>> hardware as I understand it, so the "software generated" bit doesn't
>> apply.
>
> I agree, not it looks like a copy & paste from the "End Of Frame"
> paragraph. I guess for SOE it should have been, e.g.

It is. :-) I shouldn't have written this so late in the evening... I'll 
remove it.

> "...read from the clock a small amount of time after the _first_
>      pixel has been received" ?
>
>> I would actually be inclined to drop it altogether for this particular
>> timestamp source. But it's up to Laurent.
>
> Yup, the "a small amount of time" concept seems a bit vague here.
> It's not clear how long period it could be and the tolerance would like
> very across the hardware.

This is very system and device specific so we can't say it'd be exactly 
within certain limits. What we say instead is that we don't know, which 
is true.

I think Hans's comment was related to this particular timestamp type; 
the same text is still present for the EOF timestamp.

-- 
Kind regards,

Kind regards,

Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Laurent Pinchart]

Hi Sakari,

On Friday 21 February 2014 11:31:38 Sakari Ailus wrote:
> Hans Verkuil wrote:
> > On 02/20/2014 08:41 PM, Sakari Ailus wrote:
> >> Some devices do not produce timestamps that correspond to the end of the
> >> frame. The user space should be informed on the matter. This patch
> >> achieves that by adding buffer flags (and a mask) for timestamp sources
> >> since more possible timestamping points are expected than just two.
> >> 
> >> A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of
> >> the
> >> eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end
> >> of frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure
> >> (next value).
> > 
> > Sorry, but I still have two small notes:
> >> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >> ---
> >> since v5:
> >> - Add a note on software generated timestamp inaccuracy.
> >> 
> >>   Documentation/DocBook/media/v4l/io.xml   |   38
> >>   +++++++++++++++++++++++++-----
> >>   drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
> >>   include/media/videobuf2-core.h           |    2 ++
> >>   include/uapi/linux/videodev2.h           |    4 ++++
> >>   4 files changed, 41 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/Documentation/DocBook/media/v4l/io.xml
> >> b/Documentation/DocBook/media/v4l/io.xml index 46d24b3..22b87bc 100644
> >> --- a/Documentation/DocBook/media/v4l/io.xml
> >> +++ b/Documentation/DocBook/media/v4l/io.xml
> >> @@ -653,12 +653,6 @@ plane, are stored in struct
> >> <structname>v4l2_plane</structname> instead.>> 
> >>   In that case, struct <structname>v4l2_buffer</structname> contains an
> >>   array of plane structures.</para>
> >> 
> >> -      <para>For timestamp types that are sampled from the system clock
> >> -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp
> >> is
> >> -taken after the complete frame has been received (or transmitted in
> >> -case of video output devices). For other kinds of
> >> -timestamps this may vary depending on the driver.</para>
> >> -
> >> 
> >>       <table frame="none" pgwide="1" id="v4l2-buffer">
> >>       
> >>         <title>struct <structname>v4l2_buffer</structname></title>
> >>         <tgroup cols="4">
> >> 
> >> @@ -1119,6 +1113,38 @@ in which case caches have not been used.</entry>
> >> 
> >>   	    <entry>The CAPTURE buffer timestamp has been taken from the
> >>   	    corresponding OUTPUT buffer. This flag applies only to mem2mem
> >>   	    devices.</entry>>>   	  
> >>   	  </row>
> >> 
> >> +	  <row>
> >> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
> >> +	    <entry>0x00070000</entry>
> >> +	    <entry>Mask for timestamp sources below. The timestamp source
> >> +	    defines the point of time the timestamp is taken in relation to
> >> +	    the frame. Logical and operation between the
> >> +	    <structfield>flags</structfield> field and
> >> +	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
> >> +	    value of the timestamp source.</entry>
> >> +	  </row>
> >> +	  <row>
> >> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
> >> +	    <entry>0x00000000</entry>
> >> +	    <entry>End Of Frame. The buffer timestamp has been taken
> >> +	    when the last pixel of the frame has been received or the
> >> +	    last pixel of the frame has been transmitted. In practice,
> >> +	    software generated timestamps will typically be read from
> >> +	    the clock a small amount of time after the last pixel has
> >> +	    been received, depending on the system and other activity
> > 
> > s/been received/been received or transmitted/
> 
> I'll fix that.
> 
> >> +	    in it.</entry>
> >> +	  </row>
> >> +	  <row>
> >> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
> >> +	    <entry>0x00010000</entry>
> >> +	    <entry>Start Of Exposure. The buffer timestamp has been
> >> +	    taken when the exposure of the frame has begun. In
> >> +	    practice, software generated timestamps will typically be
> >> +	    read from the clock a small amount of time after the last
> >> +	    pixel has been received, depending on the system and other
> >> +	    activity in it. This is only valid for buffer type
> >> +	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
> > 
> > I would move the last sentence up to just before "In practice...". The
> > way it is now it looks like an afterthought.
> 
> Same here.
> 
> > I am also not sure whether the whole "In practice" sentence is valid
> > here. Certainly the bit about "the last pixel" isn't since this is the
> > "SOE" case and not the End Of Frame. In the case of the UVC driver (and
> > that's the only one using this timestamp source) the timestamps come from
> > the hardware as I understand it, so the "software generated" bit doesn't
> > apply.
> 
> Indeed. I don't know how the timestamp is even produced by the hardware.

In practice I don't know either, as that's hidden in the device firmware. The 
UVC specification just describes the timestamp as "The source clock time in 
native device clock units when the raw frame capture begins."

Let's face it, I'm pretty sure many devices don't care much and will send a 
time stamp that's taken at some other, possibly semi-random, time.

> It's possible to calculate it (decrementing the readout time + exposure
> time from the end of frame timestamp) and that's what the devices
> supposedly do. The pre-frame exposure time isn't available to the host,
> so the end of frame timestamp cannot be calculated by the host from the
> camera generated timestamp.
> 
> However the link to the host is USB which has a lot more latency than
> almost anything else which makes even hardware generated timestamps a
> little imprecise.

Why so ? There will be a jitter in frame arrival, but the hardware timestamp 
should be accurate (at least if properly generated by the camera firmware).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Sakari Ailus]

Hi Laurent,

On Fri, Feb 21, 2014 at 12:58:58PM +0100, Laurent Pinchart wrote:
...
> > It's possible to calculate it (decrementing the readout time + exposure
> > time from the end of frame timestamp) and that's what the devices
> > supposedly do. The pre-frame exposure time isn't available to the host,
> > so the end of frame timestamp cannot be calculated by the host from the
> > camera generated timestamp.
> > 
> > However the link to the host is USB which has a lot more latency than
> > almost anything else which makes even hardware generated timestamps a
> > little imprecise.
> 
> Why so ? There will be a jitter in frame arrival, but the hardware timestamp 
> should be accurate (at least if properly generated by the camera firmware).

Yes, the hardware timestamp should be accurate on its own, but as there's
delay and jitter converting that into something that's relevant on the host
adds some uncertainty. AFAIR the accuracy of the camera generated timestamp
was still much better than that of the driver generated one, right?

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v5.1 3/7] v4l: Add timestamp source flags, mask and document them

[Laurent Pinchart]

Hi Sakari,

On Friday 21 February 2014 15:04:48 Sakari Ailus wrote:
> Hi Laurent,
> 
> On Fri, Feb 21, 2014 at 12:58:58PM +0100, Laurent Pinchart wrote:
> ...
> 
> > > It's possible to calculate it (decrementing the readout time + exposure
> > > time from the end of frame timestamp) and that's what the devices
> > > supposedly do. The pre-frame exposure time isn't available to the host,
> > > so the end of frame timestamp cannot be calculated by the host from the
> > > camera generated timestamp.
> > > 
> > > However the link to the host is USB which has a lot more latency than
> > > almost anything else which makes even hardware generated timestamps a
> > > little imprecise.
> > 
> > Why so ? There will be a jitter in frame arrival, but the hardware
> > timestamp should be accurate (at least if properly generated by the
> > camera firmware).
>
> Yes, the hardware timestamp should be accurate on its own, but as there's
> delay and jitter converting that into something that's relevant on the host
> adds some uncertainty. AFAIR the accuracy of the camera generated timestamp
> was still much better than that of the driver generated one, right?

Yes it is, as the conversion mechanism is stats-based and takes jitter and 
delays into account.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5.1 7/7] v4l: Document timestamp buffer flag behaviour

[Sakari Ailus]

On Thu, Feb 20, 2014 at 09:25:47PM +0100, Hans Verkuil wrote:
> On 02/20/2014 08:42 PM, Sakari Ailus wrote:
> > Timestamp buffer flags are constant at the moment. Document them so that 1)
> > they're always valid and 2) not changed by the drivers. This leaves room to
> > extend the functionality later on if needed.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks! :-)

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v5.2 3/7] v4l: Add timestamp source flags, mask and document them

[Sakari Ailus]

Some devices do not produce timestamps that correspond to the end of the
frame. The user space should be informed on the matter. This patch achieves
that by adding buffer flags (and a mask) for timestamp sources since more
possible timestamping points are expected than just two.

A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end of
frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure (next
value).

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
since v5.1:
- Remove the note on timestamp inaccuracy from SOE timestamps.
- Add ..."or transmitted" for EOF timestamps.
- Correct language related to valid buffer types and SOE timestamps.

 Documentation/DocBook/media/v4l/io.xml   |   36 +++++++++++++++++++++++++-----
 drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
 include/media/videobuf2-core.h           |    2 ++
 include/uapi/linux/videodev2.h           |    4 ++++
 4 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 46d24b3..d44401c 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -653,12 +653,6 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
 In that case, struct <structname>v4l2_buffer</structname> contains an array of
 plane structures.</para>
 
-      <para>For timestamp types that are sampled from the system clock
-(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp is
-taken after the complete frame has been received (or transmitted in
-case of video output devices). For other kinds of
-timestamps this may vary depending on the driver.</para>
-
     <table frame="none" pgwide="1" id="v4l2-buffer">
       <title>struct <structname>v4l2_buffer</structname></title>
       <tgroup cols="4">
@@ -1119,6 +1113,36 @@ in which case caches have not been used.</entry>
 	    <entry>The CAPTURE buffer timestamp has been taken from the
 	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
 	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
+	    <entry>0x00070000</entry>
+	    <entry>Mask for timestamp sources below. The timestamp source
+	    defines the point of time the timestamp is taken in relation to
+	    the frame. Logical and operation between the
+	    <structfield>flags</structfield> field and
+	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
+	    value of the timestamp source.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
+	    <entry>0x00000000</entry>
+	    <entry>End Of Frame. The buffer timestamp has been taken
+	    when the last pixel of the frame has been received or the
+	    last pixel of the frame has been transmitted. In practice,
+	    software generated timestamps will typically be read from
+	    the clock a small amount of time after the last pixel has
+	    been received or transmitten, depending on the system and
+	    other activity in it.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
+	    <entry>0x00010000</entry>
+	    <entry>Start Of Exposure. The buffer timestamp has been
+	    taken when the exposure of the frame has begun. This is
+	    only valid for the
+	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant> buffer
+	    type.</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 5a5fb7f..6e314b0 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2195,7 +2195,9 @@ int vb2_queue_init(struct vb2_queue *q)
 	    WARN_ON(!q->io_modes)	  ||
 	    WARN_ON(!q->ops->queue_setup) ||
 	    WARN_ON(!q->ops->buf_queue)   ||
-	    WARN_ON(q->timestamp_type & ~V4L2_BUF_FLAG_TIMESTAMP_MASK))
+	    WARN_ON(q->timestamp_type &
+		    ~(V4L2_BUF_FLAG_TIMESTAMP_MASK |
+		      V4L2_BUF_FLAG_TSTAMP_SRC_MASK)))
 		return -EINVAL;
 
 	/* Warn that the driver should choose an appropriate timestamp type */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bef53ce..b6b992d 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -312,6 +312,8 @@ struct v4l2_fh;
  * @buf_struct_size: size of the driver-specific buffer structure;
  *		"0" indicates the driver doesn't want to use a custom buffer
  *		structure type, so sizeof(struct vb2_buffer) will is used
+ * @timestamp_type: Timestamp flags; V4L2_BUF_FLAGS_TIMESTAMP_* and
+ *		V4L2_BUF_FLAGS_TSTAMP_SRC_*
  * @gfp_flags:	additional gfp flags used when allocating the buffers.
  *		Typically this is 0, but it may be e.g. GFP_DMA or __GFP_DMA32
  *		to force the buffer allocation to a specific memory zone.
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index e9ee444..82e8661 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -695,6 +695,10 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
 #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
 #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
+/* Timestamp sources. */
+#define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
+#define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
+#define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
 
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
-- 
1.7.10.4

Re: [PATCH v5.2 3/7] v4l: Add timestamp source flags, mask and document them

[Hans Verkuil]

On 02/23/2014 11:40 AM, Sakari Ailus wrote:
> Some devices do not produce timestamps that correspond to the end of the
> frame. The user space should be informed on the matter. This patch achieves
> that by adding buffer flags (and a mask) for timestamp sources since more
> possible timestamping points are expected than just two.
> 
> A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of the
> eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF for end of
> frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of exposure (next
> value).
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

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

Regards,

	Hans

> ---
> since v5.1:
> - Remove the note on timestamp inaccuracy from SOE timestamps.
> - Add ..."or transmitted" for EOF timestamps.
> - Correct language related to valid buffer types and SOE timestamps.
> 
>  Documentation/DocBook/media/v4l/io.xml   |   36 +++++++++++++++++++++++++-----
>  drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>  include/media/videobuf2-core.h           |    2 ++
>  include/uapi/linux/videodev2.h           |    4 ++++
>  4 files changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 46d24b3..d44401c 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -653,12 +653,6 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
>  In that case, struct <structname>v4l2_buffer</structname> contains an array of
>  plane structures.</para>
>  
> -      <para>For timestamp types that are sampled from the system clock
> -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the timestamp is
> -taken after the complete frame has been received (or transmitted in
> -case of video output devices). For other kinds of
> -timestamps this may vary depending on the driver.</para>
> -
>      <table frame="none" pgwide="1" id="v4l2-buffer">
>        <title>struct <structname>v4l2_buffer</structname></title>
>        <tgroup cols="4">
> @@ -1119,6 +1113,36 @@ in which case caches have not been used.</entry>
>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>  	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
>  	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
> +	    <entry>0x00070000</entry>
> +	    <entry>Mask for timestamp sources below. The timestamp source
> +	    defines the point of time the timestamp is taken in relation to
> +	    the frame. Logical and operation between the
> +	    <structfield>flags</structfield> field and
> +	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces the
> +	    value of the timestamp source.</entry>
> +	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
> +	    <entry>0x00000000</entry>
> +	    <entry>End Of Frame. The buffer timestamp has been taken
> +	    when the last pixel of the frame has been received or the
> +	    last pixel of the frame has been transmitted. In practice,
> +	    software generated timestamps will typically be read from
> +	    the clock a small amount of time after the last pixel has
> +	    been received or transmitten, depending on the system and
> +	    other activity in it.</entry>
> +	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
> +	    <entry>0x00010000</entry>
> +	    <entry>Start Of Exposure. The buffer timestamp has been
> +	    taken when the exposure of the frame has begun. This is
> +	    only valid for the
> +	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant> buffer
> +	    type.</entry>
> +	  </row>
>  	</tbody>
>        </tgroup>
>      </table>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 5a5fb7f..6e314b0 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2195,7 +2195,9 @@ int vb2_queue_init(struct vb2_queue *q)
>  	    WARN_ON(!q->io_modes)	  ||
>  	    WARN_ON(!q->ops->queue_setup) ||
>  	    WARN_ON(!q->ops->buf_queue)   ||
> -	    WARN_ON(q->timestamp_type & ~V4L2_BUF_FLAG_TIMESTAMP_MASK))
> +	    WARN_ON(q->timestamp_type &
> +		    ~(V4L2_BUF_FLAG_TIMESTAMP_MASK |
> +		      V4L2_BUF_FLAG_TSTAMP_SRC_MASK)))
>  		return -EINVAL;
>  
>  	/* Warn that the driver should choose an appropriate timestamp type */
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index bef53ce..b6b992d 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -312,6 +312,8 @@ struct v4l2_fh;
>   * @buf_struct_size: size of the driver-specific buffer structure;
>   *		"0" indicates the driver doesn't want to use a custom buffer
>   *		structure type, so sizeof(struct vb2_buffer) will is used
> + * @timestamp_type: Timestamp flags; V4L2_BUF_FLAGS_TIMESTAMP_* and
> + *		V4L2_BUF_FLAGS_TSTAMP_SRC_*
>   * @gfp_flags:	additional gfp flags used when allocating the buffers.
>   *		Typically this is 0, but it may be e.g. GFP_DMA or __GFP_DMA32
>   *		to force the buffer allocation to a specific memory zone.
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index e9ee444..82e8661 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -695,6 +695,10 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
>  #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
>  #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
> +/* Timestamp sources. */
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
>  
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
> 

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Hans Verkuil]

On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> Timestamp buffer flags are constant at the moment. Document them so that 1)
> they're always valid and 2) not changed by the drivers. This leaves room to
> extend the functionality later on if needed.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index fbd0c6e..4f76565 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
>  In that case, struct <structname>v4l2_buffer</structname> contains an array of
>  plane structures.</para>
>  
> +    <para>Dequeued video buffers come with timestamps. These
> +    timestamps can be taken from different clocks and at different
> +    part of the frame, depending on the driver. Please see flags in
> +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
> +    linkend="buffer-flags">. These flags are guaranteed to be always
> +    valid and will not be changed by the driver autonomously. Changes
> +    in these flags may take place due as a side effect of
> +    &VIDIOC-S-INPUT; or &VIDIOC-S-OUTPUT; however.</para>

There is one exception to this: if the timestamps are copied from the output
buffer to the capture buffer (TIMESTAMP_COPY), then it can change theoretically
for every buffer since it entirely depends on what is being sent to it. The
value comes from userspace and you simply don't have any control over that.

I'm stress testing vb2 in lots of different ways, including timestamp handling.
It's not a pretty sight, I'm afraid. Expect a looong list of patches in the
coming week.

Regards,

	Hans

> +
>      <table frame="none" pgwide="1" id="v4l2-buffer">
>        <title>struct <structname>v4l2_buffer</structname></title>
>        <tgroup cols="4">
> 

Re: [PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

[Hans Verkuil]

On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> The buffer flags field is 32 bits but the defined only used 16. This is
> fine, but as more than 16 bits will be used in the very near future, define
> them as 32-bit numbers for consistency.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++-------------
>  include/uapi/linux/videodev2.h         |   38 +++++++++++++++++++-------------
>  2 files changed, 38 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 8facac4..46d24b3 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml

<snip>

> @@ -1115,7 +1115,7 @@ in which case caches have not been used.</entry>
>  	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
> -	    <entry>0x4000</entry>
> +	    <entry>0x00004000</entry>
>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>  	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
>  	  </row>

Should we add here that if TIMESTAMP_COPY is set and the TIMECODE flag is set,
then drivers should copy the TIMECODE struct as well? This is happening already
in various drivers and I think that is appropriate. Although to be honest nobody
is actually using the timecode struct, but we plan to hijack that for hardware
timestamps in the future anyway.

Regards,

	Hans

Re: [PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

[Sakari Ailus]

Hans Verkuil wrote:
> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
>> The buffer flags field is 32 bits but the defined only used 16. This is
>> fine, but as more than 16 bits will be used in the very near future, define
>> them as 32-bit numbers for consistency.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>> ---
>>  Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++-------------
>>  include/uapi/linux/videodev2.h         |   38 +++++++++++++++++++-------------
>>  2 files changed, 38 insertions(+), 30 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
>> index 8facac4..46d24b3 100644
>> --- a/Documentation/DocBook/media/v4l/io.xml
>> +++ b/Documentation/DocBook/media/v4l/io.xml
> 
> <snip>
> 
>> @@ -1115,7 +1115,7 @@ in which case caches have not been used.</entry>
>>  	  </row>
>>  	  <row>
>>  	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
>> -	    <entry>0x4000</entry>
>> +	    <entry>0x00004000</entry>
>>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>>  	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
>>  	  </row>
> 
> Should we add here that if TIMESTAMP_COPY is set and the TIMECODE flag is set,
> then drivers should copy the TIMECODE struct as well? This is happening already
> in various drivers and I think that is appropriate. Although to be honest nobody
> is actually using the timecode struct, but we plan to hijack that for hardware
> timestamps in the future anyway.

Is there a single driver which uses the timecode field? The fact is that
many m2m drivers copy it but that's probably mostly copying what one of
them happened to do by accident. :-)

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

[Hans Verkuil]

On 02/24/2014 04:34 PM, Sakari Ailus wrote:
> Hans Verkuil wrote:
>> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
>>> The buffer flags field is 32 bits but the defined only used 16. This is
>>> fine, but as more than 16 bits will be used in the very near future, define
>>> them as 32-bit numbers for consistency.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>> ---
>>>  Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++-------------
>>>  include/uapi/linux/videodev2.h         |   38 +++++++++++++++++++-------------
>>>  2 files changed, 38 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
>>> index 8facac4..46d24b3 100644
>>> --- a/Documentation/DocBook/media/v4l/io.xml
>>> +++ b/Documentation/DocBook/media/v4l/io.xml
>>
>> <snip>
>>
>>> @@ -1115,7 +1115,7 @@ in which case caches have not been used.</entry>
>>>  	  </row>
>>>  	  <row>
>>>  	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
>>> -	    <entry>0x4000</entry>
>>> +	    <entry>0x00004000</entry>
>>>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>>>  	    corresponding OUTPUT buffer. This flag applies only to mem2mem devices.</entry>
>>>  	  </row>
>>
>> Should we add here that if TIMESTAMP_COPY is set and the TIMECODE flag is set,
>> then drivers should copy the TIMECODE struct as well? This is happening already
>> in various drivers and I think that is appropriate. Although to be honest nobody
>> is actually using the timecode struct, but we plan to hijack that for hardware
>> timestamps in the future anyway.
> 
> Is there a single driver which uses the timecode field? The fact is that
> many m2m drivers copy it but that's probably mostly copying what one of
> them happened to do by accident. :-)
> 

No, there are no drivers that use this at the moment (other than for copying).
However, it is part of the API and I'd like to close these little holes and define
clearly what should be done. I think given the purpose of the timecode field it
makes sense to copy it. Note that it is the application that might be providing
that data, it doesn't have to come from a driver at all.

I've been doing a lot of testing over the weekend and this is one of those little
things that are not clearly defined.

Regards,

	Hans

RE: [PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

[Kamil Debski]

Hi,

> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> Sent: Monday, February 24, 2014 4:35 PM
> 
> Hans Verkuil wrote:
> > On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> >> The buffer flags field is 32 bits but the defined only used 16. This
> >> is fine, but as more than 16 bits will be used in the very near
> >> future, define them as 32-bit numbers for consistency.
> >>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >> ---
> >>  Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++---------
> ----
> >>  include/uapi/linux/videodev2.h         |   38 +++++++++++++++++++--
> -----------
> >>  2 files changed, 38 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/Documentation/DocBook/media/v4l/io.xml
> >> b/Documentation/DocBook/media/v4l/io.xml
> >> index 8facac4..46d24b3 100644
> >> --- a/Documentation/DocBook/media/v4l/io.xml
> >> +++ b/Documentation/DocBook/media/v4l/io.xml
> >
> > <snip>
> >
> >> @@ -1115,7 +1115,7 @@ in which case caches have not been
> used.</entry>
> >>  	  </row>
> >>  	  <row>
> >>
> <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
> >> -	    <entry>0x4000</entry>
> >> +	    <entry>0x00004000</entry>
> >>  	    <entry>The CAPTURE buffer timestamp has been taken from the
> >>  	    corresponding OUTPUT buffer. This flag applies only to
> mem2mem devices.</entry>
> >>  	  </row>
> >
> > Should we add here that if TIMESTAMP_COPY is set and the TIMECODE
> flag
> > is set, then drivers should copy the TIMECODE struct as well? This is
> > happening already in various drivers and I think that is appropriate.
> > Although to be honest nobody is actually using the timecode struct,
> > but we plan to hijack that for hardware timestamps in the future
> anyway.
> 
> Is there a single driver which uses the timecode field? The fact is
> that many m2m drivers copy it but that's probably mostly copying what
> one of them happened to do by accident. :-)

Let's focus on not breaking m2m drivers with timestamp patches this time.
I'm sure it was a matter of accident with the initial timestamp patches.

I agree with Hans here, not sure about hijacking it in the future, though.

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: [PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

[Laurent Pinchart]

Hi Hans,

On Monday 24 February 2014 17:02:20 Hans Verkuil wrote:
> On 02/24/2014 04:34 PM, Sakari Ailus wrote:
> > Hans Verkuil wrote:
> >> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> >>> The buffer flags field is 32 bits but the defined only used 16. This is
> >>> fine, but as more than 16 bits will be used in the very near future,
> >>> define them as 32-bit numbers for consistency.
> >>> 
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >>> ---
> >>> 
> >>>  Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++-------------
> >>>  include/uapi/linux/videodev2.h         |   38 +++++++++++++++----------
> >>   2 files changed, 38 insertions(+), 30 deletions(-)
> >>> 
> >>> diff --git a/Documentation/DocBook/media/v4l/io.xml
> >>> b/Documentation/DocBook/media/v4l/io.xml index 8facac4..46d24b3 100644
> >>> --- a/Documentation/DocBook/media/v4l/io.xml
> >>> +++ b/Documentation/DocBook/media/v4l/io.xml
> >> 
> >> <snip>
> >> 
> >>> @@ -1115,7 +1115,7 @@ in which case caches have not been used.</entry>
> >>>  	  </row>
> >>>  	  <row>
> >>>  	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
> >>> -	    <entry>0x4000</entry>
> >>> +	    <entry>0x00004000</entry>
> >>>  	    <entry>The CAPTURE buffer timestamp has been taken from the
> >>>  	    corresponding OUTPUT buffer. This flag applies only to mem2mem
> >>>  	    devices.</entry>
> >>>  	  </row>
> >> 
> >> Should we add here that if TIMESTAMP_COPY is set and the TIMECODE flag is
> >> set, then drivers should copy the TIMECODE struct as well? This is
> >> happening already in various drivers and I think that is appropriate.
> >> Although to be honest nobody is actually using the timecode struct, but
> >> we plan to hijack that for hardware timestamps in the future anyway.
> > 
> > Is there a single driver which uses the timecode field? The fact is that
> > many m2m drivers copy it but that's probably mostly copying what one of
> > them happened to do by accident. :-)
> 
> No, there are no drivers that use this at the moment (other than for
> copying). However, it is part of the API and I'd like to close these little
> holes and define clearly what should be done.

What would you think about deprecating the timecode field ? There's no 
mainline driver using it, I'd rather avoid introducing a dependency on the 
timecode in M2M applications.

> I think given the purpose of the timecode field it makes sense to copy it.
> Note that it is the application that might be providing that data, it
> doesn't have to come from a driver at all.
> 
> I've been doing a lot of testing over the weekend and this is one of those
> little things that are not clearly defined.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

['Sakari Ailus']

Hi Kamil and Hans,

On Mon, Feb 24, 2014 at 05:13:49PM +0100, Kamil Debski wrote:
> > From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> > Sent: Monday, February 24, 2014 4:35 PM
> > 
> > Hans Verkuil wrote:
> > > On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> > >> The buffer flags field is 32 bits but the defined only used 16. This
> > >> is fine, but as more than 16 bits will be used in the very near
> > >> future, define them as 32-bit numbers for consistency.
> > >>
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > >> ---
> > >>  Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++---------
> > ----
> > >>  include/uapi/linux/videodev2.h         |   38 +++++++++++++++++++--
> > -----------
> > >>  2 files changed, 38 insertions(+), 30 deletions(-)
> > >>
> > >> diff --git a/Documentation/DocBook/media/v4l/io.xml
> > >> b/Documentation/DocBook/media/v4l/io.xml
> > >> index 8facac4..46d24b3 100644
> > >> --- a/Documentation/DocBook/media/v4l/io.xml
> > >> +++ b/Documentation/DocBook/media/v4l/io.xml
> > >
> > > <snip>
> > >
> > >> @@ -1115,7 +1115,7 @@ in which case caches have not been
> > used.</entry>
> > >>  	  </row>
> > >>  	  <row>
> > >>
> > <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
> > >> -	    <entry>0x4000</entry>
> > >> +	    <entry>0x00004000</entry>
> > >>  	    <entry>The CAPTURE buffer timestamp has been taken from the
> > >>  	    corresponding OUTPUT buffer. This flag applies only to
> > mem2mem devices.</entry>
> > >>  	  </row>
> > >
> > > Should we add here that if TIMESTAMP_COPY is set and the TIMECODE
> > flag
> > > is set, then drivers should copy the TIMECODE struct as well? This is
> > > happening already in various drivers and I think that is appropriate.
> > > Although to be honest nobody is actually using the timecode struct,
> > > but we plan to hijack that for hardware timestamps in the future
> > anyway.
> > 
> > Is there a single driver which uses the timecode field? The fact is
> > that many m2m drivers copy it but that's probably mostly copying what
> > one of them happened to do by accident. :-)
> 
> Let's focus on not breaking m2m drivers with timestamp patches this time.
> I'm sure it was a matter of accident with the initial timestamp patches.

This patch extends the documentation of the buffer flags from 16 bits to 32
bits. There are no other changes in functionality nor documentation.

The patchset does indeed change the way timestamp and timestamp flags are
copied: from source to destination rather than the other way around. I'd
appreciate if you'd review especially that one (patch 5/7).

There are no other changes to the way timestamps (or timecode) are handled.

> I agree with Hans here, not sure about hijacking it in the future, though.

This patchset does not change the handling of the timecode field, other than
the fixes in patch 5/7. I would prefer to get this old patchset in and unify
the timecode field handling once it has been discussed and agreed on.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v5 2/7] v4l: Use full 32 bits for buffer flags

[Hans Verkuil]

On 02/25/14 12:44, 'Sakari Ailus' wrote:
> Hi Kamil and Hans,
> 
> On Mon, Feb 24, 2014 at 05:13:49PM +0100, Kamil Debski wrote:
>>> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
>>> Sent: Monday, February 24, 2014 4:35 PM
>>>
>>> Hans Verkuil wrote:
>>>> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
>>>>> The buffer flags field is 32 bits but the defined only used 16. This
>>>>> is fine, but as more than 16 bits will be used in the very near
>>>>> future, define them as 32-bit numbers for consistency.
>>>>>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>>>> ---
>>>>>  Documentation/DocBook/media/v4l/io.xml |   30 ++++++++++++---------
>>> ----
>>>>>  include/uapi/linux/videodev2.h         |   38 +++++++++++++++++++--
>>> -----------
>>>>>  2 files changed, 38 insertions(+), 30 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/DocBook/media/v4l/io.xml
>>>>> b/Documentation/DocBook/media/v4l/io.xml
>>>>> index 8facac4..46d24b3 100644
>>>>> --- a/Documentation/DocBook/media/v4l/io.xml
>>>>> +++ b/Documentation/DocBook/media/v4l/io.xml
>>>>
>>>> <snip>
>>>>
>>>>> @@ -1115,7 +1115,7 @@ in which case caches have not been
>>> used.</entry>
>>>>>  	  </row>
>>>>>  	  <row>
>>>>>
>>> <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
>>>>> -	    <entry>0x4000</entry>
>>>>> +	    <entry>0x00004000</entry>
>>>>>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>>>>>  	    corresponding OUTPUT buffer. This flag applies only to
>>> mem2mem devices.</entry>
>>>>>  	  </row>
>>>>
>>>> Should we add here that if TIMESTAMP_COPY is set and the TIMECODE
>>> flag
>>>> is set, then drivers should copy the TIMECODE struct as well? This is
>>>> happening already in various drivers and I think that is appropriate.
>>>> Although to be honest nobody is actually using the timecode struct,
>>>> but we plan to hijack that for hardware timestamps in the future
>>> anyway.
>>>
>>> Is there a single driver which uses the timecode field? The fact is
>>> that many m2m drivers copy it but that's probably mostly copying what
>>> one of them happened to do by accident. :-)
>>
>> Let's focus on not breaking m2m drivers with timestamp patches this time.
>> I'm sure it was a matter of accident with the initial timestamp patches.
> 
> This patch extends the documentation of the buffer flags from 16 bits to 32
> bits. There are no other changes in functionality nor documentation.
> 
> The patchset does indeed change the way timestamp and timestamp flags are
> copied: from source to destination rather than the other way around. I'd
> appreciate if you'd review especially that one (patch 5/7).
> 
> There are no other changes to the way timestamps (or timecode) are handled.
> 
>> I agree with Hans here, not sure about hijacking it in the future, though.
> 
> This patchset does not change the handling of the timecode field, other than
> the fixes in patch 5/7. I would prefer to get this old patchset in and unify
> the timecode field handling once it has been discussed and agreed on.
> 

That's fine by me with respect to timecode handling. That can be handled later.
My comment about patch 7/7 still stands (about having no guarantees when
dealing with TIMESTAMP_COPY). I think this should be mentioned, perhaps with
a note that since it is userspace that provides the source timestamps in this
case, it is also userspace's problem :-) Garbage in, garbage out.

Regards,

	Hans

RE: [PATCH v5 5/7] exynos-gsc, m2m-deinterlace, mx2_emmaprp: Copy v4l2_buffer data from src to dst

[Kamil Debski]

Hi Sakari,

> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> Sent: Saturday, February 15, 2014 9:53 PM
> 
> The timestamp and timecode fields were copied from destination to
> source, not the other way around as they should. Fix it.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Acked-by: Kamil Debski <k.debski@samsung.com>

> ---
>  drivers/media/platform/exynos-gsc/gsc-m2m.c |    4 ++--
>  drivers/media/platform/m2m-deinterlace.c    |    4 ++--
>  drivers/media/platform/mx2_emmaprp.c        |    4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> index 810c3e1..62c84d5 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> @@ -88,8 +88,8 @@ void gsc_m2m_job_finish(struct gsc_ctx *ctx, int
> vb_state)
>  	dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> 
>  	if (src_vb && dst_vb) {
> -		src_vb->v4l2_buf.timestamp = dst_vb->v4l2_buf.timestamp;
> -		src_vb->v4l2_buf.timecode = dst_vb->v4l2_buf.timecode;
> +		dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
> +		dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;

It is such a silly mistake that I had to think for a while why
could it be copied the other way. I suppose this happens when
coding in a hurry :( Thank you for spotting this.

> 
>  		v4l2_m2m_buf_done(src_vb, vb_state);
>  		v4l2_m2m_buf_done(dst_vb, vb_state);
> diff --git a/drivers/media/platform/m2m-deinterlace.c
> b/drivers/media/platform/m2m-deinterlace.c
> index 6bb86b5..1f272d3 100644
> --- a/drivers/media/platform/m2m-deinterlace.c
> +++ b/drivers/media/platform/m2m-deinterlace.c
> @@ -207,8 +207,8 @@ static void dma_callback(void *data)
>  	src_vb = v4l2_m2m_src_buf_remove(curr_ctx->m2m_ctx);
>  	dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->m2m_ctx);
> 
> -	src_vb->v4l2_buf.timestamp = dst_vb->v4l2_buf.timestamp;
> -	src_vb->v4l2_buf.timecode = dst_vb->v4l2_buf.timecode;
> +	dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
> +	dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;
> 
>  	v4l2_m2m_buf_done(src_vb, VB2_BUF_STATE_DONE);
>  	v4l2_m2m_buf_done(dst_vb, VB2_BUF_STATE_DONE); diff --git
> a/drivers/media/platform/mx2_emmaprp.c
> b/drivers/media/platform/mx2_emmaprp.c
> index c690435..91056ac0 100644
> --- a/drivers/media/platform/mx2_emmaprp.c
> +++ b/drivers/media/platform/mx2_emmaprp.c
> @@ -377,8 +377,8 @@ static irqreturn_t emmaprp_irq(int irq_emma, void
> *data)
>  			src_vb = v4l2_m2m_src_buf_remove(curr_ctx->m2m_ctx);
>  			dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->m2m_ctx);
> 
> -			src_vb->v4l2_buf.timestamp = dst_vb-
> >v4l2_buf.timestamp;
> -			src_vb->v4l2_buf.timecode = dst_vb-
> >v4l2_buf.timecode;
> +			dst_vb->v4l2_buf.timestamp = src_vb-
> >v4l2_buf.timestamp;
> +			dst_vb->v4l2_buf.timecode = src_vb-
> >v4l2_buf.timecode;
> 
>  			spin_lock_irqsave(&pcdev->irqlock, flags);
>  			v4l2_m2m_buf_done(src_vb, VB2_BUF_STATE_DONE);
> --
> 1.7.10.4

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

RE: [PATCH v5 6/7] v4l: Copy timestamp source flags to destination on m2m devices

[Kamil Debski]

Hi Sakari,

> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> Sent: Saturday, February 15, 2014 9:53 PM
> 
> Copy the flags containing the timestamp source from source buffer flags
> to the destination buffer flags on memory-to-memory devices. This is
> analogous to copying the timestamp field from source to destination.

This patch looks good to me.
 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Acked-by: Kamil Debski <k.debski@samsung.com>

> ---
>  drivers/media/platform/coda.c                |    3 +++
>  drivers/media/platform/exynos-gsc/gsc-m2m.c  |    4 ++++
>  drivers/media/platform/exynos4-is/fimc-m2m.c |    3 +++
>  drivers/media/platform/m2m-deinterlace.c     |    3 +++
>  drivers/media/platform/mem2mem_testdev.c     |    3 +++
>  drivers/media/platform/mx2_emmaprp.c         |    5 +++++
>  drivers/media/platform/s5p-g2d/g2d.c         |    3 +++
>  drivers/media/platform/s5p-jpeg/jpeg-core.c  |    3 +++
>  drivers/media/platform/s5p-mfc/s5p_mfc.c     |    5 +++++
>  drivers/media/platform/ti-vpe/vpe.c          |    2 ++
>  10 files changed, 34 insertions(+)
> 
> diff --git a/drivers/media/platform/coda.c
> b/drivers/media/platform/coda.c index 61f3dbc..fe6dee6 100644
> --- a/drivers/media/platform/coda.c
> +++ b/drivers/media/platform/coda.c
> @@ -2829,6 +2829,9 @@ static void coda_finish_encode(struct coda_ctx
> *ctx)
>  	}
> 
>  	dst_buf->v4l2_buf.timestamp = src_buf->v4l2_buf.timestamp;
> +	dst_buf->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	dst_buf->v4l2_buf.flags |=
> +		src_buf->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>  	dst_buf->v4l2_buf.timecode = src_buf->v4l2_buf.timecode;
> 
>  	v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE); diff --git
> a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> index 62c84d5..4260ea5 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> @@ -90,6 +90,10 @@ void gsc_m2m_job_finish(struct gsc_ctx *ctx, int
> vb_state)
>  	if (src_vb && dst_vb) {
>  		dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
>  		dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;
> +		dst_vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +		dst_vb->v4l2_buf.flags |=
> +			src_vb->v4l2_buf.flags
> +			& V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> 
>  		v4l2_m2m_buf_done(src_vb, vb_state);
>  		v4l2_m2m_buf_done(dst_vb, vb_state);
> diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c
> b/drivers/media/platform/exynos4-is/fimc-m2m.c
> index 9da95bd..a4249a1 100644
> --- a/drivers/media/platform/exynos4-is/fimc-m2m.c
> +++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
> @@ -134,6 +134,9 @@ static void fimc_device_run(void *priv)
>  		goto dma_unlock;
> 
>  	dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
> +	dst_vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	dst_vb->v4l2_buf.flags |=
> +		src_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> 
>  	/* Reconfigure hardware if the context has changed. */
>  	if (fimc->m2m.ctx != ctx) {
> diff --git a/drivers/media/platform/m2m-deinterlace.c
> b/drivers/media/platform/m2m-deinterlace.c
> index 1f272d3..79ffdab 100644
> --- a/drivers/media/platform/m2m-deinterlace.c
> +++ b/drivers/media/platform/m2m-deinterlace.c
> @@ -208,6 +208,9 @@ static void dma_callback(void *data)
>  	dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->m2m_ctx);
> 
>  	dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp;
> +	dst_vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	dst_vb->v4l2_buf.flags |=
> +		src_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>  	dst_vb->v4l2_buf.timecode = src_vb->v4l2_buf.timecode;
> 
>  	v4l2_m2m_buf_done(src_vb, VB2_BUF_STATE_DONE); diff --git
> a/drivers/media/platform/mem2mem_testdev.c
> b/drivers/media/platform/mem2mem_testdev.c
> index 08e2437..b91da7f 100644
> --- a/drivers/media/platform/mem2mem_testdev.c
> +++ b/drivers/media/platform/mem2mem_testdev.c
> @@ -239,6 +239,9 @@ static int device_process(struct m2mtest_ctx *ctx,
>  	memcpy(&out_vb->v4l2_buf.timestamp,
>  			&in_vb->v4l2_buf.timestamp,
>  			sizeof(struct timeval));
> +	out_vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	out_vb->v4l2_buf.flags |=
> +		in_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> 
>  	switch (ctx->mode) {
>  	case MEM2MEM_HFLIP | MEM2MEM_VFLIP:
> diff --git a/drivers/media/platform/mx2_emmaprp.c
> b/drivers/media/platform/mx2_emmaprp.c
> index 91056ac0..0f59082 100644
> --- a/drivers/media/platform/mx2_emmaprp.c
> +++ b/drivers/media/platform/mx2_emmaprp.c
> @@ -378,6 +378,11 @@ static irqreturn_t emmaprp_irq(int irq_emma, void
> *data)
>  			dst_vb = v4l2_m2m_dst_buf_remove(curr_ctx->m2m_ctx);
> 
>  			dst_vb->v4l2_buf.timestamp = src_vb-
> >v4l2_buf.timestamp;
> +			dst_vb->v4l2_buf.flags &=
> +				~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +			dst_vb->v4l2_buf.flags |=
> +				src_vb->v4l2_buf.flags
> +				& V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>  			dst_vb->v4l2_buf.timecode = src_vb-
> >v4l2_buf.timecode;
> 
>  			spin_lock_irqsave(&pcdev->irqlock, flags); diff
--git
> a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-
> g2d/g2d.c
> index 0fcf7d7..48fe6ea 100644
> --- a/drivers/media/platform/s5p-g2d/g2d.c
> +++ b/drivers/media/platform/s5p-g2d/g2d.c
> @@ -560,6 +560,9 @@ static irqreturn_t g2d_isr(int irq, void *prv)
> 
>  	dst->v4l2_buf.timecode = src->v4l2_buf.timecode;
>  	dst->v4l2_buf.timestamp = src->v4l2_buf.timestamp;
> +	dst->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	dst->v4l2_buf.flags |=
> +		src->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> 
>  	v4l2_m2m_buf_done(src, VB2_BUF_STATE_DONE);
>  	v4l2_m2m_buf_done(dst, VB2_BUF_STATE_DONE); diff --git
> a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index a1c78c8..7b10120 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -1766,6 +1766,9 @@ static irqreturn_t s5p_jpeg_irq(int irq, void
> *dev_id)
> 
>  	dst_buf->v4l2_buf.timecode = src_buf->v4l2_buf.timecode;
>  	dst_buf->v4l2_buf.timestamp = src_buf->v4l2_buf.timestamp;
> +	dst_buf->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	dst_buf->v4l2_buf.flags |=
> +		src_buf->v4l2_buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> 
>  	v4l2_m2m_buf_done(src_buf, state);
>  	if (curr_ctx->mode == S5P_JPEG_ENCODE) diff --git
> a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index e2aac59..702ca1b 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -232,6 +232,11 @@ static void s5p_mfc_handle_frame_copy_time(struct
> s5p_mfc_ctx *ctx)
>
src_buf->b->v4l2_buf.timecode;
>  			dst_buf->b->v4l2_buf.timestamp =
>
src_buf->b->v4l2_buf.timestamp;
> +			dst_buf->b->v4l2_buf.flags &=
> +				~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +			dst_buf->b->v4l2_buf.flags |=
> +				src_buf->b->v4l2_buf.flags
> +				& V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>  			switch (frame_type) {
>  			case S5P_FIMV_DECODE_FRAME_I_FRAME:
>  				dst_buf->b->v4l2_buf.flags |=
> diff --git a/drivers/media/platform/ti-vpe/vpe.c
> b/drivers/media/platform/ti-vpe/vpe.c
> index 1296c53..d67c467 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1278,6 +1278,8 @@ static irqreturn_t vpe_irq(int irq_vpe, void
> *data)
>  	d_buf = &d_vb->v4l2_buf;
> 
>  	d_buf->timestamp = s_buf->timestamp;
> +	d_buf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	d_buf->flags |= s_buf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>  	if (s_buf->flags & V4L2_BUF_FLAG_TIMECODE) {
>  		d_buf->flags |= V4L2_BUF_FLAG_TIMECODE;
>  		d_buf->timecode = s_buf->timecode;
> --
> 1.7.10.4

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

RE: [PATCH v5 3/7] v4l: Add timestamp source flags, mask and document them

[Kamil Debski]

Hi Sakari,

> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> Sent: Saturday, February 15, 2014 9:53 PM
> 
> Some devices do not produce timestamps that correspond to the end of
> the frame. The user space should be informed on the matter. This patch
> achieves that by adding buffer flags (and a mask) for timestamp sources
> since more possible timestamping points are expected than just two.
> 
> A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of
> the eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF
> for end of frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of
> exposure (next value).
> 

Changes in videobuf2-core.c look good.

> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Acked-by: Kamil Debski <k.debski@samsung.com>

> ---
>  Documentation/DocBook/media/v4l/io.xml   |   31
> ++++++++++++++++++++++++------
>  drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>  include/media/videobuf2-core.h           |    2 ++
>  include/uapi/linux/videodev2.h           |    4 ++++
>  4 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml
> b/Documentation/DocBook/media/v4l/io.xml
> index 46d24b3..fbd0c6e 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -653,12 +653,6 @@ plane, are stored in struct
> <structname>v4l2_plane</structname> instead.
>  In that case, struct <structname>v4l2_buffer</structname> contains an
> array of  plane structures.</para>
> 
> -      <para>For timestamp types that are sampled from the system clock
> -(V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC) it is guaranteed that the
> timestamp is -taken after the complete frame has been received (or
> transmitted in -case of video output devices). For other kinds of -
> timestamps this may vary depending on the driver.</para>
> -
>      <table frame="none" pgwide="1" id="v4l2-buffer">
>        <title>struct <structname>v4l2_buffer</structname></title>
>        <tgroup cols="4">
> @@ -1119,6 +1113,31 @@ in which case caches have not been used.</entry>
>  	    <entry>The CAPTURE buffer timestamp has been taken from the
>  	    corresponding OUTPUT buffer. This flag applies only to
> mem2mem devices.</entry>
>  	  </row>
> +	  <row>
> +
> <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant></entry>
> +	    <entry>0x00070000</entry>
> +	    <entry>Mask for timestamp sources below. The timestamp source
> +	    defines the point of time the timestamp is taken in relation
> to
> +	    the frame. Logical and operation between the
> +	    <structfield>flags</structfield> field and
> +	    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> produces
> the
> +	    value of the timestamp source.</entry>
> +	  </row>
> +	  <row>
> +
> <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_EOF</constant></entry>
> +	    <entry>0x00000000</entry>
> +	    <entry>"End of frame." The buffer timestamp has been taken
> +	    when the last pixel of the frame has been received or the
> +	    last pixel of the frame has been transmitted.</entry>
> +	  </row>
> +	  <row>
> +
> <entry><constant>V4L2_BUF_FLAG_TSTAMP_SRC_SOE</constant></entry>
> +	    <entry>0x00010000</entry>
> +	    <entry>"Start of exposure." The buffer timestamp has been
> taken
> +	    when the exposure of the frame has begun. This is only
> +	    valid for buffer type
> +	    <constant>V4L2_BUF_TYPE_VIDEO_CAPTURE</constant>.</entry>
> +	  </row>
>  	</tbody>
>        </tgroup>
>      </table>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c
> index 5a5fb7f..6e314b0 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -2195,7 +2195,9 @@ int vb2_queue_init(struct vb2_queue *q)
>  	    WARN_ON(!q->io_modes)	  ||
>  	    WARN_ON(!q->ops->queue_setup) ||
>  	    WARN_ON(!q->ops->buf_queue)   ||
> -	    WARN_ON(q->timestamp_type & ~V4L2_BUF_FLAG_TIMESTAMP_MASK))
> +	    WARN_ON(q->timestamp_type &
> +		    ~(V4L2_BUF_FLAG_TIMESTAMP_MASK |
> +		      V4L2_BUF_FLAG_TSTAMP_SRC_MASK)))
>  		return -EINVAL;

Looks good.

> 
>  	/* Warn that the driver should choose an appropriate timestamp
> type */ diff --git a/include/media/videobuf2-core.h
> b/include/media/videobuf2-core.h index bef53ce..b6b992d 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -312,6 +312,8 @@ struct v4l2_fh;
>   * @buf_struct_size: size of the driver-specific buffer structure;
>   *		"0" indicates the driver doesn't want to use a custom
> buffer
>   *		structure type, so sizeof(struct vb2_buffer) will is used
> + * @timestamp_type: Timestamp flags; V4L2_BUF_FLAGS_TIMESTAMP_* and
> + *		V4L2_BUF_FLAGS_TSTAMP_SRC_*
>   * @gfp_flags:	additional gfp flags used when allocating the
> buffers.
>   *		Typically this is 0, but it may be e.g. GFP_DMA or
> __GFP_DMA32
>   *		to force the buffer allocation to a specific memory zone.
> diff --git a/include/uapi/linux/videodev2.h
> b/include/uapi/linux/videodev2.h index e9ee444..82e8661 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -695,6 +695,10 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
>  #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
>  #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
> +/* Timestamp sources. */
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
> +#define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
> 
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file
> descriptor
> --
> 1.7.10.4

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Sakari Ailus]

Hi Hans,

On Sun, Feb 23, 2014 at 12:45:28PM +0100, Hans Verkuil wrote:
> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> > Timestamp buffer flags are constant at the moment. Document them so that 1)
> > they're always valid and 2) not changed by the drivers. This leaves room to
> > extend the functionality later on if needed.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> > index fbd0c6e..4f76565 100644
> > --- a/Documentation/DocBook/media/v4l/io.xml
> > +++ b/Documentation/DocBook/media/v4l/io.xml
> > @@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
> >  In that case, struct <structname>v4l2_buffer</structname> contains an array of
> >  plane structures.</para>
> >  
> > +    <para>Dequeued video buffers come with timestamps. These
> > +    timestamps can be taken from different clocks and at different
> > +    part of the frame, depending on the driver. Please see flags in
> > +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
> > +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
> > +    linkend="buffer-flags">. These flags are guaranteed to be always
> > +    valid and will not be changed by the driver autonomously. Changes
> > +    in these flags may take place due as a side effect of
> > +    &VIDIOC-S-INPUT; or &VIDIOC-S-OUTPUT; however.</para>
> 
> There is one exception to this: if the timestamps are copied from the output
> buffer to the capture buffer (TIMESTAMP_COPY), then it can change theoretically
> for every buffer since it entirely depends on what is being sent to it. The
> value comes from userspace and you simply don't have any control over that.

Yes; I agree.

And a good point as well --- the timestamp source flags currently come from
__fill_v4l2_buffer() which takes them from q->timestamp. This isn't right
for m2m devices.

I'll fix and resend (3rd patch most likely).

> I'm stress testing vb2 in lots of different ways, including timestamp handling.
> It's not a pretty sight, I'm afraid. Expect a looong list of patches in the
> coming week.

:-)

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Hans Verkuil]

On 02/25/2014 06:08 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Sun, Feb 23, 2014 at 12:45:28PM +0100, Hans Verkuil wrote:
>> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
>>> Timestamp buffer flags are constant at the moment. Document them so that 1)
>>> they're always valid and 2) not changed by the drivers. This leaves room to
>>> extend the functionality later on if needed.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>> ---
>>>  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
>>> index fbd0c6e..4f76565 100644
>>> --- a/Documentation/DocBook/media/v4l/io.xml
>>> +++ b/Documentation/DocBook/media/v4l/io.xml
>>> @@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
>>>  In that case, struct <structname>v4l2_buffer</structname> contains an array of
>>>  plane structures.</para>
>>>  
>>> +    <para>Dequeued video buffers come with timestamps. These
>>> +    timestamps can be taken from different clocks and at different
>>> +    part of the frame, depending on the driver. Please see flags in
>>> +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
>>> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
>>> +    linkend="buffer-flags">. These flags are guaranteed to be always
>>> +    valid and will not be changed by the driver autonomously. Changes
>>> +    in these flags may take place due as a side effect of
>>> +    &VIDIOC-S-INPUT; or &VIDIOC-S-OUTPUT; however.</para>
>>
>> There is one exception to this: if the timestamps are copied from the output
>> buffer to the capture buffer (TIMESTAMP_COPY), then it can change theoretically
>> for every buffer since it entirely depends on what is being sent to it. The
>> value comes from userspace and you simply don't have any control over that.
> 
> Yes; I agree.
> 
> And a good point as well --- the timestamp source flags currently come from
> __fill_v4l2_buffer() which takes them from q->timestamp. This isn't right
> for m2m devices.
> 
> I'll fix and resend (3rd patch most likely).

You'll want to reference this patch I posted today:

[RFCv1 PATCH 16/20] vb2: fix timecode and flags handling for output buffers

Also available in this git repo:

http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/vb2-part4

The current implementation in vb2 is actually broken (which is one of the
things fixed by this patch): if you prepare a buffer (VIDIOC_PREPARE_BUF)
and only then call VIDIOC_QBUF with a timestamp, that timestamp will be
lost since it will use the one set by PREPARE_BUF (either that or it is
zeroed, I've forgotten which of the two it was).

If you want to take that patch and add your own changes to it, then that's
fine by me. It should be pretty much standalone.

Regards,

	Hans

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Sakari Ailus]

Hi Hans,

On Tue, Feb 25, 2014 at 06:28:06PM +0100, Hans Verkuil wrote:
> On 02/25/2014 06:08 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Sun, Feb 23, 2014 at 12:45:28PM +0100, Hans Verkuil wrote:
> >> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
> >>> Timestamp buffer flags are constant at the moment. Document them so that 1)
> >>> they're always valid and 2) not changed by the drivers. This leaves room to
> >>> extend the functionality later on if needed.
> >>>
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >>> ---
> >>>  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> >>> index fbd0c6e..4f76565 100644
> >>> --- a/Documentation/DocBook/media/v4l/io.xml
> >>> +++ b/Documentation/DocBook/media/v4l/io.xml
> >>> @@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
> >>>  In that case, struct <structname>v4l2_buffer</structname> contains an array of
> >>>  plane structures.</para>
> >>>  
> >>> +    <para>Dequeued video buffers come with timestamps. These
> >>> +    timestamps can be taken from different clocks and at different
> >>> +    part of the frame, depending on the driver. Please see flags in
> >>> +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
> >>> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
> >>> +    linkend="buffer-flags">. These flags are guaranteed to be always
> >>> +    valid and will not be changed by the driver autonomously. Changes
> >>> +    in these flags may take place due as a side effect of
> >>> +    &VIDIOC-S-INPUT; or &VIDIOC-S-OUTPUT; however.</para>
> >>
> >> There is one exception to this: if the timestamps are copied from the output
> >> buffer to the capture buffer (TIMESTAMP_COPY), then it can change theoretically
> >> for every buffer since it entirely depends on what is being sent to it. The
> >> value comes from userspace and you simply don't have any control over that.
> > 
> > Yes; I agree.
> > 
> > And a good point as well --- the timestamp source flags currently come from
> > __fill_v4l2_buffer() which takes them from q->timestamp. This isn't right
> > for m2m devices.
> > 
> > I'll fix and resend (3rd patch most likely).
> 
> You'll want to reference this patch I posted today:
> 
> [RFCv1 PATCH 16/20] vb2: fix timecode and flags handling for output buffers
> 
> Also available in this git repo:
> 
> http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/vb2-part4
> 
> The current implementation in vb2 is actually broken (which is one of the
> things fixed by this patch): if you prepare a buffer (VIDIOC_PREPARE_BUF)
> and only then call VIDIOC_QBUF with a timestamp, that timestamp will be
> lost since it will use the one set by PREPARE_BUF (either that or it is
> zeroed, I've forgotten which of the two it was).
> 
> If you want to take that patch and add your own changes to it, then that's
> fine by me. It should be pretty much standalone.

I'll keep that as-is and write another to pass the timestamp source flags
when needed. Would it be ok if I prepend the patch to the set?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v5 7/7] v4l: Document timestamp buffer flag behaviour

[Hans Verkuil]

On 02/26/2014 01:04 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Feb 25, 2014 at 06:28:06PM +0100, Hans Verkuil wrote:
>> On 02/25/2014 06:08 PM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Sun, Feb 23, 2014 at 12:45:28PM +0100, Hans Verkuil wrote:
>>>> On 02/15/2014 09:53 PM, Sakari Ailus wrote:
>>>>> Timestamp buffer flags are constant at the moment. Document them so that 1)
>>>>> they're always valid and 2) not changed by the drivers. This leaves room to
>>>>> extend the functionality later on if needed.
>>>>>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>>>> ---
>>>>>  Documentation/DocBook/media/v4l/io.xml |   10 ++++++++++
>>>>>  1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
>>>>> index fbd0c6e..4f76565 100644
>>>>> --- a/Documentation/DocBook/media/v4l/io.xml
>>>>> +++ b/Documentation/DocBook/media/v4l/io.xml
>>>>> @@ -653,6 +653,16 @@ plane, are stored in struct <structname>v4l2_plane</structname> instead.
>>>>>  In that case, struct <structname>v4l2_buffer</structname> contains an array of
>>>>>  plane structures.</para>
>>>>>  
>>>>> +    <para>Dequeued video buffers come with timestamps. These
>>>>> +    timestamps can be taken from different clocks and at different
>>>>> +    part of the frame, depending on the driver. Please see flags in
>>>>> +    the masks <constant>V4L2_BUF_FLAG_TIMESTAMP_MASK</constant> and
>>>>> +    <constant>V4L2_BUF_FLAG_TSTAMP_SRC_MASK</constant> in <xref
>>>>> +    linkend="buffer-flags">. These flags are guaranteed to be always
>>>>> +    valid and will not be changed by the driver autonomously. Changes
>>>>> +    in these flags may take place due as a side effect of
>>>>> +    &VIDIOC-S-INPUT; or &VIDIOC-S-OUTPUT; however.</para>
>>>>
>>>> There is one exception to this: if the timestamps are copied from the output
>>>> buffer to the capture buffer (TIMESTAMP_COPY), then it can change theoretically
>>>> for every buffer since it entirely depends on what is being sent to it. The
>>>> value comes from userspace and you simply don't have any control over that.
>>>
>>> Yes; I agree.
>>>
>>> And a good point as well --- the timestamp source flags currently come from
>>> __fill_v4l2_buffer() which takes them from q->timestamp. This isn't right
>>> for m2m devices.
>>>
>>> I'll fix and resend (3rd patch most likely).
>>
>> You'll want to reference this patch I posted today:
>>
>> [RFCv1 PATCH 16/20] vb2: fix timecode and flags handling for output buffers
>>
>> Also available in this git repo:
>>
>> http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/vb2-part4
>>
>> The current implementation in vb2 is actually broken (which is one of the
>> things fixed by this patch): if you prepare a buffer (VIDIOC_PREPARE_BUF)
>> and only then call VIDIOC_QBUF with a timestamp, that timestamp will be
>> lost since it will use the one set by PREPARE_BUF (either that or it is
>> zeroed, I've forgotten which of the two it was).
>>
>> If you want to take that patch and add your own changes to it, then that's
>> fine by me. It should be pretty much standalone.
> 
> I'll keep that as-is and write another to pass the timestamp source flags
> when needed. Would it be ok if I prepend the patch to the set?
> 

Sure, no problem.

	Hans

Re: [PATCH v5 3/7] v4l: Add timestamp source flags, mask and document them

['Sakari Ailus']

On Tue, Feb 25, 2014 at 02:09:41PM +0100, Kamil Debski wrote:
> Hi Sakari,
> 
> > From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> > Sent: Saturday, February 15, 2014 9:53 PM
> > 
> > Some devices do not produce timestamps that correspond to the end of
> > the frame. The user space should be informed on the matter. This patch
> > achieves that by adding buffer flags (and a mask) for timestamp sources
> > since more possible timestamping points are expected than just two.
> > 
> > A three-bit mask is defined (V4L2_BUF_FLAG_TSTAMP_SRC_MASK) and two of
> > the eight possible values is are defined V4L2_BUF_FLAG_TSTAMP_SRC_EOF
> > for end of frame (value zero) V4L2_BUF_FLAG_TSTAMP_SRC_SOE for start of
> > exposure (next value).
> > 
> 
> Changes in videobuf2-core.c look good.
> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Acked-by: Kamil Debski <k.debski@samsung.com>

Many thanks for the reviews, Kamil! :-)

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk