Re: [PATCH 4/6 v5] uvcvideo: add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Wednesday, 18 October 2017 10:32:10 EEST Guennadi Liakhovetski wrote:
> On Fri, 28 Jul 2017, Hans Verkuil wrote:
> > On 07/28/2017 02:33 PM, 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.
> >> 
> >> Signed-off-by: Guennadi Liakhovetski 
> >> ---
> >> 
> >>  drivers/media/usb/uvc/Makefile   |   2 +-
> >>  drivers/media/usb/uvc/uvc_ctrl.c |   3 +
> >>  drivers/media/usb/uvc/uvc_driver.c   |  12 +++
> >>  drivers/media/usb/uvc/uvc_isight.c   |   2 +-
> >>  drivers/media/usb/uvc/uvc_metadata.c | 139 +
> >>  drivers/media/usb/uvc/uvc_queue.c|  41 +--
> >>  drivers/media/usb/uvc/uvc_video.c| 119 +---
> >>  drivers/media/usb/uvc/uvcvideo.h |  17 -
> >>  drivers/media/v4l2-core/v4l2-ioctl.c |   1 +
> >>  include/uapi/linux/uvcvideo.h|  26 +++
> >>  10 files changed, 341 insertions(+), 21 deletions(-)
> >>  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
> 
> [snip]
> 
> >> diff --git a/include/uapi/linux/uvcvideo.h
> >> b/include/uapi/linux/uvcvideo.h
> >> index 3b08186..ffe17ec 100644
> >> --- a/include/uapi/linux/uvcvideo.h
> >> +++ b/include/uapi/linux/uvcvideo.h
> >> @@ -67,4 +67,30 @@ 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
> >> + */
> >> +
> >> +/**
> >> + * struct uvc_meta_buf - metadata buffer building block
> >> + * @ns- system timestamp of the payload in nanoseconds
> >> + * @sof   - USB Frame Number
> >> + * @length- length of the payload header
> >> + * @flags - payload header flags
> >> + * @buf   - optional device-specific header data
> >> + *
> >> + * UVC metadata nodes fill buffers with possibly multiple instances of
> >> this
> >> + * 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 {
> > > + __u64 ns;
> > > + __u16 sof;
> > > + __u8 length;
> > > + __u8 flags;
> > 
> > I think the struct layout is architecture dependent. I could be wrong, but
> > I think for x64 there is a 4-byte hole here, but not for arm32/arm64.
> > 
> > Please double-check the struct layout.
> 
> You mean this can be a problem for 64- / 32-bit compatibility? If the
> difference is just between ARM and X86 then we don't care, do we? Or do
> video buffers have to be safe for cross-system transfer?

The structure size is 12 bytes on x86-32 and 16 bytes on x86-64, arm32 and 
arm64 (using the GNU EABI).

Given that the structure is meant to describe the content of the buffer, it 
would likely be better to make it packed.

> > BTW: __u8 for length? The payload can never be longer in UVC?
> 
> No, not the payload, a single payload header cannot be larger than 255
> bytes, that's right.
> 
> >> +  __u8 buf[];
> >> +};
> >> +
> >> 
> >>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6 v5] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

Hi Hans,

Thanks for your comment.

On Fri, 28 Jul 2017, Hans Verkuil wrote:

> On 07/28/2017 02:33 PM, 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.
> > 
> > Signed-off-by: Guennadi Liakhovetski 
> > ---
> >  drivers/media/usb/uvc/Makefile   |   2 +-
> >  drivers/media/usb/uvc/uvc_ctrl.c |   3 +
> >  drivers/media/usb/uvc/uvc_driver.c   |  12 +++
> >  drivers/media/usb/uvc/uvc_isight.c   |   2 +-
> >  drivers/media/usb/uvc/uvc_metadata.c | 139 
> > +++
> >  drivers/media/usb/uvc/uvc_queue.c|  41 +--
> >  drivers/media/usb/uvc/uvc_video.c| 119 +++---
> >  drivers/media/usb/uvc/uvcvideo.h |  17 -
> >  drivers/media/v4l2-core/v4l2-ioctl.c |   1 +
> >  include/uapi/linux/uvcvideo.h|  26 +++
> >  10 files changed, 341 insertions(+), 21 deletions(-)
> >  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c

[snip]

> > diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> > index 3b08186..ffe17ec 100644
> > --- a/include/uapi/linux/uvcvideo.h
> > +++ b/include/uapi/linux/uvcvideo.h
> > @@ -67,4 +67,30 @@ 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
> > + */
> > +
> > +/**
> > + * struct uvc_meta_buf - metadata buffer building block
> > + * @ns - system timestamp of the payload in nanoseconds
> > + * @sof- USB Frame Number
> > + * @length - length of the payload header
> > + * @flags  - payload header flags
> > + * @buf- optional device-specific header data
> > + *
> > + * UVC metadata nodes fill buffers with possibly multiple instances of this
> > + * 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 {
> > +   __u64 ns;
> > +   __u16 sof;
> > +   __u8 length;
> > +   __u8 flags;
> 
> I think the struct layout is architecture dependent. I could be wrong, but I 
> think
> for x64 there is a 4-byte hole here, but not for arm32/arm64.
> 
> Please double-check the struct layout.

You mean this can be a problem for 64- / 32-bit compatibility? If the 
difference is just between ARM and X86 then we don't care, do we? Or do 
video buffers have to be safe for cross-system transfer?

> BTW: __u8 for length? The payload can never be longer in UVC?

No, not the payload, a single payload header cannot be larger than 255 
bytes, that's right.

Thanks
Guennadi

> Regards,
> 
>   Hans
> 
> > +   __u8 buf[];
> > +};
> > +
> >  #endif
> > 
> 

Re: [PATCH 4/6 v5] uvcvideo: add a metadata device node

[Guennadi Liakhovetski]

Hi Hans,

On Fri, 28 Jul 2017, Hans Verkuil wrote:

> On 07/28/2017 02:33 PM, Guennadi Liakhovetski wrote:

[snip]

> > +/**
> > + * struct uvc_meta_buf - metadata buffer building block
> > + * @ns - system timestamp of the payload in nanoseconds
> > + * @sof- USB Frame Number
> > + * @length - length of the payload header
> > + * @flags  - payload header flags
> > + * @buf- optional device-specific header data
> > + *
> > + * UVC metadata nodes fill buffers with possibly multiple instances of this
> > + * 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 {
> > +   __u64 ns;
> > +   __u16 sof;
> > +   __u8 length;
> > +   __u8 flags;
> 
> I think the struct layout is architecture dependent. I could be wrong, but I 
> think
> for x64 there is a 4-byte hole here, but not for arm32/arm64.
> 
> Please double-check the struct layout.

It worked for me so far on an x86-64 system, I was able to access buffer 
data correctly, but yes, it's probably safer to use __packed.

> BTW: __u8 for length? The payload can never be longer in UVC?

No, it cannot. That's exactly how UVC defines it.

Thanks
Guennadi

> 
> Regards,
> 
>   Hans
> 
> > +   __u8 buf[];
> > +};
> > +
> >  #endif
> > 
> 

Re: [PATCH 4/6 v5] uvcvideo: add a metadata device node

[Hans Verkuil]

On 07/28/2017 02:33 PM, 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.
> 
> Signed-off-by: Guennadi Liakhovetski 
> ---
>  drivers/media/usb/uvc/Makefile   |   2 +-
>  drivers/media/usb/uvc/uvc_ctrl.c |   3 +
>  drivers/media/usb/uvc/uvc_driver.c   |  12 +++
>  drivers/media/usb/uvc/uvc_isight.c   |   2 +-
>  drivers/media/usb/uvc/uvc_metadata.c | 139 
> +++
>  drivers/media/usb/uvc/uvc_queue.c|  41 +--
>  drivers/media/usb/uvc/uvc_video.c| 119 +++---
>  drivers/media/usb/uvc/uvcvideo.h |  17 -
>  drivers/media/v4l2-core/v4l2-ioctl.c |   1 +
>  include/uapi/linux/uvcvideo.h|  26 +++
>  10 files changed, 341 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_ctrl.c 
> b/drivers/media/usb/uvc/uvc_ctrl.c
> index c2ee6e3..91ff2c7 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2179,6 +2179,9 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>   ctrl->entity = entity;
>   ctrl->index = i;
>  
> + if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT)
> + uvc_trace(UVC_TRACE_CONTROL, "XU %u: ctrl 
> %d\n", entity->id, i);
> +
>   uvc_ctrl_init_ctrl(dev, ctrl);
>   ctrl++;
>   }
> diff --git a/drivers/media/usb/uvc/uvc_driver.c 
> b/drivers/media/usb/uvc/uvc_driver.c
> index cfe33bf..3d61cec 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1884,6 +1884,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);
>   }
> @@ -1944,6 +1945,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
> @@ -2041,6 +2047,12 @@ 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)
>   ? id->driver_info : uvc_quirks_param;
>  
> 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..062e6ec
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -0,0 +1,139 @@
> +/*
> + *  uvc_metadata.c  --  

[PATCH 4/6 v5] 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 
---
 drivers/media/usb/uvc/Makefile   |   2 +-
 drivers/media/usb/uvc/uvc_ctrl.c |   3 +
 drivers/media/usb/uvc/uvc_driver.c   |  12 +++
 drivers/media/usb/uvc/uvc_isight.c   |   2 +-
 drivers/media/usb/uvc/uvc_metadata.c | 139 +++
 drivers/media/usb/uvc/uvc_queue.c|  41 +--
 drivers/media/usb/uvc/uvc_video.c| 119 +++---
 drivers/media/usb/uvc/uvcvideo.h |  17 -
 drivers/media/v4l2-core/v4l2-ioctl.c |   1 +
 include/uapi/linux/uvcvideo.h|  26 +++
 10 files changed, 341 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_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index c2ee6e3..91ff2c7 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2179,6 +2179,9 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
ctrl->entity = entity;
ctrl->index = i;
 
+   if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT)
+   uvc_trace(UVC_TRACE_CONTROL, "XU %u: ctrl 
%d\n", entity->id, i);
+
uvc_ctrl_init_ctrl(dev, ctrl);
ctrl++;
}
diff --git a/drivers/media/usb/uvc/uvc_driver.c 
b/drivers/media/usb/uvc/uvc_driver.c
index cfe33bf..3d61cec 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1884,6 +1884,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);
}
@@ -1944,6 +1945,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
@@ -2041,6 +2047,12 @@ 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)
? id->driver_info : uvc_quirks_param;
 
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..062e6ec
--- /dev/null
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -0,0 +1,139 @@
+/*
+ *  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