Hi Guennadi,
Thank you for the patch.
> Some UVC video cameras contain metadata in their payload headers. This
> patch extracts that data, adding more clock synchronisation information,
> on both bulk and isochronous endpoints and makes it available to the
> user space on a separate video node, using the V4L2_CAP_META_CAPTURE
> capability and the V4L2_BUF_TYPE_META_CAPTURE buffer queue type. Even
> though different cameras will have different metadata formats, we use
> the same V4L2_META_FMT_UVC pixel format for all of them. Users have to
> parse data, based on the specific camera model information.
The issue we still haven't addressed is how to ensure that vendors will
document their metadata format :-S
> This version of the patch only creates such metadata nodes for cameras,
> specifying a UVC_QUIRK_METADATA_NODE quirk flag.
>
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@xxxxxxxxx>
> ---
>
> v4:
> - add support for isochronous cameras. Metadata is now collected from as
> many payloads as they fit in the buffer
> - add a USB Start Of Frame and a system timestamp to each metadata block
> for user-space clock synchronisation
> - use a default buffer size of 1024 bytes
>
> Thanks to Laurent for patient long discussions and to everybody, who
> helped me conduct all the investigation into various past, present and
> future UVC cameras :-)
>
> drivers/media/usb/uvc/Makefile | 2 +-
> drivers/media/usb/uvc/uvc_driver.c | 4 +
> drivers/media/usb/uvc/uvc_isight.c | 2 +-
> drivers/media/usb/uvc/uvc_metadata.c | 158 +++++++++++++++++++++++++++++++
> drivers/media/usb/uvc/uvc_queue.c | 68 ++++++++++-----
> drivers/media/usb/uvc/uvc_video.c | 101 +++++++++++++++++++---
> drivers/media/usb/uvc/uvcvideo.h | 23 ++++-
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> include/uapi/linux/uvcvideo.h | 19 +++++
> include/uapi/linux/videodev2.h | 3 +
> 10 files changed, 347 insertions(+), 34 deletions(-)
> create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
[snip]
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c
> index 70842c5..9f23dcf 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1880,6 +1880,7 @@ static void uvc_unregister_video(struct uvc_device
> *dev)
> continue;
>
> video_unregister_device(&stream->vdev);
> + video_unregister_device(&stream->meta.vdev);
>
> uvc_debugfs_cleanup_stream(stream);
> }
> @@ -1941,6 +1942,9 @@ static int uvc_register_video(struct uvc_device *dev,
> return ret;
> }
>
> + /* Register a metadata node. */
> + uvc_meta_register(stream);
This can fail, you should handle errors.
> if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
> else
[snip]
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c
b/drivers/media/usb/uvc/uvc_metadata.c
> new file mode 100644
> index 0000000..876badd
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -0,0 +1,158 @@
> +/*
> + * uvc_metadata.c -- USB Video Class driver - Metadata handling
> + *
> + * Copyright (C) 2016
> + * Guennadi Liakhovetski (guennadi.liakhovetski@xxxxxxxxx)
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License as published
> by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-v4l2.h>
> +#include <media/videobuf2-vmalloc.h>
> +
> +#include "uvcvideo.h"
> +
> +/* -----------------------------------------------------------------------
> + * videobuf2 Queue Operations
> + */
> +
> +static struct vb2_ops uvc_meta_queue_ops = {
> + .queue_setup = uvc_queue_setup,
> + .buf_prepare = uvc_buffer_prepare,
> + .buf_queue = uvc_buffer_queue,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .stop_streaming = uvc_stop_streaming,
This looks unbalanced without a start_streaming operation. I know that
you've modified the uvc_stop_streaming() function to not stop the video
stream for metadata video nodes, but I think the code would be clearer
if you implemented a uvc_meta_stop_streaming() function
for
> +};
> +
> +/* ------------------------------------------------------------------------
> + * V4L2 ioctls
> + */
> +
> +static int meta_v4l2_querycap(struct file *file, void *fh,
> + struct v4l2_capability *cap)
For consistency with the rest of the code, could you prefix this and the
next function with uvc_ ?
> +{
> + struct v4l2_fh *vfh = file->private_data;
> + struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
> +
> + strlcpy(cap->driver, "uvcvideo", sizeof(cap->driver));
> + strlcpy(cap->card, vfh->vdev->name, sizeof(cap->card));
> + usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap
> ->bus_info));
> +
> + return 0;
> +}
[snip]
> +int uvc_meta_register(struct uvc_streaming *stream)
> +{
> + struct uvc_device *dev = stream->dev;
> + struct uvc_meta_device *meta = &stream->meta;
> + struct video_device *vdev = &meta->vdev;
> + struct uvc_video_queue *quvc = &meta->queue;
> + struct vb2_queue *queue = &quvc->queue;
> + int ret;
> +
> + mutex_init(&quvc->mutex);
> + spin_lock_init(&quvc->irqlock);
> + INIT_LIST_HEAD(&quvc->irqqueue);
> +
> + /*
> + * We register metadata device nodes only if the METADATA_NODE quirk
> is
> + * set and only on interfaces with bulk endpoints. To meaningfully
> + * support interfaces with isochronous endpoints, we need to collect
> + * headers from all payloads, comprising a single frame. For that we
> + * need to know the maximum number of such payloads per frame to be
> able
> + * to calculate the buffer size. Currently this information is
> + * unavailable. A proposal should be made to the UVC committee to add
> + * this information to camera descriptors.
> + */
> + if (!(dev->quirks & UVC_QUIRK_METADATA_NODE) ||
> + dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT)
> + return 0;
> +
> + vdev->v4l2_dev = &dev->vdev;
> + vdev->fops = &uvc_meta_fops;
> + vdev->ioctl_ops = &uvc_meta_ioctl_ops;
> + vdev->release = video_device_release_empty;
Please, no. You'll end up crashing the kernel if an application has the meta
devnode open when the device is disconnected. Proper refcounting is needed.
> + vdev->prio = &stream->chain->prio;
> + vdev->vfl_dir = VFL_DIR_RX;
> + vdev->queue = queue;
> + vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
> + strlcpy(vdev->name, dev->name, sizeof(vdev->name));
> +
> + video_set_drvdata(vdev, stream);
> +
> + /* Initialize the video buffer queue. */
> + queue->type = V4L2_BUF_TYPE_META_CAPTURE;
> + queue->io_modes = VB2_MMAP | VB2_USERPTR;
> + queue->drv_priv = quvc;
> + queue->buf_struct_size = sizeof(struct uvc_buffer);
> + queue->ops = &uvc_meta_queue_ops;
> + queue->mem_ops = &vb2_vmalloc_memops;
> + queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> + | V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
> + queue->lock = &quvc->mutex;
> + ret = vb2_queue_init(queue);
> + if (ret < 0)
> + return ret;
How about calling uvc_queue_init() instead of duplicating the code ? You would
just need to extend the function with a parameter for the queue ops. Or,
possibly even better, you could move the definition of the queue ops to
uvc_queue.c. That way you won't have to expose a bunch of currently static
queue functions.
Similarly, it looks like you're duplicating code from uvc_register_video(),
which I believe could be shared.
> + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> + if (ret < 0)
> + uvc_printk(KERN_ERR, "Failed to register metadata device (%d).
\n", ret);
This line is over 80 characters.
> + return ret;
> +}
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c
> index aa21997..77dedbc 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -74,13 +74,26 @@ static void uvc_queue_return_buffers(struct
> uvc_video_queue *queue,
> * videobuf2 queue operations
> */
>
> -static int uvc_queue_setup(struct vb2_queue *vq,
> - unsigned int *nbuffers, unsigned int *nplanes,
> - unsigned int sizes[], struct device *alloc_devs[])
> +int uvc_queue_setup(struct vb2_queue *vq,
> + unsigned int *nbuffers, unsigned int *nplanes,
> + unsigned int sizes[], struct device *alloc_devs[])
> {
> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> - struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> - unsigned size = stream->ctrl.dwMaxVideoFrameSize;
> + struct uvc_streaming *stream;
> + unsigned int size;
> +
> + switch (vq->type) {
> + case V4L2_BUF_TYPE_META_CAPTURE:
> + size = UVC_METATADA_BUF_SIZE;
> +
> + if (*nplanes && *nplanes != 1)
> + return -EINVAL;
As the driver doesn't support any multi-plane format, should this check be
moved out of the switch ? You should then add it in a separate patch before
this one.
> + break;
Could you add a blank line between break statements and the next case to be
consistent with the driver's coding style ?
> + default:
> + stream = uvc_queue_to_stream(queue);
> + size = stream->ctrl.dwMaxVideoFrameSize;
While not strictly needed, the driver uses break statements in the default
cases. Could you please do so here (and below) too ?
> + }
>
> /* Make sure the image size is large enough. */
> if (*nplanes)
> @@ -90,34 +103,47 @@ static int uvc_queue_setup(struct vb2_queue *vq,
> return 0;
> }
>
> -static int uvc_buffer_prepare(struct vb2_buffer *vb)
> +int uvc_buffer_prepare(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> struct uvc_buffer *buf = uvc_vbuf_to_buffer(vbuf);
>
> - if (vb->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> - vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0)) {
> - uvc_trace(UVC_TRACE_CAPTURE, "[E] Bytes used out of bounds
> .\n");
> - return -EINVAL;
> - }
> -
> if (unlikely(queue->flags & UVC_QUEUE_DISCONNECTED))
> return -ENODEV;
>
> + switch (vb->type) {
> + case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> + if (vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0)) {
> + uvc_trace(UVC_TRACE_CAPTURE,
> + "[E] Bytes used out of bounds.\n");
> + return -EINVAL;
> + }
> +
> + buf->bytesused = vb2_get_plane_payload(vb, 0);
> +
> + break;
> + case V4L2_BUF_TYPE_META_CAPTURE:
> + if (vb->num_planes != 1 ||
> + vb2_plane_size(vb, 0) < UVC_METATADA_BUF_SIZE) {
Can this happen, given that queue_setup rejects buffers that verify those
conditions ?
> + uvc_trace(UVC_TRACE_CAPTURE,
> + "[E] Invalid buffer configuration.\n");
> + return -EINVAL;
> + }
> + /* Fall through */
> + default:
> + buf->bytesused = 0;
> + }
> +
> buf->state = UVC_BUF_STATE_QUEUED;
> buf->error = 0;
> buf->mem = vb2_plane_vaddr(vb, 0);
> buf->length = vb2_plane_size(vb, 0);
> - if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
Unless I was mistaken in my previous comment, I think you only need to change
the condition to vb->type != V4L2_BUF_TYPE_VIDEO_OUTPUT, no other change to
the function should be needed.
> - buf->bytesused = 0;
> - else
> - buf->bytesused = vb2_get_plane_payload(vb, 0);
>
> return 0;
> }
>
> -static void uvc_buffer_queue(struct vb2_buffer *vb)
> +void uvc_buffer_queue(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> @@ -169,13 +195,15 @@ static int uvc_start_streaming(struct vb2_queue *vq,
> unsigned int count)
> return ret;
> }
>
> -static void uvc_stop_streaming(struct vb2_queue *vq)
> +void uvc_stop_streaming(struct vb2_queue *vq)
> {
> struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> - struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> unsigned long flags;
>
> - uvc_video_enable(stream, 0);
> + if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) {
> + struct uvc_streaming *stream = uvc_queue_to_stream(queue);
Hasn't checkpatch complained about a missing blank line here ?
> + uvc_video_enable(stream, 0);
> + }
>
> spin_lock_irqsave(&queue->irqlock, flags);
> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index fb86d6a..3a54439 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1152,8 +1152,76 @@ static void uvc_video_validate_buffer(const struct
> uvc_streaming *stream,
> /*
> * Completion handler for video URBs.
> */
I think this function would be better placed in a new "Metadata" section right
before the "URB handling" section.
> +
> +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf,
You might want to call buf video_buf for clarity.
> + u8 *mem, unsigned int length)
> +{
> + struct uvc_meta_buf *meta;
> + size_t len_std = 2;
> + bool has_pts, has_scr;
> + u8 *scr;
> +
> + if (!meta_buf || length == 2 ||
> + meta_buf->length - meta_buf->bytesused <
> + length + sizeof(meta->ts) + sizeof(meta->sof) ||
> + length != mem[0])
I don't think the last condition can occur here, as it should be caught in
uvc_video_decode_start() and result in the payload being skipped.
> + return;
> +
> + has_pts = mem[1] & UVC_STREAM_PTS;
> + has_scr = mem[1] & UVC_STREAM_SCR;
> +
> + if (has_pts) {
> + len_std += 4;
> + scr = mem + 6;
> + } else {
> + scr = mem + 2;
> + }
> +
> + if (has_scr)
> + len_std += 6;
> +
> + if (length == len_std && (!has_scr ||
> + !memcmp(scr, stream->clock.last_scr, 6)))
> + return;
I think a comment would be useful to explain the strategy. I understand it now
as we've discussed it recently, but I'm not sure I still will in 6 months :-)
You could add a comment block at the beginning of the function, similarly to
uvc_video_decode_start().
> +
> + meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem + meta_buf
> ->bytesused);
> + uvc_video_get_ts(&meta->ts);
> + meta->sof = usb_get_current_frame_number(stream->dev->udev);
You should disable local interrupts around those two calls to ensure that they
will be as close as possible.
> +
> + if (!meta_buf->bytesused) {
> + meta_buf->buf.sequence = buf->buf.sequence;
> + meta_buf->buf.field = buf->buf.field;
> + meta_buf->buf.vb2_buf.timestamp = buf->buf.vb2_buf.timestamp;
I wonder whether it wouldn't be clearer to do this in
uvc_video_step_buffers(). You wouldn't need to check bytesused then, and the
code would be more future-proof in case the driver gets changed to set one of
those fields later than uvc_video_decode_start().
> + }
> +
> + if (has_scr)
> + memcpy(stream->clock.last_scr, scr, 6);
> +
> + memcpy(&meta->length, mem, length);
> + meta_buf->bytesused += length + sizeof(meta->ts) + sizeof(meta->sof);
> +
> + uvc_trace(UVC_TRACE_FRAME,
> + "%s(): t-sys %lu.%09lus, SOF %u, len %u, flags 0x%x, PTS %u,
> STC %u frame SOF %u\n",
> + __func__, meta->ts.tv_sec, meta->ts.tv_nsec, meta->sof,
> + meta->length, meta->flags, has_pts ? *(u32 *)meta->buf : 0,
> + has_scr ? *(u32 *)scr : 0,
> + has_scr ? *(u32 *)(scr + 4) & 0x7ff : 0);
> +}
> +
> +static struct uvc_buffer *uvc_video_step_buffers(struct uvc_streaming
> *stream,
> + struct uvc_buffer *buf, struct uvc_buffer **meta_buf)
Wouldn't it better to make the function more symmetrical by passing **buf the
same way we pass **meta_buf ? And as commented above, I think I would rename
buf to video_buf.
> +{
> + if (*meta_buf) {
> + (*meta_buf)->state = UVC_BUF_STATE_READY;
> + *meta_buf = uvc_queue_next_buffer(&stream->meta.queue,
> + *meta_buf);
> + }
> + return uvc_queue_next_buffer(&stream->queue, buf);
> +}
> +
> static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
> *stream,
> - struct uvc_buffer *buf)
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
> {
> u8 *mem;
> int ret, i;
[snip]
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
b/drivers/media/usb/uvc/uvcvideo.h
> index 15e415e..d06e2d6 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -185,6 +185,7 @@
> #define UVC_QUIRK_RESTRICT_FRAME_RATE 0x00000200
> #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400
> #define UVC_QUIRK_FORCE_Y8 0x00000800
> +#define UVC_QUIRK_METADATA_NODE 0x00001000
I notice that you don't set the quirk for any device. What's your plan there,
will there be more patches ?
>
> /* Format flags */
> #define UVC_FMT_FLAG_COMPRESSED 0x00000001
[snip]
> @@ -546,6 +556,8 @@ struct uvc_streaming {
> u16 last_sof;
> u16 sof_offset;
>
> + u8 last_scr[6];
> +
> spinlock_t lock;
> } clock;
> };
> @@ -684,6 +696,12 @@ extern unsigned long uvc_queue_get_unmapped_area(struct
uvc_video_queue *queue,
> unsigned long pgoff);
> #endif
> extern int uvc_queue_allocated(struct uvc_video_queue *queue);
> +extern int uvc_queue_setup(struct vb2_queue *vq,
> + unsigned int *nbuffers, unsigned int *nplanes,
> + unsigned int sizes[], struct device *alloc_devs[]);
> +extern int uvc_buffer_prepare(struct vb2_buffer *vb);
> +extern void uvc_buffer_queue(struct vb2_buffer *vb);
> +extern void uvc_stop_streaming(struct vb2_queue *vq);
You can drop the extern keyword, it's not needed.
> static inline int uvc_queue_streaming(struct uvc_video_queue *queue)
> {
> return vb2_is_streaming(&queue->queue);
[snip]
> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index 3b08186..c503dcc 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -67,4 +67,23 @@ struct uvc_xu_control_query {
> #define UVCIOC_CTRL_MAP _IOWR('u', 0x20, struct
uvc_xu_control_mapping)
> #define UVCIOC_CTRL_QUERY _IOWR('u', 0x21, struct uvc_xu_control_query)
>
> +/*
> + * Metadata node
> + */
> +
> +/*
Could you please use kerneldoc comments to document this structure ?
> + * UVC metadata nodes fill buffers with possibly multiple instances of the
> + * following struct. The first two fields are added by the driver, they can
> be
> + * used for clock synchronisation. The rest is an exact copy of a UVC
> payload
> + * header. Only complete objects with complete buffers are included.
> Therefore
> + * it's always sizeof(meta->ts) + sizeof(meta->sof) + meta->length bytes
> large.
> + */
> +struct uvc_meta_buf {
> + struct timespec ts;
timespec has a different size on 32-bit and 64-bit architectures, so there
could be issues on 32-bit userspace running on a 64-bit kernel.
Additionally, on 32-bit platforms, timespec is not year 2038-safe. I thought
that timespec64 was exposed to userspace nowadays, but it doesn't seem to be
the case. I'm not sure how to handle this.
> + __u16 sof;
> + __u8 length;
> + __u8 flags;
> + __u8 buf[];
I had imagined inserting the ts + sof after the standard header, but this
probably makes more sense.
> +};
> +
> #endif
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 45cf735..9b0cc7e 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -683,6 +683,9 @@ struct v4l2_pix_format {
> #define V4L2_META_FMT_VSP1_HGO v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car
> VSP1 1-D Histogram */
> #define V4L2_META_FMT_VSP1_HGT v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car
> VSP1 2-D Histogram */
>
> +/* Meta-data formats */
> +#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC
Payload Header metadata */
This should be documented in Documentation/media/, and probably split to a
separate patch.
> +
> /* priv field value to indicates that subsequent fields are valid. */
> #define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe
>
--
Regards,
Laurent Pinchart
