From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:34301 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751041AbcDUTYd (ORCPT ); Thu, 21 Apr 2016 15:24:33 -0400 From: Laurent Pinchart To: Sakari Ailus Cc: Laurent Pinchart , linux-media@vger.kernel.org, Hans Verkuil , Mauro Carvalho Chehab Subject: Re: [PATCH/RFC 1/2] v4l: Add meta-data video device type Date: Thu, 21 Apr 2016 22:24:48 +0300 Message-ID: <2327439.DCv8pRv1AH@avalon> In-Reply-To: <20160421084426.GA32125@valkosipuli.retiisi.org.uk> References: <1461199227-22506-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1461199227-22506-2-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <20160421084426.GA32125@valkosipuli.retiisi.org.uk> 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 Sakari, Thank you for the review. On Thursday 21 Apr 2016 11:44:26 Sakari Ailus wrote: > On Thu, Apr 21, 2016 at 03:40:26AM +0300, 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 > > I propose: > > s/-/ / How about metadata ? That's the spelling used by wikipedia > > + > > + > > + 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. > > Ditto. > > > + > > + > > + > > +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. > > Where does 255 come from? At least in kernel the minor number space has got > 20 bits, not 8. Not that I prefer having that many meta data nodes, but > still that's a possibility. We have #define VIDEO_NUM_DEVICES 256 in drivers/media/v4l2-core/v4l2-dev.c. If you want to take care of the code I'll update the documentation :-) > > + > > + > > +
> > + 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 > > +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 > > +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. > > 200 bytes is reserved for this struct in all IOCTLs. How about using closer > to that amount? 24 bytes of reserved space isn't much. Albeit you could > argue that this struct could be changed later on as it does not affect IOCTL > argument size. Should we just get rid of the reserved field then ? > > + > > + > > + > > +
> > + > > + > > +A meta-data device may support read/write > > +and/or streaming (memory mapping > > +or user pointer) I/O. > > + > > + > > +
[snip] > > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index > > bacecbd68a6d..da2d836e8887 100644 > > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > @@ -161,6 +161,20 @@ static inline int put_v4l2_sdr_format(struct > > v4l2_sdr_format *kp, struct v4l2_sd> > > return 0; > > > > } > > > > +static inline int get_v4l2_meta_format(struct v4l2_meta_format *kp, > > struct v4l2_meta_format __user *up) > > I think I'd wrap this. Same below. I've followed the existing coding style of the file, I'm fine with wrapping the lines if you think that's better. > > +{ > > + if (copy_from_user(kp, up, sizeof(struct v4l2_meta_format))) > > + return -EFAULT; > > + return 0; > > +} > > + > > +static inline int put_v4l2_meta_format(struct v4l2_meta_format *kp, > > struct v4l2_meta_format __user *up) +{ > > + if (copy_to_user(up, kp, sizeof(struct v4l2_meta_format))) > > + return -EFAULT; > > + return 0; > > +} > > + > > struct v4l2_format32 { > > __u32 type; /* enum v4l2_buf_type */ > > union { [snip] > > diff --git a/include/uapi/linux/videodev2.h > > b/include/uapi/linux/videodev2.h index e895975c5b0e..5035295a0138 100644 > > --- a/include/uapi/linux/videodev2.h > > +++ b/include/uapi/linux/videodev2.h [snip] > > @@ -2007,6 +2009,17 @@ struct v4l2_sdr_format { > > } __attribute__ ((packed)); > > > > /** > > + * struct v4l2_meta_format - meta-data format definition > > An empty line here would be nice. The kerneldoc style doesn't add an empty line after the first, and the kerneldoc blocks in this file don't either. > > + * @pixelformat: little endian four character code (fourcc) > > + * @buffersize: maximum size in bytes required for data > > + */ > > +struct v4l2_meta_format { > > + __u32 pixelformat; > > + __u32 buffersize; > > + __u8 reserved[24]; > > +} __attribute__ ((packed)); > > + > > +/** > > * struct v4l2_format - stream data format > > * @type: enum v4l2_buf_type; type of the data stream > > * @pix: definition of an image format [snip] -- Regards, Laurent Pinchart