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

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel