Message ID | 20191219054930.29513-4-jungo.lin@mediatek.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1ihoh4-004FCd-0D; Thu, 19 Dec 2019 05:49:50 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726617AbfLSFuQ (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 19 Dec 2019 00:50:16 -0500 Received: from mailgw01.mediatek.com ([210.61.82.183]:48040 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1726338AbfLSFuP (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 19 Dec 2019 00:50:15 -0500 X-UUID: 146105b05f37431dad10e90c351b6e79-20191219 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Transfer-Encoding:Content-Type:MIME-Version:Reply-To:References:In-Reply-To:Message-ID:Date:Subject:CC:To:From; bh=7fCn/ztl4AlC39YFUcLA1Fd5PU/Oyw3txLJ3UdHhRJ8=; b=NoQTv/Fj9uM+eKVXU2ao/khA+tzVXC9AfhLOO8YqZTj9djH6MOrPHtiFabCbn8vvXL3+Bxb9BfpcX5twykeJ2qjY6Bu1x33ye4/SDJYJk4T8r+3e+RNx75nVQqoMnWSHIo1joAzhX60RsQ67qFw56DfDK8Uzx673i77sLc8UjsU=; X-UUID: 146105b05f37431dad10e90c351b6e79-20191219 Received: from mtkcas08.mediatek.inc [(172.21.101.126)] by mailgw01.mediatek.com (envelope-from <jungo.lin@mediatek.com>) (Cellopoint E-mail Firewall v4.1.10 Build 0809 with TLS) with ESMTP id 792777870; Thu, 19 Dec 2019 13:50:11 +0800 Received: from mtkcas08.mediatek.inc (172.21.101.126) by mtkmbs01n1.mediatek.inc (172.21.101.68) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Thu, 19 Dec 2019 13:49:48 +0800 Received: from mtksdccf07.mediatek.inc (172.21.84.99) by mtkcas08.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1395.4 via Frontend Transport; Thu, 19 Dec 2019 13:50:12 +0800 From: Jungo Lin <jungo.lin@mediatek.com> To: <tfiga@chromium.org>, <hverkuil-cisco@xs4all.nl>, <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>, <jungo.lin@mediatek.com> Subject: [v6, 3/5] media: videodev2.h: Add new boottime timestamp type Date: Thu, 19 Dec 2019 13:49:28 +0800 Message-ID: <20191219054930.29513-4-jungo.lin@mediatek.com> X-Mailer: git-send-email 2.18.0 In-Reply-To: <20191219054930.29513-1-jungo.lin@mediatek.com> References: <Jungo Lin <jungo.lin@mediatek.com> <20191219054930.29513-1-jungo.lin@mediatek.com> Reply-To: Jungo Lin <jungo.lin@mediatek.com> MIME-Version: 1.0 Content-Type: text/plain X-MTK: N Content-Transfer-Encoding: base64 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Series |
media: media: platform: mtk-isp: Add Mediatek ISP Pass 1 driver
|
|
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
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