From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:34284 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752159AbcDUTO6 (ORCPT ); Thu, 21 Apr 2016 15:14:58 -0400 From: Laurent Pinchart To: Hans Verkuil Cc: Laurent Pinchart , linux-media@vger.kernel.org, Sakari Ailus , Hans Verkuil , Mauro Carvalho Chehab Subject: Re: [PATCH/RFC 1/2] v4l: Add meta-data video device type Date: Thu, 21 Apr 2016 22:15:12 +0300 Message-ID: <27064764.ckB5ZcOUBB@avalon> In-Reply-To: <571875BF.8080500@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> 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 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 ? > > +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"). > > + > > + > > +
[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. > > */ > > > > int __video_register_device(struct video_device *vdev, int type, int nr, > > > > int warn_if_nr_in_use, struct module *owner) [snip] -- Regards, Laurent Pinchart