[PATCH 00/11] Integration of videobuf2 with dmabuf

[PATCH 00/11] Integration of videobuf2 with dmabuf

[Tomasz Stanislawski]

Hello everyone,
This patchset adds support for DMABUF [2] importing to V4L2 stack.  It was
updated after Laurent Pinchart's review.  The support for DMABUF exporting was
moved to separate patchset due to dependency on patches for DMA mapping
redesign by Marek Szyprowski [4].

v3:
- rebased on mainline 3.4-rc1
- split 'code refactor' patch to multiple smaller patches
- squashed fixes to Sumit's patches
- patchset is no longer dependant on 'DMA mapping redesign'
- separated path for handling IO and non-IO mappings
- add documentation for DMABUF importing to V4L
- removed all DMABUF exporter related code
- removed usage of dma_get_pages extension

v2:
- extended VIDIOC_EXPBUF argument from integer memoffset to struct
  v4l2_exportbuffer
- added patch that breaks DMABUF spec on (un)map_atachment callcacks but allows
  to work with existing implementation of DMABUF prime in DRM
- all dma-contig code refactoring patches were squashed
- bugfixes

v1: List of changes since [1].
- support for DMA api extension dma_get_pages, the function is used to retrieve
  pages used to create DMA mapping.
- small fixes/code cleanup to videobuf2
- added prepare and finish callbacks to vb2 allocators, it is used keep
  consistency between dma-cpu acess to the memory (by Marek Szyprowski)
- support for exporting of DMABUF buffer in V4L2 and Videobuf2, originated from
  [3].
- support for dma-buf exporting in vb2-dma-contig allocator
- support for DMABUF for s5p-tv and s5p-fimc (capture interface) drivers,
  originated from [3]
- changed handling for userptr buffers (by Marek Szyprowski, Andrzej
  Pietrasiewicz)
- let mmap method to use dma_mmap_writecombine call (by Marek Szyprowski)

[1] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/42966/focus=42968
[2] https://lkml.org/lkml/2011/12/26/29
[3] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/36354/focus=36355
[4] http://thread.gmane.org/gmane.linux.kernel.cross-arch/12819

Andrzej Pietrasiewicz (1):
  v4l: vb2-dma-contig: add support for scatterlist in userptr mode

Laurent Pinchart (2):
  v4l: vb2-dma-contig: Shorten vb2_dma_contig prefix to vb2_dc
  v4l: vb2-dma-contig: Reorder functions

Marek Szyprowski (2):
  v4l: vb2: add prepare/finish callbacks to allocators
  v4l: vb2-dma-contig: add prepare/finish to dma-contig allocator

Sumit Semwal (4):
  v4l: Add DMABUF as a memory type
  v4l: vb2: add support for shared buffer (dma_buf)
  v4l: vb: remove warnings about MEMORY_DMABUF
  v4l: vb2: Add dma-contig allocator as dma_buf user

Tomasz Stanislawski (2):
  Documentation: media: description of DMABUF importing in V4L2
  v4l: vb2-dma-contig: Remove unneeded allocation context structure

 Documentation/DocBook/media/v4l/compat.xml         |    4 +
 Documentation/DocBook/media/v4l/io.xml             |  177 +++++++
 .../DocBook/media/v4l/vidioc-create-bufs.xml       |    1 +
 Documentation/DocBook/media/v4l/vidioc-qbuf.xml    |   15 +
 Documentation/DocBook/media/v4l/vidioc-reqbufs.xml |    8 +-
 drivers/media/video/videobuf-core.c                |    4 +
 drivers/media/video/videobuf2-core.c               |  195 +++++++-
 drivers/media/video/videobuf2-dma-contig.c         |  536 +++++++++++++++++---
 include/linux/videodev2.h                          |    8 +
 include/media/videobuf2-core.h                     |   38 ++
 10 files changed, 910 insertions(+), 76 deletions(-)

-- 
1.7.5.4

[PATCH 01/11] v4l: Add DMABUF as a memory type

[Tomasz Stanislawski]

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>
---
 include/linux/videodev2.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index c9c9a46..38259bf 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -185,6 +185,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/ */
@@ -588,6 +589,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
  *
@@ -602,6 +605,7 @@ struct v4l2_plane {
 	union {
 		__u32		mem_offset;
 		unsigned long	userptr;
+		int		fd;
 	} m;
 	__u32			data_offset;
 	__u32			reserved[11];
@@ -624,6 +628,9 @@ 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
@@ -650,6 +657,7 @@ struct v4l2_buffer {
 		__u32           offset;
 		unsigned long   userptr;
 		struct v4l2_plane *planes;
+		int		fd;
 	} m;
 	__u32			length;
 	__u32			input;
-- 
1.7.5.4

[PATCH 02/11] Documentation: media: description of DMABUF importing in V4L2

[Tomasz Stanislawski]

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>
---
 Documentation/DocBook/media/v4l/compat.xml         |    4 +
 Documentation/DocBook/media/v4l/io.xml             |  177 ++++++++++++++++++++
 .../DocBook/media/v4l/vidioc-create-bufs.xml       |    1 +
 Documentation/DocBook/media/v4l/vidioc-qbuf.xml    |   15 ++
 Documentation/DocBook/media/v4l/vidioc-reqbufs.xml |    8 +-
 5 files changed, 203 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
index bce97c5..2a2083d 100644
--- a/Documentation/DocBook/media/v4l/compat.xml
+++ b/Documentation/DocBook/media/v4l/compat.xml
@@ -2523,6 +2523,10 @@ ioctls.</para>
         <listitem>
 	  <para>Selection API. <xref linkend="selection-api" /></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 b815929..2a32363 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -472,6 +472,160 @@ 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 was introduced to provide a generic mean for sharing
+buffers between multiple HW devices. Some device drivers provide an API that
+may be capable to export a DMA buffer as a DMABUF file descriptor. The other
+device drivers may be configured to use such a buffer. They implements an API
+for importing of DMABUF. This section describes the support of DMABUF importing
+in V4L2.</para>
+
+    <para>Input and output devices support this 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. If the particular importing buffer via
+DMABUF file descriptors method is supported must be determined by calling the
+&VIDIOC-REQBUFS; ioctl.</para>
+
+    <para>This I/O method is dedicated for sharing DMA buffers between V4L and
+other APIs. Buffers (planes) are allocated by the application itself using
+a client API. The buffers must be exported as DMABUF file descriptor.  Only those
+file descriptor are exchanged, these files and meta-information are passed in
+&v4l2-buffer; (or in &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>
+
+    <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;
+
+if (ioctl (fd, &VIDIOC-REQBUFS;, &amp;reqbuf) == -1) {
+	if (errno == EINVAL)
+		printf ("Video capturing or user pointer streaming is not supported\n");
+	else
+		perror ("VIDIOC_REQBUFS");
+
+	exit (EXIT_FAILURE);
+}
+      </programlisting>
+    </example>
+
+    <para>Buffer (plane) file is passed on the fly with the &VIDIOC-QBUF;
+ioctl. In case of multiplanar buffers, every plane can be associated with a
+different DMABUF descriptor.Although buffers are commonly cycled, applications
+can pass 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 unpin the buffer at any
+time between the completion of the DMA and this ioctl. The memory is
+also unpinned 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>
+
+    <para>To start and stop capturing or output applications call the
+&VIDIOC-STREAMON; and &VIDIOC-STREAMOFF; ioctl. Note
+<constant>VIDIOC_STREAMOFF</constant> removes all buffers from both queues and
+unlocks/unpins 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 user pointer 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> ioctl, the <function>select()</function>
+and <function>poll()</function> function.</para>
+
+  </section>
+
   <section id="async">
     <title>Asynchronous I/O</title>
 
@@ -671,6 +825,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-planar 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 +908,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>
@@ -980,6 +1151,12 @@ pointer</link> I/O.</entry>
 	    <entry>3</entry>
 	    <entry>[to do]</entry>
 	  </row>
+	  <row>
+	    <entry><constant>V4L2_MEMORY_DMABUF</constant></entry>
+	    <entry>2</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 73ae8a6..adc92be 100644
--- a/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-create-bufs.xml
@@ -98,6 +98,7 @@ information.</para>
 	    <entry><structfield>memory</structfield></entry>
 	    <entry>Applications set this field to
 <constant>V4L2_MEMORY_MMAP</constant> or
+<constant>V4L2_MEMORY_DMABUF</constant> or
 <constant>V4L2_MEMORY_USERPTR</constant>.</entry>
 	  </row>
 	  <row>
diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
index 9caa49a..d4f212f 100644
--- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
@@ -112,6 +112,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>, the <structfield>m.fd</structfield>
+field to the file descriptor to a DMABUF buffer. When the multi-planar API is
+used, <structfield>m.fd</structfield> 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 may pin and lock the buffer. Buffers remain locked/pinned until dequeued,
+until the &VIDIOC-STREAMOFF; or &VIDIOC-REQBUFS; ioctl is called, or until the
+device is closed.</para>
+
     <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 7be4b1d..d758622 100644
--- a/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml
@@ -50,11 +50,14 @@
 
     <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
+I/O or <link linkend="dmabuf">DMABUF file descriptors</link>.
+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>
+driver into user pointer I/O mode and to setup some internal structures.
+A DMABUF file descriptor is a generic carrier for HW allocated buffers
+dedicated to share them between multimedia APIs.</para>
 
     <para>To allocate device buffers applications initialize all
 fields of the <structname>v4l2_requestbuffers</structname> structure.
@@ -103,6 +106,7 @@ as the &v4l2-format; <structfield>type</structfield> field. See <xref
 	    <entry><structfield>memory</structfield></entry>
 	    <entry>Applications set this field to
 <constant>V4L2_MEMORY_MMAP</constant> or
+<constant>V4L2_MEMORY_DMABUF</constant> or
 <constant>V4L2_MEMORY_USERPTR</constant>.</entry>
 	  </row>
 	  <row>
-- 
1.7.5.4

[PATCH 03/11] v4l: vb2: add support for shared buffer (dma_buf)

[Tomasz Stanislawski]

From: Sumit Semwal <sumit.semwal@ti.com>

This patch adds support for DMABUF memory type in videobuf2. It calls relevant
APIs of dma_buf for v4l reqbuf / qbuf / dqbuf operations.

For this version, the support is for videobuf2 as a user of the shared buffer;
so the allocation of the buffer is done outside of V4L2. [A sample allocator of
dma-buf shared buffer is given at [1]]

[1]: Rob Clark's DRM:
   https://github.com/robclark/kernel-omap4/commits/drmplane-dmabuf

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>
---
 drivers/media/video/videobuf2-core.c |  184 +++++++++++++++++++++++++++++++++-
 include/media/videobuf2-core.h       |   31 ++++++
 2 files changed, 214 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index 2e8f1df..b37feea 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -106,6 +106,27 @@ static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
 }
 
 /**
+ * __vb2_buf_dmabuf_put() - release memory associated with
+ * a DMABUF shared buffer
+ */
+static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
+{
+	struct vb2_queue *q = vb->vb2_queue;
+	unsigned int plane;
+
+	for (plane = 0; plane < vb->num_planes; ++plane) {
+		void *mem_priv = vb->planes[plane].mem_priv;
+
+		if (mem_priv) {
+			call_memop(q, detach_dmabuf, mem_priv);
+			dma_buf_put(vb->planes[plane].dbuf);
+			vb->planes[plane].dbuf = NULL;
+			vb->planes[plane].mem_priv = NULL;
+		}
+	}
+}
+
+/**
  * __setup_offsets() - setup unique offsets ("cookies") for every plane in
  * every buffer on the queue
  */
@@ -227,6 +248,8 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
 		/* Free MMAP buffers or release USERPTR buffers */
 		if (q->memory == V4L2_MEMORY_MMAP)
 			__vb2_buf_mem_free(vb);
+		if (q->memory == V4L2_MEMORY_DMABUF)
+			__vb2_buf_dmabuf_put(vb);
 		else
 			__vb2_buf_userptr_put(vb);
 	}
@@ -349,6 +372,12 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 		 */
 		memcpy(b->m.planes, vb->v4l2_planes,
 			b->length * sizeof(struct v4l2_plane));
+
+		if (q->memory == V4L2_MEMORY_DMABUF) {
+			unsigned int plane;
+			for (plane = 0; plane < vb->num_planes; ++plane)
+				b->m.planes[plane].m.fd = 0;
+		}
 	} else {
 		/*
 		 * We use length and offset in v4l2_planes array even for
@@ -360,6 +389,8 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 			b->m.offset = vb->v4l2_planes[0].m.mem_offset;
 		else if (q->memory == V4L2_MEMORY_USERPTR)
 			b->m.userptr = vb->v4l2_planes[0].m.userptr;
+		else if (q->memory == V4L2_MEMORY_DMABUF)
+			b->m.fd = 0;
 	}
 
 	/*
@@ -451,6 +482,21 @@ static int __verify_mmap_ops(struct vb2_queue *q)
 }
 
 /**
+ * __verify_dmabuf_ops() - verify that all memory operations required for
+ * DMABUF queue type have been provided
+ */
+static int __verify_dmabuf_ops(struct vb2_queue *q)
+{
+	if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf
+			|| !q->mem_ops->detach_dmabuf
+			|| !q->mem_ops->map_dmabuf
+			|| !q->mem_ops->unmap_dmabuf)
+		return -EINVAL;
+
+	return 0;
+}
+
+/**
  * vb2_reqbufs() - Initiate streaming
  * @q:		videobuf2 queue
  * @req:	struct passed from userspace to vidioc_reqbufs handler in driver
@@ -484,6 +530,7 @@ int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	}
 
 	if (req->memory != V4L2_MEMORY_MMAP
+			&& req->memory != V4L2_MEMORY_DMABUF
 			&& req->memory != V4L2_MEMORY_USERPTR) {
 		dprintk(1, "reqbufs: unsupported memory type\n");
 		return -EINVAL;
@@ -513,6 +560,11 @@ int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 		return -EINVAL;
 	}
 
+	if (req->memory == V4L2_MEMORY_DMABUF && __verify_dmabuf_ops(q)) {
+		dprintk(1, "reqbufs: DMABUF for current setup unsupported\n");
+		return -EINVAL;
+	}
+
 	if (req->count == 0 || q->num_buffers != 0 || q->memory != req->memory) {
 		/*
 		 * We already have buffers allocated, so first check if they
@@ -620,7 +672,8 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
 	}
 
 	if (create->memory != V4L2_MEMORY_MMAP
-			&& create->memory != V4L2_MEMORY_USERPTR) {
+			&& create->memory != V4L2_MEMORY_USERPTR
+			&& create->memory != V4L2_MEMORY_DMABUF) {
 		dprintk(1, "%s(): unsupported memory type\n", __func__);
 		return -EINVAL;
 	}
@@ -644,6 +697,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
 		return -EINVAL;
 	}
 
+	if (create->memory == V4L2_MEMORY_DMABUF && __verify_dmabuf_ops(q)) {
+		dprintk(1, "%s(): DMABUF for current setup unsupported\n", __func__);
+		return -EINVAL;
+	}
+
 	if (q->num_buffers == VIDEO_MAX_FRAME) {
 		dprintk(1, "%s(): maximum number of buffers already allocated\n",
 			__func__);
@@ -839,6 +897,14 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b,
 					b->m.planes[plane].length;
 			}
 		}
+		if (b->memory == V4L2_MEMORY_DMABUF) {
+			for (plane = 0; plane < vb->num_planes; ++plane) {
+				v4l2_planes[plane].bytesused =
+					b->m.planes[plane].bytesused;
+				v4l2_planes[plane].m.fd =
+					b->m.planes[plane].m.fd;
+			}
+		}
 	} else {
 		/*
 		 * Single-planar buffers do not use planes array,
@@ -853,6 +919,10 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b,
 			v4l2_planes[0].m.userptr = b->m.userptr;
 			v4l2_planes[0].length = b->length;
 		}
+
+		if (b->memory == V4L2_MEMORY_DMABUF)
+			v4l2_planes[0].m.fd = b->m.fd;
+
 	}
 
 	vb->v4l2_buf.field = b->field;
@@ -957,6 +1027,105 @@ static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 }
 
 /**
+ * __qbuf_dmabuf() - handle qbuf of a DMABUF buffer
+ */
+static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
+{
+	struct v4l2_plane planes[VIDEO_MAX_PLANES];
+	struct vb2_queue *q = vb->vb2_queue;
+	void *mem_priv;
+	unsigned int plane;
+	int ret;
+	int write = !V4L2_TYPE_IS_OUTPUT(q->type);
+
+	/* Verify and copy relevant information provided by the userspace */
+	ret = __fill_vb2_buffer(vb, b, planes);
+	if (ret)
+		return ret;
+
+	for (plane = 0; plane < vb->num_planes; ++plane) {
+		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
+
+		if (IS_ERR_OR_NULL(dbuf)) {
+			dprintk(1, "qbuf: invalid dmabuf fd for "
+				"plane %d\n", plane);
+			ret = -EINVAL;
+			goto err;
+		}
+
+		/* Skip the plane if already verified */
+		if (dbuf == vb->planes[plane].dbuf) {
+			planes[plane].length = dbuf->size;
+			dma_buf_put(dbuf);
+			continue;
+		}
+
+		dprintk(3, "qbuf: buffer description for plane %d changed, "
+			"reattaching dma buf\n", plane);
+
+		/* Release previously acquired memory if present */
+		if (vb->planes[plane].mem_priv) {
+			call_memop(q, detach_dmabuf,
+				vb->planes[plane].mem_priv);
+			dma_buf_put(vb->planes[plane].dbuf);
+		}
+
+		vb->planes[plane].mem_priv = NULL;
+
+		/* Acquire each plane's memory */
+		mem_priv = call_memop(q, attach_dmabuf, q->alloc_ctx[plane],
+			dbuf, q->plane_sizes[plane], write);
+		if (IS_ERR(mem_priv)) {
+			dprintk(1, "qbuf: failed acquiring dmabuf "
+				"memory for plane %d\n", plane);
+			ret = PTR_ERR(mem_priv);
+			goto err;
+		}
+
+		planes[plane].length = dbuf->size;
+		vb->planes[plane].dbuf = dbuf;
+		vb->planes[plane].mem_priv = mem_priv;
+	}
+
+	/* TODO: This pins the buffer(s) with  dma_buf_map_attachment()).. but
+	 * really we want to do this just before the DMA, not while queueing
+	 * the buffer(s)..
+	 */
+	for (plane = 0; plane < vb->num_planes; ++plane) {
+		ret = call_memop(q, map_dmabuf, vb->planes[plane].mem_priv);
+		if (ret) {
+			dprintk(1, "qbuf: failed mapping dmabuf "
+				"memory for plane %d\n", plane);
+			goto err;
+		}
+	}
+
+	/*
+	 * Call driver-specific initialization on the newly acquired buffer,
+	 * if provided.
+	 */
+	ret = call_qop(q, buf_init, vb);
+	if (ret) {
+		dprintk(1, "qbuf: buffer initialization failed\n");
+		goto err;
+	}
+
+	/*
+	 * Now that everything is in order, copy relevant information
+	 * provided by userspace.
+	 */
+	for (plane = 0; plane < vb->num_planes; ++plane)
+		vb->v4l2_planes[plane] = planes[plane];
+
+	return 0;
+err:
+	/* In case of errors, release planes that were already acquired */
+	__vb2_buf_dmabuf_put(vb);
+
+	return ret;
+}
+
+/**
  * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
  */
 static void __enqueue_in_driver(struct vb2_buffer *vb)
@@ -980,6 +1149,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	case V4L2_MEMORY_USERPTR:
 		ret = __qbuf_userptr(vb, b);
 		break;
+	case V4L2_MEMORY_DMABUF:
+		ret = __qbuf_dmabuf(vb, b);
+		break;
 	default:
 		WARN(1, "Invalid queue type\n");
 		ret = -EINVAL;
@@ -1312,6 +1484,7 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
 {
 	struct vb2_buffer *vb = NULL;
 	int ret;
+	unsigned int plane;
 
 	if (q->fileio) {
 		dprintk(1, "dqbuf: file io in progress\n");
@@ -1335,6 +1508,15 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
 		return ret;
 	}
 
+	/* TODO: this unpins the buffer(dma_buf_unmap_attachment()).. but
+	 * really we want tot do this just after DMA, not when the
+	 * buffer is dequeued..
+	 */
+	if (q->memory == V4L2_MEMORY_DMABUF)
+		for (plane = 0; plane < vb->num_planes; ++plane)
+			call_memop(q, unmap_dmabuf,
+				vb->planes[plane].mem_priv);
+
 	switch (vb->state) {
 	case VB2_BUF_STATE_DONE:
 		dprintk(3, "dqbuf: Returning done buffer\n");
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index a15d1f1..665e846 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -16,6 +16,7 @@
 #include <linux/mutex.h>
 #include <linux/poll.h>
 #include <linux/videodev2.h>
+#include <linux/dma-buf.h>
 
 struct vb2_alloc_ctx;
 struct vb2_fileio_data;
@@ -41,6 +42,20 @@ struct vb2_fileio_data;
  *		 argument to other ops in this structure
  * @put_userptr: inform the allocator that a USERPTR buffer will no longer
  *		 be used
+ * @attach_dmabuf: attach a shared struct dma_buf for a hardware operation;
+ *		   used for DMABUF memory types; alloc_ctx is the alloc context
+ *		   dbuf is the shared dma_buf; returns NULL on failure;
+ *		   allocator private per-buffer structure on success;
+ *		   this needs to be used for further accesses to the buffer
+ * @detach_dmabuf: inform the exporter of the buffer that the current DMABUF
+ *		   buffer is no longer used; the buf_priv argument is the
+ *		   allocator private per-buffer structure previously returned
+ *		   from the attach_dmabuf callback
+ * @map_dmabuf: request for access to the dmabuf from allocator; the allocator
+ *		of dmabuf is informed that this driver is going to use the
+ *		dmabuf
+ * @unmap_dmabuf: releases access control to the dmabuf - allocator is notified
+ *		  that this driver is done using the dmabuf for now
  * @vaddr:	return a kernel virtual address to a given memory buffer
  *		associated with the passed private structure or NULL if no
  *		such mapping exists
@@ -56,6 +71,8 @@ struct vb2_fileio_data;
  * Required ops for USERPTR types: get_userptr, put_userptr.
  * Required ops for MMAP types: alloc, put, num_users, mmap.
  * Required ops for read/write access types: alloc, put, num_users, vaddr
+ * Required ops for DMABUF types: attach_dmabuf, detach_dmabuf, map_dmabuf,
+ *				  unmap_dmabuf.
  */
 struct vb2_mem_ops {
 	void		*(*alloc)(void *alloc_ctx, unsigned long size);
@@ -65,6 +82,17 @@ struct vb2_mem_ops {
 					unsigned long size, int write);
 	void		(*put_userptr)(void *buf_priv);
 
+	/*
+	 * Comment from Rob Clark: XXX: I think the attach / detach could be
+	 * handled in the vb2 core, and vb2_mem_ops really just need to get/put
+	 * the sglist (and make sure that the sglist fits it's needs..)
+	 */
+	void		*(*attach_dmabuf)(void *alloc_ctx, struct dma_buf *dbuf,
+				unsigned long size, int write);
+	void		(*detach_dmabuf)(void *buf_priv);
+	int		(*map_dmabuf)(void *buf_priv);
+	void		(*unmap_dmabuf)(void *buf_priv);
+
 	void		*(*vaddr)(void *buf_priv);
 	void		*(*cookie)(void *buf_priv);
 
@@ -75,6 +103,7 @@ struct vb2_mem_ops {
 
 struct vb2_plane {
 	void			*mem_priv;
+	struct dma_buf		*dbuf;
 };
 
 /**
@@ -83,12 +112,14 @@ struct vb2_plane {
  * @VB2_USERPTR:	driver supports USERPTR with streaming API
  * @VB2_READ:		driver supports read() style access
  * @VB2_WRITE:		driver supports write() style access
+ * @VB2_DMABUF:		driver supports DMABUF with streaming API
  */
 enum vb2_io_modes {
 	VB2_MMAP	= (1 << 0),
 	VB2_USERPTR	= (1 << 1),
 	VB2_READ	= (1 << 2),
 	VB2_WRITE	= (1 << 3),
+	VB2_DMABUF	= (1 << 4),
 };
 
 /**
-- 
1.7.5.4

[PATCH 04/11] v4l: vb: remove warnings about MEMORY_DMABUF

[Tomasz Stanislawski]

From: Sumit Semwal <sumit.semwal@ti.com>

Adding DMABUF memory type causes videobuf to complain about not using it
in some switch cases. This patch removes these warnings.

Signed-off-by: Sumit Semwal <sumit.semwal@ti.com>
---
 drivers/media/video/videobuf-core.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
index de4fa4e..b457c8b 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -335,6 +335,9 @@ static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b,
 	case V4L2_MEMORY_OVERLAY:
 		b->m.offset  = vb->boff;
 		break;
+	case V4L2_MEMORY_DMABUF:
+		/* DMABUF is not handled in videobuf framework */
+		break;
 	}
 
 	b->flags    = 0;
@@ -411,6 +414,7 @@ int __videobuf_mmap_setup(struct videobuf_queue *q,
 			break;
 		case V4L2_MEMORY_USERPTR:
 		case V4L2_MEMORY_OVERLAY:
+		case V4L2_MEMORY_DMABUF:
 			/* nothing */
 			break;
 		}
-- 
1.7.5.4

[PATCH 05/11] v4l: vb2-dma-contig: Shorten vb2_dma_contig prefix to vb2_dc

[Tomasz Stanislawski]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/videobuf2-dma-contig.c |   36 ++++++++++++++--------------
 1 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index f17ad98..5207eb1 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -31,9 +31,9 @@ struct vb2_dc_buf {
 	struct vb2_vmarea_handler	handler;
 };
 
-static void vb2_dma_contig_put(void *buf_priv);
+static void vb2_dc_put(void *buf_priv);
 
-static void *vb2_dma_contig_alloc(void *alloc_ctx, unsigned long size)
+static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 {
 	struct vb2_dc_conf *conf = alloc_ctx;
 	struct vb2_dc_buf *buf;
@@ -55,7 +55,7 @@ static void *vb2_dma_contig_alloc(void *alloc_ctx, unsigned long size)
 	buf->size = size;
 
 	buf->handler.refcount = &buf->refcount;
-	buf->handler.put = vb2_dma_contig_put;
+	buf->handler.put = vb2_dc_put;
 	buf->handler.arg = buf;
 
 	atomic_inc(&buf->refcount);
@@ -63,7 +63,7 @@ static void *vb2_dma_contig_alloc(void *alloc_ctx, unsigned long size)
 	return buf;
 }
 
-static void vb2_dma_contig_put(void *buf_priv)
+static void vb2_dc_put(void *buf_priv)
 {
 	struct vb2_dc_buf *buf = buf_priv;
 
@@ -74,14 +74,14 @@ static void vb2_dma_contig_put(void *buf_priv)
 	}
 }
 
-static void *vb2_dma_contig_cookie(void *buf_priv)
+static void *vb2_dc_cookie(void *buf_priv)
 {
 	struct vb2_dc_buf *buf = buf_priv;
 
 	return &buf->dma_addr;
 }
 
-static void *vb2_dma_contig_vaddr(void *buf_priv)
+static void *vb2_dc_vaddr(void *buf_priv)
 {
 	struct vb2_dc_buf *buf = buf_priv;
 	if (!buf)
@@ -90,14 +90,14 @@ static void *vb2_dma_contig_vaddr(void *buf_priv)
 	return buf->vaddr;
 }
 
-static unsigned int vb2_dma_contig_num_users(void *buf_priv)
+static unsigned int vb2_dc_num_users(void *buf_priv)
 {
 	struct vb2_dc_buf *buf = buf_priv;
 
 	return atomic_read(&buf->refcount);
 }
 
-static int vb2_dma_contig_mmap(void *buf_priv, struct vm_area_struct *vma)
+static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
 {
 	struct vb2_dc_buf *buf = buf_priv;
 
@@ -110,7 +110,7 @@ static int vb2_dma_contig_mmap(void *buf_priv, struct vm_area_struct *vma)
 				  &vb2_common_vm_ops, &buf->handler);
 }
 
-static void *vb2_dma_contig_get_userptr(void *alloc_ctx, unsigned long vaddr,
+static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
 					unsigned long size, int write)
 {
 	struct vb2_dc_buf *buf;
@@ -137,7 +137,7 @@ static void *vb2_dma_contig_get_userptr(void *alloc_ctx, unsigned long vaddr,
 	return buf;
 }
 
-static void vb2_dma_contig_put_userptr(void *mem_priv)
+static void vb2_dc_put_userptr(void *mem_priv)
 {
 	struct vb2_dc_buf *buf = mem_priv;
 
@@ -149,14 +149,14 @@ static void vb2_dma_contig_put_userptr(void *mem_priv)
 }
 
 const struct vb2_mem_ops vb2_dma_contig_memops = {
-	.alloc		= vb2_dma_contig_alloc,
-	.put		= vb2_dma_contig_put,
-	.cookie		= vb2_dma_contig_cookie,
-	.vaddr		= vb2_dma_contig_vaddr,
-	.mmap		= vb2_dma_contig_mmap,
-	.get_userptr	= vb2_dma_contig_get_userptr,
-	.put_userptr	= vb2_dma_contig_put_userptr,
-	.num_users	= vb2_dma_contig_num_users,
+	.alloc		= vb2_dc_alloc,
+	.put		= vb2_dc_put,
+	.cookie		= vb2_dc_cookie,
+	.vaddr		= vb2_dc_vaddr,
+	.mmap		= vb2_dc_mmap,
+	.get_userptr	= vb2_dc_get_userptr,
+	.put_userptr	= vb2_dc_put_userptr,
+	.num_users	= vb2_dc_num_users,
 };
 EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
 
-- 
1.7.5.4

[PATCH 06/11] v4l: vb2-dma-contig: Remove unneeded allocation context structure

[Tomasz Stanislawski]

vb2-dma-contig returns a vb2_dc_conf structure instance as the vb2
allocation context. That structure only stores a pointer to the physical
device. Remove it and use the device pointer directly as the allocation
context.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
---
 drivers/media/video/videobuf2-dma-contig.c |   29 ++++++---------------------
 1 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index 5207eb1..ff0a662 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -17,12 +17,8 @@
 #include <media/videobuf2-core.h>
 #include <media/videobuf2-memops.h>
 
-struct vb2_dc_conf {
-	struct device		*dev;
-};
-
 struct vb2_dc_buf {
-	struct vb2_dc_conf		*conf;
+	struct device			*dev;
 	void				*vaddr;
 	dma_addr_t			dma_addr;
 	unsigned long			size;
@@ -35,23 +31,21 @@ static void vb2_dc_put(void *buf_priv);
 
 static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 {
-	struct vb2_dc_conf *conf = alloc_ctx;
+	struct device *dev = alloc_ctx;
 	struct vb2_dc_buf *buf;
 
 	buf = kzalloc(sizeof *buf, GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
 
-	buf->vaddr = dma_alloc_coherent(conf->dev, size, &buf->dma_addr,
-					GFP_KERNEL);
+	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL);
 	if (!buf->vaddr) {
-		dev_err(conf->dev, "dma_alloc_coherent of size %ld failed\n",
-			size);
+		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
 		kfree(buf);
 		return ERR_PTR(-ENOMEM);
 	}
 
-	buf->conf = conf;
+	buf->dev = dev;
 	buf->size = size;
 
 	buf->handler.refcount = &buf->refcount;
@@ -68,7 +62,7 @@ static void vb2_dc_put(void *buf_priv)
 	struct vb2_dc_buf *buf = buf_priv;
 
 	if (atomic_dec_and_test(&buf->refcount)) {
-		dma_free_coherent(buf->conf->dev, buf->size, buf->vaddr,
+		dma_free_coherent(buf->dev, buf->size, buf->vaddr,
 				  buf->dma_addr);
 		kfree(buf);
 	}
@@ -162,21 +156,12 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
 
 void *vb2_dma_contig_init_ctx(struct device *dev)
 {
-	struct vb2_dc_conf *conf;
-
-	conf = kzalloc(sizeof *conf, GFP_KERNEL);
-	if (!conf)
-		return ERR_PTR(-ENOMEM);
-
-	conf->dev = dev;
-
-	return conf;
+	return dev;
 }
 EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx);
 
 void vb2_dma_contig_cleanup_ctx(void *alloc_ctx)
 {
-	kfree(alloc_ctx);
 }
 EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx);
 
-- 
1.7.5.4

[PATCH 07/11] v4l: vb2-dma-contig: Reorder functions

[Tomasz Stanislawski]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Group functions by buffer type.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/videobuf2-dma-contig.c |   92 ++++++++++++++++-----------
 1 files changed, 54 insertions(+), 38 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index ff0a662..476e536 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -20,14 +20,56 @@
 struct vb2_dc_buf {
 	struct device			*dev;
 	void				*vaddr;
-	dma_addr_t			dma_addr;
 	unsigned long			size;
-	struct vm_area_struct		*vma;
-	atomic_t			refcount;
+	dma_addr_t			dma_addr;
+
+	/* MMAP related */
 	struct vb2_vmarea_handler	handler;
+	atomic_t			refcount;
+
+	/* USERPTR related */
+	struct vm_area_struct		*vma;
 };
 
-static void vb2_dc_put(void *buf_priv);
+/*********************************************/
+/*         callbacks for all buffers         */
+/*********************************************/
+
+static void *vb2_dc_cookie(void *buf_priv)
+{
+	struct vb2_dc_buf *buf = buf_priv;
+
+	return &buf->dma_addr;
+}
+
+static void *vb2_dc_vaddr(void *buf_priv)
+{
+	struct vb2_dc_buf *buf = buf_priv;
+
+	return buf->vaddr;
+}
+
+static unsigned int vb2_dc_num_users(void *buf_priv)
+{
+	struct vb2_dc_buf *buf = buf_priv;
+
+	return atomic_read(&buf->refcount);
+}
+
+/*********************************************/
+/*        callbacks for MMAP buffers         */
+/*********************************************/
+
+static void vb2_dc_put(void *buf_priv)
+{
+	struct vb2_dc_buf *buf = buf_priv;
+
+	if (!atomic_dec_and_test(&buf->refcount))
+		return;
+
+	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
+	kfree(buf);
+}
 
 static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 {
@@ -57,40 +99,6 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 	return buf;
 }
 
-static void vb2_dc_put(void *buf_priv)
-{
-	struct vb2_dc_buf *buf = buf_priv;
-
-	if (atomic_dec_and_test(&buf->refcount)) {
-		dma_free_coherent(buf->dev, buf->size, buf->vaddr,
-				  buf->dma_addr);
-		kfree(buf);
-	}
-}
-
-static void *vb2_dc_cookie(void *buf_priv)
-{
-	struct vb2_dc_buf *buf = buf_priv;
-
-	return &buf->dma_addr;
-}
-
-static void *vb2_dc_vaddr(void *buf_priv)
-{
-	struct vb2_dc_buf *buf = buf_priv;
-	if (!buf)
-		return 0;
-
-	return buf->vaddr;
-}
-
-static unsigned int vb2_dc_num_users(void *buf_priv)
-{
-	struct vb2_dc_buf *buf = buf_priv;
-
-	return atomic_read(&buf->refcount);
-}
-
 static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
 {
 	struct vb2_dc_buf *buf = buf_priv;
@@ -104,6 +112,10 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
 				  &vb2_common_vm_ops, &buf->handler);
 }
 
+/*********************************************/
+/*       callbacks for USERPTR buffers       */
+/*********************************************/
+
 static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
 					unsigned long size, int write)
 {
@@ -142,6 +154,10 @@ static void vb2_dc_put_userptr(void *mem_priv)
 	kfree(buf);
 }
 
+/*********************************************/
+/*       DMA CONTIG exported functions       */
+/*********************************************/
+
 const struct vb2_mem_ops vb2_dma_contig_memops = {
 	.alloc		= vb2_dc_alloc,
 	.put		= vb2_dc_put,
-- 
1.7.5.4

[PATCH 08/11] v4l: vb2-dma-contig: add support for scatterlist in userptr mode

[Tomasz Stanislawski]

From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>

This patch introduces usage of dma_map_sg to map memory behind
a userspace pointer to a device as dma-contiguous mapping.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
	[bugfixing]
Signed-off-by: Kamil Debski <k.debski@samsung.com>
	[bugfixing]
Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
	[add sglist subroutines/code refactoring]
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/videobuf2-dma-contig.c |  282 ++++++++++++++++++++++++++--
 1 files changed, 265 insertions(+), 17 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index 476e536..6ab3165 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -11,6 +11,8 @@
  */
 
 #include <linux/module.h>
+#include <linux/scatterlist.h>
+#include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 
@@ -22,6 +24,8 @@ struct vb2_dc_buf {
 	void				*vaddr;
 	unsigned long			size;
 	dma_addr_t			dma_addr;
+	enum dma_data_direction		dma_dir;
+	struct sg_table			*dma_sgt;
 
 	/* MMAP related */
 	struct vb2_vmarea_handler	handler;
@@ -32,6 +36,98 @@ struct vb2_dc_buf {
 };
 
 /*********************************************/
+/*        scatterlist table functions        */
+/*********************************************/
+
+static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages,
+	unsigned long n_pages, size_t offset, size_t offset2)
+{
+	struct sg_table *sgt;
+	int i, j; /* loop counters */
+	int cur_page, chunks;
+	int ret;
+	struct scatterlist *s;
+
+	sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
+	if (!sgt)
+		return ERR_PTR(-ENOMEM);
+
+	/* compute number of chunks */
+	chunks = 1;
+	for (i = 1; i < n_pages; ++i)
+		if (pages[i] != pages[i - 1] + 1)
+			++chunks;
+
+	ret = sg_alloc_table(sgt, chunks, GFP_KERNEL);
+	if (ret) {
+		kfree(sgt);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	/* merging chunks and putting them into the scatterlist */
+	cur_page = 0;
+	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
+		size_t size = PAGE_SIZE;
+
+		for (j = cur_page + 1; j < n_pages; ++j) {
+			if (pages[j] != pages[j - 1] + 1)
+				break;
+			size += PAGE_SIZE;
+		}
+
+		/* cut offset if chunk starts at the first page */
+		if (cur_page == 0)
+			size -= offset;
+		/* cut offset2 if chunk ends at the last page */
+		if (j == n_pages)
+			size -= offset2;
+
+		sg_set_page(s, pages[cur_page], size, offset);
+		offset = 0;
+		cur_page = j;
+	}
+
+	return sgt;
+}
+
+static void vb2_dc_release_sgtable(struct sg_table *sgt)
+{
+	sg_free_table(sgt);
+	kfree(sgt);
+}
+
+static void vb2_dc_sgt_foreach_page(struct sg_table *sgt,
+	void (*cb)(struct page *pg))
+{
+	struct scatterlist *s;
+	int i, j;
+
+	for_each_sg(sgt->sgl, s, sgt->nents, i) {
+		struct page *page = sg_page(s);
+		int n_pages = PAGE_ALIGN(s->offset + s->length) >> PAGE_SHIFT;
+
+		for (j = 0; j < n_pages; ++j, ++page)
+			cb(page);
+	}
+}
+
+static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
+{
+	struct scatterlist *s;
+	dma_addr_t expected = sg_dma_address(sgt->sgl);
+	int i;
+	unsigned long size = 0;
+
+	for_each_sg(sgt->sgl, s, sgt->nents, i) {
+		if (sg_dma_address(s) != expected)
+			break;
+		expected = sg_dma_address(s) + sg_dma_len(s);
+		size += sg_dma_len(s);
+	}
+	return size;
+}
+
+/*********************************************/
 /*         callbacks for all buffers         */
 /*********************************************/
 
@@ -116,42 +212,194 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
 /*       callbacks for USERPTR buffers       */
 /*********************************************/
 
+static inline int vma_is_io(struct vm_area_struct *vma)
+{
+	return !!(vma->vm_flags & (VM_IO | VM_PFNMAP));
+}
+
+static struct vm_area_struct *vb2_dc_get_user_vma(
+	unsigned long start, unsigned long size)
+{
+	struct vm_area_struct *vma;
+
+	/* current->mm->mmap_sem is taken by videobuf2 core */
+	vma = find_vma(current->mm, start);
+	if (!vma) {
+		printk(KERN_ERR "no vma for address %lu\n", start);
+		return ERR_PTR(-EFAULT);
+	}
+
+	if (vma->vm_end - vma->vm_start < size) {
+		printk(KERN_ERR "vma at %lu is too small for %lu bytes\n",
+			start, size);
+		return ERR_PTR(-EFAULT);
+	}
+
+	vma = vb2_get_vma(vma);
+	if (!vma) {
+		printk(KERN_ERR "failed to copy vma\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	return vma;
+}
+
+static int vb2_dc_get_user_pages(unsigned long start, struct page **pages,
+	int n_pages, struct vm_area_struct *vma, int write)
+{
+	int n;
+
+	if (vma_is_io(vma)) {
+		for (n = 0; n < n_pages; ++n, start += PAGE_SIZE) {
+			unsigned long pfn;
+			int ret = follow_pfn(vma, start, &pfn);
+
+			if (ret) {
+				printk(KERN_ERR "no page for address %lu\n",
+					start);
+				return ret;
+			}
+			pages[n] = pfn_to_page(pfn);
+		}
+	} else {
+		n = get_user_pages(current, current->mm, start & PAGE_MASK,
+			n_pages, write, 1, pages, NULL);
+		if (n != n_pages) {
+			printk(KERN_ERR "got only %d of %d user pages\n",
+				n, n_pages);
+			while (n)
+				put_page(pages[--n]);
+			return -EFAULT;
+		}
+	}
+
+	return 0;
+}
+
+static void vb2_dc_set_page_dirty(struct page *page)
+{
+	set_page_dirty_lock(page);
+}
+
+static void vb2_dc_put_userptr(void *buf_priv)
+{
+	struct vb2_dc_buf *buf = buf_priv;
+	struct sg_table *sgt = buf->dma_sgt;
+
+	dma_unmap_sg(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
+	if (!vma_is_io(buf->vma)) {
+		vb2_dc_sgt_foreach_page(sgt, vb2_dc_set_page_dirty);
+		vb2_dc_sgt_foreach_page(sgt, put_page);
+	}
+
+	vb2_dc_release_sgtable(sgt);
+	vb2_put_vma(buf->vma);
+	kfree(buf);
+}
+
 static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
-					unsigned long size, int write)
+	unsigned long size, int write)
 {
 	struct vb2_dc_buf *buf;
-	struct vm_area_struct *vma;
-	dma_addr_t dma_addr = 0;
-	int ret;
+	unsigned long start, end, offset, offset2;
+	struct page **pages;
+	int n_pages;
+	int ret = 0;
+	struct sg_table *sgt;
+	unsigned long contig_size;
 
 	buf = kzalloc(sizeof *buf, GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
 
-	ret = vb2_get_contig_userptr(vaddr, size, &vma, &dma_addr);
+	buf->dev = alloc_ctx;
+	buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+
+	start = (unsigned long)vaddr & PAGE_MASK;
+	offset = (unsigned long)vaddr & ~PAGE_MASK;
+	end = PAGE_ALIGN((unsigned long)vaddr + size);
+	offset2 = end - (unsigned long)vaddr - size;
+	n_pages = (end - start) >> PAGE_SHIFT;
+
+	pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
+	if (!pages) {
+		ret = -ENOMEM;
+		printk(KERN_ERR "failed to allocate pages table\n");
+		goto fail_buf;
+	}
+
+	buf->vma = vb2_dc_get_user_vma(start, end - start);
+	if (IS_ERR(buf->vma)) {
+		printk(KERN_ERR "failed to get VMA\n");
+		ret = PTR_ERR(buf->vma);
+		goto fail_pages;
+	}
+
+	/* extract page list from userspace mapping */
+	ret = vb2_dc_get_user_pages(start, pages, n_pages, buf->vma, write);
 	if (ret) {
-		printk(KERN_ERR "Failed acquiring VMA for vaddr 0x%08lx\n",
-				vaddr);
-		kfree(buf);
-		return ERR_PTR(ret);
+		printk(KERN_ERR "failed to get user pages\n");
+		goto fail_vma;
+	}
+
+	sgt = vb2_dc_pages_to_sgt(pages, n_pages, offset, offset2);
+	if (IS_ERR(sgt)) {
+		printk(KERN_ERR "failed to create scatterlist table\n");
+		ret = -ENOMEM;
+		goto fail_get_user_pages;
+	}
+
+	/* pages are no longer needed */
+	kfree(pages);
+	pages = NULL;
+
+	sgt->nents = dma_map_sg(buf->dev, sgt->sgl, sgt->orig_nents,
+		buf->dma_dir);
+	if (sgt->nents <= 0) {
+		printk(KERN_ERR "failed to map scatterlist\n");
+		ret = -EIO;
+		goto fail_sgt;
+	}
+
+	contig_size = vb2_dc_get_contiguous_size(sgt);
+	if (contig_size < size) {
+		printk(KERN_ERR "contiguous mapping is too small %lu/%lu\n",
+			contig_size, size);
+		ret = -EFAULT;
+		goto fail_map_sg;
 	}
 
+	buf->dma_addr = sg_dma_address(sgt->sgl);
 	buf->size = size;
-	buf->dma_addr = dma_addr;
-	buf->vma = vma;
+	buf->dma_sgt = sgt;
+
+	atomic_inc(&buf->refcount);
 
 	return buf;
-}
 
-static void vb2_dc_put_userptr(void *mem_priv)
-{
-	struct vb2_dc_buf *buf = mem_priv;
+fail_map_sg:
+	dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
 
-	if (!buf)
-		return;
+fail_sgt:
+	if (!vma_is_io(buf->vma))
+		vb2_dc_sgt_foreach_page(sgt, put_page);
+	vb2_dc_release_sgtable(sgt);
+
+fail_get_user_pages:
+	if (pages && !vma_is_io(buf->vma))
+		while (n_pages)
+			put_page(pages[--n_pages]);
 
+fail_vma:
 	vb2_put_vma(buf->vma);
+
+fail_pages:
+	kfree(pages); /* kfree is NULL-proof */
+
+fail_buf:
 	kfree(buf);
+
+	return ERR_PTR(ret);
 }
 
 /*********************************************/
-- 
1.7.5.4

[PATCH 09/11] v4l: vb2: add prepare/finish callbacks to allocators

[Tomasz Stanislawski]

From: Marek Szyprowski <m.szyprowski@samsung.com>

This patch adds support for prepare/finish callbacks in VB2 allocators. These
callback are used for buffer flushing.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/video/videobuf2-core.c |   11 +++++++++++
 include/media/videobuf2-core.h       |    7 +++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index b37feea..abb0592 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -834,6 +834,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 {
 	struct vb2_queue *q = vb->vb2_queue;
 	unsigned long flags;
+	int plane;
 
 	if (vb->state != VB2_BUF_STATE_ACTIVE)
 		return;
@@ -844,6 +845,10 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	dprintk(4, "Done processing on buffer %d, state: %d\n",
 			vb->v4l2_buf.index, vb->state);
 
+	/* sync buffers */
+	for (plane = 0; plane < vb->num_planes; ++plane)
+		call_memop(q, finish, vb->planes[plane].mem_priv);
+
 	/* Add the buffer to the done buffers list */
 	spin_lock_irqsave(&q->done_lock, flags);
 	vb->state = state;
@@ -1131,9 +1136,15 @@ err:
 static void __enqueue_in_driver(struct vb2_buffer *vb)
 {
 	struct vb2_queue *q = vb->vb2_queue;
+	int plane;
 
 	vb->state = VB2_BUF_STATE_ACTIVE;
 	atomic_inc(&q->queued_count);
+
+	/* sync buffers */
+	for (plane = 0; plane < vb->num_planes; ++plane)
+		call_memop(q, prepare, vb->planes[plane].mem_priv);
+
 	q->ops->buf_queue(vb);
 }
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 665e846..244165a 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -56,6 +56,10 @@ struct vb2_fileio_data;
  *		dmabuf
  * @unmap_dmabuf: releases access control to the dmabuf - allocator is notified
  *		  that this driver is done using the dmabuf for now
+ * @prepare:	called everytime the buffer is passed from userspace to the
+ *		driver, usefull for cache synchronisation, optional
+ * @finish:	called everytime the buffer is passed back from the driver
+ *		to the userspace, also optional
  * @vaddr:	return a kernel virtual address to a given memory buffer
  *		associated with the passed private structure or NULL if no
  *		such mapping exists
@@ -82,6 +86,9 @@ struct vb2_mem_ops {
 					unsigned long size, int write);
 	void		(*put_userptr)(void *buf_priv);
 
+	void		(*prepare)(void *buf_priv);
+	void		(*finish)(void *buf_priv);
+
 	/*
 	 * Comment from Rob Clark: XXX: I think the attach / detach could be
 	 * handled in the vb2 core, and vb2_mem_ops really just need to get/put
-- 
1.7.5.4

[PATCH 10/11] v4l: vb2-dma-contig: add prepare/finish to dma-contig allocator

[Tomasz Stanislawski]

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>
---
 drivers/media/video/videobuf2-dma-contig.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index 6ab3165..30f316588 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -152,6 +152,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         */
 /*********************************************/
@@ -414,6 +436,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);
-- 
1.7.5.4

[PATCH 11/11] v4l: vb2: Add dma-contig allocator as dma_buf user

[Tomasz Stanislawski]

From: Sumit Semwal <sumit.semwal@ti.com>

This patch makes changes for adding dma-contig as a dma_buf user. It provides
function implementations for the {attach, detach, map, unmap}_dmabuf()
mem_ops of DMABUF memory type.

Signed-off-by: Sumit Semwal <sumit.semwal@ti.com>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
	[author of the original patch]
Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
	[integration with refactored dma-contig allocator]
---
 drivers/media/video/videobuf2-dma-contig.c |  117 ++++++++++++++++++++++++++++
 1 files changed, 117 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index 30f316588..6329483 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -10,6 +10,7 @@
  * the Free Software Foundation.
  */
 
+#include <linux/dma-buf.h>
 #include <linux/module.h>
 #include <linux/scatterlist.h>
 #include <linux/sched.h>
@@ -33,6 +34,9 @@ struct vb2_dc_buf {
 
 	/* USERPTR related */
 	struct vm_area_struct		*vma;
+
+	/* DMABUF related */
+	struct dma_buf_attachment	*db_attach;
 };
 
 /*********************************************/
@@ -425,6 +429,115 @@ fail_buf:
 }
 
 /*********************************************/
+/*       callbacks for DMABUF buffers        */
+/*********************************************/
+
+static int vb2_dc_map_dmabuf(void *mem_priv)
+{
+	struct vb2_dc_buf *buf = mem_priv;
+	struct sg_table *sgt;
+	unsigned long contig_size;
+	int ret = 0;
+
+	if (WARN_ON(!buf->db_attach)) {
+		printk(KERN_ERR "trying to pin a non attached buffer\n");
+		return -EINVAL;
+	}
+
+	if (WARN_ON(buf->dma_sgt)) {
+		printk(KERN_ERR "dmabuf buffer is already pinned\n");
+		return 0;
+	}
+
+	/* get the associated scatterlist for this buffer */
+	sgt = dma_buf_map_attachment(buf->db_attach, buf->dma_dir);
+	if (IS_ERR_OR_NULL(sgt)) {
+		printk(KERN_ERR "Error getting dmabuf scatterlist\n");
+		return -EINVAL;
+	}
+
+	/* checking if dmabuf is big enough to store contiguous chunk */
+	contig_size = vb2_dc_get_contiguous_size(sgt);
+	if (contig_size < buf->size) {
+		printk(KERN_ERR "contiguous chunk of dmabuf is too small\n");
+		ret = -EFAULT;
+		goto fail_map;
+	}
+
+	buf->dma_addr = sg_dma_address(sgt->sgl);
+	buf->dma_sgt = sgt;
+
+	return 0;
+
+fail_map:
+	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
+
+	return ret;
+}
+
+static void vb2_dc_unmap_dmabuf(void *mem_priv)
+{
+	struct vb2_dc_buf *buf = mem_priv;
+	struct sg_table *sgt = buf->dma_sgt;
+
+	if (WARN_ON(!buf->db_attach)) {
+		printk(KERN_ERR "trying to unpin a not attached buffer\n");
+		return;
+	}
+
+	if (WARN_ON(!sgt)) {
+		printk(KERN_ERR "dmabuf buffer is already unpinned\n");
+		return;
+	}
+
+	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
+
+	buf->dma_addr = 0;
+	buf->dma_sgt = NULL;
+}
+
+static void vb2_dc_detach_dmabuf(void *mem_priv)
+{
+	struct vb2_dc_buf *buf = mem_priv;
+
+	if (buf->dma_addr)
+		vb2_dc_unmap_dmabuf(buf);
+
+	/* detach this attachment */
+	dma_buf_detach(buf->db_attach->dmabuf, buf->db_attach);
+	kfree(buf);
+}
+
+static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf,
+	unsigned long size, int write)
+{
+	struct vb2_dc_buf *buf;
+	struct dma_buf_attachment *dba;
+
+	if (dbuf->size < size)
+		return ERR_PTR(-EFAULT);
+
+	buf = kzalloc(sizeof *buf, GFP_KERNEL);
+	if (!buf)
+		return ERR_PTR(-ENOMEM);
+
+	buf->dev = alloc_ctx;
+	/* create attachment for the dmabuf with the user device */
+	dba = dma_buf_attach(dbuf, buf->dev);
+	if (IS_ERR(dba)) {
+		printk(KERN_ERR "failed to attach dmabuf\n");
+		kfree(buf);
+		return dba;
+	}
+
+	buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	buf->size = size;
+	buf->db_attach = dba;
+
+	return buf;
+}
+
+/*********************************************/
 /*       DMA CONTIG exported functions       */
 /*********************************************/
 
@@ -438,6 +551,10 @@ const struct vb2_mem_ops vb2_dma_contig_memops = {
 	.put_userptr	= vb2_dc_put_userptr,
 	.prepare	= vb2_dc_prepare,
 	.finish		= vb2_dc_finish,
+	.map_dmabuf	= vb2_dc_map_dmabuf,
+	.unmap_dmabuf	= vb2_dc_unmap_dmabuf,
+	.attach_dmabuf	= vb2_dc_attach_dmabuf,
+	.detach_dmabuf	= vb2_dc_detach_dmabuf,
 	.num_users	= vb2_dc_num_users,
 };
 EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
-- 
1.7.5.4

Re: [PATCH 02/11] Documentation: media: description of DMABUF importing in V4L2

[Rémi Denis-Courmont]

Le jeudi 5 avril 2012 16:59:59 Tomasz Stanislawski, vous avez écrit :
> 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>
> ---
>  Documentation/DocBook/media/v4l/compat.xml         |    4 +
>  Documentation/DocBook/media/v4l/io.xml             |  177
> ++++++++++++++++++++ .../DocBook/media/v4l/vidioc-create-bufs.xml       | 
>   1 +
>  Documentation/DocBook/media/v4l/vidioc-qbuf.xml    |   15 ++
>  Documentation/DocBook/media/v4l/vidioc-reqbufs.xml |    8 +-
>  5 files changed, 203 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/compat.xml
> b/Documentation/DocBook/media/v4l/compat.xml index bce97c5..2a2083d 100644
> --- a/Documentation/DocBook/media/v4l/compat.xml
> +++ b/Documentation/DocBook/media/v4l/compat.xml
> @@ -2523,6 +2523,10 @@ ioctls.</para>
>          <listitem>
>  	  <para>Selection API. <xref linkend="selection-api" /></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 b815929..2a32363 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -472,6 +472,160 @@ 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 was introduced to provide a generic mean for
> sharing +buffers between multiple HW devices. Some device drivers provide
> an API that +may be capable to export a DMA buffer as a DMABUF file
> descriptor. The other +device drivers may be configured to use such a
> buffer. They implements an API +for importing of DMABUF. This section
> describes the support of DMABUF importing +in V4L2.</para>
> +
> +    <para>Input and output devices support this 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. If the particular
> importing buffer via +DMABUF file descriptors method is supported must be
> determined by calling the +&VIDIOC-REQBUFS; ioctl.</para>
> +
> +    <para>This I/O method is dedicated for sharing DMA buffers between V4L
> and +other APIs. Buffers (planes) are allocated by the application itself
> using +a client API. The buffers must be exported as DMABUF file
> descriptor.  Only those +file descriptor are exchanged, these files and
> meta-information are passed in +&v4l2-buffer; (or in &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>
> +
> +    <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;
> +
> +if (ioctl (fd, &VIDIOC-REQBUFS;, &amp;reqbuf) == -1) {
> +	if (errno == EINVAL)
> +		printf ("Video capturing or user pointer streaming is not 
supported\n");

User pointer??

> +	else
> +		perror ("VIDIOC_REQBUFS");
> +
> +	exit (EXIT_FAILURE);
> +}
> +      </programlisting>
> +    </example>
> +
> +    <para>Buffer (plane) file is passed on the fly with the &VIDIOC-QBUF;
> +ioctl. In case of multiplanar buffers, every plane can be associated with
> a +different DMABUF descriptor.Although buffers are commonly cycled,
> applications +can pass 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 unpin the buffer at any
> +time between the completion of the DMA and this ioctl. The memory is
> +also unpinned 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>
> +
> +    <para>To start and stop capturing or output applications call the
> +&VIDIOC-STREAMON; and &VIDIOC-STREAMOFF; ioctl. Note
> +<constant>VIDIOC_STREAMOFF</constant> removes all buffers from both queues
> and +unlocks/unpins 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 user pointer I/O must support the

User pointer again??


-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis

Re: [PATCH 03/11] v4l: vb2: add support for shared buffer (dma_buf)

[Rémi Denis-Courmont]

Le jeudi 5 avril 2012 17:00:00 Tomasz Stanislawski, vous avez écrit :
> From: Sumit Semwal <sumit.semwal@ti.com>
> 
> This patch adds support for DMABUF memory type in videobuf2. It calls
> relevant APIs of dma_buf for v4l reqbuf / qbuf / dqbuf operations.
> 
> For this version, the support is for videobuf2 as a user of the shared
> buffer; so the allocation of the buffer is done outside of V4L2. [A sample
> allocator of dma-buf shared buffer is given at [1]]
> 
> [1]: Rob Clark's DRM:
>    https://github.com/robclark/kernel-omap4/commits/drmplane-dmabuf
> 
> 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>
> ---
>  drivers/media/video/videobuf2-core.c |  184
> +++++++++++++++++++++++++++++++++- include/media/videobuf2-core.h       | 
>  31 ++++++
>  2 files changed, 214 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c index 2e8f1df..b37feea 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -106,6 +106,27 @@ static void __vb2_buf_userptr_put(struct vb2_buffer
> *vb) }
> 
>  /**
> + * __vb2_buf_dmabuf_put() - release memory associated with
> + * a DMABUF shared buffer
> + */
> +static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
> +{
> +	struct vb2_queue *q = vb->vb2_queue;
> +	unsigned int plane;
> +
> +	for (plane = 0; plane < vb->num_planes; ++plane) {
> +		void *mem_priv = vb->planes[plane].mem_priv;
> +
> +		if (mem_priv) {
> +			call_memop(q, detach_dmabuf, mem_priv);
> +			dma_buf_put(vb->planes[plane].dbuf);
> +			vb->planes[plane].dbuf = NULL;
> +			vb->planes[plane].mem_priv = NULL;
> +		}
> +	}
> +}
> +
> +/**
>   * __setup_offsets() - setup unique offsets ("cookies") for every plane in
>   * every buffer on the queue
>   */
> @@ -227,6 +248,8 @@ static void __vb2_free_mem(struct vb2_queue *q,
> unsigned int buffers) /* Free MMAP buffers or release USERPTR buffers */
>  		if (q->memory == V4L2_MEMORY_MMAP)
>  			__vb2_buf_mem_free(vb);

Missing 'else' here? A switch() statement might be better?

> +		if (q->memory == V4L2_MEMORY_DMABUF)
> +			__vb2_buf_dmabuf_put(vb);
>  		else
>  			__vb2_buf_userptr_put(vb);
>  	}
> @@ -349,6 +372,12 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb,
> struct v4l2_buffer *b) */
>  		memcpy(b->m.planes, vb->v4l2_planes,
>  			b->length * sizeof(struct v4l2_plane));
> +
> +		if (q->memory == V4L2_MEMORY_DMABUF) {
> +			unsigned int plane;
> +			for (plane = 0; plane < vb->num_planes; ++plane)
> +				b->m.planes[plane].m.fd = 0;
> +		}
>  	} else {
>  		/*
>  		 * We use length and offset in v4l2_planes array even for
> @@ -360,6 +389,8 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb,
> struct v4l2_buffer *b) b->m.offset = vb->v4l2_planes[0].m.mem_offset;
>  		else if (q->memory == V4L2_MEMORY_USERPTR)
>  			b->m.userptr = vb->v4l2_planes[0].m.userptr;
> +		else if (q->memory == V4L2_MEMORY_DMABUF)
> +			b->m.fd = 0;
>  	}
> 
>  	/*
> @@ -451,6 +482,21 @@ static int __verify_mmap_ops(struct vb2_queue *q)
>  }
> 
>  /**
> + * __verify_dmabuf_ops() - verify that all memory operations required for
> + * DMABUF queue type have been provided
> + */
> +static int __verify_dmabuf_ops(struct vb2_queue *q)
> +{
> +	if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf
> +			|| !q->mem_ops->detach_dmabuf
> +			|| !q->mem_ops->map_dmabuf
> +			|| !q->mem_ops->unmap_dmabuf)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/**
>   * vb2_reqbufs() - Initiate streaming
>   * @q:		videobuf2 queue
>   * @req:	struct passed from userspace to vidioc_reqbufs handler in 
driver
> @@ -484,6 +530,7 @@ int vb2_reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req) }
> 
>  	if (req->memory != V4L2_MEMORY_MMAP
> +			&& req->memory != V4L2_MEMORY_DMABUF
>  			&& req->memory != V4L2_MEMORY_USERPTR) {
>  		dprintk(1, "reqbufs: unsupported memory type\n");
>  		return -EINVAL;
> @@ -513,6 +560,11 @@ int vb2_reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req) return -EINVAL;
>  	}
> 
> +	if (req->memory == V4L2_MEMORY_DMABUF && __verify_dmabuf_ops(q)) {
> +		dprintk(1, "reqbufs: DMABUF for current setup unsupported\n");
> +		return -EINVAL;
> +	}
> +
>  	if (req->count == 0 || q->num_buffers != 0 || q->memory != req->memory) {
>  		/*
>  		 * We already have buffers allocated, so first check if they
> @@ -620,7 +672,8 @@ int vb2_create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create) }
> 
>  	if (create->memory != V4L2_MEMORY_MMAP
> -			&& create->memory != V4L2_MEMORY_USERPTR) {
> +			&& create->memory != V4L2_MEMORY_USERPTR
> +			&& create->memory != V4L2_MEMORY_DMABUF) {
>  		dprintk(1, "%s(): unsupported memory type\n", __func__);
>  		return -EINVAL;
>  	}
> @@ -644,6 +697,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create) return -EINVAL;
>  	}
> 
> +	if (create->memory == V4L2_MEMORY_DMABUF && __verify_dmabuf_ops(q)) {
> +		dprintk(1, "%s(): DMABUF for current setup unsupported\n", __func__);
> +		return -EINVAL;
> +	}
> +
>  	if (q->num_buffers == VIDEO_MAX_FRAME) {
>  		dprintk(1, "%s(): maximum number of buffers already allocated\n",
>  			__func__);
> @@ -839,6 +897,14 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
> const struct v4l2_buffer *b, b->m.planes[plane].length;
>  			}
>  		}
> +		if (b->memory == V4L2_MEMORY_DMABUF) {
> +			for (plane = 0; plane < vb->num_planes; ++plane) {
> +				v4l2_planes[plane].bytesused =
> +					b->m.planes[plane].bytesused;
> +				v4l2_planes[plane].m.fd =
> +					b->m.planes[plane].m.fd;
> +			}
> +		}
>  	} else {
>  		/*
>  		 * Single-planar buffers do not use planes array,
> @@ -853,6 +919,10 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
> const struct v4l2_buffer *b, v4l2_planes[0].m.userptr = b->m.userptr;
>  			v4l2_planes[0].length = b->length;
>  		}
> +
> +		if (b->memory == V4L2_MEMORY_DMABUF)
> +			v4l2_planes[0].m.fd = b->m.fd;
> +
>  	}
> 
>  	vb->v4l2_buf.field = b->field;
> @@ -957,6 +1027,105 @@ static int __qbuf_mmap(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) }
> 
>  /**
> + * __qbuf_dmabuf() - handle qbuf of a DMABUF buffer
> + */
> +static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer
> *b) +{
> +	struct v4l2_plane planes[VIDEO_MAX_PLANES];
> +	struct vb2_queue *q = vb->vb2_queue;
> +	void *mem_priv;
> +	unsigned int plane;
> +	int ret;
> +	int write = !V4L2_TYPE_IS_OUTPUT(q->type);
> +
> +	/* Verify and copy relevant information provided by the userspace */
> +	ret = __fill_vb2_buffer(vb, b, planes);
> +	if (ret)
> +		return ret;
> +
> +	for (plane = 0; plane < vb->num_planes; ++plane) {
> +		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
> +
> +		if (IS_ERR_OR_NULL(dbuf)) {
> +			dprintk(1, "qbuf: invalid dmabuf fd for "
> +				"plane %d\n", plane);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		/* Skip the plane if already verified */
> +		if (dbuf == vb->planes[plane].dbuf) {
> +			planes[plane].length = dbuf->size;
> +			dma_buf_put(dbuf);
> +			continue;
> +		}
> +
> +		dprintk(3, "qbuf: buffer description for plane %d changed, "
> +			"reattaching dma buf\n", plane);
> +
> +		/* Release previously acquired memory if present */
> +		if (vb->planes[plane].mem_priv) {
> +			call_memop(q, detach_dmabuf,
> +				vb->planes[plane].mem_priv);
> +			dma_buf_put(vb->planes[plane].dbuf);
> +		}
> +
> +		vb->planes[plane].mem_priv = NULL;
> +
> +		/* Acquire each plane's memory */
> +		mem_priv = call_memop(q, attach_dmabuf, q->alloc_ctx[plane],
> +			dbuf, q->plane_sizes[plane], write);
> +		if (IS_ERR(mem_priv)) {
> +			dprintk(1, "qbuf: failed acquiring dmabuf "
> +				"memory for plane %d\n", plane);
> +			ret = PTR_ERR(mem_priv);
> +			goto err;
> +		}
> +
> +		planes[plane].length = dbuf->size;
> +		vb->planes[plane].dbuf = dbuf;
> +		vb->planes[plane].mem_priv = mem_priv;
> +	}
> +
> +	/* TODO: This pins the buffer(s) with  dma_buf_map_attachment()).. but
> +	 * really we want to do this just before the DMA, not while queueing
> +	 * the buffer(s)..
> +	 */
> +	for (plane = 0; plane < vb->num_planes; ++plane) {
> +		ret = call_memop(q, map_dmabuf, vb->planes[plane].mem_priv);
> +		if (ret) {
> +			dprintk(1, "qbuf: failed mapping dmabuf "
> +				"memory for plane %d\n", plane);
> +			goto err;
> +		}
> +	}
> +
> +	/*
> +	 * Call driver-specific initialization on the newly acquired buffer,
> +	 * if provided.
> +	 */
> +	ret = call_qop(q, buf_init, vb);
> +	if (ret) {
> +		dprintk(1, "qbuf: buffer initialization failed\n");
> +		goto err;
> +	}
> +
> +	/*
> +	 * Now that everything is in order, copy relevant information
> +	 * provided by userspace.
> +	 */
> +	for (plane = 0; plane < vb->num_planes; ++plane)
> +		vb->v4l2_planes[plane] = planes[plane];
> +
> +	return 0;
> +err:
> +	/* In case of errors, release planes that were already acquired */
> +	__vb2_buf_dmabuf_put(vb);
> +
> +	return ret;
> +}
> +
> +/**
>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>   */
>  static void __enqueue_in_driver(struct vb2_buffer *vb)
> @@ -980,6 +1149,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) case V4L2_MEMORY_USERPTR:
>  		ret = __qbuf_userptr(vb, b);
>  		break;
> +	case V4L2_MEMORY_DMABUF:
> +		ret = __qbuf_dmabuf(vb, b);
> +		break;
>  	default:
>  		WARN(1, "Invalid queue type\n");
>  		ret = -EINVAL;
> @@ -1312,6 +1484,7 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer
> *b, bool nonblocking) {
>  	struct vb2_buffer *vb = NULL;
>  	int ret;
> +	unsigned int plane;
> 
>  	if (q->fileio) {
>  		dprintk(1, "dqbuf: file io in progress\n");
> @@ -1335,6 +1508,15 @@ int vb2_dqbuf(struct vb2_queue *q, struct
> v4l2_buffer *b, bool nonblocking) return ret;
>  	}
> 
> +	/* TODO: this unpins the buffer(dma_buf_unmap_attachment()).. but
> +	 * really we want tot do this just after DMA, not when the

Typo.

> +	 * buffer is dequeued..
> +	 */
> +	if (q->memory == V4L2_MEMORY_DMABUF)
> +		for (plane = 0; plane < vb->num_planes; ++plane)
> +			call_memop(q, unmap_dmabuf,
> +				vb->planes[plane].mem_priv);
> +
>  	switch (vb->state) {
>  	case VB2_BUF_STATE_DONE:
>  		dprintk(3, "dqbuf: Returning done buffer\n");
> diff --git a/include/media/videobuf2-core.h
> b/include/media/videobuf2-core.h index a15d1f1..665e846 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -16,6 +16,7 @@
>  #include <linux/mutex.h>
>  #include <linux/poll.h>
>  #include <linux/videodev2.h>
> +#include <linux/dma-buf.h>
> 
>  struct vb2_alloc_ctx;
>  struct vb2_fileio_data;
> @@ -41,6 +42,20 @@ struct vb2_fileio_data;
>   *		 argument to other ops in this structure
>   * @put_userptr: inform the allocator that a USERPTR buffer will no longer
>   *		 be used
> + * @attach_dmabuf: attach a shared struct dma_buf for a hardware
> operation; + *		   used for DMABUF memory types; alloc_ctx is the 
alloc
> context + *		   dbuf is the shared dma_buf; returns NULL on failure;
> + *		   allocator private per-buffer structure on success;
> + *		   this needs to be used for further accesses to the buffer
> + * @detach_dmabuf: inform the exporter of the buffer that the current
> DMABUF + *		   buffer is no longer used; the buf_priv argument is the
> + *		   allocator private per-buffer structure previously returned
> + *		   from the attach_dmabuf callback
> + * @map_dmabuf: request for access to the dmabuf from allocator; the
> allocator + *		of dmabuf is informed that this driver is going to use 
the
> + *		dmabuf
> + * @unmap_dmabuf: releases access control to the dmabuf - allocator is
> notified + *		  that this driver is done using the dmabuf for now
>   * @vaddr:	return a kernel virtual address to a given memory buffer
>   *		associated with the passed private structure or NULL if no
>   *		such mapping exists
> @@ -56,6 +71,8 @@ struct vb2_fileio_data;
>   * Required ops for USERPTR types: get_userptr, put_userptr.
>   * Required ops for MMAP types: alloc, put, num_users, mmap.
>   * Required ops for read/write access types: alloc, put, num_users, vaddr
> + * Required ops for DMABUF types: attach_dmabuf, detach_dmabuf,
> map_dmabuf, + *				  unmap_dmabuf.
>   */
>  struct vb2_mem_ops {
>  	void		*(*alloc)(void *alloc_ctx, unsigned long size);
> @@ -65,6 +82,17 @@ struct vb2_mem_ops {
>  					unsigned long size, int write);
>  	void		(*put_userptr)(void *buf_priv);
> 
> +	/*
> +	 * Comment from Rob Clark: XXX: I think the attach / detach could be
> +	 * handled in the vb2 core, and vb2_mem_ops really just need to get/put
> +	 * the sglist (and make sure that the sglist fits it's needs..)
> +	 */
> +	void		*(*attach_dmabuf)(void *alloc_ctx, struct dma_buf *dbuf,
> +				unsigned long size, int write);
> +	void		(*detach_dmabuf)(void *buf_priv);
> +	int		(*map_dmabuf)(void *buf_priv);
> +	void		(*unmap_dmabuf)(void *buf_priv);
> +
>  	void		*(*vaddr)(void *buf_priv);
>  	void		*(*cookie)(void *buf_priv);
> 
> @@ -75,6 +103,7 @@ struct vb2_mem_ops {
> 
>  struct vb2_plane {
>  	void			*mem_priv;
> +	struct dma_buf		*dbuf;
>  };
> 
>  /**
> @@ -83,12 +112,14 @@ struct vb2_plane {
>   * @VB2_USERPTR:	driver supports USERPTR with streaming API
>   * @VB2_READ:		driver supports read() style access
>   * @VB2_WRITE:		driver supports write() style access
> + * @VB2_DMABUF:		driver supports DMABUF with streaming API
>   */
>  enum vb2_io_modes {
>  	VB2_MMAP	= (1 << 0),
>  	VB2_USERPTR	= (1 << 1),
>  	VB2_READ	= (1 << 2),
>  	VB2_WRITE	= (1 << 3),
> +	VB2_DMABUF	= (1 << 4),
>  };
> 
>  /**


-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis

Re: [PATCH 02/11] Documentation: media: description of DMABUF importing in V4L2

[Tomasz Stanislawski]

Hello,
Thank you for review. Please refer to comments below.

On 04/05/2012 04:58 PM, Rémi Denis-Courmont wrote:
> Le jeudi 5 avril 2012 16:59:59 Tomasz Stanislawski, vous avez écrit :
>> 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>
>> ---
>>  Documentation/DocBook/media/v4l/compat.xml         |    4 +
>>  Documentation/DocBook/media/v4l/io.xml             |  177
>> ++++++++++++++++++++ .../DocBook/media/v4l/vidioc-create-bufs.xml       | 
>>   1 +
>>  Documentation/DocBook/media/v4l/vidioc-qbuf.xml    |   15 ++
>>  Documentation/DocBook/media/v4l/vidioc-reqbufs.xml |    8 +-
>>  5 files changed, 203 insertions(+), 2 deletions(-)
>>

[snip]

>> +    <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;
>> +
>> +if (ioctl (fd, &VIDIOC-REQBUFS;, &amp;reqbuf) == -1) {
>> +	if (errno == EINVAL)
>> +		printf ("Video capturing or user pointer streaming is not 
> supported\n");
> 
> User pointer??
> 

Right. I will fix this copy & paste error.

>> +	else
>> +		perror ("VIDIOC_REQBUFS");
>> +
>> +	exit (EXIT_FAILURE);
>> +}
>> +      </programlisting>
>> +    </example>
>> +

[snip]

>> +    <para>To start and stop capturing or output applications call the
>> +&VIDIOC-STREAMON; and &VIDIOC-STREAMOFF; ioctl. Note
>> +<constant>VIDIOC_STREAMOFF</constant> removes all buffers from both queues
>> and +unlocks/unpins 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 user pointer I/O must support the
> 
> User pointer again??
> 
> 

Yes. Another C&P mistake. Thanks for noticing it.

Regards,
Tomasz Stanislawski

Re: [PATCH 02/11] Documentation: media: description of DMABUF importing in V4L2

[Laurent Pinchart]

Hi Tomasz,

Thanks for the patch.

On Thursday 05 April 2012 15:59:59 Tomasz Stanislawski wrote:
> This patch adds description and usage examples for importing
> DMABUF file descriptor in V4L2.

[snip]

> diff --git a/Documentation/DocBook/media/v4l/io.xml
> b/Documentation/DocBook/media/v4l/io.xml index b815929..2a32363 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -472,6 +472,160 @@ rest should be evident.</para>
>        </footnote></para>
>    </section>
> 
> +  <section id="dmabuf">
> +    <title>Streaming I/O (DMA buffer importing)</title>

This section is very similar to the Streaming I/O (User Pointers) section. Do 
you think we should merge the two ?

> +    <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 was introduced to provide a generic mean for
> sharing buffers between multiple HW devices. Some device drivers provide
> an API that may be capable to export a DMA buffer as a DMABUF file
> descriptor. The other device drivers may be configured to use such a
> buffer. They implements an API for importing of DMABUF. This section
> describes the support of DMABUF importing in V4L2.</para>

I would rephrase this as follows:

<para>The DMABUF framework provides a generic mean for sharing buffers between
 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 this 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. If the particular importing buffer
> via DMABUF file descriptors method is supported must be determined by
> calling the &VIDIOC-REQBUFS; ioctl.</para>

What about

<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 the application itself
> using a client API. The buffers must be exported as DMABUF file descriptor.

I would say that "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 &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>

[snip]

> +    <para>Filled or displayed buffers are dequeued with the
> +&VIDIOC-DQBUF; ioctl. The driver can unpin the buffer at any
> +time between the completion of the DMA and this ioctl. The memory is
> +also unpinned 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>
> +
> +    <para>To start and stop capturing or output applications call the
> +&VIDIOC-STREAMON; and &VIDIOC-STREAMOFF; ioctl.

s/ioctl/ioctls.

> + Note <constant>VIDIOC_STREAMOFF</constant> removes all buffers from both

s/Note/Note that/

> + queues and unlocks/unpins 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 user pointer 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> ioctl, the
> <function>select()</function> and <function>poll()</function>
> function.</para>
> +
> +  </section>
> +
>    <section id="async">
>      <title>Asynchronous I/O</title>
> 
> @@ -671,6 +825,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-planar API and when

s/single-planar/single-plane/

> +<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>

[snip]

> diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml index 9caa49a..d4f212f
> 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> @@ -112,6 +112,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>, the <structfield>m.fd</structfield>

s/,/ and/

> +field to the file descriptor to a DMABUF buffer.

"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 &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 may pin and lock the buffer. Buffers remain locked/pinned until
> dequeued, until the &VIDIOC-STREAMOFF; or &VIDIOC-REQBUFS; ioctl is called,
> or until the device is closed.</para>

I'm not sure if we should really mention memory pinning, especially if the 
ioctl "may" pin and lock the buffer instead of "must" or "will". BTW, what 
difference do you make between "pin" and "lock" in this context ?

>      <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 7be4b1d..d758622
> 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml
> @@ -50,11 +50,14 @@
> 
>      <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
> +I/O or <link linkend="dmabuf">DMABUF file descriptors</link>.

<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.</para>
> +driver into user pointer I/O mode and to setup some internal structures.

> +A DMABUF file descriptor is a generic carrier for HW allocated buffers
> +dedicated to share them between multimedia APIs.</para>

What about

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.
> @@ -103,6 +106,7 @@ as the &v4l2-format; <structfield>type</structfield>
> field. See <xref <entry><structfield>memory</structfield></entry>
>  	    <entry>Applications set this field to
>  <constant>V4L2_MEMORY_MMAP</constant> or

s/ or/,/

> +<constant>V4L2_MEMORY_DMABUF</constant> or
>  <constant>V4L2_MEMORY_USERPTR</constant>.</entry>
>  	  </row>
>  	  <row>
-- 
Regards,

Laurent Pinchart

Re: [PATCH 03/11] v4l: vb2: add support for shared buffer (dma_buf)

[Laurent Pinchart]

Hi Tomasz,

On Thursday 05 April 2012 16:00:00 Tomasz Stanislawski wrote:
> From: Sumit Semwal <sumit.semwal@ti.com>
> 
> This patch adds support for DMABUF memory type in videobuf2. It calls
> relevant APIs of dma_buf for v4l reqbuf / qbuf / dqbuf operations.
> 
> For this version, the support is for videobuf2 as a user of the shared
> buffer; so the allocation of the buffer is done outside of V4L2. [A sample
> allocator of dma-buf shared buffer is given at [1]]
> 
> [1]: Rob Clark's DRM:
>    https://github.com/robclark/kernel-omap4/commits/drmplane-dmabuf
> 
> 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>
> ---
>  drivers/media/video/videobuf2-core.c |  184 ++++++++++++++++++++++++++++++-
>  include/media/videobuf2-core.h       |   31 ++++++
>  2 files changed, 214 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c index 2e8f1df..b37feea 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c

[snip]

> @@ -451,6 +482,21 @@ static int __verify_mmap_ops(struct vb2_queue *q)
>  }
> 
>  /**
> + * __verify_dmabuf_ops() - verify that all memory operations required for
> + * DMABUF queue type have been provided
> + */
> +static int __verify_dmabuf_ops(struct vb2_queue *q)
> +{
> +	if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf
> +			|| !q->mem_ops->detach_dmabuf
> +			|| !q->mem_ops->map_dmabuf
> +			|| !q->mem_ops->unmap_dmabuf)

That's pretty strange indentation. What about

        if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf ||
            !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf ||
            !q->mem_ops->unmap_dmabuf)

> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/**
>   * vb2_reqbufs() - Initiate streaming
>   * @q:		videobuf2 queue
>   * @req:	struct passed from userspace to vidioc_reqbufs handler in driver
> @@ -484,6 +530,7 @@ int vb2_reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req) }
> 
>  	if (req->memory != V4L2_MEMORY_MMAP
> +			&& req->memory != V4L2_MEMORY_DMABUF
>  			&& req->memory != V4L2_MEMORY_USERPTR) {
>  		dprintk(1, "reqbufs: unsupported memory type\n");
>  		return -EINVAL;

Ditto.

[snip]

> @@ -620,7 +672,8 @@ int vb2_create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create) }
> 
>  	if (create->memory != V4L2_MEMORY_MMAP
> -			&& create->memory != V4L2_MEMORY_USERPTR) {
> +			&& create->memory != V4L2_MEMORY_USERPTR
> +			&& create->memory != V4L2_MEMORY_DMABUF) {
>  		dprintk(1, "%s(): unsupported memory type\n", __func__);
>  		return -EINVAL;
>  	}

And here too.

[snip]

> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index a15d1f1..665e846 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h

[snip]

> @@ -65,6 +82,17 @@ struct vb2_mem_ops {
>  					unsigned long size, int write);
>  	void		(*put_userptr)(void *buf_priv);
> 
> +	/*
> +	 * Comment from Rob Clark: XXX: I think the attach / detach could be
> +	 * handled in the vb2 core, and vb2_mem_ops really just need to get/put
> +	 * the sglist (and make sure that the sglist fits it's needs..)
> +	 */

I think we should address this now. We've previously discussed the question, 
but haven't reached an agreement.

Quoting my reply to "[RFCv2 PATCH 3/9] v4l: vb2: Add dma-contig allocator as 
dma_buf user" on 28/03/2012:

> > Calling dma_buf_attach could be moved to vb2-core. But it gives little
> > gain. First, the pointer of dma_buf_attachment would have to be added to
> > struct vb2_plane. Second, the allocator would have to keep in the copy of
> > this pointer in its context structure for use of vb2_dc_(un)map_dmabuf
> > functions.
> 
> Right. Would it make sense to pass the vb2_plane pointer, or possibly the 
> dma_buf_attachment pointer, to the mmap_dmabuf and unmap_dmabuf operations ?
> 
> > Third, dma_buf_attach requires a pointer to 'struct device' which is not
> > available in the vb2-core layer.
> 
> OK, that's a problem.
> 
> > Because of the mentioned reasons I decided to keep attach_dmabuf in
> > allocator-specific code.
> 
> Maybe it would make sense to create a vb2_mem_buf structure from which 
> vb2_dc_buf (and other allocator-specific buffer descriptors) would inherit ? 
> That structure would store the dma_buf_attach pointer, and common dma-buf
> code could be put in videobuf2-memops.c and shared between allocators. Just
> an idea.

If we find out that the best course of action is to leave the code as-is, we 
should remove the above comment.

> +	void		*(*attach_dmabuf)(void *alloc_ctx, struct dma_buf *dbuf,
> +				unsigned long size, int write);
> +	void		(*detach_dmabuf)(void *buf_priv);
> +	int		(*map_dmabuf)(void *buf_priv);
> +	void		(*unmap_dmabuf)(void *buf_priv);
> +
>  	void		*(*vaddr)(void *buf_priv);
>  	void		*(*cookie)(void *buf_priv);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 09/11] v4l: vb2: add prepare/finish callbacks to allocators

[Laurent Pinchart]

Hi Tomasz,

On Thursday 05 April 2012 16:00:06 Tomasz Stanislawski wrote:
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> This patch adds support for prepare/finish callbacks in VB2 allocators.
> These callback are used for buffer flushing.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/media/video/videobuf2-core.c |   11 +++++++++++
>  include/media/videobuf2-core.h       |    7 +++++++
>  2 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c index b37feea..abb0592 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -834,6 +834,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
> vb2_buffer_state state) {
>  	struct vb2_queue *q = vb->vb2_queue;
>  	unsigned long flags;
> +	int plane;

Please make plane an unsigned int, otherwise you will compare a signed and an 
unsigned int below.

> 
>  	if (vb->state != VB2_BUF_STATE_ACTIVE)
>  		return;
> @@ -844,6 +845,10 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
> vb2_buffer_state state) dprintk(4, "Done processing on buffer %d, state:
> %d\n",
>  			vb->v4l2_buf.index, vb->state);
> 
> +	/* sync buffers */
> +	for (plane = 0; plane < vb->num_planes; ++plane)
> +		call_memop(q, finish, vb->planes[plane].mem_priv);
> +
>  	/* Add the buffer to the done buffers list */
>  	spin_lock_irqsave(&q->done_lock, flags);
>  	vb->state = state;
> @@ -1131,9 +1136,15 @@ err:
>  static void __enqueue_in_driver(struct vb2_buffer *vb)
>  {
>  	struct vb2_queue *q = vb->vb2_queue;
> +	int plane;

Same here.

>  	vb->state = VB2_BUF_STATE_ACTIVE;
>  	atomic_inc(&q->queued_count);
> +
> +	/* sync buffers */
> +	for (plane = 0; plane < vb->num_planes; ++plane)
> +		call_memop(q, prepare, vb->planes[plane].mem_priv);
> +
>  	q->ops->buf_queue(vb);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/11] v4l: vb2-dma-contig: add support for scatterlist in userptr mode

[Laurent Pinchart]

Hi Tomasz,

On Thursday 05 April 2012 16:00:05 Tomasz Stanislawski wrote:
> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> 
> This patch introduces usage of dma_map_sg to map memory behind
> a userspace pointer to a device as dma-contiguous mapping.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 	[bugfixing]
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> 	[bugfixing]
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> 	[add sglist subroutines/code refactoring]
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/videobuf2-dma-contig.c |  282 +++++++++++++++++++++++--
>  1 files changed, 265 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-dma-contig.c
> b/drivers/media/video/videobuf2-dma-contig.c index 476e536..6ab3165 100644
> --- a/drivers/media/video/videobuf2-dma-contig.c
> +++ b/drivers/media/video/videobuf2-dma-contig.c

[snip]

> @@ -32,6 +36,98 @@ struct vb2_dc_buf {
>  };
> 
>  /*********************************************/
> +/*        scatterlist table functions        */
> +/*********************************************/
> +
> +static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages,
> +	unsigned long n_pages, size_t offset, size_t offset2)

"offset2" isn't very descriptive. I would replace it with the total size of 
the buffer instead (or, alternatively, rename offset to offset_start and 
offset2 to offset_end, but I like the first option better).

> +{
> +	struct sg_table *sgt;
> +	int i, j; /* loop counters */

I don't think the comment is needed.

> +	int cur_page, chunks;

i, j, cur_page and chunks can't be negative. Could you please make them 
unsigned int (and I would order them) ?

Also, Documentation/CodingStyle favors one variable declaration per line, 
without commas for multiple declarations.

> +	int ret;
> +	struct scatterlist *s;
> +
> +	sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
> +	if (!sgt)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* compute number of chunks */
> +	chunks = 1;
> +	for (i = 1; i < n_pages; ++i)
> +		if (pages[i] != pages[i - 1] + 1)
> +			++chunks;
> +
> +	ret = sg_alloc_table(sgt, chunks, GFP_KERNEL);
> +	if (ret) {
> +		kfree(sgt);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* merging chunks and putting them into the scatterlist */
> +	cur_page = 0;
> +	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
> +		size_t size = PAGE_SIZE;
> +
> +		for (j = cur_page + 1; j < n_pages; ++j) {
> +			if (pages[j] != pages[j - 1] + 1)
> +				break;
> +			size += PAGE_SIZE;
> +		}
> +
> +		/* cut offset if chunk starts at the first page */
> +		if (cur_page == 0)
> +			size -= offset;
> +		/* cut offset2 if chunk ends at the last page */
> +		if (j == n_pages)
> +			size -= offset2;
> +
> +		sg_set_page(s, pages[cur_page], size, offset);
> +		offset = 0;
> +		cur_page = j;
> +	}
> +
> +	return sgt;
> +}
> +
> +static void vb2_dc_release_sgtable(struct sg_table *sgt)
> +{
> +	sg_free_table(sgt);
> +	kfree(sgt);
> +}
> +
> +static void vb2_dc_sgt_foreach_page(struct sg_table *sgt,
> +	void (*cb)(struct page *pg))
> +{
> +	struct scatterlist *s;
> +	int i, j;
> +
> +	for_each_sg(sgt->sgl, s, sgt->nents, i) {
> +		struct page *page = sg_page(s);
> +		int n_pages = PAGE_ALIGN(s->offset + s->length) >> PAGE_SHIFT;
> +
> +		for (j = 0; j < n_pages; ++j, ++page)
> +			cb(page);

Same for i, j and n_pages here.

> +	}
> +}
> +
> +static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
> +{
> +	struct scatterlist *s;
> +	dma_addr_t expected = sg_dma_address(sgt->sgl);
> +	int i;

Same for i here.

> +	unsigned long size = 0;
> +
> +	for_each_sg(sgt->sgl, s, sgt->nents, i) {
> +		if (sg_dma_address(s) != expected)
> +			break;
> +		expected = sg_dma_address(s) + sg_dma_len(s);
> +		size += sg_dma_len(s);
> +	}
> +	return size;
> +}
> +
> +/*********************************************/
>  /*         callbacks for all buffers         */
>  /*********************************************/
> 
> @@ -116,42 +212,194 @@ static int vb2_dc_mmap(void *buf_priv, struct
> vm_area_struct *vma) /*       callbacks for USERPTR buffers       */
>  /*********************************************/
> 
> +static inline int vma_is_io(struct vm_area_struct *vma)
> +{
> +	return !!(vma->vm_flags & (VM_IO | VM_PFNMAP));
> +}
> +
> +static struct vm_area_struct *vb2_dc_get_user_vma(
> +	unsigned long start, unsigned long size)
> +{
> +	struct vm_area_struct *vma;
> +
> +	/* current->mm->mmap_sem is taken by videobuf2 core */
> +	vma = find_vma(current->mm, start);
> +	if (!vma) {
> +		printk(KERN_ERR "no vma for address %lu\n", start);
> +		return ERR_PTR(-EFAULT);
> +	}
> +
> +	if (vma->vm_end - vma->vm_start < size) {
> +		printk(KERN_ERR "vma at %lu is too small for %lu bytes\n",
> +			start, size);
> +		return ERR_PTR(-EFAULT);
> +	}

Should we support multiple VMAs, or do you think that's not worth it ?

> +	vma = vb2_get_vma(vma);
> +	if (!vma) {
> +		printk(KERN_ERR "failed to copy vma\n");
> +		return ERR_PTR(-ENOMEM);
> +	}

I still think there's no need to copy the VMA. get_user_pages() will make sure 
the memory doesn't get paged out, and we don't need to ensure that the 
userspace mapping stays in place as our cache operations use a scatter list. 
Storing the result of vma_is_io() in vb2_dc_buf should be enough.

> +	return vma;
> +}
> +
> +static int vb2_dc_get_user_pages(unsigned long start, struct page **pages,
> +	int n_pages, struct vm_area_struct *vma, int write)
> +{
> +	int n;

n_pages and n can be unsigned (and I would rename n to i, to be coherent with 
the rest of the file).

> +
> +	if (vma_is_io(vma)) {
> +		for (n = 0; n < n_pages; ++n, start += PAGE_SIZE) {
> +			unsigned long pfn;
> +			int ret = follow_pfn(vma, start, &pfn);
> +
> +			if (ret) {
> +				printk(KERN_ERR "no page for address %lu\n",
> +					start);
> +				return ret;
> +			}
> +			pages[n] = pfn_to_page(pfn);
> +		}
> +	} else {
> +		n = get_user_pages(current, current->mm, start & PAGE_MASK,
> +			n_pages, write, 1, pages, NULL);
> +		if (n != n_pages) {
> +			printk(KERN_ERR "got only %d of %d user pages\n",
> +				n, n_pages);
> +			while (n)
> +				put_page(pages[--n]);
> +			return -EFAULT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void vb2_dc_set_page_dirty(struct page *page)
> +{
> +	set_page_dirty_lock(page);
> +}
> +
> +static void vb2_dc_put_userptr(void *buf_priv)
> +{
> +	struct vb2_dc_buf *buf = buf_priv;
> +	struct sg_table *sgt = buf->dma_sgt;
> +
> +	dma_unmap_sg(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
> +	if (!vma_is_io(buf->vma)) {
> +		vb2_dc_sgt_foreach_page(sgt, vb2_dc_set_page_dirty);
> +		vb2_dc_sgt_foreach_page(sgt, put_page);

This results in two iterations over the pages. Wouldn't it better to fold the 
vb2_dc_sgt_foreach_page() function into this one, and loop once only ? 
vb2_dc_sgt_foreach_page() is also called in the cleanup path of 
vb2_dc_get_userptr(), but you have the list of pages available in the 
function, so you can iterate over it directly.

> +	}
> +
> +	vb2_dc_release_sgtable(sgt);
> +	vb2_put_vma(buf->vma);
> +	kfree(buf);
> +}
> +
>  static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
> -					unsigned long size, int write)
> +	unsigned long size, int write)
>  {
>  	struct vb2_dc_buf *buf;
> -	struct vm_area_struct *vma;
> -	dma_addr_t dma_addr = 0;
> -	int ret;
> +	unsigned long start, end, offset, offset2;

If you don't use the buffer size above, please rename offset2 here too (and 
avoid multiple variable declarations per line).

> +	struct page **pages;
> +	int n_pages;
> +	int ret = 0;
> +	struct sg_table *sgt;
> +	unsigned long contig_size;
> 
>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
>  		return ERR_PTR(-ENOMEM);
> 
> -	ret = vb2_get_contig_userptr(vaddr, size, &vma, &dma_addr);
> +	buf->dev = alloc_ctx;
> +	buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +
> +	start = (unsigned long)vaddr & PAGE_MASK;
> +	offset = (unsigned long)vaddr & ~PAGE_MASK;
> +	end = PAGE_ALIGN((unsigned long)vaddr + size);
> +	offset2 = end - (unsigned long)vaddr - size;

vaddr is already an unsigned long, there's no need to cast it.

> +	n_pages = (end - start) >> PAGE_SHIFT;
> +
> +	pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
> +	if (!pages) {
> +		ret = -ENOMEM;
> +		printk(KERN_ERR "failed to allocate pages table\n");
> +		goto fail_buf;
> +	}
> +
> +	buf->vma = vb2_dc_get_user_vma(start, end - start);
> +	if (IS_ERR(buf->vma)) {
> +		printk(KERN_ERR "failed to get VMA\n");
> +		ret = PTR_ERR(buf->vma);
> +		goto fail_pages;
> +	}
> +
> +	/* extract page list from userspace mapping */
> +	ret = vb2_dc_get_user_pages(start, pages, n_pages, buf->vma, write);
>  	if (ret) {
> -		printk(KERN_ERR "Failed acquiring VMA for vaddr 0x%08lx\n",
> -				vaddr);
> -		kfree(buf);
> -		return ERR_PTR(ret);
> +		printk(KERN_ERR "failed to get user pages\n");
> +		goto fail_vma;
> +	}
> +
> +	sgt = vb2_dc_pages_to_sgt(pages, n_pages, offset, offset2);
> +	if (IS_ERR(sgt)) {
> +		printk(KERN_ERR "failed to create scatterlist table\n");
> +		ret = -ENOMEM;
> +		goto fail_get_user_pages;
> +	}
> +
> +	/* pages are no longer needed */
> +	kfree(pages);
> +	pages = NULL;
> +
> +	sgt->nents = dma_map_sg(buf->dev, sgt->sgl, sgt->orig_nents,
> +		buf->dma_dir);
> +	if (sgt->nents <= 0) {
> +		printk(KERN_ERR "failed to map scatterlist\n");
> +		ret = -EIO;
> +		goto fail_sgt;
> +	}
> +
> +	contig_size = vb2_dc_get_contiguous_size(sgt);
> +	if (contig_size < size) {
> +		printk(KERN_ERR "contiguous mapping is too small %lu/%lu\n",
> +			contig_size, size);
> +		ret = -EFAULT;
> +		goto fail_map_sg;
>  	}
> 
> +	buf->dma_addr = sg_dma_address(sgt->sgl);
>  	buf->size = size;
> -	buf->dma_addr = dma_addr;
> -	buf->vma = vma;
> +	buf->dma_sgt = sgt;
> +
> +	atomic_inc(&buf->refcount);

refcount is only used for MMAP buffers as far as I can tell, I don't think you 
need to increment refcount here.
> 
>  	return buf;
> -}
> 
> -static void vb2_dc_put_userptr(void *mem_priv)
> -{
> -	struct vb2_dc_buf *buf = mem_priv;
> +fail_map_sg:
> +	dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
> 
> -	if (!buf)
> -		return;
> +fail_sgt:
> +	if (!vma_is_io(buf->vma))
> +		vb2_dc_sgt_foreach_page(sgt, put_page);
> +	vb2_dc_release_sgtable(sgt);
> +
> +fail_get_user_pages:
> +	if (pages && !vma_is_io(buf->vma))
> +		while (n_pages)
> +			put_page(pages[--n_pages]);
> 
> +fail_vma:
>  	vb2_put_vma(buf->vma);
> +
> +fail_pages:
> +	kfree(pages); /* kfree is NULL-proof */
> +
> +fail_buf:
>  	kfree(buf);
> +
> +	return ERR_PTR(ret);
>  }
> 
>  /*********************************************/
-- 
Regards,

Laurent Pinchart

Re: [PATCH 11/11] v4l: vb2: Add dma-contig allocator as dma_buf user

[Laurent Pinchart]

Hi Tomasz,

On Thursday 05 April 2012 16:00:08 Tomasz Stanislawski wrote:
> From: Sumit Semwal <sumit.semwal@ti.com>
> 
> This patch makes changes for adding dma-contig as a dma_buf user. It
> provides function implementations for the {attach, detach, map,
> unmap}_dmabuf() mem_ops of DMABUF memory type.
> 
> Signed-off-by: Sumit Semwal <sumit.semwal@ti.com>
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> 	[author of the original patch]
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> 	[integration with refactored dma-contig allocator]
> ---
>  drivers/media/video/videobuf2-dma-contig.c |  117 +++++++++++++++++++++++++
>  1 files changed, 117 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-dma-contig.c
> b/drivers/media/video/videobuf2-dma-contig.c index 30f316588..6329483
> 100644
> --- a/drivers/media/video/videobuf2-dma-contig.c
> +++ b/drivers/media/video/videobuf2-dma-contig.c
> @@ -10,6 +10,7 @@
>   * the Free Software Foundation.
>   */
> 
> +#include <linux/dma-buf.h>
>  #include <linux/module.h>
>  #include <linux/scatterlist.h>
>  #include <linux/sched.h>
> @@ -33,6 +34,9 @@ struct vb2_dc_buf {
> 
>  	/* USERPTR related */
>  	struct vm_area_struct		*vma;
> +
> +	/* DMABUF related */
> +	struct dma_buf_attachment	*db_attach;
>  };
> 
>  /*********************************************/
> @@ -425,6 +429,115 @@ fail_buf:
>  }
> 
>  /*********************************************/
> +/*       callbacks for DMABUF buffers        */
> +/*********************************************/
> +
> +static int vb2_dc_map_dmabuf(void *mem_priv)
> +{
> +	struct vb2_dc_buf *buf = mem_priv;
> +	struct sg_table *sgt;
> +	unsigned long contig_size;
> +	int ret = 0;
> +
> +	if (WARN_ON(!buf->db_attach)) {
> +		printk(KERN_ERR "trying to pin a non attached buffer\n");
> +		return -EINVAL;
> +	}
> +
> +	if (WARN_ON(buf->dma_sgt)) {
> +		printk(KERN_ERR "dmabuf buffer is already pinned\n");
> +		return 0;
> +	}
> +
> +	/* get the associated scatterlist for this buffer */
> +	sgt = dma_buf_map_attachment(buf->db_attach, buf->dma_dir);
> +	if (IS_ERR_OR_NULL(sgt)) {
> +		printk(KERN_ERR "Error getting dmabuf scatterlist\n");
> +		return -EINVAL;
> +	}
> +
> +	/* checking if dmabuf is big enough to store contiguous chunk */
> +	contig_size = vb2_dc_get_contiguous_size(sgt);
> +	if (contig_size < buf->size) {
> +		printk(KERN_ERR "contiguous chunk of dmabuf is too small\n");
> +		ret = -EFAULT;
> +		goto fail_map;

The fail_map label is only used here, you can move the 
dma_buf_unmap_attachment() call here and return -EFAULT directly.

> +	}
> +
> +	buf->dma_addr = sg_dma_address(sgt->sgl);
> +	buf->dma_sgt = sgt;
> +
> +	return 0;
> +
> +fail_map:
> +	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
> +
> +	return ret;
> +}
> +
> +static void vb2_dc_unmap_dmabuf(void *mem_priv)
> +{
> +	struct vb2_dc_buf *buf = mem_priv;
> +	struct sg_table *sgt = buf->dma_sgt;
> +
> +	if (WARN_ON(!buf->db_attach)) {
> +		printk(KERN_ERR "trying to unpin a not attached buffer\n");
> +		return;
> +	}
> +
> +	if (WARN_ON(!sgt)) {
> +		printk(KERN_ERR "dmabuf buffer is already unpinned\n");
> +		return;
> +	}
> +
> +	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
> +
> +	buf->dma_addr = 0;
> +	buf->dma_sgt = NULL;
> +}
> +
> +static void vb2_dc_detach_dmabuf(void *mem_priv)
> +{
> +	struct vb2_dc_buf *buf = mem_priv;
> +
> +	if (buf->dma_addr)
> +		vb2_dc_unmap_dmabuf(buf);

What would you think about calling vb2_dc_unmap_dmabuf() from vb2 core 
instead, to keep the map/unmap calls symmetrical (the second WARN_ON and the 
related printk in vb2_dc_unmap_dmabuf() might need to go then) ?

> +	/* detach this attachment */
> +	dma_buf_detach(buf->db_attach->dmabuf, buf->db_attach);
> +	kfree(buf);
> +}
> +
> +static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf,
> +	unsigned long size, int write)
> +{
> +	struct vb2_dc_buf *buf;
> +	struct dma_buf_attachment *dba;
> +
> +	if (dbuf->size < size)
> +		return ERR_PTR(-EFAULT);
> +
> +	buf = kzalloc(sizeof *buf, GFP_KERNEL);
> +	if (!buf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	buf->dev = alloc_ctx;
> +	/* create attachment for the dmabuf with the user device */
> +	dba = dma_buf_attach(dbuf, buf->dev);
> +	if (IS_ERR(dba)) {
> +		printk(KERN_ERR "failed to attach dmabuf\n");
> +		kfree(buf);
> +		return dba;
> +	}
> +
> +	buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +	buf->size = size;
> +	buf->db_attach = dba;
> +
> +	return buf;
> +}
> +
> +/*********************************************/
>  /*       DMA CONTIG exported functions       */
>  /*********************************************/
> 
> @@ -438,6 +551,10 @@ const struct vb2_mem_ops vb2_dma_contig_memops = {
>  	.put_userptr	= vb2_dc_put_userptr,
>  	.prepare	= vb2_dc_prepare,
>  	.finish		= vb2_dc_finish,
> +	.map_dmabuf	= vb2_dc_map_dmabuf,
> +	.unmap_dmabuf	= vb2_dc_unmap_dmabuf,
> +	.attach_dmabuf	= vb2_dc_attach_dmabuf,
> +	.detach_dmabuf	= vb2_dc_detach_dmabuf,
>  	.num_users	= vb2_dc_num_users,
>  };
>  EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);
-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/11] v4l: vb2-dma-contig: add support for scatterlist in userptr mode

[Tomasz Stanislawski]

Hi Laurent,
Thank you for review. Your comments are very helpful :).
Take a look on the comments below.

On 04/06/2012 05:02 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> On Thursday 05 April 2012 16:00:05 Tomasz Stanislawski wrote:
>> From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>>
>> This patch introduces usage of dma_map_sg to map memory behind
>> a userspace pointer to a device as dma-contiguous mapping.
>>
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> 	[bugfixing]
>> Signed-off-by: Kamil Debski <k.debski@samsung.com>
>> 	[bugfixing]
>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> 	[add sglist subroutines/code refactoring]
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/media/video/videobuf2-dma-contig.c |  282 +++++++++++++++++++++++--
>>  1 files changed, 265 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/media/video/videobuf2-dma-contig.c
>> b/drivers/media/video/videobuf2-dma-contig.c index 476e536..6ab3165 100644
>> --- a/drivers/media/video/videobuf2-dma-contig.c
>> +++ b/drivers/media/video/videobuf2-dma-contig.c
> 
> [snip]
> 
>> @@ -32,6 +36,98 @@ struct vb2_dc_buf {
>>  };
>>
>>  /*********************************************/
>> +/*        scatterlist table functions        */
>> +/*********************************************/
>> +
>> +static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages,
>> +	unsigned long n_pages, size_t offset, size_t offset2)
> 
> "offset2" isn't very descriptive. I would replace it with the total size of 
> the buffer instead (or, alternatively, rename offset to offset_start and 
> offset2 to offset_end, but I like the first option better).
> 
>> +{
>> +	struct sg_table *sgt;
>> +	int i, j; /* loop counters */
> 
> I don't think the comment is needed.
> 

ok

>> +	int cur_page, chunks;
> 
> i, j, cur_page and chunks can't be negative. Could you please make them 
> unsigned int (and I would order them) ?

ok

> 
> Also, Documentation/CodingStyle favors one variable declaration per line, 
> without commas for multiple declarations.

does it include loop counters like i and j?

> 
>> +	int ret;
>> +	struct scatterlist *s;
>> +
>> +	sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
>> +	if (!sgt)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	/* compute number of chunks */
>> +	chunks = 1;
>> +	for (i = 1; i < n_pages; ++i)
>> +		if (pages[i] != pages[i - 1] + 1)
>> +			++chunks;
>> +
>> +	ret = sg_alloc_table(sgt, chunks, GFP_KERNEL);
>> +	if (ret) {
>> +		kfree(sgt);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	/* merging chunks and putting them into the scatterlist */
>> +	cur_page = 0;
>> +	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
>> +		size_t size = PAGE_SIZE;
>> +
>> +		for (j = cur_page + 1; j < n_pages; ++j) {
>> +			if (pages[j] != pages[j - 1] + 1)
>> +				break;
>> +			size += PAGE_SIZE;
>> +		}
>> +
>> +		/* cut offset if chunk starts at the first page */
>> +		if (cur_page == 0)
>> +			size -= offset;
>> +		/* cut offset2 if chunk ends at the last page */
>> +		if (j == n_pages)
>> +			size -= offset2;
>> +
>> +		sg_set_page(s, pages[cur_page], size, offset);
>> +		offset = 0;
>> +		cur_page = j;
>> +	}
>> +
>> +	return sgt;
>> +}
>> +
>> +static void vb2_dc_release_sgtable(struct sg_table *sgt)
>> +{
>> +	sg_free_table(sgt);
>> +	kfree(sgt);
>> +}
>> +
>> +static void vb2_dc_sgt_foreach_page(struct sg_table *sgt,
>> +	void (*cb)(struct page *pg))
>> +{
>> +	struct scatterlist *s;
>> +	int i, j;
>> +
>> +	for_each_sg(sgt->sgl, s, sgt->nents, i) {
>> +		struct page *page = sg_page(s);
>> +		int n_pages = PAGE_ALIGN(s->offset + s->length) >> PAGE_SHIFT;
>> +
>> +		for (j = 0; j < n_pages; ++j, ++page)
>> +			cb(page);
> 
> Same for i, j and n_pages here.
> 

ok

>> +	}
>> +}
>> +
>> +static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
>> +{
>> +	struct scatterlist *s;
>> +	dma_addr_t expected = sg_dma_address(sgt->sgl);
>> +	int i;
> 
> Same for i here.
> 
>> +	unsigned long size = 0;
>> +
>> +	for_each_sg(sgt->sgl, s, sgt->nents, i) {
>> +		if (sg_dma_address(s) != expected)
>> +			break;
>> +		expected = sg_dma_address(s) + sg_dma_len(s);
>> +		size += sg_dma_len(s);
>> +	}
>> +	return size;
>> +}
>> +
>> +/*********************************************/
>>  /*         callbacks for all buffers         */
>>  /*********************************************/
>>
>> @@ -116,42 +212,194 @@ static int vb2_dc_mmap(void *buf_priv, struct
>> vm_area_struct *vma) /*       callbacks for USERPTR buffers       */
>>  /*********************************************/
>>
>> +static inline int vma_is_io(struct vm_area_struct *vma)
>> +{
>> +	return !!(vma->vm_flags & (VM_IO | VM_PFNMAP));
>> +}
>> +
>> +static struct vm_area_struct *vb2_dc_get_user_vma(
>> +	unsigned long start, unsigned long size)
>> +{
>> +	struct vm_area_struct *vma;
>> +
>> +	/* current->mm->mmap_sem is taken by videobuf2 core */
>> +	vma = find_vma(current->mm, start);
>> +	if (!vma) {
>> +		printk(KERN_ERR "no vma for address %lu\n", start);
>> +		return ERR_PTR(-EFAULT);
>> +	}
>> +
>> +	if (vma->vm_end - vma->vm_start < size) {
>> +		printk(KERN_ERR "vma at %lu is too small for %lu bytes\n",
>> +			start, size);
>> +		return ERR_PTR(-EFAULT);
>> +	}
> 
> Should we support multiple VMAs, or do you think that's not worth it ?
> 

What do you mean by multiple VMAs?

>> +	vma = vb2_get_vma(vma);
>> +	if (!vma) {
>> +		printk(KERN_ERR "failed to copy vma\n");
>> +		return ERR_PTR(-ENOMEM);
>> +	}
> 
> I still think there's no need to copy the VMA. get_user_pages() will make sure 
> the memory doesn't get paged out, and we don't need to ensure that the 
> userspace mapping stays in place as our cache operations use a scatter list. 
> Storing the result of vma_is_io() in vb2_dc_buf should be enough.

As I understand calling get_user_pages ensures that pages are not going to be
swapped or freed. I agree that it provides enough protection for the memory.

IO mappings are the problem. As you mentioned few mails ago get_page would
likely crash for such pages. AFAIK increasing reference count for VMA could be
a reliable mechanism for protecting the memory from being freed.
The problem is that VMA has no reference counters in it. Calling open ops
will protect the memory. However it will not protect VMA structure from
being freed!

Analyze following scenario:

- mmap -> returns userptr
- qbuf (userptr)
- unmap (userptr)
- dqbuf

The VMA will be destroyed at unmap but memory will not be released.
The reason is that open ops was called at qbuf.
In order to free the memory the VB2 has to call close ops. This
callback takes pointer to VMA as an argument. The original VMA
cannot be used here because it is not longer valid.

Therefore VMA has to be copied at qbuf operation. The function vb2_get_vma
is used for this purpose.

The workaround could be dropping support for IO mappings in VB2.
However it will handicap all s5p media drivers because mapping
produced by dma_mmap_coherent (aka writecombine) are IO mapping.
As result s5p-fimc could no longer create a pipeline with s5p-mfc.

Introducing DMABUF to V4L is a good alternative but only if exporting
is supported.

For now I think it is better to keep support for IO mappings. At least
until DMA mapping redesign and DMABUF exporting in V4L is merged.

> 
>> +	return vma;
>> +}
>> +
>> +static int vb2_dc_get_user_pages(unsigned long start, struct page **pages,
>> +	int n_pages, struct vm_area_struct *vma, int write)
>> +{
>> +	int n;
> 
> n_pages and n can be unsigned (and I would rename n to i, to be coherent with 
> the rest of the file).
> 

ok

>> +
>> +	if (vma_is_io(vma)) {
>> +		for (n = 0; n < n_pages; ++n, start += PAGE_SIZE) {
>> +			unsigned long pfn;
>> +			int ret = follow_pfn(vma, start, &pfn);
>> +
>> +			if (ret) {
>> +				printk(KERN_ERR "no page for address %lu\n",
>> +					start);
>> +				return ret;
>> +			}
>> +			pages[n] = pfn_to_page(pfn);
>> +		}
>> +	} else {
>> +		n = get_user_pages(current, current->mm, start & PAGE_MASK,
>> +			n_pages, write, 1, pages, NULL);
>> +		if (n != n_pages) {
>> +			printk(KERN_ERR "got only %d of %d user pages\n",
>> +				n, n_pages);
>> +			while (n)
>> +				put_page(pages[--n]);
>> +			return -EFAULT;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void vb2_dc_set_page_dirty(struct page *page)
>> +{
>> +	set_page_dirty_lock(page);
>> +}
>> +
>> +static void vb2_dc_put_userptr(void *buf_priv)
>> +{
>> +	struct vb2_dc_buf *buf = buf_priv;
>> +	struct sg_table *sgt = buf->dma_sgt;
>> +
>> +	dma_unmap_sg(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
>> +	if (!vma_is_io(buf->vma)) {
>> +		vb2_dc_sgt_foreach_page(sgt, vb2_dc_set_page_dirty);
>> +		vb2_dc_sgt_foreach_page(sgt, put_page);
> 
> This results in two iterations over the pages. Wouldn't it better to fold the 
> vb2_dc_sgt_foreach_page() function into this one, and loop once only ? 
> vb2_dc_sgt_foreach_page() is also called in the cleanup path of 
> vb2_dc_get_userptr(), but you have the list of pages available in the 
> function, so you can iterate over it directly.
> 

what do you think about using function vb2_dc_put_dirty_page which
would call both set_page_dirty_lock and put_page ?

>> +	}
>> +
>> +	vb2_dc_release_sgtable(sgt);
>> +	vb2_put_vma(buf->vma);
>> +	kfree(buf);
>> +}
>> +
>>  static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
>> -					unsigned long size, int write)
>> +	unsigned long size, int write)
>>  {
>>  	struct vb2_dc_buf *buf;
>> -	struct vm_area_struct *vma;
>> -	dma_addr_t dma_addr = 0;
>> -	int ret;
>> +	unsigned long start, end, offset, offset2;
> 
> If you don't use the buffer size above, please rename offset2 here too (and 
> avoid multiple variable declarations per line).
> 

ok

>> +	struct page **pages;
>> +	int n_pages;
>> +	int ret = 0;
>> +	struct sg_table *sgt;
>> +	unsigned long contig_size;
>>
>>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>>  	if (!buf)
>>  		return ERR_PTR(-ENOMEM);
>>
>> -	ret = vb2_get_contig_userptr(vaddr, size, &vma, &dma_addr);
>> +	buf->dev = alloc_ctx;
>> +	buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
>> +
>> +	start = (unsigned long)vaddr & PAGE_MASK;
>> +	offset = (unsigned long)vaddr & ~PAGE_MASK;
>> +	end = PAGE_ALIGN((unsigned long)vaddr + size);
>> +	offset2 = end - (unsigned long)vaddr - size;
> 
> vaddr is already an unsigned long, there's no need to cast it.
> 

roger that

>> +	n_pages = (end - start) >> PAGE_SHIFT;
>> +
>> +	pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
>> +	if (!pages) {
>> +		ret = -ENOMEM;
>> +		printk(KERN_ERR "failed to allocate pages table\n");
>> +		goto fail_buf;
>> +	}
>> +
>> +	buf->vma = vb2_dc_get_user_vma(start, end - start);
>> +	if (IS_ERR(buf->vma)) {
>> +		printk(KERN_ERR "failed to get VMA\n");
>> +		ret = PTR_ERR(buf->vma);
>> +		goto fail_pages;
>> +	}
>> +
>> +	/* extract page list from userspace mapping */
>> +	ret = vb2_dc_get_user_pages(start, pages, n_pages, buf->vma, write);
>>  	if (ret) {
>> -		printk(KERN_ERR "Failed acquiring VMA for vaddr 0x%08lx\n",
>> -				vaddr);
>> -		kfree(buf);
>> -		return ERR_PTR(ret);
>> +		printk(KERN_ERR "failed to get user pages\n");
>> +		goto fail_vma;
>> +	}
>> +
>> +	sgt = vb2_dc_pages_to_sgt(pages, n_pages, offset, offset2);
>> +	if (IS_ERR(sgt)) {
>> +		printk(KERN_ERR "failed to create scatterlist table\n");
>> +		ret = -ENOMEM;
>> +		goto fail_get_user_pages;
>> +	}
>> +
>> +	/* pages are no longer needed */
>> +	kfree(pages);
>> +	pages = NULL;
>> +
>> +	sgt->nents = dma_map_sg(buf->dev, sgt->sgl, sgt->orig_nents,
>> +		buf->dma_dir);
>> +	if (sgt->nents <= 0) {
>> +		printk(KERN_ERR "failed to map scatterlist\n");
>> +		ret = -EIO;
>> +		goto fail_sgt;
>> +	}
>> +
>> +	contig_size = vb2_dc_get_contiguous_size(sgt);
>> +	if (contig_size < size) {
>> +		printk(KERN_ERR "contiguous mapping is too small %lu/%lu\n",
>> +			contig_size, size);
>> +		ret = -EFAULT;
>> +		goto fail_map_sg;
>>  	}
>>
>> +	buf->dma_addr = sg_dma_address(sgt->sgl);
>>  	buf->size = size;
>> -	buf->dma_addr = dma_addr;
>> -	buf->vma = vma;
>> +	buf->dma_sgt = sgt;
>> +
>> +	atomic_inc(&buf->refcount);
> 
> refcount is only used for MMAP buffers as far as I can tell, I don't think you 
> need to increment refcount here.

right ... thanks for spotting it.
Fortunately, that put_userptr does not check this counter. Otherwise it would
be a memleak :).

>>
>>  	return buf;
>> -}
>>
>> -static void vb2_dc_put_userptr(void *mem_priv)
>> -{
>> -	struct vb2_dc_buf *buf = mem_priv;
>> +fail_map_sg:
>> +	dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
>>
>> -	if (!buf)
>> -		return;
>> +fail_sgt:
>> +	if (!vma_is_io(buf->vma))
>> +		vb2_dc_sgt_foreach_page(sgt, put_page);
>> +	vb2_dc_release_sgtable(sgt);
>> +
>> +fail_get_user_pages:
>> +	if (pages && !vma_is_io(buf->vma))
>> +		while (n_pages)
>> +			put_page(pages[--n_pages]);
>>
>> +fail_vma:
>>  	vb2_put_vma(buf->vma);
>> +
>> +fail_pages:
>> +	kfree(pages); /* kfree is NULL-proof */
>> +
>> +fail_buf:
>>  	kfree(buf);
>> +
>> +	return ERR_PTR(ret);
>>  }
>>
>>  /*********************************************/

Regards,
Tomasz Stanislawski

Re: [PATCH 03/11] v4l: vb2: add support for shared buffer (dma_buf)

[Tomasz Stanislawski]

On 04/06/2012 03:29 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> On Thursday 05 April 2012 16:00:00 Tomasz Stanislawski wrote:
>> From: Sumit Semwal <sumit.semwal@ti.com>
>>
>> This patch adds support for DMABUF memory type in videobuf2. It calls
>> relevant APIs of dma_buf for v4l reqbuf / qbuf / dqbuf operations.
>>
>> For this version, the support is for videobuf2 as a user of the shared
>> buffer; so the allocation of the buffer is done outside of V4L2. [A sample
>> allocator of dma-buf shared buffer is given at [1]]
>>
>> [1]: Rob Clark's DRM:
>>    https://github.com/robclark/kernel-omap4/commits/drmplane-dmabuf
>>
>> 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>
>> ---
>>  drivers/media/video/videobuf2-core.c |  184 ++++++++++++++++++++++++++++++-
>>  include/media/videobuf2-core.h       |   31 ++++++
>>  2 files changed, 214 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/video/videobuf2-core.c
>> b/drivers/media/video/videobuf2-core.c index 2e8f1df..b37feea 100644
>> --- a/drivers/media/video/videobuf2-core.c
>> +++ b/drivers/media/video/videobuf2-core.c
> 
> [snip]
> 
>> @@ -451,6 +482,21 @@ static int __verify_mmap_ops(struct vb2_queue *q)
>>  }
>>
>>  /**
>> + * __verify_dmabuf_ops() - verify that all memory operations required for
>> + * DMABUF queue type have been provided
>> + */
>> +static int __verify_dmabuf_ops(struct vb2_queue *q)
>> +{
>> +	if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf
>> +			|| !q->mem_ops->detach_dmabuf
>> +			|| !q->mem_ops->map_dmabuf
>> +			|| !q->mem_ops->unmap_dmabuf)
> 
> That's pretty strange indentation. What about
> 
>         if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf ||
>             !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf ||
>             !q->mem_ops->unmap_dmabuf)
> 

ok

>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>>   * vb2_reqbufs() - Initiate streaming
>>   * @q:		videobuf2 queue
>>   * @req:	struct passed from userspace to vidioc_reqbufs handler in driver
>> @@ -484,6 +530,7 @@ int vb2_reqbufs(struct vb2_queue *q, struct
>> v4l2_requestbuffers *req) }
>>
>>  	if (req->memory != V4L2_MEMORY_MMAP
>> +			&& req->memory != V4L2_MEMORY_DMABUF
>>  			&& req->memory != V4L2_MEMORY_USERPTR) {
>>  		dprintk(1, "reqbufs: unsupported memory type\n");
>>  		return -EINVAL;
> 
> Ditto.
> 
> [snip]
> 
>> @@ -620,7 +672,8 @@ int vb2_create_bufs(struct vb2_queue *q, struct
>> v4l2_create_buffers *create) }
>>
>>  	if (create->memory != V4L2_MEMORY_MMAP
>> -			&& create->memory != V4L2_MEMORY_USERPTR) {
>> +			&& create->memory != V4L2_MEMORY_USERPTR
>> +			&& create->memory != V4L2_MEMORY_DMABUF) {
>>  		dprintk(1, "%s(): unsupported memory type\n", __func__);
>>  		return -EINVAL;
>>  	}
> 
> And here too.
> 
> [snip]
> 
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index a15d1f1..665e846 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
> 
> [snip]
> 
>> @@ -65,6 +82,17 @@ struct vb2_mem_ops {
>>  					unsigned long size, int write);
>>  	void		(*put_userptr)(void *buf_priv);
>>
>> +	/*
>> +	 * Comment from Rob Clark: XXX: I think the attach / detach could be
>> +	 * handled in the vb2 core, and vb2_mem_ops really just need to get/put
>> +	 * the sglist (and make sure that the sglist fits it's needs..)
>> +	 */
> 
> I think we should address this now. We've previously discussed the question, 
> but haven't reached an agreement.
> 
> Quoting my reply to "[RFCv2 PATCH 3/9] v4l: vb2: Add dma-contig allocator as 
> dma_buf user" on 28/03/2012:
> 
>>> Calling dma_buf_attach could be moved to vb2-core. But it gives little
>>> gain. First, the pointer of dma_buf_attachment would have to be added to
>>> struct vb2_plane. Second, the allocator would have to keep in the copy of
>>> this pointer in its context structure for use of vb2_dc_(un)map_dmabuf
>>> functions.
>>
>> Right. Would it make sense to pass the vb2_plane pointer, or possibly the 
>> dma_buf_attachment pointer, to the mmap_dmabuf and unmap_dmabuf operations ?
>>
>>> Third, dma_buf_attach requires a pointer to 'struct device' which is not
>>> available in the vb2-core layer.
>>
>> OK, that's a problem.
>>
>>> Because of the mentioned reasons I decided to keep attach_dmabuf in
>>> allocator-specific code.
>>
>> Maybe it would make sense to create a vb2_mem_buf structure from which 
>> vb2_dc_buf (and other allocator-specific buffer descriptors) would inherit ? 
>> That structure would store the dma_buf_attach pointer, and common dma-buf
>> code could be put in videobuf2-memops.c and shared between allocators. Just
>> an idea.
> 
> If we find out that the best course of action is to leave the code as-is, we 
> should remove the above comment.
> 

Ok. Adding support for VIVI introduces new problem. This driver is not associated
with any struct device. Therefore attach_dmabuf for vmalloc allocator must not
call dma_buf_attach because this call fails if the device is NULL.

Hiding (non)calling dma_buf_attach inside allocator code really helps.

>> +	void		*(*attach_dmabuf)(void *alloc_ctx, struct dma_buf *dbuf,
>> +				unsigned long size, int write);
>> +	void		(*detach_dmabuf)(void *buf_priv);
>> +	int		(*map_dmabuf)(void *buf_priv);
>> +	void		(*unmap_dmabuf)(void *buf_priv);
>> +
>>  	void		*(*vaddr)(void *buf_priv);
>>  	void		*(*cookie)(void *buf_priv);
> 

Regards,
Tomasz Stanislawski

Re: [PATCH 11/11] v4l: vb2: Add dma-contig allocator as dma_buf user

[Tomasz Stanislawski]

Hi Laurent,
Thanks for review.

On 04/06/2012 05:12 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> On Thursday 05 April 2012 16:00:08 Tomasz Stanislawski wrote:
>> From: Sumit Semwal <sumit.semwal@ti.com>
>>
>> This patch makes changes for adding dma-contig as a dma_buf user. It
>> provides function implementations for the {attach, detach, map,
>> unmap}_dmabuf() mem_ops of DMABUF memory type.
>>
>> Signed-off-by: Sumit Semwal <sumit.semwal@ti.com>
>> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
>> 	[author of the original patch]
>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> 	[integration with refactored dma-contig allocator]
>> ---
>>  drivers/media/video/videobuf2-dma-contig.c |  117 +++++++++++++++++++++++++
>>  1 files changed, 117 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/video/videobuf2-dma-contig.c
>> b/drivers/media/video/videobuf2-dma-contig.c index 30f316588..6329483
>> 100644
>> --- a/drivers/media/video/videobuf2-dma-contig.c
>> +++ b/drivers/media/video/videobuf2-dma-contig.c
>> @@ -10,6 +10,7 @@
>>   * the Free Software Foundation.
>>   */
>>
>> +#include <linux/dma-buf.h>
>>  #include <linux/module.h>
>>  #include <linux/scatterlist.h>
>>  #include <linux/sched.h>
>> @@ -33,6 +34,9 @@ struct vb2_dc_buf {
>>
>>  	/* USERPTR related */
>>  	struct vm_area_struct		*vma;
>> +
>> +	/* DMABUF related */
>> +	struct dma_buf_attachment	*db_attach;
>>  };
>>
>>  /*********************************************/
>> @@ -425,6 +429,115 @@ fail_buf:
>>  }
>>
>>  /*********************************************/
>> +/*       callbacks for DMABUF buffers        */
>> +/*********************************************/
>> +
>> +static int vb2_dc_map_dmabuf(void *mem_priv)
>> +{
>> +	struct vb2_dc_buf *buf = mem_priv;
>> +	struct sg_table *sgt;
>> +	unsigned long contig_size;
>> +	int ret = 0;
>> +
>> +	if (WARN_ON(!buf->db_attach)) {
>> +		printk(KERN_ERR "trying to pin a non attached buffer\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (WARN_ON(buf->dma_sgt)) {
>> +		printk(KERN_ERR "dmabuf buffer is already pinned\n");
>> +		return 0;
>> +	}
>> +
>> +	/* get the associated scatterlist for this buffer */
>> +	sgt = dma_buf_map_attachment(buf->db_attach, buf->dma_dir);
>> +	if (IS_ERR_OR_NULL(sgt)) {
>> +		printk(KERN_ERR "Error getting dmabuf scatterlist\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* checking if dmabuf is big enough to store contiguous chunk */
>> +	contig_size = vb2_dc_get_contiguous_size(sgt);
>> +	if (contig_size < buf->size) {
>> +		printk(KERN_ERR "contiguous chunk of dmabuf is too small\n");
>> +		ret = -EFAULT;
>> +		goto fail_map;
> 
> The fail_map label is only used here, you can move the 
> dma_buf_unmap_attachment() call here and return -EFAULT directly.
> 

Patch "[RFC 07/13] v4l: vb2-dma-contig: change map/unmap behaviour for importers"
introduces other cleanup labels. Anyway I will remove this redundant label as
you proposed.

>> +	}
>> +
>> +	buf->dma_addr = sg_dma_address(sgt->sgl);
>> +	buf->dma_sgt = sgt;
>> +
>> +	return 0;
>> +
>> +fail_map:
>> +	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
>> +
>> +	return ret;
>> +}
>> +
>> +static void vb2_dc_unmap_dmabuf(void *mem_priv)
>> +{
>> +	struct vb2_dc_buf *buf = mem_priv;
>> +	struct sg_table *sgt = buf->dma_sgt;
>> +
>> +	if (WARN_ON(!buf->db_attach)) {
>> +		printk(KERN_ERR "trying to unpin a not attached buffer\n");
>> +		return;
>> +	}
>> +
>> +	if (WARN_ON(!sgt)) {
>> +		printk(KERN_ERR "dmabuf buffer is already unpinned\n");
>> +		return;
>> +	}
>> +
>> +	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
>> +
>> +	buf->dma_addr = 0;
>> +	buf->dma_sgt = NULL;
>> +}
>> +
>> +static void vb2_dc_detach_dmabuf(void *mem_priv)
>> +{
>> +	struct vb2_dc_buf *buf = mem_priv;
>> +
>> +	if (buf->dma_addr)
>> +		vb2_dc_unmap_dmabuf(buf);
> 
> What would you think about calling vb2_dc_unmap_dmabuf() from vb2 core 
> instead, to keep the map/unmap calls symmetrical (the second WARN_ON and the 
> related printk in vb2_dc_unmap_dmabuf() might need to go then) ?
> 

Detach without unmap may happen in a case of closing a video fd
without prior dequeuing of all buffers.

A call map_dmabuf should be moved to __enqueue_in_driver and add calling
unmap_dmabuf to vb2_buffer_done in order to keep map/unmap calls
symmetrical. I'll check if this could work.

Regards,
Tomasz Stanislawski

Re: [PATCH 08/11] v4l: vb2-dma-contig: add support for scatterlist in userptr mode

[Laurent Pinchart]

Hi Tomasz,

On Wednesday 11 April 2012 12:06:59 Tomasz Stanislawski wrote:
> On 04/06/2012 05:02 PM, Laurent Pinchart wrote:
> > On Thursday 05 April 2012 16:00:05 Tomasz Stanislawski wrote:

[snip]

> > Also, Documentation/CodingStyle favors one variable declaration per line,
> > without commas for multiple declarations.
> 
> does it include loop counters like i and j?

According to CodingStyle, yes :-)

[snip]

> >> +static struct vm_area_struct *vb2_dc_get_user_vma(
> >> +	unsigned long start, unsigned long size)
> >> +{
> >> +	struct vm_area_struct *vma;
> >> +
> >> +	/* current->mm->mmap_sem is taken by videobuf2 core */
> >> +	vma = find_vma(current->mm, start);
> >> +	if (!vma) {
> >> +		printk(KERN_ERR "no vma for address %lu\n", start);
> >> +		return ERR_PTR(-EFAULT);
> >> +	}
> >> +
> >> +	if (vma->vm_end - vma->vm_start < size) {
> >> +		printk(KERN_ERR "vma at %lu is too small for %lu bytes\n",
> >> +			start, size);
> >> +		return ERR_PTR(-EFAULT);
> >> +	}
> > 
> > Should we support multiple VMAs, or do you think that's not worth it ?
> 
> What do you mean by multiple VMAs?

I mean multiple VMAs for a single userspace buffer. It's probably overkill, 
but I'm not familiar enough with the memory management code to be sure. Do you 
have more insight ?

> >> +	vma = vb2_get_vma(vma);
> >> +	if (!vma) {
> >> +		printk(KERN_ERR "failed to copy vma\n");
> >> +		return ERR_PTR(-ENOMEM);
> >> +	}
> > 
> > I still think there's no need to copy the VMA. get_user_pages() will make
> > sure the memory doesn't get paged out, and we don't need to ensure that
> > the userspace mapping stays in place as our cache operations use a
> > scatter list. Storing the result of vma_is_io() in vb2_dc_buf should be
> > enough.
> 
> As I understand calling get_user_pages ensures that pages are not going to
> be swapped or freed. I agree that it provides enough protection for the
> memory.
> 
> IO mappings are the problem. As you mentioned few mails ago get_page would
> likely crash for such pages. AFAIK increasing reference count for VMA could
> be a reliable mechanism for protecting the memory from being freed.

The main use case here (which is actually the only use case I have 
encountered) is memory reserved at boot time to be used by specific devices 
such as frame buffers. That memory will never be paged out, so I don't think 
there's an issue here. Regarding freeing, it will likely not be freed either, 
and if it does, I doubt that duplicating the VMA will make any difference. 

> The problem is that VMA has no reference counters in it. Calling open ops
> will protect the memory. However it will not protect VMA structure from
> being freed!
> 
> Analyze following scenario:
> 
> - mmap -> returns userptr
> - qbuf (userptr)
> - unmap (userptr)
> - dqbuf
> 
> The VMA will be destroyed at unmap but memory will not be released.
>
> The reason is that open ops was called at qbuf.

I think I see your point. You want to make sure that the exporter driver (on 
which mmap() has been called) will not release the memory, and to do so you 
call the exporter's open() vm operation when you acquire the memory. To call 
the exporter's close() operation when you release the memory you need a 
pointer to the VMA, but the original VMA might have disappeared. To work 
around the problem you make a copy of the VMA and use it when releasing the 
memory.

That's a pretty dirty hack. Most of the copy VMA fields will be invalid when 
you use it. On a side note, would storing vm_ops and vm_private_data be enough 
? I'm also not sure if we need to call get_file() and put_file().

> In order to free the memory the VB2 has to call close ops. This
> callback takes pointer to VMA as an argument. The original VMA
> cannot be used here because it is not longer valid.
> 
> Therefore VMA has to be copied at qbuf operation. The function vb2_get_vma
> is used for this purpose.
>
> The workaround could be dropping support for IO mappings in VB2.
> However it will handicap all s5p media drivers because mapping
> produced by dma_mmap_coherent (aka writecombine) are IO mapping.
> As result s5p-fimc could no longer create a pipeline with s5p-mfc.

There's no reason to drop that, even if it's currently hackish :-) (at least 
until we have a working replacement).
 
> Introducing DMABUF to V4L is a good alternative but only if exporting
> is supported.
> 
> For now I think it is better to keep support for IO mappings. At least
> until DMA mapping redesign and DMABUF exporting in V4L is merged.

I agree. DMABUF is the way to go, but we need to get it in first.

> >> +	return vma;
> >> +}

-- 
Regards,

Laurent Pinchart

RE: [PATCH 08/11] v4l: vb2-dma-contig: add support for scatterlist in userptr mode

[Marek Szyprowski]

Hi Laurent,

On Tuesday, April 17, 2012 2:41 AM Laurent Pinchart wrote:

(snipped)

> > >> +static struct vm_area_struct *vb2_dc_get_user_vma(
> > >> +	unsigned long start, unsigned long size)
> > >> +{
> > >> +	struct vm_area_struct *vma;
> > >> +
> > >> +	/* current->mm->mmap_sem is taken by videobuf2 core */
> > >> +	vma = find_vma(current->mm, start);
> > >> +	if (!vma) {
> > >> +		printk(KERN_ERR "no vma for address %lu\n", start);
> > >> +		return ERR_PTR(-EFAULT);
> > >> +	}
> > >> +
> > >> +	if (vma->vm_end - vma->vm_start < size) {
> > >> +		printk(KERN_ERR "vma at %lu is too small for %lu bytes\n",
> > >> +			start, size);
> > >> +		return ERR_PTR(-EFAULT);
> > >> +	}
> > >
> > > Should we support multiple VMAs, or do you think that's not worth it ?
> >
> > What do you mean by multiple VMAs?
> 
> I mean multiple VMAs for a single userspace buffer. It's probably overkill,
> but I'm not familiar enough with the memory management code to be sure. Do you
> have more insight ?

Multiple VMAs means that userspace did something really hacky in the specified 
address range, I'm really convinced that we should not bother supporting such 
cases.

With user pointer mode You usually want to get access to either anonymous pages
(malloc and friends) or the memory somehow allocated by the other device 
(mmaped to userspace). In both cases it available as a single VMA. VMAs with 
anonymous memory are merged together if they get extended to meet side-by-side 
each other.

> 
> > >> +	vma = vb2_get_vma(vma);
> > >> +	if (!vma) {
> > >> +		printk(KERN_ERR "failed to copy vma\n");
> > >> +		return ERR_PTR(-ENOMEM);
> > >> +	}
> > >
> > > I still think there's no need to copy the VMA. get_user_pages() will make
> > > sure the memory doesn't get paged out, and we don't need to ensure that
> > > the userspace mapping stays in place as our cache operations use a
> > > scatter list. Storing the result of vma_is_io() in vb2_dc_buf should be
> > > enough.
> >
> > As I understand calling get_user_pages ensures that pages are not going to
> > be swapped or freed. I agree that it provides enough protection for the
> > memory.
> >
> > IO mappings are the problem. As you mentioned few mails ago get_page would
> > likely crash for such pages. AFAIK increasing reference count for VMA could
> > be a reliable mechanism for protecting the memory from being freed.
> 
> The main use case here (which is actually the only use case I have
> encountered) is memory reserved at boot time to be used by specific devices
> such as frame buffers. That memory will never be paged out, so I don't think
> there's an issue here. Regarding freeing, it will likely not be freed either,
> and if it does, I doubt that duplicating the VMA will make any difference.

We use user pointer method also to access buffers allocated dynamically by 
other v4l2 devices (we have quite a lot of the in our system). In this case
duplicating VMA is necessary.

> > The problem is that VMA has no reference counters in it. Calling open ops
> > will protect the memory. However it will not protect VMA structure from
> > being freed!
> >
> > Analyze following scenario:
> >
> > - mmap -> returns userptr
> > - qbuf (userptr)
> > - unmap (userptr)
> > - dqbuf
> >
> > The VMA will be destroyed at unmap but memory will not be released.
> >
> > The reason is that open ops was called at qbuf.
> 
> I think I see your point. You want to make sure that the exporter driver (on
> which mmap() has been called) will not release the memory, and to do so you
> call the exporter's open() vm operation when you acquire the memory. To call
> the exporter's close() operation when you release the memory you need a
> pointer to the VMA, but the original VMA might have disappeared. To work
> around the problem you make a copy of the VMA and use it when releasing the
> memory.
> 
> That's a pretty dirty hack. Most of the copy VMA fields will be invalid when
> you use it. On a side note, would storing vm_ops and vm_private_data be enough
> ? I'm also not sure if we need to call get_file() and put_file().

This code is there from the beginning of the videobuf2. The main problem is the
fact that you cannot get a reliable access to user pointer memory which is not 
described with anonymous pages. The hacks/workarounds we use at works really
well with the memory mmaped by the other v4l2 devices (which use vm_open/
vm_close refcounting) and framebuffers which use no refcounting on vma, but we
force them not to release memory by calling get_file() (so the framebuffer 
driver cannot be removed/unloaded once we use its memory, yes, pretty 
theoretical case).

Copying vma was the only solution for the races that usually happen on process 
cleanup or special 'nasty' test case which closed the device before closing the
other v4l2 which used its memory with user pointer method.

The most critical parts of vma are NULLed (vm_mm, vm_next, vm_prev) to catch 
possible issues, but the sane close callback should not touch them anyway. 
Besides vm_private_data, close callback might need to access vm_file, 
vm_start/vm_end and vm_flags. Maybe coping them explicitly while keeping 
everything else NULLed would be a better idea. 

(snipped)

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH 08/11] v4l: vb2-dma-contig: add support for scatterlist in userptr mode

[Laurent Pinchart]

Hi Marek,

On Tuesday 17 April 2012 13:25:56 Marek Szyprowski wrote:
> On Tuesday, April 17, 2012 2:41 AM Laurent Pinchart wrote:
> 
> (snipped)
> 
> > > >> +static struct vm_area_struct *vb2_dc_get_user_vma(
> > > >> +	unsigned long start, unsigned long size)
> > > >> +{
> > > >> +	struct vm_area_struct *vma;
> > > >> +
> > > >> +	/* current->mm->mmap_sem is taken by videobuf2 core */
> > > >> +	vma = find_vma(current->mm, start);
> > > >> +	if (!vma) {
> > > >> +		printk(KERN_ERR "no vma for address %lu\n", start);
> > > >> +		return ERR_PTR(-EFAULT);
> > > >> +	}
> > > >> +
> > > >> +	if (vma->vm_end - vma->vm_start < size) {
> > > >> +		printk(KERN_ERR "vma at %lu is too small for %lu bytes\n",
> > > >> +			start, size);
> > > >> +		return ERR_PTR(-EFAULT);
> > > >> +	}
> > > > 
> > > > Should we support multiple VMAs, or do you think that's not worth it ?
> > > 
> > > What do you mean by multiple VMAs?
> > 
> > I mean multiple VMAs for a single userspace buffer. It's probably
> > overkill, but I'm not familiar enough with the memory management code to
> > be sure. Do you have more insight ?
> 
> Multiple VMAs means that userspace did something really hacky in the
> specified address range, I'm really convinced that we should not bother
> supporting such cases.

I agree. I just wanted to make sure that I wasn't forgetting an important use 
case.

> With user pointer mode You usually want to get access to either anonymous
> pages (malloc and friends) or the memory somehow allocated by the other
> device (mmaped to userspace). In both cases it available as a single VMA.
> VMAs with anonymous memory are merged together if they get extended to meet
> side-by-side each other.
> 
> > > >> +	vma = vb2_get_vma(vma);
> > > >> +	if (!vma) {
> > > >> +		printk(KERN_ERR "failed to copy vma\n");
> > > >> +		return ERR_PTR(-ENOMEM);
> > > >> +	}
> > > > 
> > > > I still think there's no need to copy the VMA. get_user_pages() will
> > > > make sure the memory doesn't get paged out, and we don't need to
> > > > ensure that the userspace mapping stays in place as our cache
> > > > operations use a scatter list. Storing the result of vma_is_io() in
> > > > vb2_dc_buf should be enough.
> > > 
> > > As I understand calling get_user_pages ensures that pages are not going
> > > to be swapped or freed. I agree that it provides enough protection for
> > > the memory.
> > > 
> > > IO mappings are the problem. As you mentioned few mails ago get_page
> > > would likely crash for such pages. AFAIK increasing reference count for
> > > VMA could be a reliable mechanism for protecting the memory from being
> > > freed.
> > 
> > The main use case here (which is actually the only use case I have
> > encountered) is memory reserved at boot time to be used by specific
> > devices such as frame buffers. That memory will never be paged out, so I
> > don't think there's an issue here. Regarding freeing, it will likely not
> > be freed either, and if it does, I doubt that duplicating the VMA will
> > make any difference.
>
> We use user pointer method also to access buffers allocated dynamically by
> other v4l2 devices (we have quite a lot of the in our system). In this case
> duplicating VMA is necessary.
> 
> > > The problem is that VMA has no reference counters in it. Calling open
> > > ops will protect the memory. However it will not protect VMA structure
> > > from being freed!
> > > 
> > > Analyze following scenario:
> > > 
> > > - mmap -> returns userptr
> > > - qbuf (userptr)
> > > - unmap (userptr)
> > > - dqbuf
> > > 
> > > The VMA will be destroyed at unmap but memory will not be released.
> > > 
> > > The reason is that open ops was called at qbuf.
> > 
> > I think I see your point. You want to make sure that the exporter driver
> > (on which mmap() has been called) will not release the memory, and to do
> > so you call the exporter's open() vm operation when you acquire the
> > memory. To call the exporter's close() operation when you release the
> > memory you need a pointer to the VMA, but the original VMA might have
> > disappeared. To work around the problem you make a copy of the VMA and
> > use it when releasing the memory.
> > 
> > That's a pretty dirty hack. Most of the copy VMA fields will be invalid
> > when you use it. On a side note, would storing vm_ops and vm_private_data
> > be enough ? I'm also not sure if we need to call get_file() and
> > put_file().
> 
> This code is there from the beginning of the videobuf2. The main problem is
> the fact that you cannot get a reliable access to user pointer memory which
> is not described with anonymous pages. The hacks/workarounds we use at
> works really well with the memory mmaped by the other v4l2 devices (which
> use vm_open/ vm_close refcounting) and framebuffers which use no
> refcounting on vma, but we force them not to release memory by calling
> get_file() (so the framebuffer driver cannot be removed/unloaded once we
> use its memory, yes, pretty theoretical case).
> 
> Copying vma was the only solution for the races that usually happen on
> process cleanup or special 'nasty' test case which closed the device before
> closing the other v4l2 which used its memory with user pointer method.
> 
> The most critical parts of vma are NULLed (vm_mm, vm_next, vm_prev) to catch
> possible issues, but the sane close callback should not touch them anyway.
> Besides vm_private_data, close callback might need to access vm_file,
> vm_start/vm_end and vm_flags. Maybe coping them explicitly while keeping
> everything else NULLed would be a better idea.

Thank you for the clarification. I'm very eager to see DMABUF being adopted 
everywhere, so that we can get rid of those hacks :-)

-- 
Regards,

Laurent Pinchart