Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Wednesday, 6 December 2017 17:08:14 EET Guennadi Liakhovetski wrote:
> Hi Laurent,
> 
> While testing the new patch version, we did introduce a couple of
> differences:
> 
> 1. We cannot (easily) reuse .vidioc_querycap() - the metadata node uses
> v4l2_fh_open() directly, so, it has a different struct file::private_data
> pointer.

OK, no problem, it's not a big deal.µ

> 2. After your video device unification, the order has swapped: now
> /dev/video0 is a metadata node and /dev/video1 is a video node. Is that
> how you wanted to have this or you don't mind or shall I swap them back?
> For now I've swapped them back, I think that would be more appropriate.

I agree, that's better, thank you.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

Hi Laurent,

While testing the new patch version, we did introduce a couple of 
differences:

1. We cannot (easily) reuse .vidioc_querycap() - the metadata node uses 
v4l2_fh_open() directly, so, it has a different struct file::private_data 
pointer.

2. After your video device unification, the order has swapped: now 
/dev/video0 is a metadata node and /dev/video1 is a video node. Is that 
how you wanted to have this or you don't mind or shall I swap them back? 
For now I've swapped them back, I think that would be more appropriate.

Thanks
Guennadi

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

On Tue, 5 Dec 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday, 5 December 2017 15:44:34 EET Guennadi Liakhovetski wrote:
> > On Tue, 5 Dec 2017, Laurent Pinchart wrote:
> > > 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
> >  
> >  ---
> >  
> >  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 000..219
> >  --- /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.
> > 
> > Hm, but that's how applications would naturally work - open a node, query
> > its capabilities and handle it accordingly. What good would be having
> > equal capabilities on all nodes? Why do you think that should be the case?
> 
> Quoting 
> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-querycap.html,
> 
> __u32 capabilities
> 
> Available capabilities of the 

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Tuesday, 5 December 2017 15:44:34 EET Guennadi Liakhovetski wrote:
> On Tue, 5 Dec 2017, Laurent Pinchart wrote:
> > 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
>  
>  ---
>  
>  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 000..219
>  --- /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.
> 
> Hm, but that's how applications would naturally work - open a node, query
> its capabilities and handle it accordingly. What good would be having
> equal capabilities on all nodes? Why do you think that should be the case?

Quoting 
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-querycap.html,

__u32   capabilities

Available capabilities of the physical device as a whole, see Device 
Capabilities Flags. The same physical device can export multiple devices in /
dev (e.g. /dev/videoX, /dev/vbiY and /dev/radioZ). The capabilities field 
should contain a union of all capabilities available around 

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

On Tue, 5 Dec 2017, Laurent Pinchart wrote:

> 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 
> > >> ---
> > >> 
> > >> 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 000..219
> > >> --- /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.

Hm, but that's how applications would naturally work - open a node, query 
its capabilities and handle it accordingly. What good would be having 
equal capabilities on all nodes? Why do you think that should be the case?

> > >> +
> > >> +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 = >fmt.meta;
> > >> +
> > >> +if (format->type != vfh->vdev->queue->type)
> > >> +return -EINVAL;
> > >> +
> > >> +memset(fmt, 0, sizeof(*fmt));
> 

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Laurent Pinchart]

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 
> >> ---
> >> 
> >> 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 000..219
> >> --- /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 = >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]

> > > 

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

Hi Laurent,

Thanks for a review.

On Tue, 5 Dec 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch. We're getting very close, I only have small 
> comments, 
> please see below.
> 
> 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.
"

> > Signed-off-by: Guennadi Liakhovetski 
> > ---
> > 
> > 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_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index cbf79b9..5f7ce97 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1877,6 +1877,7 @@ static void uvc_unregister_video(struct uvc_device
> > *dev) continue;
> > 
> > video_unregister_device(>vdev);
> > +   video_unregister_device(>meta.vdev);
> > 
> > uvc_debugfs_cleanup_stream(stream);
> > }
> > @@ -1934,6 +1935,11 @@ static int uvc_register_video(struct uvc_device *dev,
> > return ret;
> > }
> > 
> > +   /* Register a metadata node, but ignore a possible failure, complete
> > +* registration of video nodes anyway.
> > +*/
> > +   uvc_meta_register(stream);
> > +
> > if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
> > else
> > @@ -2003,6 +2009,7 @@ static int uvc_register_chains(struct uvc_device *dev)
> > 
> >  struct uvc_device_info {
> > u32 quirks;
> > +   u32 meta_format;
> >  };
> > 
> >  static int uvc_probe(struct usb_interface *intf,
> > @@ -2038,8 +2045,16 @@ static int uvc_probe(struct usb_interface *intf,
> > dev->udev = usb_get_dev(udev);
> > dev->intf = usb_get_intf(intf);
> > dev->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
> > +   if (uvc_quirks_param != -1 &&
> > +   uvc_quirks_param & UVC_DEV_FLAG_METADATA_NODE) {
> > +   uvc_quirks_param &= ~UVC_DEV_FLAG_METADATA_NODE;
> > +   if (uvc_quirks_param == 0)
> > +   uvc_quirks_param = -1;
> > +   }
> 
> I think we can remove the UVC_DEV_FLAG_METADATA_NODE flag. We can use the 
> metadata format specified in the device information structure if present, and 
> default to the standard metadata format otherwise. A metadata video node will 
> always be registered, which I think is the right thing to do as exposing 
> timestamp information to userspace is useful.
> 
> Do you see any adverse effect of registering a metadata video node 
> unconditionally ?

No, apart from confusion on behalf of applications and users.

> > dev->quirks = (uvc_quirks_param == -1)
> > ? quirks : uvc_quirks_param;
> > +   if (info)
> > +   dev->meta_format = info->meta_format;

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Tuesday, 5 December 2017 10:06:02 EET Guennadi Liakhovetski wrote:
> On Tue, 5 Dec 2017, Laurent Pinchart wrote:
> > On Tuesday, 5 December 2017 02:24:30 EET Laurent Pinchart wrote:
> >> On Wednesday, 8 November 2017 18:00:14 EET Guennadi Liakhovetski wrote:
> > [snip]
> > 
> >>> +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> >>> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf,
> >>> + u8 *mem, unsigned int length)
> >> 
> >> The buf parameter is unused, you can remove it. mem isn't modified, I
> >> would make it const.
> >> 
> >>> +{
> >>> + struct uvc_meta_buf *meta;
> >>> + size_t len_std = 2;
> >>> + bool has_pts, has_scr;
> >>> + unsigned long flags;
> >>> + struct timespec ts;
> >>> + u8 *scr;
> >> 
> >> And scr should be const too.
> >> 
> >>> +
> >>> + 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.
> >> 
> >>> + 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();
> >> 
> >> FYI, Arnd has posted https://patchwork.kernel.org/patch/10076887/. If
> >> the patch gets merged first I can help with the rebasing.
> > 
> > I've reviewed and merged Arnd patches in my tree, and...
> > 
> >>> + meta->sof = usb_get_current_frame_number(stream->dev->udev);
> >>> + local_irq_restore(flags);
> >>> + meta->ns = timespec_to_ns();
> >> 
> >> 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 :-)).
> >> 
> >>> + if (has_scr)
> >>> + memcpy(stream->clock.last_scr, scr, 6);
> >>> +
> >>> + memcpy(>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
> 
> In your rebased version you've also dropped the hunk for
> drivers/media/v4l2-core/v4l2-ioctl.c adding a description for the new
> V4L2_META_FMT_UVC format - is that on purpose?

Unless I'm mistaken I've moved it to patch 1/3.

> >>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.
> > 
> > ... updated the above branch with a rebased version of the series.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

Hi Laurent,

On Tue, 5 Dec 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday, 5 December 2017 02:24:30 EET Laurent Pinchart wrote:
> > On Wednesday, 8 November 2017 18:00:14 EET Guennadi Liakhovetski wrote:
> 
> [snip]
> 
> > > +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> > > + struct uvc_buffer *buf, struct uvc_buffer *meta_buf,
> > > + u8 *mem, unsigned int length)
> > 
> > The buf parameter is unused, you can remove it. mem isn't modified, I would
> > make it const.
> > 
> > > +{
> > > + struct uvc_meta_buf *meta;
> > > + size_t len_std = 2;
> > > + bool has_pts, has_scr;
> > > + unsigned long flags;
> > > + struct timespec ts;
> > > + u8 *scr;
> > 
> > And scr should be const too.
> > 
> > > +
> > > + 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.
> > 
> > > + 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();
> > 
> > FYI, Arnd has posted https://patchwork.kernel.org/patch/10076887/. If the
> > patch gets merged first I can help with the rebasing.
> 
> I've reviewed and merged Arnd patches in my tree, and...
> 
> > > + meta->sof = usb_get_current_frame_number(stream->dev->udev);
> > > + local_irq_restore(flags);
> > > + meta->ns = timespec_to_ns();
> > 
> > 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 :-)).
> > 
> > > + if (has_scr)
> > > + memcpy(stream->clock.last_scr, scr, 6);
> > > +
> > > + memcpy(>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

In your rebased version you've also dropped the hunk for 
drivers/media/v4l2-core/v4l2-ioctl.c adding a description for the new 
V4L2_META_FMT_UVC format - is that on purpose?

Thanks
Guennadi

> > 
> > 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.
> 
> ... updated the above branch with a rebased version of the series.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Tuesday, 5 December 2017 02:24:30 EET Laurent Pinchart wrote:
> On Wednesday, 8 November 2017 18:00:14 EET Guennadi Liakhovetski wrote:

[snip]

> > +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> > +   struct uvc_buffer *buf, struct uvc_buffer *meta_buf,
> > +   u8 *mem, unsigned int length)
> 
> The buf parameter is unused, you can remove it. mem isn't modified, I would
> make it const.
> 
> > +{
> > +   struct uvc_meta_buf *meta;
> > +   size_t len_std = 2;
> > +   bool has_pts, has_scr;
> > +   unsigned long flags;
> > +   struct timespec ts;
> > +   u8 *scr;
> 
> And scr should be const too.
> 
> > +
> > +   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.
> 
> > +   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();
> 
> FYI, Arnd has posted https://patchwork.kernel.org/patch/10076887/. If the
> patch gets merged first I can help with the rebasing.

I've reviewed and merged Arnd patches in my tree, and...

> > +   meta->sof = usb_get_current_frame_number(stream->dev->udev);
> > +   local_irq_restore(flags);
> > +   meta->ns = timespec_to_ns();
> 
> 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 :-)).
> 
> > +   if (has_scr)
> > +   memcpy(stream->clock.last_scr, scr, 6);
> > +
> > +   memcpy(>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.

... updated the above branch with a rebased version of the series.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch. We're getting very close, I only have small comments, 
please see below.

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 ?

> Signed-off-by: Guennadi Liakhovetski 
> ---
> 
> 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_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index cbf79b9..5f7ce97 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1877,6 +1877,7 @@ static void uvc_unregister_video(struct uvc_device
> *dev) continue;
> 
>   video_unregister_device(>vdev);
> + video_unregister_device(>meta.vdev);
> 
>   uvc_debugfs_cleanup_stream(stream);
>   }
> @@ -1934,6 +1935,11 @@ static int uvc_register_video(struct uvc_device *dev,
> return ret;
>   }
> 
> + /* Register a metadata node, but ignore a possible failure, complete
> +  * registration of video nodes anyway.
> +  */
> + uvc_meta_register(stream);
> +
>   if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>   stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
>   else
> @@ -2003,6 +2009,7 @@ static int uvc_register_chains(struct uvc_device *dev)
> 
>  struct uvc_device_info {
>   u32 quirks;
> + u32 meta_format;
>  };
> 
>  static int uvc_probe(struct usb_interface *intf,
> @@ -2038,8 +2045,16 @@ static int uvc_probe(struct usb_interface *intf,
>   dev->udev = usb_get_dev(udev);
>   dev->intf = usb_get_intf(intf);
>   dev->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
> + if (uvc_quirks_param != -1 &&
> + uvc_quirks_param & UVC_DEV_FLAG_METADATA_NODE) {
> + uvc_quirks_param &= ~UVC_DEV_FLAG_METADATA_NODE;
> + if (uvc_quirks_param == 0)
> + uvc_quirks_param = -1;
> + }

I think we can remove the UVC_DEV_FLAG_METADATA_NODE flag. We can use the 
metadata format specified in the device information structure if present, and 
default to the standard metadata format otherwise. A metadata video node will 
always be registered, which I think is the right thing to do as exposing 
timestamp information to userspace is useful.

Do you see any adverse effect of registering a metadata video node 
unconditionally ?

>   dev->quirks = (uvc_quirks_param == -1)
>   ? quirks : uvc_quirks_param;
> + if (info)
> + dev->meta_format = info->meta_format;
> 
>   if (udev->product != NULL)
>   strlcpy(dev->name, udev->product, sizeof dev->name);

[snip]

> diff --git a/drivers/media/usb/uvc/uvc_metadata.c
> b/drivers/media/usb/uvc/uvc_metadata.c new file mode 100644
> index 000..219
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_metadata.c

[snip]

> +/* 
> + * videobuf2 Queue Operations
> + */
> +

You can remove this now that the section is empty :-)

> +/* 
> + * V4L2 ioctls
> + */
> +
> +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", 

[PATCH 3/3 v7] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

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.

Signed-off-by: Guennadi Liakhovetski 
---

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

diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
index c26d12f..06c7cd3 100644
--- a/drivers/media/usb/uvc/Makefile
+++ b/drivers/media/usb/uvc/Makefile
@@ -1,5 +1,5 @@
 uvcvideo-objs  := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
- uvc_status.o uvc_isight.o uvc_debugfs.o
+ uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
 ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
 uvcvideo-objs  += uvc_entity.o
 endif
diff --git a/drivers/media/usb/uvc/uvc_driver.c 
b/drivers/media/usb/uvc/uvc_driver.c
index cbf79b9..5f7ce97 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1877,6 +1877,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
continue;
 
video_unregister_device(>vdev);
+   video_unregister_device(>meta.vdev);
 
uvc_debugfs_cleanup_stream(stream);
}
@@ -1934,6 +1935,11 @@ static int uvc_register_video(struct uvc_device *dev,
return ret;
}
 
+   /* Register a metadata node, but ignore a possible failure, complete
+* registration of video nodes anyway.
+*/
+   uvc_meta_register(stream);
+
if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
else
@@ -2003,6 +2009,7 @@ static int uvc_register_chains(struct uvc_device *dev)
 
 struct uvc_device_info {
u32 quirks;
+   u32 meta_format;
 };
 
 static int uvc_probe(struct usb_interface *intf,
@@ -2038,8 +2045,16 @@ static int uvc_probe(struct usb_interface *intf,
dev->udev = usb_get_dev(udev);
dev->intf = usb_get_intf(intf);
dev->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
+   if (uvc_quirks_param != -1 &&
+   uvc_quirks_param & UVC_DEV_FLAG_METADATA_NODE) {
+   uvc_quirks_param &= ~UVC_DEV_FLAG_METADATA_NODE;
+   if (uvc_quirks_param == 0)
+   uvc_quirks_param = -1;
+   }
dev->quirks = (uvc_quirks_param == -1)
? quirks : uvc_quirks_param;
+   if (info)
+   dev->meta_format = info->meta_format;
 
if (udev->product != NULL)
strlcpy(dev->name, udev->product, sizeof dev->name);
diff --git a/drivers/media/usb/uvc/uvc_isight.c 
b/drivers/media/usb/uvc/uvc_isight.c
index 8510e725..fb940cf 100644
--- a/drivers/media/usb/uvc/uvc_isight.c
+++ b/drivers/media/usb/uvc/uvc_isight.c
@@ -100,7 +100,7 @@ static int isight_decode(struct uvc_video_queue *queue, 
struct uvc_buffer *buf,
 }
 
 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)
 {
int ret, i;
 
diff --git a/drivers/media/usb/uvc/uvc_metadata.c 
b/drivers/media/usb/uvc/uvc_metadata.c
new file mode 100644
index 000..219
--- /dev/null
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -0,0 +1,204 @@
+/*
+ *  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