From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtp.nokia.com ([147.243.128.24]:46873 "EHLO mgw-da01.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751403Ab1HHIWR (ORCPT ); Mon, 8 Aug 2011 04:22:17 -0400 Message-ID: <4E3D8B03.3090601@iki.fi> Date: Sat, 06 Aug 2011 21:42:11 +0300 From: Sakari Ailus MIME-Version: 1.0 To: Guennadi Liakhovetski CC: Linux Media Mailing List , Hans Verkuil , Pawel Osciak , Sakari Ailus , Laurent Pinchart , Mauro Carvalho Chehab Subject: Re: [PATCH 1/6 v4] V4L: add two new ioctl()s for multi-size videobuffer management References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Guennadi Liakhovetski wrote: > A possibility to preallocate and initialise buffers of different sizes > in V4L2 is required for an efficient implementation of asnapshot mode. > This patch adds two new ioctl()s: VIDIOC_CREATE_BUFS and > VIDIOC_PREPARE_BUF and defines respective data structures. > > Signed-off-by: Guennadi Liakhovetski > --- Hi Guennadi, Many thanks for the patchset! I have a few comments below, after reading through the patch once again. > > v4: > > 1. CREATE_BUFS now takes an array of plane sizes and a fourcc code in its > argument, instead of a frame format specification, including > documentation update > 2. documentation improvements, as suggested by Hans > 3. increased reserved fields to 18, as suggested by Sakari > > Documentation/DocBook/media/v4l/io.xml | 17 ++ > Documentation/DocBook/media/v4l/v4l2.xml | 2 + > .../DocBook/media/v4l/vidioc-create-bufs.xml | 161 ++++++++++++++++++++ > .../DocBook/media/v4l/vidioc-prepare-buf.xml | 96 ++++++++++++ > drivers/media/video/v4l2-compat-ioctl32.c | 6 + > drivers/media/video/v4l2-ioctl.c | 26 +++ > include/linux/videodev2.h | 18 +++ > include/media/v4l2-ioctl.h | 2 + > 8 files changed, 328 insertions(+), 0 deletions(-) > create mode 100644 Documentation/DocBook/media/v4l/vidioc-create-bufs.xml > create mode 100644 Documentation/DocBook/media/v4l/vidioc-prepare-buf.xml > > diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml > index 227e7ac..ff03dd2 100644 > --- a/Documentation/DocBook/media/v4l/io.xml > +++ b/Documentation/DocBook/media/v4l/io.xml > @@ -927,6 +927,23 @@ ioctl is called. > Applications set or clear this flag before calling the > VIDIOC_QBUF ioctl. > > + > + V4L2_BUF_FLAG_NO_CACHE_INVALIDATE > + 0x0400 > + Caches do not have to be invalidated for this buffer. > +Typically applications shall use this flag if the data captured in the buffer > +is not going to be touched by the CPU, instead the buffer will, probably, be > +passed on to a DMA-capable hardware unit for further processing or output. > + > + > + > + V4L2_BUF_FLAG_NO_CACHE_CLEAN > + 0x0800 > + Caches do not have to be cleaned for this buffer. > +Typically applications shall use this flag for output buffers if the data > +in this buffer has not been created by the CPU but by some DMA-capable unit, > +in which case caches have not been used. > + > > > > diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml > index 0d05e87..06bb179 100644 > --- a/Documentation/DocBook/media/v4l/v4l2.xml > +++ b/Documentation/DocBook/media/v4l/v4l2.xml > @@ -462,6 +462,7 @@ and discussions on the V4L mailing list. > &sub-close; > &sub-ioctl; > > + &sub-create-bufs; > &sub-cropcap; > &sub-dbg-g-chip-ident; > &sub-dbg-g-register; > @@ -504,6 +505,7 @@ and discussions on the V4L mailing list. > &sub-queryctrl; > &sub-query-dv-preset; > &sub-querystd; > + &sub-prepare-buf; > &sub-reqbufs; > &sub-s-hw-freq-seek; > &sub-streamon; > diff --git a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml > new file mode 100644 > index 0000000..b37b9a4 > --- /dev/null > +++ b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml > @@ -0,0 +1,161 @@ > + > + > + ioctl VIDIOC_CREATE_BUFS > + &manvol; > + > + > + > + VIDIOC_CREATE_BUFS > + Create buffers for Memory Mapped or User Pointer I/O > + > + > + > + > + > + int ioctl > + int fd > + int request > + struct v4l2_create_buffers *argp > + > + > + > + > + > + Arguments > + > + > + > + fd > + > + &fd; > + > + > + > + request > + > + VIDIOC_CREATE_BUFS > + > + > + > + argp > + > + > + > + > + > + > + > + > + Description > + > + This ioctl is used to create buffers for memory > +mapped or user pointer > +I/O. It can be used as an alternative or in addition to the > +VIDIOC_REQBUFS ioctl, when a tighter control over buffers > +is required. This ioctl can be called multiple times to create buffers of > +different sizes. > + > + To allocate device buffers applications initialize relevant > +fields of the v4l2_create_buffers structure. > +They set the type field to the respective stream > +or buffer type. count must be set to the number of > +required buffers. memory specifies the required I/O > +method. If num_planes == 0 or all elements of the > +sizes array are 0, then buffers, suitable for the > +currently configured video format, are allocated, exactly like for the > +VIDIOC_REQBUFS ioctl. If > +num_planes > 0 and at least some of the respective > +sizes elements are non-zero, this information will be > +used for buffer-allocation. The reserved array must > +be zeroed. When the ioctl is called with a pointer to this structure the driver > +will attempt to allocate up to the requested number of buffers and store the > +actual number allocated and the starting index in the > +count and the index > +fields respectively. On return count can be smaller > +than the number requested. > + When the I/O method is not supported the ioctl > +returns an &EINVAL;. > + > + > + struct <structname>v4l2_create_buffers</structname> > + > + &cs-str; > + > + > + __u32 > + index > + The starting buffer index, returned by the driver. > + > + > + __u32 > + count > + The number of buffers requested or granted. > + > + > + __u32 > + type > + V4L2 buffer type: one of V4L2_BUF_TYPE_* > +values. &v4l2-buf-type; here? > + > + > + __u32 > + memory > + Applications set this field to > +V4L2_MEMORY_MMAP or > +V4L2_MEMORY_USERPTR. &v4l2-memory; > + > + > + __u32 > + fourcc > + One of V4L2_PIX_FMT_* FOURCCs of the video data. > + > + > + __u32 > + num_planes > + Number of planes or 0 to use the current format. > + > + > + __u32 > + size[VIDEO_MAX_PLANES] > + Explicit sizes of buffers, being created. > + > + > + __u32 > + reserved[18] > + A place holder for future extensions. > + > + > + > +
> +
> + > + > + &return-value; > + > + > + > + ENOMEM > + > + No memory to allocate buffers for memory > +mapped I/O. > + > + > + > + EINVAL > + > + The buffer type (type field) or the > +requested I/O method (memory) is not > +supported. > + > + > + > + > +
> + > + > diff --git a/Documentation/DocBook/media/v4l/vidioc-prepare-buf.xml b/Documentation/DocBook/media/v4l/vidioc-prepare-buf.xml > new file mode 100644 > index 0000000..509e752 > --- /dev/null > +++ b/Documentation/DocBook/media/v4l/vidioc-prepare-buf.xml > @@ -0,0 +1,96 @@ > + > + > + ioctl VIDIOC_PREPARE_BUF > + &manvol; > + > + > + > + VIDIOC_PREPARE_BUF > + Prepare a buffer for I/O > + > + > + > + > + > + int ioctl > + int fd > + int request > + struct v4l2_buffer *argp > + > + > + > + > + > + Arguments > + > + > + > + fd > + > + &fd; > + > + > + > + request > + > + VIDIOC_PREPARE_BUF > + > + > + > + argp > + > + > + > + > + > + > + > + > + Description > + > + Applications can optionally call the > +VIDIOC_PREPARE_BUF ioctl to pass ownership of the buffer > +to the driver before actually enqueuing it, using the > +VIDIOC_QBUF ioctl, and to prepare it for future I/O. s/VIDIOC_QBUF/&VIDIOC_QBUF;/ > +Such preparations may include cache invalidation or cleaning. Performing them > +in advance saves time during the actual I/O. In case such cache operations are > +not required, the application can use one of > +V4L2_BUF_FLAG_NO_CACHE_INVALIDATE and > +V4L2_BUF_FLAG_NO_CACHE_CLEAN flags to skip the respective > +step. > + > + The v4l2_buffer structure is s/v4l2_buffer/&v4l2-buffer;/ > +specified in . > + > + > + > + &return-value; > + > + > + > + EBUSY > + > + File I/O is in progress. > + > + > + > + EINVAL > + > + The buffer type is not > +supported, or the index is out of bounds, > +or no buffers have been allocated yet, or the > +userptr or > +length are invalid. > + > + > + > + > + > + > + > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c > index 61979b7..ee5eec8 100644 > --- a/drivers/media/video/v4l2-compat-ioctl32.c > +++ b/drivers/media/video/v4l2-compat-ioctl32.c > @@ -702,6 +702,7 @@ static int put_v4l2_event32(struct v4l2_event *kp, struct v4l2_event32 __user *u > #define VIDIOC_S_EXT_CTRLS32 _IOWR('V', 72, struct v4l2_ext_controls32) > #define VIDIOC_TRY_EXT_CTRLS32 _IOWR('V', 73, struct v4l2_ext_controls32) > #define VIDIOC_DQEVENT32 _IOR ('V', 89, struct v4l2_event32) > +#define VIDIOC_PREPARE_BUF32 _IOWR('V', 93, struct v4l2_buffer32) > > #define VIDIOC_OVERLAY32 _IOW ('V', 14, s32) > #define VIDIOC_STREAMON32 _IOW ('V', 18, s32) > @@ -751,6 +752,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > case VIDIOC_S_INPUT32: cmd = VIDIOC_S_INPUT; break; > case VIDIOC_G_OUTPUT32: cmd = VIDIOC_G_OUTPUT; break; > case VIDIOC_S_OUTPUT32: cmd = VIDIOC_S_OUTPUT; break; > + case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break; > } > > switch (cmd) { > @@ -775,6 +777,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > compatible_arg = 0; > break; > > + case VIDIOC_PREPARE_BUF: > case VIDIOC_QUERYBUF: > case VIDIOC_QBUF: > case VIDIOC_DQBUF: > @@ -860,6 +863,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > err = put_v4l2_format32(&karg.v2f, up); > break; > > + case VIDIOC_PREPARE_BUF: > case VIDIOC_QUERYBUF: > case VIDIOC_QBUF: > case VIDIOC_DQBUF: > @@ -959,6 +963,8 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) > case VIDIOC_DQEVENT32: > case VIDIOC_SUBSCRIBE_EVENT: > case VIDIOC_UNSUBSCRIBE_EVENT: > + case VIDIOC_CREATE_BUFS: > + case VIDIOC_PREPARE_BUF32: > ret = do_video_ioctl(file, cmd, arg); > break; > > diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c > index 002ce13..3da87c0 100644 > --- a/drivers/media/video/v4l2-ioctl.c > +++ b/drivers/media/video/v4l2-ioctl.c > @@ -260,6 +260,8 @@ static const char *v4l2_ioctls[] = { > [_IOC_NR(VIDIOC_DQEVENT)] = "VIDIOC_DQEVENT", > [_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)] = "VIDIOC_SUBSCRIBE_EVENT", > [_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT", > + [_IOC_NR(VIDIOC_CREATE_BUFS)] = "VIDIOC_CREATE_BUFS", > + [_IOC_NR(VIDIOC_PREPARE_BUF)] = "VIDIOC_PREPARE_BUF", > }; > #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls) > > @@ -2216,6 +2218,30 @@ static long __video_do_ioctl(struct file *file, > dbgarg(cmd, "type=0x%8.8x", sub->type); > break; > } > + case VIDIOC_CREATE_BUFS: > + { > + struct v4l2_create_buffers *create = arg; > + > + if (!ops->vidioc_create_bufs) > + break; > + > + ret = ops->vidioc_create_bufs(file, fh, create); > + > + dbgarg(cmd, "count=%u @ %u\n", create->count, create->index); > + break; > + } > + case VIDIOC_PREPARE_BUF: > + { > + struct v4l2_buffer *b = arg; > + > + if (!ops->vidioc_prepare_buf) > + break; > + > + ret = ops->vidioc_prepare_buf(file, fh, b); > + > + dbgarg(cmd, "index=%d", b->index); > + break; > + } > default: > { > bool valid_prio = true; > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h > index fca24cc..3cd0cb3 100644 > --- a/include/linux/videodev2.h > +++ b/include/linux/videodev2.h > @@ -653,6 +653,9 @@ struct v4l2_buffer { > #define V4L2_BUF_FLAG_ERROR 0x0040 > #define V4L2_BUF_FLAG_TIMECODE 0x0100 /* timecode field is valid */ > #define V4L2_BUF_FLAG_INPUT 0x0200 /* input field is valid */ > +/* Cache handling flags */ > +#define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0400 > +#define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x0800 Do we want to have a flag to tell the buffer has been prepared but not queued yet? I'm not at all certain such a flag would be required; just asking. Perhaps the ERROR flag should be reset if it is present. If the answer to either of the above is yes, the PREPARE_BUF would need to be WR ioctl. > /* > * O V E R L A Y P R E V I E W > @@ -2092,6 +2095,18 @@ struct v4l2_dbg_chip_ident { > __u32 revision; /* chip revision, chip specific */ > } __attribute__ ((packed)); > > +/* VIDIOC_CREATE_BUFS */ > +struct v4l2_create_buffers { > + __u32 index; /* output: buffers index...index + count - 1 have been created */ > + __u32 count; > + __u32 type; > + __u32 memory; > + __u32 fourcc; > + __u32 num_planes; > + __u32 sizes[VIDEO_MAX_PLANES]; > + __u32 reserved[18]; > +}; > + > /* > * I O C T L C O D E S F O R V I D E O D E V I C E S > * > @@ -2182,6 +2197,9 @@ struct v4l2_dbg_chip_ident { > #define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct v4l2_event_subscription) > #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription) > > +#define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct v4l2_create_buffers) > +#define VIDIOC_PREPARE_BUF _IOW('V', 93, struct v4l2_buffer) > /* Reminder: when adding new ioctls please add support for them to > drivers/media/video/v4l2-compat-ioctl32.c as well! */ > > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h > index dd9f1e7..4d1c74a 100644 > --- a/include/media/v4l2-ioctl.h > +++ b/include/media/v4l2-ioctl.h > @@ -122,6 +122,8 @@ struct v4l2_ioctl_ops { > int (*vidioc_qbuf) (struct file *file, void *fh, struct v4l2_buffer *b); > int (*vidioc_dqbuf) (struct file *file, void *fh, struct v4l2_buffer *b); > > + int (*vidioc_create_bufs)(struct file *file, void *fh, struct v4l2_create_buffers *b); > + int (*vidioc_prepare_buf)(struct file *file, void *fh, struct v4l2_buffer *b); > > int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i); > int (*vidioc_g_fbuf) (struct file *file, void *fh, Kind regards, -- Sakari Ailus sakari.ailus@iki.fi