Re: [Linaro-mm-sig] [RFC v2] dma-buf: Add buffer sharing framework

On Tue, Sep 13, 2011 at 07:59:09PM +0530, Sumit Semwal wrote:

This is the first step in defining a buffer sharing framework. A new dma_buf buffer object is added, with hooks to allow for easy sharing of this buffer object across devices.

The framework allows:

• a new buffer-object to be created with fixed size.
• different devices to 'attach' themselves to this buffer, to facilitate backing storage negotiation, using dma_buf_attach() API.
• association of a file pointer with each user-buffer and associated allocator-defined operations on that buffer. This operation is called the 'export' operation.
• this 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 importer to share the scatterlist using get_scatterlist and put_scatterlist operations.

Atleast one 'attach()' call is required to be made prior to calling the buffer_map() callback.

Couple of building blocks in get_scatterlist() are added to ease introduction of sync'ing across exporter and importers, and late allocation by the exporter.

Optionally, mmap() file operation is provided for the associated 'fd', as wrapper over the allocator defined mmap()[optional], to be used by devices that might need one.

The idea was first mooted at the Linaro memory management mini-summit in Budapest in May 2011, as part of multiple things needed for a 'unified memory management framework'. It took a more concrete shape at Linaro memory-management mini-summit in Cambridge, Aug 2011.

This is based on design suggestions from many people at both the mini-summits, 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.

---

v1: initial RFC. v2:

• added attach() / detach() dma_buf_ops, and dma_buf_attach(),dma_buf_detach().
• added handling of list of attachment in the dma_buf central API itself.
• corrected copyright information.

Signed-off-by: Sumit Semwal sumit.semwal@linaro.org Signed-off-by: Sumit Semwal sumit.semwal@ti.com

Hi Summit,

Sorry for the late review, I've been a bit busy. Some nitpicks below, but most of it has already been brought up by Rob and Subash.

---

drivers/base/Kconfig | 10 ++ drivers/base/Makefile | 1 + drivers/base/dma-buf.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/dma-buf.h | 179 +++++++++++++++++++++++++++++++++++++ 4 files changed, 414 insertions(+), 0 deletions(-) create mode 100644 drivers/base/dma-buf.c create mode 100644 include/linux/dma-buf.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 21cf46f..07d8095 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -174,4 +174,14 @@ config SYS_HYPERVISOR source "drivers/base/regmap/Kconfig" +config DMA_SHARED_BUFFER

• bool "Buffer framework to be shared between drivers"
• default n
• depends on ANON_INODES
• help

•  This option enables the framework for buffer-sharing between
  

•  multiple drivers. A buffer is associated with a file using driver
  

•  APIs extension; the file's descriptor can then be passed on to other
  

•  driver.
  

endmenu diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 99a375a..d0df046 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_DEVTMPFS) += devtmpfs.o obj-y += power/ obj-$(CONFIG_HAS_DMA) += dma-mapping.o obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o +obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf.o obj-$(CONFIG_ISA) += isa.o obj-$(CONFIG_FW_LOADER) += firmware_class.o obj-$(CONFIG_NUMA) += node.o diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c new file mode 100644 index 0000000..8936da7 --- /dev/null +++ b/drivers/base/dma-buf.c @@ -0,0 +1,224 @@ +/*

• • Framework for buffer objects that can be shared across devices/subsystems.
• • Copyright(C) 2011 Linaro Limited. All rights reserved.
• • Author: Sumit Semwal sumit.semwal@ti.com
• • Many thanks to linaro-mm-sig list, and specially
• • Arnd Bergmann arnd@arndb.de, Rob Clark rob@ti.com and
• • Daniel Vetter daniel@ffwll.ch for their support in creation and
• • refining of this idea.
• • This program is free software; you can redistribute it and/or modify it
• • under the terms of the GNU General Public License version 2 as published by
• • the Free Software Foundation.
• • This program is distributed in the hope that it will be useful, but WITHOUT
• • ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
• • FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
• • more details.
• • You should have received a copy of the GNU General Public License along with
• • this program. If not, see http://www.gnu.org/licenses/.
• */

+#include <linux/fs.h> +#include <linux/slab.h> +#include <linux/dma-buf.h> +#include <linux/anon_inodes.h>

+static inline int is_dma_buf_file(struct file *);

+static int dma_buf_mmap(struct file *file, struct vm_area_struct *vma) +{

• struct dma_buf *dmabuf;
• if (!is_dma_buf_file(file))

• return -EINVAL;
  

• dmabuf = file->private_data;
• if (!dmabuf->ops->mmap)

• return -EINVAL;
  

• return dmabuf->ops->mmap(dmabuf, vma);

+}

+static int dma_buf_release(struct inode *inode, struct file *file) +{

• struct dma_buf *dmabuf;
• if (!is_dma_buf_file(file))

• return -EINVAL;
  

• dmabuf = file->private_data;
• dmabuf->ops->release(dmabuf);
• kfree(dmabuf);
• return 0;

+}

+static const struct file_operations dma_buf_fops = {

• .mmap = dma_buf_mmap,
• .release = dma_buf_release,

+};

+/*

• • is_dma_buf_file - Check if struct file* is associated with dma_buf
• */

+static inline int is_dma_buf_file(struct file *file) +{

• return file->f_op == &dma_buf_fops;

+}

+/**

• • 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.
• • @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 failure to
• • allocate the dma_buf object, it can return NULL.
• */

+struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops,

• 		int flags)
  

+{

• struct dma_buf *dmabuf;
• struct file *file;
• BUG_ON(!priv || !ops);
• dmabuf = kzalloc(sizeof(struct dma_buf), GFP_KERNEL);
• if (dmabuf == NULL)

• return dmabuf;
  

• dmabuf->priv = priv;
• dmabuf->ops = ops;
• file = anon_inode_getfile("dmabuf", &dma_buf_fops, dmabuf, flags);
• dmabuf->file = file;
• INIT_LIST_HEAD(&dmabuf->attachments);
• return dmabuf;

+} +EXPORT_SYMBOL(dma_buf_export);

+/**

• • dma_buf_fd - returns a file descriptor for the given dma_buf
• • @dmabuf: [in] pointer to dma_buf for which fd is required.
• • On success, returns an associated 'fd'. Else, returns error.
• */

+int dma_buf_fd(struct dma_buf *dmabuf) +{

• int error, fd;
• if (!dmabuf->file)

• return -EINVAL;
  

• error = get_unused_fd_flags(0);
• if (error < 0)

• return error;
  

• fd = error;
• fd_install(fd, dmabuf->file);
• return fd;

+} +EXPORT_SYMBOL(dma_buf_fd);

+/**

• • dma_buf_get - returns the dma_buf structure related to an fd
• • @fd: [in] fd associated with the dma_buf to be returned
• • On success, returns the dma_buf structure associated with an fd; uses
• • file's refcounting done by fget to increase refcount. returns ERR_PTR
• • otherwise.
• */

+struct dma_buf *dma_buf_get(int fd) +{

• struct file *file;
• file = fget(fd);
• if (!file)

• return ERR_PTR(-EBADF);
  

• if (!is_dma_buf_file(file)) {

• fput(file);
  

• return ERR_PTR(-EINVAL);
  

• }
• return file->private_data;

+} +EXPORT_SYMBOL(dma_buf_get);

+/**

• • dma_buf_put - decreases refcount of the buffer
• • @dmabuf: [in] buffer to reduce refcount of
• • Uses file's refcounting done implicitly by fput()
• */

+void dma_buf_put(struct dma_buf *dmabuf) +{

• BUG_ON(!dmabuf->file);
• fput(dmabuf->file);
• return;

+} +EXPORT_SYMBOL(dma_buf_put);

+/**

• • dma_buf_attach - Add the device to dma_buf's attachments list; calls the
• • 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 NULL.
• */

+struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• 				struct device *dev)
  

+{

• struct dma_buf_attachment *attach;
• BUG_ON(!dmabuf || !dev);
• attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
• if (attach == NULL)

• return attach;
  

• dmabuf->ops->attach(dmabuf, dev, attach);
• list_add(&attach->node, &dmabuf->attachments);
• return attach;

+} +EXPORT_SYMBOL(dma_buf_attach);

I agree with Subash that we need a look here to protect the list manipulation. I think for simplicity just add a mutex and wrap it around both the list manipulation and the call to attach/detach.

Also, I think the common code shoudl set dma_buf_attachment->dev and ops->attach/detach should be optional (for simpler cases) and attach needs to be able to return an error (which should then be converted into an ERR_PTR by this function) in case the allocation is already fixed and the device can't access it at its current position.

+/**

• • dma_buf_detach - Remove the given attachment from dmabuf's attachments list;
• • calls detach() of dma_buf_ops for device-specific detach
• • @dmabuf: [in] buffer to detach from.
• • @attach: [in] attachment to be detached; is free'd after this call.
• */

+void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) +{

• BUG_ON(!dmabuf || !attach);
• list_del(&attach->node);
• dmabuf->ops->detach(dmabuf, attach);
• kfree(attach);
• return;

+} +EXPORT_SYMBOL(dma_buf_detach); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h new file mode 100644 index 0000000..2894b45 --- /dev/null +++ b/include/linux/dma-buf.h @@ -0,0 +1,179 @@ +/*

• • Header file for dma buffer sharing framework.
• • Copyright(C) 2011 Linaro Limited. All rights reserved.
• • Author: Sumit Semwal sumit.semwal@ti.com
• • Many thanks to linaro-mm-sig list, and specially
• • Arnd Bergmann arnd@arndb.de, Rob Clark rob@ti.com and
• • Daniel Vetter daniel@ffwll.ch for their support in creation and
• • refining of this idea.
• • This program is free software; you can redistribute it and/or modify it
• • under the terms of the GNU General Public License version 2 as published by
• • the Free Software Foundation.
• • This program is distributed in the hope that it will be useful, but WITHOUT
• • ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
• • FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
• • more details.
• • You should have received a copy of the GNU General Public License along with
• • this program. If not, see http://www.gnu.org/licenses/.
• */

+#ifndef __DMA_BUF_H__ +#define __DMA_BUF_H__

+#include <linux/file.h> +#include <linux/err.h> +#include <linux/device.h> +#include <linux/scatterlist.h> +#include <linux/list.h>

+struct dma_buf;

+/**

• • enum dma_buf_optype - Used to denote the operation for which
• • get_scatterlist() is called. This will help in implementing a wait(op)
• • for sync'ing.
• • @DMA_BUF_OP_READ: read operation will be done on this scatterlist
• • @DMA_BUF_OP_WRITE: write operation will be done on this scatterlist
• */

+enum dma_buf_optype {

• DMA_BUF_OP_READ = (1 << 0),
• DMA_BUF_OP_WRITE = (1 << 1),
• DMA_BUF_OP_MAX,

+};

Can we just use DMA_TO_DEVICE, DMA_FROM_DEVICE, DMA_BIDIRECTIONAL from the dma api? I think that would be much better for 2 reasons: - consistency between dma api and dma_buf api - on sane machines (x86) the dma_buf allocator could just grab an sg_list and directly map the get_scatterlist to the dma_map_sg call.

+/**

• • enum dma_buf_attr_flags - Defines the attributes for this buffer. This
• • can allow 'late backing of buffer' based on first get_scatterlist() call.
• • @DMA_BUF_ATTR_CONTIG: Contiguous buffer
• • @DMA_BUF_ATTR_DISCONTIG: Discontiguous buffer
• • @DMA_BUF_ATTR_CUSTOM: Platform specific buffer; should evolve into some
• • attributes that define buffers on given specific platform
• */

+enum dma_buf_attr_flags {

• DMA_BUF_ATTR_CONTIG,
• DMA_BUF_ATTR_DISCONTIG,
• DMA_BUF_ATTR_CUSTOM,
• DMA_BUF_ATTR_MAX,

+};

I don't like this for essentially the same reasons as Rob. Also, if we really need this I think it should be passed in at attach time, not at get_scatterlist (where it might be too late). Also, for the same reasons as with the direction flags above, I want this to be consistent with the solution the normal dma mapping api provides.

Furthermore adding flags for extendability always souns great until you actually want to extend your api: Usually you then notice gazillions of ad-hoc extension and the associated mess, making extensions harder than easier. So for dma_buf, first try I'd aim for the simplest possible solution that can get the job done. No strange flags, please ;-)

+/**

• • struct dma_buf_attachment - holds device-buffer attachment data
• • @node: list_head to allow manipulation of list of dma_buf_attachment.
• • @dev: device attached to the buffer.
• • @priv: exporter-specific attachment data.
• */

+struct dma_buf_attachment {

• struct list_head node;
• struct device *dev;
• void *priv;

+};

+/**

• • struct dma_buf_ops - operations possible on struct dma_buf
• • @create: creates a struct dma_buf of a fixed size. Actual allocation

• •    does not happen here.
    

• • @attach: allows different devices to 'attach' themselves to the given

• •    buffer. It might return -EBUSY to signal that backing storage
    

• •    is already allocated and incompatible with the requirements
    

• •    of requesting device.
    

• • @detach: detach a given device from this buffer.
• • @get_scatterlist: returns list of scatter pages allocated, increases

• •      usecount of the buffer. Requires atleast one attach to be
    

• •      called before.
    

I think the important bit to spec here is that the returned sg list is already mapped into the _device_ address space. This would support the use case of a special remmapper (like ompas TILER) that a group of devices could access to e.g. see tiled buffers as linear buffers. That would obviously require some driver/platform specific magic, but I think we should design the interface to allow this.

Also, I think this will give us a cleaner interface, because about the only thing the driver can do with that sg list is to map it. So better fold that into the get_scatterlist.

I'd still like to see a name for the get/put_scatterlist that matches more the dma api (i.e. a wraper dma_buf_map or something), but this is a bit bikeshedding, so I don't care much.

• • @put_scatterlist: decreases usecount of buffer, might deallocate scatter

• •      pages.
    

• • @mmap: memory map this buffer - optional.
• • @release: release this buffer; to be called after the last dma_buf_put.
• • @sync_sg_for_cpu: sync the sg list for cpu.
• • @sync_sg_for_device: synch the sg list for device.
• */

+struct dma_buf_ops {

• void (*attach)(struct dma_buf *, struct device *,

• 	struct dma_buf_attachment *);
  

• void (*detach)(struct dma_buf *, struct dma_buf_attachment *);
• struct scatterlist * (*get_scatterlist)(struct dma_buf *,

• 				struct dma_buf_attachment *,
  

• 				enum dma_buf_optype,
  

• 				enum dma_buf_attr_flags);
  

• void (*put_scatterlist)(struct dma_buf *, struct dma_buf_attachment *,

• 				struct scatterlist *);
  

• /* allow mmap optionally for devices that need it */
• int (*mmap)(struct dma_buf *, struct vm_area_struct *);
• /* after final dma_buf_put() */
• void (*release)(struct dma_buf *);
• /* allow allocator to take care of cache ops */
• void (*sync_sg_for_cpu) (struct dma_buf *, struct device *);
• void (*sync_sg_for_device)(struct dma_buf *, struct device *);

+};

+/**

• • struct dma_buf - shared buffer object
• • @file: file pointer used for sharing buffers across, and for refcounting.
• • @attachments: list of dma_buf_attachment that denotes all devices attached.
• • @ops: dma_buf_ops associated with this buffer object
• • @priv: user specific private data
• */

+struct dma_buf {

• size_t size;
• struct file *file;
• struct list_head attachments;
• struct dma_buf_ops *ops;
• void *priv;

+};

+#ifdef CONFIG_DMA_SHARED_BUFFER +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• 					struct device *dev);
  

+void dma_buf_detach(struct dma_buf *dmabuf,

• 		struct dma_buf_attachment *dmabuf_attach);
  

+struct dma_buf *dma_buf_export(void *priv, struct dma_buf_ops *ops, int flags); +int dma_buf_fd(struct dma_buf *dmabuf); +struct dma_buf *dma_buf_get(int fd); +void dma_buf_put(struct dma_buf *dmabuf);

+#else

+static inline struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,

• 					struct device *dev)
  

+{

• return ERR_PTR(-ENODEV);

+}

+static inline void dma_buf_detach(struct dma_buf *dmabuf,

• 		  struct dma_buf_attachment *dmabuf_attach)
  

+{

• return;

+}

+static inline struct dma_buf *dma_buf_export(void *priv,

• 				struct dma_buf_ops *ops,
  

• 				int flags)
  

+{

• return ERR_PTR(-ENODEV);

+}

+static inline int dma_buf_fd(struct dma_buf *dmabuf) +{

• return -ENODEV;

+}

+static inline struct dma_buf *dma_buf_get(int fd) +{

• return ERR_PTR(-ENODEV);

+}

+static inline void dma_buf_put(struct dma_buf *dmabuf) +{

• return;

+} +#endif /* CONFIG_DMA_SHARED_BUFFER */

+#endif /* __DMA_BUF_H__ */

1.7.4.1