From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from lb3-smtp-cloud3.xs4all.net ([194.109.24.30]:58354 "EHLO lb3-smtp-cloud3.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751972AbcDVHqL (ORCPT ); Fri, 22 Apr 2016 03:46:11 -0400 Subject: Re: [PATCH/RFC 1/2] v4l: Add meta-data video device type To: Laurent Pinchart 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> Cc: Laurent Pinchart , linux-media@vger.kernel.org, Sakari Ailus , Hans Verkuil , Mauro Carvalho Chehab From: Hans Verkuil Message-ID: <5719D6BC.1010000@xs4all.nl> Date: Fri, 22 Apr 2016 09:46:04 +0200 MIME-Version: 1.0 In-Reply-To: <27064764.ckB5ZcOUBB@avalon> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: 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. > >>> +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). > >>> + >>> + >>> +
> > [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, Hans