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 <guennadi.liakhovetski@xxxxxxxxx>
> > >>>>>
> > >>>>> 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
> > >>>>> <guennadi.liakhovetski@xxxxxxxxx>
> > >>>>> ---
> > >>>>>
> > >>>>> 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, you *can* implement that badly - check the destination type, which
in this case is 16 bit, and use the address of the first argument to get
16 bits from there... But yes, I agree, that would be a bug, so, maybe
it's ok to rely on those implementations being bug-free.
Thanks
Guennadi
