| Message ID | Pine.LNX.4.64.1108050908590.26715@axis700.grange (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
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 <g.liakhovetski@gmx.de> > --- 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.</entry> > Applications set or clear this flag before calling the > <constant>VIDIOC_QBUF</constant> ioctl.</entry> > </row> > + <row> > + <entry><constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant></entry> > + <entry>0x0400</entry> > + <entry>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. > +</entry> > + </row> > + <row> > + <entry><constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant></entry> > + <entry>0x0800</entry> > + <entry>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.</entry> > + </row> > </tbody> > </tgroup> > </table> > 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.</revremark> > &sub-close; > &sub-ioctl; > <!-- All ioctls go here. --> > + &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.</revremark> > &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 @@ > +<refentry id="vidioc-create-bufs"> > + <refmeta> > + <refentrytitle>ioctl VIDIOC_CREATE_BUFS</refentrytitle> > + &manvol; > + </refmeta> > + > + <refnamediv> > + <refname>VIDIOC_CREATE_BUFS</refname> > + <refpurpose>Create buffers for Memory Mapped or User Pointer I/O</refpurpose> > + </refnamediv> > + > + <refsynopsisdiv> > + <funcsynopsis> > + <funcprototype> > + <funcdef>int <function>ioctl</function></funcdef> > + <paramdef>int <parameter>fd</parameter></paramdef> > + <paramdef>int <parameter>request</parameter></paramdef> > + <paramdef>struct v4l2_create_buffers *<parameter>argp</parameter></paramdef> > + </funcprototype> > + </funcsynopsis> > + </refsynopsisdiv> > + > + <refsect1> > + <title>Arguments</title> > + > + <variablelist> > + <varlistentry> > + <term><parameter>fd</parameter></term> > + <listitem> > + <para>&fd;</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><parameter>request</parameter></term> > + <listitem> > + <para>VIDIOC_CREATE_BUFS</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><parameter>argp</parameter></term> > + <listitem> > + <para></para> > + </listitem> > + </varlistentry> > + </variablelist> > + </refsect1> > + > + <refsect1> > + <title>Description</title> > + > + <para>This ioctl is used to create buffers for <link linkend="mmap">memory > +mapped</link> or <link linkend="userp">user pointer</link> > +I/O. It can be used as an alternative or in addition to the > +<constant>VIDIOC_REQBUFS</constant> ioctl, when a tighter control over buffers > +is required. This ioctl can be called multiple times to create buffers of > +different sizes.</para> > + > + <para>To allocate device buffers applications initialize relevant > +fields of the <structname>v4l2_create_buffers</structname> structure. > +They set the <structfield>type</structfield> field to the respective stream > +or buffer type. <structfield>count</structfield> must be set to the number of > +required buffers. <structfield>memory</structfield> specifies the required I/O > +method. If <structfield>num_planes</structfield> == 0 or all elements of the > +<structfield>sizes</structfield> array are 0, then buffers, suitable for the > +currently configured video format, are allocated, exactly like for the > +<constant>VIDIOC_REQBUFS</constant> ioctl. If > +<structfield>num_planes</structfield> > 0 and at least some of the respective > +<structfield>sizes</structfield> elements are non-zero, this information will be > +used for buffer-allocation. The <structfield>reserved</structfield> 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 > +<structfield>count</structfield> and the <structfield>index</structfield> > +fields respectively. On return <structfield>count</structfield> can be smaller > +than the number requested.</para> > + <para>When the I/O method is not supported the ioctl > +returns an &EINVAL;.</para> > + > + <table pgwide="1" frame="none" id="v4l2-create-buffers"> > + <title>struct <structname>v4l2_create_buffers</structname></title> > + <tgroup cols="3"> > + &cs-str; > + <tbody valign="top"> > + <row> > + <entry>__u32</entry> > + <entry><structfield>index</structfield></entry> > + <entry>The starting buffer index, returned by the driver.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>count</structfield></entry> > + <entry>The number of buffers requested or granted.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>type</structfield></entry> > + <entry>V4L2 buffer type: one of <constant>V4L2_BUF_TYPE_*</constant> > +values.</entry> &v4l2-buf-type; here? > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>memory</structfield></entry> > + <entry>Applications set this field to > +<constant>V4L2_MEMORY_MMAP</constant> or > +<constant>V4L2_MEMORY_USERPTR</constant>.</entry> &v4l2-memory; > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>fourcc</structfield></entry> > + <entry>One of V4L2_PIX_FMT_* FOURCCs of the video data.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>num_planes</structfield></entry> > + <entry>Number of planes or 0 to use the current format.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>size[VIDEO_MAX_PLANES]</structfield></entry> > + <entry>Explicit sizes of buffers, being created.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>reserved</structfield>[18]</entry> > + <entry>A place holder for future extensions.</entry> > + </row> > + </tbody> > + </tgroup> > + </table> > + </refsect1> > + > + <refsect1> > + &return-value; > + > + <variablelist> > + <varlistentry> > + <term><errorcode>ENOMEM</errorcode></term> > + <listitem> > + <para>No memory to allocate buffers for <link linkend="mmap">memory > +mapped</link> I/O.</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><errorcode>EINVAL</errorcode></term> > + <listitem> > + <para>The buffer type (<structfield>type</structfield> field) or the > +requested I/O method (<structfield>memory</structfield>) is not > +supported.</para> > + </listitem> > + </varlistentry> > + </variablelist> > + </refsect1> > +</refentry> > + > +<!-- > +Local Variables: > +mode: sgml > +sgml-parent-document: "v4l2.sgml" > +indent-tabs-mode: nil > +End: > +--> > 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 @@ > +<refentry id="vidioc-prepare-buf"> > + <refmeta> > + <refentrytitle>ioctl VIDIOC_PREPARE_BUF</refentrytitle> > + &manvol; > + </refmeta> > + > + <refnamediv> > + <refname>VIDIOC_PREPARE_BUF</refname> > + <refpurpose>Prepare a buffer for I/O</refpurpose> > + </refnamediv> > + > + <refsynopsisdiv> > + <funcsynopsis> > + <funcprototype> > + <funcdef>int <function>ioctl</function></funcdef> > + <paramdef>int <parameter>fd</parameter></paramdef> > + <paramdef>int <parameter>request</parameter></paramdef> > + <paramdef>struct v4l2_buffer *<parameter>argp</parameter></paramdef> > + </funcprototype> > + </funcsynopsis> > + </refsynopsisdiv> > + > + <refsect1> > + <title>Arguments</title> > + > + <variablelist> > + <varlistentry> > + <term><parameter>fd</parameter></term> > + <listitem> > + <para>&fd;</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><parameter>request</parameter></term> > + <listitem> > + <para>VIDIOC_PREPARE_BUF</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><parameter>argp</parameter></term> > + <listitem> > + <para></para> > + </listitem> > + </varlistentry> > + </variablelist> > + </refsect1> > + > + <refsect1> > + <title>Description</title> > + > + <para>Applications can optionally call the > +<constant>VIDIOC_PREPARE_BUF</constant> ioctl to pass ownership of the buffer > +to the driver before actually enqueuing it, using the > +<constant>VIDIOC_QBUF</constant> ioctl, and to prepare it for future I/O. s/<constant>VIDIOC_QBUF</constant>/&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 > +<constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant> and > +<constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant> flags to skip the respective > +step.</para> > + > + <para>The <structname>v4l2_buffer</structname> structure is s/<structname>v4l2_buffer</structname>/&v4l2-buffer;/ > +specified in <xref linkend="buffer" />.</para> > + </refsect1> > + > + <refsect1> > + &return-value; > + > + <variablelist> > + <varlistentry> > + <term><errorcode>EBUSY</errorcode></term> > + <listitem> > + <para>File I/O is in progress.</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><errorcode>EINVAL</errorcode></term> > + <listitem> > + <para>The buffer <structfield>type</structfield> is not > +supported, or the <structfield>index</structfield> is out of bounds, > +or no buffers have been allocated yet, or the > +<structfield>userptr</structfield> or > +<structfield>length</structfield> are invalid.</para> > + </listitem> > + </varlistentry> > + </variablelist> > + </refsect1> > +</refentry> > + > +<!-- > +Local Variables: > +mode: sgml > +sgml-parent-document: "v4l2.sgml" > +indent-tabs-mode: nil > +End: > +--> > 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,
Hi Guennadi, On Fri, Aug 5, 2011 at 00:47, Guennadi Liakhovetski <g.liakhovetski@gmx.de> 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 <g.liakhovetski@gmx.de> > --- > > 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.</entry> > Applications set or clear this flag before calling the > <constant>VIDIOC_QBUF</constant> ioctl.</entry> > </row> > + <row> > + <entry><constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant></entry> > + <entry>0x0400</entry> > + <entry>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. > +</entry> > + </row> > + <row> > + <entry><constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant></entry> > + <entry>0x0800</entry> > + <entry>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.</entry> > + </row> > </tbody> > </tgroup> > </table> > 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.</revremark> > &sub-close; > &sub-ioctl; > <!-- All ioctls go here. --> > + &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.</revremark> > &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 @@ > +<refentry id="vidioc-create-bufs"> > + <refmeta> > + <refentrytitle>ioctl VIDIOC_CREATE_BUFS</refentrytitle> > + &manvol; > + </refmeta> > + > + <refnamediv> > + <refname>VIDIOC_CREATE_BUFS</refname> > + <refpurpose>Create buffers for Memory Mapped or User Pointer I/O</refpurpose> > + </refnamediv> > + > + <refsynopsisdiv> > + <funcsynopsis> > + <funcprototype> > + <funcdef>int <function>ioctl</function></funcdef> > + <paramdef>int <parameter>fd</parameter></paramdef> > + <paramdef>int <parameter>request</parameter></paramdef> > + <paramdef>struct v4l2_create_buffers *<parameter>argp</parameter></paramdef> > + </funcprototype> > + </funcsynopsis> > + </refsynopsisdiv> > + > + <refsect1> > + <title>Arguments</title> > + > + <variablelist> > + <varlistentry> > + <term><parameter>fd</parameter></term> > + <listitem> > + <para>&fd;</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><parameter>request</parameter></term> > + <listitem> > + <para>VIDIOC_CREATE_BUFS</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><parameter>argp</parameter></term> > + <listitem> > + <para></para> > + </listitem> > + </varlistentry> > + </variablelist> > + </refsect1> > + > + <refsect1> > + <title>Description</title> > + > + <para>This ioctl is used to create buffers for <link linkend="mmap">memory > +mapped</link> or <link linkend="userp">user pointer</link> > +I/O. It can be used as an alternative or in addition to the > +<constant>VIDIOC_REQBUFS</constant> ioctl, when a tighter control over buffers > +is required. This ioctl can be called multiple times to create buffers of > +different sizes.</para> > + > + <para>To allocate device buffers applications initialize relevant > +fields of the <structname>v4l2_create_buffers</structname> structure. > +They set the <structfield>type</structfield> field to the respective stream > +or buffer type. <structfield>count</structfield> must be set to the number of > +required buffers. <structfield>memory</structfield> specifies the required I/O > +method. If <structfield>num_planes</structfield> == 0 or all elements of the > +<structfield>sizes</structfield> array are 0, then buffers, suitable for the > +currently configured video format, are allocated, exactly like for the > +<constant>VIDIOC_REQBUFS</constant> ioctl. If > +<structfield>num_planes</structfield> > 0 and at least some of the respective > +<structfield>sizes</structfield> elements are non-zero, this information will be Instead of "at least some must be filled", I would say it more strongly: "If num_planes > 0, the elements in range 0 to num_planes-1 have to be filled with desired plane sizes for each buffer. This information will then be used instead of current format for buffer allocation." or something similar. Unless we want to allow sizes for some planes to be taken from format and some from sizes[], which I doubt... > +used for buffer-allocation. The <structfield>reserved</structfield> 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 > +<structfield>count</structfield> and the <structfield>index</structfield> > +fields respectively. On return <structfield>count</structfield> can be smaller Needs a comma after "return". > +than the number requested.</para> > + <para>When the I/O method is not supported the ioctl > +returns an &EINVAL;.</para> > + > + <table pgwide="1" frame="none" id="v4l2-create-buffers"> > + <title>struct <structname>v4l2_create_buffers</structname></title> > + <tgroup cols="3"> > + &cs-str; > + <tbody valign="top"> > + <row> > + <entry>__u32</entry> > + <entry><structfield>index</structfield></entry> > + <entry>The starting buffer index, returned by the driver.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>count</structfield></entry> > + <entry>The number of buffers requested or granted.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>type</structfield></entry> > + <entry>V4L2 buffer type: one of <constant>V4L2_BUF_TYPE_*</constant> > +values.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>memory</structfield></entry> > + <entry>Applications set this field to > +<constant>V4L2_MEMORY_MMAP</constant> or > +<constant>V4L2_MEMORY_USERPTR</constant>.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>fourcc</structfield></entry> > + <entry>One of V4L2_PIX_FMT_* FOURCCs of the video data.</entry> > + </row> If sizes is zero, we use the currently configured format, but if this is filled, this overrides the currently selected format, right? But in that case, sizes should be specified as well. I think it should be described explicitly above. So maybe "if fourcc is specified, sizes has to be filled and those parameters override the currently configured format; if either fourcc or sizes is not specified, the currently configured format is used". > + <row> > + <entry>__u32</entry> > + <entry><structfield>num_planes</structfield></entry> > + <entry>Number of planes or 0 to use the current format.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>size[VIDEO_MAX_PLANES]</structfield></entry> > + <entry>Explicit sizes of buffers, being created.</entry> > + </row> > + <row> > + <entry>__u32</entry> > + <entry><structfield>reserved</structfield>[18]</entry> > + <entry>A place holder for future extensions.</entry> > + </row> > + </tbody> > + </tgroup> > + </table> > + </refsect1> > + > + <refsect1> > + &return-value; > + > + <variablelist> > + <varlistentry> > + <term><errorcode>ENOMEM</errorcode></term> > + <listitem> > + <para>No memory to allocate buffers for <link linkend="mmap">memory > +mapped</link> I/O.</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><errorcode>EINVAL</errorcode></term> > + <listitem> > + <para>The buffer type (<structfield>type</structfield> field) or the > +requested I/O method (<structfield>memory</structfield>) is not > +supported.</para> Do we also return this error if the provided fourcc format is not supported? > + </listitem> > + </varlistentry> > + </variablelist> > + </refsect1> > +</refentry> > + > +<!-- > +Local Variables: > +mode: sgml > +sgml-parent-document: "v4l2.sgml" > +indent-tabs-mode: nil > +End: > +--> > 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 @@ > +<refentry id="vidioc-prepare-buf"> > + <refmeta> > + <refentrytitle>ioctl VIDIOC_PREPARE_BUF</refentrytitle> > + &manvol; > + </refmeta> > + > + <refnamediv> > + <refname>VIDIOC_PREPARE_BUF</refname> > + <refpurpose>Prepare a buffer for I/O</refpurpose> > + </refnamediv> > + > + <refsynopsisdiv> > + <funcsynopsis> > + <funcprototype> > + <funcdef>int <function>ioctl</function></funcdef> > + <paramdef>int <parameter>fd</parameter></paramdef> > + <paramdef>int <parameter>request</parameter></paramdef> > + <paramdef>struct v4l2_buffer *<parameter>argp</parameter></paramdef> > + </funcprototype> > + </funcsynopsis> > + </refsynopsisdiv> > + > + <refsect1> > + <title>Arguments</title> > + > + <variablelist> > + <varlistentry> > + <term><parameter>fd</parameter></term> > + <listitem> > + <para>&fd;</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><parameter>request</parameter></term> > + <listitem> > + <para>VIDIOC_PREPARE_BUF</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><parameter>argp</parameter></term> > + <listitem> > + <para></para> > + </listitem> > + </varlistentry> > + </variablelist> > + </refsect1> > + > + <refsect1> > + <title>Description</title> > + > + <para>Applications can optionally call the > +<constant>VIDIOC_PREPARE_BUF</constant> ioctl to pass ownership of the buffer > +to the driver before actually enqueuing it, using the > +<constant>VIDIOC_QBUF</constant> ioctl, and to prepare it for future I/O. > +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 > +<constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant> and > +<constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant> flags to skip the respective > +step.</para> > + > + <para>The <structname>v4l2_buffer</structname> structure is > +specified in <xref linkend="buffer" />.</para> > + </refsect1> > + > + <refsect1> > + &return-value; > + > + <variablelist> > + <varlistentry> > + <term><errorcode>EBUSY</errorcode></term> > + <listitem> > + <para>File I/O is in progress.</para> > + </listitem> > + </varlistentry> > + <varlistentry> > + <term><errorcode>EINVAL</errorcode></term> > + <listitem> > + <para>The buffer <structfield>type</structfield> is not > +supported, or the <structfield>index</structfield> is out of bounds, > +or no buffers have been allocated yet, or the > +<structfield>userptr</structfield> or > +<structfield>length</structfield> are invalid.</para> > + </listitem> > + </varlistentry> > + </variablelist> > + </refsect1> > +</refentry> > + > +<!-- > +Local Variables: > +mode: sgml > +sgml-parent-document: "v4l2.sgml" > +indent-tabs-mode: nil > +End: > +--> > 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 > > /* > * 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, > -- > 1.7.2.5 > >
Hi Guennadi! On Friday, August 05, 2011 09:47:13 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 <g.liakhovetski@gmx.de> > --- > > 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 > <snip> > 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 > > /* > * 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 know you are going to hate me for this, but I've changed my mind: I think this should use a struct v4l2_format after all. This change of heart came out of discussions during the V4L2 brainstorm meeting last week. The only way to be sure the buffers are allocated optimally is if the driver has all the information. The easiest way to do that is by passing struct v4l2_format. This is also consistent with REQBUFS since that uses the information from the currently selected format (i.e. what you get back from VIDIOC_G_FMT). There can be subtle behaviors such as allocating from different memory back based on the fourcc and the size of the image. One reason why I liked passing sizes directly is that it allows the caller to ask for more memory than is strictly necessary. However, while brainstorming last week the suggestion was made that there is no reason why the user can't set the sizeimage field in v4l2_pix_format(_mplane) to something higher. The S/TRY_FMT spec explicitly mentions that the sizeimage field is set by the driver, but for the new CREATEBUFS ioctl no such limitation has to be placed. The only thing necessary is to ensure that sizeimage is not too small (and you probably want some sanity check against crazy values as well). This way the decision on how to allocate memory is the same between REQBUFS and CREATEBUFS (i.e. both use v4l2_format information), but there is no need for a union as we had in the initial proposal since apps can set the sizeimage to something larger than strictly necessary (or just leave it to 0 to get the smallest size). Regards, Hans > + > /* > * 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, > -- > 1.7.2.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 August 2011 11:16:41 Hans Verkuil wrote: > On Friday, August 05, 2011 09:47:13 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 <g.liakhovetski@gmx.de> > > --- > > > > 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 > > <snip> > > > 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 > > > > /* > > > > * 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 know you are going to hate me for this, but I've changed my mind: I think > this should use a struct v4l2_format after all. > > This change of heart came out of discussions during the V4L2 brainstorm > meeting last week. The only way to be sure the buffers are allocated > optimally is if the driver has all the information. The easiest way to do > that is by passing struct v4l2_format. This is also consistent with > REQBUFS since that uses the information from the currently selected format > (i.e. what you get back from VIDIOC_G_FMT). > > There can be subtle behaviors such as allocating from different memory back > based on the fourcc and the size of the image. > > One reason why I liked passing sizes directly is that it allows the caller > to ask for more memory than is strictly necessary. > > However, while brainstorming last week the suggestion was made that there > is no reason why the user can't set the sizeimage field in > v4l2_pix_format(_mplane) to something higher. The S/TRY_FMT spec explicitly > mentions that the sizeimage field is set by the driver, but for the new > CREATEBUFS ioctl no such limitation has to be placed. The only thing > necessary is to ensure that sizeimage is not too small (and you probably > want some sanity check against crazy values as well). We need to decide on a policy here. What should be the maximum allowable size for MMAP buffers ? How do we restrict the requested image size so that application won't be allowed to starve the system by requesting memory for 1GP images ? > This way the decision on how to allocate memory is the same between REQBUFS > and CREATEBUFS (i.e. both use v4l2_format information), but there is no > need for a union as we had in the initial proposal since apps can set the > sizeimage to something larger than strictly necessary (or just leave it to > 0 to get the smallest size).
On Monday, August 08, 2011 13:40:23 Laurent Pinchart wrote: > On Monday 08 August 2011 11:16:41 Hans Verkuil wrote: > > On Friday, August 05, 2011 09:47:13 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 <g.liakhovetski@gmx.de> > > > --- > > > > > > 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 > > > > <snip> > > > > > 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 > > > > > > /* > > > > > > * 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 know you are going to hate me for this, but I've changed my mind: I think > > this should use a struct v4l2_format after all. > > > > This change of heart came out of discussions during the V4L2 brainstorm > > meeting last week. The only way to be sure the buffers are allocated > > optimally is if the driver has all the information. The easiest way to do > > that is by passing struct v4l2_format. This is also consistent with > > REQBUFS since that uses the information from the currently selected format > > (i.e. what you get back from VIDIOC_G_FMT). > > > > There can be subtle behaviors such as allocating from different memory back > > based on the fourcc and the size of the image. > > > > One reason why I liked passing sizes directly is that it allows the caller > > to ask for more memory than is strictly necessary. > > > > However, while brainstorming last week the suggestion was made that there > > is no reason why the user can't set the sizeimage field in > > v4l2_pix_format(_mplane) to something higher. The S/TRY_FMT spec explicitly > > mentions that the sizeimage field is set by the driver, but for the new > > CREATEBUFS ioctl no such limitation has to be placed. The only thing > > necessary is to ensure that sizeimage is not too small (and you probably > > want some sanity check against crazy values as well). > > We need to decide on a policy here. What should be the maximum allowable size > for MMAP buffers ? How do we restrict the requested image size so that > application won't be allowed to starve the system by requesting memory for 1GP > images ? Either just a arbitrary cap like 1 GB (mainly to prevent any weird calculation problems around the 2 GB (signedness) and 4 GB (wrap-around) boundaries), or something like 3 or 4 times the minimum buffer size. I'm in favor of enforcing a 1 GB cap in vb2 and letting drivers enforce a policy of their own if that makes sense for them. I don't really see a problem with requesting large amounts of memory. What constitutes 'large' is not something the kernel knows, that's dependent on the use case. Regards, Hans > > > This way the decision on how to allocate memory is the same between REQBUFS > > and CREATEBUFS (i.e. both use v4l2_format information), but there is no > > need for a union as we had in the initial proposal since apps can set the > > sizeimage to something larger than strictly necessary (or just leave it to > > 0 to get the smallest size). > > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Monday 08 August 2011 14:40:27 Hans Verkuil wrote: > On Monday, August 08, 2011 13:40:23 Laurent Pinchart wrote: > > On Monday 08 August 2011 11:16:41 Hans Verkuil wrote: > > > On Friday, August 05, 2011 09:47:13 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 <g.liakhovetski@gmx.de> > > > > --- > > > > > > > > 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 > > > > > > <snip> > > > > > > > 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 > > > > > > > > /* > > > > > > > > * 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 know you are going to hate me for this, but I've changed my mind: I > > > think this should use a struct v4l2_format after all. > > > > > > This change of heart came out of discussions during the V4L2 brainstorm > > > meeting last week. The only way to be sure the buffers are allocated > > > optimally is if the driver has all the information. The easiest way to > > > do that is by passing struct v4l2_format. This is also consistent with > > > REQBUFS since that uses the information from the currently selected > > > format (i.e. what you get back from VIDIOC_G_FMT). > > > > > > There can be subtle behaviors such as allocating from different memory > > > back based on the fourcc and the size of the image. > > > > > > One reason why I liked passing sizes directly is that it allows the > > > caller to ask for more memory than is strictly necessary. > > > > > > However, while brainstorming last week the suggestion was made that > > > there is no reason why the user can't set the sizeimage field in > > > v4l2_pix_format(_mplane) to something higher. The S/TRY_FMT spec > > > explicitly mentions that the sizeimage field is set by the driver, but > > > for the new CREATEBUFS ioctl no such limitation has to be placed. The > > > only thing necessary is to ensure that sizeimage is not too small (and > > > you probably want some sanity check against crazy values as well). > > > > We need to decide on a policy here. What should be the maximum allowable > > size for MMAP buffers ? How do we restrict the requested image size so > > that application won't be allowed to starve the system by requesting > > memory for 1GP images ? > > Either just a arbitrary cap like 1 GB (mainly to prevent any weird > calculation problems around the 2 GB (signedness) and 4 GB (wrap-around) > boundaries), or something like 3 or 4 times the minimum buffer size. > > I'm in favor of enforcing a 1 GB cap in vb2 and letting drivers enforce a > policy of their own if that makes sense for them. Wouldn't that be a security issue ? Any application with permissions to access the video device could DoS the system. > I don't really see a problem with requesting large amounts of memory. What > constitutes 'large' is not something the kernel knows, that's dependent on > the use case.
Laurent Pinchart wrote: > Hi Hans, > > On Monday 08 August 2011 14:40:27 Hans Verkuil wrote: >> On Monday, August 08, 2011 13:40:23 Laurent Pinchart wrote: >>> On Monday 08 August 2011 11:16:41 Hans Verkuil wrote: >>>> On Friday, August 05, 2011 09:47:13 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<g.liakhovetski@gmx.de> >>>>> --- >>>>> >>>>> 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 >>>> >>>> <snip> >>>> >>>>> 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 >>>>> >>>>> /* >>>>> >>>>> * 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 know you are going to hate me for this, but I've changed my mind: I >>>> think this should use a struct v4l2_format after all. >>>> >>>> This change of heart came out of discussions during the V4L2 brainstorm >>>> meeting last week. The only way to be sure the buffers are allocated >>>> optimally is if the driver has all the information. The easiest way to >>>> do that is by passing struct v4l2_format. This is also consistent with >>>> REQBUFS since that uses the information from the currently selected >>>> format (i.e. what you get back from VIDIOC_G_FMT). >>>> >>>> There can be subtle behaviors such as allocating from different memory >>>> back based on the fourcc and the size of the image. >>>> >>>> One reason why I liked passing sizes directly is that it allows the >>>> caller to ask for more memory than is strictly necessary. >>>> >>>> However, while brainstorming last week the suggestion was made that >>>> there is no reason why the user can't set the sizeimage field in >>>> v4l2_pix_format(_mplane) to something higher. The S/TRY_FMT spec >>>> explicitly mentions that the sizeimage field is set by the driver, but >>>> for the new CREATEBUFS ioctl no such limitation has to be placed. The >>>> only thing necessary is to ensure that sizeimage is not too small (and >>>> you probably want some sanity check against crazy values as well). >>> >>> We need to decide on a policy here. What should be the maximum allowable >>> size for MMAP buffers ? How do we restrict the requested image size so >>> that application won't be allowed to starve the system by requesting >>> memory for 1GP images ? >> >> Either just a arbitrary cap like 1 GB (mainly to prevent any weird >> calculation problems around the 2 GB (signedness) and 4 GB (wrap-around) >> boundaries), or something like 3 or 4 times the minimum buffer size. >> >> I'm in favor of enforcing a 1 GB cap in vb2 and letting drivers enforce a >> policy of their own if that makes sense for them. > > Wouldn't that be a security issue ? Any application with permissions to access > the video device could DoS the system. I wonder if it would make sense to add a new resource limit for this. That should make it easy to have a common default while keeping it easily changeable. The limit could apply to multimedia related buffers that typically are pinned to memory. Or perhaps we just use RLIMIT_MEMLOCK; that's what it really is after all. The manual page (man getrlimit) isn't very clear whether it's supposed to apply to process or user id, though. The defaults would need to change; in my system with 3 GiB of memory the default seems to be 64 kiB... Cheers,
On Tuesday, August 09, 2011 00:06:10 Laurent Pinchart wrote: > Hi Hans, > > On Monday 08 August 2011 14:40:27 Hans Verkuil wrote: > > On Monday, August 08, 2011 13:40:23 Laurent Pinchart wrote: > > > On Monday 08 August 2011 11:16:41 Hans Verkuil wrote: > > > > On Friday, August 05, 2011 09:47:13 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 <g.liakhovetski@gmx.de> > > > > > --- > > > > > > > > > > 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 > > > > > > > > <snip> > > > > > > > > > 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 > > > > > > > > > > /* > > > > > > > > > > * 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 know you are going to hate me for this, but I've changed my mind: I > > > > think this should use a struct v4l2_format after all. > > > > > > > > This change of heart came out of discussions during the V4L2 brainstorm > > > > meeting last week. The only way to be sure the buffers are allocated > > > > optimally is if the driver has all the information. The easiest way to > > > > do that is by passing struct v4l2_format. This is also consistent with > > > > REQBUFS since that uses the information from the currently selected > > > > format (i.e. what you get back from VIDIOC_G_FMT). > > > > > > > > There can be subtle behaviors such as allocating from different memory > > > > back based on the fourcc and the size of the image. > > > > > > > > One reason why I liked passing sizes directly is that it allows the > > > > caller to ask for more memory than is strictly necessary. > > > > > > > > However, while brainstorming last week the suggestion was made that > > > > there is no reason why the user can't set the sizeimage field in > > > > v4l2_pix_format(_mplane) to something higher. The S/TRY_FMT spec > > > > explicitly mentions that the sizeimage field is set by the driver, but > > > > for the new CREATEBUFS ioctl no such limitation has to be placed. The > > > > only thing necessary is to ensure that sizeimage is not too small (and > > > > you probably want some sanity check against crazy values as well). > > > > > > We need to decide on a policy here. What should be the maximum allowable > > > size for MMAP buffers ? How do we restrict the requested image size so > > > that application won't be allowed to starve the system by requesting > > > memory for 1GP images ? > > > > Either just a arbitrary cap like 1 GB (mainly to prevent any weird > > calculation problems around the 2 GB (signedness) and 4 GB (wrap-around) > > boundaries), or something like 3 or 4 times the minimum buffer size. > > > > I'm in favor of enforcing a 1 GB cap in vb2 and letting drivers enforce a > > policy of their own if that makes sense for them. > > Wouldn't that be a security issue ? Any application with permissions to access > the video device could DoS the system. How is this any different from an application that tries to use more memory then there is available? It's an out-of-memory situation, that can happen at any time. Anyone can make an application that runs out of memory. Out-of-memory is not a security risk AFAIK. Note BTW that in practice kmalloc already has a cap (something like 16 or 32 MB, I believe it depends on the kernel .config) and so has CMA (the size of the CMA memory region(s)). So I do not think we need to do anything special here. Regards, Hans > > I don't really see a problem with requesting large amounts of memory. What > > constitutes 'large' is not something the kernel knows, that's dependent on > > the use case. > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 09, 2011 at 09:26:30AM +0200, Hans Verkuil wrote: ... > > Wouldn't that be a security issue ? Any application with permissions to access > > the video device could DoS the system. > > How is this any different from an application that tries to use more memory > then there is available? It's an out-of-memory situation, that can happen at > any time. Anyone can make an application that runs out of memory. > > Out-of-memory is not a security risk AFAIK. If you coun availability to security, then it is. This might not be an issue in embedded systems which have a single user, but think of the availability of the interface in e.g. a server. Also, this memory is locked to system physical memory, making it impossible to page it out to a block device. > Note BTW that in practice kmalloc already has a cap (something like 16 or 32 > MB, I believe it depends on the kernel .config) and so has CMA (the size of This is per a single allocation. A user could create any number of them.
On Wednesday, August 10, 2011 01:37:27 Sakari Ailus wrote: > On Tue, Aug 09, 2011 at 09:26:30AM +0200, Hans Verkuil wrote: > ... > > > Wouldn't that be a security issue ? Any application with permissions to access > > > the video device could DoS the system. > > > > How is this any different from an application that tries to use more memory > > then there is available? It's an out-of-memory situation, that can happen at > > any time. Anyone can make an application that runs out of memory. > > > > Out-of-memory is not a security risk AFAIK. > > If you coun availability to security, then it is. > > This might not be an issue in embedded systems which have a single user, but > think of the availability of the interface in e.g. a server. > > Also, this memory is locked to system physical memory, making it impossible > to page it out to a block device. So? Anyone can make a program that allocates and uses a lot of memory causing an out of memory error. I still don't see how that differs from trying to allocate these buffers. If the system has swap space (which I haven't used in years) then it may take longer before you run out of memory, but the effect is the same. Out of memory is a normal condition, not a security risk. The problem I have is that you can't really determine a valid policy here since that will depend entirely on your use-case and (embedded) device. Regards, Hans > > Note BTW that in practice kmalloc already has a cap (something like 16 or 32 > > MB, I believe it depends on the kernel .config) and so has CMA (the size of > > This is per a single allocation. A user could create any number of them. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 10, 2011 at 08:25:24AM +0200, Hans Verkuil wrote: > On Wednesday, August 10, 2011 01:37:27 Sakari Ailus wrote: > > On Tue, Aug 09, 2011 at 09:26:30AM +0200, Hans Verkuil wrote: > > ... > > > > Wouldn't that be a security issue ? Any application with permissions to access > > > > the video device could DoS the system. > > > > > > How is this any different from an application that tries to use more memory > > > then there is available? It's an out-of-memory situation, that can happen at > > > any time. Anyone can make an application that runs out of memory. > > > > > > Out-of-memory is not a security risk AFAIK. > > > > If you coun availability to security, then it is. > > > > This might not be an issue in embedded systems which have a single user, but > > think of the availability of the interface in e.g. a server. > > > > Also, this memory is locked to system physical memory, making it impossible > > to page it out to a block device. > > So? Anyone can make a program that allocates and uses a lot of memory causing > an out of memory error. I still don't see how that differs from trying to allocate > these buffers. The difference is between physical and virtual memory. Reserving buffers pinned in physical memory will starve all the other users very efficiently. > Out of memory is a normal condition, not a security risk. Administrators of largish servers with thousands of users might disagree. I have to admit I don't know their usage patterns very well so I have no demands on the issue. ulimit is being used in those systems as is quota, that I know. On the other hand, those systems typically do not contain V4L2 devices either. > The problem I have is that you can't really determine a valid policy here > since that will depend entirely on your use-case and (embedded) device. This is quite similar case as with the CMA in my opinion. The proposal (by Arnd, if my memory serves me correctly) was to limit the CMA allocations under certain percentage of the system memory address space. The limit could be overriddend e.g. in board code.
Hi Hans On Mon, 8 Aug 2011, Hans Verkuil wrote: > Hi Guennadi! > > On Friday, August 05, 2011 09:47:13 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 <g.liakhovetski@gmx.de> > > --- > > > > 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 > > > > <snip> > > > 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 > > > > /* > > * 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 know you are going to hate me for this, hm, I'll consider this possibility;-) > but I've changed my mind: I think > this should use a struct v4l2_format after all. > > This change of heart came out of discussions during the V4L2 brainstorm > meeting last week. The only way to be sure the buffers are allocated optimally > is if the driver has all the information. The easiest way to do that is by > passing struct v4l2_format. This is also consistent with REQBUFS since that > uses the information from the currently selected format (i.e. what you get > back from VIDIOC_G_FMT). > > There can be subtle behaviors such as allocating from different memory back > based on the fourcc and the size of the image. > > One reason why I liked passing sizes directly is that it allows the caller to > ask for more memory than is strictly necessary. > > However, while brainstorming last week the suggestion was made that there is > no reason why the user can't set the sizeimage field in > v4l2_pix_format(_mplane) to something higher. The S/TRY_FMT spec explicitly > mentions that the sizeimage field is set by the driver, but for the new > CREATEBUFS ioctl no such limitation has to be placed. The only thing necessary > is to ensure that sizeimage is not too small (and you probably want some > sanity check against crazy values as well). Centrally in videobuf2 or in each driver? > This way the decision on how to allocate memory is the same between REQBUFS > and CREATEBUFS (i.e. both use v4l2_format information), but there is no need > for a union as we had in the initial proposal since apps can set the sizeimage > to something larger than strictly necessary (or just leave it to 0 to get the > smallest size). There was no union in previous versions of the patch. You mean, we don't need the .size member? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: > Hi Hans > > On Mon, 8 Aug 2011, Hans Verkuil wrote: > > > Hi Guennadi! > > > > On Friday, August 05, 2011 09:47:13 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 <g.liakhovetski@gmx.de> > > > --- > > > > > > 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 > > > > > > > <snip> > > > > > 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 > > > > > > /* > > > * 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 know you are going to hate me for this, > > hm, I'll consider this possibility;-) > > > but I've changed my mind: I think > > this should use a struct v4l2_format after all. > > > > This change of heart came out of discussions during the V4L2 brainstorm > > meeting last week. The only way to be sure the buffers are allocated optimally > > is if the driver has all the information. The easiest way to do that is by > > passing struct v4l2_format. This is also consistent with REQBUFS since that > > uses the information from the currently selected format (i.e. what you get > > back from VIDIOC_G_FMT). > > > > There can be subtle behaviors such as allocating from different memory back > > based on the fourcc and the size of the image. > > > > One reason why I liked passing sizes directly is that it allows the caller to > > ask for more memory than is strictly necessary. > > > > However, while brainstorming last week the suggestion was made that there is > > no reason why the user can't set the sizeimage field in > > v4l2_pix_format(_mplane) to something higher. The S/TRY_FMT spec explicitly > > mentions that the sizeimage field is set by the driver, but for the new > > CREATEBUFS ioctl no such limitation has to be placed. The only thing necessary > > is to ensure that sizeimage is not too small (and you probably want some > > sanity check against crazy values as well). > > Centrally in videobuf2 or in each driver? The 'too small' check can only be done in the driver since the driver has to calculate the size based on the format. The 'is crazy value' check can be done centrally. But note the discussion on what constitutes 'crazy'. > > This way the decision on how to allocate memory is the same between REQBUFS > > and CREATEBUFS (i.e. both use v4l2_format information), but there is no need > > for a union as we had in the initial proposal since apps can set the sizeimage > > to something larger than strictly necessary (or just leave it to 0 to get the > > smallest size). > > There was no union in previous versions of the patch. You mean, we don't > need the .size member? Sorry, I thought we had a union containing the size and the format. Anyway, we don't need the .size member if we allow the user to set the sizeimage fields in the format when calling CREATEBUFS. Regards, Hans > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 15 Aug 2011, Hans Verkuil wrote: > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: > > Hi Hans > > > > On Mon, 8 Aug 2011, Hans Verkuil wrote: > > > > > Hi Guennadi! > > > > > > On Friday, August 05, 2011 09:47:13 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 <g.liakhovetski@gmx.de> > > > > --- > > > > > > > > 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 > > > > > > > > > > <snip> > > > > > > > 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 > > > > > > > > /* > > > > * 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 know you are going to hate me for this, > > > > hm, I'll consider this possibility;-) > > > > > but I've changed my mind: I think > > > this should use a struct v4l2_format after all. While switching back, I have to change the struct vb2_ops::queue_setup() operation to take a struct v4l2_create_buffers pointer. An earlier version of this patch just added one more parameter to .queue_setup(), which is easier - changes to videobuf2-core.c are smaller, but it is then redundant. We could use the create pointer for both input and output. The video plane configuration in frame format is the same as what is calculated in .queue_setup(), IIUC. So, we could just let the driver fill that one in. This would require then the videobuf2-core.c to parse struct v4l2_format to decide which union member we need, depending on the buffer type. Do we want this or shall drivers duplicate plane sizes in separate .queue_setup() parameters? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 15 Aug 2011, Guennadi Liakhovetski wrote: > On Mon, 15 Aug 2011, Hans Verkuil wrote: > > > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: > > > Hi Hans > > > > > > On Mon, 8 Aug 2011, Hans Verkuil wrote: [snip] > > > > but I've changed my mind: I think > > > > this should use a struct v4l2_format after all. > > While switching back, I have to change the struct vb2_ops::queue_setup() > operation to take a struct v4l2_create_buffers pointer. An earlier version > of this patch just added one more parameter to .queue_setup(), which is > easier - changes to videobuf2-core.c are smaller, but it is then > redundant. We could use the create pointer for both input and output. The > video plane configuration in frame format is the same as what is > calculated in .queue_setup(), IIUC. So, we could just let the driver fill > that one in. This would require then the videobuf2-core.c to parse struct > v4l2_format to decide which union member we need, depending on the buffer > type. Do we want this or shall drivers duplicate plane sizes in separate > .queue_setup() parameters? Let me explain my question a bit. The current .queue_setup() method is int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers, unsigned int *num_planes, unsigned int sizes[], void *alloc_ctxs[]); To support multiple-size buffers we also have to pass a pointer to struct v4l2_create_buffers to this function now. We can either do it like this: int (*queue_setup)(struct vb2_queue *q, struct v4l2_create_buffers *create, unsigned int *num_buffers, unsigned int *num_planes, unsigned int sizes[], void *alloc_ctxs[]); and let all drivers fill in respective fields in *create, e.g., either do create->format.fmt.pix_mp.plane_fmt[i].sizeimage = ...; create->format.fmt.pix_mp.num_planes = ...; and also duplicate it in method parameters *num_planes = create->format.fmt.pix_mp.num_planes; sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; or with create->format.fmt.pix.sizeimage = ...; for single-plane. Alternatively we make the prototype int (*queue_setup)(struct vb2_queue *q, struct v4l2_create_buffers *create, unsigned int *num_buffers, void *alloc_ctxs[]); then drivers only fill in *create, and the videobuf2-core will have to check create->format.type to decide, which of create->format.fmt.* is relevant and extract plane sizes from there. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Tue, Aug 16, 2011 at 06:13, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Mon, 15 Aug 2011, Guennadi Liakhovetski wrote: > >> On Mon, 15 Aug 2011, Hans Verkuil wrote: >> >> > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: >> > > Hi Hans >> > > >> > > On Mon, 8 Aug 2011, Hans Verkuil wrote: > > [snip] > >> > > > but I've changed my mind: I think >> > > > this should use a struct v4l2_format after all. >> >> While switching back, I have to change the struct vb2_ops::queue_setup() >> operation to take a struct v4l2_create_buffers pointer. An earlier version >> of this patch just added one more parameter to .queue_setup(), which is >> easier - changes to videobuf2-core.c are smaller, but it is then >> redundant. We could use the create pointer for both input and output. The >> video plane configuration in frame format is the same as what is >> calculated in .queue_setup(), IIUC. So, we could just let the driver fill >> that one in. This would require then the videobuf2-core.c to parse struct >> v4l2_format to decide which union member we need, depending on the buffer >> type. Do we want this or shall drivers duplicate plane sizes in separate >> .queue_setup() parameters? > > Let me explain my question a bit. The current .queue_setup() method is > > int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers, > unsigned int *num_planes, unsigned int sizes[], > void *alloc_ctxs[]); > > To support multiple-size buffers we also have to pass a pointer to struct > v4l2_create_buffers to this function now. We can either do it like this: > > int (*queue_setup)(struct vb2_queue *q, > struct v4l2_create_buffers *create, > unsigned int *num_buffers, > unsigned int *num_planes, unsigned int sizes[], > void *alloc_ctxs[]); > > and let all drivers fill in respective fields in *create, e.g., either do > > create->format.fmt.pix_mp.plane_fmt[i].sizeimage = ...; > create->format.fmt.pix_mp.num_planes = ...; > > and also duplicate it in method parameters > > *num_planes = create->format.fmt.pix_mp.num_planes; > sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; > > or with > > create->format.fmt.pix.sizeimage = ...; > > for single-plane. Alternatively we make the prototype > > int (*queue_setup)(struct vb2_queue *q, > struct v4l2_create_buffers *create, > unsigned int *num_buffers, > void *alloc_ctxs[]); > > then drivers only fill in *create, and the videobuf2-core will have to > check create->format.type to decide, which of create->format.fmt.* is > relevant and extract plane sizes from there. Could we try exploring an alternative idea? The queue_setup callback was added to decouple formats from vb2 (and add some asynchronousness). But now we are doing the opposite, adding format awareness to vb2. Does vb2 really need to know about formats? I really believe it doesn't. It only needs sizes and counts. Also, we are actually complicating things I think. The proposal, IIUC, would look like this: driver_queue_setup(..., create, num_buffers, [num_planes], ...) { if (create != NULL && create->format != NULL) { /* use create->fmt to fill sizes */ } else if (create != NULL) { /* this assumes we have both format or sizes */ /* use create->sizes to fill sizes */ } else { /* use currently selected format to fill sizes */ } } driver_s_fmt(format) { /* ... */ driver_fill_format(&create->fmt); /* ... */ } driver_create_bufs(create) { vb2_create_bufs(create); } vb2_create_bufs(create) { driver_queue_setup(..., create, ...); vb2_fill_format(&create->fmt); /* note different from driver_fill_format(), but both needed */ } vb2_reqbufs(reqbufs) { driver_queue_setup(..., NULL, ...); } The queue_setup not only becomes unnecessarily complicated, but I'm starting to question the convenience of it. And we are teaching vb2 how to interpret format structs, even though vb2 only needs sizes, and even though the driver has to do it anyway and knows better how. As for the idea to fill fmt in vb2, even if vb2 was to do it in create_bufs, some code to parse and fill the format fields would need to be in the driver anyway, because it still has to support s_fmt and friends. So adding that code to vb2 would duplicate it, and if the driver wanted to be non-standard in a way it filled the format fields, we'd not be allowing that. My suggestion would be to remove queue_setup callback and instead modify vb2_reqbufs and vb2_create_bufs to accept sizes and number of buffers. I think it should simplify things both for drivers and vb2, would keep vb2 format-unaware and save us some round trips between vb2 and driver: driver_create_bufs(...) /* optional */ { /* use create->fmt (or sizes) */ ret = vb2_create_bufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs); fill_format(&create->fmt) /* because s_fmt has to do it anyway, so have a common function for that */ return ret; } driver_reqbufs(...) { /* use current format */ return vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs); } And the call to both could easily converge into one in vb2, as the only difference is that vb2_reqbufs would need to free first, if any allocated buffers were present: vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs) { if (buffers_allocated(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs)) { free_buffers(...); } return vb2_create_bufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs); } If the driver didn't want create_bufs, it'd just not implement it. What do you think?
On Sat, 6 Aug 2011, Sakari Ailus wrote: > 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 <g.liakhovetski@gmx.de> > > --- > > Hi Guennadi, [snip] > > + <para>When the I/O method is not supported the ioctl > > +returns an &EINVAL;.</para> > > + > > + <table pgwide="1" frame="none" id="v4l2-create-buffers"> > > + <title>struct <structname>v4l2_create_buffers</structname></title> > > + <tgroup cols="3"> > > + &cs-str; > > + <tbody valign="top"> > > + <row> > > + <entry>__u32</entry> > > + <entry><structfield>index</structfield></entry> > > + <entry>The starting buffer index, returned by the driver.</entry> > > + </row> > > + <row> > > + <entry>__u32</entry> > > + <entry><structfield>count</structfield></entry> > > + <entry>The number of buffers requested or granted.</entry> > > + </row> > > + <row> > > + <entry>__u32</entry> > > + <entry><structfield>type</structfield></entry> > > + <entry>V4L2 buffer type: one of <constant>V4L2_BUF_TYPE_*</constant> > > +values.</entry> > > &v4l2-buf-type; > > here? No idea and I don't care all that much, tbh. I certainly copy-pasted those constructs from other documents, and yes, they are inconsistent across v4l: some use my version, some yours, others <xref linkend=...>. I'm happy with either or none of those. Just tell me _something_, that's approapriate. > > > + </row> > > + <row> > > + <entry>__u32</entry> > > + <entry><structfield>memory</structfield></entry> > > + <entry>Applications set this field to > > +<constant>V4L2_MEMORY_MMAP</constant> or > > +<constant>V4L2_MEMORY_USERPTR</constant>.</entry> > > &v4l2-memory; [snip] > > + <para>Applications can optionally call the > > +<constant>VIDIOC_PREPARE_BUF</constant> ioctl to pass ownership of the buffer > > +to the driver before actually enqueuing it, using the > > +<constant>VIDIOC_QBUF</constant> ioctl, and to prepare it for future I/O. > > s/<constant>VIDIOC_QBUF</constant>/&VIDIOC_QBUF;/ *shrug* > > > +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 > > +<constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant> and > > +<constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant> flags to skip the respective > > +step.</para> > > + > > + <para>The <structname>v4l2_buffer</structname> structure is > > s/<structname>v4l2_buffer</structname>/&v4l2-buffer;/ "structname" seems to be more precise, but well... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 16 Aug 2011, Pawel Osciak wrote: > Hi Guennadi, > > On Tue, Aug 16, 2011 at 06:13, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > On Mon, 15 Aug 2011, Guennadi Liakhovetski wrote: > > > >> On Mon, 15 Aug 2011, Hans Verkuil wrote: > >> > >> > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: > >> > > Hi Hans > >> > > > >> > > On Mon, 8 Aug 2011, Hans Verkuil wrote: > > > > [snip] > > > >> > > > but I've changed my mind: I think > >> > > > this should use a struct v4l2_format after all. > >> > >> While switching back, I have to change the struct vb2_ops::queue_setup() > >> operation to take a struct v4l2_create_buffers pointer. An earlier version > >> of this patch just added one more parameter to .queue_setup(), which is > >> easier - changes to videobuf2-core.c are smaller, but it is then > >> redundant. We could use the create pointer for both input and output. The > >> video plane configuration in frame format is the same as what is > >> calculated in .queue_setup(), IIUC. So, we could just let the driver fill > >> that one in. This would require then the videobuf2-core.c to parse struct > >> v4l2_format to decide which union member we need, depending on the buffer > >> type. Do we want this or shall drivers duplicate plane sizes in separate > >> .queue_setup() parameters? > > > > Let me explain my question a bit. The current .queue_setup() method is > > > > int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers, > > unsigned int *num_planes, unsigned int sizes[], > > void *alloc_ctxs[]); > > > > To support multiple-size buffers we also have to pass a pointer to struct > > v4l2_create_buffers to this function now. We can either do it like this: > > > > int (*queue_setup)(struct vb2_queue *q, > > struct v4l2_create_buffers *create, > > unsigned int *num_buffers, > > unsigned int *num_planes, unsigned int sizes[], > > void *alloc_ctxs[]); > > > > and let all drivers fill in respective fields in *create, e.g., either do > > > > create->format.fmt.pix_mp.plane_fmt[i].sizeimage = ...; > > create->format.fmt.pix_mp.num_planes = ...; > > > > and also duplicate it in method parameters > > > > *num_planes = create->format.fmt.pix_mp.num_planes; > > sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; > > > > or with > > > > create->format.fmt.pix.sizeimage = ...; > > > > for single-plane. Alternatively we make the prototype > > > > int (*queue_setup)(struct vb2_queue *q, > > struct v4l2_create_buffers *create, > > unsigned int *num_buffers, > > void *alloc_ctxs[]); > > > > then drivers only fill in *create, and the videobuf2-core will have to > > check create->format.type to decide, which of create->format.fmt.* is > > relevant and extract plane sizes from there. > > > Could we try exploring an alternative idea? > The queue_setup callback was added to decouple formats from vb2 (and > add some asynchronousness). But now we are doing the opposite, adding > format awareness to vb2. Does vb2 really need to know about formats? I > really believe it doesn't. It only needs sizes and counts. This kind of objection was expected:-) However, I think, you're a bit exaggerating. VB2 does not have to _fill_ the format. All frame-format fields like fourcc code, width, height, colorspace are only input from the user. If the user didn't fill them in, they should not be used. The only thing, that vb2 will have to learn about formats is to find the location of (plane-)buffer sizes in struct v4l2_format, for which it will have to interpret the .type value. I.e., we just have to add this: static int vb2_parse_planes(const struct v4l2_create_buffers *create, unsigned int *num_planes, unsigned int *plane_sizes) { int i; switch (create->format.type) { case V4L2_BUF_TYPE_VIDEO_CAPTURE: case V4L2_BUF_TYPE_VIDEO_OUTPUT: *num_planes = 1; plane_sizes[0] = create->format.fmt.pix.sizeimage; return 0; case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: *num_planes = create->format.fmt.pix_mp.num_planes; for (i = 0; i < *num_planes; i++) plane_sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; return 0; default: return -EINVAL; } } Can you live with this or you still think it's too much format-knowledge for vb2? Or am I missing something, why we need more knowledge? > Also, we > are actually complicating things I think. The proposal, IIUC, would > look like this: > > driver_queue_setup(..., create, num_buffers, [num_planes], ...) > { > if (create != NULL && create->format != NULL) { > /* use create->fmt to fill sizes */ > } else if (create != NULL) { /* this assumes we have both format or sizes */ > /* use create->sizes to fill sizes */ > } else { > /* use currently selected format to fill sizes */ > } > } > > driver_s_fmt(format) > { > /* ... */ > driver_fill_format(&create->fmt); > /* ... */ > } > > driver_create_bufs(create) > { > vb2_create_bufs(create); > } > > vb2_create_bufs(create) > { > driver_queue_setup(..., create, ...); > vb2_fill_format(&create->fmt); /* note different from > driver_fill_format(), but both needed */ > } > > vb2_reqbufs(reqbufs) > { > driver_queue_setup(..., NULL, ...); > } > > The queue_setup not only becomes unnecessarily complicated, but I'm > starting to question the convenience of it. And we are teaching vb2 > how to interpret format structs, even though vb2 only needs sizes, and > even though the driver has to do it anyway and knows better how. > > As for the idea to fill fmt in vb2, even if vb2 was to do it in > create_bufs, some code to parse and fill the format fields would need > to be in the driver anyway, because it still has to support s_fmt and > friends. So adding that code to vb2 would duplicate it, and if the > driver wanted to be non-standard in a way it filled the format fields, > we'd not be allowing that. > > My suggestion would be to remove queue_setup callback and instead > modify vb2_reqbufs and vb2_create_bufs to accept sizes and number of > buffers. I think it should simplify things both for drivers and vb2, > would keep vb2 format-unaware and save us some round trips between vb2 > and driver: Right, I see what you mean. Well, this seems doable. Do we want this? It does seem to simplify things a bit by removing .queue_setup()... Opinions? > driver_create_bufs(...) /* optional */ > { > /* use create->fmt (or sizes) */ > ret = vb2_create_bufs(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs); > fill_format(&create->fmt) /* because s_fmt has to do it anyway, so > have a common function for that */ > return ret; > } > > driver_reqbufs(...) > { > /* use current format */ > return vb2_reqbufs(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs); > } > > And the call to both could easily converge into one in vb2, as the > only difference is that vb2_reqbufs would need to free first, if any > allocated buffers were present: > > vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs) > { > if (buffers_allocated(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs)) { > free_buffers(...); > } > > return vb2_create_bufs(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs); > } > > If the driver didn't want create_bufs, it'd just not implement it. > What do you think? > > -- > Best regards, > Pawel Osciak Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 17 August 2011 11:11:01 Guennadi Liakhovetski wrote: > On Tue, 16 Aug 2011, Pawel Osciak wrote: > > On Tue, Aug 16, 2011 at 06:13, Guennadi Liakhovetski wrote: > > > On Mon, 15 Aug 2011, Guennadi Liakhovetski wrote: > > >> On Mon, 15 Aug 2011, Hans Verkuil wrote: > > >> > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: > > >> > > On Mon, 8 Aug 2011, Hans Verkuil wrote: > > > [snip] > > > > > >> > > > but I've changed my mind: I think > > >> > > > this should use a struct v4l2_format after all. > > >> > > >> While switching back, I have to change the struct > > >> vb2_ops::queue_setup() operation to take a struct v4l2_create_buffers > > >> pointer. An earlier version of this patch just added one more > > >> parameter to .queue_setup(), which is easier - changes to > > >> videobuf2-core.c are smaller, but it is then redundant. We could use > > >> the create pointer for both input and output. The video plane > > >> configuration in frame format is the same as what is calculated in > > >> .queue_setup(), IIUC. So, we could just let the driver fill that one > > >> in. This would require then the videobuf2-core.c to parse struct > > >> v4l2_format to decide which union member we need, depending on the > > >> buffer type. Do we want this or shall drivers duplicate plane sizes > > >> in separate .queue_setup() parameters? > > > > > > Let me explain my question a bit. The current .queue_setup() method is > > > > > > int (*queue_setup)(struct vb2_queue *q, unsigned int > > > *num_buffers, > > > > > > unsigned int *num_planes, unsigned int > > > sizes[], void *alloc_ctxs[]); > > > > > > To support multiple-size buffers we also have to pass a pointer to > > > struct > > > > > > v4l2_create_buffers to this function now. We can either do it like this: > > > int (*queue_setup)(struct vb2_queue *q, > > > > > > struct v4l2_create_buffers *create, > > > unsigned int *num_buffers, > > > unsigned int *num_planes, unsigned int > > > sizes[], void *alloc_ctxs[]); > > > > > > and let all drivers fill in respective fields in *create, e.g., either > > > do > > > > > > create->format.fmt.pix_mp.plane_fmt[i].sizeimage = ...; > > > create->format.fmt.pix_mp.num_planes = ...; > > > > > > and also duplicate it in method parameters > > > > > > *num_planes = create->format.fmt.pix_mp.num_planes; > > > sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; > > > > > > or with > > > > > > create->format.fmt.pix.sizeimage = ...; > > > > > > for single-plane. Alternatively we make the prototype > > > > > > int (*queue_setup)(struct vb2_queue *q, > > > > > > struct v4l2_create_buffers *create, > > > unsigned int *num_buffers, > > > void *alloc_ctxs[]); > > > > > > then drivers only fill in *create, and the videobuf2-core will have to > > > check create->format.type to decide, which of create->format.fmt.* is > > > relevant and extract plane sizes from there. > > > > Could we try exploring an alternative idea? > > The queue_setup callback was added to decouple formats from vb2 (and > > add some asynchronousness). But now we are doing the opposite, adding > > format awareness to vb2. Does vb2 really need to know about formats? I > > really believe it doesn't. It only needs sizes and counts. > > This kind of objection was expected:-) However, I think, you're a bit > exaggerating. VB2 does not have to _fill_ the format. All frame-format > fields like fourcc code, width, height, colorspace are only input from the > user. If the user didn't fill them in, they should not be used. The only > thing, that vb2 will have to learn about formats is to find the location > of (plane-)buffer sizes in struct v4l2_format, for which it will have to > interpret the .type value. I.e., we just have to add this: > > static int vb2_parse_planes(const struct v4l2_create_buffers *create, > unsigned int *num_planes, unsigned int *plane_sizes) > { > int i; > > switch (create->format.type) { > case V4L2_BUF_TYPE_VIDEO_CAPTURE: > case V4L2_BUF_TYPE_VIDEO_OUTPUT: > *num_planes = 1; > plane_sizes[0] = create->format.fmt.pix.sizeimage; > return 0; > case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > *num_planes = create->format.fmt.pix_mp.num_planes; > for (i = 0; i < *num_planes; i++) > plane_sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; > return 0; > default: > return -EINVAL; > } > } > > Can you live with this or you still think it's too much format-knowledge > for vb2? Or am I missing something, why we need more knowledge? > > > Also, we > > are actually complicating things I think. The proposal, IIUC, would > > look like this: > > > > driver_queue_setup(..., create, num_buffers, [num_planes], ...) > > { > > > > if (create != NULL && create->format != NULL) { > > > > /* use create->fmt to fill sizes */ > > > > } else if (create != NULL) { /* this assumes we have both format or > > sizes */ > > > > /* use create->sizes to fill sizes */ > > > > } else { > > > > /* use currently selected format to fill sizes */ > > > > } > > > > } > > > > driver_s_fmt(format) > > { > > > > /* ... */ > > driver_fill_format(&create->fmt); > > /* ... */ > > > > } > > > > driver_create_bufs(create) > > { > > > > vb2_create_bufs(create); > > > > } > > > > vb2_create_bufs(create) > > { > > > > driver_queue_setup(..., create, ...); > > vb2_fill_format(&create->fmt); /* note different from > > > > driver_fill_format(), but both needed */ > > } > > > > vb2_reqbufs(reqbufs) > > { > > > > driver_queue_setup(..., NULL, ...); > > > > } > > > > The queue_setup not only becomes unnecessarily complicated, but I'm > > starting to question the convenience of it. And we are teaching vb2 > > how to interpret format structs, even though vb2 only needs sizes, and > > even though the driver has to do it anyway and knows better how. > > > > As for the idea to fill fmt in vb2, even if vb2 was to do it in > > create_bufs, some code to parse and fill the format fields would need > > to be in the driver anyway, because it still has to support s_fmt and > > friends. So adding that code to vb2 would duplicate it, and if the > > driver wanted to be non-standard in a way it filled the format fields, > > we'd not be allowing that. > > > > My suggestion would be to remove queue_setup callback and instead > > modify vb2_reqbufs and vb2_create_bufs to accept sizes and number of > > buffers. I think it should simplify things both for drivers and vb2, > > would keep vb2 format-unaware and save us some round trips between vb2 > > and driver: > Right, I see what you mean. Well, this seems doable. Do we want this? It > does seem to simplify things a bit by removing .queue_setup()... Opinions? I might not have thought about all the possible issues this could bring, but I like it. > > driver_create_bufs(...) /* optional */ > > { > > > > /* use create->fmt (or sizes) */ > > ret = vb2_create_bufs(num_buffers, num_planes, buf_sizes, > > > > plane_sizes, alloc_ctxs); > > > > fill_format(&create->fmt) /* because s_fmt has to do it anyway, so > > > > have a common function for that */ > > > > return ret; > > > > } > > > > driver_reqbufs(...) > > { > > > > /* use current format */ > > return vb2_reqbufs(num_buffers, num_planes, buf_sizes, > > > > plane_sizes, alloc_ctxs); > > } > > > > And the call to both could easily converge into one in vb2, as the > > only difference is that vb2_reqbufs would need to free first, if any > > allocated buffers were present: > > > > vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs) > > { > > > > if (buffers_allocated(num_buffers, num_planes, buf_sizes, > > > > plane_sizes, alloc_ctxs)) { > > > > free_buffers(...); > > > > } > > > > return vb2_create_bufs(num_buffers, num_planes, buf_sizes, > > > > plane_sizes, alloc_ctxs); > > } > > > > If the driver didn't want create_bufs, it'd just not implement it. > > What do you think?
On Wed, Aug 17, 2011 at 10:41:51AM +0200, Guennadi Liakhovetski wrote: > On Sat, 6 Aug 2011, Sakari Ailus wrote: > > > 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 <g.liakhovetski@gmx.de> > > > --- > > > > Hi Guennadi, > > [snip] > > > > + <para>When the I/O method is not supported the ioctl > > > +returns an &EINVAL;.</para> > > > + > > > + <table pgwide="1" frame="none" id="v4l2-create-buffers"> > > > + <title>struct <structname>v4l2_create_buffers</structname></title> > > > + <tgroup cols="3"> > > > + &cs-str; > > > + <tbody valign="top"> > > > + <row> > > > + <entry>__u32</entry> > > > + <entry><structfield>index</structfield></entry> > > > + <entry>The starting buffer index, returned by the driver.</entry> > > > + </row> > > > + <row> > > > + <entry>__u32</entry> > > > + <entry><structfield>count</structfield></entry> > > > + <entry>The number of buffers requested or granted.</entry> > > > + </row> > > > + <row> > > > + <entry>__u32</entry> > > > + <entry><structfield>type</structfield></entry> > > > + <entry>V4L2 buffer type: one of <constant>V4L2_BUF_TYPE_*</constant> > > > +values.</entry> > > > > &v4l2-buf-type; > > > > here? > > No idea and I don't care all that much, tbh. I certainly copy-pasted those > constructs from other documents, and yes, they are inconsistent across > v4l: some use my version, some yours, others <xref linkend=...>. I'm happy > with either or none of those. Just tell me _something_, that's > approapriate. Links are being used in other parts of V4L2 documentation. I think PREPARE_BUF documentation should use them, too, rather than duplicating parts of definitions. > > > > > + </row> > > > + <row> > > > + <entry>__u32</entry> > > > + <entry><structfield>memory</structfield></entry> > > > + <entry>Applications set this field to > > > +<constant>V4L2_MEMORY_MMAP</constant> or > > > +<constant>V4L2_MEMORY_USERPTR</constant>.</entry> > > > > &v4l2-memory; > > [snip] > > > > + <para>Applications can optionally call the > > > +<constant>VIDIOC_PREPARE_BUF</constant> ioctl to pass ownership of the buffer > > > +to the driver before actually enqueuing it, using the > > > +<constant>VIDIOC_QBUF</constant> ioctl, and to prepare it for future I/O. > > > > s/<constant>VIDIOC_QBUF</constant>/&VIDIOC_QBUF;/ > > *shrug* > > > > > +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 > > > +<constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant> and > > > +<constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant> flags to skip the respective > > > +step.</para> > > > + > > > + <para>The <structname>v4l2_buffer</structname> structure is > > > > s/<structname>v4l2_buffer</structname>/&v4l2-buffer;/ > > "structname" seems to be more precise, but well... It's precise but precision isn't everything: giving the user a link to the definition of e.g. v4l2_buffer is more useful than forcing him or her to look for it elsewhere in the documentation. These are small issues but the usability of the documentation counts a lot. Regards,
Hello, On Monday, August 15, 2011 3:46 PM Guennadi Liakhovetski wrote: > On Mon, 15 Aug 2011, Hans Verkuil wrote: > > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: > > > On Mon, 8 Aug 2011, Hans Verkuil wrote: > > > > On Friday, August 05, 2011 09:47:13 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 <g.liakhovetski@gmx.de> > > > > > --- > > > > > > > > > > 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 > > > > > > > > > > > > > <snip> > > > > > > > > > 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 > > > > > > > > > > /* > > > > > * 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 know you are going to hate me for this, > > > > > > hm, I'll consider this possibility;-) > > > > > > > but I've changed my mind: I think > > > > this should use a struct v4l2_format after all. > > While switching back, I have to change the struct vb2_ops::queue_setup() > operation to take a struct v4l2_create_buffers pointer. An earlier version > of this patch just added one more parameter to .queue_setup(), which is > easier - changes to videobuf2-core.c are smaller, but it is then > redundant. We could use the create pointer for both input and output. The > video plane configuration in frame format is the same as what is > calculated in .queue_setup(), IIUC. So, we could just let the driver fill > that one in. This would require then the videobuf2-core.c to parse struct > v4l2_format to decide which union member we need, depending on the buffer > type. Do we want this or shall drivers duplicate plane sizes in separate > .queue_setup() parameters? IMHO if possible we should have only one callback for the driver. Please notice that the driver should be also allowed to increase (or decrease) the number of buffers for particular format/fourcc. Best regards
On Wed, Aug 17, 2011 at 06:22, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > On Monday, August 15, 2011 3:46 PM Guennadi Liakhovetski wrote: >> While switching back, I have to change the struct vb2_ops::queue_setup() >> operation to take a struct v4l2_create_buffers pointer. An earlier version >> of this patch just added one more parameter to .queue_setup(), which is >> easier - changes to videobuf2-core.c are smaller, but it is then >> redundant. We could use the create pointer for both input and output. The >> video plane configuration in frame format is the same as what is >> calculated in .queue_setup(), IIUC. So, we could just let the driver fill >> that one in. This would require then the videobuf2-core.c to parse struct >> v4l2_format to decide which union member we need, depending on the buffer >> type. Do we want this or shall drivers duplicate plane sizes in separate >> .queue_setup() parameters? > > IMHO if possible we should have only one callback for the driver. Please > notice that the driver should be also allowed to increase (or decrease) the > number of buffers for particular format/fourcc. > Or remove queue_setup altogether (please see my example above). What do you think Marek?
Hi On Wed, Aug 17, 2011 at 02:11, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Tue, 16 Aug 2011, Pawel Osciak wrote: > >> Hi Guennadi, >> >> On Tue, Aug 16, 2011 at 06:13, Guennadi Liakhovetski >> <g.liakhovetski@gmx.de> wrote: >> > On Mon, 15 Aug 2011, Guennadi Liakhovetski wrote: >> > >> >> On Mon, 15 Aug 2011, Hans Verkuil wrote: >> >> >> >> > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: >> >> > > Hi Hans >> >> > > >> >> > > On Mon, 8 Aug 2011, Hans Verkuil wrote: >> > >> > [snip] >> > >> >> > > > but I've changed my mind: I think >> >> > > > this should use a struct v4l2_format after all. >> >> >> >> While switching back, I have to change the struct vb2_ops::queue_setup() >> >> operation to take a struct v4l2_create_buffers pointer. An earlier version >> >> of this patch just added one more parameter to .queue_setup(), which is >> >> easier - changes to videobuf2-core.c are smaller, but it is then >> >> redundant. We could use the create pointer for both input and output. The >> >> video plane configuration in frame format is the same as what is >> >> calculated in .queue_setup(), IIUC. So, we could just let the driver fill >> >> that one in. This would require then the videobuf2-core.c to parse struct >> >> v4l2_format to decide which union member we need, depending on the buffer >> >> type. Do we want this or shall drivers duplicate plane sizes in separate >> >> .queue_setup() parameters? >> > >> > Let me explain my question a bit. The current .queue_setup() method is >> > >> > int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers, >> > unsigned int *num_planes, unsigned int sizes[], >> > void *alloc_ctxs[]); >> > >> > To support multiple-size buffers we also have to pass a pointer to struct >> > v4l2_create_buffers to this function now. We can either do it like this: >> > >> > int (*queue_setup)(struct vb2_queue *q, >> > struct v4l2_create_buffers *create, >> > unsigned int *num_buffers, >> > unsigned int *num_planes, unsigned int sizes[], >> > void *alloc_ctxs[]); >> > >> > and let all drivers fill in respective fields in *create, e.g., either do >> > >> > create->format.fmt.pix_mp.plane_fmt[i].sizeimage = ...; >> > create->format.fmt.pix_mp.num_planes = ...; >> > >> > and also duplicate it in method parameters >> > >> > *num_planes = create->format.fmt.pix_mp.num_planes; >> > sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; >> > >> > or with >> > >> > create->format.fmt.pix.sizeimage = ...; >> > >> > for single-plane. Alternatively we make the prototype >> > >> > int (*queue_setup)(struct vb2_queue *q, >> > struct v4l2_create_buffers *create, >> > unsigned int *num_buffers, >> > void *alloc_ctxs[]); >> > >> > then drivers only fill in *create, and the videobuf2-core will have to >> > check create->format.type to decide, which of create->format.fmt.* is >> > relevant and extract plane sizes from there. >> >> >> Could we try exploring an alternative idea? >> The queue_setup callback was added to decouple formats from vb2 (and >> add some asynchronousness). But now we are doing the opposite, adding >> format awareness to vb2. Does vb2 really need to know about formats? I >> really believe it doesn't. It only needs sizes and counts. > > This kind of objection was expected:-) However, I think, you're a bit > exaggerating. VB2 does not have to _fill_ the format. All frame-format > fields like fourcc code, width, height, colorspace are only input from the > user. If the user didn't fill them in, they should not be used. The only > thing, that vb2 will have to learn about formats is to find the location > of (plane-)buffer sizes in struct v4l2_format, for which it will have to > interpret the .type value. I.e., we just have to add this: > > static int vb2_parse_planes(const struct v4l2_create_buffers *create, > unsigned int *num_planes, unsigned int *plane_sizes) > { > int i; > > switch (create->format.type) { > case V4L2_BUF_TYPE_VIDEO_CAPTURE: > case V4L2_BUF_TYPE_VIDEO_OUTPUT: > *num_planes = 1; > plane_sizes[0] = create->format.fmt.pix.sizeimage; > return 0; > case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: > *num_planes = create->format.fmt.pix_mp.num_planes; > for (i = 0; i < *num_planes; i++) > plane_sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; > return 0; > default: > return -EINVAL; > } > } > > Can you live with this or you still think it's too much format-knowledge > for vb2? Or am I missing something, why we need more knowledge? > True, I exaggerated a bit there. You are right with the above (I was also thinking of a case when the driver would want to adjust the format passed to CREATE_BUFS, but that seems to work as well). But I think my point still stands though: the drivers will have to parse the format struct themselves for both createbufs and s_fmt and friends, and know how to fill the sizeimage for both, but in the createbufs case instead of assigning them in fmt struct, drivers would be passing them to vb2 "outside" of it, as separate arguments. And the filling code would have to be in drivers anyway for s_fmt. I'm definitely for adding more things to vb2 to simplify drivers, but in this case I'm just not seeing much of a gain :) Now that I think of it, if we were to make vb2 get the format here, why not simply pass the format struct to vb2 with sizeimages filled in and forget about separate sizes arguments? I mean, it would be simpler for the driver to just fill in fmt struct with numbers of planes and sizeimages and pass only fmt to vb2 (and make vb2 use those for allocation), instead of passing fmt unfilled and sizes separately and make vb2 fill them in. Either way, I'm still strongly leaning towards removing the queue_setup callback and not passing format to vb2, for the reasons I've already stated above. >> Also, we >> are actually complicating things I think. The proposal, IIUC, would >> look like this: >> >> driver_queue_setup(..., create, num_buffers, [num_planes], ...) >> { >> if (create != NULL && create->format != NULL) { >> /* use create->fmt to fill sizes */ >> } else if (create != NULL) { /* this assumes we have both format or sizes */ >> /* use create->sizes to fill sizes */ >> } else { >> /* use currently selected format to fill sizes */ >> } >> } >> >> driver_s_fmt(format) >> { >> /* ... */ >> driver_fill_format(&create->fmt); >> /* ... */ >> } >> >> driver_create_bufs(create) >> { >> vb2_create_bufs(create); >> } >> >> vb2_create_bufs(create) >> { >> driver_queue_setup(..., create, ...); >> vb2_fill_format(&create->fmt); /* note different from >> driver_fill_format(), but both needed */ >> } >> >> vb2_reqbufs(reqbufs) >> { >> driver_queue_setup(..., NULL, ...); >> } >> >> The queue_setup not only becomes unnecessarily complicated, but I'm >> starting to question the convenience of it. And we are teaching vb2 >> how to interpret format structs, even though vb2 only needs sizes, and >> even though the driver has to do it anyway and knows better how. >> >> As for the idea to fill fmt in vb2, even if vb2 was to do it in >> create_bufs, some code to parse and fill the format fields would need >> to be in the driver anyway, because it still has to support s_fmt and >> friends. So adding that code to vb2 would duplicate it, and if the >> driver wanted to be non-standard in a way it filled the format fields, >> we'd not be allowing that. >> >> My suggestion would be to remove queue_setup callback and instead >> modify vb2_reqbufs and vb2_create_bufs to accept sizes and number of >> buffers. I think it should simplify things both for drivers and vb2, >> would keep vb2 format-unaware and save us some round trips between vb2 >> and driver: > > Right, I see what you mean. Well, this seems doable. Do we want this? It > does seem to simplify things a bit by removing .queue_setup()... Opinions? > Thanks :) The more I think of queue_setup, the more I think that although it was giving us that nice callback-based design, it's not worth keeping if we were to make it more complicated. And it really isn't such a big difference from directly calling vb2_reqbufs/vb2_createbufs, which we have to call anyway, and the flow and parameters are simpler. >> driver_create_bufs(...) /* optional */ >> { >> /* use create->fmt (or sizes) */ >> ret = vb2_create_bufs(num_buffers, num_planes, buf_sizes, >> plane_sizes, alloc_ctxs); >> fill_format(&create->fmt) /* because s_fmt has to do it anyway, so >> have a common function for that */ >> return ret; >> } >> >> driver_reqbufs(...) >> { >> /* use current format */ >> return vb2_reqbufs(num_buffers, num_planes, buf_sizes, >> plane_sizes, alloc_ctxs); >> } >> >> And the call to both could easily converge into one in vb2, as the >> only difference is that vb2_reqbufs would need to free first, if any >> allocated buffers were present: >> >> vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs) >> { >> if (buffers_allocated(num_buffers, num_planes, buf_sizes, >> plane_sizes, alloc_ctxs)) { >> free_buffers(...); >> } >> >> return vb2_create_bufs(num_buffers, num_planes, buf_sizes, >> plane_sizes, alloc_ctxs); >> } >> >> If the driver didn't want create_bufs, it'd just not implement it. >> What do you think? >> >> -- >> Best regards, >> Pawel Osciak > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ >
Hello, On Wednesday, August 17, 2011 4:58 PM Pawel Osciak wrote: > On Wed, Aug 17, 2011 at 06:22, Marek Szyprowski <m.szyprowski@samsung.com> > wrote: > > On Monday, August 15, 2011 3:46 PM Guennadi Liakhovetski wrote: > >> While switching back, I have to change the struct vb2_ops::queue_setup() > >> operation to take a struct v4l2_create_buffers pointer. An earlier version > >> of this patch just added one more parameter to .queue_setup(), which is > >> easier - changes to videobuf2-core.c are smaller, but it is then > >> redundant. We could use the create pointer for both input and output. The > >> video plane configuration in frame format is the same as what is > >> calculated in .queue_setup(), IIUC. So, we could just let the driver fill > >> that one in. This would require then the videobuf2-core.c to parse struct > >> v4l2_format to decide which union member we need, depending on the buffer > >> type. Do we want this or shall drivers duplicate plane sizes in separate > >> .queue_setup() parameters? > > > > IMHO if possible we should have only one callback for the driver. Please > > notice that the driver should be also allowed to increase (or decrease) the > > number of buffers for particular format/fourcc. > > > > Or remove queue_setup altogether (please see my example above). What > do you think Marek? I'm perfectly fine with replacing queue_setup callback with something else. Best regards
Sorry for starting this discussion and then disappearing. I've been very busy lately, so my apologies for that. On Tuesday, August 16, 2011 18:14:33 Pawel Osciak wrote: > Hi Guennadi, > > On Tue, Aug 16, 2011 at 06:13, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > On Mon, 15 Aug 2011, Guennadi Liakhovetski wrote: > > > >> On Mon, 15 Aug 2011, Hans Verkuil wrote: > >> > >> > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: > >> > > Hi Hans > >> > > > >> > > On Mon, 8 Aug 2011, Hans Verkuil wrote: > > > > [snip] > > > >> > > > but I've changed my mind: I think > >> > > > this should use a struct v4l2_format after all. > >> > >> While switching back, I have to change the struct vb2_ops::queue_setup() > >> operation to take a struct v4l2_create_buffers pointer. An earlier version > >> of this patch just added one more parameter to .queue_setup(), which is > >> easier - changes to videobuf2-core.c are smaller, but it is then > >> redundant. We could use the create pointer for both input and output. The > >> video plane configuration in frame format is the same as what is > >> calculated in .queue_setup(), IIUC. So, we could just let the driver fill > >> that one in. This would require then the videobuf2-core.c to parse struct > >> v4l2_format to decide which union member we need, depending on the buffer > >> type. Do we want this or shall drivers duplicate plane sizes in separate > >> .queue_setup() parameters? > > > > Let me explain my question a bit. The current .queue_setup() method is > > > > int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers, > > unsigned int *num_planes, unsigned int sizes[], > > void *alloc_ctxs[]); > > > > To support multiple-size buffers we also have to pass a pointer to struct > > v4l2_create_buffers to this function now. We can either do it like this: > > > > int (*queue_setup)(struct vb2_queue *q, > > struct v4l2_create_buffers *create, > > unsigned int *num_buffers, > > unsigned int *num_planes, unsigned int sizes[], > > void *alloc_ctxs[]); > > > > and let all drivers fill in respective fields in *create, e.g., either do > > > > create->format.fmt.pix_mp.plane_fmt[i].sizeimage = ...; > > create->format.fmt.pix_mp.num_planes = ...; > > > > and also duplicate it in method parameters > > > > *num_planes = create->format.fmt.pix_mp.num_planes; > > sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; > > > > or with > > > > create->format.fmt.pix.sizeimage = ...; > > > > for single-plane. Alternatively we make the prototype > > > > int (*queue_setup)(struct vb2_queue *q, > > struct v4l2_create_buffers *create, > > unsigned int *num_buffers, > > void *alloc_ctxs[]); > > > > then drivers only fill in *create, and the videobuf2-core will have to > > check create->format.type to decide, which of create->format.fmt.* is > > relevant and extract plane sizes from there. > > > Could we try exploring an alternative idea? > The queue_setup callback was added to decouple formats from vb2 (and > add some asynchronousness). But now we are doing the opposite, adding > format awareness to vb2. Does vb2 really need to know about formats? I > really believe it doesn't. It only needs sizes and counts. Also, we > are actually complicating things I think. The proposal, IIUC, would > look like this: > > driver_queue_setup(..., create, num_buffers, [num_planes], ...) > { > if (create != NULL && create->format != NULL) { > /* use create->fmt to fill sizes */ Right. > } else if (create != NULL) { /* this assumes we have both format or sizes */ > /* use create->sizes to fill sizes */ No, create->format should always be set. If the application can fill in the sizeimage field(s), then there is no need for create->sizes. > } else { > /* use currently selected format to fill sizes */ Right. > } > } > > driver_s_fmt(format) > { > /* ... */ > driver_fill_format(&create->fmt); > /* ... */ > } ??? > > driver_create_bufs(create) > { > vb2_create_bufs(create); > } > > vb2_create_bufs(create) > { > driver_queue_setup(..., create, ...); > vb2_fill_format(&create->fmt); /* note different from > driver_fill_format(), but both needed */ Huh? Why call vb2_fill_format? vb2 should have no knowledge whatsoever about formats. The driver needs that information in order to be able to allocate buffers correctly since that depends on the required format. But vb2 doesn't need that knowledge. > } > > vb2_reqbufs(reqbufs) > { > driver_queue_setup(..., NULL, ...); > } > > The queue_setup not only becomes unnecessarily complicated, but I'm > starting to question the convenience of it. And we are teaching vb2 > how to interpret format structs, even though vb2 only needs sizes, and > even though the driver has to do it anyway and knows better how. No, vb2 just needs to pass the format information from the user to the driver. There seems to be some misunderstanding here. The point of my original suggestion that create_bufs should use v4l2_format is that the driver needs the format information in order to decide how and where the buffers have to be allocated. Having the format available is the only reliable way to do that. This is already done for REQBUFS since the driver will use the current format to make these decisions. One way of simplifying queue_setup is actually to always supply the format. In the case of REQBUFS the driver might do something like this: driver_reqbufs(requestbuffers) { struct v4l2_format fmt; struct v4l2_create_buffers create; vb2_free_bufs(); // reqbufs should free any existing bufs if (requestbuffers->count == 0) return 0; driver_g_fmt(&fmt); // call the g_fmt ioctl op // fill in create vb2_create_bufs(create); } So vb2 just sees a call requesting to create so many buffers for a particular format, and it just hands that information over to the driver *without* parsing it. And the driver gets the request from vb2 to create X buffers for format F, and will figure out how to do that and returns the buffer/plane/allocator context information back to vb2. Regards, Hans > As for the idea to fill fmt in vb2, even if vb2 was to do it in > create_bufs, some code to parse and fill the format fields would need > to be in the driver anyway, because it still has to support s_fmt and > friends. So adding that code to vb2 would duplicate it, and if the > driver wanted to be non-standard in a way it filled the format fields, > we'd not be allowing that. > > My suggestion would be to remove queue_setup callback and instead > modify vb2_reqbufs and vb2_create_bufs to accept sizes and number of > buffers. I think it should simplify things both for drivers and vb2, > would keep vb2 format-unaware and save us some round trips between vb2 > and driver: > > driver_create_bufs(...) /* optional */ > { > /* use create->fmt (or sizes) */ > ret = vb2_create_bufs(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs); > fill_format(&create->fmt) /* because s_fmt has to do it anyway, so > have a common function for that */ > return ret; > } > > driver_reqbufs(...) > { > /* use current format */ > return vb2_reqbufs(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs); > } > > And the call to both could easily converge into one in vb2, as the > only difference is that vb2_reqbufs would need to free first, if any > allocated buffers were present: > > vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs) > { > if (buffers_allocated(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs)) { > free_buffers(...); > } > > return vb2_create_bufs(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs); > } > > If the driver didn't want create_bufs, it'd just not implement it. > What do you think? > > -- > Best regards, > Pawel Osciak > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans On Mon, 22 Aug 2011, Hans Verkuil wrote: > Sorry for starting this discussion and then disappearing. I've been very > busy lately, so my apologies for that. > > On Tuesday, August 16, 2011 18:14:33 Pawel Osciak wrote: > > Hi Guennadi, > > > > On Tue, Aug 16, 2011 at 06:13, Guennadi Liakhovetski > > <g.liakhovetski@gmx.de> wrote: > > > On Mon, 15 Aug 2011, Guennadi Liakhovetski wrote: > > > > > >> On Mon, 15 Aug 2011, Hans Verkuil wrote: > > >> > > >> > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: > > >> > > Hi Hans > > >> > > > > >> > > On Mon, 8 Aug 2011, Hans Verkuil wrote: > > > > > > [snip] > > > > > >> > > > but I've changed my mind: I think > > >> > > > this should use a struct v4l2_format after all. > > >> > > >> While switching back, I have to change the struct vb2_ops::queue_setup() > > >> operation to take a struct v4l2_create_buffers pointer. An earlier > version > > >> of this patch just added one more parameter to .queue_setup(), which is > > >> easier - changes to videobuf2-core.c are smaller, but it is then > > >> redundant. We could use the create pointer for both input and output. The > > >> video plane configuration in frame format is the same as what is > > >> calculated in .queue_setup(), IIUC. So, we could just let the driver fill > > >> that one in. This would require then the videobuf2-core.c to parse struct > > >> v4l2_format to decide which union member we need, depending on the buffer > > >> type. Do we want this or shall drivers duplicate plane sizes in separate > > >> .queue_setup() parameters? > > > > > > Let me explain my question a bit. The current .queue_setup() method is > > > > > > int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers, > > > unsigned int *num_planes, unsigned int sizes[], > > > void *alloc_ctxs[]); > > > > > > To support multiple-size buffers we also have to pass a pointer to struct > > > v4l2_create_buffers to this function now. We can either do it like this: > > > > > > int (*queue_setup)(struct vb2_queue *q, > > > struct v4l2_create_buffers *create, > > > unsigned int *num_buffers, > > > unsigned int *num_planes, unsigned int sizes[], > > > void *alloc_ctxs[]); > > > > > > and let all drivers fill in respective fields in *create, e.g., either do > > > > > > create->format.fmt.pix_mp.plane_fmt[i].sizeimage = ...; > > > create->format.fmt.pix_mp.num_planes = ...; > > > > > > and also duplicate it in method parameters > > > > > > *num_planes = create->format.fmt.pix_mp.num_planes; > > > sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; > > > > > > or with > > > > > > create->format.fmt.pix.sizeimage = ...; > > > > > > for single-plane. Alternatively we make the prototype > > > > > > int (*queue_setup)(struct vb2_queue *q, > > > struct v4l2_create_buffers *create, > > > unsigned int *num_buffers, > > > void *alloc_ctxs[]); > > > > > > then drivers only fill in *create, and the videobuf2-core will have to > > > check create->format.type to decide, which of create->format.fmt.* is > > > relevant and extract plane sizes from there. > > > > > > Could we try exploring an alternative idea? > > The queue_setup callback was added to decouple formats from vb2 (and > > add some asynchronousness). But now we are doing the opposite, adding > > format awareness to vb2. Does vb2 really need to know about formats? I > > really believe it doesn't. It only needs sizes and counts. Also, we > > are actually complicating things I think. The proposal, IIUC, would > > look like this: > > > > driver_queue_setup(..., create, num_buffers, [num_planes], ...) > > { > > if (create != NULL && create->format != NULL) { > > /* use create->fmt to fill sizes */ > > Right. > > > } else if (create != NULL) { /* this assumes we have both format or > sizes */ > > /* use create->sizes to fill sizes */ > > No, create->format should always be set. If the application can fill in the > sizeimage field(s), then there is no need for create->sizes. > > > } else { > > /* use currently selected format to fill sizes */ > > Right. > > > } > > } > > > > driver_s_fmt(format) > > { > > /* ... */ > > driver_fill_format(&create->fmt); > > /* ... */ > > } > > ??? > > > > > driver_create_bufs(create) > > { > > vb2_create_bufs(create); > > } > > > > vb2_create_bufs(create) > > { > > driver_queue_setup(..., create, ...); > > vb2_fill_format(&create->fmt); /* note different from > > driver_fill_format(), but both needed */ > > Huh? Why call vb2_fill_format? vb2 should have no knowledge whatsoever about > formats. The driver needs that information in order to be able to allocate > buffers correctly since that depends on the required format. But vb2 doesn't > need that knowledge. It would be good if you also could have a look at my reply to this Pawel's mail: http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/36905 and, specifically, at the vb2_parse_planes() function in it. That's my understanding of what would be needed, if we preserve .queue_setup() and use your last suggestion to include struct v4l2_format in struct v4l2_create_buffers. However, from your reply I couldn't understand your attitude to removing .queue_setup() altogether. Do you see any disadvantages in doing so? Is it serving any special role, that we are overseeing? Thanks Guennadi > > > } > > > > vb2_reqbufs(reqbufs) > > { > > driver_queue_setup(..., NULL, ...); > > } > > > > The queue_setup not only becomes unnecessarily complicated, but I'm > > starting to question the convenience of it. And we are teaching vb2 > > how to interpret format structs, even though vb2 only needs sizes, and > > even though the driver has to do it anyway and knows better how. > > No, vb2 just needs to pass the format information from the user to the > driver. > > There seems to be some misunderstanding here. > > The point of my original suggestion that create_bufs should use v4l2_format > is that the driver needs the format information in order to decide how and > where the buffers have to be allocated. Having the format available is the > only reliable way to do that. > > This is already done for REQBUFS since the driver will use the current format > to make these decisions. > > One way of simplifying queue_setup is actually to always supply the format. > In the case of REQBUFS the driver might do something like this: > > driver_reqbufs(requestbuffers) > { > struct v4l2_format fmt; > struct v4l2_create_buffers create; > > vb2_free_bufs(); // reqbufs should free any existing bufs > if (requestbuffers->count == 0) > return 0; > driver_g_fmt(&fmt); // call the g_fmt ioctl op > // fill in create > vb2_create_bufs(create); > } > > So vb2 just sees a call requesting to create so many buffers for a particular > format, and it just hands that information over to the driver *without* > parsing it. > > And the driver gets the request from vb2 to create X buffers for format F, and > will figure out how to do that and returns the buffer/plane/allocator context > information back to vb2. > > Regards, > > Hans > > > As for the idea to fill fmt in vb2, even if vb2 was to do it in > > create_bufs, some code to parse and fill the format fields would need > > to be in the driver anyway, because it still has to support s_fmt and > > friends. So adding that code to vb2 would duplicate it, and if the > > driver wanted to be non-standard in a way it filled the format fields, > > we'd not be allowing that. > > > > My suggestion would be to remove queue_setup callback and instead > > modify vb2_reqbufs and vb2_create_bufs to accept sizes and number of > > buffers. I think it should simplify things both for drivers and vb2, > > would keep vb2 format-unaware and save us some round trips between vb2 > > and driver: > > > > driver_create_bufs(...) /* optional */ > > { > > /* use create->fmt (or sizes) */ > > ret = vb2_create_bufs(num_buffers, num_planes, buf_sizes, > > plane_sizes, alloc_ctxs); > > fill_format(&create->fmt) /* because s_fmt has to do it anyway, so > > have a common function for that */ > > return ret; > > } > > > > driver_reqbufs(...) > > { > > /* use current format */ > > return vb2_reqbufs(num_buffers, num_planes, buf_sizes, > > plane_sizes, alloc_ctxs); > > } > > > > And the call to both could easily converge into one in vb2, as the > > only difference is that vb2_reqbufs would need to free first, if any > > allocated buffers were present: > > > > vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs) > > { > > if (buffers_allocated(num_buffers, num_planes, buf_sizes, > > plane_sizes, alloc_ctxs)) { > > free_buffers(...); > > } > > > > return vb2_create_bufs(num_buffers, num_planes, buf_sizes, > > plane_sizes, alloc_ctxs); > > } > > > > If the driver didn't want create_bufs, it'd just not implement it. > > What do you think? > > > > -- > > Best regards, > > Pawel Osciak > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-media" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: > Hi Hans > > On Mon, 22 Aug 2011, Hans Verkuil wrote: > > > Sorry for starting this discussion and then disappearing. I've been very > > busy lately, so my apologies for that. > > > > On Tuesday, August 16, 2011 18:14:33 Pawel Osciak wrote: > > > Hi Guennadi, > > > > > > On Tue, Aug 16, 2011 at 06:13, Guennadi Liakhovetski > > > <g.liakhovetski@gmx.de> wrote: > > > > On Mon, 15 Aug 2011, Guennadi Liakhovetski wrote: > > > > > > > >> On Mon, 15 Aug 2011, Hans Verkuil wrote: > > > >> > > > >> > On Monday, August 15, 2011 13:28:23 Guennadi Liakhovetski wrote: > > > >> > > Hi Hans > > > >> > > > > > >> > > On Mon, 8 Aug 2011, Hans Verkuil wrote: > > > > > > > > [snip] > > > > > > > >> > > > but I've changed my mind: I think > > > >> > > > this should use a struct v4l2_format after all. > > > >> > > > >> While switching back, I have to change the struct vb2_ops::queue_setup() > > > >> operation to take a struct v4l2_create_buffers pointer. An earlier > > version > > > >> of this patch just added one more parameter to .queue_setup(), which is > > > >> easier - changes to videobuf2-core.c are smaller, but it is then > > > >> redundant. We could use the create pointer for both input and output. The > > > >> video plane configuration in frame format is the same as what is > > > >> calculated in .queue_setup(), IIUC. So, we could just let the driver fill > > > >> that one in. This would require then the videobuf2-core.c to parse struct > > > >> v4l2_format to decide which union member we need, depending on the buffer > > > >> type. Do we want this or shall drivers duplicate plane sizes in separate > > > >> .queue_setup() parameters? > > > > > > > > Let me explain my question a bit. The current .queue_setup() method is > > > > > > > > int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers, > > > > unsigned int *num_planes, unsigned int sizes[], > > > > void *alloc_ctxs[]); > > > > > > > > To support multiple-size buffers we also have to pass a pointer to struct > > > > v4l2_create_buffers to this function now. We can either do it like this: > > > > > > > > int (*queue_setup)(struct vb2_queue *q, > > > > struct v4l2_create_buffers *create, > > > > unsigned int *num_buffers, > > > > unsigned int *num_planes, unsigned int sizes[], > > > > void *alloc_ctxs[]); > > > > > > > > and let all drivers fill in respective fields in *create, e.g., either do > > > > > > > > create->format.fmt.pix_mp.plane_fmt[i].sizeimage = ...; > > > > create->format.fmt.pix_mp.num_planes = ...; > > > > > > > > and also duplicate it in method parameters > > > > > > > > *num_planes = create->format.fmt.pix_mp.num_planes; > > > > sizes[i] = create->format.fmt.pix_mp.plane_fmt[i].sizeimage; > > > > > > > > or with > > > > > > > > create->format.fmt.pix.sizeimage = ...; > > > > > > > > for single-plane. Alternatively we make the prototype > > > > > > > > int (*queue_setup)(struct vb2_queue *q, > > > > struct v4l2_create_buffers *create, > > > > unsigned int *num_buffers, > > > > void *alloc_ctxs[]); > > > > > > > > then drivers only fill in *create, and the videobuf2-core will have to > > > > check create->format.type to decide, which of create->format.fmt.* is > > > > relevant and extract plane sizes from there. > > > > > > > > > Could we try exploring an alternative idea? > > > The queue_setup callback was added to decouple formats from vb2 (and > > > add some asynchronousness). But now we are doing the opposite, adding > > > format awareness to vb2. Does vb2 really need to know about formats? I > > > really believe it doesn't. It only needs sizes and counts. Also, we > > > are actually complicating things I think. The proposal, IIUC, would > > > look like this: > > > > > > driver_queue_setup(..., create, num_buffers, [num_planes], ...) > > > { > > > if (create != NULL && create->format != NULL) { > > > /* use create->fmt to fill sizes */ > > > > Right. > > > > > } else if (create != NULL) { /* this assumes we have both format or > > sizes */ > > > /* use create->sizes to fill sizes */ > > > > No, create->format should always be set. If the application can fill in the > > sizeimage field(s), then there is no need for create->sizes. > > > > > } else { > > > /* use currently selected format to fill sizes */ > > > > Right. > > > > > } > > > } > > > > > > driver_s_fmt(format) > > > { > > > /* ... */ > > > driver_fill_format(&create->fmt); > > > /* ... */ > > > } > > > > ??? > > > > > > > > driver_create_bufs(create) > > > { > > > vb2_create_bufs(create); > > > } > > > > > > vb2_create_bufs(create) > > > { > > > driver_queue_setup(..., create, ...); > > > vb2_fill_format(&create->fmt); /* note different from > > > driver_fill_format(), but both needed */ > > > > Huh? Why call vb2_fill_format? vb2 should have no knowledge whatsoever about > > formats. The driver needs that information in order to be able to allocate > > buffers correctly since that depends on the required format. But vb2 doesn't > > need that knowledge. > > It would be good if you also could have a look at my reply to this Pawel's > mail: > > http://article.gmane.org/gmane.linux.drivers.video-input- infrastructure/36905 > > and, specifically, at the vb2_parse_planes() function in it. That's my > understanding of what would be needed, if we preserve .queue_setup() and > use your last suggestion to include struct v4l2_format in struct > v4l2_create_buffers. vb2_parse_planes can be useful as a utility function that 'normal' drivers can call from the queue_setup. But vb2 should not parse the format directly, it should just pass it on to the driver through the queue_setup function. You also mention: "All frame-format fields like fourcc code, width, height, colorspace are only input from the user. If the user didn't fill them in, they should not be used." I disagree with that. The user should fill in a full format description, just as with S/TRY_FMT. That's the information that the driver will use to set up the buffers. It could have weird rules like: if the fourcc is this, and the size is less than that, then we can allocate in this memory bank. It is also consistent with REQBUFS: there too the driver uses a full format (i.e. the last set format). I would modify queue_setup to something like this: int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, unsigned int *num_buffers, unsigned int *num_planes, unsigned int sizes[], void *alloc_ctxs[]); Whether fmt is left to NULL in the reqbufs case, or whether the driver has to call g_fmt first before calling vb2 is something that could be decided by what is easiest to implement. Regards, Hans > However, from your reply I couldn't understand your attitude to removing > .queue_setup() altogether. Do you see any disadvantages in doing so? Is it > serving any special role, that we are overseeing? > > Thanks > Guennadi > > > > > > } > > > > > > vb2_reqbufs(reqbufs) > > > { > > > driver_queue_setup(..., NULL, ...); > > > } > > > > > > The queue_setup not only becomes unnecessarily complicated, but I'm > > > starting to question the convenience of it. And we are teaching vb2 > > > how to interpret format structs, even though vb2 only needs sizes, and > > > even though the driver has to do it anyway and knows better how. > > > > No, vb2 just needs to pass the format information from the user to the > > driver. > > > > There seems to be some misunderstanding here. > > > > The point of my original suggestion that create_bufs should use v4l2_format > > is that the driver needs the format information in order to decide how and > > where the buffers have to be allocated. Having the format available is the > > only reliable way to do that. > > > > This is already done for REQBUFS since the driver will use the current format > > to make these decisions. > > > > One way of simplifying queue_setup is actually to always supply the format. > > In the case of REQBUFS the driver might do something like this: > > > > driver_reqbufs(requestbuffers) > > { > > struct v4l2_format fmt; > > struct v4l2_create_buffers create; > > > > vb2_free_bufs(); // reqbufs should free any existing bufs > > if (requestbuffers->count == 0) > > return 0; > > driver_g_fmt(&fmt); // call the g_fmt ioctl op > > // fill in create > > vb2_create_bufs(create); > > } > > > > So vb2 just sees a call requesting to create so many buffers for a particular > > format, and it just hands that information over to the driver *without* > > parsing it. > > > > And the driver gets the request from vb2 to create X buffers for format F, and > > will figure out how to do that and returns the buffer/plane/allocator context > > information back to vb2. > > > > Regards, > > > > Hans > > > > > As for the idea to fill fmt in vb2, even if vb2 was to do it in > > > create_bufs, some code to parse and fill the format fields would need > > > to be in the driver anyway, because it still has to support s_fmt and > > > friends. So adding that code to vb2 would duplicate it, and if the > > > driver wanted to be non-standard in a way it filled the format fields, > > > we'd not be allowing that. > > > > > > My suggestion would be to remove queue_setup callback and instead > > > modify vb2_reqbufs and vb2_create_bufs to accept sizes and number of > > > buffers. I think it should simplify things both for drivers and vb2, > > > would keep vb2 format-unaware and save us some round trips between vb2 > > > and driver: > > > > > > driver_create_bufs(...) /* optional */ > > > { > > > /* use create->fmt (or sizes) */ > > > ret = vb2_create_bufs(num_buffers, num_planes, buf_sizes, > > > plane_sizes, alloc_ctxs); > > > fill_format(&create->fmt) /* because s_fmt has to do it anyway, so > > > have a common function for that */ > > > return ret; > > > } > > > > > > driver_reqbufs(...) > > > { > > > /* use current format */ > > > return vb2_reqbufs(num_buffers, num_planes, buf_sizes, > > > plane_sizes, alloc_ctxs); > > > } > > > > > > And the call to both could easily converge into one in vb2, as the > > > only difference is that vb2_reqbufs would need to free first, if any > > > allocated buffers were present: > > > > > > vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs) > > > { > > > if (buffers_allocated(num_buffers, num_planes, buf_sizes, > > > plane_sizes, alloc_ctxs)) { > > > free_buffers(...); > > > } > > > > > > return vb2_create_bufs(num_buffers, num_planes, buf_sizes, > > > plane_sizes, alloc_ctxs); > > > } > > > > > > If the driver didn't want create_bufs, it'd just not implement it. > > > What do you think? > > > > > > -- > > > Best regards, > > > Pawel Osciak > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-media" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
We discussed a bit more with Hans on IRC, and below is my attempt of a summary. Hans, please, correct me, if I misunderstood anything. Pawel, Sakari, Laurent: please, reply, whether you're ok with this. On Mon, 22 Aug 2011, Hans Verkuil wrote: > On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: [snip] > > It would be good if you also could have a look at my reply to this Pawel's > > mail: > > > > http://article.gmane.org/gmane.linux.drivers.video-input- > infrastructure/36905 > > > > and, specifically, at the vb2_parse_planes() function in it. That's my > > understanding of what would be needed, if we preserve .queue_setup() and > > use your last suggestion to include struct v4l2_format in struct > > v4l2_create_buffers. > > vb2_parse_planes can be useful as a utility function that 'normal' drivers can > call from the queue_setup. But vb2 should not parse the format directly, it > should just pass it on to the driver through the queue_setup function. > > You also mention: "All frame-format fields like fourcc code, width, height, > colorspace are only input from the user. If the user didn't fill them in, they > should not be used." > > I disagree with that. The user should fill in a full format description, just > as with S/TRY_FMT. That's the information that the driver will use to set up > the buffers. It could have weird rules like: if the fourcc is this, and the > size is less than that, then we can allocate in this memory bank. > > It is also consistent with REQBUFS: there too the driver uses a full format > (i.e. the last set format). > > I would modify queue_setup to something like this: > > int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, > unsigned int *num_buffers, > unsigned int *num_planes, unsigned int sizes[], > void *alloc_ctxs[]); > > Whether fmt is left to NULL in the reqbufs case, or whether the driver has to > call g_fmt first before calling vb2 is something that could be decided by what > is easiest to implement. 1. VIDIOC_CREATE_BUFS passes struct v4l2_create_buffers from the user to the kernel, in which struct v4l2_format is embedded. The user _must_ fill in .type member of struct v4l2_format. For .type == V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT .fmt.pix is used, for .type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE .fmt.pix_mp is used. In both these cases the user _must_ fill in .width, .height, .pixelformat, .field, .colorspace by possibly calling VIDIOC_G_FMT or VIDIOC_TRY_FMT. The user also _may_ optionally fill in any further buffer-size related fields, if it believes to have any special requirements to them. On a successful return from the ioctl() .count and .index fields are filled in by the kernel, .format stays unchanged. The user has to call VIDIOC_QUERYBUF to retrieve specific buffer information. 2. Videobuf2 drivers, that implement .vidioc_create_bufs() operation, call vb2_create_bufs() with a pointer to struct v4l2_create_buffers as a second argument. vb2_create_bufs() in turn calls the .queue_setup() driver callback, whose prototype is modified as follows: int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt, unsigned int *num_buffers, unsigned int *num_planes, unsigned int sizes[], void *alloc_ctxs[]); with &create->format as a second argument. As pointed out above, this struct is not modified by V4L, instead, the usual arguments 3-6 are filled in by the driver, which are then used by vb2_create_bufs() to call __vb2_queue_alloc(). 3. vb2_reqbufs() shall call .queue_setup() with fmt == NULL, which will be a signal to the driver to use the current format. 4. We keep .queue_setup(), because its removal would inevitably push a part of the common code from vb2_reqbufs() and vb2_create_bufs() down into drivers, thus creating code redundancy and increasing its complexity. You have 24 hours to object, before I proceed with the next version;-) Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, August 22, 2011 15:54:03 Guennadi Liakhovetski wrote: > We discussed a bit more with Hans on IRC, and below is my attempt of a > summary. Hans, please, correct me, if I misunderstood anything. Looks good, that's exactly what I meant. Regards, Hans > Pawel, > Sakari, Laurent: please, reply, whether you're ok with this. > > On Mon, 22 Aug 2011, Hans Verkuil wrote: > > > On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: > > [snip] > > > > It would be good if you also could have a look at my reply to this Pawel's > > > mail: > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input- > > infrastructure/36905 > > > > > > and, specifically, at the vb2_parse_planes() function in it. That's my > > > understanding of what would be needed, if we preserve .queue_setup() and > > > use your last suggestion to include struct v4l2_format in struct > > > v4l2_create_buffers. > > > > vb2_parse_planes can be useful as a utility function that 'normal' drivers can > > call from the queue_setup. But vb2 should not parse the format directly, it > > should just pass it on to the driver through the queue_setup function. > > > > You also mention: "All frame-format fields like fourcc code, width, height, > > colorspace are only input from the user. If the user didn't fill them in, they > > should not be used." > > > > I disagree with that. The user should fill in a full format description, just > > as with S/TRY_FMT. That's the information that the driver will use to set up > > the buffers. It could have weird rules like: if the fourcc is this, and the > > size is less than that, then we can allocate in this memory bank. > > > > It is also consistent with REQBUFS: there too the driver uses a full format > > (i.e. the last set format). > > > > I would modify queue_setup to something like this: > > > > int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, > > unsigned int *num_buffers, > > unsigned int *num_planes, unsigned int sizes[], > > void *alloc_ctxs[]); > > > > Whether fmt is left to NULL in the reqbufs case, or whether the driver has to > > call g_fmt first before calling vb2 is something that could be decided by what > > is easiest to implement. > > 1. VIDIOC_CREATE_BUFS passes struct v4l2_create_buffers from the user to > the kernel, in which struct v4l2_format is embedded. The user _must_ > fill in .type member of struct v4l2_format. For .type == > V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT .fmt.pix is > used, for .type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE .fmt.pix_mp is used. In both these > cases the user _must_ fill in .width, .height, .pixelformat, .field, > .colorspace by possibly calling VIDIOC_G_FMT or VIDIOC_TRY_FMT. The > user also _may_ optionally fill in any further buffer-size related > fields, if it believes to have any special requirements to them. On > a successful return from the ioctl() .count and .index fields are > filled in by the kernel, .format stays unchanged. The user has to call > VIDIOC_QUERYBUF to retrieve specific buffer information. > > 2. Videobuf2 drivers, that implement .vidioc_create_bufs() operation, call > vb2_create_bufs() with a pointer to struct v4l2_create_buffers as a > second argument. vb2_create_bufs() in turn calls the .queue_setup() > driver callback, whose prototype is modified as follows: > > int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt, > unsigned int *num_buffers, > unsigned int *num_planes, unsigned int sizes[], > void *alloc_ctxs[]); > > with &create->format as a second argument. As pointed out above, this > struct is not modified by V4L, instead, the usual arguments 3-6 are > filled in by the driver, which are then used by vb2_create_bufs() to > call __vb2_queue_alloc(). > > 3. vb2_reqbufs() shall call .queue_setup() with fmt == NULL, which will be > a signal to the driver to use the current format. > > 4. We keep .queue_setup(), because its removal would inevitably push a > part of the common code from vb2_reqbufs() and vb2_create_bufs() down > into drivers, thus creating code redundancy and increasing its > complexity. > > You have 24 hours to object, before I proceed with the next version;-) > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Monday 22 August 2011 15:54:03 Guennadi Liakhovetski wrote: > We discussed a bit more with Hans on IRC, and below is my attempt of a > summary. Hans, please, correct me, if I misunderstood anything. Pawel, > Sakari, Laurent: please, reply, whether you're ok with this. Sakari is on holidays this week. > On Mon, 22 Aug 2011, Hans Verkuil wrote: > > On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: > [snip] > > > > It would be good if you also could have a look at my reply to this > > > Pawel's mail: > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input- > > > > infrastructure/36905 > > > > > and, specifically, at the vb2_parse_planes() function in it. That's my > > > understanding of what would be needed, if we preserve .queue_setup() > > > and use your last suggestion to include struct v4l2_format in struct > > > v4l2_create_buffers. > > > > vb2_parse_planes can be useful as a utility function that 'normal' > > drivers can call from the queue_setup. But vb2 should not parse the > > format directly, it should just pass it on to the driver through the > > queue_setup function. > > > > You also mention: "All frame-format fields like fourcc code, width, > > height, colorspace are only input from the user. If the user didn't fill > > them in, they should not be used." > > > > I disagree with that. The user should fill in a full format description, > > just as with S/TRY_FMT. That's the information that the driver will use > > to set up the buffers. It could have weird rules like: if the fourcc is > > this, and the size is less than that, then we can allocate in this > > memory bank. > > > > It is also consistent with REQBUFS: there too the driver uses a full > > format (i.e. the last set format). > > > > I would modify queue_setup to something like this: > > > > int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, > > > > unsigned int *num_buffers, > > unsigned int *num_planes, unsigned int sizes[], > > void *alloc_ctxs[]); > > > > Whether fmt is left to NULL in the reqbufs case, or whether the driver > > has to call g_fmt first before calling vb2 is something that could be > > decided by what is easiest to implement. > > 1. VIDIOC_CREATE_BUFS passes struct v4l2_create_buffers from the user to > the kernel, in which struct v4l2_format is embedded. The user _must_ > fill in .type member of struct v4l2_format. For .type == > V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT .fmt.pix is > used, for .type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE .fmt.pix_mp is used. In both these > cases the user _must_ fill in .width, .height, .pixelformat, .field, > .colorspace by possibly calling VIDIOC_G_FMT or VIDIOC_TRY_FMT. The > user also _may_ optionally fill in any further buffer-size related > fields, if it believes to have any special requirements to them. On > a successful return from the ioctl() .count and .index fields are > filled in by the kernel, .format stays unchanged. The user has to call > VIDIOC_QUERYBUF to retrieve specific buffer information. > > 2. Videobuf2 drivers, that implement .vidioc_create_bufs() operation, call > vb2_create_bufs() with a pointer to struct v4l2_create_buffers as a > second argument. vb2_create_bufs() in turn calls the .queue_setup() > driver callback, whose prototype is modified as follows: > > int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt, > unsigned int *num_buffers, > unsigned int *num_planes, unsigned int sizes[], > void *alloc_ctxs[]); > > with &create->format as a second argument. As pointed out above, this > struct is not modified by V4L, instead, the usual arguments 3-6 are > filled in by the driver, which are then used by vb2_create_bufs() to > call __vb2_queue_alloc(). > > 3. vb2_reqbufs() shall call .queue_setup() with fmt == NULL, which will be > a signal to the driver to use the current format. > > 4. We keep .queue_setup(), because its removal would inevitably push a > part of the common code from vb2_reqbufs() and vb2_create_bufs() down > into drivers, thus creating code redundancy and increasing its > complexity. How much common code would be pushed down to drivers ? I don't think this is a real issue. I like Pawel's proposal of removing .queue_setup() better. > You have 24 hours to object, before I proceed with the next version;-)
On Monday, August 22, 2011 17:42:36 Laurent Pinchart wrote: > Hi Guennadi, > > On Monday 22 August 2011 15:54:03 Guennadi Liakhovetski wrote: > > We discussed a bit more with Hans on IRC, and below is my attempt of a > > summary. Hans, please, correct me, if I misunderstood anything. Pawel, > > Sakari, Laurent: please, reply, whether you're ok with this. > > Sakari is on holidays this week. > > > On Mon, 22 Aug 2011, Hans Verkuil wrote: > > > On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: > > [snip] > > > > > > It would be good if you also could have a look at my reply to this > > > > Pawel's mail: > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input- > > > > > > infrastructure/36905 > > > > > > > and, specifically, at the vb2_parse_planes() function in it. That's my > > > > understanding of what would be needed, if we preserve .queue_setup() > > > > and use your last suggestion to include struct v4l2_format in struct > > > > v4l2_create_buffers. > > > > > > vb2_parse_planes can be useful as a utility function that 'normal' > > > drivers can call from the queue_setup. But vb2 should not parse the > > > format directly, it should just pass it on to the driver through the > > > queue_setup function. > > > > > > You also mention: "All frame-format fields like fourcc code, width, > > > height, colorspace are only input from the user. If the user didn't fill > > > them in, they should not be used." > > > > > > I disagree with that. The user should fill in a full format description, > > > just as with S/TRY_FMT. That's the information that the driver will use > > > to set up the buffers. It could have weird rules like: if the fourcc is > > > this, and the size is less than that, then we can allocate in this > > > memory bank. > > > > > > It is also consistent with REQBUFS: there too the driver uses a full > > > format (i.e. the last set format). > > > > > > I would modify queue_setup to something like this: > > > > > > int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, > > > > > > unsigned int *num_buffers, > > > unsigned int *num_planes, unsigned int sizes[], > > > void *alloc_ctxs[]); > > > > > > Whether fmt is left to NULL in the reqbufs case, or whether the driver > > > has to call g_fmt first before calling vb2 is something that could be > > > decided by what is easiest to implement. > > > > 1. VIDIOC_CREATE_BUFS passes struct v4l2_create_buffers from the user to > > the kernel, in which struct v4l2_format is embedded. The user _must_ > > fill in .type member of struct v4l2_format. For .type == > > V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT .fmt.pix is > > used, for .type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE .fmt.pix_mp is used. In both these > > cases the user _must_ fill in .width, .height, .pixelformat, .field, > > .colorspace by possibly calling VIDIOC_G_FMT or VIDIOC_TRY_FMT. The > > user also _may_ optionally fill in any further buffer-size related > > fields, if it believes to have any special requirements to them. On > > a successful return from the ioctl() .count and .index fields are > > filled in by the kernel, .format stays unchanged. The user has to call > > VIDIOC_QUERYBUF to retrieve specific buffer information. > > > > 2. Videobuf2 drivers, that implement .vidioc_create_bufs() operation, call > > vb2_create_bufs() with a pointer to struct v4l2_create_buffers as a > > second argument. vb2_create_bufs() in turn calls the .queue_setup() > > driver callback, whose prototype is modified as follows: > > > > int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt, > > unsigned int *num_buffers, > > unsigned int *num_planes, unsigned int sizes[], > > void *alloc_ctxs[]); > > > > with &create->format as a second argument. As pointed out above, this > > struct is not modified by V4L, instead, the usual arguments 3-6 are > > filled in by the driver, which are then used by vb2_create_bufs() to > > call __vb2_queue_alloc(). > > > > 3. vb2_reqbufs() shall call .queue_setup() with fmt == NULL, which will be > > a signal to the driver to use the current format. > > > > 4. We keep .queue_setup(), because its removal would inevitably push a > > part of the common code from vb2_reqbufs() and vb2_create_bufs() down > > into drivers, thus creating code redundancy and increasing its > > complexity. > > How much common code would be pushed down to drivers ? I don't think this is a > real issue. I like Pawel's proposal of removing .queue_setup() better. I still don't see what removing queue_setup will solve or improve. I'd say leave it as it is to keep the diff as small as possible and someone can always attempt to remove it later. Removing queue_setup is independent from multi-size videobuffer management and we should not mix the two. Regards, Hans > > You have 24 hours to object, before I proceed with the next version;-) > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Monday 22 August 2011 17:52:12 Hans Verkuil wrote: > On Monday, August 22, 2011 17:42:36 Laurent Pinchart wrote: > > On Monday 22 August 2011 15:54:03 Guennadi Liakhovetski wrote: > > > We discussed a bit more with Hans on IRC, and below is my attempt of a > > > summary. Hans, please, correct me, if I misunderstood anything. Pawel, > > > Sakari, Laurent: please, reply, whether you're ok with this. > > > > Sakari is on holidays this week. > > > > > On Mon, 22 Aug 2011, Hans Verkuil wrote: > > > > On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: > > > [snip] > > > > > > > > It would be good if you also could have a look at my reply to this > > > > > Pawel's mail: > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input- > > > > > > > > infrastructure/36905 > > > > > > > > > and, specifically, at the vb2_parse_planes() function in it. That's > > > > > my understanding of what would be needed, if we preserve > > > > > .queue_setup() and use your last suggestion to include struct > > > > > v4l2_format in struct v4l2_create_buffers. > > > > > > > > vb2_parse_planes can be useful as a utility function that 'normal' > > > > drivers can call from the queue_setup. But vb2 should not parse the > > > > format directly, it should just pass it on to the driver through the > > > > queue_setup function. > > > > > > > > You also mention: "All frame-format fields like fourcc code, width, > > > > height, colorspace are only input from the user. If the user didn't > > > > fill them in, they should not be used." > > > > > > > > I disagree with that. The user should fill in a full format > > > > description, just as with S/TRY_FMT. That's the information that the > > > > driver will use to set up the buffers. It could have weird rules > > > > like: if the fourcc is this, and the size is less than that, then we > > > > can allocate in this memory bank. > > > > > > > > It is also consistent with REQBUFS: there too the driver uses a full > > > > format (i.e. the last set format). > > > > > > > > I would modify queue_setup to something like this: > > > > > > > > int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, > > > > > > > > unsigned int *num_buffers, > > > > unsigned int *num_planes, unsigned int sizes[], > > > > void *alloc_ctxs[]); > > > > > > > > Whether fmt is left to NULL in the reqbufs case, or whether the > > > > driver has to call g_fmt first before calling vb2 is something that > > > > could be decided by what is easiest to implement. > > > > > > 1. VIDIOC_CREATE_BUFS passes struct v4l2_create_buffers from the user > > > to > > > > > > the kernel, in which struct v4l2_format is embedded. The user _must_ > > > fill in .type member of struct v4l2_format. For .type == > > > V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT .fmt.pix > > > is used, for .type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or > > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE .fmt.pix_mp is used. In both these > > > cases the user _must_ fill in .width, .height, .pixelformat, .field, > > > .colorspace by possibly calling VIDIOC_G_FMT or VIDIOC_TRY_FMT. The > > > user also _may_ optionally fill in any further buffer-size related > > > fields, if it believes to have any special requirements to them. On > > > a successful return from the ioctl() .count and .index fields are > > > filled in by the kernel, .format stays unchanged. The user has to > > > call VIDIOC_QUERYBUF to retrieve specific buffer information. > > > > > > 2. Videobuf2 drivers, that implement .vidioc_create_bufs() operation, > > > call > > > > > > vb2_create_bufs() with a pointer to struct v4l2_create_buffers as a > > > second argument. vb2_create_bufs() in turn calls the .queue_setup() > > > > > > driver callback, whose prototype is modified as follows: > > > int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt, > > > > > > unsigned int *num_buffers, > > > unsigned int *num_planes, unsigned int sizes[], > > > void *alloc_ctxs[]); > > > > > > with &create->format as a second argument. As pointed out above, > > > this struct is not modified by V4L, instead, the usual arguments > > > 3-6 are filled in by the driver, which are then used by > > > vb2_create_bufs() to call __vb2_queue_alloc(). > > > > > > 3. vb2_reqbufs() shall call .queue_setup() with fmt == NULL, which will > > > be > > > > > > a signal to the driver to use the current format. > > > > > > 4. We keep .queue_setup(), because its removal would inevitably push a > > > > > > part of the common code from vb2_reqbufs() and vb2_create_bufs() > > > down into drivers, thus creating code redundancy and increasing its > > > complexity. > > > > How much common code would be pushed down to drivers ? I don't think this > > is a real issue. I like Pawel's proposal of removing .queue_setup() > > better. > > I still don't see what removing queue_setup will solve or improve. It will remove handling of the format in vb2 (even if it's a pass-through operation). I think it would be cleaner that way. It will also avoid going back and forth between drivers and vb2, which would improve code readability. > I'd say leave it as it is to keep the diff as small as possible and someone > can always attempt to remove it later. Removing queue_setup is independent > from multi-size videobuffer management and we should not mix the two. Guennadi's patch will (at least in my opinion) be cleaner if built on top of queue_setup() removal.
On Monday, August 22, 2011 19:21:18 Laurent Pinchart wrote: > Hi Hans, > > On Monday 22 August 2011 17:52:12 Hans Verkuil wrote: > > On Monday, August 22, 2011 17:42:36 Laurent Pinchart wrote: > > > On Monday 22 August 2011 15:54:03 Guennadi Liakhovetski wrote: > > > > We discussed a bit more with Hans on IRC, and below is my attempt of a > > > > summary. Hans, please, correct me, if I misunderstood anything. Pawel, > > > > Sakari, Laurent: please, reply, whether you're ok with this. > > > > > > Sakari is on holidays this week. > > > > > > > On Mon, 22 Aug 2011, Hans Verkuil wrote: > > > > > On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: > > > > [snip] > > > > > > > > > > It would be good if you also could have a look at my reply to this > > > > > > Pawel's mail: > > > > > > > > > > > > http://article.gmane.org/gmane.linux.drivers.video-input- > > > > > > > > > > infrastructure/36905 > > > > > > > > > > > and, specifically, at the vb2_parse_planes() function in it. That's > > > > > > my understanding of what would be needed, if we preserve > > > > > > .queue_setup() and use your last suggestion to include struct > > > > > > v4l2_format in struct v4l2_create_buffers. > > > > > > > > > > vb2_parse_planes can be useful as a utility function that 'normal' > > > > > drivers can call from the queue_setup. But vb2 should not parse the > > > > > format directly, it should just pass it on to the driver through the > > > > > queue_setup function. > > > > > > > > > > You also mention: "All frame-format fields like fourcc code, width, > > > > > height, colorspace are only input from the user. If the user didn't > > > > > fill them in, they should not be used." > > > > > > > > > > I disagree with that. The user should fill in a full format > > > > > description, just as with S/TRY_FMT. That's the information that the > > > > > driver will use to set up the buffers. It could have weird rules > > > > > like: if the fourcc is this, and the size is less than that, then we > > > > > can allocate in this memory bank. > > > > > > > > > > It is also consistent with REQBUFS: there too the driver uses a full > > > > > format (i.e. the last set format). > > > > > > > > > > I would modify queue_setup to something like this: > > > > > > > > > > int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, > > > > > > > > > > unsigned int *num_buffers, > > > > > unsigned int *num_planes, unsigned int sizes[], > > > > > void *alloc_ctxs[]); > > > > > > > > > > Whether fmt is left to NULL in the reqbufs case, or whether the > > > > > driver has to call g_fmt first before calling vb2 is something that > > > > > could be decided by what is easiest to implement. > > > > > > > > 1. VIDIOC_CREATE_BUFS passes struct v4l2_create_buffers from the user > > > > to > > > > > > > > the kernel, in which struct v4l2_format is embedded. The user _must_ > > > > fill in .type member of struct v4l2_format. For .type == > > > > V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT .fmt.pix > > > > is used, for .type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or > > > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE .fmt.pix_mp is used. In both these > > > > cases the user _must_ fill in .width, .height, .pixelformat, .field, > > > > .colorspace by possibly calling VIDIOC_G_FMT or VIDIOC_TRY_FMT. The > > > > user also _may_ optionally fill in any further buffer-size related > > > > fields, if it believes to have any special requirements to them. On > > > > a successful return from the ioctl() .count and .index fields are > > > > filled in by the kernel, .format stays unchanged. The user has to > > > > call VIDIOC_QUERYBUF to retrieve specific buffer information. > > > > > > > > 2. Videobuf2 drivers, that implement .vidioc_create_bufs() operation, > > > > call > > > > > > > > vb2_create_bufs() with a pointer to struct v4l2_create_buffers as a > > > > second argument. vb2_create_bufs() in turn calls the .queue_setup() > > > > > > > > driver callback, whose prototype is modified as follows: > > > > int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt, > > > > > > > > unsigned int *num_buffers, > > > > unsigned int *num_planes, unsigned int sizes[], > > > > void *alloc_ctxs[]); > > > > > > > > with &create->format as a second argument. As pointed out above, > > > > this struct is not modified by V4L, instead, the usual arguments > > > > 3-6 are filled in by the driver, which are then used by > > > > vb2_create_bufs() to call __vb2_queue_alloc(). > > > > > > > > 3. vb2_reqbufs() shall call .queue_setup() with fmt == NULL, which will > > > > be > > > > > > > > a signal to the driver to use the current format. > > > > > > > > 4. We keep .queue_setup(), because its removal would inevitably push a > > > > > > > > part of the common code from vb2_reqbufs() and vb2_create_bufs() > > > > down into drivers, thus creating code redundancy and increasing its > > > > complexity. > > > > > > How much common code would be pushed down to drivers ? I don't think this > > > is a real issue. I like Pawel's proposal of removing .queue_setup() > > > better. > > > > I still don't see what removing queue_setup will solve or improve. > > It will remove handling of the format in vb2 (even if it's a pass-through > operation). I think it would be cleaner that way. It will also avoid going > back and forth between drivers and vb2, which would improve code readability. I very much doubt it will be more readable or cleaner. For one thing it is inconsistent with the other ioctl ops where you just call a vb2 function to handle it. Suddenly here you have to do lots of things to make it work. > > I'd say leave it as it is to keep the diff as small as possible and someone > > can always attempt to remove it later. Removing queue_setup is independent > > from multi-size videobuffer management and we should not mix the two. > > Guennadi's patch will (at least in my opinion) be cleaner if built on top of > queue_setup() removal. Really? Just adding a single v4l2_format pointer is less clean than removing queue_setup? That would really surprise me. Anyway, let Guennadi choose what is easiest. It's an implementation detail in the end and I just want to get this functionality in. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Aug 22, 2011 at 23:31, Hans Verkuil <hverkuil@xs4all.nl> wrote: > On Monday, August 22, 2011 19:21:18 Laurent Pinchart wrote: >> Hi Hans, >> >> On Monday 22 August 2011 17:52:12 Hans Verkuil wrote: >> > On Monday, August 22, 2011 17:42:36 Laurent Pinchart wrote: >> > > On Monday 22 August 2011 15:54:03 Guennadi Liakhovetski wrote: >> > > > We discussed a bit more with Hans on IRC, and below is my attempt of a >> > > > summary. Hans, please, correct me, if I misunderstood anything. Pawel, >> > > > Sakari, Laurent: please, reply, whether you're ok with this. >> > > >> > > Sakari is on holidays this week. >> > > >> > > > On Mon, 22 Aug 2011, Hans Verkuil wrote: >> > > > > On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: >> > > > [snip] >> > > > >> > > > > > It would be good if you also could have a look at my reply to this >> > > > > > Pawel's mail: >> > > > > > >> > > > > > http://article.gmane.org/gmane.linux.drivers.video-input- >> > > > > >> > > > > infrastructure/36905 >> > > > > >> > > > > > and, specifically, at the vb2_parse_planes() function in it. That's >> > > > > > my understanding of what would be needed, if we preserve >> > > > > > .queue_setup() and use your last suggestion to include struct >> > > > > > v4l2_format in struct v4l2_create_buffers. >> > > > > >> > > > > vb2_parse_planes can be useful as a utility function that 'normal' >> > > > > drivers can call from the queue_setup. But vb2 should not parse the >> > > > > format directly, it should just pass it on to the driver through the >> > > > > queue_setup function. >> > > > > >> > > > > You also mention: "All frame-format fields like fourcc code, width, >> > > > > height, colorspace are only input from the user. If the user didn't >> > > > > fill them in, they should not be used." >> > > > > >> > > > > I disagree with that. The user should fill in a full format >> > > > > description, just as with S/TRY_FMT. That's the information that the >> > > > > driver will use to set up the buffers. It could have weird rules >> > > > > like: if the fourcc is this, and the size is less than that, then we >> > > > > can allocate in this memory bank. >> > > > > >> > > > > It is also consistent with REQBUFS: there too the driver uses a full >> > > > > format (i.e. the last set format). >> > > > > >> > > > > I would modify queue_setup to something like this: >> > > > > >> > > > > int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, >> > > > > >> > > > > unsigned int *num_buffers, >> > > > > unsigned int *num_planes, unsigned int sizes[], >> > > > > void *alloc_ctxs[]); >> > > > > >> > > > > Whether fmt is left to NULL in the reqbufs case, or whether the >> > > > > driver has to call g_fmt first before calling vb2 is something that >> > > > > could be decided by what is easiest to implement. >> > > > >> > > > 1. VIDIOC_CREATE_BUFS passes struct v4l2_create_buffers from the user >> > > > to >> > > > >> > > > the kernel, in which struct v4l2_format is embedded. The user _must_ >> > > > fill in .type member of struct v4l2_format. For .type == >> > > > V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT .fmt.pix >> > > > is used, for .type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or >> > > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE .fmt.pix_mp is used. In both these >> > > > cases the user _must_ fill in .width, .height, .pixelformat, .field, >> > > > .colorspace by possibly calling VIDIOC_G_FMT or VIDIOC_TRY_FMT. The >> > > > user also _may_ optionally fill in any further buffer-size related >> > > > fields, if it believes to have any special requirements to them. On >> > > > a successful return from the ioctl() .count and .index fields are >> > > > filled in by the kernel, .format stays unchanged. The user has to >> > > > call VIDIOC_QUERYBUF to retrieve specific buffer information. >> > > > Sounds good, just one question: we deliberately don't want to allow CREATE_BUFS to adjust the format in any way, as S_FMT could? >> > > > 2. Videobuf2 drivers, that implement .vidioc_create_bufs() operation, >> > > > call >> > > > >> > > > vb2_create_bufs() with a pointer to struct v4l2_create_buffers as a >> > > > second argument. vb2_create_bufs() in turn calls the .queue_setup() >> > > > >> > > > driver callback, whose prototype is modified as follows: >> > > > int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt, >> > > > >> > > > unsigned int *num_buffers, >> > > > unsigned int *num_planes, unsigned int sizes[], >> > > > void *alloc_ctxs[]); >> > > > >> > > > with &create->format as a second argument. As pointed out above, >> > > > this struct is not modified by V4L, instead, the usual arguments >> > > > 3-6 are filled in by the driver, which are then used by >> > > > vb2_create_bufs() to call __vb2_queue_alloc(). >> > > > >> > > > 3. vb2_reqbufs() shall call .queue_setup() with fmt == NULL, which will >> > > > be >> > > > >> > > > a signal to the driver to use the current format. >> > > > >> > > > 4. We keep .queue_setup(), because its removal would inevitably push a >> > > > >> > > > part of the common code from vb2_reqbufs() and vb2_create_bufs() >> > > > down into drivers, thus creating code redundancy and increasing its >> > > > complexity. >> > > What part would be passed down to drivers? Please see my example below. I actually feel the drivers would become slightly simpler with this change. >> > > How much common code would be pushed down to drivers ? I don't think this >> > > is a real issue. I like Pawel's proposal of removing .queue_setup() >> > > better. >> > >> > I still don't see what removing queue_setup will solve or improve. >> >> It will remove handling of the format in vb2 (even if it's a pass-through >> operation). I think it would be cleaner that way. It will also avoid going >> back and forth between drivers and vb2, which would improve code readability. > > I very much doubt it will be more readable or cleaner. For one thing it is > inconsistent with the other ioctl ops where you just call a vb2 function to > handle it. Suddenly here you have to do lots of things to make it work. > This flow: REQBUFS->driver_reqbufs()->vb2_reqbufs()->queue_setup(fmt=NULL, nums, sizes)->vb2_reqbufs->driver_reqbufs CREATE_BUFS->driver_create_bufs()->vb2_create_bufs(fmt)->queue_setup(fmt, nums, sizes)->vb2_create_bufs->driver_create_bufs is more complicated than this flow: REQBUFS->driver_reqbufs()->vb2_reqbufs(nums, sizes)->driver_reqbufs CREATE_BUFS->driver_create_bufs()->vb2_create_bufs(nums, sizes)->driver_create_bufs without giving any clear advantage (at least from what I can see), apart from making Guennadi's change simpler. Below is what I'm proposing. This, in my opinion, makes vb2's interface cleaner, as the format is never passed to it. I don't see what code it'd be passing down to drivers. The goal is to pass (return) nums/sizes to vb2 anyway. Existing queue_setup() could with some modifications become figure_out_params(): driver_create_bufs(create) /* optional */ { /* use create->fmt to figure out num_* and *_sizes */ figure_out_params(create->fmt, &num_buffers, &num_planes, &buf_sizes, &plane_sizes); return vb2_create_bufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs); } driver_reqbufs() { /* use current format to figure out num_* and *_sizes */ figure_out_params(current_fmt, &num_buffers, &num_planes, &buf_sizes, &plane_sizes); return vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs); } /* ---------- */ vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs) { if (buffers_allocated) free_buffers(...); return vb2_create_bufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs); } >> > I'd say leave it as it is to keep the diff as small as possible and someone >> > can always attempt to remove it later. Removing queue_setup is independent >> > from multi-size videobuffer management and we should not mix the two. >> >> Guennadi's patch will (at least in my opinion) be cleaner if built on top of >> queue_setup() removal. > > Really? Just adding a single v4l2_format pointer is less clean than removing > queue_setup? That would really surprise me. > Anyway, let Guennadi choose what is easiest. It's an implementation detail in > the end and I just want to get this functionality in. I'm not completely opposed to your last suggestion. I agree the patch would be much shorter. Your proposal is reasonable and simple enough. I just feel that it'd pay off to put a little bit more effort to make the changes I'm proposing, to make the interface cleaner and simplify ping-ponging calls and parameters between drivers and vb2. But I don't mind if we make it simple for now. As you suggested, I'll be more than happy to look into removing queue_setup later.
On Wednesday, August 24, 2011 06:05:44 Pawel Osciak wrote: > Hi, > > On Mon, Aug 22, 2011 at 23:31, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On Monday, August 22, 2011 19:21:18 Laurent Pinchart wrote: > >> Hi Hans, > >> > >> On Monday 22 August 2011 17:52:12 Hans Verkuil wrote: > >> > On Monday, August 22, 2011 17:42:36 Laurent Pinchart wrote: > >> > > On Monday 22 August 2011 15:54:03 Guennadi Liakhovetski wrote: > >> > > > We discussed a bit more with Hans on IRC, and below is my attempt of a > >> > > > summary. Hans, please, correct me, if I misunderstood anything. Pawel, > >> > > > Sakari, Laurent: please, reply, whether you're ok with this. > >> > > > >> > > Sakari is on holidays this week. > >> > > > >> > > > On Mon, 22 Aug 2011, Hans Verkuil wrote: > >> > > > > On Monday, August 22, 2011 12:40:25 Guennadi Liakhovetski wrote: > >> > > > [snip] > >> > > > > >> > > > > > It would be good if you also could have a look at my reply to this > >> > > > > > Pawel's mail: > >> > > > > > > >> > > > > > http://article.gmane.org/gmane.linux.drivers.video-input- > >> > > > > > >> > > > > infrastructure/36905 > >> > > > > > >> > > > > > and, specifically, at the vb2_parse_planes() function in it. That's > >> > > > > > my understanding of what would be needed, if we preserve > >> > > > > > .queue_setup() and use your last suggestion to include struct > >> > > > > > v4l2_format in struct v4l2_create_buffers. > >> > > > > > >> > > > > vb2_parse_planes can be useful as a utility function that 'normal' > >> > > > > drivers can call from the queue_setup. But vb2 should not parse the > >> > > > > format directly, it should just pass it on to the driver through the > >> > > > > queue_setup function. > >> > > > > > >> > > > > You also mention: "All frame-format fields like fourcc code, width, > >> > > > > height, colorspace are only input from the user. If the user didn't > >> > > > > fill them in, they should not be used." > >> > > > > > >> > > > > I disagree with that. The user should fill in a full format > >> > > > > description, just as with S/TRY_FMT. That's the information that the > >> > > > > driver will use to set up the buffers. It could have weird rules > >> > > > > like: if the fourcc is this, and the size is less than that, then we > >> > > > > can allocate in this memory bank. > >> > > > > > >> > > > > It is also consistent with REQBUFS: there too the driver uses a full > >> > > > > format (i.e. the last set format). > >> > > > > > >> > > > > I would modify queue_setup to something like this: > >> > > > > > >> > > > > int (*queue_setup)(struct vb2_queue *q, struct v4l2_format *fmt, > >> > > > > > >> > > > > unsigned int *num_buffers, > >> > > > > unsigned int *num_planes, unsigned int sizes[], > >> > > > > void *alloc_ctxs[]); > >> > > > > > >> > > > > Whether fmt is left to NULL in the reqbufs case, or whether the > >> > > > > driver has to call g_fmt first before calling vb2 is something that > >> > > > > could be decided by what is easiest to implement. > >> > > > > >> > > > 1. VIDIOC_CREATE_BUFS passes struct v4l2_create_buffers from the user > >> > > > to > >> > > > > >> > > > the kernel, in which struct v4l2_format is embedded. The user _must_ > >> > > > fill in .type member of struct v4l2_format. For .type == > >> > > > V4L2_BUF_TYPE_VIDEO_CAPTURE or V4L2_BUF_TYPE_VIDEO_OUTPUT .fmt.pix > >> > > > is used, for .type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or > >> > > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE .fmt.pix_mp is used. In both these > >> > > > cases the user _must_ fill in .width, .height, .pixelformat, .field, > >> > > > .colorspace by possibly calling VIDIOC_G_FMT or VIDIOC_TRY_FMT. The > >> > > > user also _may_ optionally fill in any further buffer-size related > >> > > > fields, if it believes to have any special requirements to them. On > >> > > > a successful return from the ioctl() .count and .index fields are > >> > > > filled in by the kernel, .format stays unchanged. The user has to > >> > > > call VIDIOC_QUERYBUF to retrieve specific buffer information. > >> > > > > > Sounds good, just one question: we deliberately don't want to allow > CREATE_BUFS to adjust the format in any way, as S_FMT could? That was my initial idea, but I don't think that holds water. The sequence probably has to be (ideal for a utility function): 1) make a copy of the sizeimage fields 2) call try_fmt 3) replace the sizeimage fields with the max value of the 'try' values and the copy. Then this final fmt can be returned. > >> > > > 2. Videobuf2 drivers, that implement .vidioc_create_bufs() operation, > >> > > > call > >> > > > > >> > > > vb2_create_bufs() with a pointer to struct v4l2_create_buffers as a > >> > > > second argument. vb2_create_bufs() in turn calls the .queue_setup() > >> > > > > >> > > > driver callback, whose prototype is modified as follows: > >> > > > int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt, > >> > > > > >> > > > unsigned int *num_buffers, > >> > > > unsigned int *num_planes, unsigned int sizes[], > >> > > > void *alloc_ctxs[]); > >> > > > > >> > > > with &create->format as a second argument. As pointed out above, > >> > > > this struct is not modified by V4L, instead, the usual arguments > >> > > > 3-6 are filled in by the driver, which are then used by > >> > > > vb2_create_bufs() to call __vb2_queue_alloc(). > >> > > > > >> > > > 3. vb2_reqbufs() shall call .queue_setup() with fmt == NULL, which will > >> > > > be > >> > > > > >> > > > a signal to the driver to use the current format. > >> > > > > >> > > > 4. We keep .queue_setup(), because its removal would inevitably push a > >> > > > > >> > > > part of the common code from vb2_reqbufs() and vb2_create_bufs() > >> > > > down into drivers, thus creating code redundancy and increasing its > >> > > > complexity. > >> > > > > What part would be passed down to drivers? Please see my example > below. I actually feel the drivers would become slightly simpler with > this change. Roughly speaking it has to do at least part of what is in vb2_reqbufs before the call to queue_setup. In particular for reqbufs it will have to explicitly test against count == 0 since that's a special case. > >> > > How much common code would be pushed down to drivers ? I don't think this > >> > > is a real issue. I like Pawel's proposal of removing .queue_setup() > >> > > better. > >> > > >> > I still don't see what removing queue_setup will solve or improve. > >> > >> It will remove handling of the format in vb2 (even if it's a pass-through > >> operation). I think it would be cleaner that way. It will also avoid going > >> back and forth between drivers and vb2, which would improve code readability. > > > > I very much doubt it will be more readable or cleaner. For one thing it is > > inconsistent with the other ioctl ops where you just call a vb2 function to > > handle it. Suddenly here you have to do lots of things to make it work. > > > > This flow: > > REQBUFS->driver_reqbufs()->vb2_reqbufs()->queue_setup(fmt=NULL, nums, > sizes)->vb2_reqbufs->driver_reqbufs > CREATE_BUFS->driver_create_bufs()->vb2_create_bufs(fmt)->queue_setup(fmt, > nums, sizes)->vb2_create_bufs->driver_create_bufs > > is more complicated than this flow: > > REQBUFS->driver_reqbufs()->vb2_reqbufs(nums, sizes)->driver_reqbufs > CREATE_BUFS->driver_create_bufs()->vb2_create_bufs(nums, > sizes)->driver_create_bufs > > without giving any clear advantage (at least from what I can see), > apart from making Guennadi's change simpler. > > > Below is what I'm proposing. This, in my opinion, makes vb2's > interface cleaner, as the format is never passed to it. I don't see > what code it'd be passing down to drivers. The goal is to pass > (return) nums/sizes to vb2 anyway. Existing queue_setup() could with > some modifications become figure_out_params(): > > driver_create_bufs(create) /* optional */ > { > /* use create->fmt to figure out num_* and *_sizes */ > figure_out_params(create->fmt, &num_buffers, &num_planes, > &buf_sizes, &plane_sizes); > return vb2_create_bufs(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs); > } > > driver_reqbufs() > { > /* use current format to figure out num_* and *_sizes */ > figure_out_params(current_fmt, &num_buffers, &num_planes, > &buf_sizes, &plane_sizes); So you have to set up the necessary variables etc. to store the num_planes, buf_sizes, alloc_ctxs, etc. And you have to do that for each driver as well. > return vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, > alloc_ctxs); > } > > /* ---------- */ > > vb2_reqbufs(num_buffers, num_planes, buf_sizes, plane_sizes, alloc_ctxs) > { > if (buffers_allocated) > free_buffers(...); > > return vb2_create_bufs(num_buffers, num_planes, buf_sizes, > plane_sizes, alloc_ctxs); > } Where I would simplify the flow is to change the vb2_reqbufs prototype to be a precise match to the ioctl op. That way a driver no longer has to implement a reqbufs op (or any of the others). The only thing necessary for that is to add a vb2_queue pointer to struct video_device. It would also solve another issue that's been bugging me for some time now: vb2 has no knowledge of which filehandle is streaming. For non-mem2mem drivers that's important information since you need to be able to check whether the current filehandle is streaming or not. Right now each driver needs to store that information itself, something that's easy to do wrong. This would simplify the flow to: REQBUFS->vb2_reqbufs()->queue_setup(fmt=NULL, nums, sizes)->vb2_reqbufs CREATE_BUFS->vb2_create_bufs(fmt)->queue_setup(fmt, nums, sizes)->vb2_create_bufs I like this much better since it allows for greater integration of vb2 with the v4l2 framework. > >> > I'd say leave it as it is to keep the diff as small as possible and someone > >> > can always attempt to remove it later. Removing queue_setup is independent > >> > from multi-size videobuffer management and we should not mix the two. > >> > >> Guennadi's patch will (at least in my opinion) be cleaner if built on top of > >> queue_setup() removal. > > > > Really? Just adding a single v4l2_format pointer is less clean than removing > > queue_setup? That would really surprise me. > > Anyway, let Guennadi choose what is easiest. It's an implementation detail in > > the end and I just want to get this functionality in. > > I'm not completely opposed to your last suggestion. I agree the patch > would be much shorter. Your proposal is reasonable and simple enough. > I just feel that it'd pay off to put a little bit more effort to make > the changes I'm proposing, to make the interface cleaner and simplify > ping-ponging calls and parameters between drivers and vb2. > > But I don't mind if we make it simple for now. As you suggested, I'll > be more than happy to look into removing queue_setup later. I think we should go for the simple solution right now. All this is independent from adding createbufs. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.</entry> Applications set or clear this flag before calling the <constant>VIDIOC_QBUF</constant> ioctl.</entry> </row> + <row> + <entry><constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant></entry> + <entry>0x0400</entry> + <entry>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. +</entry> + </row> + <row> + <entry><constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant></entry> + <entry>0x0800</entry> + <entry>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.</entry> + </row> </tbody> </tgroup> </table> 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.</revremark> &sub-close; &sub-ioctl; <!-- All ioctls go here. --> + &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.</revremark> &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 @@ +<refentry id="vidioc-create-bufs"> + <refmeta> + <refentrytitle>ioctl VIDIOC_CREATE_BUFS</refentrytitle> + &manvol; + </refmeta> + + <refnamediv> + <refname>VIDIOC_CREATE_BUFS</refname> + <refpurpose>Create buffers for Memory Mapped or User Pointer I/O</refpurpose> + </refnamediv> + + <refsynopsisdiv> + <funcsynopsis> + <funcprototype> + <funcdef>int <function>ioctl</function></funcdef> + <paramdef>int <parameter>fd</parameter></paramdef> + <paramdef>int <parameter>request</parameter></paramdef> + <paramdef>struct v4l2_create_buffers *<parameter>argp</parameter></paramdef> + </funcprototype> + </funcsynopsis> + </refsynopsisdiv> + + <refsect1> + <title>Arguments</title> + + <variablelist> + <varlistentry> + <term><parameter>fd</parameter></term> + <listitem> + <para>&fd;</para> + </listitem> + </varlistentry> + <varlistentry> + <term><parameter>request</parameter></term> + <listitem> + <para>VIDIOC_CREATE_BUFS</para> + </listitem> + </varlistentry> + <varlistentry> + <term><parameter>argp</parameter></term> + <listitem> + <para></para> + </listitem> + </varlistentry> + </variablelist> + </refsect1> + + <refsect1> + <title>Description</title> + + <para>This ioctl is used to create buffers for <link linkend="mmap">memory +mapped</link> or <link linkend="userp">user pointer</link> +I/O. It can be used as an alternative or in addition to the +<constant>VIDIOC_REQBUFS</constant> ioctl, when a tighter control over buffers +is required. This ioctl can be called multiple times to create buffers of +different sizes.</para> + + <para>To allocate device buffers applications initialize relevant +fields of the <structname>v4l2_create_buffers</structname> structure. +They set the <structfield>type</structfield> field to the respective stream +or buffer type. <structfield>count</structfield> must be set to the number of +required buffers. <structfield>memory</structfield> specifies the required I/O +method. If <structfield>num_planes</structfield> == 0 or all elements of the +<structfield>sizes</structfield> array are 0, then buffers, suitable for the +currently configured video format, are allocated, exactly like for the +<constant>VIDIOC_REQBUFS</constant> ioctl. If +<structfield>num_planes</structfield> > 0 and at least some of the respective +<structfield>sizes</structfield> elements are non-zero, this information will be +used for buffer-allocation. The <structfield>reserved</structfield> 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 +<structfield>count</structfield> and the <structfield>index</structfield> +fields respectively. On return <structfield>count</structfield> can be smaller +than the number requested.</para> + <para>When the I/O method is not supported the ioctl +returns an &EINVAL;.</para> + + <table pgwide="1" frame="none" id="v4l2-create-buffers"> + <title>struct <structname>v4l2_create_buffers</structname></title> + <tgroup cols="3"> + &cs-str; + <tbody valign="top"> + <row> + <entry>__u32</entry> + <entry><structfield>index</structfield></entry> + <entry>The starting buffer index, returned by the driver.</entry> + </row> + <row> + <entry>__u32</entry> + <entry><structfield>count</structfield></entry> + <entry>The number of buffers requested or granted.</entry> + </row> + <row> + <entry>__u32</entry> + <entry><structfield>type</structfield></entry> + <entry>V4L2 buffer type: one of <constant>V4L2_BUF_TYPE_*</constant> +values.</entry> + </row> + <row> + <entry>__u32</entry> + <entry><structfield>memory</structfield></entry> + <entry>Applications set this field to +<constant>V4L2_MEMORY_MMAP</constant> or +<constant>V4L2_MEMORY_USERPTR</constant>.</entry> + </row> + <row> + <entry>__u32</entry> + <entry><structfield>fourcc</structfield></entry> + <entry>One of V4L2_PIX_FMT_* FOURCCs of the video data.</entry> + </row> + <row> + <entry>__u32</entry> + <entry><structfield>num_planes</structfield></entry> + <entry>Number of planes or 0 to use the current format.</entry> + </row> + <row> + <entry>__u32</entry> + <entry><structfield>size[VIDEO_MAX_PLANES]</structfield></entry> + <entry>Explicit sizes of buffers, being created.</entry> + </row> + <row> + <entry>__u32</entry> + <entry><structfield>reserved</structfield>[18]</entry> + <entry>A place holder for future extensions.</entry> + </row> + </tbody> + </tgroup> + </table> + </refsect1> + + <refsect1> + &return-value; + + <variablelist> + <varlistentry> + <term><errorcode>ENOMEM</errorcode></term> + <listitem> + <para>No memory to allocate buffers for <link linkend="mmap">memory +mapped</link> I/O.</para> + </listitem> + </varlistentry> + <varlistentry> + <term><errorcode>EINVAL</errorcode></term> + <listitem> + <para>The buffer type (<structfield>type</structfield> field) or the +requested I/O method (<structfield>memory</structfield>) is not +supported.</para> + </listitem> + </varlistentry> + </variablelist> + </refsect1> +</refentry> + +<!-- +Local Variables: +mode: sgml +sgml-parent-document: "v4l2.sgml" +indent-tabs-mode: nil +End: +--> 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 @@ +<refentry id="vidioc-prepare-buf"> + <refmeta> + <refentrytitle>ioctl VIDIOC_PREPARE_BUF</refentrytitle> + &manvol; + </refmeta> + + <refnamediv> + <refname>VIDIOC_PREPARE_BUF</refname> + <refpurpose>Prepare a buffer for I/O</refpurpose> + </refnamediv> + + <refsynopsisdiv> + <funcsynopsis> + <funcprototype> + <funcdef>int <function>ioctl</function></funcdef> + <paramdef>int <parameter>fd</parameter></paramdef> + <paramdef>int <parameter>request</parameter></paramdef> + <paramdef>struct v4l2_buffer *<parameter>argp</parameter></paramdef> + </funcprototype> + </funcsynopsis> + </refsynopsisdiv> + + <refsect1> + <title>Arguments</title> + + <variablelist> + <varlistentry> + <term><parameter>fd</parameter></term> + <listitem> + <para>&fd;</para> + </listitem> + </varlistentry> + <varlistentry> + <term><parameter>request</parameter></term> + <listitem> + <para>VIDIOC_PREPARE_BUF</para> + </listitem> + </varlistentry> + <varlistentry> + <term><parameter>argp</parameter></term> + <listitem> + <para></para> + </listitem> + </varlistentry> + </variablelist> + </refsect1> + + <refsect1> + <title>Description</title> + + <para>Applications can optionally call the +<constant>VIDIOC_PREPARE_BUF</constant> ioctl to pass ownership of the buffer +to the driver before actually enqueuing it, using the +<constant>VIDIOC_QBUF</constant> ioctl, and to prepare it for future I/O. +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 +<constant>V4L2_BUF_FLAG_NO_CACHE_INVALIDATE</constant> and +<constant>V4L2_BUF_FLAG_NO_CACHE_CLEAN</constant> flags to skip the respective +step.</para> + + <para>The <structname>v4l2_buffer</structname> structure is +specified in <xref linkend="buffer" />.</para> + </refsect1> + + <refsect1> + &return-value; + + <variablelist> + <varlistentry> + <term><errorcode>EBUSY</errorcode></term> + <listitem> + <para>File I/O is in progress.</para> + </listitem> + </varlistentry> + <varlistentry> + <term><errorcode>EINVAL</errorcode></term> + <listitem> + <para>The buffer <structfield>type</structfield> is not +supported, or the <structfield>index</structfield> is out of bounds, +or no buffers have been allocated yet, or the +<structfield>userptr</structfield> or +<structfield>length</structfield> are invalid.</para> + </listitem> + </varlistentry> + </variablelist> + </refsect1> +</refentry> + +<!-- +Local Variables: +mode: sgml +sgml-parent-document: "v4l2.sgml" +indent-tabs-mode: nil +End: +--> 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 /* * 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,
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 <g.liakhovetski@gmx.de> --- 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