Hi Guennadi, Thank you for the patch.
On Friday 02 Dec 2016 11:53:23 Guennadi Liakhovetski wrote: > Some UVC video cameras contain metadata in their payload headers. This > patch extracts that data, skipping the standard part of the header, 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. > > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovet...@intel.com> > --- > > v2: > - updated to the current media/master > - removed superfluous META capability from capture nodes > - now the complete UVC payload header is copied to buffers, including > standard fields > > Still open for discussion: is this really OK to always create an > additional metadata node for each UVC camera or UVC video interface. > > IIUC, Laurent's metadata node patch > https://patchwork.linuxtv.org/patch/36810/ has been acked by Hans and the > only thing, that's preventing it from being merged it the lack of > documentation. While waiting for documentation, I'd appreciate some > discussion of this patch to beat it into shape soon enough and have it > ready for merge soon after Laurent's patches are pulled in. > > Thanks > Guennadi > > drivers/media/usb/uvc/Makefile | 2 +- > drivers/media/usb/uvc/uvc_driver.c | 10 ++ > drivers/media/usb/uvc/uvc_isight.c | 2 +- > drivers/media/usb/uvc/uvc_metadata.c | 228 +++++++++++++++++++++++++++++++ > drivers/media/usb/uvc/uvc_video.c | 47 ++++++-- > drivers/media/usb/uvc/uvcvideo.h | 12 +- > drivers/media/v4l2-core/v4l2-ioctl.c | 1 + > include/uapi/linux/uvcvideo.h | 10 ++ > include/uapi/linux/videodev2.h | 3 + > 9 files changed, 304 insertions(+), 11 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 04bf350..edb67ac 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1866,6 +1866,9 @@ static void uvc_unregister_video(struct uvc_device > *dev) > > video_unregister_device(&stream->vdev); > > + if (video_is_registered(&stream->meta.vdev)) > + video_unregister_device(&stream->meta.vdev); video_unregister_device() contains a video_is_registered(), no need to duplicate it. > + > uvc_debugfs_cleanup_stream(stream); > } > > @@ -1926,6 +1929,13 @@ static int uvc_register_video(struct uvc_device *dev, > return ret; > } > > + /* > + * Register a metadata node. TODO: shall this only be enabled for some > + * cameras? > + */ > + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT)) > + uvc_meta_register(stream); > + I think so, only for the cameras that can produce metadata. > 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..ddf77d9 > --- /dev/null > +++ b/drivers/media/usb/uvc/uvc_metadata.c > @@ -0,0 +1,228 @@ > +/* > + * uvc_metadata.c -- USB Video Class driver - Metadata handling > + * > + * Copyright (C) 2016 > + * Guennadi Liakhovetski (guennadi.liakhovet...@intel.com) > + * > + * 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" > + > +static inline struct uvc_buffer *to_uvc_buffer(struct vb2_v4l2_buffer > *vbuf) > +{ > + return container_of(vbuf, struct uvc_buffer, buf); > +} You can move this function to uvcvideo.h and use it in uvc_queue.c (a separate patch would be best). > +/* ------------------------------------------------------------------------ > + * videobuf2 Queue Operations > + */ > + > +static int meta_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers, > + unsigned int *nplanes, unsigned int sizes[], > + struct device *alloc_devs[]) > +{ > + if (*nplanes) { > + if (*nplanes != 1) > + return -EINVAL; > + > + if (sizes[0] < UVC_PAYLOAD_HEADER_MAX_SIZE) > + return -EINVAL; > + > + return 0; > + } > + > + *nplanes = 1; > + sizes[0] = UVC_PAYLOAD_HEADER_MAX_SIZE; > + > + return 0; > +} > + > +static int meta_buffer_prepare(struct vb2_buffer *vb) > +{ > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct uvc_buffer *buf = to_uvc_buffer(vbuf); > + > + if (vb->num_planes != 1) > + return -EINVAL; > + > + if (vb2_plane_size(vb, 0) < UVC_PAYLOAD_HEADER_MAX_SIZE) > + return -EINVAL; > + > + buf->state = UVC_BUF_STATE_QUEUED; > + buf->error = 0; > + buf->mem = vb2_plane_vaddr(vb, 0); > + buf->length = vb2_plane_size(vb, 0); > + buf->bytesused = 0; > + > + return 0; > +} > + > +static void meta_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); > + struct uvc_buffer *buf = to_uvc_buffer(vbuf); > + unsigned long flags; > + > + spin_lock_irqsave(&queue->irqlock, flags); Shouldn't you handle the UVC_QUEUE_DISCONNECTED flag here ? > + list_add_tail(&buf->queue, &queue->irqqueue); > + spin_unlock_irqrestore(&queue->irqlock, flags); > +} > + > +static int meta_start_streaming(struct vb2_queue *vq, unsigned int count) > +{ > + return 0; > +} > + > +static void meta_stop_streaming(struct vb2_queue *vq) > +{ > + struct uvc_video_queue *queue = vb2_get_drv_priv(vq); > + struct uvc_buffer *buffer; > + unsigned long flags; > + > + spin_lock_irqsave(&queue->irqlock, flags); > + > + /* Remove all buffers from the IRQ queue. */ > + list_for_each_entry(buffer, &queue->irqqueue, queue) > + vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_ERROR); > + INIT_LIST_HEAD(&queue->irqqueue); > + > + spin_unlock_irqrestore(&queue->irqlock, flags); > +} > + > +static struct vb2_ops uvc_meta_queue_ops = { > + .queue_setup = meta_queue_setup, > + .buf_prepare = meta_buffer_prepare, > + .buf_queue = meta_buffer_queue, > + .wait_prepare = vb2_ops_wait_prepare, > + .wait_finish = vb2_ops_wait_finish, > + .start_streaming = meta_start_streaming, > + .stop_streaming = meta_stop_streaming, > +}; How about reusing the uvc_queue.c implementation, with a few new checks for metadata buffers where needed, instead of duplicating code ? At first sight the changes don't seem to be too intrusive (but I might have overlooked something). > +/* ------------------------------------------------------------------------ > + * V4L2 ioctls > + */ > + > +static int meta_v4l2_querycap(struct file *file, void *fh, > + struct v4l2_capability *cap) > +{ > + struct v4l2_fh *vfh = file->private_data; > + struct uvc_streaming *stream = video_get_drvdata(vfh->vdev); > + > + cap->device_caps = V4L2_CAP_META_CAPTURE > + | V4L2_CAP_STREAMING; > + cap->capabilities = V4L2_CAP_DEVICE_CAPS | cap->device_caps > + | stream->chain->caps; If you set the device_caps field of struct video_device you won't need the above four lines. > + 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; > +} > + > +static int meta_v4l2_get_format(struct file *file, void *fh, > + struct v4l2_format *format) > +{ > + struct v4l2_fh *vfh = file->private_data; > + struct v4l2_meta_format *fmt = &format->fmt.meta; > + > + if (format->type != vfh->vdev->queue->type) > + return -EINVAL; > + > + memset(fmt, 0, sizeof(*fmt)); > + > + fmt->dataformat = V4L2_META_FMT_UVC; > + fmt->buffersize = UVC_PAYLOAD_HEADER_MAX_SIZE; > + > + return 0; > +} > + > +static const struct v4l2_ioctl_ops uvc_meta_ioctl_ops = { > + .vidioc_querycap = meta_v4l2_querycap, > + .vidioc_g_fmt_meta_cap = meta_v4l2_get_format, > + .vidioc_s_fmt_meta_cap = meta_v4l2_get_format, > + .vidioc_try_fmt_meta_cap = meta_v4l2_get_format, > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > + .vidioc_querybuf = vb2_ioctl_querybuf, > + .vidioc_qbuf = vb2_ioctl_qbuf, > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, > + .vidioc_streamon = vb2_ioctl_streamon, > + .vidioc_streamoff = vb2_ioctl_streamoff, > +}; > + > +/* ------------------------------------------------------------------------ > + * V4L2 File Operations > + */ > + > +static struct v4l2_file_operations uvc_meta_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = video_ioctl2, > + .open = v4l2_fh_open, > + .release = vb2_fop_release, > + .poll = vb2_fop_poll, > + .mmap = vb2_fop_mmap, > +}; > + > +int uvc_meta_register(struct uvc_streaming *stream) > +{ > + struct uvc_device *dev = stream->dev; > + struct uvc_meta_dev *meta = &stream->meta; > + struct video_device *vdev = &meta->vdev; > + struct uvc_video_queue *quvc = &meta->queue; > + struct vb2_queue *queue = &quvc->queue; > + int ret; > + > + vdev->v4l2_dev = &dev->vdev; > + vdev->fops = &uvc_meta_fops; > + vdev->ioctl_ops = &uvc_meta_ioctl_ops; > + vdev->release = video_device_release_empty; > + vdev->prio = &stream->chain->prio; > + vdev->vfl_dir = VFL_DIR_RX; > + 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; > + > + mutex_init(&quvc->mutex); > + spin_lock_init(&quvc->irqlock); > + INIT_LIST_HEAD(&quvc->irqqueue); > + > + vdev->queue = queue; You can set vdev->queue above with the rest of the vdev initialization. > + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1); > + if (ret < 0) > + uvc_printk(KERN_ERR, "Failed to register metadata device (%d). \n", ret); > + > + return ret; > +} > diff --git a/drivers/media/usb/uvc/uvc_video.c > b/drivers/media/usb/uvc/uvc_video.c index b5589d5..1bda8e1 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -1158,8 +1158,30 @@ static void uvc_video_validate_buffer(const struct > uvc_streaming *stream, /* > * Completion handler for video URBs. > */ > +static void uvc_video_decode_meta(struct uvc_streaming *stream, > + struct uvc_buffer *buf, struct uvc_buffer *meta_buf, > + u8 *mem, int length) I don't think length can be negative, you can make it an unsigned int and replace min_t with min below. > +{ > + size_t nbytes; > + > + if (!meta_buf || !length) > + return; > + > + nbytes = min_t(unsigned int, length, meta_buf->length); > + > + 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; > + > + memcpy(meta_buf->mem, mem, nbytes); Do you need the whole header ? Shouldn't you strip the part already handled by the driver ? > + meta_buf->bytesused = nbytes; > + meta_buf->state = UVC_BUF_STATE_READY; > + > + uvc_queue_next_buffer(&stream->meta.queue, meta_buf); This essentially means that you'll need one buffer per isochronous packet. Given the number of isochronous packets that make a complete image the cost seem prohibitive to me. You should instead gather metadata from all headers into a single buffer. > +} > + > 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; > @@ -1189,6 +1211,8 @@ static void uvc_video_decode_isoc(struct urb *urb, > struct uvc_streaming *stream, if (ret < 0) > continue; > > + uvc_video_decode_meta(stream, buf, meta_buf, mem, ret); > + > /* Decode the payload data. */ > uvc_video_decode_data(stream, buf, mem + ret, > urb->iso_frame_desc[i].actual_length - ret); > @@ -1205,7 +1229,7 @@ static void uvc_video_decode_isoc(struct urb *urb, > struct uvc_streaming *stream, } > > static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming > *stream, > - struct uvc_buffer *buf) > + struct uvc_buffer *buf, struct uvc_buffer *meta_buf) > { > u8 *mem; > int len, ret; > @@ -1239,6 +1263,8 @@ static void uvc_video_decode_bulk(struct urb *urb, > struct uvc_streaming *stream, memcpy(stream->bulk.header, mem, ret); > stream->bulk.header_size = ret; > > + uvc_video_decode_meta(stream, buf, meta_buf, mem, ret); > + > mem += ret; > len -= ret; > } > @@ -1262,8 +1288,7 @@ static void uvc_video_decode_bulk(struct urb *urb, > struct uvc_streaming *stream, uvc_video_decode_end(stream, buf, > stream->bulk.header, > stream->bulk.payload_size); > if (buf->state == UVC_BUF_STATE_READY) > - buf = uvc_queue_next_buffer(&stream->queue, > - buf); > + uvc_queue_next_buffer(&stream->queue, buf); This could be split to a separate patch. > } > > stream->bulk.header_size = 0; > @@ -1273,7 +1298,7 @@ static void uvc_video_decode_bulk(struct urb *urb, > struct uvc_streaming *stream, } > > static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming > *stream, > - struct uvc_buffer *buf) > + struct uvc_buffer *buf, struct uvc_buffer *meta_buf) > { > u8 *mem = urb->transfer_buffer; > int len = stream->urb_size, ret; > @@ -1319,7 +1344,8 @@ static void uvc_video_complete(struct urb *urb) > { > struct uvc_streaming *stream = urb->context; > struct uvc_video_queue *queue = &stream->queue; > - struct uvc_buffer *buf = NULL; > + struct uvc_video_queue *qmeta = &stream->meta.queue; > + struct uvc_buffer *buf = NULL, *buf_meta = NULL; One variable per line please. > unsigned long flags; > int ret; > > @@ -1338,6 +1364,7 @@ static void uvc_video_complete(struct urb *urb) > case -ECONNRESET: /* usb_unlink_urb() called. */ > case -ESHUTDOWN: /* The endpoint is being disabled. */ > uvc_queue_cancel(queue, urb->status == -ESHUTDOWN); > + uvc_queue_cancel(qmeta, urb->status == -ESHUTDOWN); > return; > } > > @@ -1347,7 +1374,13 @@ static void uvc_video_complete(struct urb *urb) > queue); > spin_unlock_irqrestore(&queue->irqlock, flags); > > - stream->decode(urb, stream, buf); > + spin_lock_irqsave(&qmeta->irqlock, flags); > + if (!list_empty(&qmeta->irqqueue)) > + buf_meta = list_first_entry(&qmeta->irqqueue, struct uvc_buffer, > + queue); > + spin_unlock_irqrestore(&qmeta->irqlock, flags); > + > + stream->decode(urb, stream, buf, buf_meta); > > if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) { > uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n", > diff --git a/drivers/media/usb/uvc/uvcvideo.h > b/drivers/media/usb/uvc/uvcvideo.h index 3d6cc62..ebff4b6 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -464,6 +464,11 @@ struct uvc_stats_stream { > unsigned int max_sof; /* Maximum STC.SOF value */ > }; > > +struct uvc_meta_dev { All other structures use _device, not _dev. Let's be consistent. > + struct video_device vdev; > + struct uvc_video_queue queue; > +}; > + > struct uvc_streaming { > struct list_head list; > struct uvc_device *dev; > @@ -495,7 +500,9 @@ struct uvc_streaming { > unsigned int frozen : 1; > struct uvc_video_queue queue; > void (*decode) (struct urb *urb, struct uvc_streaming *video, > - struct uvc_buffer *buf); > + struct uvc_buffer *buf, struct uvc_buffer *meta_buf); > + > + struct uvc_meta_dev meta; > > /* Context data used by the bulk completion handler. */ > struct { > @@ -700,6 +707,7 @@ extern int uvc_query_ctrl(struct uvc_device *dev, __u8 > query, __u8 unit, void uvc_video_clock_update(struct uvc_streaming *stream, > struct vb2_v4l2_buffer *vbuf, > struct uvc_buffer *buf); > +int uvc_meta_register(struct uvc_streaming *stream); > > /* Status */ > extern int uvc_status_init(struct uvc_device *dev); > @@ -754,7 +762,7 @@ extern struct usb_host_endpoint *uvc_find_endpoint( > > /* Quirks support */ > void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream, > - struct uvc_buffer *buf); > + struct uvc_buffer *buf, struct uvc_buffer *meta_buf); > > /* debugfs and statistics */ > int uvc_debugfs_init(void); > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c > b/drivers/media/v4l2-core/v4l2-ioctl.c index 44a29af..1618be4 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -1232,6 +1232,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) > case V4L2_TCH_FMT_DELTA_TD08: descr = "8-bit signed deltas"; break; case > V4L2_TCH_FMT_TU16: descr = "16-bit unsigned touch data"; break; case > V4L2_TCH_FMT_TU08: descr = "8-bit unsigned touch data"; break; > + case > V4L2_META_FMT_UVC: descr = "UVC payload header metadata"; break; > > default: > /* Compressed formats */ > diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h > index 3b08186..e98de14 100644 > --- a/include/uapi/linux/uvcvideo.h > +++ b/include/uapi/linux/uvcvideo.h > @@ -67,4 +67,14 @@ 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 > + */ > + > +/* > + * Actually 255 bytes, but 256 is just a nicer number. We keep the buffer > size > + * constant and just set .usedbytes accordingly > + */ > +#define UVC_PAYLOAD_HEADER_MAX_SIZE 256 Why is lying about the size better ? > #endif -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
