lkml.org 
[lkml]   [2012]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [Linaro-mm-sig] [PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism
Date
Hi Summit,

Sorry for the late review. I know that this code is now in mainline, but I
still have a couple of comments. I'll send patches if you agree with them.

On Monday 26 December 2011 10:23:15 Sumit Semwal wrote:
> This is the first step in defining a dma buffer sharing mechanism.
>
> A new buffer object dma_buf is added, with operations and API to allow easy
> sharing of this buffer object across devices.
>
> The framework allows:
> - creation of a buffer object, its association with a file pointer, and
> associated allocator-defined operations on that buffer. This operation
> is called the 'export' operation.
> - different devices to 'attach' themselves to this exported buffer object,
> to facilitate backing storage negotiation, using dma_buf_attach() API. -
> the exported buffer object to be shared with the other entity by asking
> for its 'file-descriptor (fd)', and sharing the fd across.
> - a received fd to get the buffer object back, where it can be accessed
> using the associated exporter-defined operations.
> - the exporter and user to share the scatterlist associated with this
> buffer object using map_dma_buf and unmap_dma_buf operations.
>
> Atleast one 'attach()' call is required to be made prior to calling the
> map_dma_buf() operation.
>
> Couple of building blocks in map_dma_buf() are added to ease introduction
> of sync'ing across exporter and users, and late allocation by the exporter.
>
> For this first version, this framework will work with certain conditions:
> - *ONLY* exporter will be allowed to mmap to userspace (outside of this
> framework - mmap is not a buffer object operation),
> - currently, *ONLY* users that do not need CPU access to the buffer are
> allowed.
>
> More details are there in the documentation patch.
>
> This is based on design suggestions from many people at the
> mini-summits[1], most notably from Arnd Bergmann <arnd@arndb.de>, Rob
> Clark <rob@ti.com> and Daniel Vetter <daniel@ffwll.ch>.
>
> The implementation is inspired from proof-of-concept patch-set from
> Tomasz Stanislawski <t.stanislaws@samsung.com>, who demonstrated buffer
> sharing between two v4l2 devices. [2]
>
> [1]: https://wiki.linaro.org/OfficeofCTO/MemoryManagement
> [2]: http://lwn.net/Articles/454389
>
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> Signed-off-by: Sumit Semwal <sumit.semwal@ti.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Dave Airlie <airlied@redhat.com>
> Reviewed-and-Tested-by: Rob Clark <rob.clark@linaro.org>
> ---
> drivers/base/Kconfig | 10 ++
> drivers/base/Makefile | 1 +
> drivers/base/dma-buf.c | 291
> +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/dma-buf.h |
> 176 ++++++++++++++++++++++++++++
> 4 files changed, 478 insertions(+), 0 deletions(-)
> create mode 100644 drivers/base/dma-buf.c
> create mode 100644 include/linux/dma-buf.h
>

[snip]

> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> new file mode 100644
> index 0000000..e38ad24
> --- /dev/null
> +++ b/drivers/base/dma-buf.c
> @@ -0,0 +1,291 @@

[snip]

> +/**
> + * dma_buf_export - Creates a new dma_buf, and associates an anon file
> + * with this buffer, so it can be exported.
> + * Also connect the allocator specific data and ops to the buffer.
> + *
> + * @priv: [in] Attach private data of allocator to this buffer
> + * @ops: [in] Attach allocator-defined dma buf ops to the new buffer.
> + * @size: [in] Size of the buffer
> + * @flags: [in] mode flags for the file.
> + *
> + * Returns, on success, a newly created dma_buf object, which wraps the
> + * supplied private data and operations for dma_buf_ops. On either missing
> + * ops, or error in allocating struct dma_buf, will return negative error.
> + *
> + */
> +struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,
> + size_t size, int flags)
> +{
> + struct dma_buf *dmabuf;
> + struct file *file;
> +
> + if (WARN_ON(!priv || !ops
> + || !ops->map_dma_buf
> + || !ops->unmap_dma_buf
> + || !ops->release)) {
> + return ERR_PTR(-EINVAL);
> + }
> +
> + dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
> + if (dmabuf == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + dmabuf->priv = priv;
> + dmabuf->ops = ops;

dmabuf->ops will never but NULL, but (see below)

> + dmabuf->size = size;
> +
> + file = anon_inode_getfile("dmabuf", &dma_buf_fops, dmabuf, flags);
> +
> + dmabuf->file = file;
> +
> + mutex_init(&dmabuf->lock);
> + INIT_LIST_HEAD(&dmabuf->attachments);
> +
> + return dmabuf;
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_export);

[snip]

> +/**
> + * dma_buf_attach - Add the device to dma_buf's attachments list;
> optionally, + * calls attach() of dma_buf_ops to allow device-specific
> attach functionality + * @dmabuf: [in] buffer to attach device to.
> + * @dev: [in] device to be attached.
> + *
> + * Returns struct dma_buf_attachment * for this attachment; may return
> negative + * error codes.
> + *
> + */
> +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> + struct device *dev)
> +{
> + struct dma_buf_attachment *attach;
> + int ret;
> +
> + if (WARN_ON(!dmabuf || !dev || !dmabuf->ops))

you still check dmabuf->ops here, as well as in several places below.
Shouldn't these checks be removed ?

> + return ERR_PTR(-EINVAL);
> +
> + attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
> + if (attach == NULL)
> + goto err_alloc;

What about returning ERR_PTR(-ENOMEM) directly here ?

> +
> + mutex_lock(&dmabuf->lock);
> +
> + attach->dev = dev;
> + attach->dmabuf = dmabuf;

These two lines can be moved before mutex_lock().

> + if (dmabuf->ops->attach) {
> + ret = dmabuf->ops->attach(dmabuf, dev, attach);
> + if (ret)
> + goto err_attach;
> + }
> + list_add(&attach->node, &dmabuf->attachments);
> +
> + mutex_unlock(&dmabuf->lock);
> + return attach;
> +
> +err_alloc:
> + return ERR_PTR(-ENOMEM);
> +err_attach:
> + kfree(attach);
> + mutex_unlock(&dmabuf->lock);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_attach);

--
Regards,

Laurent Pinchart


\
 
 \ /
  Last update: 2012-01-20 14:27    [W:0.752 / U:0.100 seconds]
©2003-2014 Jasper Spaans. Advertise on this site