Hello everyone, The patches adds support for DMABUF [1] exporting to V4L2 stack. It was updated from [7] after Laurent Pinchart's review. The previous patchset was split into two parts. The support for DMABUF importing was posted in [2]. The exporter part is dependant on DMA mapping redesign [3] which is not merged into the mainline. Therefore it is posted as a separate patchset.
This patchset is rebased on 3.4-rc1 plus the following patchsets:
- support for DMABUF importing in V4L2 [2] - DMA mapping redesign [3] - support for dma_get_pages extension [4] - support for vmap extension to dmabuf framework by Dave Airlie [5][6]
[1] https://lkml.org/lkml/2011/12/26/29 [2] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/46586 [3] http://thread.gmane.org/gmane.linux.kernel.cross-arch/12819 [4] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/43793... [5] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/46713 [6] http://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-dmabuf2&id=c481... [7] http://thread.gmane.org/gmane.comp.video.dri.devel/66213
Marek Szyprowski (1): v4l: vb2-dma-contig: let mmap method to use dma_mmap_coherent call
Tomasz Stanislawski (12): v4l: add buffer exporting via dmabuf v4l: vb2: add buffer exporting via dmabuf v4l: vb2-dma-contig: add setup of sglist for MMAP buffers v4l: vb2-dma-contig: add support for DMABUF exporting v4l: vb2-dma-contig: add vmap/kmap for dmabuf exporting v4l: vb2-dma-contig: change map/unmap behaviour for importers v4l: vb2-dma-contig: change map/unmap behaviour for exporters v4l: s5p-tv: mixer: support for dmabuf importing v4l: s5p-tv: mixer: support for dmabuf exporting v4l: fimc: support for dmabuf importing v4l: fimc: support for dmabuf exporting v4l: vivi: support for dmabuf exporting
drivers/media/video/Kconfig | 1 + drivers/media/video/s5p-fimc/fimc-capture.c | 11 ++- drivers/media/video/s5p-tv/Kconfig | 1 + drivers/media/video/s5p-tv/mixer_video.c | 12 ++- drivers/media/video/v4l2-compat-ioctl32.c | 1 + drivers/media/video/v4l2-ioctl.c | 7 + drivers/media/video/videobuf2-core.c | 66 ++++++++ drivers/media/video/videobuf2-dma-contig.c | 224 +++++++++++++++++++++++++- drivers/media/video/vivi.c | 9 + include/linux/videodev2.h | 23 +++ include/media/v4l2-ioctl.h | 2 + include/media/videobuf2-core.h | 2 + 12 files changed, 348 insertions(+), 11 deletions(-)
This patch adds extension to V4L2 api. It allow to export a mmap buffer as file descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset used by mmap and return a file descriptor on success.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/v4l2-compat-ioctl32.c | 1 + drivers/media/video/v4l2-ioctl.c | 7 +++++++ include/linux/videodev2.h | 23 +++++++++++++++++++++++ include/media/v4l2-ioctl.h | 2 ++ 4 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c index 2829d25..f32b291 100644 --- a/drivers/media/video/v4l2-compat-ioctl32.c +++ b/drivers/media/video/v4l2-compat-ioctl32.c @@ -954,6 +954,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) case VIDIOC_S_FBUF32: case VIDIOC_OVERLAY32: case VIDIOC_QBUF32: + case VIDIOC_EXPBUF: case VIDIOC_DQBUF32: case VIDIOC_STREAMON32: case VIDIOC_STREAMOFF32: diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 5b2ec1f..3ca9d68 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -207,6 +207,7 @@ static const char *v4l2_ioctls[] = { [_IOC_NR(VIDIOC_S_FBUF)] = "VIDIOC_S_FBUF", [_IOC_NR(VIDIOC_OVERLAY)] = "VIDIOC_OVERLAY", [_IOC_NR(VIDIOC_QBUF)] = "VIDIOC_QBUF", + [_IOC_NR(VIDIOC_EXPBUF)] = "VIDIOC_EXPBUF", [_IOC_NR(VIDIOC_DQBUF)] = "VIDIOC_DQBUF", [_IOC_NR(VIDIOC_STREAMON)] = "VIDIOC_STREAMON", [_IOC_NR(VIDIOC_STREAMOFF)] = "VIDIOC_STREAMOFF", @@ -938,6 +939,12 @@ static long __video_do_ioctl(struct file *file, dbgbuf(cmd, vfd, p); break; } + case VIDIOC_EXPBUF: + { + if (ops->vidioc_expbuf) + ret = ops->vidioc_expbuf(file, fh, arg); + break; + } case VIDIOC_DQBUF: { struct v4l2_buffer *p = arg; diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 38259bf..627e235 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -680,6 +680,28 @@ struct v4l2_buffer { #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0800 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x1000
+/** + * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor + * + * @fd: file descriptor associated with DMABUF (set by driver) + * @mem_offset: a "cookie" that is passed to mmap() as offset + * @flags: flags for newly created file, currently only O_CLOEXEC is + * supported, refer to manual of open syscall for more details + * + * Contains data used for exporting a video buffer as DMABUF file + * descriptor. Uses the same 'cookie' as mmap() syscall. All reserved fields + * must be set to zero. The field reserved0 is expected to become a structure + * 'type' allowing an alternative layout of the structure content. Therefore + * this field should not be used for any other extensions. + */ +struct v4l2_exportbuffer { + __u32 fd; + __u32 reserved0; + __u32 mem_offset; + __u32 flags; + __u32 reserved[12]; +}; + /* * O V E R L A Y P R E V I E W */ @@ -2327,6 +2349,7 @@ struct v4l2_create_buffers { #define VIDIOC_S_FBUF _IOW('V', 11, struct v4l2_framebuffer) #define VIDIOC_OVERLAY _IOW('V', 14, int) #define VIDIOC_QBUF _IOWR('V', 15, struct v4l2_buffer) +#define VIDIOC_EXPBUF _IOWR('V', 16, struct v4l2_exportbuffer) #define VIDIOC_DQBUF _IOWR('V', 17, struct v4l2_buffer) #define VIDIOC_STREAMON _IOW('V', 18, int) #define VIDIOC_STREAMOFF _IOW('V', 19, int) diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h index 3cb939c..c161ec3 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -119,6 +119,8 @@ struct v4l2_ioctl_ops { int (*vidioc_reqbufs) (struct file *file, void *fh, struct v4l2_requestbuffers *b); int (*vidioc_querybuf)(struct file *file, void *fh, struct v4l2_buffer *b); int (*vidioc_qbuf) (struct file *file, void *fh, struct v4l2_buffer *b); + int (*vidioc_expbuf) (struct file *file, void *fh, + struct v4l2_exportbuffer *e); int (*vidioc_dqbuf) (struct file *file, void *fh, struct v4l2_buffer *b);
int (*vidioc_create_bufs)(struct file *file, void *fh, struct v4l2_create_buffers *b);
This patch adds extension to videobuf2-core. It allow to export a mmap buffer as a file descriptor.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/videobuf2-core.c | 66 ++++++++++++++++++++++++++++++++++ include/media/videobuf2-core.h | 2 + 2 files changed, 68 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c index b37feea..ff902aa 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -1710,6 +1710,72 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, }
/** + * vb2_expbuf() - Export a buffer as a file descriptor + * @q: videobuf2 queue + * @b: export buffer structure passed from userspace to vidioc_expbuf + * handler in driver + * + * The return values from this function are intended to be directly returned + * from vidioc_expbuf handler in driver. + */ +int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb) +{ + struct vb2_buffer *vb = NULL; + struct vb2_plane *vb_plane; + unsigned int buffer, plane; + int ret; + struct dma_buf *dbuf; + + if (q->memory != V4L2_MEMORY_MMAP) { + dprintk(1, "Queue is not currently set up for mmap\n"); + return -EINVAL; + } + + if (!q->mem_ops->get_dmabuf) { + dprintk(1, "Queue does not support DMA buffer exporting\n"); + return -EINVAL; + } + + if (eb->flags & ~O_CLOEXEC) { + dprintk(1, "Queue supports only O_CLOEXEC flag\n"); + return -EINVAL; + } + + /* + * Find the plane corresponding to the offset passed by userspace. + */ + ret = __find_plane_by_offset(q, eb->mem_offset, &buffer, &plane); + if (ret) { + dprintk(1, "invalid offset %u\n", eb->mem_offset); + return ret; + } + + vb = q->bufs[buffer]; + vb_plane = &vb->planes[plane]; + + dbuf = call_memop(q, get_dmabuf, vb_plane->mem_priv); + if (IS_ERR_OR_NULL(dbuf)) { + dprintk(1, "Failed to export buffer %d, plane %d\n", + buffer, plane); + return -EINVAL; + } + + ret = dma_buf_fd(dbuf, eb->flags); + if (ret < 0) { + dprintk(3, "buffer %d, plane %d failed to export (%d)\n", + buffer, plane, ret); + return ret; + } + + dprintk(3, "buffer %d, plane %d exported as %d descriptor\n", + buffer, plane, ret); + eb->fd = ret; + + return 0; +} +EXPORT_SYMBOL_GPL(vb2_expbuf); + +/** * vb2_mmap() - map video buffers into application address space * @q: videobuf2 queue * @vma: vma passed to the mmap file operation handler in the driver diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 244165a..3bd4225 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -81,6 +81,7 @@ struct vb2_fileio_data; struct vb2_mem_ops { void *(*alloc)(void *alloc_ctx, unsigned long size); void (*put)(void *buf_priv); + struct dma_buf *(*get_dmabuf)(void *buf_priv);
void *(*get_userptr)(void *alloc_ctx, unsigned long vaddr, unsigned long size, int write); @@ -354,6 +355,7 @@ int vb2_queue_init(struct vb2_queue *q); void vb2_queue_release(struct vb2_queue *q);
int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b); +int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb); int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking);
int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);
From: Marek Szyprowski m.szyprowski@samsung.com
Let mmap method to use dma_mmap_coherent call. This patch depends on DMA mapping redesign patches because the usage of dma_mmap_coherent breaks dma-contig allocator for architectures other than ARM and AVR.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/media/video/videobuf2-dma-contig.c | 28 ++++++++++++++++++++++++++-- 1 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 6329483..f4df9e2 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -224,14 +224,38 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) { struct vb2_dc_buf *buf = buf_priv; + int ret;
if (!buf) { printk(KERN_ERR "No buffer to map\n"); return -EINVAL; }
- return vb2_mmap_pfn_range(vma, buf->dma_addr, buf->size, - &vb2_common_vm_ops, &buf->handler); + /* + * dma_mmap_* uses vm_pgoff as in-buffer offset, but we want to + * map whole buffer + */ + vma->vm_pgoff = 0; + + ret = dma_mmap_coherent(buf->dev, vma, buf->vaddr, + buf->dma_addr, buf->size); + + if (ret) { + printk(KERN_ERR "Remapping memory failed, error: %d\n", ret); + return ret; + } + + vma->vm_flags |= VM_DONTEXPAND | VM_RESERVED; + vma->vm_private_data = &buf->handler; + vma->vm_ops = &vb2_common_vm_ops; + + vma->vm_ops->open(vma); + + printk(KERN_DEBUG "%s: mapped dma addr 0x%08lx at 0x%08lx, size %ld\n", + __func__, (unsigned long)buf->dma_addr, vma->vm_start, + buf->size); + + return 0; }
/*********************************************/
This patch adds the setup of sglist list for MMAP buffers. It is needed for buffer exporting via DMABUF mechanism.
This patch depends on dma_get_pages extension to DMA api.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/videobuf2-dma-contig.c | 51 ++++++++++++++++++++++++++- 1 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index f4df9e2..0cdcd2b 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -31,6 +31,7 @@ struct vb2_dc_buf { /* MMAP related */ struct vb2_vmarea_handler handler; atomic_t refcount; + struct sg_table *sgt_base;
/* USERPTR related */ struct vm_area_struct *vma; @@ -189,6 +190,7 @@ static void vb2_dc_put(void *buf_priv) if (!atomic_dec_and_test(&buf->refcount)) return;
+ vb2_dc_release_sgtable(buf->sgt_base); dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr); kfree(buf); } @@ -197,6 +199,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) { struct device *dev = alloc_ctx; struct vb2_dc_buf *buf; + int ret = -ENOMEM; + int n_pages; + struct page **pages = NULL;
buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf) @@ -205,10 +210,41 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL); if (!buf->vaddr) { dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size); - kfree(buf); - return ERR_PTR(-ENOMEM); + goto fail_buf; + } + + WARN_ON((unsigned long)buf->vaddr & ~PAGE_MASK); + WARN_ON(buf->dma_addr & ~PAGE_MASK); + + n_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; + + pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL); + if (!pages) { + dev_err(dev, "failed to alloc page table\n"); + goto fail_dma; + } + + ret = dma_get_pages(dev, buf->vaddr, buf->dma_addr, pages, n_pages); + if (ret < 0) { + dev_err(dev, "failed to get buffer pages from DMA API\n"); + goto fail_pages; + } + if (ret != n_pages) { + ret = -EFAULT; + dev_err(dev, "failed to get all pages from DMA API\n"); + goto fail_pages; }
+ buf->sgt_base = vb2_dc_pages_to_sgt(pages, n_pages, 0, 0); + if (IS_ERR(buf->sgt_base)) { + ret = PTR_ERR(buf->sgt_base); + dev_err(dev, "failed to prepare sg table\n"); + goto fail_pages; + } + + /* pages are no longer needed */ + kfree(pages); + buf->dev = dev; buf->size = size;
@@ -219,6 +255,17 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) atomic_inc(&buf->refcount);
return buf; + +fail_pages: + kfree(pages); + +fail_dma: + dma_free_coherent(dev, size, buf->vaddr, buf->dma_addr); + +fail_buf: + kfree(buf); + + return ERR_PTR(ret); }
static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
This patch adds support for exporting a dma-contig buffer using DMABUF interface.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/videobuf2-dma-contig.c | 128 ++++++++++++++++++++++++++++ 1 files changed, 128 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 0cdcd2b..e1ad47e 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -31,6 +31,7 @@ struct vb2_dc_buf { /* MMAP related */ struct vb2_vmarea_handler handler; atomic_t refcount; + struct dma_buf *dma_buf; struct sg_table *sgt_base;
/* USERPTR related */ @@ -190,6 +191,8 @@ static void vb2_dc_put(void *buf_priv) if (!atomic_dec_and_test(&buf->refcount)) return;
+ if (buf->dma_buf) + dma_buf_put(buf->dma_buf); vb2_dc_release_sgtable(buf->sgt_base); dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr); kfree(buf); @@ -306,6 +309,130 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) }
/*********************************************/ +/* DMABUF ops for exporters */ +/*********************************************/ + +struct vb2_dc_attachment { + struct sg_table sgt; + enum dma_data_direction dir; +}; + +static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev, + struct dma_buf_attachment *dbuf_attach) +{ + /* nothing to be done */ + return 0; +} + +static void vb2_dc_dmabuf_ops_detach(struct dma_buf *dbuf, + struct dma_buf_attachment *db_attach) +{ + struct vb2_dc_attachment *attach = db_attach->priv; + struct sg_table *sgt; + + if (!attach) + return; + + sgt = &attach->sgt; + + dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->nents, attach->dir); + sg_free_table(sgt); + kfree(attach); + db_attach->priv = NULL; +} + +static struct sg_table *vb2_dc_dmabuf_ops_map( + struct dma_buf_attachment *db_attach, enum dma_data_direction dir) +{ + struct dma_buf *dbuf = db_attach->dmabuf; + struct vb2_dc_buf *buf = dbuf->priv; + struct vb2_dc_attachment *attach = db_attach->priv; + struct sg_table *sgt; + struct scatterlist *rd, *wr; + int i, ret; + + /* return previously mapped sg table */ + if (attach) + return &attach->sgt; + + attach = kzalloc(sizeof *attach, GFP_KERNEL); + if (!attach) + return ERR_PTR(-ENOMEM); + + sgt = &attach->sgt; + attach->dir = dir; + + /* copying the buf->base_sgt to attachment */ + ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL); + if (ret) { + kfree(attach); + return ERR_PTR(-ENOMEM); + } + + rd = buf->sgt_base->sgl; + wr = sgt->sgl; + for (i = 0; i < sgt->orig_nents; ++i) { + sg_set_page(wr, sg_page(rd), rd->length, rd->offset); + rd = sg_next(rd); + wr = sg_next(wr); + } + + /* mapping new sglist to the client */ + ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir); + if (ret <= 0) { + printk(KERN_ERR "failed to map scatterlist\n"); + sg_free_table(sgt); + kfree(attach); + return ERR_PTR(-EIO); + } + + db_attach->priv = attach; + + return sgt; +} + +static void vb2_dc_dmabuf_ops_unmap(struct dma_buf_attachment *db_attach, + struct sg_table *sgt, enum dma_data_direction dir) +{ + /* nothing to be done here */ +} + +static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf) +{ + /* drop reference obtained in vb2_dc_get_dmabuf */ + vb2_dc_put(dbuf->priv); +} + +static struct dma_buf_ops vb2_dc_dmabuf_ops = { + .attach = vb2_dc_dmabuf_ops_attach, + .detach = vb2_dc_dmabuf_ops_detach, + .map_dma_buf = vb2_dc_dmabuf_ops_map, + .unmap_dma_buf = vb2_dc_dmabuf_ops_unmap, + .release = vb2_dc_dmabuf_ops_release, +}; + +static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv) +{ + struct vb2_dc_buf *buf = buf_priv; + struct dma_buf *dbuf; + + if (buf->dma_buf) + return buf->dma_buf; + + /* dmabuf keeps reference to vb2 buffer */ + atomic_inc(&buf->refcount); + dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0); + if (IS_ERR(dbuf)) { + atomic_dec(&buf->refcount); + return NULL; + } + + buf->dma_buf = dbuf; + + return dbuf; +} + +/*********************************************/ /* callbacks for USERPTR buffers */ /*********************************************/
@@ -615,6 +742,7 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf, const struct vb2_mem_ops vb2_dma_contig_memops = { .alloc = vb2_dc_alloc, .put = vb2_dc_put, + .get_dmabuf = vb2_dc_get_dmabuf, .cookie = vb2_dc_cookie, .vaddr = vb2_dc_vaddr, .mmap = vb2_dc_mmap,
This patch adds support for vmap and kmap callbacks for DMABUF exporter.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/videobuf2-dma-contig.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index e1ad47e..537926b 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -403,11 +403,28 @@ static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf) vb2_dc_put(dbuf->priv); }
+static void *vb2_dc_dmabuf_ops_kmap(struct dma_buf *dbuf, unsigned long pgnum) +{ + struct vb2_dc_buf *buf = dbuf->priv; + + return buf->vaddr + pgnum * PAGE_SIZE; +} + +static void *vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf) +{ + struct vb2_dc_buf *buf = dbuf->priv; + + return buf->vaddr; +} + static struct dma_buf_ops vb2_dc_dmabuf_ops = { .attach = vb2_dc_dmabuf_ops_attach, .detach = vb2_dc_dmabuf_ops_detach, .map_dma_buf = vb2_dc_dmabuf_ops_map, .unmap_dma_buf = vb2_dc_dmabuf_ops_unmap, + .kmap = vb2_dc_dmabuf_ops_kmap, + .kmap_atomic = vb2_dc_dmabuf_ops_kmap, + .vmap = vb2_dc_dmabuf_ops_vmap, .release = vb2_dc_dmabuf_ops_release, };
The DMABUF documentation says that the map_dma_buf callback should return scatterlist that is mapped into a caller's address space. In practice, almost none of existing implementations of DMABUF exporter does it. This patch breaks the DMABUF specification in order to allow exchange DMABUF buffers between other APIs like DRM.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/videobuf2-dma-contig.c | 22 +++++++++++++++++----- 1 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 537926b..7f4a58a 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -652,7 +652,7 @@ 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; + int ret = -EFAULT;
if (WARN_ON(!buf->db_attach)) { printk(KERN_ERR "trying to pin a non attached buffer\n"); @@ -671,12 +671,20 @@ static int vb2_dc_map_dmabuf(void *mem_priv) return -EINVAL; }
+ /* mapping new sglist to the client */ + 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"); + goto fail_map_attachment; + } + /* 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; + printk(KERN_ERR "contiguous chunk is too small %lu/%lu b\n", + contig_size, buf->size); + goto fail_map_sg; }
buf->dma_addr = sg_dma_address(sgt->sgl); @@ -684,7 +692,10 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
return 0;
-fail_map: +fail_map_sg: + dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); + +fail_map_attachment: dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
return ret; @@ -705,6 +716,7 @@ static void vb2_dc_unmap_dmabuf(void *mem_priv) return; }
+ dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
buf->dma_addr = 0;
The DMABUF documentation says that the map_dma_buf callback should return scatterlist that is mapped into a caller's address space. In practice, almost none of existing implementations of DMABUF exporter does it. This patch breaks the DMABUF specification in order to allow exchange DMABUF buffers between other APIs like DRM.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/videobuf2-dma-contig.c | 42 ++++++--------------------- 1 files changed, 10 insertions(+), 32 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 7f4a58a..a6676bd 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -312,11 +312,6 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) /* DMABUF ops for exporters */ /*********************************************/
-struct vb2_dc_attachment { - struct sg_table sgt; - enum dma_data_direction dir; -}; - static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev, struct dma_buf_attachment *dbuf_attach) { @@ -327,17 +322,13 @@ static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev, static void vb2_dc_dmabuf_ops_detach(struct dma_buf *dbuf, struct dma_buf_attachment *db_attach) { - struct vb2_dc_attachment *attach = db_attach->priv; - struct sg_table *sgt; + struct sg_table *sgt = db_attach->priv;
- if (!attach) + if (!sgt) return;
- sgt = &attach->sgt; - - dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->nents, attach->dir); sg_free_table(sgt); - kfree(attach); + kfree(sgt); db_attach->priv = NULL; }
@@ -346,26 +337,22 @@ static struct sg_table *vb2_dc_dmabuf_ops_map( { struct dma_buf *dbuf = db_attach->dmabuf; struct vb2_dc_buf *buf = dbuf->priv; - struct vb2_dc_attachment *attach = db_attach->priv; - struct sg_table *sgt; + struct sg_table *sgt = db_attach->priv; struct scatterlist *rd, *wr; int i, ret;
/* return previously mapped sg table */ - if (attach) - return &attach->sgt; + if (sgt) + return sgt;
- attach = kzalloc(sizeof *attach, GFP_KERNEL); - if (!attach) + sgt = kzalloc(sizeof *sgt, GFP_KERNEL); + if (!sgt) return ERR_PTR(-ENOMEM);
- sgt = &attach->sgt; - attach->dir = dir; - /* copying the buf->base_sgt to attachment */ ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL); if (ret) { - kfree(attach); + kfree(sgt); return ERR_PTR(-ENOMEM); }
@@ -377,16 +364,7 @@ static struct sg_table *vb2_dc_dmabuf_ops_map( wr = sg_next(wr); }
- /* mapping new sglist to the client */ - ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir); - if (ret <= 0) { - printk(KERN_ERR "failed to map scatterlist\n"); - sg_free_table(sgt); - kfree(attach); - return ERR_PTR(-EIO); - } - - db_attach->priv = attach; + db_attach->priv = sgt;
return sgt; }
This patch enhances s5p-tv with support for DMABUF importing via V4L2_MEMORY_DMABUF memory type.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/s5p-tv/Kconfig | 1 + drivers/media/video/s5p-tv/mixer_video.c | 2 +- 2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/s5p-tv/Kconfig b/drivers/media/video/s5p-tv/Kconfig index f248b28..2e80126 100644 --- a/drivers/media/video/s5p-tv/Kconfig +++ b/drivers/media/video/s5p-tv/Kconfig @@ -10,6 +10,7 @@ config VIDEO_SAMSUNG_S5P_TV bool "Samsung TV driver for S5P platform (experimental)" depends on PLAT_S5P && PM_RUNTIME depends on EXPERIMENTAL + select DMA_SHARED_BUFFER default n ---help--- Say Y here to enable selecting the TV output devices for diff --git a/drivers/media/video/s5p-tv/mixer_video.c b/drivers/media/video/s5p-tv/mixer_video.c index f7ca5cc..6b45d93 100644 --- a/drivers/media/video/s5p-tv/mixer_video.c +++ b/drivers/media/video/s5p-tv/mixer_video.c @@ -1074,7 +1074,7 @@ struct mxr_layer *mxr_base_layer_create(struct mxr_device *mdev,
layer->vb_queue = (struct vb2_queue) { .type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, - .io_modes = VB2_MMAP | VB2_USERPTR, + .io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF, .drv_priv = layer, .buf_struct_size = sizeof(struct mxr_buffer), .ops = &mxr_video_qops,
This patch enhances s5p-tv with support for DMABUF exporting via VIDIOC_EXPBUF ioctl.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/s5p-tv/mixer_video.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/s5p-tv/mixer_video.c b/drivers/media/video/s5p-tv/mixer_video.c index 6b45d93..f08edbf 100644 --- a/drivers/media/video/s5p-tv/mixer_video.c +++ b/drivers/media/video/s5p-tv/mixer_video.c @@ -697,6 +697,15 @@ static int mxr_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p) return vb2_dqbuf(&layer->vb_queue, p, file->f_flags & O_NONBLOCK); }
+static int mxr_expbuf(struct file *file, void *priv, + struct v4l2_exportbuffer *eb) +{ + struct mxr_layer *layer = video_drvdata(file); + + mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__); + return vb2_expbuf(&layer->vb_queue, eb); +} + static int mxr_streamon(struct file *file, void *priv, enum v4l2_buf_type i) { struct mxr_layer *layer = video_drvdata(file); @@ -724,6 +733,7 @@ static const struct v4l2_ioctl_ops mxr_ioctl_ops = { .vidioc_querybuf = mxr_querybuf, .vidioc_qbuf = mxr_qbuf, .vidioc_dqbuf = mxr_dqbuf, + .vidioc_expbuf = mxr_expbuf, /* Streaming control */ .vidioc_streamon = mxr_streamon, .vidioc_streamoff = mxr_streamoff,
This patch enhances s5p-fimc with support for DMABUF importing via V4L2_MEMORY_DMABUF memory type.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/Kconfig | 1 + drivers/media/video/s5p-fimc/fimc-capture.c | 2 +- 2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index d6294d6..2c3250a 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -1134,6 +1134,7 @@ config VIDEO_SAMSUNG_S5P_FIMC VIDEO_V4L2_SUBDEV_API && EXPERIMENTAL select VIDEOBUF2_DMA_CONTIG select V4L2_MEM2MEM_DEV + select DMA_SHARED_BUFFER ---help--- This is a v4l2 driver for Samsung S5P and EXYNOS4 camera host interface and video postprocessor. diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c index b06efd2..38fb39e 100644 --- a/drivers/media/video/s5p-fimc/fimc-capture.c +++ b/drivers/media/video/s5p-fimc/fimc-capture.c @@ -1530,7 +1530,7 @@ int fimc_register_capture_device(struct fimc_dev *fimc, q = &fimc->vid_cap.vbq; memset(q, 0, sizeof(*q)); q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; - q->io_modes = VB2_MMAP | VB2_USERPTR; + q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF; q->drv_priv = fimc->vid_cap.ctx; q->ops = &fimc_capture_qops; q->mem_ops = &vb2_dma_contig_memops;
This patch enhances s5p-fimc with support for DMABUF exporting via VIDIOC_EXPBUF ioctl.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/s5p-fimc/fimc-capture.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c index 38fb39e..78a4db9 100644 --- a/drivers/media/video/s5p-fimc/fimc-capture.c +++ b/drivers/media/video/s5p-fimc/fimc-capture.c @@ -1011,6 +1011,14 @@ static int fimc_cap_qbuf(struct file *file, void *priv, return vb2_qbuf(&fimc->vid_cap.vbq, buf); }
+static int fimc_cap_expbuf(struct file *file, void *priv, + struct v4l2_exportbuffer *eb) +{ + struct fimc_dev *fimc = video_drvdata(file); + + return vb2_expbuf(&fimc->vid_cap.vbq, eb); +} + static int fimc_cap_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf) { @@ -1146,6 +1154,7 @@ static const struct v4l2_ioctl_ops fimc_capture_ioctl_ops = {
.vidioc_qbuf = fimc_cap_qbuf, .vidioc_dqbuf = fimc_cap_dqbuf, + .vidioc_expbuf = fimc_cap_expbuf,
.vidioc_prepare_buf = fimc_cap_prepare_buf, .vidioc_create_bufs = fimc_cap_create_bufs,
This patch enhances vivi driver with a support for DMABUF exporting via VIDIOC_EXPBUF ioctl.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/vivi.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c index 489d685..e34aa70 100644 --- a/drivers/media/video/vivi.c +++ b/drivers/media/video/vivi.c @@ -947,6 +947,14 @@ static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p) return vb2_dqbuf(&dev->vb_vidq, p, file->f_flags & O_NONBLOCK); }
+static int vidioc_expbuf(struct file *file, void *priv, + struct v4l2_exportbuffer *eb) +{ + struct vivi_dev *dev = video_drvdata(file); + return vb2_expbuf(&dev->vb_vidq, eb); +} + + static int vidioc_streamon(struct file *file, void *priv, enum v4l2_buf_type i) { struct vivi_dev *dev = video_drvdata(file); @@ -1185,6 +1193,7 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = { .vidioc_querybuf = vidioc_querybuf, .vidioc_qbuf = vidioc_qbuf, .vidioc_dqbuf = vidioc_dqbuf, + .vidioc_expbuf = vidioc_expbuf, .vidioc_s_std = vidioc_s_std, .vidioc_enum_input = vidioc_enum_input, .vidioc_g_input = vidioc_g_input,
Hi Tomasz,
Thanks for the patch.
On Tuesday 10 April 2012 15:10:35 Tomasz Stanislawski wrote:
This patch adds extension to V4L2 api. It allow to export a mmap buffer as file descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset used by mmap and return a file descriptor on success.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
This mostly looks good to me, except for the lack of documentation of course :-)
drivers/media/video/v4l2-compat-ioctl32.c | 1 + drivers/media/video/v4l2-ioctl.c | 7 +++++++ include/linux/videodev2.h | 23 +++++++++++++++++++++++ include/media/v4l2-ioctl.h | 2 ++ 4 files changed, 33 insertions(+), 0 deletions(-)
[snip]
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 38259bf..627e235 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -680,6 +680,28 @@ struct v4l2_buffer { #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE 0x0800 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN 0x1000
+/**
- struct v4l2_exportbuffer - export of video buffer as DMABUF file
descriptor
- @fd: file descriptor associated with DMABUF (set by driver)
- @mem_offset: a "cookie" that is passed to mmap() as offset
I wouldn't mention mmap() here, as that's unrelated to the DMABUF exporter API. Maybe something like
"buffer memory offset as returned by VIDIOC_QUERYBUF in struct v4l2_buffer::m.offset (for single-plane formats) or v4l2_plane::m.offset (for multi-planar formats)"
- @flags: flags for newly created file, currently only O_CLOEXEC is
supported, refer to manual of open syscall for more details
- Contains data used for exporting a video buffer as DMABUF file
- *descriptor. Uses the same 'cookie' as mmap() syscall.
The buffer is identified by a 'cookie' returned by VIDIOC_QUERYBUF (identical to the cookie used to mmap() the buffer to userspace).
All reserved fields +*must be set to zero. The field reserved0 is expected to become a structure
- *'type' allowing an alternative layout of the structure content. Therefore
- this field should not be used for any other extensions.
- */
+struct v4l2_exportbuffer {
- __u32 fd;
- __u32 reserved0;
- __u32 mem_offset;
- __u32 flags;
- __u32 reserved[12];
+};
Hi Tomasz,
Thanks for the patch.
On Tuesday 10 April 2012 15:10:37 Tomasz Stanislawski wrote:
From: Marek Szyprowski m.szyprowski@samsung.com
Let mmap method to use dma_mmap_coherent call. This patch depends on DMA mapping redesign patches because the usage of dma_mmap_coherent breaks dma-contig allocator for architectures other than ARM and AVR.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/media/video/videobuf2-dma-contig.c | 28 +++++++++++++++++++++++-- 1 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 6329483..f4df9e2 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -224,14 +224,38 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) { struct vb2_dc_buf *buf = buf_priv;
int ret;
if (!buf) { printk(KERN_ERR "No buffer to map\n"); return -EINVAL; }
- return vb2_mmap_pfn_range(vma, buf->dma_addr, buf->size,
&vb2_common_vm_ops, &buf->handler);
This was the only vb2_mmap_pfn_range() if I'm not mistaken. Should the function be removed ?
- /*
* dma_mmap_* uses vm_pgoff as in-buffer offset, but we want to* map whole buffer*/- vma->vm_pgoff = 0;
Is it safe to set vma->vm_pgoff to 0 here behind memory core's back ?
- ret = dma_mmap_coherent(buf->dev, vma, buf->vaddr,
buf->dma_addr, buf->size);- if (ret) {
printk(KERN_ERR "Remapping memory failed, error: %d\n", ret);return ret;- }
- vma->vm_flags |= VM_DONTEXPAND | VM_RESERVED;
- vma->vm_private_data = &buf->handler;
- vma->vm_ops = &vb2_common_vm_ops;
- vma->vm_ops->open(vma);
- printk(KERN_DEBUG "%s: mapped dma addr 0x%08lx at 0x%08lx, size %ld\n",
__func__, (unsigned long)buf->dma_addr, vma->vm_start,buf->size);- return 0;
}
/*********************************************/
Hi Tomasz,
Thanks for the patch.
On Tuesday 10 April 2012 15:10:36 Tomasz Stanislawski wrote:
This patch adds extension to videobuf2-core. It allow to export a mmap buffer as a file descriptor.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/media/video/videobuf2-core.c | 66 +++++++++++++++++++++++++++++++ include/media/videobuf2-core.h | 2 + 2 files changed, 68 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c index b37feea..ff902aa 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -1710,6 +1710,72 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, }
/**
- vb2_expbuf() - Export a buffer as a file descriptor
- @q: videobuf2 queue
- @b: export buffer structure passed from userspace to vidioc_expbuf
handler in driver
This should be @eb.
- *The return values from this function are intended to be directly returned
- from vidioc_expbuf handler in driver.
- */
+int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb) +{
- struct vb2_buffer *vb = NULL;
- struct vb2_plane *vb_plane;
- unsigned int buffer, plane;
- int ret;
- struct dma_buf *dbuf;
- if (q->memory != V4L2_MEMORY_MMAP) {
dprintk(1, "Queue is not currently set up for mmap\n");return -EINVAL;- }
- if (!q->mem_ops->get_dmabuf) {
dprintk(1, "Queue does not support DMA buffer exporting\n");return -EINVAL;- }
- if (eb->flags & ~O_CLOEXEC) {
dprintk(1, "Queue supports only O_CLOEXEC flag\n");return -EINVAL;- }
- /*
* Find the plane corresponding to the offset passed by userspace.*/- ret = __find_plane_by_offset(q, eb->mem_offset, &buffer, &plane);
- if (ret) {
dprintk(1, "invalid offset %u\n", eb->mem_offset);return ret;- }
- vb = q->bufs[buffer];
- vb_plane = &vb->planes[plane];
- dbuf = call_memop(q, get_dmabuf, vb_plane->mem_priv);
- if (IS_ERR_OR_NULL(dbuf)) {
dprintk(1, "Failed to export buffer %d, plane %d\n",buffer, plane);return -EINVAL;- }
- ret = dma_buf_fd(dbuf, eb->flags);
- if (ret < 0) {
dprintk(3, "buffer %d, plane %d failed to export (%d)\n",buffer, plane, ret);return ret;- }
- dprintk(3, "buffer %d, plane %d exported as %d descriptor\n",
buffer, plane, ret);- eb->fd = ret;
- return 0;
+} +EXPORT_SYMBOL_GPL(vb2_expbuf);
+/**
- vb2_mmap() - map video buffers into application address space
- @q: videobuf2 queue
- @vma: vma passed to the mmap file operation handler in the driver
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 244165a..3bd4225 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -81,6 +81,7 @@ struct vb2_fileio_data; struct vb2_mem_ops { void *(*alloc)(void *alloc_ctx, unsigned long size); void (*put)(void *buf_priv);
struct dma_buf *(*get_dmabuf)(void *buf_priv);
void *(*get_userptr)(void *alloc_ctx, unsigned long vaddr, unsigned long size, int write);
@@ -354,6 +355,7 @@ int vb2_queue_init(struct vb2_queue *q); void vb2_queue_release(struct vb2_queue *q);
int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b); +int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb); int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking);
int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);
Hi Tomasz,
Thanks for the patch.
On Tuesday 10 April 2012 15:10:38 Tomasz Stanislawski wrote:
This patch adds the setup of sglist list for MMAP buffers. It is needed for buffer exporting via DMABUF mechanism.
This patch depends on dma_get_pages extension to DMA api.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/media/video/videobuf2-dma-contig.c | 51 ++++++++++++++++++++++++- 1 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index f4df9e2..0cdcd2b 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c
[snip]
@@ -197,6 +199,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) { struct device *dev = alloc_ctx; struct vb2_dc_buf *buf;
int ret = -ENOMEM;
int n_pages;
struct page **pages = NULL;
buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf)
@@ -205,10 +210,41 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL); if (!buf->vaddr) { dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
kfree(buf);return ERR_PTR(-ENOMEM);
goto fail_buf;}
WARN_ON((unsigned long)buf->vaddr & ~PAGE_MASK);
WARN_ON(buf->dma_addr & ~PAGE_MASK);
n_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
if (!pages) {
dev_err(dev, "failed to alloc page table\n");goto fail_dma;}
ret = dma_get_pages(dev, buf->vaddr, buf->dma_addr, pages, n_pages);
if (ret < 0) {
dev_err(dev, "failed to get buffer pages from DMA API\n");goto fail_pages;}
if (ret != n_pages) {
ret = -EFAULT;dev_err(dev, "failed to get all pages from DMA API\n");goto fail_pages;}
buf->sgt_base = vb2_dc_pages_to_sgt(pages, n_pages, 0, 0);
if (IS_ERR(buf->sgt_base)) {
ret = PTR_ERR(buf->sgt_base);dev_err(dev, "failed to prepare sg table\n");goto fail_pages;}
I still (at least partially) share Daniel's opinion regarding dma_get_pages(), As I stated before, I think what we need here would be either
- a DMA API call that maps the memory to the importer device instead of dma_get_pages() + vb2_dc_pages_to_sgt(). The call would take a DMA memory "cookie" (see the "Minutes from V4L2 update call" mail thread) and a pointer to the importer device.
- a DMA API call to retrieve a scatter list suitable to be passed to dma_map_sg(). This would be similar to dma_get_pages() + vb2_dc_pages_to_sgt().
(And we still have to figure out whether the mapping call should be in the exporter or importer, which might have an influence here).
- /* pages are no longer needed */
- kfree(pages);
- buf->dev = dev; buf->size = size;
@@ -219,6 +255,17 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) atomic_inc(&buf->refcount);
return buf;
+fail_pages:
- kfree(pages);
As kfree(NULL) is legal, you can remove the fail_pages label and move the kfree() call just before dma_free_coherent().
+fail_dma:
- dma_free_coherent(dev, size, buf->vaddr, buf->dma_addr);
+fail_buf:
- kfree(buf);
- return ERR_PTR(ret);
}
static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
Hi Tomasz,
Thanks for the patch.
On Tuesday 10 April 2012 15:10:39 Tomasz Stanislawski wrote:
This patch adds support for exporting a dma-contig buffer using DMABUF interface.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/media/video/videobuf2-dma-contig.c | 128 +++++++++++++++++++++++++ 1 files changed, 128 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 0cdcd2b..e1ad47e 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -31,6 +31,7 @@ struct vb2_dc_buf { /* MMAP related */ struct vb2_vmarea_handler handler; atomic_t refcount;
struct dma_buf *dma_buf; struct sg_table *sgt_base;
/* USERPTR related */
@@ -190,6 +191,8 @@ static void vb2_dc_put(void *buf_priv) if (!atomic_dec_and_test(&buf->refcount)) return;
- if (buf->dma_buf)
dma_buf_put(buf->dma_buf);@@ -306,6 +309,130 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) }
/*********************************************/ +/* DMABUF ops for exporters */ +/*********************************************/
+struct vb2_dc_attachment {
- struct sg_table sgt;
- enum dma_data_direction dir;
+};
+static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev,
- struct dma_buf_attachment *dbuf_attach)
+{
- /* nothing to be done */
- return 0;
+}
+static void vb2_dc_dmabuf_ops_detach(struct dma_buf *dbuf,
- struct dma_buf_attachment *db_attach)
+{
- struct vb2_dc_attachment *attach = db_attach->priv;
- struct sg_table *sgt;
- if (!attach)
return;- sgt = &attach->sgt;
- dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->nents, attach->dir);
- sg_free_table(sgt);
- kfree(attach);
- db_attach->priv = NULL;
+}
+static struct sg_table *vb2_dc_dmabuf_ops_map(
- struct dma_buf_attachment *db_attach, enum dma_data_direction dir)
+{
- struct dma_buf *dbuf = db_attach->dmabuf;
- struct vb2_dc_buf *buf = dbuf->priv;
- struct vb2_dc_attachment *attach = db_attach->priv;
- struct sg_table *sgt;
- struct scatterlist *rd, *wr;
- int i, ret;
You can make i an unsigned int :-)
- /* return previously mapped sg table */
- if (attach)
return &attach->sgt;
This effectively keeps the mapping around as long as the attachment exists. We don't try to swap out buffers in V4L2 as is done in DRM at the moment, so it might not be too much of an issue, but the behaviour of the implementation will change if we later decide to map/unmap the buffers in the map/unmap handlers. Do you think that could be a problem ?
- attach = kzalloc(sizeof *attach, GFP_KERNEL);
- if (!attach)
return ERR_PTR(-ENOMEM);
Why don't you allocate the vb2_dc_attachment here instead of vb2_dc_dmabuf_ops_attach() ?
- sgt = &attach->sgt;
- attach->dir = dir;
- /* copying the buf->base_sgt to attachment */
I would add an explanation regarding why you need to copy the SG list. Something like.
"Copy the buf->base_sgt scatter list to the attachment, as we can't map the same scatter list to multiple devices at the same time."
ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
if (ret) {
kfree(attach);return ERR_PTR(-ENOMEM);}
rd = buf->sgt_base->sgl;
wr = sgt->sgl;
for (i = 0; i < sgt->orig_nents; ++i) {
sg_set_page(wr, sg_page(rd), rd->length, rd->offset);rd = sg_next(rd);wr = sg_next(wr);}
/* mapping new sglist to the client */
ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
if (ret <= 0) {
printk(KERN_ERR "failed to map scatterlist\n");sg_free_table(sgt);kfree(attach);return ERR_PTR(-EIO);}
db_attach->priv = attach;
return sgt;
+}
+static void vb2_dc_dmabuf_ops_unmap(struct dma_buf_attachment *db_attach,
- struct sg_table *sgt, enum dma_data_direction dir)
+{
- /* nothing to be done here */
+}
+static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf) +{
- /* drop reference obtained in vb2_dc_get_dmabuf */
- vb2_dc_put(dbuf->priv);
Shouldn't you set vb2_dc_buf::dma_buf to NULL here ? Otherwise the next vb2_dc_get_dmabuf() call will return a DMABUF object that has been freed.
+}
+static struct dma_buf_ops vb2_dc_dmabuf_ops = {
- .attach = vb2_dc_dmabuf_ops_attach,
- .detach = vb2_dc_dmabuf_ops_detach,
- .map_dma_buf = vb2_dc_dmabuf_ops_map,
- .unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
- .release = vb2_dc_dmabuf_ops_release,
+};
+static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv) +{
- struct vb2_dc_buf *buf = buf_priv;
- struct dma_buf *dbuf;
- if (buf->dma_buf)
return buf->dma_buf;
Can't there be a race condition here if the user closes the DMABUF file handle before vb2 core calls dma_buf_fd() ?
- /* dmabuf keeps reference to vb2 buffer */
- atomic_inc(&buf->refcount);
- dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
- if (IS_ERR(dbuf)) {
atomic_dec(&buf->refcount);return NULL;- }
- buf->dma_buf = dbuf;
- return dbuf;
+}
+/*********************************************/ /* callbacks for USERPTR buffers */ /*********************************************/
@@ -615,6 +742,7 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf, const struct vb2_mem_ops vb2_dma_contig_memops = { .alloc = vb2_dc_alloc, .put = vb2_dc_put,
- .get_dmabuf = vb2_dc_get_dmabuf, .cookie = vb2_dc_cookie, .vaddr = vb2_dc_vaddr, .mmap = vb2_dc_mmap,
Hi Laurent, Thank you for your review. Please refer to the comments below.
On 04/17/2012 04:08 PM, Laurent Pinchart wrote:
Hi Tomasz,
Thanks for the patch.
On Tuesday 10 April 2012 15:10:39 Tomasz Stanislawski wrote:
This patch adds support for exporting a dma-contig buffer using DMABUF interface.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
[snip]
+static struct sg_table *vb2_dc_dmabuf_ops_map(
- struct dma_buf_attachment *db_attach, enum dma_data_direction dir)
+{
- struct dma_buf *dbuf = db_attach->dmabuf;
- struct vb2_dc_buf *buf = dbuf->priv;
- struct vb2_dc_attachment *attach = db_attach->priv;
- struct sg_table *sgt;
- struct scatterlist *rd, *wr;
- int i, ret;
You can make i an unsigned int :-)
Right.. splitting declaration may be also a good idea :)
- /* return previously mapped sg table */
- if (attach)
return &attach->sgt;This effectively keeps the mapping around as long as the attachment exists. We don't try to swap out buffers in V4L2 as is done in DRM at the moment, so it might not be too much of an issue, but the behaviour of the implementation will change if we later decide to map/unmap the buffers in the map/unmap handlers. Do you think that could be a problem ?
I don't that it is a problem. If an importer calls dma_map_sg then caching sgt on an exporter side reduces a cost of an allocating and an initialization of sgt.
- attach = kzalloc(sizeof *attach, GFP_KERNEL);
- if (!attach)
return ERR_PTR(-ENOMEM);Why don't you allocate the vb2_dc_attachment here instead of vb2_dc_dmabuf_ops_attach() ?
Good point. The attachment could be allocated at vb2_dc_attachment but all its fields would be uninitialized. I mean an empty sgt and an undefined dma direction. I decided to allocate the attachment in vb2_dc_dmabuf_ops_map because only than all information needed to create a valid attachment object are available.
The other solution might be the allocation at vb2_dc_attachment. The field dir would be set to DMA_NONE. If this filed is equal to DMA_NONE at vb2_dc_dmabuf_ops_map then sgt is allocated and mapped and direction field is updated. If value is not DMA_NONE then the sgt is reused.
Do you think that it is a good idea?
- sgt = &attach->sgt;
- attach->dir = dir;
- /* copying the buf->base_sgt to attachment */
I would add an explanation regarding why you need to copy the SG list. Something like.
"Copy the buf->base_sgt scatter list to the attachment, as we can't map the same scatter list to multiple devices at the same time."
ok
ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
if (ret) {
kfree(attach);return ERR_PTR(-ENOMEM);}
rd = buf->sgt_base->sgl;
wr = sgt->sgl;
for (i = 0; i < sgt->orig_nents; ++i) {
sg_set_page(wr, sg_page(rd), rd->length, rd->offset);rd = sg_next(rd);wr = sg_next(wr);}
/* mapping new sglist to the client */
ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
if (ret <= 0) {
printk(KERN_ERR "failed to map scatterlist\n");sg_free_table(sgt);kfree(attach);return ERR_PTR(-EIO);}
db_attach->priv = attach;
return sgt;
+}
+static void vb2_dc_dmabuf_ops_unmap(struct dma_buf_attachment *db_attach,
- struct sg_table *sgt, enum dma_data_direction dir)
+{
- /* nothing to be done here */
+}
+static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf) +{
- /* drop reference obtained in vb2_dc_get_dmabuf */
- vb2_dc_put(dbuf->priv);
Shouldn't you set vb2_dc_buf::dma_buf to NULL here ? Otherwise the next vb2_dc_get_dmabuf() call will return a DMABUF object that has been freed.
No.
The buffer object is destroyed at vb2_dc_put when reference count drops to 0. It happens could happen after only REQBUF(count=0) or on last close(). The DMABUF object is created only for MMAP buffers. The DMABUF object is based only on results of dma_alloc_coherent and dma_get_pages (or its future equivalent). Therefore the DMABUF object is valid as long as the buffer is valid.
Notice that dmabuf object could be created in vb2_dc_alloc. I moved it to vb2_dc_get_dmabuf to avoid a creation of an object that may not be used.
+}
+static struct dma_buf_ops vb2_dc_dmabuf_ops = {
- .attach = vb2_dc_dmabuf_ops_attach,
- .detach = vb2_dc_dmabuf_ops_detach,
- .map_dma_buf = vb2_dc_dmabuf_ops_map,
- .unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
- .release = vb2_dc_dmabuf_ops_release,
+};
+static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv) +{
- struct vb2_dc_buf *buf = buf_priv;
- struct dma_buf *dbuf;
- if (buf->dma_buf)
return buf->dma_buf;Can't there be a race condition here if the user closes the DMABUF file handle before vb2 core calls dma_buf_fd() ?
The user cannot access the file until it is associated with a file descriptor. How can the user close it? Could you give me a more detailed description of this potential race condition?
- /* dmabuf keeps reference to vb2 buffer */
- atomic_inc(&buf->refcount);
- dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
- if (IS_ERR(dbuf)) {
atomic_dec(&buf->refcount);return NULL;- }
- buf->dma_buf = dbuf;
- return dbuf;
+}
+/*********************************************/ /* callbacks for USERPTR buffers */ /*********************************************/
@@ -615,6 +742,7 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf, const struct vb2_mem_ops vb2_dma_contig_memops = { .alloc = vb2_dc_alloc, .put = vb2_dc_put,
- .get_dmabuf = vb2_dc_get_dmabuf, .cookie = vb2_dc_cookie, .vaddr = vb2_dc_vaddr, .mmap = vb2_dc_mmap,
Regards, Tomasz Stanislawski
Hi Tomasz,
Sorry for the late reply, this one slipped through the cracks.
On Thursday 19 April 2012 12:42:12 Tomasz Stanislawski wrote:
On 04/17/2012 04:08 PM, Laurent Pinchart wrote:
On Tuesday 10 April 2012 15:10:39 Tomasz Stanislawski wrote:
This patch adds support for exporting a dma-contig buffer using DMABUF interface.
Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
[snip]
+static struct sg_table *vb2_dc_dmabuf_ops_map(
- struct dma_buf_attachment *db_attach, enum dma_data_direction dir)
+{
- struct dma_buf *dbuf = db_attach->dmabuf;
- struct vb2_dc_buf *buf = dbuf->priv;
- struct vb2_dc_attachment *attach = db_attach->priv;
- struct sg_table *sgt;
- struct scatterlist *rd, *wr;
- int i, ret;
You can make i an unsigned int :-)
Right.. splitting declaration may be also a good idea :)
- /* return previously mapped sg table */
- if (attach)
return &attach->sgt;This effectively keeps the mapping around as long as the attachment exists. We don't try to swap out buffers in V4L2 as is done in DRM at the moment, so it might not be too much of an issue, but the behaviour of the implementation will change if we later decide to map/unmap the buffers in the map/unmap handlers. Do you think that could be a problem ?
I don't that it is a problem. If an importer calls dma_map_sg then caching sgt on an exporter side reduces a cost of an allocating and an initialization of sgt.
- attach = kzalloc(sizeof *attach, GFP_KERNEL);
- if (!attach)
return ERR_PTR(-ENOMEM);Why don't you allocate the vb2_dc_attachment here instead of vb2_dc_dmabuf_ops_attach() ?
Good point. The attachment could be allocated at vb2_dc_attachment but all its fields would be uninitialized. I mean an empty sgt and an undefined dma direction. I decided to allocate the attachment in vb2_dc_dmabuf_ops_map because only than all information needed to create a valid attachment object are available.
The other solution might be the allocation at vb2_dc_attachment. The field dir would be set to DMA_NONE. If this filed is equal to DMA_NONE at vb2_dc_dmabuf_ops_map then sgt is allocated and mapped and direction field is updated. If value is not DMA_NONE then the sgt is reused.
Do you think that it is a good idea?
I think I would prefer that. It sounds more logical to allocate the attachment in the attach operation handler.
- sgt = &attach->sgt;
- attach->dir = dir;
- /* copying the buf->base_sgt to attachment */
I would add an explanation regarding why you need to copy the SG list. Something like.
"Copy the buf->base_sgt scatter list to the attachment, as we can't map the same scatter list to multiple devices at the same time."
ok
ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
if (ret) {
kfree(attach);return ERR_PTR(-ENOMEM);}
rd = buf->sgt_base->sgl;
wr = sgt->sgl;
for (i = 0; i < sgt->orig_nents; ++i) {
sg_set_page(wr, sg_page(rd), rd->length, rd->offset);rd = sg_next(rd);wr = sg_next(wr);}
/* mapping new sglist to the client */
ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
if (ret <= 0) {
printk(KERN_ERR "failed to map scatterlist\n");sg_free_table(sgt);kfree(attach);return ERR_PTR(-EIO);}
db_attach->priv = attach;
return sgt;
+}
+static void vb2_dc_dmabuf_ops_unmap(struct dma_buf_attachment *db_attach,
- struct sg_table *sgt, enum dma_data_direction dir)
+{
- /* nothing to be done here */
+}
+static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf) +{
- /* drop reference obtained in vb2_dc_get_dmabuf */
- vb2_dc_put(dbuf->priv);
Shouldn't you set vb2_dc_buf::dma_buf to NULL here ? Otherwise the next vb2_dc_get_dmabuf() call will return a DMABUF object that has been freed.
No.
The buffer object is destroyed at vb2_dc_put when reference count drops to 0. It happens could happen after only REQBUF(count=0) or on last close(). The DMABUF object is created only for MMAP buffers. The DMABUF object is based only on results of dma_alloc_coherent and dma_get_pages (or its future equivalent). Therefore the DMABUF object is valid as long as the buffer is valid.
OK.
Notice that dmabuf object could be created in vb2_dc_alloc. I moved it to vb2_dc_get_dmabuf to avoid a creation of an object that may not be used.
+}
+static struct dma_buf_ops vb2_dc_dmabuf_ops = {
- .attach = vb2_dc_dmabuf_ops_attach,
- .detach = vb2_dc_dmabuf_ops_detach,
- .map_dma_buf = vb2_dc_dmabuf_ops_map,
- .unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
- .release = vb2_dc_dmabuf_ops_release,
+};
+static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv) +{
- struct vb2_dc_buf *buf = buf_priv;
- struct dma_buf *dbuf;
- if (buf->dma_buf)
return buf->dma_buf;Can't there be a race condition here if the user closes the DMABUF file handle before vb2 core calls dma_buf_fd() ?
The user cannot access the file until it is associated with a file descriptor. How can the user close it? Could you give me a more detailed description of this potential race condition?
Let's assume the V4L2 buffer has already been exported once. buf->dma_buf is set to a non-NULL value, and the application has an open file handle for the buffer. The application then tries to export the buffer a second time. vb2_dc_get_dmabuf() gets called, checks buf->dma_buf and returns it as it's non-NULL. Right after that, before the vb2 core calls dma_buf_fd() on the struct dma_buf, the application closes the file handle to the exported buffer. The struct dma_buf object gets freed, as the reference count drops to 0. The vb2 core will then try to call dma_buf_fd() on a dma_buf object that has been freed.
- /* dmabuf keeps reference to vb2 buffer */
- atomic_inc(&buf->refcount);
- dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
- if (IS_ERR(dbuf)) {
atomic_dec(&buf->refcount);return NULL;- }
- buf->dma_buf = dbuf;
- return dbuf;
+}
Hi Laurent,
Sorry for the late reply. Thank you very much for noticing the issue.
+static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv) +{
- struct vb2_dc_buf *buf = buf_priv;
- struct dma_buf *dbuf;
- if (buf->dma_buf)
return buf->dma_buf;Can't there be a race condition here if the user closes the DMABUF file handle before vb2 core calls dma_buf_fd() ?
The user cannot access the file until it is associated with a file descriptor. How can the user close it? Could you give me a more detailed description of this potential race condition?
Let's assume the V4L2 buffer has already been exported once. buf->dma_buf is set to a non-NULL value, and the application has an open file handle for the buffer. The application then tries to export the buffer a second time. vb2_dc_get_dmabuf() gets called, checks buf->dma_buf and returns it as it's non-NULL. Right after that, before the vb2 core calls dma_buf_fd() on the struct dma_buf, the application closes the file handle to the exported buffer. The struct dma_buf object gets freed, as the reference count drops to 0.
I am not sure if reference counter drops to 0 in this case. Notice that after first dma_buf_export the dma_buf's refcnt is 1, after first dma_buf_fd it goes to 2. After closing a file handle the refcnt drops back to 1 so the file (and dma_buf) is not released. Therefore IMO there no dangling pointer issue.
However it looks that there is a circular reference between vb2_dc_buf and dma_buf. vb2_dc_buf::dma_buf is pointing to dma_buf with reference taken at dma_buf_export. On the other hand the dma_buf->priv is pointing to vb2_dc_buf with reference taken at atomic_inc(&buf->refcount) in vb2_dc_get_dmabuf.
The circular reference leads into resource leakage. The problem could be fixed by dropping caching of dma_buf pointer. The new dma_buf would be created and exported at every export event. The dma_buf_put would be called in vb2_expbuf just after successful dma_buf_fd.
Do you have any ideas how I could deal with resource leakage/dangling problems without creating a new dma_buf instance at every export event?
Regards, Tomasz Stanislawski
vb2 core will then try to call dma_buf_fd() on a dma_buf object that has been freed.
- /* dmabuf keeps reference to vb2 buffer */
- atomic_inc(&buf->refcount);
- dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
- if (IS_ERR(dbuf)) {
atomic_dec(&buf->refcount);return NULL;- }
- buf->dma_buf = dbuf;
- return dbuf;
+}
linaro-mm-sig@lists.linaro.org
-
Laurent Pinchart -
Tomasz Stanislawski