| Message ID | 20191219054930.29513-4-jungo.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: media: platform: mtk-isp: Add Mediatek ISP Pass 1 driver | expand |
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 >
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 > >
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
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
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(-)