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.
[snip]
--
Regards,
Laurent Pinchart
