Re: [RFC PATCH v9 1/6] media: videobuf2: Move timestamp to vb2_buffer

[Junghak Sung]

Dear Hans,

First of all, thank you for your review.

On 11/04/2015 09:28 PM, Hans Verkuil wrote:

On 11/03/15 11:16, Junghak Sung wrote:

Move timestamp from struct vb2_v4l2_buffer to struct vb2_buffer
for common use, and change its type to u64 in order to handling
y2038 problem. This patch also includes all device drivers' changes related to
this restructuring.

Signed-off-by: Junghak Sung 
Signed-off-by: Geunyoung Kim 
Acked-by: Seung-Woo Kim 
Acked-by: Inki Dae 
---





diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
index 1bd2fd4..61df3e4 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
@@ -531,8 +531,8 @@ static int solo_enc_fillbuf(struct solo_enc_dev *solo_enc,

if (!ret) {
vbuf->sequence = solo_enc->sequence++;
-   vbuf->timestamp.tv_sec = vop_sec(vh);
-   vbuf->timestamp.tv_usec = vop_usec(vh);
+   vb->timestamp = ((u64) vop_sec(vh) * NSEC_PER_SEC) +
+   (vop_usec(vh) * NSEC_PER_USEC);


This is wrong. Just use ktime_get_ns() here. It is probably best to first make a
single patch to change the solo driver to use v4l2_get_timestamp(), then convert
that to ktime_get_ns() in this patch.

The problem is that the timestamp is taken from the mpeg header, and so it is
not a CLOCK_MONOTONIC timestamp as is signaled to the user. Never noticed this
before, but it is a solo driver bug.


OK, I will prepare a single patch for solo driver to use
v4l2_get_timestamp(), and then converting to ktime_get_ns()
will be included in next version - v10.
I'm not aware of this historical problem.
so, it's very helpful. Thank you, Hans.




/* Check for motion flags */
if (solo_is_motion_on(solo_enc) && enc_buf->motion) {
diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2.c 
b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
index 26df903..44b00b8 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
@@ -225,7 +225,7 @@ finish_buf:
vb2_set_plane_payload(vb, 0,
solo_vlines(solo_dev) * solo_bytesperline(solo_dev));
vbuf->sequence = solo_dev->sequence++;
-   v4l2_get_timestamp(>timestamp);
+   vb->timestamp = ktime_get_ns();
}

vb2_buffer_done(vb, error ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);





diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c 
b/drivers/media/platform/vivid/vivid-kthread-cap.c
index 83cc6d3..b0ad054 100644
--- a/drivers/media/platform/vivid/vivid-kthread-cap.c
+++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
@@ -441,7 +441,7 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct 
vivid_buffer *buf)
 * "Start of Exposure".
 */
if (dev->tstamp_src_is_soe)
-   v4l2_get_timestamp(>vb.timestamp);
+   buf->vb.vb2_buf.timestamp = ktime_get_ns();
if (dev->field_cap == V4L2_FIELD_ALTERNATE) {
/*
 * 60 Hz standards start with the bottom field, 50 Hz standards
@@ -558,8 +558,9 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct 
vivid_buffer *buf)
 * the timestamp now.
 */
if (!dev->tstamp_src_is_soe)
-   v4l2_get_timestamp(>vb.timestamp);
-   buf->vb.timestamp.tv_sec += dev->time_wrap_offset;
+   buf->vb.vb2_buf.timestamp = ktime_get_ns();
+   buf->vb.vb2_buf.timestamp +=
+   ((u64) dev->time_wrap_offset * NSEC_PER_SEC);


I'd do this differently: make time_wrap_offset of type u64 and assign it
accordingly with nanoseconds. That way you can just do:

timestamp += dev->time_wrap_offset

vivid-ctrls.c also needs to be modified (vivid_streaming_s_ctrl(), 
VIVID_CID_TIME_WRAP
case) to:

dev->time_wrap_offset = (0x1ULL - 16) * NSEC_PER_SEC - 
ktime_get_ns();

The v4l2_get_timestamp() call there can be dropped.



I agree with your opinion. But it is too hard to read the code
getting time_wrap_offset.
How about this way?

 in vivid_streaming_s_ctrl() of vivid-ctrls.c
dev->time_wrap_offset = ktime_get_ns() + 16 * NSEC_PER_SEC;
 and in vivid_fillbuff() of vivid-kthread-cap.c
buf->vb.vb2_buf.timestamp -= dev->time_wrap_offset;



  }

  /*
diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c 
b/drivers/media/platform/vivid/vivid-kthread-out.c
index c2c46dc..6fd02c9 100644
--- a/drivers/media/platform/vivid/vivid-kthread-out.c
+++ b/drivers/media/platform/vivid/vivid-kthread-out.c
@@ -95,8 +95,9 @@ static void vivid_thread_vid_out_tick(struct vivid_dev *dev)
 */
vid_out_buf->vb.sequence /= 2;
}
-   

Re: [RFC PATCH v9 1/6] media: videobuf2: Move timestamp to vb2_buffer

[Hans Verkuil]

On 11/05/2015 04:12 AM, Junghak Sung wrote:
>>> diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c 
>>> b/drivers/media/platform/vivid/vivid-kthread-cap.c
>>> index 83cc6d3..b0ad054 100644
>>> --- a/drivers/media/platform/vivid/vivid-kthread-cap.c
>>> +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
>>> @@ -441,7 +441,7 @@ static void vivid_fillbuff(struct vivid_dev *dev, 
>>> struct vivid_buffer *buf)
>>>  * "Start of Exposure".
>>>  */
>>> if (dev->tstamp_src_is_soe)
>>> -   v4l2_get_timestamp(>vb.timestamp);
>>> +   buf->vb.vb2_buf.timestamp = ktime_get_ns();
>>> if (dev->field_cap == V4L2_FIELD_ALTERNATE) {
>>> /*
>>>  * 60 Hz standards start with the bottom field, 50 Hz standards
>>> @@ -558,8 +558,9 @@ static void vivid_fillbuff(struct vivid_dev *dev, 
>>> struct vivid_buffer *buf)
>>>  * the timestamp now.
>>>  */
>>> if (!dev->tstamp_src_is_soe)
>>> -   v4l2_get_timestamp(>vb.timestamp);
>>> -   buf->vb.timestamp.tv_sec += dev->time_wrap_offset;
>>> +   buf->vb.vb2_buf.timestamp = ktime_get_ns();
>>> +   buf->vb.vb2_buf.timestamp +=
>>> +   ((u64) dev->time_wrap_offset * NSEC_PER_SEC);
>>
>> I'd do this differently: make time_wrap_offset of type u64 and assign it
>> accordingly with nanoseconds. That way you can just do:
>>
>>  timestamp += dev->time_wrap_offset
>>
>> vivid-ctrls.c also needs to be modified (vivid_streaming_s_ctrl(), 
>> VIVID_CID_TIME_WRAP
>> case) to:
>>
>>  dev->time_wrap_offset = (0x1ULL - 16) * NSEC_PER_SEC - 
>> ktime_get_ns();
>>
>> The v4l2_get_timestamp() call there can be dropped.
>>
> 
> I agree with your opinion. But it is too hard to read the code
> getting time_wrap_offset.
> How about this way?
> 
>   in vivid_streaming_s_ctrl() of vivid-ctrls.c
>  dev->time_wrap_offset = ktime_get_ns() + 16 * NSEC_PER_SEC;
>   and in vivid_fillbuff() of vivid-kthread-cap.c
>   buf->vb.vb2_buf.timestamp -= dev->time_wrap_offset;

That doesn't do what I want it to do, which is to wrap around in the struct 
timeval
where seconds are 32 bits. The code above is actually wrong since I forgot that
tv_sec in struct timeval is signed, so 0x1ULL should be 0x8000ULL.
Also, that code will fail after 2038, so that's no good either.

On a related note I send out a question to Arnd whether a timestamp should be 
u64
or s64. It's not clear to me which should be used as ktime_get_ns() returns a 
s64.

Once we return the full 64 bits to userspace, then we have a second wrap around 
when
the u64 (or s64) wraps. I'll add a second wrap-around control to vivid at that 
time.

I'll wait for Arnd to answer before fixing the time_wrap_offset calculation 
since I
need to know whether a timestamp is u64 or s64.

Regards,

Hans
--
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: [RFC PATCH v9 1/6] media: videobuf2: Move timestamp to vb2_buffer

[Hans Verkuil]

On 11/03/15 11:16, Junghak Sung wrote:
> Move timestamp from struct vb2_v4l2_buffer to struct vb2_buffer
> for common use, and change its type to u64 in order to handling
> y2038 problem. This patch also includes all device drivers' changes related to
> this restructuring.
> 
> Signed-off-by: Junghak Sung 
> Signed-off-by: Geunyoung Kim 
> Acked-by: Seung-Woo Kim 
> Acked-by: Inki Dae 
> ---



> diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
> b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> index 1bd2fd4..61df3e4 100644
> --- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> @@ -531,8 +531,8 @@ static int solo_enc_fillbuf(struct solo_enc_dev *solo_enc,
>  
>   if (!ret) {
>   vbuf->sequence = solo_enc->sequence++;
> - vbuf->timestamp.tv_sec = vop_sec(vh);
> - vbuf->timestamp.tv_usec = vop_usec(vh);
> + vb->timestamp = ((u64) vop_sec(vh) * NSEC_PER_SEC) +
> + (vop_usec(vh) * NSEC_PER_USEC);

This is wrong. Just use ktime_get_ns() here. It is probably best to first make a
single patch to change the solo driver to use v4l2_get_timestamp(), then convert
that to ktime_get_ns() in this patch.

The problem is that the timestamp is taken from the mpeg header, and so it is
not a CLOCK_MONOTONIC timestamp as is signaled to the user. Never noticed this
before, but it is a solo driver bug.

>  
>   /* Check for motion flags */
>   if (solo_is_motion_on(solo_enc) && enc_buf->motion) {
> diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2.c 
> b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
> index 26df903..44b00b8 100644
> --- a/drivers/media/pci/solo6x10/solo6x10-v4l2.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2.c
> @@ -225,7 +225,7 @@ finish_buf:
>   vb2_set_plane_payload(vb, 0,
>   solo_vlines(solo_dev) * solo_bytesperline(solo_dev));
>   vbuf->sequence = solo_dev->sequence++;
> - v4l2_get_timestamp(>timestamp);
> + vb->timestamp = ktime_get_ns();
>   }
>  
>   vb2_buffer_done(vb, error ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);



> diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c 
> b/drivers/media/platform/vivid/vivid-kthread-cap.c
> index 83cc6d3..b0ad054 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
> @@ -441,7 +441,7 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct 
> vivid_buffer *buf)
>* "Start of Exposure".
>*/
>   if (dev->tstamp_src_is_soe)
> - v4l2_get_timestamp(>vb.timestamp);
> + buf->vb.vb2_buf.timestamp = ktime_get_ns();
>   if (dev->field_cap == V4L2_FIELD_ALTERNATE) {
>   /*
>* 60 Hz standards start with the bottom field, 50 Hz standards
> @@ -558,8 +558,9 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct 
> vivid_buffer *buf)
>* the timestamp now.
>*/
>   if (!dev->tstamp_src_is_soe)
> - v4l2_get_timestamp(>vb.timestamp);
> - buf->vb.timestamp.tv_sec += dev->time_wrap_offset;
> + buf->vb.vb2_buf.timestamp = ktime_get_ns();
> + buf->vb.vb2_buf.timestamp +=
> + ((u64) dev->time_wrap_offset * NSEC_PER_SEC);

I'd do this differently: make time_wrap_offset of type u64 and assign it
accordingly with nanoseconds. That way you can just do:

timestamp += dev->time_wrap_offset

vivid-ctrls.c also needs to be modified (vivid_streaming_s_ctrl(), 
VIVID_CID_TIME_WRAP
case) to:

dev->time_wrap_offset = (0x1ULL - 16) * NSEC_PER_SEC - 
ktime_get_ns();

The v4l2_get_timestamp() call there can be dropped.

>  }
>  
>  /*
> diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c 
> b/drivers/media/platform/vivid/vivid-kthread-out.c
> index c2c46dc..6fd02c9 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-out.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-out.c
> @@ -95,8 +95,9 @@ static void vivid_thread_vid_out_tick(struct vivid_dev *dev)
>*/
>   vid_out_buf->vb.sequence /= 2;
>   }
> - v4l2_get_timestamp(_out_buf->vb.timestamp);
> - vid_out_buf->vb.timestamp.tv_sec += dev->time_wrap_offset;
> + vid_out_buf->vb.vb2_buf.timestamp = ktime_get_ns();
> + vid_out_buf->vb.vb2_buf.timestamp +=
> + ((u64) dev->time_wrap_offset * NSEC_PER_SEC);
>   vb2_buffer_done(_out_buf->vb.vb2_buf, dev->dqbuf_error ?
>   VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
>   dprintk(dev, 2, "vid_out buffer %d done\n",
> @@ -108,8 +109,9 @@ static void vivid_thread_vid_out_tick(struct vivid_dev 
> *dev)
>  

[RFC PATCH v9 1/6] media: videobuf2: Move timestamp to vb2_buffer

[Junghak Sung]

Move timestamp from struct vb2_v4l2_buffer to struct vb2_buffer
for common use, and change its type to u64 in order to handling
y2038 problem. This patch also includes all device drivers' changes related to
this restructuring.

Signed-off-by: Junghak Sung 
Signed-off-by: Geunyoung Kim 
Acked-by: Seung-Woo Kim 
Acked-by: Inki Dae 
---
 drivers/input/touchscreen/sur40.c  |2 +-
 drivers/media/dvb-frontends/rtl2832_sdr.c  |2 +-
 drivers/media/pci/cobalt/cobalt-irq.c  |2 +-
 drivers/media/pci/cx23885/cx23885-core.c   |2 +-
 drivers/media/pci/cx23885/cx23885-video.c  |2 +-
 drivers/media/pci/cx25821/cx25821-video.c  |2 +-
 drivers/media/pci/cx88/cx88-core.c |2 +-
 drivers/media/pci/dt3155/dt3155.c  |2 +-
 drivers/media/pci/netup_unidvb/netup_unidvb_core.c |2 +-
 drivers/media/pci/saa7134/saa7134-core.c   |2 +-
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c |4 ++--
 drivers/media/pci/solo6x10/solo6x10-v4l2.c |2 +-
 drivers/media/pci/sta2x11/sta2x11_vip.c|2 +-
 drivers/media/pci/tw68/tw68-video.c|2 +-
 drivers/media/platform/am437x/am437x-vpfe.c|2 +-
 drivers/media/platform/blackfin/bfin_capture.c |2 +-
 drivers/media/platform/coda/coda-bit.c |6 +++---
 drivers/media/platform/coda/coda.h |2 +-
 drivers/media/platform/davinci/vpbe_display.c  |2 +-
 drivers/media/platform/davinci/vpif_capture.c  |2 +-
 drivers/media/platform/davinci/vpif_display.c  |6 +++---
 drivers/media/platform/exynos-gsc/gsc-m2m.c|4 ++--
 drivers/media/platform/exynos4-is/fimc-capture.c   |2 +-
 drivers/media/platform/exynos4-is/fimc-isp-video.c |2 +-
 drivers/media/platform/exynos4-is/fimc-lite.c  |2 +-
 drivers/media/platform/exynos4-is/fimc-m2m.c   |2 +-
 drivers/media/platform/m2m-deinterlace.c   |2 +-
 drivers/media/platform/marvell-ccic/mcam-core.c|2 +-
 drivers/media/platform/mx2_emmaprp.c   |2 +-
 drivers/media/platform/omap3isp/ispvideo.c |2 +-
 drivers/media/platform/rcar_jpu.c  |2 +-
 drivers/media/platform/s3c-camif/camif-capture.c   |2 +-
 drivers/media/platform/s5p-g2d/g2d.c   |2 +-
 drivers/media/platform/s5p-jpeg/jpeg-core.c|4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc.c   |4 ++--
 drivers/media/platform/sh_veu.c|2 +-
 drivers/media/platform/sh_vou.c|2 +-
 drivers/media/platform/soc_camera/atmel-isi.c  |2 +-
 drivers/media/platform/soc_camera/mx2_camera.c |2 +-
 drivers/media/platform/soc_camera/mx3_camera.c |2 +-
 drivers/media/platform/soc_camera/rcar_vin.c   |2 +-
 .../platform/soc_camera/sh_mobile_ceu_camera.c |2 +-
 drivers/media/platform/sti/bdisp/bdisp-v4l2.c  |4 ++--
 drivers/media/platform/ti-vpe/vpe.c|2 +-
 drivers/media/platform/vim2m.c |2 +-
 drivers/media/platform/vivid/vivid-kthread-cap.c   |7 ---
 drivers/media/platform/vivid/vivid-kthread-out.c   |   10 ++
 drivers/media/platform/vivid/vivid-sdr-cap.c   |5 +++--
 drivers/media/platform/vivid/vivid-vbi-cap.c   |   10 ++
 drivers/media/platform/vsp1/vsp1_video.c   |2 +-
 drivers/media/platform/xilinx/xilinx-dma.c |2 +-
 drivers/media/usb/airspy/airspy.c  |2 +-
 drivers/media/usb/au0828/au0828-video.c|2 +-
 drivers/media/usb/em28xx/em28xx-video.c|2 +-
 drivers/media/usb/go7007/go7007-driver.c   |2 +-
 drivers/media/usb/hackrf/hackrf.c  |4 ++--
 drivers/media/usb/pwc/pwc-if.c |3 +--
 drivers/media/usb/s2255/s2255drv.c |2 +-
 drivers/media/usb/stk1160/stk1160-video.c  |2 +-
 drivers/media/usb/usbtv/usbtv-video.c  |2 +-
 drivers/media/usb/uvc/uvc_video.c  |   15 +--
 drivers/media/v4l2-core/videobuf2-v4l2.c   |   10 +-
 drivers/staging/media/davinci_vpfe/vpfe_video.c|2 +-
 drivers/staging/media/omap4iss/iss_video.c |2 +-
 drivers/usb/gadget/function/uvc_queue.c|2 +-
 include/media/videobuf2-core.h |2 ++
 include/media/videobuf2-v4l2.h |2 --
 include/trace/events/v4l2.h|4 ++--
 include/trace/events/vb2.h |7 +--
 69 files changed, 107 insertions(+), 104 deletions(-)

diff --git a/drivers/input/touchscreen/sur40.c 
b/drivers/input/touchscreen/sur40.c
index d214f22..1a2eeaf 100644
--- a/drivers/input/touchscreen/sur40.c
+++