[PATCH/RFC 0/3] Add proper timestamp types handling in videobuf2

[PATCH/RFC 0/3] Add proper timestamp types handling in videobuf2

[Kamil Debski]

Hi,

The recent addition of timestamp types (and monotonic timestamp) left some room
for improvement. First of all not all drivers use monotonic timestamp. There are
for example mem2mem drivers that copy the timestamp from the OUTPUT buffer to
the corresponding CAPTURE buffer. Some videobuf2 drivers do not fill the
timestamp field altogether (yeah, I can agree that a constant is monotonic, but
still...).

Hence, I propose the following change to videobuf2. After applying this patch
the default timestamp type is UNKNOWN. It is up to the driver to set the
timestamp type to either MONOTONIC or COPY in vb2_queue_init.

This patch also adds setting proper timestamp type value in case of drivers
where I determined that type. This list might be missing some drivers, but
in these cases it will leave the UNKNOWN type which is a safe assumption.

Best wishes,
Kamil Debski

Kamil Debski (3):
  v4l: Define video buffer flag for the COPY timestamp type
  vb2: Add support for non monotonic timestamps
  v4l: Set proper timestamp type in selected drivers which use
    videobuf2

 Documentation/DocBook/media/v4l/io.xml             |    6 ++++++
 drivers/media/platform/blackfin/bfin_capture.c     |    1 +
 drivers/media/platform/davinci/vpif_capture.c      |    1 +
 drivers/media/platform/davinci/vpif_display.c      |    1 +
 drivers/media/platform/s3c-camif/camif-capture.c   |    1 +
 drivers/media/platform/s5p-fimc/fimc-capture.c     |    1 +
 drivers/media/platform/s5p-fimc/fimc-lite.c        |    1 +
 drivers/media/platform/s5p-mfc/s5p_mfc.c           |    2 ++
 drivers/media/platform/soc_camera/atmel-isi.c      |    1 +
 drivers/media/platform/soc_camera/mx2_camera.c     |    1 +
 drivers/media/platform/soc_camera/mx3_camera.c     |    1 +
 .../platform/soc_camera/sh_mobile_ceu_camera.c     |    1 +
 drivers/media/platform/vivi.c                      |    1 +
 drivers/media/usb/pwc/pwc-if.c                     |    1 +
 drivers/media/usb/stk1160/stk1160-v4l.c            |    1 +
 drivers/media/usb/uvc/uvc_queue.c                  |    1 +
 drivers/media/v4l2-core/videobuf2-core.c           |    5 +++--
 include/media/videobuf2-core.h                     |    1 +
 include/uapi/linux/videodev2.h                     |    1 +
 19 files changed, 27 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] v4l: Define video buffer flag for the COPY timestamp type

[Kamil Debski]

Define video buffer flag for the COPY timestamp. In this case the timestamp
value is copied from the OUTPUT to the corresponding CAPTURE buffer.

Signed-off-by: Kamil Debski <k.debski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/io.xml |    6 ++++++
 include/uapi/linux/videodev2.h         |    1 +
 2 files changed, 7 insertions(+)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 73f202f..fdd1822 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -1145,6 +1145,12 @@ in which case caches have not been used.</entry>
 	    same clock outside V4L2, use
 	    <function>clock_gettime(2)</function> .</entry>
 	  </row>
+	  <row>
+	    <entry><constant>V4L2_BUF_FLAG_TIMESTAMP_COPY</constant></entry>
+	    <entry>0x4000</entry>
+	    <entry>The CAPTURE buffer timestamp has been taken from the
+	    corresponding OUTPUT buffer.</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 72e9921..d5a59af 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -697,6 +697,7 @@ struct v4l2_buffer {
 #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
 
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
-- 
1.7.9.5

[PATCH 2/3] vb2: Add support for non monotonic timestamps

[Kamil Debski]

Not all drivers use monotonic timestamps. This patch adds a way to set the
timestamp type per every queue.

Signed-off-by: Kamil Debski <k.debski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/v4l2-core/videobuf2-core.c |    5 +++--
 include/media/videobuf2-core.h           |    1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 85e3c22..c02472c 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -403,7 +403,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 	 * Clear any buffer state related flags.
 	 */
 	b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
-	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	b->flags |= q->timestamp_type;
 
 	switch (vb->state) {
 	case VB2_BUF_STATE_QUEUED:
@@ -2032,7 +2032,8 @@ int vb2_queue_init(struct vb2_queue *q)
 	    WARN_ON(!q->type)		  ||
 	    WARN_ON(!q->io_modes)	  ||
 	    WARN_ON(!q->ops->queue_setup) ||
-	    WARN_ON(!q->ops->buf_queue))
+	    WARN_ON(!q->ops->buf_queue)   ||
+	    WARN_ON(q->timestamp_type & ~V4L2_BUF_FLAG_TIMESTAMP_MASK))
 		return -EINVAL;
 
 	INIT_LIST_HEAD(&q->queued_list);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 9cfd4ee..7ce4656 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -326,6 +326,7 @@ struct vb2_queue {
 	const struct vb2_mem_ops	*mem_ops;
 	void				*drv_priv;
 	unsigned int			buf_struct_size;
+	u32	                   	timestamp_type;
 
 /* private: internal use only */
 	enum v4l2_memory		memory;
-- 
1.7.9.5

[PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Kamil Debski]

Set proper timestamp type in drivers that I am sure that use either
MONOTONIC or COPY timestamps. Other drivers will correctly report
UNKNOWN timestamp type instead of assuming that all drivers use monotonic
timestamps.

Signed-off-by: Kamil Debski <k.debski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/blackfin/bfin_capture.c     |    1 +
 drivers/media/platform/davinci/vpif_capture.c      |    1 +
 drivers/media/platform/davinci/vpif_display.c      |    1 +
 drivers/media/platform/s3c-camif/camif-capture.c   |    1 +
 drivers/media/platform/s5p-fimc/fimc-capture.c     |    1 +
 drivers/media/platform/s5p-fimc/fimc-lite.c        |    1 +
 drivers/media/platform/s5p-mfc/s5p_mfc.c           |    2 ++
 drivers/media/platform/soc_camera/atmel-isi.c      |    1 +
 drivers/media/platform/soc_camera/mx2_camera.c     |    1 +
 drivers/media/platform/soc_camera/mx3_camera.c     |    1 +
 .../platform/soc_camera/sh_mobile_ceu_camera.c     |    1 +
 drivers/media/platform/vivi.c                      |    1 +
 drivers/media/usb/pwc/pwc-if.c                     |    1 +
 drivers/media/usb/stk1160/stk1160-v4l.c            |    1 +
 drivers/media/usb/uvc/uvc_queue.c                  |    1 +
 15 files changed, 16 insertions(+)

diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c
index d422d3c..365d6ef 100644
--- a/drivers/media/platform/blackfin/bfin_capture.c
+++ b/drivers/media/platform/blackfin/bfin_capture.c
@@ -939,6 +939,7 @@ static int __devinit bcap_probe(struct platform_device *pdev)
 	q->buf_struct_size = sizeof(struct bcap_buffer);
 	q->ops = &bcap_video_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	vb2_queue_init(q);
 
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 5892d2b..1943e41 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1035,6 +1035,7 @@ static int vpif_reqbufs(struct file *file, void *priv,
 	q->ops = &video_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct vpif_cap_buffer);
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	ret = vb2_queue_init(q);
 	if (ret) {
diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
index dd249c9..5477c2c 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -1001,6 +1001,7 @@ static int vpif_reqbufs(struct file *file, void *priv,
 	q->ops = &video_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct vpif_disp_buffer);
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	ret = vb2_queue_init(q);
 	if (ret) {
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
index a55793c..e91f350 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -1153,6 +1153,7 @@ int s3c_camif_register_video_node(struct camif_dev *camif, int idx)
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct camif_buffer);
 	q->drv_priv = vp;
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	ret = vb2_queue_init(q);
 	if (ret)
diff --git a/drivers/media/platform/s5p-fimc/fimc-capture.c b/drivers/media/platform/s5p-fimc/fimc-capture.c
index ddd689b..02cfb2b 100644
--- a/drivers/media/platform/s5p-fimc/fimc-capture.c
+++ b/drivers/media/platform/s5p-fimc/fimc-capture.c
@@ -1747,6 +1747,7 @@ static int fimc_register_capture_device(struct fimc_dev *fimc,
 	q->ops = &fimc_capture_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct fimc_vid_buffer);
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	ret = vb2_queue_init(q);
 	if (ret)
diff --git a/drivers/media/platform/s5p-fimc/fimc-lite.c b/drivers/media/platform/s5p-fimc/fimc-lite.c
index 1b309a7..39ea893 100644
--- a/drivers/media/platform/s5p-fimc/fimc-lite.c
+++ b/drivers/media/platform/s5p-fimc/fimc-lite.c
@@ -1251,6 +1251,7 @@ static int fimc_lite_subdev_registered(struct v4l2_subdev *sd)
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct flite_buffer);
 	q->drv_priv = fimc;
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	ret = vb2_queue_init(q);
 	if (ret < 0)
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index a527f85..30b4d15 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -807,6 +807,7 @@ static int s5p_mfc_open(struct file *file)
 		goto err_queue_init;
 	}
 	q->mem_ops = (struct vb2_mem_ops *)&vb2_dma_contig_memops;
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	ret = vb2_queue_init(q);
 	if (ret) {
 		mfc_err("Failed to initialize videobuf2 queue(capture)\n");
@@ -828,6 +829,7 @@ static int s5p_mfc_open(struct file *file)
 		goto err_queue_init;
 	}
 	q->mem_ops = (struct vb2_mem_ops *)&vb2_dma_contig_memops;
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	ret = vb2_queue_init(q);
 	if (ret) {
 		mfc_err("Failed to initialize videobuf2 queue(output)\n");
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index c8d748a..e531b82 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -514,6 +514,7 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
 	q->buf_struct_size = sizeof(struct frame_buffer);
 	q->ops = &isi_video_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	return vb2_queue_init(q);
 }
diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c
index 5fbac4f..5ff6a5d 100644
--- a/drivers/media/platform/soc_camera/mx2_camera.c
+++ b/drivers/media/platform/soc_camera/mx2_camera.c
@@ -1022,6 +1022,7 @@ static int mx2_camera_init_videobuf(struct vb2_queue *q,
 	q->ops = &mx2_videobuf_ops;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct mx2_buffer);
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	return vb2_queue_init(q);
 }
diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
index 574d125..abe9db6 100644
--- a/drivers/media/platform/soc_camera/mx3_camera.c
+++ b/drivers/media/platform/soc_camera/mx3_camera.c
@@ -455,6 +455,7 @@ static int mx3_camera_init_videobuf(struct vb2_queue *q,
 	q->ops = &mx3_videobuf_ops;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct mx3_camera_buffer);
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	return vb2_queue_init(q);
 }
diff --git a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
index 8a6d58d..b6c1c97 100644
--- a/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
+++ b/drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
@@ -2026,6 +2026,7 @@ static int sh_mobile_ceu_init_videobuf(struct vb2_queue *q,
 	q->ops = &sh_mobile_ceu_videobuf_ops;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct sh_mobile_ceu_buffer);
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	return vb2_queue_init(q);
 }
diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index c2f424f..86a5432 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -1305,6 +1305,7 @@ static int __init vivi_create_instance(int inst)
 	q->buf_struct_size = sizeof(struct vivi_buffer);
 	q->ops = &vivi_video_qops;
 	q->mem_ops = &vb2_vmalloc_memops;
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	ret = vb2_queue_init(q);
 	if (ret)
diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index 21c1523..1b65e0c 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -1001,6 +1001,7 @@ static int usb_pwc_probe(struct usb_interface *intf, const struct usb_device_id
 	pdev->vb_queue.buf_struct_size = sizeof(struct pwc_frame_buf);
 	pdev->vb_queue.ops = &pwc_vb_queue_ops;
 	pdev->vb_queue.mem_ops = &vb2_vmalloc_memops;
+	pdev->vb_queue.timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	rc = vb2_queue_init(&pdev->vb_queue);
 	if (rc < 0) {
 		PWC_ERROR("Oops, could not initialize vb2 queue.\n");
diff --git a/drivers/media/usb/stk1160/stk1160-v4l.c b/drivers/media/usb/stk1160/stk1160-v4l.c
index 6694f9e..5307a63 100644
--- a/drivers/media/usb/stk1160/stk1160-v4l.c
+++ b/drivers/media/usb/stk1160/stk1160-v4l.c
@@ -687,6 +687,7 @@ int stk1160_vb2_setup(struct stk1160 *dev)
 	q->buf_struct_size = sizeof(struct stk1160_buffer);
 	q->ops = &stk1160_video_qops;
 	q->mem_ops = &vb2_vmalloc_memops;
+	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	rc = vb2_queue_init(q);
 	if (rc < 0)
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 778addc..82d01d8 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -133,6 +133,7 @@ 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;
 	ret = vb2_queue_init(&queue->queue);
 	if (ret)
 		return ret;
-- 
1.7.9.5

Re: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Sakari Ailus]

Hi Kamil,

Thanks for the patch.

On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> Set proper timestamp type in drivers that I am sure that use either
> MONOTONIC or COPY timestamps. Other drivers will correctly report
> UNKNOWN timestamp type instead of assuming that all drivers use monotonic
> timestamps.

What other kind of timestamps there can be? All drivers (at least those not
mem-to-mem) that do obtain timestamps using system clock use monotonic ones.

I'd think that there should no longer be any drivers using the UNKNOWN
timestamp type: UNKNOWN is either from monotonic or realtime clock, and we
just replaced all of them with the monotonic ones. No driver uses realtime
timestamps anymore.

How about making MONOTONIC timestamps default instead, or at least assigning
all drivers something else than UNKNOWN?

-- 
Kind regards,

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

RE: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Kamil Debski]

Hi,

> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> Sent: Saturday, January 19, 2013 6:43 PM
> Hi Kamil,
> 
> Thanks for the patch.
> 
> On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> > Set proper timestamp type in drivers that I am sure that use either
> > MONOTONIC or COPY timestamps. Other drivers will correctly report
> > UNKNOWN timestamp type instead of assuming that all drivers use
> > monotonic timestamps.
> 
> What other kind of timestamps there can be? All drivers (at least those
> not
> mem-to-mem) that do obtain timestamps using system clock use monotonic
> ones.

Not set. It is not a COPY or MONOTONIC either. Any new or custom kind of
timestamp, maybe?

> I'd think that there should no longer be any drivers using the UNKNOWN
> timestamp type: UNKNOWN is either from monotonic or realtime clock, and
> we just replaced all of them with the monotonic ones. No driver uses
> realtime timestamps anymore.

Maybe there should be no drivers using UNKNOWN. But definitely
there should be no driver reporting MONOTONIC when the timestamp is not
monotonic.
 
> How about making MONOTONIC timestamps default instead, or at least
> assigning all drivers something else than UNKNOWN?

So why did you add the UNKNOWN flag?

The way I see it - UNKNOWN is the default and the one who coded the driver
will set it to either MONOTONIC or COPY if it is one of these two. It won't
be changed otherwise. There are drivers, which do not fill the timestamp
field
at all:
- drivers/media/platform/coda.c
- drivers/media/platform/exynos-gsc/gsc-m2m.c
- drivers/media/platform/m2m-deinterlace.c
- drivers/media/platform/mx2_emmaprp.c
- drivers/media/platform/s5p-fimc/fimc-m2m.c
- drivers/media/platform/s5p-g2d.c
- drivers/media/platform/s5p-jpeg/jpeg-core.c
 
The way you did it in your patches left no room for any kind of choice. I
did
comment at least twice about mem-2-mem devices in your RFCs, if I remember
correctly. I think Sylwester was also writing about this. 
Still everything got marked as MONOTONIC. 

If we were to assume that there were no other timestamp types then monotonic
(which is not true, but this was your assumption), then what was the reason
to add this timestamp framework?

Best wishes,
-- 
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

RE: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Kamil Debski]

Hi,

One more comment. Also adding videobuf2 maintainers to CC, as they were not
added in the patch "v4l: Tell user space we're using monotonic timestamps".

> From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> Sent: Saturday, January 19, 2013 6:43 PM
> 
> Hi Kamil,
> 
> Thanks for the patch.
> 
> On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> > Set proper timestamp type in drivers that I am sure that use either
> > MONOTONIC or COPY timestamps. Other drivers will correctly report
> > UNKNOWN timestamp type instead of assuming that all drivers use
> > monotonic timestamps.
> 
> What other kind of timestamps there can be? All drivers (at least those
> not
> mem-to-mem) that do obtain timestamps using system clock use monotonic
> ones.
> 
> I'd think that there should no longer be any drivers using the UNKNOWN
> timestamp type: UNKNOWN is either from monotonic or realtime clock, and
> we just replaced all of them with the monotonic ones. No driver uses
> realtime timestamps anymore.
> 
> How about making MONOTONIC timestamps default instead, or at least
> assigning all drivers something else than UNKNOWN?

In addition to my previous comment - making MONOTONIC default in videobuf2
would be inconsistent. For non videobuf2 drivers the default is UNKNOWN, so
making
MONOTONIC default for videobuf2 drivers is, in my opinion, wrong.

Best wishes,
Kamil Debski

Re: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

['Sakari Ailus']

Hi Kamil,

(Cc'ing Pawel and Marek as well.)

On Mon, Jan 21, 2013 at 03:07:55PM +0100, Kamil Debski wrote:
> Hi,
> 
> > From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> > Sent: Saturday, January 19, 2013 6:43 PM
> > Hi Kamil,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> > > Set proper timestamp type in drivers that I am sure that use either
> > > MONOTONIC or COPY timestamps. Other drivers will correctly report
> > > UNKNOWN timestamp type instead of assuming that all drivers use
> > > monotonic timestamps.
> > 
> > What other kind of timestamps there can be? All drivers (at least those
> > not
> > mem-to-mem) that do obtain timestamps using system clock use monotonic
> > ones.
> 
> Not set. It is not a COPY or MONOTONIC either. Any new or custom kind of
> timestamp, maybe?

Then new timestamp types should be defined for the purpose. Which is indeed
what your patch is about.

And about "COPY" timestamps: if an application wants to use timestamps, it
probably need to know what kind of timestamps they are. "COPY" doesn't
provide that information as such. Only the program that sets the timestamps
for the OUTPUT buffers does.

> > I'd think that there should no longer be any drivers using the UNKNOWN
> > timestamp type: UNKNOWN is either from monotonic or realtime clock, and
> > we just replaced all of them with the monotonic ones. No driver uses
> > realtime timestamps anymore.
> 
> Maybe there should be no drivers using UNKNOWN. But definitely
> there should be no driver reporting MONOTONIC when the timestamp is not
> monotonic.
>  
> > How about making MONOTONIC timestamps default instead, or at least
> > assigning all drivers something else than UNKNOWN?
> 
> So why did you add the UNKNOWN flag?

This is for API compatibility only. Applications running on kernels prior to
the headers of which define timestamp types will not have timestamp type set
(i.e. is zero, which equals to UNKNOWN). There was a lengthy discussion on
the topic back then, and the conclusion was that the kernel version itself
isn't enough to tell what kind of timestamps are provided to the user.

Any new driver shouldn't use UNKNOWN timestamps since in this case the
application would have to know what kind of timestamps the driver uses ---
which is why we now specify it in the API.

> The way I see it - UNKNOWN is the default and the one who coded the driver
> will set it to either MONOTONIC or COPY if it is one of these two. It won't
> be changed otherwise. There are drivers, which do not fill the timestamp
> field
> at all:
> - drivers/media/platform/coda.c
> - drivers/media/platform/exynos-gsc/gsc-m2m.c
> - drivers/media/platform/m2m-deinterlace.c
> - drivers/media/platform/mx2_emmaprp.c
> - drivers/media/platform/s5p-fimc/fimc-m2m.c
> - drivers/media/platform/s5p-g2d.c
> - drivers/media/platform/s5p-jpeg/jpeg-core.c

Excellent point.

But --- should these drivers then fill the timestamp field? Isn't it a bug
in the driver not to do so?

> The way you did it in your patches left no room for any kind of choice. I
> did
> comment at least twice about mem-2-mem devices in your RFCs, if I remember
> correctly. I think Sylwester was also writing about this. 
> Still everything got marked as MONOTONIC. 

I must have missed this in the discussion back then.

> If we were to assume that there were no other timestamp types then monotonic
> (which is not true, but this was your assumption), then what was the reason
> to add this timestamp framework?

For capture devices whose video source has no native timestamps the
timestamps are MONOTONIC, at least until it is made selectable. Other
examples could include video decoders or encoders, but these timestamps will
be entirely different kind, and probably doesn't end up to the timestamp
field.

-- 
Kind regards,

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

RE: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Kamil Debski]

Hi Sakari,

Thank you for your quick reply.

> From: 'Sakari Ailus' [mailto:sakari.ailus@iki.fi]
> Sent: Tuesday, January 22, 2013 11:03 AM
> Hi Kamil,
> 
> (Cc'ing Pawel and Marek as well.)
> 
> On Mon, Jan 21, 2013 at 03:07:55PM +0100, Kamil Debski wrote:
> > Hi,
> >
> > > From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> > > Sent: Saturday, January 19, 2013 6:43 PM Hi Kamil,
> > >
> > > Thanks for the patch.
> > >
> > > On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> > > > Set proper timestamp type in drivers that I am sure that use
> > > > either MONOTONIC or COPY timestamps. Other drivers will correctly
> > > > report UNKNOWN timestamp type instead of assuming that all
> drivers
> > > > use monotonic timestamps.
> > >
> > > What other kind of timestamps there can be? All drivers (at least
> > > those not
> > > mem-to-mem) that do obtain timestamps using system clock use
> > > monotonic ones.
> >
> > Not set. It is not a COPY or MONOTONIC either. Any new or custom kind
> > of timestamp, maybe?
> 
> Then new timestamp types should be defined for the purpose. Which is
> indeed what your patch is about.
> 
> And about "COPY" timestamps: if an application wants to use timestamps,
> it probably need to know what kind of timestamps they are. "COPY"
> doesn't provide that information as such. Only the program that sets
> the timestamps for the OUTPUT buffers does.

For me the COPY type is clear. If the app gets a COPY timestamp on a
buffer (CAPTURE) then it knows that it was copied from the OUTPUT buffer.
If the application did not set the timestamp for OUTPUT buffer, then
it has to cope with the consequences.

> 
> > > I'd think that there should no longer be any drivers using the
> > > UNKNOWN timestamp type: UNKNOWN is either from monotonic or
> realtime
> > > clock, and we just replaced all of them with the monotonic ones. No
> > > driver uses realtime timestamps anymore.
> >
> > Maybe there should be no drivers using UNKNOWN. But definitely there
> > should be no driver reporting MONOTONIC when the timestamp is not
> > monotonic.
> >
> > > How about making MONOTONIC timestamps default instead, or at least
> > > assigning all drivers something else than UNKNOWN?
> >
> > So why did you add the UNKNOWN flag?
> 
> This is for API compatibility only. Applications running on kernels
> prior to the headers of which define timestamp types will not have
> timestamp type set (i.e. is zero, which equals to UNKNOWN). There was a
> lengthy discussion on the topic back then, and the conclusion was that
> the kernel version itself isn't enough to tell what kind of timestamps
> are provided to the user.
> 
> Any new driver shouldn't use UNKNOWN timestamps since in this case the
> application would have to know what kind of timestamps the driver uses
> --- which is why we now specify it in the API.
> 
> > The way I see it - UNKNOWN is the default and the one who coded the
> > driver will set it to either MONOTONIC or COPY if it is one of these
> > two. It won't be changed otherwise. There are drivers, which do not
> > fill the timestamp field at all:
> > - drivers/media/platform/coda.c
> > - drivers/media/platform/exynos-gsc/gsc-m2m.c
> > - drivers/media/platform/m2m-deinterlace.c
> > - drivers/media/platform/mx2_emmaprp.c
> > - drivers/media/platform/s5p-fimc/fimc-m2m.c
> > - drivers/media/platform/s5p-g2d.c
> > - drivers/media/platform/s5p-jpeg/jpeg-core.c
> 
> Excellent point.
> 
> But --- should these drivers then fill the timestamp field? Isn't it a
> bug in the driver not to do so?

Could be. The only thing I am complaining about is that the type should not
be reported as monotonic when it is not. (We can argue that constant is
monotonic, but I think that it is not the point :-) ). This is why I
propose to leave UNKNOWN as the default choice (which it is in case of non
videobuf2 drivers). It is possible to eliminate the UNKNOWN in my opinion,
but
it should be up to the programmer to set the type. Another option is to
eliminate
the UNKNOWN flag in the next, or in two kernel releases. After the drivers
are
changed. Still I prefer to leave the driver's programmer in charge and leave
the
UNKNOWN type.

> 
> > The way you did it in your patches left no room for any kind of
> > choice. I did comment at least twice about mem-2-mem devices in your
> > RFCs, if I remember correctly. I think Sylwester was also writing
> > about this.
> > Still everything got marked as MONOTONIC.
> 
> I must have missed this in the discussion back then.
> 
> > If we were to assume that there were no other timestamp types then
> > monotonic (which is not true, but this was your assumption), then
> what
> > was the reason to add this timestamp framework?
> 
> For capture devices whose video source has no native timestamps the
> timestamps are MONOTONIC, at least until it is made selectable. Other
> examples could include video decoders or encoders, but these timestamps
> will be entirely different kind, and probably doesn't end up to the
> timestamp field.
> 

Thanks.

Best wishes,
-- 
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

Re: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Hans Verkuil]

On Tue 22 January 2013 11:03:03 'Sakari Ailus' wrote:
> Hi Kamil,
> 
> (Cc'ing Pawel and Marek as well.)
> 
> On Mon, Jan 21, 2013 at 03:07:55PM +0100, Kamil Debski wrote:
> > Hi,
> > 
> > > From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> > > Sent: Saturday, January 19, 2013 6:43 PM
> > > Hi Kamil,
> > > 
> > > Thanks for the patch.
> > > 
> > > On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> > > > Set proper timestamp type in drivers that I am sure that use either
> > > > MONOTONIC or COPY timestamps. Other drivers will correctly report
> > > > UNKNOWN timestamp type instead of assuming that all drivers use
> > > > monotonic timestamps.
> > > 
> > > What other kind of timestamps there can be? All drivers (at least those
> > > not
> > > mem-to-mem) that do obtain timestamps using system clock use monotonic
> > > ones.
> > 
> > Not set. It is not a COPY or MONOTONIC either. Any new or custom kind of
> > timestamp, maybe?
> 
> Then new timestamp types should be defined for the purpose. Which is indeed
> what your patch is about.
> 
> And about "COPY" timestamps: if an application wants to use timestamps, it
> probably need to know what kind of timestamps they are. "COPY" doesn't
> provide that information as such. Only the program that sets the timestamps
> for the OUTPUT buffers does.

For these m2m devices the driver does not use the timestamp value at all, it
just copies it from the output stream (i.e. towards the codec) to the input
stream (i.e. from the codec). Since the application got the video from somewhere
the application presumably knows the type of the timestamp.

So I think marking this as COPY is a very good idea. It makes no sense to
put in a specific timestamp type since the driver doesn't control that at all.

Things are quite different when it is the driver that generates the timestamp,
then the application needs to know what sort of timestamp the driver generated
and that should be filled in correctly by the driver.

> > > I'd think that there should no longer be any drivers using the UNKNOWN
> > > timestamp type: UNKNOWN is either from monotonic or realtime clock, and
> > > we just replaced all of them with the monotonic ones. No driver uses
> > > realtime timestamps anymore.
> > 
> > Maybe there should be no drivers using UNKNOWN. But definitely
> > there should be no driver reporting MONOTONIC when the timestamp is not
> > monotonic.
> >  
> > > How about making MONOTONIC timestamps default instead, or at least
> > > assigning all drivers something else than UNKNOWN?
> > 
> > So why did you add the UNKNOWN flag?
> 
> This is for API compatibility only. Applications running on kernels prior to
> the headers of which define timestamp types will not have timestamp type set
> (i.e. is zero, which equals to UNKNOWN). There was a lengthy discussion on
> the topic back then, and the conclusion was that the kernel version itself
> isn't enough to tell what kind of timestamps are provided to the user.
> 
> Any new driver shouldn't use UNKNOWN timestamps since in this case the
> application would have to know what kind of timestamps the driver uses ---
> which is why we now specify it in the API.
> 
> > The way I see it - UNKNOWN is the default and the one who coded the driver
> > will set it to either MONOTONIC or COPY if it is one of these two. It won't
> > be changed otherwise. There are drivers, which do not fill the timestamp
> > field
> > at all:
> > - drivers/media/platform/coda.c
> > - drivers/media/platform/exynos-gsc/gsc-m2m.c
> > - drivers/media/platform/m2m-deinterlace.c
> > - drivers/media/platform/mx2_emmaprp.c
> > - drivers/media/platform/s5p-fimc/fimc-m2m.c
> > - drivers/media/platform/s5p-g2d.c
> > - drivers/media/platform/s5p-jpeg/jpeg-core.c
> 
> Excellent point.
> 
> But --- should these drivers then fill the timestamp field? Isn't it a bug
> in the driver not to do so?

Not for mem2mem devices. You give it a frame with an associate timestamp which
is copied to the (de)coded frame. The timestamps here indicate when the original
frame was generated, which is information you want to keep.

Note that the COPY timestamp assumes that there is a 1-to-1 mapping between
an input frame and an output frame. If that's no longer the case (e.g. if
a sequence of discrete frames is encoded as an MPEG bitstream), then drivers
should just generate MONOTONIC timestamps. Unlikely to be very useful, but
if nothing else it might be used for some performance measurements.

> > The way you did it in your patches left no room for any kind of choice. I
> > did
> > comment at least twice about mem-2-mem devices in your RFCs, if I remember
> > correctly. I think Sylwester was also writing about this. 
> > Still everything got marked as MONOTONIC. 
> 
> I must have missed this in the discussion back then.
> 
> > If we were to assume that there were no other timestamp types then monotonic
> > (which is not true, but this was your assumption), then what was the reason
> > to add this timestamp framework?
> 
> For capture devices whose video source has no native timestamps the
> timestamps are MONOTONIC, at least until it is made selectable. Other
> examples could include video decoders or encoders, but these timestamps will
> be entirely different kind, and probably doesn't end up to the timestamp
> field.

Regards,

	Hans

Re: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Sylwester Nawrocki]

Hi,

On 01/21/2013 03:07 PM, Kamil Debski wrote:
>> How about making MONOTONIC timestamps default instead, or at least
>> assigning all drivers something else than UNKNOWN?
> 
> So why did you add the UNKNOWN flag?
> 
> The way I see it - UNKNOWN is the default and the one who coded the driver
> will set it to either MONOTONIC or COPY if it is one of these two. It won't
> be changed otherwise. There are drivers, which do not fill the timestamp
> field
> at all:
> - drivers/media/platform/coda.c
> - drivers/media/platform/exynos-gsc/gsc-m2m.c

Hmm, there is already a patch queued for this driver. It was intended
for v3.8 but has been delayed to 3.9.

http://git.linuxtv.org/media_tree.git/commitdiff/f60e160e126bdd8f0d928cd8b3fce54659597394

> - drivers/media/platform/m2m-deinterlace.c
> - drivers/media/platform/mx2_emmaprp.c
> - drivers/media/platform/s5p-fimc/fimc-m2m.c
> - drivers/media/platform/s5p-g2d.c
> - drivers/media/platform/s5p-jpeg/jpeg-core.c
>  
> The way you did it in your patches left no room for any kind of choice. I
> did
> comment at least twice about mem-2-mem devices in your RFCs, if I remember
> correctly. I think Sylwester was also writing about this. 
> Still everything got marked as MONOTONIC. 
> 
> If we were to assume that there were no other timestamp types then monotonic
> (which is not true, but this was your assumption), then what was the reason
> to add this timestamp framework?

Hmm, we could likely leave MONOTONIC as the default timestamp type. It
doesn't really matter what is the default, as long as drivers are provided
with an API to override it.

The reason why the above drivers don't do anything with v4l2_buffer::timestamp
field is there is no clear definitions at the specification for mem-to-mem
devices. We are working here on a Video Memory-to-memory Interface DocBook
documentation.

I think we will need a way to tell user space that timestamps are copied
from OUTPUT to CAPTURE buffer queue. At least that's what seems more
useful for applications. i.e. copying timestamps, rather than filling them
with the monotonic clock value.

OTOH I'm not certain what's the main purpose of such copied timestamps, is
it to identify which CAPTURE buffer comes from which OUTPUT buffer ?

--

Regards,
Sylwester

RE: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Kamil Debski]

Hi Hans,

Thank you for your comments.

> From: Hans Verkuil [mailto:hansverk@cisco.com]
> Sent: Tuesday, January 22, 2013 11:35 AM
> 
> On Tue 22 January 2013 11:03:03 'Sakari Ailus' wrote:
> > Hi Kamil,
> >
> > (Cc'ing Pawel and Marek as well.)
> >
> > On Mon, Jan 21, 2013 at 03:07:55PM +0100, Kamil Debski wrote:
> > > Hi,
> > >
> > > > From: Sakari Ailus [mailto:sakari.ailus@iki.fi]
> > > > Sent: Saturday, January 19, 2013 6:43 PM Hi Kamil,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> > > > > Set proper timestamp type in drivers that I am sure that use
> > > > > either MONOTONIC or COPY timestamps. Other drivers will
> > > > > correctly report UNKNOWN timestamp type instead of assuming
> that
> > > > > all drivers use monotonic timestamps.
> > > >
> > > > What other kind of timestamps there can be? All drivers (at least
> > > > those not
> > > > mem-to-mem) that do obtain timestamps using system clock use
> > > > monotonic ones.
> > >
> > > Not set. It is not a COPY or MONOTONIC either. Any new or custom
> > > kind of timestamp, maybe?
> >
> > Then new timestamp types should be defined for the purpose. Which is
> > indeed what your patch is about.
> >
> > And about "COPY" timestamps: if an application wants to use
> > timestamps, it probably need to know what kind of timestamps they are.
> > "COPY" doesn't provide that information as such. Only the program
> that
> > sets the timestamps for the OUTPUT buffers does.
> 
> For these m2m devices the driver does not use the timestamp value at
> all, it just copies it from the output stream (i.e. towards the codec)
> to the input stream (i.e. from the codec). Since the application got
> the video from somewhere the application presumably knows the type of
> the timestamp.
> 
> So I think marking this as COPY is a very good idea. It makes no sense
> to put in a specific timestamp type since the driver doesn't control
> that at all.
> 
> Things are quite different when it is the driver that generates the
> timestamp, then the application needs to know what sort of timestamp
> the driver generated and that should be filled in correctly by the
> driver.
> 
> > > > I'd think that there should no longer be any drivers using the
> > > > UNKNOWN timestamp type: UNKNOWN is either from monotonic or
> > > > realtime clock, and we just replaced all of them with the
> > > > monotonic ones. No driver uses realtime timestamps anymore.
> > >
> > > Maybe there should be no drivers using UNKNOWN. But definitely
> there
> > > should be no driver reporting MONOTONIC when the timestamp is not
> > > monotonic.
> > >
> > > > How about making MONOTONIC timestamps default instead, or at
> least
> > > > assigning all drivers something else than UNKNOWN?
> > >
> > > So why did you add the UNKNOWN flag?
> >
> > This is for API compatibility only. Applications running on kernels
> > prior to the headers of which define timestamp types will not have
> > timestamp type set (i.e. is zero, which equals to UNKNOWN). There was
> > a lengthy discussion on the topic back then, and the conclusion was
> > that the kernel version itself isn't enough to tell what kind of
> timestamps are provided to the user.
> >
> > Any new driver shouldn't use UNKNOWN timestamps since in this case
> the
> > application would have to know what kind of timestamps the driver
> uses
> > --- which is why we now specify it in the API.
> >
> > > The way I see it - UNKNOWN is the default and the one who coded the
> > > driver will set it to either MONOTONIC or COPY if it is one of
> these
> > > two. It won't be changed otherwise. There are drivers, which do not
> > > fill the timestamp field at all:
> > > - drivers/media/platform/coda.c
> > > - drivers/media/platform/exynos-gsc/gsc-m2m.c
> > > - drivers/media/platform/m2m-deinterlace.c
> > > - drivers/media/platform/mx2_emmaprp.c
> > > - drivers/media/platform/s5p-fimc/fimc-m2m.c
> > > - drivers/media/platform/s5p-g2d.c
> > > - drivers/media/platform/s5p-jpeg/jpeg-core.c
> >
> > Excellent point.
> >
> > But --- should these drivers then fill the timestamp field? Isn't it
> a
> > bug in the driver not to do so?
> 
> Not for mem2mem devices. You give it a frame with an associate
> timestamp which is copied to the (de)coded frame. The timestamps here
> indicate when the original frame was generated, which is information
> you want to keep.
> 
> Note that the COPY timestamp assumes that there is a 1-to-1 mapping
> between an input frame and an output frame. If that's no longer the
> case (e.g. if a sequence of discrete frames is encoded as an MPEG
> bitstream), then drivers should just generate MONOTONIC timestamps.
> Unlikely to be very useful, but if nothing else it might be used for
> some performance measurements.

In case the relationship is not 1-1 it gets quite complicated, but the
copy method can be still used. In that case if N CAPTURE buffers are
produced from one OUTPUT buffer then all of them have the same timestamp.
They can be distinguished by the sequence number. In the opposite case
- when N OUTPUT buffers create one CAPTURE buffer it will have the
timestamp of the last OUTPUT buffer processed.

> 
> > > The way you did it in your patches left no room for any kind of
> > > choice. I did comment at least twice about mem-2-mem devices in
> your
> > > RFCs, if I remember correctly. I think Sylwester was also writing
> > > about this.
> > > Still everything got marked as MONOTONIC.
> >
> > I must have missed this in the discussion back then.
> >
> > > If we were to assume that there were no other timestamp types then
> > > monotonic (which is not true, but this was your assumption), then
> > > what was the reason to add this timestamp framework?
> >

[snip]


Best wishes,
-- 
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

RE: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Kamil Debski]

Hi,
> From: Sylwester Nawrocki [mailto:s.nawrocki@samsung.com]
> Sent: Tuesday, January 22, 2013 11:38 AM
> 
> Hi,
> 
> On 01/21/2013 03:07 PM, Kamil Debski wrote:
> >> How about making MONOTONIC timestamps default instead, or at least
> >> assigning all drivers something else than UNKNOWN?
> >
> > So why did you add the UNKNOWN flag?
> >
> > The way I see it - UNKNOWN is the default and the one who coded the
> > driver will set it to either MONOTONIC or COPY if it is one of these
> > two. It won't be changed otherwise. There are drivers, which do not
> > fill the timestamp field at all:
> > - drivers/media/platform/coda.c
> > - drivers/media/platform/exynos-gsc/gsc-m2m.c
> 
> Hmm, there is already a patch queued for this driver. It was intended
> for v3.8 but has been delayed to 3.9.
> 
> http://git.linuxtv.org/media_tree.git/commitdiff/f60e160e126bdd8f0d928c
> d8b3fce54659597394
> 
> > - drivers/media/platform/m2m-deinterlace.c
> > - drivers/media/platform/mx2_emmaprp.c
> > - drivers/media/platform/s5p-fimc/fimc-m2m.c
> > - drivers/media/platform/s5p-g2d.c
> > - drivers/media/platform/s5p-jpeg/jpeg-core.c
> >
> > The way you did it in your patches left no room for any kind of
> > choice. I did comment at least twice about mem-2-mem devices in your
> > RFCs, if I remember correctly. I think Sylwester was also writing
> > about this.
> > Still everything got marked as MONOTONIC.
> >
> > If we were to assume that there were no other timestamp types then
> > monotonic (which is not true, but this was your assumption), then
> what
> > was the reason to add this timestamp framework?
> 
> Hmm, we could likely leave MONOTONIC as the default timestamp type. It
> doesn't really matter what is the default, as long as drivers are
> provided with an API to override it.
> 
> The reason why the above drivers don't do anything with
> v4l2_buffer::timestamp field is there is no clear definitions at the
> specification for mem-to-mem devices. We are working here on a Video
> Memory-to-memory Interface DocBook documentation.
> 
> I think we will need a way to tell user space that timestamps are
> copied from OUTPUT to CAPTURE buffer queue. At least that's what seems
> more useful for applications. i.e. copying timestamps, rather than
> filling them with the monotonic clock value.
> 
> OTOH I'm not certain what's the main purpose of such copied timestamps,
> is it to identify which CAPTURE buffer comes from which OUTPUT buffer ?
> 

Yes, indeed. This is especially useful when the CAPTURE buffers can be
returned in an order different than the order of corresponding OUTPUT
buffers.

Best wishes,
-- 
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

Re: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

['Sakari Ailus']

Hi Kamil,

On Tue, Jan 22, 2013 at 06:58:09PM +0100, Kamil Debski wrote:
...
> > OTOH I'm not certain what's the main purpose of such copied timestamps,
> > is it to identify which CAPTURE buffer comes from which OUTPUT buffer ?
> > 
> 
> Yes, indeed. This is especially useful when the CAPTURE buffers can be
> returned in an order different than the order of corresponding OUTPUT
> buffers.

How about sequence numbers then? Shouldn't that be also copied?

If you're interested in the order alone, comparing the sequence numbers is a
better way to figure out the order. That does require strict one-to-one
mapping between the output and capture buffers, though, and that does not
help in knowing when it might be a good time to display a frame, for
instance.

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

Re: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Laurent Pinchart]

Hi Kamil,

On Tuesday 22 January 2013 11:24:09 Kamil Debski wrote:
> On Tuesday, January 22, 2013 11:03 AM Sakari Ailus wrote:
> > On Mon, Jan 21, 2013 at 03:07:55PM +0100, Kamil Debski wrote:
> > > On Saturday, January 19, 2013 6:43 PM Sakari Ailus wrote:
> > > > On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> > > > > Set proper timestamp type in drivers that I am sure that use
> > > > > either MONOTONIC or COPY timestamps. Other drivers will correctly
> > > > > report UNKNOWN timestamp type instead of assuming that all
> > > > > drivers use monotonic timestamps.
> > > > 
> > > > What other kind of timestamps there can be? All drivers (at least
> > > > those not mem-to-mem) that do obtain timestamps using system clock use
> > > > monotonic ones.
> > > 
> > > Not set. It is not a COPY or MONOTONIC either. Any new or custom kind
> > > of timestamp, maybe?
> > 
> > Then new timestamp types should be defined for the purpose. Which is
> > indeed what your patch is about.
> > 
> > And about "COPY" timestamps: if an application wants to use timestamps,
> > it probably need to know what kind of timestamps they are. "COPY"
> > doesn't provide that information as such. Only the program that sets
> > the timestamps for the OUTPUT buffers does.
> 
> For me the COPY type is clear. If the app gets a COPY timestamp on a
> buffer (CAPTURE) then it knows that it was copied from the OUTPUT buffer.
> If the application did not set the timestamp for OUTPUT buffer, then
> it has to cope with the consequences.
> 
> > > > I'd think that there should no longer be any drivers using the
> > > > UNKNOWN timestamp type: UNKNOWN is either from monotonic or realtime
> > > > clock, and we just replaced all of them with the monotonic ones. No
> > > > driver uses realtime timestamps anymore.
> > > 
> > > Maybe there should be no drivers using UNKNOWN. But definitely there
> > > should be no driver reporting MONOTONIC when the timestamp is not
> > > monotonic.
> > > 
> > > > How about making MONOTONIC timestamps default instead, or at least
> > > > assigning all drivers something else than UNKNOWN?
> > > 
> > > So why did you add the UNKNOWN flag?
> > 
> > This is for API compatibility only. Applications running on kernels
> > prior to the headers of which define timestamp types will not have
> > timestamp type set (i.e. is zero, which equals to UNKNOWN). There was a
> > lengthy discussion on the topic back then, and the conclusion was that
> > the kernel version itself isn't enough to tell what kind of timestamps
> > are provided to the user.
> > 
> > Any new driver shouldn't use UNKNOWN timestamps since in this case the
> > application would have to know what kind of timestamps the driver uses
> > --- which is why we now specify it in the API.
> > 
> > > The way I see it - UNKNOWN is the default and the one who coded the
> > > driver will set it to either MONOTONIC or COPY if it is one of these
> > > two. It won't be changed otherwise. There are drivers, which do not
> > > fill the timestamp field at all:
> > > - drivers/media/platform/coda.c
> > > - drivers/media/platform/exynos-gsc/gsc-m2m.c
> > > - drivers/media/platform/m2m-deinterlace.c
> > > - drivers/media/platform/mx2_emmaprp.c
> > > - drivers/media/platform/s5p-fimc/fimc-m2m.c
> > > - drivers/media/platform/s5p-g2d.c
> > > - drivers/media/platform/s5p-jpeg/jpeg-core.c
> > 
> > Excellent point.
> > 
> > But --- should these drivers then fill the timestamp field? Isn't it a
> > bug in the driver not to do so?
> 
> Could be. The only thing I am complaining about is that the type should not
> be reported as monotonic when it is not.

Of course.

> (We can argue that constant is monotonic, but I think that it is not the
> point :-) ). This is why I propose to leave UNKNOWN as the default choice
> (which it is in case of non videobuf2 drivers). It is possible to eliminate
> the UNKNOWN in my opinion, but it should be up to the programmer to set the
> type. Another option is to eliminate the UNKNOWN flag in the next, or in two
> kernel releases. After the drivers are changed. Still I prefer to leave the
> driver's programmer in charge and leave the UNKNOWN type.

I think that Sakari's point is that all non mem-to-mem drivers should use the 
MONOTONIC timestamps type. UNKNOWN, in the non mem-to-mem case, is only meant 
for compatibility with older kernels. We should thus default to MONOTONIC and 
let drivers select a different timestamp type when needed (for mem-to-mem 
drivers for instance).

> > > The way you did it in your patches left no room for any kind of
> > > choice. I did comment at least twice about mem-2-mem devices in your
> > > RFCs, if I remember correctly. I think Sylwester was also writing
> > > about this. Still everything got marked as MONOTONIC.
> > 
> > I must have missed this in the discussion back then.
> > 
> > > If we were to assume that there were no other timestamp types then
> > > monotonic (which is not true, but this was your assumption), then
> > > what was the reason to add this timestamp framework?
> > 
> > For capture devices whose video source has no native timestamps the
> > timestamps are MONOTONIC, at least until it is made selectable. Other
> > examples could include video decoders or encoders, but these timestamps
> > will be entirely different kind, and probably doesn't end up to the
> > timestamp field.

-- 
Regards,

Laurent Pinchart

RE: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Kamil Debski]

Hi,

> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Wednesday, January 23, 2013 1:26 AM
> 
> Hi Kamil,
> 
> On Tuesday 22 January 2013 11:24:09 Kamil Debski wrote:
> > On Tuesday, January 22, 2013 11:03 AM Sakari Ailus wrote:
> > > On Mon, Jan 21, 2013 at 03:07:55PM +0100, Kamil Debski wrote:
> > > > On Saturday, January 19, 2013 6:43 PM Sakari Ailus wrote:
> > > > > On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> > > > > > Set proper timestamp type in drivers that I am sure that use
> > > > > > either MONOTONIC or COPY timestamps. Other drivers will
> > > > > > correctly report UNKNOWN timestamp type instead of assuming
> > > > > > that all drivers use monotonic timestamps.
> > > > >
> > > > > What other kind of timestamps there can be? All drivers (at
> > > > > least those not mem-to-mem) that do obtain timestamps using
> > > > > system clock use monotonic ones.
> > > >
> > > > Not set. It is not a COPY or MONOTONIC either. Any new or custom
> > > > kind of timestamp, maybe?
> > >
> > > Then new timestamp types should be defined for the purpose. Which
> is
> > > indeed what your patch is about.
> > >
> > > And about "COPY" timestamps: if an application wants to use
> > > timestamps, it probably need to know what kind of timestamps they
> are. "COPY"
> > > doesn't provide that information as such. Only the program that
> sets
> > > the timestamps for the OUTPUT buffers does.
> >
> > For me the COPY type is clear. If the app gets a COPY timestamp on a
> > buffer (CAPTURE) then it knows that it was copied from the OUTPUT
> buffer.
> > If the application did not set the timestamp for OUTPUT buffer, then
> > it has to cope with the consequences.
> >
> > > > > I'd think that there should no longer be any drivers using the
> > > > > UNKNOWN timestamp type: UNKNOWN is either from monotonic or
> > > > > realtime clock, and we just replaced all of them with the
> > > > > monotonic ones. No driver uses realtime timestamps anymore.
> > > >
> > > > Maybe there should be no drivers using UNKNOWN. But definitely
> > > > there should be no driver reporting MONOTONIC when the timestamp
> > > > is not monotonic.
> > > >
> > > > > How about making MONOTONIC timestamps default instead, or at
> > > > > least assigning all drivers something else than UNKNOWN?
> > > >
> > > > So why did you add the UNKNOWN flag?
> > >
> > > This is for API compatibility only. Applications running on kernels
> > > prior to the headers of which define timestamp types will not have
> > > timestamp type set (i.e. is zero, which equals to UNKNOWN). There
> > > was a lengthy discussion on the topic back then, and the conclusion
> > > was that the kernel version itself isn't enough to tell what kind
> of
> > > timestamps are provided to the user.
> > >
> > > Any new driver shouldn't use UNKNOWN timestamps since in this case
> > > the application would have to know what kind of timestamps the
> > > driver uses
> > > --- which is why we now specify it in the API.
> > >
> > > > The way I see it - UNKNOWN is the default and the one who coded
> > > > the driver will set it to either MONOTONIC or COPY if it is one
> of
> > > > these two. It won't be changed otherwise. There are drivers,
> which
> > > > do not fill the timestamp field at all:
> > > > - drivers/media/platform/coda.c
> > > > - drivers/media/platform/exynos-gsc/gsc-m2m.c
> > > > - drivers/media/platform/m2m-deinterlace.c
> > > > - drivers/media/platform/mx2_emmaprp.c
> > > > - drivers/media/platform/s5p-fimc/fimc-m2m.c
> > > > - drivers/media/platform/s5p-g2d.c
> > > > - drivers/media/platform/s5p-jpeg/jpeg-core.c
> > >
> > > Excellent point.
> > >
> > > But --- should these drivers then fill the timestamp field? Isn't
> it
> > > a bug in the driver not to do so?
> >
> > Could be. The only thing I am complaining about is that the type
> > should not be reported as monotonic when it is not.
> 
> Of course.
> 
> > (We can argue that constant is monotonic, but I think that it is not
> > the point :-) ). This is why I propose to leave UNKNOWN as the
> default
> > choice (which it is in case of non videobuf2 drivers). It is possible
> > to eliminate the UNKNOWN in my opinion, but it should be up to the
> > programmer to set the type. Another option is to eliminate the
> UNKNOWN
> > flag in the next, or in two kernel releases. After the drivers are
> > changed. Still I prefer to leave the driver's programmer in charge
> and leave the UNKNOWN type.
> 
> I think that Sakari's point is that all non mem-to-mem drivers should
> use the MONOTONIC timestamps type. 

Good point. I didn't say they should not. It's all ok with me.

> UNKNOWN, in the non mem-to-mem case,
> is only meant for compatibility with older kernels. We should thus
> default to MONOTONIC and let drivers select a different timestamp type
> when needed (for mem-to-mem drivers for instance).

I can agree with that if the MONOTONIC is a real default. Not videobuf2
exclusive default. In case of non vb2 drivers the default (driver does
not set the timestamp type flag at all) is UNKNOWN.

I would expect that if no action on the programmer side is taken then
videobuf2 and non videobuf2 drivers would act in a similar way.

Laurent, I really think that it may confuse someone. I think the
MONOTONIC timestamp type should indicate a driver that supplies a
monotonic timestamp. Not a newer kernel that sets it as default (and
it does only in case of vb2 drivers, not others).

> 
> > > > The way you did it in your patches left no room for any kind of
> > > > choice. I did comment at least twice about mem-2-mem devices in
> > > > your RFCs, if I remember correctly. I think Sylwester was also
> > > > writing about this. Still everything got marked as MONOTONIC.
> > >
> > > I must have missed this in the discussion back then.
> > >
> > > > If we were to assume that there were no other timestamp types
> then
> > > > monotonic (which is not true, but this was your assumption), then
> > > > what was the reason to add this timestamp framework?
> > >
> > > For capture devices whose video source has no native timestamps the
> > > timestamps are MONOTONIC, at least until it is made selectable.
> > > Other examples could include video decoders or encoders, but these
> > > timestamps will be entirely different kind, and probably doesn't
> end
> > > up to the timestamp field.
> 
> --
> Regards,
> 
> Laurent Pinchart

Best wishes,
-- 
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

RE: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Kamil Debski]

Hi,

> From: 'Sakari Ailus' [mailto:sakari.ailus@iki.fi]
> Sent: Tuesday, January 22, 2013 7:45 PM
> 
> Hi Kamil,
> 
> On Tue, Jan 22, 2013 at 06:58:09PM +0100, Kamil Debski wrote:
> ...
> > > OTOH I'm not certain what's the main purpose of such copied
> > > timestamps, is it to identify which CAPTURE buffer comes from which
> OUTPUT buffer ?
> > >
> >
> > Yes, indeed. This is especially useful when the CAPTURE buffers can
> be
> > returned in an order different than the order of corresponding OUTPUT
> > buffers.
> 
> How about sequence numbers then? Shouldn't that be also copied?
> 
> If you're interested in the order alone, comparing the sequence numbers
> is a better way to figure out the order. That does require strict one-
> to-one mapping between the output and capture buffers, though, and that
> does not help in knowing when it might be a good time to display a
> frame, for instance.
> 

The idea behind copying the timestamp was that it can propagate the
timestamp
from the video stream. If this info is absent application can generate them
and
still be able to connect OUTPUT and CAPTURE frames. 

While decoding MPEG4 it is possible to get a compressed frame saying
"nothing
to do here, move along" (which basically means that the last frame should be
repeated).
This is where increasing sequence number comes in handy. Even if no
timestamp
is set the application can detect such empty frames and display the decoded
video
correctly.

Copying sequence numbers was already discussed in January 2012 IIRC. The
recommendation
was that the device keeps an internal sequence counter and assigns it to
both OUTPUT and
CAPTURE buffers, so they can be associated.

I think that we're diverting from the main topic of this discussion. My
patches fix a
problem and the only thing that, we cannot agree about is what the default
timestamp type
should be.

Best wishes,
-- 
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

Re: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Hans Verkuil]

On Wed 23 January 2013 09:47:06 Kamil Debski wrote:
> Hi,
> 
> > From: 'Sakari Ailus' [mailto:sakari.ailus@iki.fi]
> > Sent: Tuesday, January 22, 2013 7:45 PM
> > 
> > Hi Kamil,
> > 
> > On Tue, Jan 22, 2013 at 06:58:09PM +0100, Kamil Debski wrote:
> > ...
> > > > OTOH I'm not certain what's the main purpose of such copied
> > > > timestamps, is it to identify which CAPTURE buffer comes from which
> > OUTPUT buffer ?
> > > >
> > >
> > > Yes, indeed. This is especially useful when the CAPTURE buffers can
> > be
> > > returned in an order different than the order of corresponding OUTPUT
> > > buffers.
> > 
> > How about sequence numbers then? Shouldn't that be also copied?
> > 
> > If you're interested in the order alone, comparing the sequence numbers
> > is a better way to figure out the order. That does require strict one-
> > to-one mapping between the output and capture buffers, though, and that
> > does not help in knowing when it might be a good time to display a
> > frame, for instance.
> > 
> 
> The idea behind copying the timestamp was that it can propagate the
> timestamp
> from the video stream. If this info is absent application can generate them
> and
> still be able to connect OUTPUT and CAPTURE frames. 
> 
> While decoding MPEG4 it is possible to get a compressed frame saying
> "nothing
> to do here, move along" (which basically means that the last frame should be
> repeated).
> This is where increasing sequence number comes in handy. Even if no
> timestamp
> is set the application can detect such empty frames and display the decoded
> video
> correctly.
> 
> Copying sequence numbers was already discussed in January 2012 IIRC. The
> recommendation
> was that the device keeps an internal sequence counter and assigns it to
> both OUTPUT and
> CAPTURE buffers, so they can be associated.
> 
> I think that we're diverting from the main topic of this discussion. My
> patches fix a
> problem and the only thing that, we cannot agree about is what the default
> timestamp type
> should be.

Right. And in my view there should be no default timestamp. Drivers should
always select MONOTONIC or COPY, and never UNKNOWN. The vb2 code should
check for that and issue a WARN_ON if no proper timestamp type was provided.

v4l2-compliance already checks for that as well.

Regards,

	Hans

Re: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

['Sakari Ailus']

On Wed, Jan 23, 2013 at 10:03:47AM +0100, Hans Verkuil wrote:
...
> Right. And in my view there should be no default timestamp. Drivers should
> always select MONOTONIC or COPY, and never UNKNOWN. The vb2 code should
> check for that and issue a WARN_ON if no proper timestamp type was provided.
> 
> v4l2-compliance already checks for that as well.

I agree with that.

Speaking of non-vb2 drivers --- I guess there's no reason for a driver not
to use vb2 these days. There are actually already multple reasons to use it
instead.

So, vb2 drivers should choose the timestamps, and non-vb2 drivers... well,
we shouldn't have more, but in case we do, they _must_ set the timestamp
type, as there's no "default" since the relevant IOCTLs are handled by the
driver itself rather than the V4L2 framework.

-- 
Kind regards,

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

RE: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Kamil Debski]

Hi,

> From: 'Sakari Ailus' [mailto:sakari.ailus@iki.fi]
> Sent: Wednesday, January 23, 2013 2:55 PM
> 
> On Wed, Jan 23, 2013 at 10:03:47AM +0100, Hans Verkuil wrote:
> ...
> > Right. And in my view there should be no default timestamp. Drivers
> > should always select MONOTONIC or COPY, and never UNKNOWN. The vb2
> > code should check for that and issue a WARN_ON if no proper timestamp
> type was provided.
> >
> > v4l2-compliance already checks for that as well.
> 
> I agree with that.

I also agree. I will post patches that issue a WARN_ON.

> Speaking of non-vb2 drivers --- I guess there's no reason for a driver
> not to use vb2 these days. There are actually already multple reasons
> to use it instead.
> 
> So, vb2 drivers should choose the timestamps, and non-vb2 drivers...
> well, we shouldn't have more, but in case we do, they _must_ set the
> timestamp type, as there's no "default" since the relevant IOCTLs are
> handled by the driver itself rather than the V4L2 framework.
> 


Best wishes,
-- 
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

Re: [PATCH 3/3] v4l: Set proper timestamp type in selected drivers which use videobuf2

[Laurent Pinchart]

Hi Kamil,

On Wednesday 23 January 2013 09:46:53 Kamil Debski wrote:
> On Wednesday, January 23, 2013 1:26 AM Laurent Pinchart wrote:
> > On Tuesday 22 January 2013 11:24:09 Kamil Debski wrote:
> > > On Tuesday, January 22, 2013 11:03 AM Sakari Ailus wrote:
> > > > On Mon, Jan 21, 2013 at 03:07:55PM +0100, Kamil Debski wrote:
> > > > > On Saturday, January 19, 2013 6:43 PM Sakari Ailus wrote:
> > > > > > On Mon, Jan 14, 2013 at 10:36:04AM +0100, Kamil Debski wrote:
> > > > > > > Set proper timestamp type in drivers that I am sure that use
> > > > > > > either MONOTONIC or COPY timestamps. Other drivers will
> > > > > > > correctly report UNKNOWN timestamp type instead of assuming
> > > > > > > that all drivers use monotonic timestamps.
> > > > > > 
> > > > > > What other kind of timestamps there can be? All drivers (at
> > > > > > least those not mem-to-mem) that do obtain timestamps using
> > > > > > system clock use monotonic ones.
> > > > > 
> > > > > Not set. It is not a COPY or MONOTONIC either. Any new or custom
> > > > > kind of timestamp, maybe?
> > > > 
> > > > Then new timestamp types should be defined for the purpose. Which
> > > > is indeed what your patch is about.
> > > > 
> > > > And about "COPY" timestamps: if an application wants to use
> > > > timestamps, it probably need to know what kind of timestamps they
> > > > are. "COPY" doesn't provide that information as such. Only the program 
> > > > that sets the timestamps for the OUTPUT buffers does.
> > > 
> > > For me the COPY type is clear. If the app gets a COPY timestamp on a
> > > buffer (CAPTURE) then it knows that it was copied from the OUTPUT
> > > buffer.
> > >
> > > If the application did not set the timestamp for OUTPUT buffer, then
> > > it has to cope with the consequences.
> > > 
> > > > > > I'd think that there should no longer be any drivers using the
> > > > > > UNKNOWN timestamp type: UNKNOWN is either from monotonic or
> > > > > > realtime clock, and we just replaced all of them with the
> > > > > > monotonic ones. No driver uses realtime timestamps anymore.
> > > > > 
> > > > > Maybe there should be no drivers using UNKNOWN. But definitely
> > > > > there should be no driver reporting MONOTONIC when the timestamp
> > > > > is not monotonic.
> > > > > 
> > > > > > How about making MONOTONIC timestamps default instead, or at
> > > > > > least assigning all drivers something else than UNKNOWN?
> > > > > 
> > > > > So why did you add the UNKNOWN flag?
> > > > 
> > > > This is for API compatibility only. Applications running on kernels
> > > > prior to the headers of which define timestamp types will not have
> > > > timestamp type set (i.e. is zero, which equals to UNKNOWN). There
> > > > was a lengthy discussion on the topic back then, and the conclusion
> > > > was that the kernel version itself isn't enough to tell what kind of
> > > > timestamps are provided to the user.
> > > > 
> > > > Any new driver shouldn't use UNKNOWN timestamps since in this case
> > > > the application would have to know what kind of timestamps the
> > > > driver uses --- which is why we now specify it in the API.
> > > > 
> > > > > The way I see it - UNKNOWN is the default and the one who coded
> > > > > the driver will set it to either MONOTONIC or COPY if it is one of
> > > > > these two. It won't be changed otherwise. There are drivers, which
> > > > > do not fill the timestamp field at all:
> > > > > - drivers/media/platform/coda.c
> > > > > - drivers/media/platform/exynos-gsc/gsc-m2m.c
> > > > > - drivers/media/platform/m2m-deinterlace.c
> > > > > - drivers/media/platform/mx2_emmaprp.c
> > > > > - drivers/media/platform/s5p-fimc/fimc-m2m.c
> > > > > - drivers/media/platform/s5p-g2d.c
> > > > > - drivers/media/platform/s5p-jpeg/jpeg-core.c
> > > > 
> > > > Excellent point.
> > > > 
> > > > But --- should these drivers then fill the timestamp field? Isn't it
> > > > a bug in the driver not to do so?
> > > 
> > > Could be. The only thing I am complaining about is that the type
> > > should not be reported as monotonic when it is not.
> > 
> > Of course.
> > 
> > > (We can argue that constant is monotonic, but I think that it is not
> > > the point :-) ). This is why I propose to leave UNKNOWN as the default
> > > choice (which it is in case of non videobuf2 drivers). It is possible
> > > to eliminate the UNKNOWN in my opinion, but it should be up to the
> > > programmer to set the type. Another option is to eliminate the UNKNOWN
> > > flag in the next, or in two kernel releases. After the drivers are
> > > changed. Still I prefer to leave the driver's programmer in charge and
> > > leave the UNKNOWN type.
> > 
> > I think that Sakari's point is that all non mem-to-mem drivers should
> > use the MONOTONIC timestamps type.
> 
> Good point. I didn't say they should not. It's all ok with me.
> 
> > UNKNOWN, in the non mem-to-mem case,
> > is only meant for compatibility with older kernels. We should thus
> > default to MONOTONIC and let drivers select a different timestamp type
> > when needed (for mem-to-mem drivers for instance).
> 
> I can agree with that if the MONOTONIC is a real default. Not videobuf2
> exclusive default. In case of non vb2 drivers the default (driver does
> not set the timestamp type flag at all) is UNKNOWN.
> 
> I would expect that if no action on the programmer side is taken then
> videobuf2 and non videobuf2 drivers would act in a similar way.
> 
> Laurent, I really think that it may confuse someone. I think the MONOTONIC
> timestamp type should indicate a driver that supplies a monotonic timestamp.
> Not a newer kernel that sets it as default (and it does only in case of vb2
> drivers, not others).

If I understand it correctly, the timestamp flag reports the timestamp field 
format to the application. Possible values are

- UNKNOWN: No information is available about the timestamp format.
- MONOTONIC: The timestamp is taken from the monotonic clock.

UNKNOWN was the value reported by all drivers before v3.7, as no information 
was available about timestamps. Starting from v3.7 all capture drivers have 
been converted to supply monotonic timestamps, and thus to report the 
MONOTONIC timestamp format (videobuf and videobuf2 now hardcode the MONOTONIC 
flag, and drivers that use neither set the flag manually).

As mem-to-mem drivers need to report a different timestamp we can't hardcode 
the MONOTONIC flag anymore in videobuf(2). A patch that allows selecting the 
timestamp type in videobuf(2) thus makes sense, but we don't want to revert to 
the UNKNOWN timestamp type for all capture drivers.

> > > > > The way you did it in your patches left no room for any kind of
> > > > > choice. I did comment at least twice about mem-2-mem devices in
> > > > > your RFCs, if I remember correctly. I think Sylwester was also
> > > > > writing about this. Still everything got marked as MONOTONIC.
> > > > 
> > > > I must have missed this in the discussion back then.
> > > > 
> > > > > If we were to assume that there were no other timestamp types then
> > > > > monotonic (which is not true, but this was your assumption), then
> > > > > what was the reason to add this timestamp framework?
> > > > 
> > > > For capture devices whose video source has no native timestamps the
> > > > timestamps are MONOTONIC, at least until it is made selectable.
> > > > Other examples could include video decoders or encoders, but these
> > > > timestamps will be entirely different kind, and probably doesn't end
> > > > up to the timestamp field.

-- 
Regards,

Laurent Pinchart