Hi Guennadi,
On Tuesday, 5 December 2017 12:56:53 EET Guennadi Liakhovetski wrote:
> On Tue, 5 Dec 2017, Laurent Pinchart wrote:
> > On Wednesday, 8 November 2017 18:00:14 EET Guennadi Liakhovetski wrote:
> >> 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. This
> >> version of the patch only creates such metadata nodes for cameras,
> >> specifying a UVC_QUIRK_METADATA_NODE quirk flag.
> >
> > I don't think this is correct anymore, as we'll use different 4CCs for
> > different vendor metadata. How would you like to rephrase the commit
> > message ?
>
> Something like
>
> "
> 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. By default, only the
> V4L2_META_FMT_UVC pixel format is available from those nodes. However,
> cameras can be added to the device ID table to additionally specify their
> own metadata format, in which case that format will also become available
> from the metadata node.
> "
Sounds good to me.
> >> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovet...@intel.com>
> >> ---
> >>
> >> v7: support up to two metadata formats per camera - the standard one and
> >> an optional private one, if specified in device information
> >>
> >> drivers/media/usb/uvc/Makefile | 2 +-
> >> drivers/media/usb/uvc/uvc_driver.c | 15 +++
> >> drivers/media/usb/uvc/uvc_isight.c | 2 +-
> >> drivers/media/usb/uvc/uvc_metadata.c | 204 +++++++++++++++++++++++++++++
> >> drivers/media/usb/uvc/uvc_queue.c | 41 +++++--
> >> drivers/media/usb/uvc/uvc_video.c | 127 ++++++++++++++++++++--
> >> drivers/media/usb/uvc/uvcvideo.h | 19 +++-
> >> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> >> include/uapi/linux/uvcvideo.h | 26 +++++
> >> 9 files changed, 416 insertions(+), 21 deletions(-)
> >> create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
[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..21eeee9
> >> --- /dev/null
> >> +++ b/drivers/media/usb/uvc/uvc_metadata.c
[snip]
> >> +static int uvc_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);
> >> +
> >> + 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;
> >> +}
> >
> > Do you think we could reuse uvc_ioctl_querycap() as-is ?
>
> AFAICS it still has
>
> cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
> | chain->caps;
>
> in it, which doesn't suit the metadata node?
I'd say this is debatable, isn't the capabilities field supposed to include
all capabilities from all video nodes for the device ? chain->caps would need
to include metadata capability in that case.
Code reuse is not a big deal as the function is small, but getting the
capabilities value right is important regardless.
> >> +
> >> +static int uvc_meta_v4l2_get_format(struct file *file, void *fh,
> >> + struct v4l2_format *format)
> >> +{
> >> + struct v4l2_fh *vfh = file->private_data;
> >> + struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
> >> + struct v4l2_meta_format *fmt = &format->fmt.meta;
> >> +
> >> + if (format->type != vfh->vdev->queue->type)
> >> + return -EINVAL;
> >> +
> >> + memset(fmt, 0, sizeof(*fmt));
> >> +
> >> + fmt->dataformat = stream->cur_meta_format;
> >> + fmt->buffersize = UVC_METATADA_BUF_SIZE;
> >
> > You need to take the stream->mutex lock here to protect against races with
> > the set format ioctl.
>
> Well, strictly speaking you don't? The buffersize is constant and getting
> the current metadata format is an atomic read.
I was concerned by the race condition due to lack of memory barriers, but if
userspace issues concurrent G_FMT and S_FMT calls the order isn't guaranteed
anyway, so you're right about that.
[snip]
> > > diff --git a/drivers/media/usb/uvc/uvc_queue.c
> > > b/drivers/media/usb/uvc/uvc_queue.c index c8d78b2..b2998f5 100644
> > > --- a/drivers/media/usb/uvc/uvc_queue.c
> > > +++ b/drivers/media/usb/uvc/uvc_queue.c
>
> [snip]
>
> >> + if (!meta_buf || length == 2 ||
> >> + meta_buf->length - meta_buf->bytesused <
> >> + length + sizeof(meta->ns) + sizeof(meta->sof))
> >> + return;
> >
> > If the metadata buffer overflows should we also set the error bit like we
> > do for video buffers ? I have mixed feelings about this, I'd appreciate
> > your input.
>
> I think it would be good to know if we ever run into such overruns.
> Setting an error flag is more likely to be noticed and reported than a
> printk()?
I believe so, yes.
> >> + 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 (stream->cur_meta_format == V4L2_META_FMT_UVC)
> >> + length = len_std;
> >> +
> >> + if (length == len_std && (!has_scr ||
> >> + !memcmp(scr, stream->clock.last_scr, 6)))
> >> + return;
> >> +
> >> + meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem +
> >> meta_buf->bytesused);
> >> + local_irq_save(flags);
> >> + uvc_video_get_ts(&ts);
> >
> > FYI, Arnd has posted https://patchwork.kernel.org/patch/10076887/. If the
> > patch gets merged first I can help with the rebasing.
> >
> >> + meta->sof = usb_get_current_frame_number(stream->dev->udev);
> >> + local_irq_restore(flags);
> >> + meta->ns = timespec_to_ns(&ts);
> >
> > The meta pointer can be unaligned as the structure is packed and its size
> > isn't a multiple of the size of the largest field (and it can contain an
> > unspecified amount of vendor data anyway). You thus can't access it
> > directly on all architectures, you will need to use the put_unaligned
> > macro. As I haven't checked whether all architectures can handle
> > unaligned accesses without generating a trap, I would store the USB frame
> > number in a local variable and use the put_unaligned macro output of the
> > IRQ disabled section (feel free to show me that I'm unnecessarily
> > cautious :-)).
>
> I think you're right. But __put_unaligned_cpu64() is an inline function,
> not a macro, so, no local variable is needed?
Even if it's a function I'm not 100% confident that it can work well with
interrupts disabled on all architectures, but I'm probably overcautious.
> >> + if (has_scr)
> >> + memcpy(stream->clock.last_scr, scr, 6);
> >> +
> >> + memcpy(&meta->length, mem, length);
> >> + meta_buf->bytesused += length + sizeof(meta->ns) + 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__, ts.tv_sec, 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);
> >> +}
[snip]
> > For your convenience I've rebased your patch series on top of the two
> > patches I mentioned and added another patch on top that contains fixes
> > for all the small issues mentioned above. The result is available at
> >
> > git://linuxtv.org/pinchartl/media.git uvc/metadata
> >
> > There are just a handful of issues or questions I haven't addressed, if we
> > handle them I think we'll be good to go.
>
> Thanks for rebasing. I've addressed the remaining issues, and once I get
> your answers to remaining questions above, I'll send an updated version.
Done :-)
Thank you.
--
Regards,
Laurent Pinchart
