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

Hans Verkuil
2020-01-07T14:10:50Z

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
>