From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:43109 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751754AbdLEBD3 (ORCPT ); Mon, 4 Dec 2017 20:03:29 -0500 From: Laurent Pinchart To: Laurent Pinchart Cc: Guennadi Liakhovetski , linux-media@vger.kernel.org, Hans Verkuil Subject: Re: [PATCH 3/3 v7] uvcvideo: add a metadata device node Date: Tue, 05 Dec 2017 03:03:42 +0200 Message-ID: <1780731.RZdITBElWU@avalon> In-Reply-To: <2006709.83fRcNtr9s@avalon> References: <1510156814-28645-1-git-send-email-g.liakhovetski@gmx.de> <1510156814-28645-4-git-send-email-g.liakhovetski@gmx.de> <2006709.83fRcNtr9s@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: 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(&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(&ts); > > 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(&meta->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