On Wed, Jul 27, 2011 at 09:11:38PM -0700, Pawel Osciak wrote: > Hi Guennadi, Hi Pawel, > On Wed, Jul 20, 2011 at 01:43, Guennadi Liakhovetski > <g.liakhovetski@xxxxxx> 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@xxxxxx> > > --- > > > <snip> > > This looks nicer, I like how we got rid of destroy and gave up on > making holes, it would've given us a lot of headaches. I'm thinking > about some issues though and also have some comments/questions further > below. Holes could be re-introduced by defining DESTROY_BUFS. But it would be up to the application to use it or not. > Already mentioned by others mixing of REQBUFS and CREATE_BUFS. > Personally I'd like to allow mixing, including REQBUFS for non-zero, > because I think it would be easy to do. I think it could work in the > same way as REQBUFS for !=0 works currently (at least in vb2), if we > already have some buffers allocated and they are not in use, we free > them and a new set is allocated. So I guess it could just stay this > way. REQBUFS(0) would of course free everything. > > Passing format to CREATE_BUFS will make vb2 a bit format-aware, as it > would have to pass it forward to the driver somehow. The obvious way > would be just vb2 calling the driver's s_fmt handler, but that won't > work, as you can't pass indexes to s_fmt. So we'd have to implement a > new driver callback for setting formats per index. I guess there is no > way around it, unless we actually take the format struct out of > CREATE_BUFS and somehow do it via S_FMT. The single-planar structure > is full already though, the only way would be to use > v4l2_pix_format_mplane instead with plane count = 1 (or more if > needed). I've been pondering a while an idea to change v4l2_format. None of the structures in the union use the full size of it. This would allow adding fields to the end of the union and reduce its size accordingly. No-one would be harmed in the process, struct v4l2_format would just change slightly. Guennadi might have thought this in the past but at least I misunderstood it at the time. :-) > Another thing is the case of passing size to CREATE_BUFS. vb2, when > allocating buffers, gets their sizes from the driver (via > queue_setup), it never "suggest" any particular size. So that flow > would have to be changed as well. I guess vb2 could pass the size to > queue_setup in a similar way as it does with buffer count. This would > mean though that the ioctl would fail if the driver didn't agree to > the given size. Right now I don't see an option to "negotiate" the > size with the driver via this option. > > The more I think of it though, why do we need the size argument? It's > not needed in the existing flows (that use S_FMT) and, more > importantly, I don't think the application can know more than the > driver can, so why giving that option? The driver should know the size > for a format at least as well as the application... Also, is there a > real use case of providing just size, will the driver know which > format to use given a size? > > > > + <para>To allocate device buffers applications initialize all > > +fields of the <structname>v4l2_create_buffers</structname> structure. > > +They set the <structfield>type</structfield> field in the > > +<structname>v4l2_format</structname> structure, embedded in this > > +structure, 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. Applications have two possibilities to specify the size of buffers > > +to be prepared: they can either set the <structfield>size</structfield> > > +field explicitly to a non-zero value, or fill in the frame format data in the > > +<structfield>format</structfield> field. In the latter case buffer sizes > > +will be calculated automatically by the driver. The > > Technically, we shouldn't say "applications initialize all fields" in > the first sentence if they do not have to initialize both size and > format fields at the same time. > > > +<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. > > +<structfield>count</structfield> can be smaller than the number requested. > > +-ENOMEM is returned, if the driver runs out of free memory.</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>&v4l2-memory;</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>size</structfield></entry> > > + <entry>Explicit size of buffers, being created.</entry> > > + </row> > > + <row> > > + <entry>&v4l2-format;</entry> > > + <entry><structfield>format</structfield></entry> > > + <entry>Application has to set the <structfield>type</structfield> > > +field, other fields should be used, if the application wants to allocate buffers > > +for a specific frame format.</entry> > > + </row> > > + <row> > > + <entry>__u32</entry> > > + <entry><structfield>reserved</structfield>[8]</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> > > + > > What happens if the driver does not agree to the provided size? EINVAL? > > > +<!-- > > +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> > > + > > I'm probably forgetting something, but why would we want to do both > PREPARE_BUF and QBUF? Why not queue in advance? > Could you give a full sequence of ioctls how it will be used > (including streamons, etc.)? I'm trying to picture how passing of both > types of buffers will have to look like between vb2 and a driver. > > <snip> > > -- > Best regards, > Pawel Osciak -- Sakari Ailus sakari.ailus@xxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
