From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from lb3-smtp-cloud3.xs4all.net ([194.109.24.30]:54591 "EHLO lb3-smtp-cloud3.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752031AbcDVOBn (ORCPT ); Fri, 22 Apr 2016 10:01:43 -0400 Subject: Re: [PATCH/RFC 1/2] v4l: Add meta-data video device type To: Mauro Carvalho Chehab 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> <20160422105834.6c86c86c@recife.lan> Cc: Laurent Pinchart , Laurent Pinchart , linux-media@vger.kernel.org, Sakari Ailus , Hans Verkuil From: Hans Verkuil Message-ID: <571A2EBF.3010209@xs4all.nl> Date: Fri, 22 Apr 2016 16:01:35 +0200 MIME-Version: 1.0 In-Reply-To: <20160422105834.6c86c86c@recife.lan> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: On 04/22/2016 03:58 PM, Mauro Carvalho Chehab wrote: > 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. Then I would say 'like statistics' here. But I still don't like this to be honest. Heck, Nick Dyer posted a patch series for getting diagnostics yesterday, which would be a good fit as well. Anyway, it's not worth a long discussion :-) Regards, Hans