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

2013-01-22 Thread 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.ai...@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

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

2013-01-21 Thread Kamil Debski 
Hi,

 From: Sakari Ailus [mailto:sakari.ai...@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 RD Center


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-19 Thread 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.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-14 Thread 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.deb...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@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)