[v6,3/5] media: videodev2.h: Add new boottime timestamp type
diff mbox series

Message ID 20191219054930.29513-4-jungo.lin@mediatek.com
State New
Headers show
Series
  • media: media: platform: mtk-isp: Add Mediatek ISP Pass 1 driver
Related show

Commit Message

Jungo Lin Dec. 19, 2019, 5:49 a.m. UTC
For Camera AR(Augmented Reality) application requires camera timestamps
to be reported with CLOCK_BOOTTIME to sync timestamp with other sensor
sources.

The boottime timestamp is identical to monotonic timestamp,
except it also includes any time that the system is suspended.

Signed-off-by: Jungo Lin <jungo.lin@mediatek.com>
---
Changes from v6:
 - No change.
---
 Documentation/media/uapi/v4l/buffer.rst | 11 ++++++++++-
 include/uapi/linux/videodev2.h          |  2 ++
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Hans Verkuil Jan. 7, 2020, 2:10 p.m. UTC | #1
On 12/19/19 6:49 AM, Jungo Lin wrote:
> For Camera AR(Augmented Reality) application requires camera timestamps
> to be reported with CLOCK_BOOTTIME to sync timestamp with other sensor
> sources.
> 
> The boottime timestamp is identical to monotonic timestamp,
> except it also includes any time that the system is suspended.
> 
> Signed-off-by: Jungo Lin <jungo.lin@mediatek.com>
> ---
> Changes from v6:
>  - No change.
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 11 ++++++++++-
>  include/uapi/linux/videodev2.h          |  2 ++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index 9149b57728e5..f45bfce7fddd 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -662,13 +662,22 @@ Buffer Flags
>        - 0x00002000
>        - The buffer timestamp has been taken from the ``CLOCK_MONOTONIC``
>  	clock. To access the same clock outside V4L2, use
> -	:c:func:`clock_gettime`.
> +	:c:func:`clock_gettime` using clock IDs ``CLOCK_MONOTONIC``.

IDs -> ID

>      * .. _`V4L2-BUF-FLAG-TIMESTAMP-COPY`:
>  
>        - ``V4L2_BUF_FLAG_TIMESTAMP_COPY``
>        - 0x00004000
>        - The CAPTURE buffer timestamp has been taken from the corresponding
>  	OUTPUT buffer. This flag applies only to mem2mem devices.
> +    * .. _`V4L2_BUF_FLAG_TIMESTAMP_BOOTIME`:

You mistyped BOOTTIME as BOOTIME in a lot of places. Please check.

> +
> +      - ``V4L2_BUF_FLAG_TIMESTAMP_BOOTIME``
> +      - 0x00008000
> +      - The buffer timestamp has been taken from the ``CLOCK_BOOTTIME``
> +	clock. To access the same clock outside V4L2, use
> +	:c:func:`clock_gettime` using clock IDs ``CLOCK_BOOTTIME``.

IDs -> ID

> +	Identical to CLOCK_MONOTONIC, except it also includes any time that
> +	the system is suspended.
>      * .. _`V4L2-BUF-FLAG-TSTAMP-SRC-MASK`:
>  
>        - ``V4L2_BUF_FLAG_TSTAMP_SRC_MASK``
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 04481c717fee..74ef9472e702 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1060,6 +1060,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
>  #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
>  #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
>  #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
> +#define V4L2_BUF_FLAG_TIMESTAMP_BOOTIME		0x00008000

This should be 0x00006000.

(flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) is a value that determines the timestamp
source, so these timestamp defines are values, not bitmasks.

However, I don't like your approach. Whether to use MONOTONIC or BOOTTIME is really
a userspace decision, and locking a driver to one of these two options seems
wrong to me.

Instead add new V4L2_BUF_FLAG_USE_BOOTTIME flag that userspace can set when queuing
the buffer and that indicates that instead of the MONOTONIC timestamp, it should return
the BOOTTIME timestamp. This requires a simple helper function that returns either
ktime_get_ns or ktime_get_boottime_ns based on the vb2_v4l2_buffer flags field.

It's definitely more work (although it can be limited to drivers that use vb2),
but much more useful.

Regards,

	Hans

> +
>  /* Timestamp sources. */
>  #define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
>  #define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
>
Jungo Lin Jan. 10, 2020, 9:59 a.m. UTC | #2
Hi Hans:

Appreciate your comments on this patch.

> From: Hans Verkuil [mailto:hverkuil-cisco@xs4all.nl]
> Sent: Tuesday, January 07, 2020 10:11 PM
> To: jungo.lin@mediatek.com; tfiga@chromium.org; laurent.pinchart@ideasonboard.com; matthias.bgg@gmail.com; mchehab@kernel.org
> Cc: linux-media@vger.kernel.org; linux-mediatek@lists.infradead.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; srv_heupstream@mediatek.com; ddavenport@chromium.org; robh@kernel.org; Sean.Cheng@mediatek.com; sj.huang@mediatek.com; Frederic.Chen@mediatek.com; Jerry-ch.Chen@mediatek.com; Frankie.Chiu@mediatek.com; ryan.yu@mediatek.com; Rynn.Wu@mediatek.com; yuzhao@chromium.org; zwisler@chromium.org; shik@chromium.org; suleiman@chromium.org
> Subject: Re: [v6, 3/5] media: videodev2.h: Add new boottime timestamp type
> 
> On 12/19/19 6:49 AM, Jungo Lin wrote:
> > For Camera AR(Augmented Reality) application requires camera
> > timestamps to be reported with CLOCK_BOOTTIME to sync timestamp with
> > other sensor sources.
> >
> > The boottime timestamp is identical to monotonic timestamp, except it
> > also includes any time that the system is suspended.
> >
> > Signed-off-by: Jungo Lin <jungo.lin@mediatek.com>
> > ---
> > Changes from v6:
> >  - No change.
> > ---
> >  Documentation/media/uapi/v4l/buffer.rst | 11 ++++++++++-
> >  include/uapi/linux/videodev2.h          |  2 ++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/media/uapi/v4l/buffer.rst
> > b/Documentation/media/uapi/v4l/buffer.rst
> > index 9149b57728e5..f45bfce7fddd 100644
> > --- a/Documentation/media/uapi/v4l/buffer.rst
> > +++ b/Documentation/media/uapi/v4l/buffer.rst
> > @@ -662,13 +662,22 @@ Buffer Flags
> >        - 0x00002000
> >        - The buffer timestamp has been taken from the ``CLOCK_MONOTONIC``
> >  clock. To access the same clock outside V4L2, use
> > -:c:func:`clock_gettime`.
> > +:c:func:`clock_gettime` using clock IDs ``CLOCK_MONOTONIC``.
> 
> IDs -> ID
> 

Ok, fix in next version.

> >      * .. _`V4L2-BUF-FLAG-TIMESTAMP-COPY`:
> >
> >        - ``V4L2_BUF_FLAG_TIMESTAMP_COPY``
> >        - 0x00004000
> >        - The CAPTURE buffer timestamp has been taken from the corresponding
> >  OUTPUT buffer. This flag applies only to mem2mem devices.
> > +    * .. _`V4L2_BUF_FLAG_TIMESTAMP_BOOTIME`:
> 
> You mistyped BOOTTIME as BOOTIME in a lot of places. Please check.
> 

Ok, fix this typo in next version.

> > +
> > +      - ``V4L2_BUF_FLAG_TIMESTAMP_BOOTIME``
> > +      - 0x00008000
> > +      - The buffer timestamp has been taken from the ``CLOCK_BOOTTIME``
> > +clock. To access the same clock outside V4L2, use
> > +:c:func:`clock_gettime` using clock IDs ``CLOCK_BOOTTIME``.
> 
> IDs -> ID
> 

Ditto.

> > +Identical to CLOCK_MONOTONIC, except it also includes any time that
> > +the system is suspended.
> >      * .. _`V4L2-BUF-FLAG-TSTAMP-SRC-MASK`:
> >
> >        - ``V4L2_BUF_FLAG_TSTAMP_SRC_MASK`` diff --git
> > a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 04481c717fee..74ef9472e702 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1060,6 +1060,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
> >  #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN0x00000000
> >  #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC0x00002000
> >  #define V4L2_BUF_FLAG_TIMESTAMP_COPY0x00004000
> > +#define V4L2_BUF_FLAG_TIMESTAMP_BOOTIME0x00008000
> 
> This should be 0x00006000.
> 
> (flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) is a value that determines the timestamp source, so these timestamp defines are values, not bitmasks.
> 
> However, I don't like your approach. Whether to use MONOTONIC or BOOTTIME is really a userspace decision, and locking a driver to one of these two options seems wrong to me.
> 
> Instead add new V4L2_BUF_FLAG_USE_BOOTTIME flag that userspace can set when queuing the buffer and that indicates that instead of the MONOTONIC timestamp, it should return the BOOTTIME timestamp. This requires a simple helper function that returns either ktime_get_ns or ktime_get_boottime_ns based on the vb2_v4l2_buffer flags field.
> 
> It's definitely more work (although it can be limited to drivers that use vb2), but much more useful.
> 
> Regards,
> 
> Hans
> 

Agree.
We will add new V4L2_BUF_FLAG_USE_BOOTTIME flag (0x00006000.) to replace
this V4L2_BUF_FLAG_TIMESTAMP_BOOTIME flag for better usage.

Sincerely

Jungo

> > +
> >  /* Timestamp sources. */
> >  #define V4L2_BUF_FLAG_TSTAMP_SRC_MASK0x00070000
> >  #define V4L2_BUF_FLAG_TSTAMP_SRC_EOF0x00000000
> >
Jungo Lin Jan. 10, 2020, 10:08 a.m. UTC | #3
Hi Hans:

Appreciate your comments on this patch.

On Tue, 2020-01-07 at 15:10 +0100, Hans Verkuil wrote:
> On 12/19/19 6:49 AM, Jungo Lin wrote:
> > For Camera AR(Augmented Reality) application requires camera timestamps
> > to be reported with CLOCK_BOOTTIME to sync timestamp with other sensor
> > sources.
> > 
> > The boottime timestamp is identical to monotonic timestamp,
> > except it also includes any time that the system is suspended.
> > 
> > Signed-off-by: Jungo Lin <jungo.lin@mediatek.com>
> > ---
> > Changes from v6:
> >  - No change.
> > ---
> >  Documentation/media/uapi/v4l/buffer.rst | 11 ++++++++++-
> >  include/uapi/linux/videodev2.h          |  2 ++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> > index 9149b57728e5..f45bfce7fddd 100644
> > --- a/Documentation/media/uapi/v4l/buffer.rst
> > +++ b/Documentation/media/uapi/v4l/buffer.rst
> > @@ -662,13 +662,22 @@ Buffer Flags
> >        - 0x00002000
> >        - The buffer timestamp has been taken from the ``CLOCK_MONOTONIC``
> >  	clock. To access the same clock outside V4L2, use
> > -	:c:func:`clock_gettime`.
> > +	:c:func:`clock_gettime` using clock IDs ``CLOCK_MONOTONIC``.
> 
> IDs -> ID
> 

Ok, fix in next version.

> >      * .. _`V4L2-BUF-FLAG-TIMESTAMP-COPY`:
> >  
> >        - ``V4L2_BUF_FLAG_TIMESTAMP_COPY``
> >        - 0x00004000
> >        - The CAPTURE buffer timestamp has been taken from the corresponding
> >  	OUTPUT buffer. This flag applies only to mem2mem devices.
> > +    * .. _`V4L2_BUF_FLAG_TIMESTAMP_BOOTIME`:
> 
> You mistyped BOOTTIME as BOOTIME in a lot of places. Please check.
> 

Ok, fix this typo in next version.

> > +
> > +      - ``V4L2_BUF_FLAG_TIMESTAMP_BOOTIME``
> > +      - 0x00008000
> > +      - The buffer timestamp has been taken from the ``CLOCK_BOOTTIME``
> > +	clock. To access the same clock outside V4L2, use
> > +	:c:func:`clock_gettime` using clock IDs ``CLOCK_BOOTTIME``.
> 
> IDs -> ID
> 

Ditto.

> > +	Identical to CLOCK_MONOTONIC, except it also includes any time that
> > +	the system is suspended.
> >      * .. _`V4L2-BUF-FLAG-TSTAMP-SRC-MASK`:
> >  
> >        - ``V4L2_BUF_FLAG_TSTAMP_SRC_MASK``
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 04481c717fee..74ef9472e702 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1060,6 +1060,8 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
> >  #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
> >  #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
> >  #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
> > +#define V4L2_BUF_FLAG_TIMESTAMP_BOOTIME		0x00008000
> 
> This should be 0x00006000.
> 
> (flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) is a value that determines the timestamp
> source, so these timestamp defines are values, not bitmasks.
> 
> However, I don't like your approach. Whether to use MONOTONIC or BOOTTIME is really
> a userspace decision, and locking a driver to one of these two options seems
> wrong to me.
> 
> Instead add new V4L2_BUF_FLAG_USE_BOOTTIME flag that userspace can set when queuing
> the buffer and that indicates that instead of the MONOTONIC timestamp, it should return
> the BOOTTIME timestamp. This requires a simple helper function that returns either
> ktime_get_ns or ktime_get_boottime_ns based on the vb2_v4l2_buffer flags field.
> 
> It's definitely more work (although it can be limited to drivers that use vb2),
> but much more useful.
> 
> Regards,
> 
> 	Hans
> 

Agree.
We will add new V4L2_BUF_FLAG_USE_BOOTTIME flag (0x00006000.) to replace
this V4L2_BUF_FLAG_TIMESTAMP_BOOTIME flag for better usage.

> > +
> >  /* Timestamp sources. */
> >  #define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
> >  #define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000
> > 
> 

Sincerely

Jungo

Patch
diff mbox series

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index 9149b57728e5..f45bfce7fddd 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -662,13 +662,22 @@  Buffer Flags
       - 0x00002000
       - The buffer timestamp has been taken from the ``CLOCK_MONOTONIC``
 	clock. To access the same clock outside V4L2, use
-	:c:func:`clock_gettime`.
+	:c:func:`clock_gettime` using clock IDs ``CLOCK_MONOTONIC``.
     * .. _`V4L2-BUF-FLAG-TIMESTAMP-COPY`:
 
       - ``V4L2_BUF_FLAG_TIMESTAMP_COPY``
       - 0x00004000
       - The CAPTURE buffer timestamp has been taken from the corresponding
 	OUTPUT buffer. This flag applies only to mem2mem devices.
+    * .. _`V4L2_BUF_FLAG_TIMESTAMP_BOOTIME`:
+
+      - ``V4L2_BUF_FLAG_TIMESTAMP_BOOTIME``
+      - 0x00008000
+      - The buffer timestamp has been taken from the ``CLOCK_BOOTTIME``
+	clock. To access the same clock outside V4L2, use
+	:c:func:`clock_gettime` using clock IDs ``CLOCK_BOOTTIME``.
+	Identical to CLOCK_MONOTONIC, except it also includes any time that
+	the system is suspended.
     * .. _`V4L2-BUF-FLAG-TSTAMP-SRC-MASK`:
 
       - ``V4L2_BUF_FLAG_TSTAMP_SRC_MASK``
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 04481c717fee..74ef9472e702 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1060,6 +1060,8 @@  static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
 #define V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN		0x00000000
 #define V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC	0x00002000
 #define V4L2_BUF_FLAG_TIMESTAMP_COPY		0x00004000
+#define V4L2_BUF_FLAG_TIMESTAMP_BOOTIME		0x00008000
+
 /* Timestamp sources. */
 #define V4L2_BUF_FLAG_TSTAMP_SRC_MASK		0x00070000
 #define V4L2_BUF_FLAG_TSTAMP_SRC_EOF		0x00000000