Re: [Linaro-mm-sig] [RFCv1 2/4] v4l:vb2: add support for shared buffer (dma_buf)

Hi Sumit,

Thanks for the patch!

Sumit Semwal wrote: ...

@@ -962,6 +1030,109 @@ 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 = PTR_ERR(dbuf);
  

• 	goto err;
  

• }
  

• /* this doesn't get filled in until __fill_vb2_buffer(),
  

•  * since it isn't known until after dma_buf_get()..
  

•  */
  

• planes[plane].length = dbuf->size;
  

• /* Skip the plane if already verified */
  

• if (dbuf == vb->planes[plane].dbuf) {
  

• 	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, plane, 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 = q->mem_ops->attach_dmabuf(
  

• 		q->alloc_ctx[plane], dbuf);
  

• if (IS_ERR(mem_priv)) {
  

• 	dprintk(1, "qbuf: failed acquiring dmabuf "
  

• 		"memory for plane %d\n", plane);
  

• 	ret = PTR_ERR(mem_priv);
  

• 	goto err;
  

• }
  

• 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 = q->mem_ops->map_dmabuf(
  

• 		vb->planes[plane].mem_priv, write);
  

• if (ret) {
  

• 	dprintk(1, "qbuf: failed mapping dmabuf "
  

• 		"memory for plane %d\n", plane);
  

• 	goto err;
  

• }
  

• }

Shouldn't the buffer mapping only be done at the first call to __qbuf_dmabuf()? On latter calls, the cache would need to be handled according to presence of V4L2_BUF_FLAG_NO_CACHE_CLEAN / V4L2_BUF_FLAG_NO_CACHE_INVALIDATE in v4l2_buffer.

• /*

• * 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) @@ -985,6 +1156,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;

@@ -1284,6 +1458,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"); @@ -1307,6 +1482,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, plane, unmap_dmabuf,
  

• 		vb->planes[plane].mem_priv);
  

Same here, except reverse: this only should be done when the buffer is destroyed --- either when the user explicitly calls reqbufs(0) or when the file handle owning this buffer is being closed.

Mapping buffers at every prepare_buf and unmapping them in dqbuf is prohibitively expensive. Same goes for many other APIs than V4L2, I think.

I wonder if the means to do this exists already.

I have to admit I haven't followed the dma_buf discussion closely so I might be missing something relevant here.

Kind regards,