On Tue August 14 2012 17:34:31 Tomasz Stanislawski wrote:

From: Sumit Semwal sumit.semwal@ti.com

Adds DMABUF memory type to v4l framework. Also adds the related file descriptor in v4l2_plane and v4l2_buffer.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com [original work in the PoC for buffer sharing] Signed-off-by: Sumit Semwal sumit.semwal@ti.com Signed-off-by: Sumit Semwal sumit.semwal@linaro.org Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/media/video/v4l2-compat-ioctl32.c | 18 ++++++++++++++++++ drivers/media/video/v4l2-ioctl.c | 1 + include/linux/videodev2.h | 7 +++++++ 3 files changed, 26 insertions(+)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c index 9ebd5c5..a2e0549 100644 --- a/drivers/media/video/v4l2-compat-ioctl32.c +++ b/drivers/media/video/v4l2-compat-ioctl32.c @@ -304,6 +304,7 @@ struct v4l2_plane32 { union { __u32 mem_offset; compat_long_t userptr;

• __u32		fd;
  

Shouldn't this be int?

} m; __u32 data_offset; __u32 reserved[11]; @@ -325,6 +326,7 @@ struct v4l2_buffer32 { __u32 offset; compat_long_t userptr; compat_caddr_t planes;

• __u32		fd;
  

Ditto.

} m; __u32 length; __u32 reserved2; @@ -348,6 +350,9 @@ static int get_v4l2_plane32(struct v4l2_plane *up, struct v4l2_plane32 *up32, up_pln = compat_ptr(p); if (put_user((unsigned long)up_pln, &up->m.userptr)) return -EFAULT;

• } else if (memory == V4L2_MEMORY_DMABUF) {

• if (copy_in_user(&up->m.fd, &up32->m.fd, sizeof(int)))
  

• 	return -EFAULT;
  

  } else { if (copy_in_user(&up->m.mem_offset, &up32->m.mem_offset, sizeof(__u32)))

@@ -371,6 +376,11 @@ static int put_v4l2_plane32(struct v4l2_plane *up, struct v4l2_plane32 *up32, if (copy_in_user(&up32->m.mem_offset, &up->m.mem_offset, sizeof(__u32))) return -EFAULT;

• /* For DMABUF, driver might've set up the fd, so copy it back. */

• if (memory == V4L2_MEMORY_DMABUF)

• if (copy_in_user(&up32->m.fd, &up->m.fd,
  

• 			sizeof(int)))
  

• 	return -EFAULT;
  
  

  return 0;

} @@ -453,6 +463,10 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user if (get_user(kp->m.offset, &up->m.offset)) return -EFAULT; break;

• case V4L2_MEMORY_DMABUF:
  

• 	if (get_user(kp->m.fd, &up->m.fd))
  

• 		return -EFAULT;
  

• 	break;
  

  } }

@@ -517,6 +531,10 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user if (put_user(kp->m.offset, &up->m.offset)) return -EFAULT; break;

• case V4L2_MEMORY_DMABUF:
  

• 	if (put_user(kp->m.fd, &up->m.fd))
  

• 		return -EFAULT;
  

• 	break;
  

  } }

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 6bc47fc..dffd3c9 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -155,6 +155,7 @@ static const char *v4l2_memory_names[] = { [V4L2_MEMORY_MMAP] = "mmap", [V4L2_MEMORY_USERPTR] = "userptr", [V4L2_MEMORY_OVERLAY] = "overlay",

• [V4L2_MEMORY_DMABUF] = "dmabuf",

};

#define prt_names(a, arr) ((((a) >= 0) && ((a) < ARRAY_SIZE(arr))) ? \ diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 7a147c8..7f918dc 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -186,6 +186,7 @@ enum v4l2_memory { V4L2_MEMORY_MMAP = 1, V4L2_MEMORY_USERPTR = 2, V4L2_MEMORY_OVERLAY = 3,

• V4L2_MEMORY_DMABUF = 4,

};

/* see also http://vektor.theorem.ca/graphics/ycbcr/ */ @@ -596,6 +597,8 @@ struct v4l2_requestbuffers {

• 	should be passed to mmap() called on the video node)
  

• @userptr: when memory is V4L2_MEMORY_USERPTR, a userspace pointer

• 	pointing to this plane
  

• • @fd: when memory is V4L2_MEMORY_DMABUF, a userspace file

• • 	descriptor associated with this plane
    

  • @data_offset: offset in the plane to the start of data; usually 0,

  • 	unless there is a header in front of the data
    

@@ -610,6 +613,7 @@ struct v4l2_plane { union { __u32 mem_offset; unsigned long userptr;

• int		fd;
  

  } m; __u32 data_offset; __u32 reserved[11];

@@ -634,6 +638,8 @@ struct v4l2_plane {

• (or a "cookie" that should be passed to mmap() as offset)
  

• @userptr: for non-multiplanar buffers with memory == V4L2_MEMORY_USERPTR;

• a userspace pointer pointing to this buffer
  

• • @fd: for non-multiplanar buffers with memory == V4L2_MEMORY_DMABUF;

• • a userspace file descriptor associated with this buffer
    

  • @planes: for multiplanar buffers; userspace pointer to the array of plane

  • info structs for this buffer
    

  • @length: size in bytes of the buffer (NOT its payload) for single-plane

@@ -660,6 +666,7 @@ struct v4l2_buffer { __u32 offset; unsigned long userptr; struct v4l2_plane *planes;

• int		fd;
  

  } m; __u32 length; __u32 reserved2;

Regards,

Hans

On Wed August 22 2012 14:09:18 Tomasz Stanislawski wrote:

Hi Hans, Thank your for the review. Please refer to the comments below.

On 08/22/2012 12:27 PM, Hans Verkuil wrote:

...

On Tue August 14 2012 17:34:31 Tomasz Stanislawski wrote:

...

From: Sumit Semwal sumit.semwal@ti.com

Adds DMABUF memory type to v4l framework. Also adds the related file descriptor in v4l2_plane and v4l2_buffer.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com [original work in the PoC for buffer sharing] Signed-off-by: Sumit Semwal sumit.semwal@ti.com Signed-off-by: Sumit Semwal sumit.semwal@linaro.org Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/media/video/v4l2-compat-ioctl32.c | 18 ++++++++++++++++++ drivers/media/video/v4l2-ioctl.c | 1 + include/linux/videodev2.h | 7 +++++++ 3 files changed, 26 insertions(+)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c index 9ebd5c5..a2e0549 100644 --- a/drivers/media/video/v4l2-compat-ioctl32.c +++ b/drivers/media/video/v4l2-compat-ioctl32.c @@ -304,6 +304,7 @@ struct v4l2_plane32 { union { __u32 mem_offset; compat_long_t userptr;

• __u32		fd;
  

Shouldn't this be int?

Notice that this field should be consistent with fd field used in 'struct v4l2_exportbuffer'. Therefore I prefer to use fixed-size types. One could use __s32 here but notice that file descriptors are defined as small, nonnegative integers according to POSIX spec. The type __u32 suits well for this purpose. The negative values returned by open syscall are used only to indicate failures.

On the other hand, using __s32 may help to avoid compiler warning while building userspace apps due to 'signed-vs-unsigned comparisons'.

However, I do not have any strong opinion about 'int vs __u32' issue :). Do you think that using __s32 for both QUERYBUF and EXPBUF is a good compromise?

The type of a fd is highly variable in the kernel. Just try this for fun:

grep [^a-z]fd[^a-z] -rsI include/linux/

'int' or 'unsigned int' are by far the most common types.

But in structs I did see a few __s32 types, so I think __s32 is a decent type to use. Just make sure it is __s32 everywhere. Right now __u32 and int are both used.

Regards,

Hans

...

...

} m; __u32 data_offset; __u32 reserved[11]; @@ -325,6 +326,7 @@ struct v4l2_buffer32 { __u32 offset; compat_long_t userptr; compat_caddr_t planes;

• __u32		fd;
  

Ditto.

...

} m; __u32 length; __u32 reserved2;

...

Regards,

Hans

Regards,

Tomasz

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 August 14 2012 17:34:32 Tomasz Stanislawski wrote:

This patch adds description and usage examples for importing DMABUF file descriptor in V4L2.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: linux-doc@vger.kernel.org

---

Documentation/DocBook/media/v4l/compat.xml | 4 + Documentation/DocBook/media/v4l/io.xml | 180 ++++++++++++++++++++ .../DocBook/media/v4l/vidioc-create-bufs.xml | 3 +- Documentation/DocBook/media/v4l/vidioc-qbuf.xml | 15 ++ Documentation/DocBook/media/v4l/vidioc-reqbufs.xml | 47 ++--- 5 files changed, 226 insertions(+), 23 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml index 98e8d08..ff45330 100644 --- a/Documentation/DocBook/media/v4l/compat.xml +++ b/Documentation/DocBook/media/v4l/compat.xml @@ -2605,6 +2605,10 @@ ioctls.</para> <listitem> <para>Support for frequency band enumeration: &VIDIOC-ENUM-FREQ-BANDS; ioctl.</para> </listitem>

•    <listitem>
  

•  <para>Importing DMABUF file descriptors as a new IO method described
  

•  in <xref linkend="dmabuf" />.</para>
  

•    </listitem>
   </itemizedlist>
  

  </section>

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml index 1885cc0..98253ee 100644 --- a/Documentation/DocBook/media/v4l/io.xml +++ b/Documentation/DocBook/media/v4l/io.xml @@ -472,6 +472,163 @@ rest should be evident.</para> </footnote></para>

</section>

• <section id="dmabuf">

• <title>Streaming I/O (DMA buffer importing)</title>

• <note>

•  <title>Experimental</title>
  

•  <para>This is an <link linkend="experimental"> experimental </link>
  

•  interface and may change in the future.</para>
  

• </note>

+<para>The DMABUF framework provides a generic mean for sharing buffers between

s/mean/method/

• multiple devices. Device drivers that support DMABUF can export a DMA buffer

+to userspace as a file descriptor (known as the exporter role), import a DMA +buffer from userspace using a file descriptor previously exported for a +different or the same device (known as the importer role), or both. This +section describes the DMABUF importer role API in V4L2.</para>

+<para>Input and output devices support the streaming I/O method when the +<constant>V4L2_CAP_STREAMING</constant> flag in the +<structfield>capabilities</structfield> field of &v4l2-capability; returned by +the &VIDIOC-QUERYCAP; ioctl is set. Whether importing DMA buffers through +DMABUF file descriptors is supported is determined by calling the +&VIDIOC-REQBUFS; ioctl with the memory type set to +<constant>V4L2_MEMORY_DMABUF</constant>.</para>

• <para>This I/O method is dedicated for sharing DMA buffers between V4L and

+other APIs. Buffers (planes) are allocated by a driver on behalf of the +application, and exported to the application as file descriptors using an API +specific to the allocator driver. Only those file descriptor are exchanged, +these files and meta-information are passed in &v4l2-buffer; (or in

This sentence doesn't work well. It's unclear what is meant by 'these files'. Do you mean 'these file descriptors'?

+&v4l2-plane; in the multi-planar API case). The driver must be switched into +DMABUF I/O mode by calling the &VIDIOC-REQBUFS; with the desired buffer type. +No buffers (planes) are allocated beforehand, consequently they are not indexed +and cannot be queried like mapped buffers with the +<constant>VIDIOC_QUERYBUF</constant> ioctl.</para>

I disagree with that. Userptr buffers can use QUERYBUF just fine. Even for the userptr you still have to fill in the buffer index when calling QBUF.

So I see no reason why you couldn't use QUERYBUF in the DMABUF case. The only difference is that the fd field is undefined (set to -1 perhaps?) if the bufffer isn't queued.

QUERYBUF can be very useful for debugging, for example to see what the status is of each buffer and how many are queued.

• <example>

•  <title>Initiating streaming I/O with DMABUF file descriptors</title>
  

•  <programlisting>
  

+&v4l2-requestbuffers; reqbuf;

+memset (&amp;reqbuf, 0, sizeof (reqbuf)); +reqbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; +reqbuf.memory = V4L2_MEMORY_DMABUF; +reqbuf.count = 1;

+if (ioctl (fd, &VIDIOC-REQBUFS;, &amp;reqbuf) == -1) {

• if (errno == EINVAL)

• printf ("Video capturing or DMABUF streaming is not supported\n");
  

• else

• perror ("VIDIOC_REQBUFS");
  

• exit (EXIT_FAILURE);

Let's stick to the kernel coding style, so no ' ' before '(' in function calls. Same for the other program examples below.

+}

•  </programlisting>
  

• </example>

• <para>Buffer (plane) file descriptor is passed on the fly with the

s/Buffer/The buffer/

+&VIDIOC-QBUF; ioctl. In case of multiplanar buffers, every plane can be

'Can be', 'should be' or 'must be'? Does it ever make sense to have the same fd for different planes? Do we have restrictions on this in the userptr case?

+associated with a different DMABUF descriptor. Although buffers are commonly +cycled, applications can pass a different DMABUF descriptor at each +<constant>VIDIOC_QBUF</constant> call.</para>

• <example>

•  <title>Queueing DMABUF using single plane API</title>
  

•  <programlisting>
  

+int buffer_queue(int v4lfd, int index, int dmafd) +{

• &v4l2-buffer; buf;
• memset(&amp;buf, 0, sizeof buf);
• buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
• buf.memory = V4L2_MEMORY_DMABUF;
• buf.index = index;
• buf.m.fd = dmafd;
• if (ioctl (v4lfd, &VIDIOC-QBUF;, &amp;buf) == -1) {

• perror ("VIDIOC_QBUF");
  

• return -1;
  

• }
• return 0;

+}

•  </programlisting>
  

• </example>

• <example>

•  <title>Queueing DMABUF using multi plane API</title>
  

•  <programlisting>
  

+int buffer_queue_mp(int v4lfd, int index, int dmafd[], int n_planes) +{

• &v4l2-buffer; buf;
• &v4l2-plane; planes[VIDEO_MAX_PLANES];
• int i;
• memset(&amp;buf, 0, sizeof buf);
• buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
• buf.memory = V4L2_MEMORY_DMABUF;
• buf.index = index;
• buf.m.planes = planes;
• buf.length = n_planes;
• memset(&amp;planes, 0, sizeof planes);
• for (i = 0; i &lt; n_planes; ++i)

• buf.m.planes[i].m.fd = dmafd[i];
  

• if (ioctl (v4lfd, &VIDIOC-QBUF;, &amp;buf) == -1) {

• perror ("VIDIOC_QBUF");
  

• return -1;
  

• }
• return 0;

+}

•  </programlisting>
  

• </example>

• <para>Filled or displayed buffers are dequeued with the

+&VIDIOC-DQBUF; ioctl. The driver can unlock the buffer at any +time between the completion of the DMA and this ioctl. The memory is +also unlocked when &VIDIOC-STREAMOFF; is called, &VIDIOC-REQBUFS;, or +when the device is closed.</para>

• <para>For capturing applications it is customary to enqueue a

+number of empty buffers, to start capturing and enter the read loop. +Here the application waits until a filled buffer can be dequeued, and +re-enqueues the buffer when the data is no longer needed. Output +applications fill and enqueue buffers, when enough buffers are stacked +up output is started. In the write loop, when the application +runs out of free buffers it must wait until an empty buffer can be +dequeued and reused. Two methods exist to suspend execution of the +application until one or more buffers can be dequeued. By default +<constant>VIDIOC_DQBUF</constant> blocks when no buffer is in the +outgoing queue. When the <constant>O_NONBLOCK</constant> flag was +given to the &func-open; function, <constant>VIDIOC_DQBUF</constant> +returns immediately with an &EAGAIN; when no buffer is available. The +&func-select; or &func-poll; function are always available.</para>

s/function/functions/

• <para>To start and stop capturing or output applications call the

+&VIDIOC-STREAMON; and &VIDIOC-STREAMOFF; ioctls. Note that +<constant>VIDIOC_STREAMOFF</constant> removes all buffers from both queues and +unlocks all buffers as a side effect. Since there is no notion of doing +anything "now" on a multitasking system, if an application needs to synchronize +with another event it should examine the &v4l2-buffer; +<structfield>timestamp</structfield> of captured buffers, or set the field +before enqueuing buffers for output.</para>

• <para>Drivers implementing DMABUF importing I/O must support the

+<constant>VIDIOC_REQBUFS</constant>, <constant>VIDIOC_QBUF</constant>, +<constant>VIDIOC_DQBUF</constant>, <constant>VIDIOC_STREAMON</constant> and +<constant>VIDIOC_STREAMOFF</constant> ioctls, and the +<function>select()</function> and <function>poll()</function> functions.</para>

• </section>

• <section id="async"> <title>Asynchronous I/O</title>

@@ -673,6 +830,14 @@ memory, set by the application. See <xref linkend="userp" /> for details. <structname>v4l2_buffer</structname> structure.</entry> </row> <row>

•    <entry></entry>
  

•    <entry>int</entry>
  

•    <entry><structfield>fd</structfield></entry>
  

•    <entry>For the single-plane API and when
  

+<structfield>memory</structfield> is <constant>V4L2_MEMORY_DMABUF</constant> this +is the file descriptor associated with a DMABUF buffer.</entry>

•  </row>
  

•  <row>
   <entry>__u32</entry>
   <entry><structfield>length</structfield></entry>
   <entry></entry>
  

@@ -746,6 +911,15 @@ should set this to 0.</entry> </entry> </row> <row>

•    <entry></entry>
  

•    <entry>int</entry>
  

•    <entry><structfield>fd</structfield></entry>
  

•    <entry>When the memory type in the containing &v4l2-buffer; is
  

• <constant>V4L2_MEMORY_DMABUF</constant>, this is a file
  

• descriptor associated with a DMABUF buffer, similar to the
  

• <structfield>fd</structfield> field in &v4l2-buffer;.</entry>
  

•  </row>
  

•  <row>
   <entry>__u32</entry>
   <entry><structfield>data_offset</structfield></entry>
   <entry></entry>
  

@@ -973,6 +1147,12 @@ pointer</link> I/O.</entry> <entry>3</entry> <entry>[to do]</entry> </row>

•  <row>
  

•    <entry><constant>V4L2_MEMORY_DMABUF</constant></entry>
  

•    <entry>4</entry>
  

•    <entry>The buffer is used for <link linkend="dmabuf">DMA shared
  

+buffer</link> I/O.</entry>

•  </row>
  

  </tbody> </tgroup> </table>

diff --git a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml index a8cda1a..1125468 100644 --- a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml +++ b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml @@ -109,7 +109,8 @@ information.</para> <entry>__u32</entry> <entry><structfield>memory</structfield></entry> <entry>Applications set this field to -<constant>V4L2_MEMORY_MMAP</constant> or +<constant>V4L2_MEMORY_MMAP</constant>, +<constant>V4L2_MEMORY_DMABUF</constant> or <constant>V4L2_MEMORY_USERPTR</constant>. See <xref linkend="v4l2-memory" /></entry> </row> diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml index 77ff5be..436d21c 100644 --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml @@ -109,6 +109,21 @@ they cannot be swapped out to disk. Buffers remain locked until dequeued, until the &VIDIOC-STREAMOFF; or &VIDIOC-REQBUFS; ioctl is called, or until the device is closed.</para>

• <para>To enqueue a <link linkend="dmabuf">DMABUF</link> buffer applications

+set the <structfield>memory</structfield> field to +<constant>V4L2_MEMORY_DMABUF</constant> and the <structfield>m.fd</structfield> +field to a file descriptor associated with a DMABUF buffer. When the +multi-planar API is used <structfield>m.fd</structfield> of the passed array of

multi-planar API is used the <structfield>m.fd</structfield> fields of the passed array of

+&v4l2-plane; have to be used instead. When <constant>VIDIOC_QBUF</constant> is +called with a pointer to this structure the driver sets the +<constant>V4L2_BUF_FLAG_QUEUED</constant> flag and clears the +<constant>V4L2_BUF_FLAG_MAPPED</constant> and +<constant>V4L2_BUF_FLAG_DONE</constant> flags in the +<structfield>flags</structfield> field, or it returns an error code. This +ioctl locks the buffer. Buffers remain locked until dequeued, until the +&VIDIOC-STREAMOFF; or &VIDIOC-REQBUFS; ioctl is called, or until the device is +closed.</para>

You need to explain what a 'locked buffer' means.

• <para>Applications call the <constant>VIDIOC_DQBUF</constant>

ioctl to dequeue a filled (capturing) or displayed (output) buffer from the driver's outgoing queue. They just set the diff --git a/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml b/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml index d7c9505..20f4323 100644 --- a/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml +++ b/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml @@ -48,28 +48,30 @@

<refsect1> <title>Description</title>

• <para>This ioctl is used to initiate <link linkend="mmap">memory

-mapped</link> or <link linkend="userp">user pointer</link> -I/O. Memory mapped buffers are located in device memory and must be -allocated with this ioctl before they can be mapped into the -application's address space. User buffers are allocated by -applications themselves, and this ioctl is merely used to switch the -driver into user pointer I/O mode and to setup some internal structures.</para> +<para>This ioctl is used to initiate <link linkend="mmap">memory mapped</link>, +<link linkend="userp">user pointer</link> or <link +linkend="dmabuf">DMABUF</link> based I/O. Memory mapped buffers are located in +device memory and must be allocated with this ioctl before they can be mapped +into the application's address space. User buffers are allocated by +applications themselves, and this ioctl is merely used to switch the driver +into user pointer I/O mode and to setup some internal structures. +Similarly, DMABUF buffers are allocated by applications through a device +driver, and this ioctl only configures the driver into DMABUF I/O mode without +performing any direct allocation.</para>

• <para>To allocate device buffers applications initialize all

-fields of the <structname>v4l2_requestbuffers</structname> structure. -They set the <structfield>type</structfield> field to the respective -stream or buffer type, the <structfield>count</structfield> field to -the desired number of buffers, <structfield>memory</structfield> -must be set to the requested I/O method and 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 -the requested number of buffers and it stores the actual number -allocated in the <structfield>count</structfield> field. It can be -smaller than the number requested, even zero, when the driver runs out -of free memory. A larger number is also possible when the driver requires -more buffers to function correctly. For example video output requires at least two buffers, -one displayed and one filled by the application.</para>

• <para>To allocate device buffers applications initialize all fields of the

+<structname>v4l2_requestbuffers</structname> structure. They set the +<structfield>type</structfield> field to the respective stream or buffer type, +the <structfield>count</structfield> field to the desired number of buffers, +<structfield>memory</structfield> must be set to the requested I/O method and +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 the +requested number of buffers and it stores the actual number allocated in the +<structfield>count</structfield> field. It can be smaller than the number +requested, even zero, when the driver runs out of free memory. A larger number +is also possible when the driver requires more buffers to function correctly. +For example video output requires at least two buffers, one displayed and one +filled by the application.</para> <para>When the I/O method is not supported the ioctl returns an &EINVAL;.</para>

@@ -102,7 +104,8 @@ as the &v4l2-format; <structfield>type</structfield> field. See <xref <entry>__u32</entry> <entry><structfield>memory</structfield></entry> <entry>Applications set this field to -<constant>V4L2_MEMORY_MMAP</constant> or +<constant>V4L2_MEMORY_MMAP</constant>, +<constant>V4L2_MEMORY_DMABUF</constant> or <constant>V4L2_MEMORY_USERPTR</constant>. See <xref linkend="v4l2-memory" />.</entry> </row>

You also have to update the VIDIOC_CREATE_BUFS ioctl documentation!

I think the VIDIOC_PREPARE_BUF ioctl documentation is OK, but you might want to check that yourself, just in case.

Regards,

Hans

On Mon 24 September 2012 14:17:00 Tomasz Stanislawski wrote:

Hi Hans, Thank you for review.

On 08/22/2012 12:47 PM, Hans Verkuil wrote:

...

On Tue August 14 2012 17:34:32 Tomasz Stanislawski wrote:

...

This patch adds description and usage examples for importing DMABUF file descriptor in V4L2.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: linux-doc@vger.kernel.org

---

Documentation/DocBook/media/v4l/compat.xml | 4 + Documentation/DocBook/media/v4l/io.xml | 180 ++++++++++++++++++++ .../DocBook/media/v4l/vidioc-create-bufs.xml | 3 +- Documentation/DocBook/media/v4l/vidioc-qbuf.xml | 15 ++ Documentation/DocBook/media/v4l/vidioc-reqbufs.xml | 47 ++--- 5 files changed, 226 insertions(+), 23 deletions(-)

[snip]

...

...

+&v4l2-plane; in the multi-planar API case). The driver must be switched into +DMABUF I/O mode by calling the &VIDIOC-REQBUFS; with the desired buffer type. +No buffers (planes) are allocated beforehand, consequently they are not indexed +and cannot be queried like mapped buffers with the +<constant>VIDIOC_QUERYBUF</constant> ioctl.</para>

I disagree with that. Userptr buffers can use QUERYBUF just fine. Even for the userptr you still have to fill in the buffer index when calling QBUF.

So I see no reason why you couldn't use QUERYBUF in the DMABUF case. The only difference is that the fd field is undefined (set to -1 perhaps?) if the bufffer isn't queued.

QUERYBUF can be very useful for debugging, for example to see what the status is of each buffer and how many are queued.

Ok. I agree that QUERYBUF can be useful for debugging. The value of fd field should be the last value passed using QBUF. It would simplify streaming because an application would not have to keep the file descriptor around.

If a buffer isn't queued, then fd really is undefined. I don't see any advantage of remembering the last value used there (and after a REQBUFS there isn't a 'last value' anyway).

...

...

• <example>

•  <title>Initiating streaming I/O with DMABUF file descriptors</title>
  

•  <programlisting>
  

+&v4l2-requestbuffers; reqbuf;

+memset (&amp;reqbuf, 0, sizeof (reqbuf)); +reqbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; +reqbuf.memory = V4L2_MEMORY_DMABUF; +reqbuf.count = 1;

+if (ioctl (fd, &VIDIOC-REQBUFS;, &amp;reqbuf) == -1) {

• if (errno == EINVAL)

• printf ("Video capturing or DMABUF streaming is not supported\n");
  

• else

• perror ("VIDIOC_REQBUFS");
  

• exit (EXIT_FAILURE);

Let's stick to the kernel coding style, so no ' ' before '(' in function calls. Same for the other program examples below.

The ' ' before function was used for userptr and mmap usage examples. These examples should be fixed too.

...

...

+}

•  </programlisting>
  

• </example>

• <para>Buffer (plane) file descriptor is passed on the fly with the

s/Buffer/The buffer/

...

+&VIDIOC-QBUF; ioctl. In case of multiplanar buffers, every plane can be

'Can be', 'should be' or 'must be'? Does it ever make sense to have the same fd for different planes? Do we have restrictions on this in the userptr case?

I think that we should keep to 'can be'. I see no good reason to prevent the same dmabuf to be used for different planes. Allowing reusing of dmabufs with assistance of data_offset field would allow to pass a 2-planar YUV420 from V4L2-single-plane API to a driver with V4L2-multi-plane API.

That's true.

[snip]

...

...

diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml index 77ff5be..436d21c 100644 --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml @@ -109,6 +109,21 @@ they cannot be swapped out to disk. Buffers remain locked until dequeued, until the &VIDIOC-STREAMOFF; or &VIDIOC-REQBUFS; ioctl is called, or until the device is closed.</para>

• <para>To enqueue a <link linkend="dmabuf">DMABUF</link> buffer applications

+set the <structfield>memory</structfield> field to +<constant>V4L2_MEMORY_DMABUF</constant> and the <structfield>m.fd</structfield> +field to a file descriptor associated with a DMABUF buffer. When the +multi-planar API is used <structfield>m.fd</structfield> of the passed array of

multi-planar API is used the <structfield>m.fd</structfield> fields of the passed array of

...

+&v4l2-plane; have to be used instead. When <constant>VIDIOC_QBUF</constant> is +called with a pointer to this structure the driver sets the +<constant>V4L2_BUF_FLAG_QUEUED</constant> flag and clears the +<constant>V4L2_BUF_FLAG_MAPPED</constant> and +<constant>V4L2_BUF_FLAG_DONE</constant> flags in the

What does it do to the PREPARED flag I wonder?

...

...

+<structfield>flags</structfield> field, or it returns an error code. This +ioctl locks the buffer. Buffers remain locked until dequeued, until the +&VIDIOC-STREAMOFF; or &VIDIOC-REQBUFS; ioctl is called, or until the device is +closed.</para>

You need to explain what a 'locked buffer' means.

I propose following definition: "Locking a buffer means passing it to a driver for an access by hardware. "If an application accesses (reads/writes) a locked buffer then the result is undefined."

What is your opinion about proposed definition?

Sounds good.

...

...

• <para>Applications call the <constant>VIDIOC_DQBUF</constant>

ioctl to dequeue a filled (capturing) or displayed (output) buffer from the driver's outgoing queue. They just set the diff --git a/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml b/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml index d7c9505..20f4323 100644 --- a/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml +++ b/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml @@ -48,28 +48,30 @@

<refsect1> <title>Description</title>

• <para>This ioctl is used to initiate <link linkend="mmap">memory

-mapped</link> or <link linkend="userp">user pointer</link> -I/O. Memory mapped buffers are located in device memory and must be -allocated with this ioctl before they can be mapped into the -application's address space. User buffers are allocated by -applications themselves, and this ioctl is merely used to switch the -driver into user pointer I/O mode and to setup some internal structures.</para> +<para>This ioctl is used to initiate <link linkend="mmap">memory mapped</link>, +<link linkend="userp">user pointer</link> or <link +linkend="dmabuf">DMABUF</link> based I/O. Memory mapped buffers are located in +device memory and must be allocated with this ioctl before they can be mapped +into the application's address space. User buffers are allocated by +applications themselves, and this ioctl is merely used to switch the driver +into user pointer I/O mode and to setup some internal structures. +Similarly, DMABUF buffers are allocated by applications through a device +driver, and this ioctl only configures the driver into DMABUF I/O mode without +performing any direct allocation.</para>

• <para>To allocate device buffers applications initialize all

-fields of the <structname>v4l2_requestbuffers</structname> structure. -They set the <structfield>type</structfield> field to the respective -stream or buffer type, the <structfield>count</structfield> field to -the desired number of buffers, <structfield>memory</structfield> -must be set to the requested I/O method and 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 -the requested number of buffers and it stores the actual number -allocated in the <structfield>count</structfield> field. It can be -smaller than the number requested, even zero, when the driver runs out -of free memory. A larger number is also possible when the driver requires -more buffers to function correctly. For example video output requires at least two buffers, -one displayed and one filled by the application.</para>

• <para>To allocate device buffers applications initialize all fields of the

+<structname>v4l2_requestbuffers</structname> structure. They set the +<structfield>type</structfield> field to the respective stream or buffer type, +the <structfield>count</structfield> field to the desired number of buffers, +<structfield>memory</structfield> must be set to the requested I/O method and +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 the +requested number of buffers and it stores the actual number allocated in the +<structfield>count</structfield> field. It can be smaller than the number +requested, even zero, when the driver runs out of free memory. A larger number +is also possible when the driver requires more buffers to function correctly. +For example video output requires at least two buffers, one displayed and one +filled by the application.</para> <para>When the I/O method is not supported the ioctl returns an &EINVAL;.</para>

@@ -102,7 +104,8 @@ as the &v4l2-format; <structfield>type</structfield> field. See <xref <entry>__u32</entry> <entry><structfield>memory</structfield></entry> <entry>Applications set this field to -<constant>V4L2_MEMORY_MMAP</constant> or +<constant>V4L2_MEMORY_MMAP</constant>, +<constant>V4L2_MEMORY_DMABUF</constant> or <constant>V4L2_MEMORY_USERPTR</constant>. See <xref linkend="v4l2-memory" />.</entry> </row>

You also have to update the VIDIOC_CREATE_BUFS ioctl documentation!

I think the VIDIOC_PREPARE_BUF ioctl documentation is OK, but you might want to check that yourself, just in case.

Ok. Regards, Tomasz Stanislawski

...

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 Tomasz,

Thanks for the patch.

On Tuesday 14 August 2012 17:34:40 Tomasz Stanislawski wrote:

From: Marek Szyprowski m.szyprowski@samsung.com

Add prepare/finish callbacks to vb2-dma-contig allocator.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

As for v7,

Laurent Pinchart laurent.pinchart@ideasonboard.com

:-)

---

drivers/media/video/videobuf2-dma-contig.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 8486e06..494a824 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -103,6 +103,28 @@ static unsigned int vb2_dc_num_users(void *buf_priv) return atomic_read(&buf->refcount); }

+static void vb2_dc_prepare(void *buf_priv) +{

• struct vb2_dc_buf *buf = buf_priv;
• struct sg_table *sgt = buf->dma_sgt;
• if (!sgt)

• return;
  

• dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);

+}

+static void vb2_dc_finish(void *buf_priv) +{

• struct vb2_dc_buf *buf = buf_priv;
• struct sg_table *sgt = buf->dma_sgt;
• if (!sgt)

• return;
  

• dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);

+}

/*********************************************/ /* callbacks for MMAP buffers */ /*********************************************/ @@ -366,6 +388,8 @@ const struct vb2_mem_ops vb2_dma_contig_memops = { .mmap = vb2_dc_mmap, .get_userptr = vb2_dc_get_userptr, .put_userptr = vb2_dc_put_userptr,

• .prepare = vb2_dc_prepare,
• .finish = vb2_dc_finish, .num_users = vb2_dc_num_users,

}; EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);

On Tue August 14 2012 17:34:43 Tomasz Stanislawski wrote:

This patch enhances VIVI driver with a support for importing a buffer from DMABUF file descriptors.

Thanks for adding DMABUF support to vivi.

What would be great is if DMABUF support is also added to mem2mem_testdev. It would make an excellent test case to take the vivi output, pass it through mem2mem_testdev, and finally output the image using the gpu, all using dmabuf.

It's also very useful for application developers to test dmabuf support without requiring special hardware (other than a dmabuf-enabled gpu driver).

Regards,

Hans

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com

---

drivers/media/video/Kconfig | 1 + drivers/media/video/vivi.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 966954d..8fa81be 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -653,6 +653,7 @@ config VIDEO_VIVI depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE select FONT_8x16 select VIDEOBUF2_VMALLOC

• select DMA_SHARED_BUFFER default n ---help--- Enables a virtual video driver. This device shows a color bar

diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c index a6351c4..37d8fd4 100644 --- a/drivers/media/video/vivi.c +++ b/drivers/media/video/vivi.c @@ -1308,7 +1308,7 @@ static int __init vivi_create_instance(int inst) q = &dev->vb_vidq; memset(q, 0, sizeof(dev->vb_vidq)); q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;

• q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ;

• q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ; q->drv_priv = dev; q->buf_struct_size = sizeof(struct vivi_buffer); q->ops = &vivi_video_qops;

On Wed August 22 2012 12:56:30 Hans Verkuil wrote:

On Tue August 14 2012 17:34:43 Tomasz Stanislawski wrote:

...

This patch enhances VIVI driver with a support for importing a buffer from DMABUF file descriptors.

Thanks for adding DMABUF support to vivi.

What would be great is if DMABUF support is also added to mem2mem_testdev. It would make an excellent test case to take the vivi output, pass it through mem2mem_testdev, and finally output the image using the gpu, all using dmabuf.

It's also very useful for application developers to test dmabuf support without requiring special hardware (other than a dmabuf-enabled gpu driver).

Adding VIDIOC_EXPBUF support to vivi and mem2mem_testdev would be welcome as well for the same reasons.

Regards,

Hans

Regards,

Hans

...

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com

---

drivers/media/video/Kconfig | 1 + drivers/media/video/vivi.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 966954d..8fa81be 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -653,6 +653,7 @@ config VIDEO_VIVI depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE select FONT_8x16 select VIDEOBUF2_VMALLOC

• select DMA_SHARED_BUFFER default n ---help--- Enables a virtual video driver. This device shows a color bar

diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c index a6351c4..37d8fd4 100644 --- a/drivers/media/video/vivi.c +++ b/drivers/media/video/vivi.c @@ -1308,7 +1308,7 @@ static int __init vivi_create_instance(int inst) q = &dev->vb_vidq; memset(q, 0, sizeof(dev->vb_vidq)); q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;

• q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_READ;

• q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ; q->drv_priv = dev; q->buf_struct_size = sizeof(struct vivi_buffer); q->ops = &vivi_video_qops;

-- 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 August 14 2012 17:34:47 Tomasz Stanislawski wrote:

This patch adds description and usage examples for exporting DMABUF file descriptor in V4L2.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: linux-doc@vger.kernel.org

---

Documentation/DocBook/media/v4l/compat.xml | 3 + Documentation/DocBook/media/v4l/io.xml | 3 + Documentation/DocBook/media/v4l/v4l2.xml | 1 + Documentation/DocBook/media/v4l/vidioc-expbuf.xml | 223 +++++++++++++++++++++ 4 files changed, 230 insertions(+) create mode 100644 Documentation/DocBook/media/v4l/vidioc-expbuf.xml

diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml index ff45330..802c1ab 100644 --- a/Documentation/DocBook/media/v4l/compat.xml +++ b/Documentation/DocBook/media/v4l/compat.xml @@ -2609,6 +2609,9 @@ ioctls.</para> <para>Importing DMABUF file descriptors as a new IO method described in <xref linkend="dmabuf" />.</para> </listitem>

•    <listitem>
  

•  <para>Exporting DMABUF files using &VIDIOC-EXPBUF; ioctl.</para>
  

•    </listitem>
   </itemizedlist>
  

  </section>

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml index 98253ee..c27e59b 100644 --- a/Documentation/DocBook/media/v4l/io.xml +++ b/Documentation/DocBook/media/v4l/io.xml @@ -488,6 +488,9 @@ buffer from userspace using a file descriptor previously exported for a different or the same device (known as the importer role), or both. This section describes the DMABUF importer role API in V4L2.</para>

• <para>Refer to <link linked="vidioc-expbuf"> DMABUF exporting </link> for

+details about exporting a V4L2 buffers as DMABUF file descriptors.</para>

<para>Input and output devices support the streaming I/O method when the <constant>V4L2_CAP_STREAMING</constant> flag in the <structfield>capabilities</structfield> field of &v4l2-capability; returned by diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml index 0292ed1..874c085 100644 --- a/Documentation/DocBook/media/v4l/v4l2.xml +++ b/Documentation/DocBook/media/v4l/v4l2.xml @@ -568,6 +568,7 @@ and discussions on the V4L mailing list.</revremark> &sub-overlay; &sub-prepare-buf; &sub-qbuf;

• &sub-expbuf;

This list is sorted alphabetically, so sub-expbuf should go after sub-enumstd.

 &sub-querybuf;
 &sub-querycap;
 &sub-queryctrl;

diff --git a/Documentation/DocBook/media/v4l/vidioc-expbuf.xml b/Documentation/DocBook/media/v4l/vidioc-expbuf.xml new file mode 100644 index 0000000..30ebf67 --- /dev/null +++ b/Documentation/DocBook/media/v4l/vidioc-expbuf.xml @@ -0,0 +1,223 @@ +<refentry id="vidioc-expbuf">

• <refmeta>

• <refentrytitle>ioctl VIDIOC_EXPBUF</refentrytitle>
• &manvol;

• </refmeta>

• <refnamediv>

• <refname>VIDIOC_EXPBUF</refname>
• <refpurpose>Export a buffer as a DMABUF file descriptor.</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_exportbuffer *<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_EXPBUF</para>
  

• </listitem>

•  </varlistentry>
  

•  <varlistentry>
  

• <term><parameter>argp</parameter></term>

• <listitem>

•  <para></para>
  

• </listitem>

•  </varlistentry>
  

• </variablelist>

• </refsect1>

• <refsect1>

• <title>Description</title>

• <note>

•  <title>Experimental</title>
  

•  <para>This is an <link linkend="experimental"> experimental </link>
  

•  interface and may change in the future.</para>
  

• </note>

+<para>This ioctl is an extension to the <link linkend="mmap">memory +mapping</link> I/O method therefore it is available only for +<constant>V4L2_MEMORY_MMAP</constant> buffers. It can be used to export a +buffer as DMABUF file at any time after buffers have been allocated with the +&VIDIOC-REQBUFS; ioctl.</para>

+<para>Prior to exporting an application calls <link +linkend="vidioc-querybuf">VIDIOC_QUERYBUF</link> to obtain memory offsets. When +using the <link linkend="planar-apis">multi-planar API</link> every plane has +own offset.</para>

+<para>To export a buffer, the application fills &v4l2-exportbuffer;. The +<structfield> mem_offset </structfield> field is set to the offset obtained +from <constant> VIDIOC_QUERYBUF </constant>. Additional flags may be posted in +the <structfield> flags </structfield> field. Refer to manual for open syscall

Better IMHO: 'Refer to the manual for open()'

+for details. Currently only O_CLOEXEC is guaranteed to be supported. All other +fields must be set to zero. In a case of multi-planar API, every plane is +exported separately using multiple <constant> VIDIOC_EXPBUF </constant> +calls.</para>

+<para> After calling <constant>VIDIOC_EXPBUF</constant> the <structfield> fd +</structfield> field will be set by a driver. This is a DMABUF file +descriptor. The application may pass it to other API. Refer to <link +linkend="dmabuf">DMABUF importing</link> for details about importing DMABUF +files into V4L2 nodes. A developer is encouraged to close a DMABUF file when it +is no longer used. </para>

Some explanation of why this is recommended would be useful.

• </refsect1>

• <refsect1>

• <section>

•  <title>Examples</title>
  

•  <example>
  

• <title>Exporting a buffer.</title>

• <programlisting>

+int buffer_export(int v4lfd, &v4l2-buf-type; bt, int index, int *dmafd) +{

• &v4l2-buffer; buf;
• &v4l2-exportbuffer; expbuf;
• memset(&amp;buf, 0, sizeof buf);
• buf.type = bt;
• buf.memory = V4L2_MEMORY_MMAP;
• buf.index = index;
• if (ioctl (v4lfd, &VIDIOC-QUERYBUF;, &amp;buf) == -1) {

• perror ("VIDIOC_QUERYBUF");
  

• return -1;
  

• }
• memset(&amp;expbuf, 0, sizeof expbuf);
• expbuf.mem_offset = buf.m.offset;
• if (ioctl (v4lfd, &VIDIOC-EXPBUF;, &amp;expbuf) == -1) {

• perror ("VIDIOC_EXPBUF");
  

• return -1;
  

• }

Same as in the review of the previous documentation patch: use the kernel coding style in these examples, so no space before '('.

• *dmafd = expbuf.fd;
• return 0;

+}

•    </programlisting>
  

•  </example>
  

•  <example>
  

• <title>Exporting a buffer using multi plane API.</title>

• <programlisting>

+int buffer_export_mp(int v4lfd, &v4l2-buf-type; bt, int index,

• int dmafd[], int n_planes)

+{

• &v4l2-buffer; buf;
• &v4l2-plane; planes[VIDEO_MAX_PLANES];
• int i;
• memset(&amp;buf, 0, sizeof buf);
• buf.type = bt;
• buf.memory = V4L2_MEMORY_MMAP;
• buf.index = index;
• buf.m.planes = planes;
• buf.length = n_planes;
• memset(&amp;planes, 0, sizeof planes);
• if (ioctl (v4lfd, &VIDIOC-QUERYBUF;, &amp;buf) == -1) {

• perror ("VIDIOC_QUERYBUF");
  

• return -1;
  

• }
• for (i = 0; i &lt; n_planes; ++i) {

• &v4l2-exportbuffer; expbuf;
  

• memset(&expbuf, 0, sizeof expbuf);
  

• expbuf.mem_offset = plane[i].m.offset;
  

• if (ioctl (v4lfd, &VIDIOC-EXPBUF;, &expbuf) == -1) {
  

• 	perror ("VIDIOC_EXPBUF");
  

• 	while (i)
  

• 		close(dmafd[--i]);
  

• 	return -1;
  

• }
  

• dmafd[i] = expbuf.fd;
  

• }
• return 0;

+}

•    </programlisting>
  

•  </example>
  

• </section>

• </refsect1>

• <refsect1>

• <table pgwide="1" frame="none" id="v4l2-exportbuffer">

•  <title>struct <structname>v4l2_exportbuffer</structname></title>
  

•  <tgroup cols="3">
  

• &cs-str;

• <tbody valign="top">

•  <row>
  

•    <entry>__u32</entry>
  

•    <entry><structfield>fd</structfield></entry>
  

•    <entry>The DMABUF file descriptor associated with a buffer. Set by
  

• a driver.</entry>
  

•  </row>
  

•  <row>
  

•    <entry>__u32</entry>
  

•    <entry><structfield>reserved0</structfield></entry>
  

•    <entry>Reserved field for future use. Must be set to zero.</entry>
  

•  </row>
  

•  <row>
  

•    <entry>__u32</entry>
  

•    <entry><structfield>mem_offset</structfield></entry>
  

•    <entry>Buffer memory offset as returned by <constant>
  

+VIDIOC_QUERYBUF </constant> in &v4l2-buffer;<structfield> ::m.offset +</structfield> (for single-plane formats) or &v4l2-plane;<structfield> +::m.offset </structfield> (for multi-planar formats)</entry>

•  </row>
  

•  <row>
  

•    <entry>__u32</entry>
  

•    <entry><structfield>flags</structfield></entry>
  

•    <entry>Flags for newly created file, currently only <constant>
  

+O_CLOEXEC </constant> is supported, refer to manual of open syscall for more

'to the manual of open() for more'

+details.</entry>

•  </row>
  

•  <row>
  

•    <entry>__u32</entry>
  

•    <entry><structfield>reserved[12]</structfield></entry>
  

•    <entry>Reserved field for future use. Must be set to zero.</entry>
  

•  </row>
  

• </tbody>

•  </tgroup>
  

• </table>

• </refsect1>

• <refsect1>

• &return-value;

• <variablelist>

•  <varlistentry>
  

• <term><errorcode>EINVAL</errorcode></term>

• <listitem>

•  <para>A queue is not in MMAP mode or DMABUF exporting is not
  

+supported or <structfield> flag </structfield> or <structfield> mem_offset

flag -> flags

+</structfield> fields are invalid.</para>

• </listitem>

•  </varlistentry>
  

• </variablelist>

• </refsect1>

+</refentry>

Regards,

Hans

Hi Tomasz,

Thanks for the patch.

On Tuesday 14 August 2012 17:34:48 Tomasz Stanislawski wrote:

This patch adds extension to V4L2 api. It allow to export a mmap buffer as file descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset used by mmap and return a file descriptor on success.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com

---

drivers/media/video/v4l2-compat-ioctl32.c | 1 + drivers/media/video/v4l2-dev.c | 1 + drivers/media/video/v4l2-ioctl.c | 15 +++++++++++++++ include/linux/videodev2.h | 26 ++++++++++++++++++++++++++ include/media/v4l2-ioctl.h | 2 ++ 5 files changed, 45 insertions(+)

[snip]

b/include/linux/videodev2.h index 7f918dc..b5d058b 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -688,6 +688,31 @@ struct v4l2_buffer { #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0800 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x1000

+/**

• • struct v4l2_exportbuffer - export of video buffer as DMABUF file

descriptor

• • @fd: file descriptor associated with DMABUF (set by driver)
• • @mem_offset: buffer memory offset as returned by VIDIOC_QUERYBUF in

struct

• • v4l2_buffer::m.offset (for single-plane formats) or
    

• • v4l2_plane::m.offset (for multi-planar formats)
    

• • @flags: flags for newly created file, currently only O_CLOEXEC is

• • supported, refer to manual of open syscall for more details
    

• • Contains data used for exporting a video buffer as DMABUF file

descriptor.

• • The buffer is identified by a 'cookie' returned by VIDIOC_QUERYBUF
• • (identical to the cookie used to mmap() the buffer to userspace). All
• • reserved fields must be set to zero. The field reserved0 is expected to
• • become a structure 'type' allowing an alternative layout of the

structure

• • content. Therefore this field should not be used for any other

extensions.

• */

+struct v4l2_exportbuffer {

• __u32 fd;
• __u32 reserved0;
• __u32 mem_offset;
• __u32 flags;
• __u32 reserved[12];

+};

As Hans recently pointed out, this could be layed out as

struct v4l2_exportbuffer { __u32 fd; __u32 memory; union { __u32 mem_offset; unsigned long userptr; } m; __u32 flags; __u32 reserved[12]; };

32-bit compatibility code would need to be added.

Hi Hans,

On Wednesday 22 August 2012 13:41:05 Hans Verkuil wrote:

On Tue August 14 2012 17:34:48 Tomasz Stanislawski wrote:

...

This patch adds extension to V4L2 api. It allow to export a mmap buffer as file descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset used by mmap and return a file descriptor on success.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com

[snip]

...

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 7f918dc..b5d058b 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -688,6 +688,31 @@ struct v4l2_buffer {

#define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0800 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x1000

+/**

• • struct v4l2_exportbuffer - export of video buffer as DMABUF file

descriptor + *

• • @fd: file descriptor associated with DMABUF (set by driver)
• • @mem_offset: buffer memory offset as returned by VIDIOC_QUERYBUF in

struct + * v4l2_buffer::m.offset (for single-plane formats) or

• • v4l2_plane::m.offset (for multi-planar formats)
    

• • @flags: flags for newly created file, currently only O_CLOEXEC is

• • supported, refer to manual of open syscall for more details
    

• • Contains data used for exporting a video buffer as DMABUF file

descriptor. + * The buffer is identified by a 'cookie' returned by VIDIOC_QUERYBUF + * (identical to the cookie used to mmap() the buffer to userspace). All + * reserved fields must be set to zero. The field reserved0 is expected to + * become a structure 'type' allowing an alternative layout of the structure + * content. Therefore this field should not be used for any other extensions. + */ +struct v4l2_exportbuffer {

• __u32 fd;
• __u32 reserved0;
• __u32 mem_offset;
• __u32 flags;
• __u32 reserved[12];

+};

OK, I realized that we also need a type field here: you need the type field (same as in v4l2_buffer) to know which queue the mem_offset refers to. For M2M devices you have two queues, so you need this information.

Is there any reason not to use __u32 memory instead of __u32 reserved0? I really dislike 'reserved0'. It's also very inconsistent with the other buffer ioctls which all have type+memory fields.

I'm concerned that we might need to export buffers in the future based on something else than the memory type. That's probably an irrational fear though.

Regarding mem_offset: I would prefer a union (possibly anonymous):

    union {
            __u32           mem_offset;
            unsigned long   reserved;
    } m;

Again, it's more consistent with the existing buffer ioctls, and it prepares the API for future pointer values. 'reserved' in the union above could even safely be renamed to userptr, even though userptr isn't supported at the moment.

Once again I would really want to see compeling use cases before we export a userptr buffer as a dmabuf object. As Mauro previously stated, userptr for buffer sharing was a hack in the first place (although a pretty successful one so far).

Hi Hans, Thank you for review. Please refer to the comments below.

On 08/22/2012 01:41 PM, Hans Verkuil wrote:

On Tue August 14 2012 17:34:48 Tomasz Stanislawski wrote:

...

This patch adds extension to V4L2 api. It allow to export a mmap buffer as file descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset used by mmap and return a file descriptor on success.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com

---

drivers/media/video/v4l2-compat-ioctl32.c | 1 + drivers/media/video/v4l2-dev.c | 1 + drivers/media/video/v4l2-ioctl.c | 15 +++++++++++++++ include/linux/videodev2.h | 26 ++++++++++++++++++++++++++ include/media/v4l2-ioctl.h | 2 ++ 5 files changed, 45 insertions(+)

[snip]

...

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index dffd3c9..c4e8c7e 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -458,6 +458,14 @@ static void v4l_print_buffer(const void *arg, bool write_only) tc->type, tc->flags, tc->frames, *(__u32 *)tc->userbits); }

+static void v4l_print_exportbuffer(const void *arg, bool write_only) +{

• const struct v4l2_exportbuffer *p = arg;
• pr_cont("fd=%d, mem_offset=%lx, flags=%lx\n",

• p->fd, (unsigned long)p->mem_offset, (unsigned long)p->flags);
  

Why the unsigned long casts?

It is needed to avoid compiler warnings on machines where "%lx" is not compatible with u32.

...

+}

static void v4l_print_create_buffers(const void *arg, bool write_only) { const struct v4l2_create_buffers *p = arg;

[snip]

...

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 7f918dc..b5d058b 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -688,6 +688,31 @@ struct v4l2_buffer { #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0800 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x1000

+/**

• • struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
• • @fd: file descriptor associated with DMABUF (set by driver)
• • @mem_offset: buffer memory offset as returned by VIDIOC_QUERYBUF in struct

• • v4l2_buffer::m.offset (for single-plane formats) or
    

• • v4l2_plane::m.offset (for multi-planar formats)
    

• • @flags: flags for newly created file, currently only O_CLOEXEC is

• • supported, refer to manual of open syscall for more details
    

• • Contains data used for exporting a video buffer as DMABUF file descriptor.
• • The buffer is identified by a 'cookie' returned by VIDIOC_QUERYBUF
• • (identical to the cookie used to mmap() the buffer to userspace). All
• • reserved fields must be set to zero. The field reserved0 is expected to
• • become a structure 'type' allowing an alternative layout of the structure
• • content. Therefore this field should not be used for any other extensions.
• */

+struct v4l2_exportbuffer {

• __u32 fd;
• __u32 reserved0;
• __u32 mem_offset;
• __u32 flags;
• __u32 reserved[12];

+};

OK, I realized that we also need a type field here: you need the type field (same as in v4l2_buffer) to know which queue the mem_offset refers to. For M2M devices you have two queues, so you need this information.

I do not agree with you. The mmap() does not need buffer_type. So VIDIOC_EXPBUF should not need the field either. Please refer to patch "[PATCHv8 26/26] v4l: s5p-mfc: support for dmabuf exporting" for example how to deal without buffer_type.

Is there any reason not to use __u32 memory instead of __u32 reserved0? I really dislike 'reserved0'. It's also very inconsistent with the other buffer ioctls which all have type+memory fields.

The type is not needed for exporting if mem_offset is available like in mmap() case. The memory is not needed because exporting is available only for MMAP buffers.

I see two ways to describe a buffer for exporting: a) by mem_offset b) by (buffer_type, index, plane_index) tuple

For know I prefer to implement only method (a) to avoid "single vs. multi plane" madness. Moreover it guarantees that only MMAP buffers are exported.

The second method assisted by VIDIOC_PREPARE_BUF will allow userptr-to-dmabuf conversion or dmabuf reexporting.

Regarding mem_offset: I would prefer a union (possibly anonymous):

I prefer not to add anonymous unions (though I like them) because the unions are not compatible with multiple C standards including C89.

    union {
            __u32           mem_offset;
            unsigned long   reserved;
    } m;

Again, it's more consistent with the existing buffer ioctls, and it prepares the API for future pointer values. 'reserved' in the union above could even safely be renamed to userptr, even though userptr isn't supported at the moment.

There is no guarantee that DMABUF could ever be created from user pointer. IMO, there is little need for support for such a feature. I prefer not to add any fields that to not immediately map to DMABUF functionalities. Let's keep the structures simple for now.

Moreover I prefer not to add 'unsigned long' as union member because the size of the union would be different on 32 and 64 bit machines.

The API is marked as experimental so it can change significantly.

Regards, Tomasz Stanislawski

Hi Hans, Thank your for your comments.

On 09/26/2012 08:39 AM, Hans Verkuil wrote:

On Tue September 25 2012 18:30:43 Tomasz Stanislawski wrote:

...

Hi Hans, Thank you for review. Please refer to the comments below.

On 08/22/2012 01:41 PM, Hans Verkuil wrote:

...

On Tue August 14 2012 17:34:48 Tomasz Stanislawski wrote:

...

This patch adds extension to V4L2 api. It allow to export a mmap buffer as file descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset used by mmap and return a file descriptor on success.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com

---

drivers/media/video/v4l2-compat-ioctl32.c | 1 + drivers/media/video/v4l2-dev.c | 1 + drivers/media/video/v4l2-ioctl.c | 15 +++++++++++++++ include/linux/videodev2.h | 26 ++++++++++++++++++++++++++ include/media/v4l2-ioctl.h | 2 ++ 5 files changed, 45 insertions(+)

[snip]

...

...

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index dffd3c9..c4e8c7e 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -458,6 +458,14 @@ static void v4l_print_buffer(const void *arg, bool write_only) tc->type, tc->flags, tc->frames, *(__u32 *)tc->userbits); }

+static void v4l_print_exportbuffer(const void *arg, bool write_only) +{

• const struct v4l2_exportbuffer *p = arg;
• pr_cont("fd=%d, mem_offset=%lx, flags=%lx\n",

• p->fd, (unsigned long)p->mem_offset, (unsigned long)p->flags);
  

Why the unsigned long casts?

It is needed to avoid compiler warnings on machines where "%lx" is not compatible with u32.

Why not use %x instead of %lx?

Ok. '%x' or '%08x' should be compatible with u32. Hopefully, no one is using V4L2 on 16-bit machines :).

...

...

...

+}

static void v4l_print_create_buffers(const void *arg, bool write_only) { const struct v4l2_create_buffers *p = arg;

[snip]

...

...

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 7f918dc..b5d058b 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -688,6 +688,31 @@ struct v4l2_buffer { #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0800 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x1000

+/**

• • struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
• • @fd: file descriptor associated with DMABUF (set by driver)
• • @mem_offset: buffer memory offset as returned by VIDIOC_QUERYBUF in struct

• • v4l2_buffer::m.offset (for single-plane formats) or
    

• • v4l2_plane::m.offset (for multi-planar formats)
    

• • @flags: flags for newly created file, currently only O_CLOEXEC is

• • supported, refer to manual of open syscall for more details
    

• • Contains data used for exporting a video buffer as DMABUF file descriptor.
• • The buffer is identified by a 'cookie' returned by VIDIOC_QUERYBUF
• • (identical to the cookie used to mmap() the buffer to userspace). All
• • reserved fields must be set to zero. The field reserved0 is expected to
• • become a structure 'type' allowing an alternative layout of the structure
• • content. Therefore this field should not be used for any other extensions.
• */

+struct v4l2_exportbuffer {

• __u32 fd;
• __u32 reserved0;
• __u32 mem_offset;
• __u32 flags;
• __u32 reserved[12];

+};

OK, I realized that we also need a type field here: you need the type field (same as in v4l2_buffer) to know which queue the mem_offset refers to. For M2M devices you have two queues, so you need this information.

I do not agree with you. The mmap() does not need buffer_type. So VIDIOC_EXPBUF should not need the field either. Please refer to patch "[PATCHv8 26/26] v4l: s5p-mfc: support for dmabuf exporting" for example how to deal without buffer_type.

...

Is there any reason not to use __u32 memory instead of __u32 reserved0? I really dislike 'reserved0'. It's also very inconsistent with the other buffer ioctls which all have type+memory fields.

The type is not needed for exporting if mem_offset is available like in mmap() case. The memory is not needed because exporting is available only for MMAP buffers.

Today, yes. Perhaps not in the future. mem_offset uniquely identifies a buffer, but only in the MMAP case. That happens to be the only one we support at the moment, but in the future we might want to support others as well.

...

I see two ways to describe a buffer for exporting: a) by mem_offset b) by (buffer_type, index, plane_index) tuple

Actually a (buffer_type, index, plane_index, memory) tuple.

Why the memory goes into the tuple? Is it possible that the queue can work in two modes at the same time? Is it possible that userptr and mmap buffers both with index 0 can coexist at the same queue?

IMO, currently the memory is a parameter of a V4L queue, not buffer's. In time, the field may become a buffer parameter if VIDIOC_CREATE_BUFS would permit a creation of buffers with different memory modes. Anyway, the memory would become only a parameter of a buffer. Similar to 'length' or 'sequence' or etc. As I understand, the memory field is used for v4l2_buffer only to inform which members of 'union m' should be copied to/from the userspace by V4L middle layer operation between ioctl() and a driver.

...

For know I prefer to implement only method (a) to avoid "single vs. multi plane" madness. Moreover it guarantees that only MMAP buffers are exported.

You see that as an advantage, I see that as a disadvantage. Because b is more future-proof than a. And while you need to provide more information, you do have that information available anyway in struct v4l2_buffer. It's the same information we use in any of the other streaming I/O ioctls, and I really like that consistency.

I agree that proposition (b) is more future-proof. Bad news is that implementing it implies significant changes to vb2-dmabuf code and documentation. It will postpone v9 which is already late. However, it is better to sacrifice some time on doing things right. I will start modifications now.

Does the structure layout suit you?

struct v4l2_exportbuffer { s32 fd; u32 reserved0; /* place for export-mode if some fields below become a union */ u32 type; /* enum v4l2_buf_type */ u32 index; u32 plane; u32 reserved[10]; u32 flags; };

...

The second method assisted by VIDIOC_PREPARE_BUF will allow userptr-to-dmabuf conversion or dmabuf reexporting.

...

Regarding mem_offset: I would prefer a union (possibly anonymous):

I prefer not to add anonymous unions (though I like them) because the unions are not compatible with multiple C standards including C89.

...

    union {
            __u32           mem_offset;
            unsigned long   reserved;
    } m;

Again, it's more consistent with the existing buffer ioctls, and it prepares the API for future pointer values. 'reserved' in the union above could even safely be renamed to userptr, even though userptr isn't supported at the moment.

There is no guarantee that DMABUF could ever be created from user pointer.

There is no guarantee that it could never be created from a user pointer either. Or some other newer memory type that doesn't exist yet.

...

IMO, there is little need for support for such a feature. I prefer not to add any fields that to not immediately map to DMABUF functionalities. Let's keep the structures simple for now.

Moreover I prefer not to add 'unsigned long' as union member because the size of the union would be different on 32 and 64 bit machines.

Well, that's the point. sizeof(unsigned long) == sizeof(void *) You could also write 'void *reserved;', as long as we can stick a pointer in there.

The problem is that the size of void* varies on different architectures. The u64 can always store void*, it has a fixed size and should be sufficient for at least 50 years :).

...

The API is marked as experimental so it can change significantly.

If we ever add new memory types or userptr dmabuf support, then that's most likely a long time in the future. By that time the experimental status of the API is surely revoked. So we need to do this right the first time. One reason the V4L2 API has scaled so well for the most part is due to the fact that the original designers were very conscious of the fact that the API should be as future proof as possible. We should follow that.

So instead of thinking of this ioctl as "exporting an mmaped buffer", think of it as "exporting a buffer". The first will work today, the second will work in the future as well.

Regards,

Hans

Regards, Tomasz Stanislawski

On Tue August 14 2012 17:34:49 Tomasz Stanislawski wrote:

This patch adds extension to videobuf2-core. It allow to export a mmap buffer as a file descriptor.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

drivers/media/video/videobuf2-core.c | 67 ++++++++++++++++++++++++++++++++++ include/media/videobuf2-core.h | 2 + 2 files changed, 69 insertions(+)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c index aed21e4..61354ec 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -1743,6 +1743,73 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, }

/**

• • vb2_expbuf() - Export a buffer as a file descriptor
• • @q: videobuf2 queue
• • @eb: export buffer structure passed from userspace to vidioc_expbuf

• • handler in driver
    

• • The return values from this function are intended to be directly returned
• • from vidioc_expbuf handler in driver.
• */

+int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb) +{

• struct vb2_buffer *vb = NULL;
• struct vb2_plane *vb_plane;
• unsigned int buffer, plane;
• int ret;
• struct dma_buf *dbuf;
• if (q->memory != V4L2_MEMORY_MMAP) {

• dprintk(1, "Queue is not currently set up for mmap\n");
  

• return -EINVAL;
  

• }
• if (!q->mem_ops->get_dmabuf) {

• dprintk(1, "Queue does not support DMA buffer exporting\n");
  

• return -EINVAL;
  

• }
• if (eb->flags & ~O_CLOEXEC) {

• dprintk(1, "Queue does support only O_CLOEXEC flag\n");
  

• return -EINVAL;
  

• }
• /*

• * Find the plane corresponding to the offset passed by userspace.
  

• */
  

• ret = __find_plane_by_offset(q, eb->mem_offset, &buffer, &plane);
• if (ret) {

• dprintk(1, "invalid offset %u\n", eb->mem_offset);
  

• return ret;
  

• }
• vb = q->bufs[buffer];
• vb_plane = &vb->planes[plane];
• dbuf = call_memop(q, get_dmabuf, vb_plane->mem_priv);
• if (IS_ERR_OR_NULL(dbuf)) {

• dprintk(1, "Failed to export buffer %d, plane %d\n",
  

• 	buffer, plane);
  

• return -EINVAL;
  

• }
• ret = dma_buf_fd(dbuf, eb->flags);
• if (ret < 0) {

• dprintk(3, "buffer %d, plane %d failed to export (%d)\n",
  

• 	buffer, plane, ret);
  

• dma_buf_put(dbuf);
  

• return ret;
  

• }
• dprintk(3, "buffer %d, plane %d exported as %d descriptor\n",

• buffer, plane, ret);
  

• eb->fd = ret;
• return 0;

+} +EXPORT_SYMBOL_GPL(vb2_expbuf);

+/**

• vb2_mmap() - map video buffers into application address space
• @q: videobuf2 queue
• @vma: vma passed to the mmap file operation handler in the driver

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index c306fec..b034424 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -81,6 +81,7 @@ struct vb2_fileio_data; struct vb2_mem_ops { void *(*alloc)(void *alloc_ctx, unsigned long size); void (*put)(void *buf_priv);

• struct dma_buf *(*get_dmabuf)(void *buf_priv);

  void *(*get_userptr)(void *alloc_ctx, unsigned long vaddr, unsigned long size, int write);

@@ -363,6 +364,7 @@ int vb2_queue_init(struct vb2_queue *q); void vb2_queue_release(struct vb2_queue *q);

int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b); +int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb); int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking);

int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);

Please add a vb2_ioctl_expbuf helper function as well!

Regards,

Hans

Hi Tomasz,

Thanks for the patch.

On Tuesday 14 August 2012 17:34:51 Tomasz Stanislawski wrote:

This patch adds taking reference to the device for MMAP buffers.

Such buffers, may be exported using DMABUF mechanism. If the driver that created a queue is unloaded then the queue is released, the device might be released too. However, buffers cannot be released if they are referenced by DMABUF descriptor(s). The device pointer kept in a buffer must be valid for the whole buffer's lifetime. Therefore MMAP buffers should take a reference to the device to avoid risk of dangling pointers.

That patch looks good, but that approach won't scale if we ever need a more complex context that can't be copied. We can ignore that for now, but if we decide to support "orphan" vb2 buffers, that's an issue we need to be aware of as it might come back to bite us in the future.

I assume that this patch requires your dmabuf owner patch to fix the orphan buffer case properly.

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com

drivers/media/video/videobuf2-dma-contig.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index bb2b4ac8..d44766e 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -148,6 +148,7 @@ static void vb2_dc_put(void *buf_priv) kfree(buf->sgt_base); } dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);

• put_device(buf->dev); kfree(buf);

}

@@ -161,9 +162,13 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) if (!buf) return ERR_PTR(-ENOMEM);

• /* prevent the device from release while the buffer is exported */
• get_device(dev);
• buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL); if (!buf->vaddr) { dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);

• put_device(dev);
  

  kfree(buf); return ERR_PTR(-ENOMEM); }

On Tue August 14 2012 17:34:53 Tomasz Stanislawski wrote:

From: Marek Szyprowski m.szyprowski@samsung.com

Please add some more information in the commit message. I've no idea what's going on here or why :-)

Regards,

Hans

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/media/video/atmel-isi.c | 2 +- drivers/media/video/blackfin/bfin_capture.c | 2 +- drivers/media/video/marvell-ccic/mcam-core.c | 3 ++- drivers/media/video/mx2_camera.c | 2 +- drivers/media/video/mx2_emmaprp.c | 2 +- drivers/media/video/mx3_camera.c | 2 +- drivers/media/video/s5p-fimc/fimc-core.c | 2 +- drivers/media/video/s5p-fimc/fimc-lite.c | 2 +- drivers/media/video/s5p-g2d/g2d.c | 2 +- drivers/media/video/s5p-jpeg/jpeg-core.c | 2 +- drivers/media/video/s5p-mfc/s5p_mfc.c | 5 ++-- drivers/media/video/s5p-tv/mixer_video.c | 2 +- drivers/media/video/sh_mobile_ceu_camera.c | 2 +- drivers/media/video/videobuf2-dma-contig.c | 33 +++++++++++++++++++------- drivers/staging/media/dt3155v4l/dt3155v4l.c | 2 +- include/media/videobuf2-dma-contig.h | 4 +++- 16 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c index 6274a91..9fb5283 100644 --- a/drivers/media/video/atmel-isi.c +++ b/drivers/media/video/atmel-isi.c @@ -1000,7 +1000,7 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev) list_add(&isi->dma_desc[i].list, &isi->dma_desc_head); }

• isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, 0); if (IS_ERR(isi->alloc_ctx)) { ret = PTR_ERR(isi->alloc_ctx); goto err_alloc_ctx;

diff --git a/drivers/media/video/blackfin/bfin_capture.c b/drivers/media/video/blackfin/bfin_capture.c index 1677623..7e90b65 100644 --- a/drivers/media/video/blackfin/bfin_capture.c +++ b/drivers/media/video/blackfin/bfin_capture.c @@ -893,7 +893,7 @@ static int __devinit bcap_probe(struct platform_device *pdev) } bcap_dev->ppi->priv = bcap_dev;

• bcap_dev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• bcap_dev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, 0); if (IS_ERR(bcap_dev->alloc_ctx)) { ret = PTR_ERR(bcap_dev->alloc_ctx); goto err_free_ppi;

diff --git a/drivers/media/video/marvell-ccic/mcam-core.c b/drivers/media/video/marvell-ccic/mcam-core.c index ce2b7b4..10d4db5 100644 --- a/drivers/media/video/marvell-ccic/mcam-core.c +++ b/drivers/media/video/marvell-ccic/mcam-core.c @@ -1111,7 +1111,8 @@ static int mcam_setup_vb2(struct mcam_camera *cam) #ifdef MCAM_MODE_DMA_CONTIG vq->ops = &mcam_vb2_ops; vq->mem_ops = &vb2_dma_contig_memops;

• cam->vb_alloc_ctx = vb2_dma_contig_init_ctx(cam->dev);
  

• cam->vb_alloc_ctx = vb2_dma_contig_init_ctx(cam->dev,
  

• 	VB2_CREATE_VADDR);
  

  vq->io_modes = VB2_MMAP | VB2_USERPTR; cam->dma_setup = mcam_ctlr_dma_contig; cam->frame_complete = mcam_dma_contig_done;

diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c index 637bde8..5c30302 100644 --- a/drivers/media/video/mx2_camera.c +++ b/drivers/media/video/mx2_camera.c @@ -1766,7 +1766,7 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev) if (cpu_is_mx25()) pcdev->soc_host.capabilities = SOCAM_HOST_CAP_STRIDE;

• pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, 0); if (IS_ERR(pcdev->alloc_ctx)) { err = PTR_ERR(pcdev->alloc_ctx); goto eallocctx;

diff --git a/drivers/media/video/mx2_emmaprp.c b/drivers/media/video/mx2_emmaprp.c index 2810015..23c6c42 100644 --- a/drivers/media/video/mx2_emmaprp.c +++ b/drivers/media/video/mx2_emmaprp.c @@ -962,7 +962,7 @@ static int emmaprp_probe(struct platform_device *pdev) 0, MEM2MEM_NAME, pcdev) < 0) goto rel_vdev;

• pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, 0); if (IS_ERR(pcdev->alloc_ctx)) { v4l2_err(&pcdev->v4l2_dev, "Failed to alloc vb2 context\n"); ret = PTR_ERR(pcdev->alloc_ctx);

diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c index f13643d..882026f 100644 --- a/drivers/media/video/mx3_camera.c +++ b/drivers/media/video/mx3_camera.c @@ -1227,7 +1227,7 @@ static int __devinit mx3_camera_probe(struct platform_device *pdev) soc_host->v4l2_dev.dev = &pdev->dev; soc_host->nr = pdev->id;

• mx3_cam->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• mx3_cam->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, 0); if (IS_ERR(mx3_cam->alloc_ctx)) { err = PTR_ERR(mx3_cam->alloc_ctx); goto eallocctx;

diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c index 1a44540..e1747f2 100644 --- a/drivers/media/video/s5p-fimc/fimc-core.c +++ b/drivers/media/video/s5p-fimc/fimc-core.c @@ -929,7 +929,7 @@ static int fimc_probe(struct platform_device *pdev) if (ret < 0) goto err_sd; /* Initialize contiguous memory allocator */

• fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, 0); if (IS_ERR(fimc->alloc_ctx)) { ret = PTR_ERR(fimc->alloc_ctx); goto err_pm;

diff --git a/drivers/media/video/s5p-fimc/fimc-lite.c b/drivers/media/video/s5p-fimc/fimc-lite.c index c5b57e8..ccc70a0 100644 --- a/drivers/media/video/s5p-fimc/fimc-lite.c +++ b/drivers/media/video/s5p-fimc/fimc-lite.c @@ -1459,7 +1459,7 @@ static int __devinit fimc_lite_probe(struct platform_device *pdev) if (ret < 0) goto err_sd;

• fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• fimc->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, 0); if (IS_ERR(fimc->alloc_ctx)) { ret = PTR_ERR(fimc->alloc_ctx); goto err_pm;

diff --git a/drivers/media/video/s5p-g2d/g2d.c b/drivers/media/video/s5p-g2d/g2d.c index 0edc2df..ad7837f 100644 --- a/drivers/media/video/s5p-g2d/g2d.c +++ b/drivers/media/video/s5p-g2d/g2d.c @@ -756,7 +756,7 @@ static int g2d_probe(struct platform_device *pdev) goto put_clk_gate; }

• dev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• dev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, 0); if (IS_ERR(dev->alloc_ctx)) { ret = PTR_ERR(dev->alloc_ctx); goto unprep_clk_gate;

diff --git a/drivers/media/video/s5p-jpeg/jpeg-core.c b/drivers/media/video/s5p-jpeg/jpeg-core.c index be04d58..900059e 100644 --- a/drivers/media/video/s5p-jpeg/jpeg-core.c +++ b/drivers/media/video/s5p-jpeg/jpeg-core.c @@ -1372,7 +1372,7 @@ static int s5p_jpeg_probe(struct platform_device *pdev) goto device_register_rollback; }

• jpeg->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• jpeg->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, VB2_CREATE_VADDR); if (IS_ERR(jpeg->alloc_ctx)) { v4l2_err(&jpeg->v4l2_dev, "Failed to init memory allocator\n"); ret = PTR_ERR(jpeg->alloc_ctx);

diff --git a/drivers/media/video/s5p-mfc/s5p_mfc.c b/drivers/media/video/s5p-mfc/s5p_mfc.c index e3e616d..9056a10 100644 --- a/drivers/media/video/s5p-mfc/s5p_mfc.c +++ b/drivers/media/video/s5p-mfc/s5p_mfc.c @@ -1018,12 +1018,13 @@ static int s5p_mfc_probe(struct platform_device *pdev) goto err_res; }

• dev->alloc_ctx[0] = vb2_dma_contig_init_ctx(dev->mem_dev_l);

• dev->alloc_ctx[0] = vb2_dma_contig_init_ctx(dev->mem_dev_l,

• VB2_CREATE_VADDR);
  

  if (IS_ERR_OR_NULL(dev->alloc_ctx[0])) { ret = PTR_ERR(dev->alloc_ctx[0]); goto err_res; }

• dev->alloc_ctx[1] = vb2_dma_contig_init_ctx(dev->mem_dev_r);

• dev->alloc_ctx[1] = vb2_dma_contig_init_ctx(dev->mem_dev_r, 0); if (IS_ERR_OR_NULL(dev->alloc_ctx[1])) { ret = PTR_ERR(dev->alloc_ctx[1]); goto err_mem_init_ctx_1;

diff --git a/drivers/media/video/s5p-tv/mixer_video.c b/drivers/media/video/s5p-tv/mixer_video.c index da5b7a5..a7e3b53 100644 --- a/drivers/media/video/s5p-tv/mixer_video.c +++ b/drivers/media/video/s5p-tv/mixer_video.c @@ -78,7 +78,7 @@ int __devinit mxr_acquire_video(struct mxr_device *mdev, goto fail; }

• mdev->alloc_ctx = vb2_dma_contig_init_ctx(mdev->dev);

• mdev->alloc_ctx = vb2_dma_contig_init_ctx(mdev->dev, 0); if (IS_ERR_OR_NULL(mdev->alloc_ctx)) { mxr_err(mdev, "could not acquire vb2 allocator\n"); goto fail_v4l2_dev;

diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c index 0baaf94..e085c27 100644 --- a/drivers/media/video/sh_mobile_ceu_camera.c +++ b/drivers/media/video/sh_mobile_ceu_camera.c @@ -2158,7 +2158,7 @@ static int __devinit sh_mobile_ceu_probe(struct platform_device *pdev) pcdev->ici.ops = &sh_mobile_ceu_host_ops; pcdev->ici.capabilities = SOCAM_HOST_CAP_STRIDE;

• pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);

• pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev, 0); if (IS_ERR(pcdev->alloc_ctx)) { err = PTR_ERR(pcdev->alloc_ctx); goto exit_free_clk;

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 11f4a46..0729187 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -23,10 +23,12 @@

struct vb2_dc_conf { struct device *dev;

• unsigned int flags;

};

struct vb2_dc_buf { struct device *dev;

• unsigned int flags; void *vaddr; unsigned long size; dma_addr_t dma_addr;

@@ -34,6 +36,7 @@ struct vb2_dc_buf { struct sg_table *dma_sgt;

/* MMAP related */

• struct dma_attrs dma_attrs; struct vb2_vmarea_handler handler; atomic_t refcount; struct sg_table *sgt_base;

@@ -98,6 +101,9 @@ static void *vb2_dc_vaddr(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv;

• if (WARN_ON(~buf->flags & VB2_CREATE_VADDR))

• return NULL;
  

• return buf->vaddr;

}

@@ -147,7 +153,8 @@ static void vb2_dc_put(void *buf_priv) sg_free_table(buf->sgt_base); kfree(buf->sgt_base); }

• dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);

• dma_free_attrs(buf->dev, buf->size, buf->vaddr, buf->dma_addr,

•        &buf->dma_attrs);
  

  put_device(buf->dev); kfree(buf);

} @@ -165,7 +172,14 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) /* prevent the device from release while the buffer is exported */ get_device(dev);

• buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL);

• /* set up alloca attributes */
• init_dma_attrs(&buf->dma_attrs);
• dma_set_attr(DMA_ATTR_WRITE_COMBINE, &buf->dma_attrs);
• if (!(conf->flags & VB2_CREATE_VADDR))

• dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, &buf->dma_attrs);
  

• buf->vaddr = dma_alloc_attrs(dev, size, &buf->dma_addr, GFP_KERNEL,

• 		     &buf->dma_attrs);
  

  if (!buf->vaddr) { dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size); put_device(dev);

@@ -174,6 +188,7 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) }

buf->dev = dev;

• buf->flags = conf->flags; buf->size = size;

  buf->handler.refcount = &buf->refcount;

@@ -201,9 +216,8 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) */ vma->vm_pgoff = 0;

• ret = dma_mmap_coherent(buf->dev, vma, buf->vaddr,

• buf->dma_addr, buf->size);
  

• ret = dma_mmap_attrs(buf->dev, vma, buf->vaddr, buf->dma_addr,

• 	     buf->size, &buf->dma_attrs);
  

  if (ret) { pr_err("Remapping memory failed, error: %d\n", ret); return ret;

@@ -345,7 +359,7 @@ static void *vb2_dc_dmabuf_ops_kmap(struct dma_buf *dbuf, unsigned long pgnum) { struct vb2_dc_buf *buf = dbuf->priv;

• return buf->vaddr + pgnum * PAGE_SIZE;

• return buf->vaddr ? buf->vaddr + pgnum * PAGE_SIZE : NULL;

}

static void *vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf) @@ -385,8 +399,8 @@ static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf) return ERR_PTR(-ENOMEM); }

• ret = dma_get_sgtable(buf->dev, sgt, buf->vaddr, buf->dma_addr,

• buf->size);
  

• ret = dma_get_sgtable_attrs(buf->dev, sgt, buf->vaddr, buf->dma_addr,

• buf->size, &buf->dma_attrs);
  

  if (ret < 0) { dev_err(buf->dev, "failed to get scatterlist from DMA API\n"); kfree(sgt);

@@ -753,7 +767,7 @@ const struct vb2_mem_ops vb2_dma_contig_memops = { }; EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);

-void *vb2_dma_contig_init_ctx(struct device *dev) +void *vb2_dma_contig_init_ctx(struct device *dev, unsigned int flags) { struct vb2_dc_conf *conf;

@@ -762,6 +776,7 @@ void *vb2_dma_contig_init_ctx(struct device *dev) return ERR_PTR(-ENOMEM);

conf->dev = dev;

• conf->flags = flags;

  return conf;

} diff --git a/drivers/staging/media/dt3155v4l/dt3155v4l.c b/drivers/staging/media/dt3155v4l/dt3155v4l.c index 2e7b711..3fcf15a 100644 --- a/drivers/staging/media/dt3155v4l/dt3155v4l.c +++ b/drivers/staging/media/dt3155v4l/dt3155v4l.c @@ -232,7 +232,7 @@ dt3155_queue_setup(struct vb2_queue *q, const struct v4l2_format *fmt, sizes[0] = img_width * img_height; if (pd->q->alloc_ctx[0]) return 0;

• ret = vb2_dma_contig_init_ctx(&pd->pdev->dev);

• ret = vb2_dma_contig_init_ctx(&pd->pdev->dev, VB2_CREATE_VADDR); if (IS_ERR(ret)) return PTR_ERR(ret); pd->q->alloc_ctx[0] = ret;

diff --git a/include/media/videobuf2-dma-contig.h b/include/media/videobuf2-dma-contig.h index 8197f87..8bf4b29 100644 --- a/include/media/videobuf2-dma-contig.h +++ b/include/media/videobuf2-dma-contig.h @@ -24,7 +24,9 @@ vb2_dma_contig_plane_dma_addr(struct vb2_buffer *vb, unsigned int plane_no) return *addr; }

-void *vb2_dma_contig_init_ctx(struct device *dev); +#define VB2_CREATE_VADDR (1 << 0)

+void *vb2_dma_contig_init_ctx(struct device *dev, unsigned int flags); void vb2_dma_contig_cleanup_ctx(void *alloc_ctx);

extern const struct vb2_mem_ops vb2_dma_contig_memops;