From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from lists.s-osg.org ([54.187.51.154]:43226 "EHLO lists.s-osg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754221AbcDVN6k (ORCPT ); Fri, 22 Apr 2016 09:58:40 -0400 Date: Fri, 22 Apr 2016 10:58:34 -0300 From: Mauro Carvalho Chehab To: Hans Verkuil Cc: Laurent Pinchart , Laurent Pinchart , , Sakari Ailus , Hans Verkuil Subject: Re: [PATCH/RFC 1/2] v4l: Add meta-data video device type Message-ID: <20160422105834.6c86c86c@recife.lan> In-Reply-To: <5719D6BC.1010000@xs4all.nl> References: <1461199227-22506-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1461199227-22506-2-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <571875BF.8080500@xs4all.nl> <27064764.ckB5ZcOUBB@avalon> <5719D6BC.1010000@xs4all.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Em Fri, 22 Apr 2016 09:46:04 +0200 Hans Verkuil escreveu: > On 04/21/2016 09:15 PM, Laurent Pinchart wrote: > > Hi Hans, > > > > Thank you for the review. > > > > On Thursday 21 Apr 2016 08:39:59 Hans Verkuil wrote: > >> Hi Laurent, > >> > >> Thanks for the patch! > >> > >> Some, mostly small, comments follow: > >> > >> On 04/21/2016 02:40 AM, Laurent Pinchart wrote: > >>> The meta-data video device is used to transfer meta-data between > >>> userspace and kernelspace through a V4L2 buffers queue. It comes with a > >>> new meta-data capture capability, buffer type and format description. > >>> > >>> Signed-off-by: Laurent Pinchart > >>> > >>> --- > >>> > >>> Documentation/DocBook/media/v4l/dev-meta.xml | 100 +++++++++++++++++++++ > >>> Documentation/DocBook/media/v4l/v4l2.xml | 1 + > >>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 19 +++++ > >>> drivers/media/v4l2-core/v4l2-dev.c | 21 +++++- > >>> drivers/media/v4l2-core/v4l2-ioctl.c | 39 ++++++++++ > >>> drivers/media/v4l2-core/videobuf2-v4l2.c | 3 + > >>> include/media/v4l2-dev.h | 3 +- > >>> include/media/v4l2-ioctl.h | 8 +++ > >>> include/uapi/linux/media.h | 2 + > >>> include/uapi/linux/videodev2.h | 14 ++++ > >>> 10 files changed, 208 insertions(+), 2 deletions(-) > >>> create mode 100644 Documentation/DocBook/media/v4l/dev-meta.xml > >>> > >>> diff --git a/Documentation/DocBook/media/v4l/dev-meta.xml > >>> b/Documentation/DocBook/media/v4l/dev-meta.xml new file mode 100644 > >>> index 000000000000..ddc685186015 > >>> --- /dev/null > >>> +++ b/Documentation/DocBook/media/v4l/dev-meta.xml > >>> @@ -0,0 +1,100 @@ > >>> + Meta-Data Interface > >>> + > >>> + > >>> + Experimental > >>> + This is an experimental > >>> + interface and may change in the future. > >>> + > >>> + > >>> + > >>> +Meta-data refers to any non-image data that supplements video frames with > >>> +additional information. This may include statistics computed over the > >>> image +or frame capture parameters supplied by the image source. This > >>> interface is +intended for transfer of meta-data to userspace and control > >>> of that operation. + > >>> + > >>> + > >>> +Meta-data devices are accessed through character device special files > >>> named +/dev/v4l-meta0 to > >>> /dev/v4l-meta255 +with major number 81 and > >>> dynamically allocated minor numbers 0 to 255. + > >>> + > >>> +
> >>> + Querying Capabilities > >>> + > >>> + > >>> +Devices supporting the meta-data interface set the > >>> +V4L2_CAP_META_CAPTURE flag in the > >>> +capabilities field of &v4l2-capability; > >>> +returned by the &VIDIOC-QUERYCAP; ioctl. That flag means the device can > >>> capture +meta-data to memory. > >>> + > >>> + > >>> +At least one of the read/write, streaming or asynchronous I/O methods > >>> must > >> > >> I think we can drop 'asynchronous I/O' since that's never been used. I > >> assume this is copy-and-pasted and we should probably just remove any > >> reference to async IO. > > > > Agreed. I'll fix it. > > > >>> +be supported. > >>> + > >>> +
> >>> + > >>> +
> >>> + Data Format Negotiation > >>> + > >>> + > >>> +The meta-data device uses the format ioctls > >>> to +select the capture format. The meta-data buffer content format is > >>> bound to that +selectable format. In addition to the basic > >>> +format ioctls, the &VIDIOC-ENUM-FMT; ioctl > >>> +must be supported as well. > >>> + > >>> + > >>> + > >>> +To use the format ioctls applications set > >>> the +type field of a &v4l2-format; to > >>> +V4L2_BUF_TYPE_META_CAPTURE and use the > >>> &v4l2-meta-format; +meta member of the > >>> fmt +union as needed per the desired > >>> operation. > >>> +Currently there are two fields, pixelformat > >>> and > >> > >> Shouldn't that be metaformat? Since there are no pixels here? It was a bit > >> dubious to call it pixelformat for SDR as well, but at least there you > >> still have discrete samples which might be called pixels with some > >> imagination. But certainly doesn't apply to meta data. > > > > How about dataformat ? Or just format ? > > Since the fourcc defines are called V4L2_META_FMT_ I think metaformat is a > good name. It's consistent with the other meta-related namings. metaformat is OK. > > > > >>> +buffersize, of struct &v4l2-meta-format; that > >>> are +used. Content of the pixelformat is the > >>> V4L2 FourCC +code of the data format. The > >>> buffersize field is the +maximum buffer size > >>> in bytes required for data transfer, set by the driver in +order to > >>> inform applications. > >>> + > >>> + > >>> + > >>> + struct <structname>v4l2_meta_format</structname> > >>> + > >>> + &cs-str; > >>> + > >>> + > >>> + __u32 > >>> + pixelformat > >>> + > >>> +The data format or type of compression, set by the application. This is a > >>> +little endian four character code. > >>> +V4L2 defines meta-data formats in . > >>> + > >>> + > >>> + > >>> + __u32 > >>> + buffersize > >>> + > >>> +Maximum size in bytes required for data. Value is set by the driver. > >>> + > >>> + > >>> + > >>> + __u8 > >>> + reserved[24] > >>> + This array is reserved for future extensions. > >>> +Drivers and applications must set it to zero. > >>> + > >>> + > >>> + > >>> +
> >>> + > >>> + > >>> +A meta-data device may support read/write > >>> +and/or streaming (memory mapping > >>> +or user pointer) I/O. > >> > >> Add dma-buf to this as well, or just say "streaming I/O" without listing > >> the possibilities. If this is copied-and-pasted, then the same should be > >> done in the original. > > > > How about removing the paragraph completely ? This is already addressed in the > > previous section ("Querying Capabilities"). > > Let's remove this. I am considering to make a proposal to tighten this up. > I've never been happy with the way stream I/O capabilities are signaled (or > really the lack of any signaling). Agreed. > > > >>> + > >>> + > >>> +
> > > > [snip] > > > >>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c > >>> b/drivers/media/v4l2-core/v4l2-dev.c index 70b559d7ca80..d8cbf11eae4e > >>> 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-dev.c > >>> +++ b/drivers/media/v4l2-core/v4l2-dev.c > > > > [snip] > > > >>> @@ -845,6 +859,8 @@ static int video_register_media_controller(struct > >>> video_device *vdev, int type)> > >>> * %VFL_TYPE_SUBDEV - A subdevice > >>> * > >>> * %VFL_TYPE_SDR - Software Defined Radio > >>> > >>> + * > >>> + * %VFL_TYPE_META - Meta-data (including statistics) > >> > >> I would drop the '(including statistics)' part. It feels weird that > >> 'statistics' are singled out, it makes the reader wonder what is so special > >> about it that it needs to be mentioned explicitly. > > > > Done. It actually makes sense to put statistics as an example of such metadata, as this is the main(and currently only) usage for this devnode. > > > >>> */ > >>> > >>> int __video_register_device(struct video_device *vdev, int type, int nr, > >>> > >>> int warn_if_nr_in_use, struct module *owner) > > > > [snip] > > > > Regards, > > Hans -- Thanks, Mauro