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

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>

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

};

/**

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

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);

}

/*********************************************/

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);

}

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