Re: [PATCH v8] uvcvideo: Add a metadata device node

[Guennadi Liakhovetski]

Hi Laurent,

On Wed, 13 Dec 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday, 12 December 2017 10:30:39 EET Guennadi Liakhovetski wrote:
> > On Mon, 11 Dec 2017, Laurent Pinchart wrote:
> > > On Monday, 11 December 2017 23:44:09 EET Guennadi Liakhovetski wrote:
> > >> On Mon, 11 Dec 2017, Laurent Pinchart wrote:
> > >>> On Monday, 11 December 2017 22:16:23 EET Laurent Pinchart wrote:
> >  On Wednesday, 6 December 2017 17:15:40 EET Guennadi Liakhovetski wrote:
> > > From: 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. 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
> > > 
> > > ---
> > > 
> > > v8: addressed comments and integrated changes from Laurent, thanks
> > > again, e.g.:
> > > 
> > > - multiple stylistic changes
> > > - remove the UVC_DEV_FLAG_METADATA_NODE flag / quirk: nodes are now
> > >   created unconditionally
> > > - reuse uvc_ioctl_querycap()
> > > - reuse code in uvc_register_video()
> > > - set an error flag when the metadata buffer overflows
> > > 
> > >  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 | 179+++
> > >  drivers/media/usb/uvc/uvc_queue.c|  44 +++--
> > >  drivers/media/usb/uvc/uvc_video.c| 132 +++--
> > >  drivers/media/usb/uvc/uvcvideo.h |  16 +++-
> > >  include/uapi/linux/uvcvideo.h|  26 +
> > >  8 files changed, 394 insertions(+), 22 deletions(-)
> > >  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
> >  
> >  [snip]
> >  
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > > b/drivers/media/usb/uvc/uvc_video.c index 13f459e..2fc0bf2 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> >  
> >  [snip]
> >  
> > > +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> > > +   struct uvc_buffer *meta_buf,
> > > +   const u8 *mem, unsigned int length)
> > > +{
> > > + struct uvc_meta_buf *meta;
> > > + size_t len_std = 2;
> > > + bool has_pts, has_scr;
> > > + unsigned long flags;
> > > + ktime_t time;
> > > + const u8 *scr;
> 
> [snip]
> 
> > > + meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem +
> > > meta_buf->bytesused);
> > > + local_irq_save(flags);
> > > + time = uvc_video_get_time();
> > > + meta->sof = usb_get_current_frame_number(stream->dev->udev);
> >  
> >  You need a put_unaligned here too. If you're fine with the patch
> >  below there's no need to resubmit, and
> > >>> 
> > >>> One more thing, __put_unaligned_cpu16() and __put_unaligned_cpu64()
> > >>> don't compile on x86_64 with v4.12 (using media_build.git). I propose
> > >>> replacing them with put_unaligned() which compiles and should do the
> > >>> right thing.
> > >> 
> > >> Sure, thanks for catching! Shall I fix and resubmit?
> > > 
> > > If you're fine with
> > > 
> > >   git://linuxtv.org/pinchartl/media.git uvc/next
> > 
> > I was a bit concerned about just using "int" for unaligned writing of a
> > 16-bit value, but looking at definitions, after a couple of macros
> > put_unaligned() currently resolves to one inline functions, which should
> > make that safe. However, at least theoretically, an arch could decide to
> > implement put_unaligned() as a macro, which might turn out to be unsafe
> > for this... Not sure how concerned should we be about such a possibility
> > :-) If you think, that's fine, then I'm ok with using the version from
> > that your branch.
> 
> Why do you think that would be unsafe ? The return type of 
> usb_get_current_frame_number() is int, so introducing an intermediate int 
> variable should at least not make things worse. If put_unaligned() is 
> implemented solely using macros I would still expect them to operate on the 
> type of the destination operand, and cast the source value appropriately.

Well, 

Re: [PATCH v8] uvcvideo: Add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Tuesday, 12 December 2017 10:30:39 EET Guennadi Liakhovetski wrote:
> On Mon, 11 Dec 2017, Laurent Pinchart wrote:
> > On Monday, 11 December 2017 23:44:09 EET Guennadi Liakhovetski wrote:
> >> On Mon, 11 Dec 2017, Laurent Pinchart wrote:
> >>> On Monday, 11 December 2017 22:16:23 EET Laurent Pinchart wrote:
>  On Wednesday, 6 December 2017 17:15:40 EET Guennadi Liakhovetski wrote:
> > From: 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. 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
> > 
> > ---
> > 
> > v8: addressed comments and integrated changes from Laurent, thanks
> > again, e.g.:
> > 
> > - multiple stylistic changes
> > - remove the UVC_DEV_FLAG_METADATA_NODE flag / quirk: nodes are now
> >   created unconditionally
> > - reuse uvc_ioctl_querycap()
> > - reuse code in uvc_register_video()
> > - set an error flag when the metadata buffer overflows
> > 
> >  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 | 179+++
> >  drivers/media/usb/uvc/uvc_queue.c|  44 +++--
> >  drivers/media/usb/uvc/uvc_video.c| 132 +++--
> >  drivers/media/usb/uvc/uvcvideo.h |  16 +++-
> >  include/uapi/linux/uvcvideo.h|  26 +
> >  8 files changed, 394 insertions(+), 22 deletions(-)
> >  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
>  
>  [snip]
>  
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index 13f459e..2fc0bf2 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
>  
>  [snip]
>  
> > +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> > + struct uvc_buffer *meta_buf,
> > + const u8 *mem, unsigned int length)
> > +{
> > +   struct uvc_meta_buf *meta;
> > +   size_t len_std = 2;
> > +   bool has_pts, has_scr;
> > +   unsigned long flags;
> > +   ktime_t time;
> > +   const u8 *scr;

[snip]

> > +   meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem +
> > meta_buf->bytesused);
> > +   local_irq_save(flags);
> > +   time = uvc_video_get_time();
> > +   meta->sof = usb_get_current_frame_number(stream->dev->udev);
>  
>  You need a put_unaligned here too. If you're fine with the patch
>  below there's no need to resubmit, and
> >>> 
> >>> One more thing, __put_unaligned_cpu16() and __put_unaligned_cpu64()
> >>> don't compile on x86_64 with v4.12 (using media_build.git). I propose
> >>> replacing them with put_unaligned() which compiles and should do the
> >>> right thing.
> >> 
> >> Sure, thanks for catching! Shall I fix and resubmit?
> > 
> > If you're fine with
> > 
> > git://linuxtv.org/pinchartl/media.git uvc/next
> 
> I was a bit concerned about just using "int" for unaligned writing of a
> 16-bit value, but looking at definitions, after a couple of macros
> put_unaligned() currently resolves to one inline functions, which should
> make that safe. However, at least theoretically, an arch could decide to
> implement put_unaligned() as a macro, which might turn out to be unsafe
> for this... Not sure how concerned should we be about such a possibility
> :-) If you think, that's fine, then I'm ok with using the version from
> that your branch.

Why do you think that would be unsafe ? The return type of 
usb_get_current_frame_number() is int, so introducing an intermediate int 
variable should at least not make things worse. If put_unaligned() is 
implemented solely using macros I would still expect them to operate on the 
type of the destination operand, and cast the source value appropriately.

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8] uvcvideo: Add a metadata device node

[Guennadi Liakhovetski]

Hi Laurent,

On Mon, 11 Dec 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Monday, 11 December 2017 23:44:09 EET Guennadi Liakhovetski wrote:
> > On Mon, 11 Dec 2017, Laurent Pinchart wrote:
> > > On Monday, 11 December 2017 22:16:23 EET Laurent Pinchart wrote:
> > >> On Wednesday, 6 December 2017 17:15:40 EET Guennadi Liakhovetski wrote:
> > >>> From: 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. 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 
> > >>> ---
> > >>> 
> > >>> v8: addressed comments and integrated changes from Laurent, thanks
> > >>> again, e.g.:
> > >>> 
> > >>> - multiple stylistic changes
> > >>> - remove the UVC_DEV_FLAG_METADATA_NODE flag / quirk: nodes are now
> > >>>   created unconditionally
> > >>> - reuse uvc_ioctl_querycap()
> > >>> - reuse code in uvc_register_video()
> > >>> - set an error flag when the metadata buffer overflows
> > >>> 
> > >>>  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 | 179 
> > >>>  drivers/media/usb/uvc/uvc_queue.c|  44 +++--
> > >>>  drivers/media/usb/uvc/uvc_video.c| 132 --
> > >>>  drivers/media/usb/uvc/uvcvideo.h |  16 +++-
> > >>>  include/uapi/linux/uvcvideo.h|  26 +
> > >>>  8 files changed, 394 insertions(+), 22 deletions(-)
> > >>>  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
> > >> 
> > >> [snip]
> > >> 
> > >> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > >> > b/drivers/media/usb/uvc/uvc_video.c index 13f459e..2fc0bf2 100644
> > >> > --- a/drivers/media/usb/uvc/uvc_video.c
> > >> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > >> 
> > >> [snip]
> > >> 
> > >>> +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> > >>> + struct uvc_buffer *meta_buf,
> > >>> + const u8 *mem, unsigned int length)
> > >>> +{
> > >>> +   struct uvc_meta_buf *meta;
> > >>> +   size_t len_std = 2;
> > >>> +   bool has_pts, has_scr;
> > >>> +   unsigned long flags;
> > >>> +   ktime_t time;
> > >>> +   const u8 *scr;
> > >>> +
> > >>> +   if (!meta_buf || length == 2)
> > >>> +   return;
> > >>> +
> > >>> +   if (meta_buf->length - meta_buf->bytesused <
> > >>> +   length + sizeof(meta->ns) + sizeof(meta->sof)) {
> > >>> +   meta_buf->error = 1;
> > >>> +   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 (stream->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);
> > >>> +   time = uvc_video_get_time();
> > >>> +   meta->sof = usb_get_current_frame_number(stream->dev->udev);
> > >> 
> > >> You need a put_unaligned here too. If you're fine with the patch below
> > >> there's no need to resubmit, and
> > > 
> > > One more thing, __put_unaligned_cpu16() and __put_unaligned_cpu64() don't
> > > compile on x86_64 with v4.12 (using media_build.git). I propose replacing
> > > them with put_unaligned() which compiles and should do the right thing.
> > Sure, thanks for catching! Shall I fix and resubmit?
> 
> If you're fine with
> 
>   git://linuxtv.org/pinchartl/media.git uvc/next

I was a bit concerned about just using "int" for unaligned writing of a 
16-bit value, but looking at definitions, after a couple of macros 
put_unaligned() currently resolves to one inline functions, which should 

Re: [PATCH v8] uvcvideo: Add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Monday, 11 December 2017 23:44:09 EET Guennadi Liakhovetski wrote:
> On Mon, 11 Dec 2017, Laurent Pinchart wrote:
> > On Monday, 11 December 2017 22:16:23 EET Laurent Pinchart wrote:
> >> On Wednesday, 6 December 2017 17:15:40 EET Guennadi Liakhovetski wrote:
> >>> From: 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. 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 
> >>> ---
> >>> 
> >>> v8: addressed comments and integrated changes from Laurent, thanks
> >>> again, e.g.:
> >>> 
> >>> - multiple stylistic changes
> >>> - remove the UVC_DEV_FLAG_METADATA_NODE flag / quirk: nodes are now
> >>>   created unconditionally
> >>> - reuse uvc_ioctl_querycap()
> >>> - reuse code in uvc_register_video()
> >>> - set an error flag when the metadata buffer overflows
> >>> 
> >>>  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 | 179 
> >>>  drivers/media/usb/uvc/uvc_queue.c|  44 +++--
> >>>  drivers/media/usb/uvc/uvc_video.c| 132 --
> >>>  drivers/media/usb/uvc/uvcvideo.h |  16 +++-
> >>>  include/uapi/linux/uvcvideo.h|  26 +
> >>>  8 files changed, 394 insertions(+), 22 deletions(-)
> >>>  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
> >> 
> >> [snip]
> >> 
> >> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> >> > b/drivers/media/usb/uvc/uvc_video.c index 13f459e..2fc0bf2 100644
> >> > --- a/drivers/media/usb/uvc/uvc_video.c
> >> > +++ b/drivers/media/usb/uvc/uvc_video.c
> >> 
> >> [snip]
> >> 
> >>> +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> >>> +   struct uvc_buffer *meta_buf,
> >>> +   const u8 *mem, unsigned int length)
> >>> +{
> >>> + struct uvc_meta_buf *meta;
> >>> + size_t len_std = 2;
> >>> + bool has_pts, has_scr;
> >>> + unsigned long flags;
> >>> + ktime_t time;
> >>> + const u8 *scr;
> >>> +
> >>> + if (!meta_buf || length == 2)
> >>> + return;
> >>> +
> >>> + if (meta_buf->length - meta_buf->bytesused <
> >>> + length + sizeof(meta->ns) + sizeof(meta->sof)) {
> >>> + meta_buf->error = 1;
> >>> + 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 (stream->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);
> >>> + time = uvc_video_get_time();
> >>> + meta->sof = usb_get_current_frame_number(stream->dev->udev);
> >> 
> >> You need a put_unaligned here too. If you're fine with the patch below
> >> there's no need to resubmit, and
> > 
> > One more thing, __put_unaligned_cpu16() and __put_unaligned_cpu64() don't
> > compile on x86_64 with v4.12 (using media_build.git). I propose replacing
> > them with put_unaligned() which compiles and should do the right thing.
> Sure, thanks for catching! Shall I fix and resubmit?

If you're fine with

git://linuxtv.org/pinchartl/media.git uvc/next

there's no need to resubmit.

By the way, could you please review "uvcvideo: Factor out video device 
registration to a function" and "uvcvideo: Report V4L2 device caps through the 
video_device structure" ?

I will send a pull request after testing the code and getting those two 
patches reviewed.

> >> Reviewed-by: Laurent Pinchart 
> >> 
> >> Would you mind sending me your test tool patch ?
> 
> Will send it offline.
> 
> Thanks
> Guennadi
> 
> >> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/
> >> uvc_video.c
> >> index 2fc0bf2221db..02e4997a32bb 100644
> >> --- a/drivers/media/usb/uvc/uvc_video.c
> >> +++ 

Re: [PATCH v8] uvcvideo: Add a metadata device node

[Guennadi Liakhovetski]

Hi Laurent,

On Mon, 11 Dec 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Monday, 11 December 2017 22:16:23 EET Laurent Pinchart wrote:
> > On Wednesday, 6 December 2017 17:15:40 EET Guennadi Liakhovetski wrote:
> > > From: 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. 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 
> > > ---
> > > 
> > > v8: addressed comments and integrated changes from Laurent, thanks again,
> > > e.g.:
> > > 
> > > - multiple stylistic changes
> > > - remove the UVC_DEV_FLAG_METADATA_NODE flag / quirk: nodes are now
> > >   created unconditionally
> > > - reuse uvc_ioctl_querycap()
> > > - reuse code in uvc_register_video()
> > > - set an error flag when the metadata buffer overflows
> > > 
> > >  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 | 179 ++
> > >  drivers/media/usb/uvc/uvc_queue.c|  44 +++--
> > >  drivers/media/usb/uvc/uvc_video.c| 132 --
> > >  drivers/media/usb/uvc/uvcvideo.h |  16 +++-
> > >  include/uapi/linux/uvcvideo.h|  26 +
> > >  8 files changed, 394 insertions(+), 22 deletions(-)
> > >  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
> > 
> > [snip]
> > 
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > > b/drivers/media/usb/uvc/uvc_video.c index 13f459e..2fc0bf2 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > 
> > [snip]
> > 
> > > +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> > > +   struct uvc_buffer *meta_buf,
> > > +   const u8 *mem, unsigned int length)
> > > +{
> > > + struct uvc_meta_buf *meta;
> > > + size_t len_std = 2;
> > > + bool has_pts, has_scr;
> > > + unsigned long flags;
> > > + ktime_t time;
> > > + const u8 *scr;
> > > +
> > > + if (!meta_buf || length == 2)
> > > + return;
> > > +
> > > + if (meta_buf->length - meta_buf->bytesused <
> > > + length + sizeof(meta->ns) + sizeof(meta->sof)) {
> > > + meta_buf->error = 1;
> > > + 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 (stream->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);
> > > + time = uvc_video_get_time();
> > > + meta->sof = usb_get_current_frame_number(stream->dev->udev);
> > 
> > You need a put_unaligned here too. If you're fine with the patch below
> > there's no need to resubmit, and
> 
> One more thing, __put_unaligned_cpu16() and __put_unaligned_cpu64() don't 
> compile on x86_64 with v4.12 (using media_build.git). I propose replacing 
> them 
> with put_unaligned() which compiles and should do the right thing.

Sure, thanks for catching! Shall I fix and resubmit?

> > Reviewed-by: Laurent Pinchart 
> > 
> > Would you mind sending me your test tool patch ?

Will send it offline.

Thanks
Guennadi

> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/
> > uvc_video.c
> > index 2fc0bf2221db..02e4997a32bb 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1142,6 +1142,7 @@ static void uvc_video_decode_meta(struct uvc_streaming
> > *stream,
> > size_t len_std = 2;
> > bool has_pts, has_scr;
> > unsigned long flags;
> > +   unsigned int sof;
> > ktime_t time;
> > const u8 *scr;
> > 
> > @@ -1177,9 +1178,10 @@ static void uvc_video_decode_meta(struct
> > uvc_streaming *stream,
> > meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem + 
> > meta_buf->bytesused);
> > 

Re: [PATCH v8] uvcvideo: Add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

On Monday, 11 December 2017 22:16:23 EET Laurent Pinchart wrote:
> On Wednesday, 6 December 2017 17:15:40 EET Guennadi Liakhovetski wrote:
> > From: 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. 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 
> > ---
> > 
> > v8: addressed comments and integrated changes from Laurent, thanks again,
> > e.g.:
> > 
> > - multiple stylistic changes
> > - remove the UVC_DEV_FLAG_METADATA_NODE flag / quirk: nodes are now
> >   created unconditionally
> > - reuse uvc_ioctl_querycap()
> > - reuse code in uvc_register_video()
> > - set an error flag when the metadata buffer overflows
> > 
> >  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 | 179 ++
> >  drivers/media/usb/uvc/uvc_queue.c|  44 +++--
> >  drivers/media/usb/uvc/uvc_video.c| 132 --
> >  drivers/media/usb/uvc/uvcvideo.h |  16 +++-
> >  include/uapi/linux/uvcvideo.h|  26 +
> >  8 files changed, 394 insertions(+), 22 deletions(-)
> >  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
> 
> [snip]
> 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index 13f459e..2fc0bf2 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> 
> [snip]
> 
> > +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> > + struct uvc_buffer *meta_buf,
> > + const u8 *mem, unsigned int length)
> > +{
> > +   struct uvc_meta_buf *meta;
> > +   size_t len_std = 2;
> > +   bool has_pts, has_scr;
> > +   unsigned long flags;
> > +   ktime_t time;
> > +   const u8 *scr;
> > +
> > +   if (!meta_buf || length == 2)
> > +   return;
> > +
> > +   if (meta_buf->length - meta_buf->bytesused <
> > +   length + sizeof(meta->ns) + sizeof(meta->sof)) {
> > +   meta_buf->error = 1;
> > +   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 (stream->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);
> > +   time = uvc_video_get_time();
> > +   meta->sof = usb_get_current_frame_number(stream->dev->udev);
> 
> You need a put_unaligned here too. If you're fine with the patch below
> there's no need to resubmit, and

One more thing, __put_unaligned_cpu16() and __put_unaligned_cpu64() don't 
compile on x86_64 with v4.12 (using media_build.git). I propose replacing them 
with put_unaligned() which compiles and should do the right thing.

> Reviewed-by: Laurent Pinchart 
> 
> Would you mind sending me your test tool patch ?
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/
> uvc_video.c
> index 2fc0bf2221db..02e4997a32bb 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1142,6 +1142,7 @@ static void uvc_video_decode_meta(struct uvc_streaming
> *stream,
>   size_t len_std = 2;
>   bool has_pts, has_scr;
>   unsigned long flags;
> + unsigned int sof;
>   ktime_t time;
>   const u8 *scr;
> 
> @@ -1177,9 +1178,10 @@ static void uvc_video_decode_meta(struct
> uvc_streaming *stream,
>   meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem + 
> meta_buf->bytesused);
> local_irq_save(flags);
>   time = uvc_video_get_time();
> - meta->sof = usb_get_current_frame_number(stream->dev->udev);
> + sof = usb_get_current_frame_number(stream->dev->udev);
>   local_irq_restore(flags);
>   __put_unaligned_cpu64(ktime_to_ns(time), >ns);
> + __put_unaligned_cpu16(sof, >sof);
> 

Re: [PATCH v8] uvcvideo: Add a metadata device node

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Wednesday, 6 December 2017 17:15:40 EET Guennadi Liakhovetski wrote:
> From: 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. 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 
> ---
> 
> v8: addressed comments and integrated changes from Laurent, thanks again,
> e.g.:
> 
> - multiple stylistic changes
> - remove the UVC_DEV_FLAG_METADATA_NODE flag / quirk: nodes are now
>   created unconditionally
> - reuse uvc_ioctl_querycap()
> - reuse code in uvc_register_video()
> - set an error flag when the metadata buffer overflows
> 
>  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 | 179 
>  drivers/media/usb/uvc/uvc_queue.c|  44 +++--
>  drivers/media/usb/uvc/uvc_video.c| 132 --
>  drivers/media/usb/uvc/uvcvideo.h |  16 +++-
>  include/uapi/linux/uvcvideo.h|  26 +
>  8 files changed, 394 insertions(+), 22 deletions(-)
>  create mode 100644 drivers/media/usb/uvc/uvc_metadata.c

[snip]

> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 13f459e..2fc0bf2 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c

[snip]

> +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> +   struct uvc_buffer *meta_buf,
> +   const u8 *mem, unsigned int length)
> +{
> + struct uvc_meta_buf *meta;
> + size_t len_std = 2;
> + bool has_pts, has_scr;
> + unsigned long flags;
> + ktime_t time;
> + const u8 *scr;
> +
> + if (!meta_buf || length == 2)
> + return;
> +
> + if (meta_buf->length - meta_buf->bytesused <
> + length + sizeof(meta->ns) + sizeof(meta->sof)) {
> + meta_buf->error = 1;
> + 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 (stream->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);
> + time = uvc_video_get_time();
> + meta->sof = usb_get_current_frame_number(stream->dev->udev);

You need a put_unaligned here too. If you're fine with the patch below there's 
no need to resubmit, and

Reviewed-by: Laurent Pinchart 

Would you mind sending me your test tool patch ?

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/
uvc_video.c
index 2fc0bf2221db..02e4997a32bb 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1142,6 +1142,7 @@ static void uvc_video_decode_meta(struct uvc_streaming 
*stream,
size_t len_std = 2;
bool has_pts, has_scr;
unsigned long flags;
+   unsigned int sof;
ktime_t time;
const u8 *scr;
 
@@ -1177,9 +1178,10 @@ static void uvc_video_decode_meta(struct uvc_streaming 
*stream,
meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem + 
meta_buf->bytesused);
local_irq_save(flags);
time = uvc_video_get_time();
-   meta->sof = usb_get_current_frame_number(stream->dev->udev);
+   sof = usb_get_current_frame_number(stream->dev->udev);
local_irq_restore(flags);
__put_unaligned_cpu64(ktime_to_ns(time), >ns);
+   __put_unaligned_cpu16(sof, >sof);
 
if (has_scr)
memcpy(stream->clock.last_scr, scr, 6);

> + local_irq_restore(flags);
> + __put_unaligned_cpu64(ktime_to_ns(time), >ns);
> +
> + if (has_scr)
> + memcpy(stream->clock.last_scr, scr, 6);
> +
> + memcpy(>length, mem, length);
> + meta_buf->bytesused += length + sizeof(meta->ns) + sizeof(meta->sof);
> +
> +