Re: [Linaro-mm-sig] [PATCH 1/3] dma-buf: Introduce dma buffer sharing mechanism

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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