From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [RFCv2 PATCH 3/9] v4l: vb2: Add dma-contig allocator as dma_buf user Date: Thu, 22 Mar 2012 12:04:36 +0100 Message-ID: <2700146.cfupX1bb0I@avalon> References: <1331633827-503-1-git-send-email-t.stanislaws@samsung.com> <1331633827-503-4-git-send-email-t.stanislaws@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 9CFB19E8D1 for ; Thu, 22 Mar 2012 04:04:05 -0700 (PDT) In-Reply-To: <1331633827-503-4-git-send-email-t.stanislaws@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Tomasz Stanislawski Cc: ben@bwidawsk.net, daniel.vetter@ffwll.ch, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, kyungmin.park@samsung.com, sakari.ailus@iki.fi, airlied@redhat.com, Sumit Semwal , m.szyprowski@samsung.com List-Id: dri-devel@lists.freedesktop.org Hi Tomasz, On Tuesday 13 March 2012 11:17:01 Tomasz Stanislawski wrote: > From: Sumit Semwal > > 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 > Signed-off-by: Sumit Semwal > [author of the original patch] > Signed-off-by: Tomasz Stanislawski > [integration with refactored dma-contig allocator] > --- > drivers/media/video/videobuf2-dma-contig.c | 116 > ++++++++++++++++++++++++++++ 1 files changed, 116 insertions(+), 0 > deletions(-) > > diff --git a/drivers/media/video/videobuf2-dma-contig.c > b/drivers/media/video/videobuf2-dma-contig.c index c1dc043..746dd5f 100644 > --- a/drivers/media/video/videobuf2-dma-contig.c > +++ b/drivers/media/video/videobuf2-dma-contig.c > @@ -35,6 +35,9 @@ struct vb2_dc_buf { > > /* USERPTR related */ > struct vm_area_struct *vma; > + > + /* DMABUF related */ > + struct dma_buf_attachment *db_attach; > }; > > /*********************************************/ > @@ -485,6 +488,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; > + } I've checked why dma_map_sg() was missing from here, and then remembered that the exporter is still responsible for mapping the buffer to the importer's device address space :-) I'll raise that topic separately. > + > + /* 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); > + > + 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_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); Can detach() be called with the buffer still mapped() ? Wouldn't that be a bug in the caller ? > + > + /* detach this attachment */ > + dma_buf_detach(buf->db_attach->dmabuf, buf->db_attach); > + kfree(buf); > +} There's nothing allocator-specific in the attach/detach operations. Shouldn't they be moved to the vb2 core ? > + > +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 */ > /*********************************************/ > > @@ -498,6 +610,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