Re: [PATCH v3] V4L: add two new ioctl()s for multi-size videobuffer management

From: Sakari Ailus <sakari.ailus@iki.fi>
To: Pawel Osciak <pawel@osciak.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>,
	Linux Media Mailing List <linux-media@vger.kernel.org>,
	Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Mauro Carvalho Chehab <mchehab@infradead.org>
Subject: Re: [PATCH v3] V4L: add two new ioctl()s for multi-size videobuffer management
Date: Wed, 3 Aug 2011 00:38:09 +0300	[thread overview]
Message-ID: <20110802213809.GT32629@valkosipuli.localdomain> (raw)
In-Reply-To: <CAMm-=zB3dOJyCy7ZhqiTQkeL2b=Dvtz8geMR8zbHYBCVR6=pEw@mail.gmail.com>

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@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>
> > ---
> >
> <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@iki.fi