[RFC v3 0/2] Introduce DMA buffer sharing mechanism

[RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Sumit Semwal]

Hello Everyone,

This is RFC v3 for DMA buffer sharing mechanism - changes from v2 are in the
changelog below.

Various subsystems - V4L2, GPU-accessors, DRI to name a few - have felt the 
need to have a common mechanism to share memory buffers across different
devices - ARM, video hardware, GPU.

This need comes forth from a variety of use cases including cameras, image 
processing, video recorders, sound processing, DMA engines, GPU and display
buffers, and others.

This RFC is an attempt to define such a buffer sharing mechanism- it is the
result of discussions from a couple of memory-management mini-summits held by
Linaro to understand and address common needs around memory management. [1]

A new dma_buf buffer object is added, with operations and API to allow 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 user to share the scatterlist using map_dma_buf and
   unmap_dma_buf operations.

Documentation present in the patch-set gives more details.

This is based on design suggestions from many people at 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. [2]

References:
[1]: https://wiki.linaro.org/OfficeofCTO/MemoryManagement
[2]: http://lwn.net/Articles/454389

Patchset based on top of 3.2-rc3, the current version can be found at

http://git.linaro.org/gitweb?p=people/sumitsemwal/linux-3.x.git
Branch: dma-buf-upstr-v2

Earlier versions:
v2 at: https://lkml.org/lkml/2011/12/2/53
v1 at: https://lkml.org/lkml/2011/10/11/92

Best regards,
~Sumit Semwal

History:

v3:
- Review comments incorporated:
   - from Konrad Rzeszutek Wilk [https://lkml.org/lkml/2011/12/3/45]
     - replaced BUG_ON with WARN_ON - various places
     - added some error-checks
     - replaced EXPORT_SYMBOL with EXPORT_SYMBOL_GPL
     - some cosmetic / documentation comments

   - from Arnd Bergmann, Daniel Vetter, Rob Clark
      [https://lkml.org/lkml/2011/12/5/321]
     - removed mmap() fop and dma_buf_op, also the sg_sync* operations, and
        documented that mmap is not allowed for exported buffer
     - updated documentation to clearly state when migration is allowed
     - changed kconfig
     - some error code checks

   - from Rob Clark [https://lkml.org/lkml/2011/12/5/572]
     - update documentation to allow map_dma_buf to return -EINTR

v2:
- Review comments incorporated:
   - from Tomasz Stanislawski [https://lkml.org/lkml/2011/10/14/136]
     - kzalloc moved out of critical section
     - corrected some in-code comments

   - from Dave Airlie [https://lkml.org/lkml/2011/11/25/123]

   - from Daniel Vetter and Rob Clark [https://lkml.org/lkml/2011/11/26/53]
     - use struct sg_table in place of struct scatterlist
     - rename {get,put}_scatterlist to {map,unmap}_dma_buf
     - add new wrapper APIs dma_buf_{map,unmap}_attachment for ease of users
     
- documentation updates as per review comments from Randy Dunlap
     [https://lkml.org/lkml/2011/10/12/439]

v1: original


Sumit Semwal (2):
  dma-buf: Introduce dma buffer sharing mechanism
  dma-buf: Documentation for buffer sharing framework

 Documentation/dma-buf-sharing.txt |  222 ++++++++++++++++++++++++++++
 drivers/base/Kconfig              |   10 ++
 drivers/base/Makefile             |    1 +
 drivers/base/dma-buf.c            |  289 +++++++++++++++++++++++++++++++++++++
 include/linux/dma-buf.h           |  172 ++++++++++++++++++++++
 5 files changed, 694 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/dma-buf-sharing.txt
 create mode 100644 drivers/base/dma-buf.c
 create mode 100644 include/linux/dma-buf.h

-- 
1.7.4.1

[RFC v3 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Konrad Rzeszutek Wilk]

On Mon, Dec 19, 2011 at 02:03:30PM +0530, 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:
> - different devices to 'attach' themselves to this buffer, to facilitate
>   backing storage negotiation, using dma_buf_attach() API.

Any thoughts of adding facility to track them? So you can see who is using what?

> - association of a file pointer with each user-buffer and associated
>    allocator-defined operations on that buffer. This operation is called the
>    'export' operation.

 'create'? or 'alloc' ?

export implies an import somwhere and I don't think that is the case here.

> - 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 user to share the scatterlist 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.

for the whole memory region or just for the device itself?

> 
> 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.
> 
> 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>
> ---
>  drivers/base/Kconfig    |   10 ++
>  drivers/base/Makefile   |    1 +
>  drivers/base/dma-buf.c  |  289 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-buf.h |  172 ++++++++++++++++++++++++++++
>  4 files changed, 472 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..8a0e87f 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
> +	select 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..e920709
> --- /dev/null
> +++ b/drivers/base/dma-buf.c
> @@ -0,0 +1,289 @@
> +/*
> + * 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>
> +#include <linux/export.h>
> +
> +static inline int is_dma_buf_file(struct file *);
> +
> +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 = {
> +	.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;
> +}
> +
> +/**

Wrong kerneldoc.

> + * 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->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);
> +
> +
> +/**
> + * 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 || !dmabuf->file)
> +		return -EINVAL;
> +
> +	error = get_unused_fd();
> +	if (error < 0)
> +		return error;
> +	fd = error;
> +
> +	fd_install(fd, dmabuf->file);
> +
> +	return fd;
> +}
> +EXPORT_SYMBOL_GPL(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_GPL(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)
> +{
> +	if (WARN_ON(!dmabuf || !dmabuf->file))
> +		return;
> +
> +	fput(dmabuf->file);
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_put);
> +
> +/**
> + * 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))
> +		return ERR_PTR(-EINVAL);
> +
> +	attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
> +	if (attach == NULL)
> +		goto err_alloc;
> +
> +	mutex_lock(&dmabuf->lock);
> +
> +	attach->dev = dev;
> +	attach->dmabuf = dmabuf;
> +	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);
> +
> +/**
> + * dma_buf_detach - Remove the given attachment from dmabuf's attachments list;
> + * optionally 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)
> +{
> +	if (WARN_ON(!dmabuf || !attach || !dmabuf->ops))
> +		return;
> +
> +	mutex_lock(&dmabuf->lock);
> +	list_del(&attach->node);
> +	if (dmabuf->ops->detach)
> +		dmabuf->ops->detach(dmabuf, attach);
> +
> +	mutex_unlock(&dmabuf->lock);
> +	kfree(attach);
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_detach);
> +
> +/**
> + * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
> + * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
> + * dma_buf_ops.
> + * @attach:	[in]	attachment whose scatterlist is to be returned
> + * @direction:	[in]	direction of DMA transfer
> + *
> + * Returns sg_table containing the scatterlist to be returned; may return NULL
> + * or ERR_PTR.
> + *
> + */
> +struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
> +					enum dma_data_direction direction)
> +{
> +	struct sg_table *sg_table = ERR_PTR(-EINVAL);
> +
> +	if (WARN_ON(!attach || !attach->dmabuf || !attach->dmabuf->ops))
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&attach->dmabuf->lock);
> +	if (attach->dmabuf->ops->map_dma_buf)
> +		sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
> +	mutex_unlock(&attach->dmabuf->lock);
> +
> +	return sg_table;
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
> +
> +/**
> + * dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
> + * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
> + * dma_buf_ops.
> + * @attach:	[in]	attachment to unmap buffer from
> + * @sg_table:	[in]	scatterlist info of the buffer to unmap
> + *
> + */
> +void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> +				struct sg_table *sg_table)
> +{
> +	if (WARN_ON(!attach || !attach->dmabuf || !sg_table
> +			    || !attach->dmabuf->ops))
> +		return;
> +
> +	mutex_lock(&attach->dmabuf->lock);
> +	if (attach->dmabuf->ops->unmap_dma_buf)
> +		attach->dmabuf->ops->unmap_dma_buf(attach, sg_table);
> +	mutex_unlock(&attach->dmabuf->lock);
> +
> +}
> +EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> new file mode 100644
> index 0000000..3e3ecf3
> --- /dev/null
> +++ b/include/linux/dma-buf.h
> @@ -0,0 +1,172 @@
> +/*
> + * 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>
> +#include <linux/dma-mapping.h>
> +
> +struct dma_buf;
> +
> +/**
> + * struct dma_buf_attachment - holds device-buffer attachment data

OK, but what is the purpose of it?

> + * @dmabuf: buffer for this attachment.
> + * @dev: device attached to the buffer.
                                ^^^ this
> + * @node: list_head to allow manipulation of list of dma_buf_attachment.

Just say: "list of dma_buf_attachment"'

> + * @priv: exporter-specific attachment data.

That "exporter-specific.." brings to my mind custom decleration forms. But maybe that is me.

> + */
> +struct dma_buf_attachment {
> +	struct dma_buf *dmabuf;
> +	struct device *dev;
> +	struct list_head node;
> +	void *priv;
> +};

Why don't you move the decleration of this below 'struct dma_buf'?
It would easier than to read this structure..

> +
> +/**
> + * struct dma_buf_ops - operations possible on struct dma_buf
> + * @attach: allows different devices to 'attach' themselves to the given

register?
> + *	    buffer. It might return -EBUSY to signal that backing storage
> + *	    is already allocated and incompatible with the requirements

Wait.. allocated or attached?

> + *	    of requesting device. [optional]

What is optional? The return value? Or the 'attach' call? If the later , say
that in the first paragraph.


> + * @detach: detach a given device from this buffer. [optional]
> + * @map_dma_buf: returns list of scatter pages allocated, increases usecount
> + *		 of the buffer. Requires atleast one attach to be called
> + *		 before. Returned sg list should already be mapped into
> + *		 _device_ address space. This call may sleep. May also return

Ok, there is some __might_sleep macro you should put on the function.

> + *		 -EINTR.

Ok. What is the return code if attach has _not_ been called?

> + * @unmap_dma_buf: decreases usecount of buffer, might deallocate scatter
> + *		   pages.
> + * @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.

Not seeing those two.
> + */
> +struct dma_buf_ops {
> +	int (*attach)(struct dma_buf *, struct device *,
> +			struct dma_buf_attachment *);
> +
> +	void (*detach)(struct dma_buf *, struct dma_buf_attachment *);
> +
> +	/* For {map,unmap}_dma_buf below, any specific buffer attributes
> +	 * required should get added to device_dma_parameters accessible
> +	 * via dev->dma_params.
> +	 */
> +	struct sg_table * (*map_dma_buf)(struct dma_buf_attachment *,
> +						enum dma_data_direction);
> +	void (*unmap_dma_buf)(struct dma_buf_attachment *,
> +						struct sg_table *);
> +	/* TODO: Add try_map_dma_buf version, to return immed with -EBUSY

Ewww. Why? Why not just just the 'map_dma_buf' and return that?

> +	 * if the call would block.
> +	 */
> +
> +	/* after final dma_buf_put() */
> +	void (*release)(struct dma_buf *);
> +
> +};
> +
> +/**
> + * struct dma_buf - shared buffer object

Missing the 'size'.

> + * @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


Can you elaborate on this? Is this the "exporter" using this? Or is
it for the "user" using it? If so, why provide it? Wouldn't the 
user of this have something like this:

struct my_dma_bufs {
	struct dma_buf[20];
	void *priv;
}

Anyhow?

> + */
> +struct dma_buf {
> +	size_t size;
> +	struct file *file;
> +	struct list_head attachments;
> +	const struct dma_buf_ops *ops;
> +	/* mutex to serialize list manipulation and other ops */
> +	struct mutex lock;
> +	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,
> +			size_t size, 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);
> +
> +struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
> +					enum dma_data_direction);
> +void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *);
> +#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,
> +						size_t size, 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;
> +}
> +
> +static inline struct sg_table *dma_buf_map_attachment(
> +	struct dma_buf_attachment *attach, enum dma_data_direction write)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static inline void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> +						struct sg_table *sg)
> +{
> +	return;
> +}
> +
> +#endif /* CONFIG_DMA_SHARED_BUFFER */
> +
> +#endif /* __DMA_BUF_H__ */
> -- 
> 1.7.4.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v3 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Rob Clark]

On Tue, Dec 20, 2011 at 10:36 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Dec 19, 2011 at 02:03:30PM +0530, 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:
>> - different devices to 'attach' themselves to this buffer, to facilitate
>> ? backing storage negotiation, using dma_buf_attach() API.
>
> Any thoughts of adding facility to track them? So you can see who is using what?
>
>> - association of a file pointer with each user-buffer and associated
>> ? ?allocator-defined operations on that buffer. This operation is called the
>> ? ?'export' operation.
>
> ?'create'? or 'alloc' ?
>
> export implies an import somwhere and I don't think that is the case here.

Well, the import is the attach/map stuff.  The userspace facing export
ioctl would call dma_buf_export()..  the userspace facing import ioctl
of the importing device would call dma_buf_attach/map().

Perhaps the language should be a "create dma buf" API used by the
exporting driver.  I guess the dmabuf is being created, the backing
memory/pages are being exported.

>> - 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 user to share the scatterlist 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.
>
> for the whole memory region or just for the device itself?

Attach should be called per buffer.  It is basically registering
intent of the importing device to user the buffer 1 or more times.
Detach is letting the exporter know that the importer won't be using
the buffer again (at least for a while).

For example:
 attach()
   map()
     // buffer is backed and pinned
     do dma to/from buffer
   unmap()
   // exporter could mark the buffer evictable here
   map()
     // buffer is pinned
     do dma to/from buffer
   unmap()
   ...
 detach()

Also, in case of multiple importers, the exporter could also wait to
allocate backing pages until first map() in case one of the importers
has stricter memory requirements (physically contiguous, certain
memory range, etc) than the others.  Some exporters, like v4l, might
have some restrictions about the buffer being backed/pinned earlier,
for those attach() might be basically a no-op.  But the idea is to
give exporters that are a bit more dynamic w/ memory management some
flexibility.

BR,
-R


>>
>> 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.
>>
>> 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>
>> ---
>> ?drivers/base/Kconfig ? ?| ? 10 ++
>> ?drivers/base/Makefile ? | ? ?1 +
>> ?drivers/base/dma-buf.c ?| ?289 +++++++++++++++++++++++++++++++++++++++++++++++
>> ?include/linux/dma-buf.h | ?172 ++++++++++++++++++++++++++++
>> ?4 files changed, 472 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..8a0e87f 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
>> + ? ? select 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..e920709
>> --- /dev/null
>> +++ b/drivers/base/dma-buf.c
>> @@ -0,0 +1,289 @@
>> +/*
>> + * 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>
>> +#include <linux/export.h>
>> +
>> +static inline int is_dma_buf_file(struct file *);
>> +
>> +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 = {
>> + ? ? .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;
>> +}
>> +
>> +/**
>
> Wrong kerneldoc.
>
>> + * 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->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);
>> +
>> +
>> +/**
>> + * 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 || !dmabuf->file)
>> + ? ? ? ? ? ? return -EINVAL;
>> +
>> + ? ? error = get_unused_fd();
>> + ? ? if (error < 0)
>> + ? ? ? ? ? ? return error;
>> + ? ? fd = error;
>> +
>> + ? ? fd_install(fd, dmabuf->file);
>> +
>> + ? ? return fd;
>> +}
>> +EXPORT_SYMBOL_GPL(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_GPL(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)
>> +{
>> + ? ? if (WARN_ON(!dmabuf || !dmabuf->file))
>> + ? ? ? ? ? ? return;
>> +
>> + ? ? fput(dmabuf->file);
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_put);
>> +
>> +/**
>> + * 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))
>> + ? ? ? ? ? ? return ERR_PTR(-EINVAL);
>> +
>> + ? ? attach = kzalloc(sizeof(struct dma_buf_attachment), GFP_KERNEL);
>> + ? ? if (attach == NULL)
>> + ? ? ? ? ? ? goto err_alloc;
>> +
>> + ? ? mutex_lock(&dmabuf->lock);
>> +
>> + ? ? attach->dev = dev;
>> + ? ? attach->dmabuf = dmabuf;
>> + ? ? 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);
>> +
>> +/**
>> + * dma_buf_detach - Remove the given attachment from dmabuf's attachments list;
>> + * optionally 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)
>> +{
>> + ? ? if (WARN_ON(!dmabuf || !attach || !dmabuf->ops))
>> + ? ? ? ? ? ? return;
>> +
>> + ? ? mutex_lock(&dmabuf->lock);
>> + ? ? list_del(&attach->node);
>> + ? ? if (dmabuf->ops->detach)
>> + ? ? ? ? ? ? dmabuf->ops->detach(dmabuf, attach);
>> +
>> + ? ? mutex_unlock(&dmabuf->lock);
>> + ? ? kfree(attach);
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_detach);
>> +
>> +/**
>> + * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
>> + * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
>> + * dma_buf_ops.
>> + * @attach: ?[in] ? ?attachment whose scatterlist is to be returned
>> + * @direction: ? ? ? [in] ? ?direction of DMA transfer
>> + *
>> + * Returns sg_table containing the scatterlist to be returned; may return NULL
>> + * or ERR_PTR.
>> + *
>> + */
>> +struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum dma_data_direction direction)
>> +{
>> + ? ? struct sg_table *sg_table = ERR_PTR(-EINVAL);
>> +
>> + ? ? if (WARN_ON(!attach || !attach->dmabuf || !attach->dmabuf->ops))
>> + ? ? ? ? ? ? return ERR_PTR(-EINVAL);
>> +
>> + ? ? mutex_lock(&attach->dmabuf->lock);
>> + ? ? if (attach->dmabuf->ops->map_dma_buf)
>> + ? ? ? ? ? ? sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
>> + ? ? mutex_unlock(&attach->dmabuf->lock);
>> +
>> + ? ? return sg_table;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_map_attachment);
>> +
>> +/**
>> + * dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
>> + * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
>> + * dma_buf_ops.
>> + * @attach: ?[in] ? ?attachment to unmap buffer from
>> + * @sg_table: ? ? ? ?[in] ? ?scatterlist info of the buffer to unmap
>> + *
>> + */
>> +void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct sg_table *sg_table)
>> +{
>> + ? ? if (WARN_ON(!attach || !attach->dmabuf || !sg_table
>> + ? ? ? ? ? ? ? ? ? ? ? ? || !attach->dmabuf->ops))
>> + ? ? ? ? ? ? return;
>> +
>> + ? ? mutex_lock(&attach->dmabuf->lock);
>> + ? ? if (attach->dmabuf->ops->unmap_dma_buf)
>> + ? ? ? ? ? ? attach->dmabuf->ops->unmap_dma_buf(attach, sg_table);
>> + ? ? mutex_unlock(&attach->dmabuf->lock);
>> +
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>> new file mode 100644
>> index 0000000..3e3ecf3
>> --- /dev/null
>> +++ b/include/linux/dma-buf.h
>> @@ -0,0 +1,172 @@
>> +/*
>> + * 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>
>> +#include <linux/dma-mapping.h>
>> +
>> +struct dma_buf;
>> +
>> +/**
>> + * struct dma_buf_attachment - holds device-buffer attachment data
>
> OK, but what is the purpose of it?
>
>> + * @dmabuf: buffer for this attachment.
>> + * @dev: device attached to the buffer.
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^ this
>> + * @node: list_head to allow manipulation of list of dma_buf_attachment.
>
> Just say: "list of dma_buf_attachment"'
>
>> + * @priv: exporter-specific attachment data.
>
> That "exporter-specific.." brings to my mind custom decleration forms. But maybe that is me.
>
>> + */
>> +struct dma_buf_attachment {
>> + ? ? struct dma_buf *dmabuf;
>> + ? ? struct device *dev;
>> + ? ? struct list_head node;
>> + ? ? void *priv;
>> +};
>
> Why don't you move the decleration of this below 'struct dma_buf'?
> It would easier than to read this structure..
>
>> +
>> +/**
>> + * struct dma_buf_ops - operations possible on struct dma_buf
>> + * @attach: allows different devices to 'attach' themselves to the given
>
> register?
>> + * ? ? ? buffer. It might return -EBUSY to signal that backing storage
>> + * ? ? ? is already allocated and incompatible with the requirements
>
> Wait.. allocated or attached?
>
>> + * ? ? ? of requesting device. [optional]
>
> What is optional? The return value? Or the 'attach' call? If the later , say
> that in the first paragraph.
>
>
>> + * @detach: detach a given device from this buffer. [optional]
>> + * @map_dma_buf: returns list of scatter pages allocated, increases usecount
>> + * ? ? ? ? ? ?of the buffer. Requires atleast one attach to be called
>> + * ? ? ? ? ? ?before. Returned sg list should already be mapped into
>> + * ? ? ? ? ? ?_device_ address space. This call may sleep. May also return
>
> Ok, there is some __might_sleep macro you should put on the function.
>
>> + * ? ? ? ? ? ?-EINTR.
>
> Ok. What is the return code if attach has _not_ been called?
>
>> + * @unmap_dma_buf: decreases usecount of buffer, might deallocate scatter
>> + * ? ? ? ? ? ? ?pages.
>> + * @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.
>
> Not seeing those two.
>> + */
>> +struct dma_buf_ops {
>> + ? ? int (*attach)(struct dma_buf *, struct device *,
>> + ? ? ? ? ? ? ? ? ? ? struct dma_buf_attachment *);
>> +
>> + ? ? void (*detach)(struct dma_buf *, struct dma_buf_attachment *);
>> +
>> + ? ? /* For {map,unmap}_dma_buf below, any specific buffer attributes
>> + ? ? ?* required should get added to device_dma_parameters accessible
>> + ? ? ?* via dev->dma_params.
>> + ? ? ?*/
>> + ? ? struct sg_table * (*map_dma_buf)(struct dma_buf_attachment *,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum dma_data_direction);
>> + ? ? void (*unmap_dma_buf)(struct dma_buf_attachment *,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct sg_table *);
>> + ? ? /* TODO: Add try_map_dma_buf version, to return immed with -EBUSY
>
> Ewww. Why? Why not just just the 'map_dma_buf' and return that?
>
>> + ? ? ?* if the call would block.
>> + ? ? ?*/
>> +
>> + ? ? /* after final dma_buf_put() */
>> + ? ? void (*release)(struct dma_buf *);
>> +
>> +};
>> +
>> +/**
>> + * struct dma_buf - shared buffer object
>
> Missing the 'size'.
>
>> + * @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
>
>
> Can you elaborate on this? Is this the "exporter" using this? Or is
> it for the "user" using it? If so, why provide it? Wouldn't the
> user of this have something like this:
>
> struct my_dma_bufs {
> ? ? ? ?struct dma_buf[20];
> ? ? ? ?void *priv;
> }
>
> Anyhow?
>
>> + */
>> +struct dma_buf {
>> + ? ? size_t size;
>> + ? ? struct file *file;
>> + ? ? struct list_head attachments;
>> + ? ? const struct dma_buf_ops *ops;
>> + ? ? /* mutex to serialize list manipulation and other ops */
>> + ? ? struct mutex lock;
>> + ? ? 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,
>> + ? ? ? ? ? ? ? ? ? ? size_t size, 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);
>> +
>> +struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum dma_data_direction);
>> +void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *);
>> +#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,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? size_t size, 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;
>> +}
>> +
>> +static inline struct sg_table *dma_buf_map_attachment(
>> + ? ? struct dma_buf_attachment *attach, enum dma_data_direction write)
>> +{
>> + ? ? return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static inline void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct sg_table *sg)
>> +{
>> + ? ? return;
>> +}
>> +
>> +#endif /* CONFIG_DMA_SHARED_BUFFER */
>> +
>> +#endif /* __DMA_BUF_H__ */
>> --
>> 1.7.4.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Daniel Vetter]

On Mon, Dec 19, 2011 at 02:03:28PM +0530, Sumit Semwal wrote:
> Hello Everyone,
> 
> This is RFC v3 for DMA buffer sharing mechanism - changes from v2 are in the
> changelog below.
> 
> Various subsystems - V4L2, GPU-accessors, DRI to name a few - have felt the 
> need to have a common mechanism to share memory buffers across different
> devices - ARM, video hardware, GPU.
> 
> This need comes forth from a variety of use cases including cameras, image 
> processing, video recorders, sound processing, DMA engines, GPU and display
> buffers, and others.
> 
> This RFC is an attempt to define such a buffer sharing mechanism- it is the
> result of discussions from a couple of memory-management mini-summits held by
> Linaro to understand and address common needs around memory management. [1]
> 
> A new dma_buf buffer object is added, with operations and API to allow 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 user to share the scatterlist using map_dma_buf and
>    unmap_dma_buf operations.
> 
> Documentation present in the patch-set gives more details.
> 
> This is based on design suggestions from many people at 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. [2]
> 
> References:
> [1]: https://wiki.linaro.org/OfficeofCTO/MemoryManagement
> [2]: http://lwn.net/Articles/454389
> 
> Patchset based on top of 3.2-rc3, the current version can be found at
> 
> http://git.linaro.org/gitweb?p=people/sumitsemwal/linux-3.x.git
> Branch: dma-buf-upstr-v2
> 
> Earlier versions:
> v2 at: https://lkml.org/lkml/2011/12/2/53
> v1 at: https://lkml.org/lkml/2011/10/11/92
> 
> Best regards,
> ~Sumit Semwal

I think this is a really good v1 version of dma_buf. It contains all the
required bits (with well-specified semantics in the doc patch) to
implement some basic use-cases and start fleshing out the integration with
various subsystem (like drm and v4l). All the things still under
discussion like
- userspace mmap support
- more advanced (and more strictly specified) coherency models
- and shared infrastructure for implementing exporters
are imo much clearer once we have a few example drivers at hand and a
better understanding of some of the insane corner cases we need to be able
to handle.

And I think any risk that the resulting clarifications will break a basic
use-case is really minimal, so I think it'd be great if this could go into
3.3 (maybe as some kind of staging/experimental infrastructure).

Hence for both patches:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48

[Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Dave Airlie]

>>
>> This is RFC v3 for DMA buffer sharing mechanism - changes from v2 are in the
>> changelog below.
>>
>> Various subsystems - V4L2, GPU-accessors, DRI to name a few - have felt the
>> need to have a common mechanism to share memory buffers across different
>> devices - ARM, video hardware, GPU.
>>
>> This need comes forth from a variety of use cases including cameras, image
>> processing, video recorders, sound processing, DMA engines, GPU and display
>> buffers, and others.
>>
>> This RFC is an attempt to define such a buffer sharing mechanism- it is the
>> result of discussions from a couple of memory-management mini-summits held by
>> Linaro to understand and address common needs around memory management. [1]
>>
>> A new dma_buf buffer object is added, with operations and API to allow 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 user to share the scatterlist using map_dma_buf and
>> ? ?unmap_dma_buf operations.
>>
>> Documentation present in the patch-set gives more details.
>>
>> This is based on design suggestions from many people at 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. [2]
>>
>> References:
>> [1]: https://wiki.linaro.org/OfficeofCTO/MemoryManagement
>> [2]: http://lwn.net/Articles/454389
>>
>> Patchset based on top of 3.2-rc3, the current version can be found at
>>
>> http://git.linaro.org/gitweb?p=people/sumitsemwal/linux-3.x.git
>> Branch: dma-buf-upstr-v2
>>
>> Earlier versions:
>> v2 at: https://lkml.org/lkml/2011/12/2/53
>> v1 at: https://lkml.org/lkml/2011/10/11/92
>>
>> Best regards,
>> ~Sumit Semwal
>
> I think this is a really good v1 version of dma_buf. It contains all the
> required bits (with well-specified semantics in the doc patch) to
> implement some basic use-cases and start fleshing out the integration with
> various subsystem (like drm and v4l). All the things still under
> discussion like
> - userspace mmap support
> - more advanced (and more strictly specified) coherency models
> - and shared infrastructure for implementing exporters
> are imo much clearer once we have a few example drivers at hand and a
> better understanding of some of the insane corner cases we need to be able
> to handle.
>
> And I think any risk that the resulting clarifications will break a basic
> use-case is really minimal, so I think it'd be great if this could go into
> 3.3 (maybe as some kind of staging/experimental infrastructure).
>
> Hence for both patches:
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Yeah I'm with Daniel, I like this one, I can definitely build the drm
buffer sharing layer on top of this.

How do we see this getting merged? I'm quite happy to push it to Linus
if we don't have an identified path, though it could go via a Linaro
tree as well.

so feel free to add:
Reviewed-by: Dave Airlie <airlied@redhat.com>

Dave.

[Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Rob Clark]

On Tue, Dec 20, 2011 at 2:20 PM, Dave Airlie <airlied@gmail.com> wrote:
>>>
>>> This is RFC v3 for DMA buffer sharing mechanism - changes from v2 are in the
>>> changelog below.
>>>
>>> Various subsystems - V4L2, GPU-accessors, DRI to name a few - have felt the
>>> need to have a common mechanism to share memory buffers across different
>>> devices - ARM, video hardware, GPU.
>>>
>>> This need comes forth from a variety of use cases including cameras, image
>>> processing, video recorders, sound processing, DMA engines, GPU and display
>>> buffers, and others.
>>>
>>> This RFC is an attempt to define such a buffer sharing mechanism- it is the
>>> result of discussions from a couple of memory-management mini-summits held by
>>> Linaro to understand and address common needs around memory management. [1]
>>>
>>> A new dma_buf buffer object is added, with operations and API to allow 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 user to share the scatterlist using map_dma_buf and
>>> ? ?unmap_dma_buf operations.
>>>
>>> Documentation present in the patch-set gives more details.
>>>
>>> This is based on design suggestions from many people at 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. [2]
>>>
>>> References:
>>> [1]: https://wiki.linaro.org/OfficeofCTO/MemoryManagement
>>> [2]: http://lwn.net/Articles/454389
>>>
>>> Patchset based on top of 3.2-rc3, the current version can be found at
>>>
>>> http://git.linaro.org/gitweb?p=people/sumitsemwal/linux-3.x.git
>>> Branch: dma-buf-upstr-v2
>>>
>>> Earlier versions:
>>> v2 at: https://lkml.org/lkml/2011/12/2/53
>>> v1 at: https://lkml.org/lkml/2011/10/11/92
>>>
>>> Best regards,
>>> ~Sumit Semwal
>>
>> I think this is a really good v1 version of dma_buf. It contains all the
>> required bits (with well-specified semantics in the doc patch) to
>> implement some basic use-cases and start fleshing out the integration with
>> various subsystem (like drm and v4l). All the things still under
>> discussion like
>> - userspace mmap support
>> - more advanced (and more strictly specified) coherency models
>> - and shared infrastructure for implementing exporters
>> are imo much clearer once we have a few example drivers at hand and a
>> better understanding of some of the insane corner cases we need to be able
>> to handle.
>>
>> And I think any risk that the resulting clarifications will break a basic
>> use-case is really minimal, so I think it'd be great if this could go into
>> 3.3 (maybe as some kind of staging/experimental infrastructure).
>>
>> Hence for both patches:
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Yeah I'm with Daniel, I like this one, I can definitely build the drm
> buffer sharing layer on top of this.
>
> How do we see this getting merged? I'm quite happy to push it to Linus
> if we don't have an identified path, though it could go via a Linaro
> tree as well.
>
> so feel free to add:
> Reviewed-by: Dave Airlie <airlied@redhat.com>

fwiw, patches to share buffers between drm and v4l2 are here:

https://github.com/robclark/kernel-omap4/commits/drmplane-dmabuf

(need a bit of cleanup before the vb2 patches are submitted.. but that
is unrelated to the dmabuf patches)

so,

Reviewed-and-Tested-by: Rob Clark <rob.clark@linaro.org>

> Dave.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 21, 2011 at 3:56 AM, Rob Clark <rob@ti.com> wrote:
> On Tue, Dec 20, 2011 at 2:20 PM, Dave Airlie <airlied@gmail.com> wrote:
>>>>
<snip>
>>>
>>> I think this is a really good v1 version of dma_buf. It contains all the
>>> required bits (with well-specified semantics in the doc patch) to
>>> implement some basic use-cases and start fleshing out the integration with
>>> various subsystem (like drm and v4l). All the things still under
>>> discussion like
>>> - userspace mmap support
>>> - more advanced (and more strictly specified) coherency models
>>> - and shared infrastructure for implementing exporters
>>> are imo much clearer once we have a few example drivers at hand and a
>>> better understanding of some of the insane corner cases we need to be able
>>> to handle.
>>>
>>> And I think any risk that the resulting clarifications will break a basic
>>> use-case is really minimal, so I think it'd be great if this could go into
>>> 3.3 (maybe as some kind of staging/experimental infrastructure).
>>>
>>> Hence for both patches:
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Yeah I'm with Daniel, I like this one, I can definitely build the drm
>> buffer sharing layer on top of this.
>>
>> How do we see this getting merged? I'm quite happy to push it to Linus
>> if we don't have an identified path, though it could go via a Linaro
>> tree as well.
>>
>> so feel free to add:
>> Reviewed-by: Dave Airlie <airlied@redhat.com>
>
> fwiw, patches to share buffers between drm and v4l2 are here:
>
> https://github.com/robclark/kernel-omap4/commits/drmplane-dmabuf
>
> (need a bit of cleanup before the vb2 patches are submitted.. but that
> is unrelated to the dmabuf patches)
>
> so,
>
> Reviewed-and-Tested-by: Rob Clark <rob.clark@linaro.org>
Thanks Daniel, Dave, and Rob!
BR,
Sumit.
>
>> Dave.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[RFC v3 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Semwal, Sumit]

Hi Konrad,

On Tue, Dec 20, 2011 at 10:06 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Dec 19, 2011 at 02:03:30PM +0530, 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:
>> - different devices to 'attach' themselves to this buffer, to facilitate
>> ? backing storage negotiation, using dma_buf_attach() API.
>
> Any thoughts of adding facility to track them? So you can see who is using what?
Not for version 1, but it would be a useful addition once we start
using this mechanism.

>
>> - association of a file pointer with each user-buffer and associated
>> ? ?allocator-defined operations on that buffer. This operation is called the
>> ? ?'export' operation.
>
> ?'create'? or 'alloc' ?
>
> export implies an import somwhere and I don't think that is the case here.
I will rephrase it as suggested by Rob as well.

>
>> - 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 user to share the scatterlist 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.
>
> for the whole memory region or just for the device itself?
Rob has very eloquently and kindly explained it in his reply.

>
>>
<snip>
>> +/*
>> + * 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;
>> +}
>> +
>> +/**
>
> Wrong kerneldoc.
I looked into scripts/kernel-doc, and
Documentation/kernel-doc-na-HOWTO.txt => both these places mention
that the kernel-doc comments have to start with /**, and I couldn't
spot an error in what's wrong with my usage - would you please
elaborate on what you think is not right?
>
<snip>
>> +/**
>> + * struct dma_buf_attachment - holds device-buffer attachment data
>
> OK, but what is the purpose of it?
I will add that in the comments.
>
>> + * @dmabuf: buffer for this attachment.
>> + * @dev: device attached to the buffer.
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^ this
>> + * @node: list_head to allow manipulation of list of dma_buf_attachment.
>
> Just say: "list of dma_buf_attachment"'
ok.
>
>> + * @priv: exporter-specific attachment data.
>
> That "exporter-specific.." brings to my mind custom decleration forms. But maybe that is me.
:) well, in context of dma-buf 'exporter', it makes sense.

>
>> + */
>> +struct dma_buf_attachment {
>> + ? ? struct dma_buf *dmabuf;
>> + ? ? struct device *dev;
>> + ? ? struct list_head node;
>> + ? ? void *priv;
>> +};
>
> Why don't you move the decleration of this below 'struct dma_buf'?
> It would easier than to read this structure..
I could do that, but then anyways I will have to do a
forward-declaration of dma_buf_attachment, since I have to use it in
dma_buf_ops. If it improves readability, I am happy to move it below
struct dma_buf.

>
>> +
>> +/**
>> + * struct dma_buf_ops - operations possible on struct dma_buf
>> + * @attach: allows different devices to 'attach' themselves to the given
>
> register?
>> + * ? ? ? buffer. It might return -EBUSY to signal that backing storage
>> + * ? ? ? is already allocated and incompatible with the requirements
>
> Wait.. allocated or attached?
This and above comment on 'register' are already answered by Rob in
his explanation of the sequence in earlier reply. [the Documentation
patch [2/2] also tries to explain it]

>
>> + * ? ? ? of requesting device. [optional]
>
> What is optional? The return value? Or the 'attach' call? If the later , say
> that in the first paragraph.
>
ok, sure. it is meant for the attach op.
>
>> + * @detach: detach a given device from this buffer. [optional]
>> + * @map_dma_buf: returns list of scatter pages allocated, increases usecount
>> + * ? ? ? ? ? ?of the buffer. Requires atleast one attach to be called
>> + * ? ? ? ? ? ?before. Returned sg list should already be mapped into
>> + * ? ? ? ? ? ?_device_ address space. This call may sleep. May also return
>
> Ok, there is some __might_sleep macro you should put on the function.
>
That's a nice suggestion; I will add it to the wrapper function for
map_dma_buf().

>> + * ? ? ? ? ? ?-EINTR.
>
> Ok. What is the return code if attach has _not_ been called?
Will document it to return -EINVAL if atleast on attach() hasn't been called.

>
>> + * @unmap_dma_buf: decreases usecount of buffer, might deallocate scatter
>> + * ? ? ? ? ? ? ?pages.
>> + * @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.
>
> Not seeing those two.
Oops; removed in v3 - will correct.

>> + */
<snip>
>> + ? ? /* TODO: Add try_map_dma_buf version, to return immed with -EBUSY
>
> Ewww. Why? Why not just just the 'map_dma_buf' and return that?
Requirement is to allow for blocking and non-blocking versions of
map_dma_buf. try_map_dma_buf could be used for the non-blocking
version.

>
<snip>
>> +/**
>> + * struct dma_buf - shared buffer object
>
> Missing the 'size'.
Will add.
>
>> + * @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
>
>
> Can you elaborate on this? Is this the "exporter" using this? Or is
> it for the "user" using it? If so, why provide it? Wouldn't the
> user of this have something like this:
>
> struct my_dma_bufs {
> ? ? ? ?struct dma_buf[20];
> ? ? ? ?void *priv;
> }
>
> Anyhow?
My bad - it is meant for the exporter - exporter gives this as one of
the parameters to 'dma_buf_export()' API. I will correct the comment.
>
Thanks for your review!
Best regards,
~Sumit.

[Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Semwal, Sumit]

On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie <airlied@gmail.com> wrote:
<snip>
>>
>> I think this is a really good v1 version of dma_buf. It contains all the
>> required bits (with well-specified semantics in the doc patch) to
>> implement some basic use-cases and start fleshing out the integration with
>> various subsystem (like drm and v4l). All the things still under
>> discussion like
>> - userspace mmap support
>> - more advanced (and more strictly specified) coherency models
>> - and shared infrastructure for implementing exporters
>> are imo much clearer once we have a few example drivers at hand and a
>> better understanding of some of the insane corner cases we need to be able
>> to handle.
>>
>> And I think any risk that the resulting clarifications will break a basic
>> use-case is really minimal, so I think it'd be great if this could go into
>> 3.3 (maybe as some kind of staging/experimental infrastructure).
>>
>> Hence for both patches:
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Yeah I'm with Daniel, I like this one, I can definitely build the drm
> buffer sharing layer on top of this.
>
> How do we see this getting merged? I'm quite happy to push it to Linus
> if we don't have an identified path, though it could go via a Linaro
> tree as well.
>
> so feel free to add:
> Reviewed-by: Dave Airlie <airlied@redhat.com>
Thanks Daniel and Dave!

I guess we can start with staging for 3.3, and see how it shapes up. I
will post the latest patch version pretty soon.

Arnd, Dave: do you have any preference on the path it takes to get
merged? In my mind, Linaro tree might make more sense, but I would
leave it upto you gentlemen.
>
> Dave.
Best regards,
~Sumit.

[Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Rob Clark]

On Fri, Dec 23, 2011 at 4:08 AM, Semwal, Sumit <sumit.semwal@ti.com> wrote:
> On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie <airlied@gmail.com> wrote:
> <snip>
>>>
>>> I think this is a really good v1 version of dma_buf. It contains all the
>>> required bits (with well-specified semantics in the doc patch) to
>>> implement some basic use-cases and start fleshing out the integration with
>>> various subsystem (like drm and v4l). All the things still under
>>> discussion like
>>> - userspace mmap support
>>> - more advanced (and more strictly specified) coherency models
>>> - and shared infrastructure for implementing exporters
>>> are imo much clearer once we have a few example drivers at hand and a
>>> better understanding of some of the insane corner cases we need to be able
>>> to handle.
>>>
>>> And I think any risk that the resulting clarifications will break a basic
>>> use-case is really minimal, so I think it'd be great if this could go into
>>> 3.3 (maybe as some kind of staging/experimental infrastructure).
>>>
>>> Hence for both patches:
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Yeah I'm with Daniel, I like this one, I can definitely build the drm
>> buffer sharing layer on top of this.
>>
>> How do we see this getting merged? I'm quite happy to push it to Linus
>> if we don't have an identified path, though it could go via a Linaro
>> tree as well.
>>
>> so feel free to add:
>> Reviewed-by: Dave Airlie <airlied@redhat.com>
> Thanks Daniel and Dave!
>
> I guess we can start with staging for 3.3, and see how it shapes up. I
> will post the latest patch version pretty soon.

not sure about staging, but could make sense to mark as experimental.

> Arnd, Dave: do you have any preference on the path it takes to get
> merged? In my mind, Linaro tree might make more sense, but I would
> leave it upto you gentlemen.

Looks like Dave is making some progress on drm usage of buffer sharing
between gpu's.. if that is ready to go in at the same time, it might
be a bit logistically simpler for him to put dmabuf in the same pull
req.  I don't have strong preference one way or another, so do what is
collectively simpler ;-)

BR,
-R

>>
>> Dave.
> Best regards,
> ~Sumit.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism

[Semwal, Sumit]

On Fri, Dec 23, 2011 at 10:50 PM, Rob Clark <rob@ti.com> wrote:
> On Fri, Dec 23, 2011 at 4:08 AM, Semwal, Sumit <sumit.semwal@ti.com> wrote:
>> On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie <airlied@gmail.com> wrote:
>> <snip>
>>>>
>>>> Hence for both patches:
>>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> Yeah I'm with Daniel, I like this one, I can definitely build the drm
>>> buffer sharing layer on top of this.
>>>
>>> How do we see this getting merged? I'm quite happy to push it to Linus
>>> if we don't have an identified path, though it could go via a Linaro
>>> tree as well.
>>>
>>> so feel free to add:
>>> Reviewed-by: Dave Airlie <airlied@redhat.com>
>> Thanks Daniel and Dave!
>>
>> I guess we can start with staging for 3.3, and see how it shapes up. I
>> will post the latest patch version pretty soon.
>
> not sure about staging, but could make sense to mark as experimental.
Thanks, I will mark it experimental for the first version; we can
remove that once it is more widely used and tested.
>
>> Arnd, Dave: do you have any preference on the path it takes to get
>> merged? In my mind, Linaro tree might make more sense, but I would
>> leave it upto you gentlemen.
>
> Looks like Dave is making some progress on drm usage of buffer sharing
> between gpu's.. if that is ready to go in at the same time, it might
> be a bit logistically simpler for him to put dmabuf in the same pull
> req. ?I don't have strong preference one way or another, so do what is
> collectively simpler ;-)
:) Right - I am quite happy for it to get merged in either ways :)
>
> BR,
> -R
Best regards,
~Sumit.

[RFC v3 1/2] dma-buf: Introduce dma buffer sharing mechanism

[Konrad Rzeszutek Wilk]

On Fri, Dec 23, 2011 at 03:22:35PM +0530, Semwal, Sumit wrote:
> Hi Konrad,
> 
> On Tue, Dec 20, 2011 at 10:06 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Mon, Dec 19, 2011 at 02:03:30PM +0530, 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:
> >> - different devices to 'attach' themselves to this buffer, to facilitate
> >> ? backing storage negotiation, using dma_buf_attach() API.
> >
> > Any thoughts of adding facility to track them? So you can see who is using what?
> Not for version 1, but it would be a useful addition once we start
> using this mechanism.

OK. Looking forward to V2.
> 
> >
> >> - association of a file pointer with each user-buffer and associated
> >> ? ?allocator-defined operations on that buffer. This operation is called the
> >> ? ?'export' operation.
> >
> > ?'create'? or 'alloc' ?
> >
> > export implies an import somwhere and I don't think that is the case here.
> I will rephrase it as suggested by Rob as well.
> 
> >
> >> - 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 user to share the scatterlist 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.
> >
> > for the whole memory region or just for the device itself?
> Rob has very eloquently and kindly explained it in his reply.

Can you include his words of wisdom in the git description?

> 
> >
> >>
> <snip>
> >> +/*
> >> + * 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;
> >> +}
> >> +
> >> +/**
> >
> > Wrong kerneldoc.
> I looked into scripts/kernel-doc, and
> Documentation/kernel-doc-na-HOWTO.txt => both these places mention
> that the kernel-doc comments have to start with /**, and I couldn't
> spot an error in what's wrong with my usage - would you please
> elaborate on what you think is not right?

The issue I had was with '/**' but let me double-check where I learned
that /** was a bad. Either way, it is a style-guide thing and the
Documentation/* trumps what I recall.

> >
> <snip>
> >> +/**
> >> + * struct dma_buf_attachment - holds device-buffer attachment data
> >
> > OK, but what is the purpose of it?
> I will add that in the comments.
> >
> >> + * @dmabuf: buffer for this attachment.
> >> + * @dev: device attached to the buffer.
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^ this
> >> + * @node: list_head to allow manipulation of list of dma_buf_attachment.
> >
> > Just say: "list of dma_buf_attachment"'
> ok.
> >
> >> + * @priv: exporter-specific attachment data.
> >
> > That "exporter-specific.." brings to my mind custom decleration forms. But maybe that is me.
> :) well, in context of dma-buf 'exporter', it makes sense.

Or just private contents of the backend driver. But the naming is not that important
to inhibit this patch from being merged.
> 
> >
> >> + */
> >> +struct dma_buf_attachment {
> >> + ? ? struct dma_buf *dmabuf;
> >> + ? ? struct device *dev;
> >> + ? ? struct list_head node;
> >> + ? ? void *priv;
> >> +};
> >
> > Why don't you move the decleration of this below 'struct dma_buf'?
> > It would easier than to read this structure..
> I could do that, but then anyways I will have to do a
> forward-declaration of dma_buf_attachment, since I have to use it in
> dma_buf_ops. If it improves readability, I am happy to move it below
> struct dma_buf

It is more of just making the readability easier. As in reading from top bottom
one. But if it is too ugly, don't bother.
> 
> >
> >> +
> >> +/**
> >> + * struct dma_buf_ops - operations possible on struct dma_buf
> >> + * @attach: allows different devices to 'attach' themselves to the given
> >
> > register?
> >> + * ? ? ? buffer. It might return -EBUSY to signal that backing storage
> >> + * ? ? ? is already allocated and incompatible with the requirements
> >
> > Wait.. allocated or attached?
> This and above comment on 'register' are already answered by Rob in
> his explanation of the sequence in earlier reply. [the Documentation
> patch [2/2] also tries to explain it]

OK. Might want to mention the user to look in the Documentation, in case you don't
already have it.
> 
> >
> >> + * ? ? ? of requesting device. [optional]
> >
> > What is optional? The return value? Or the 'attach' call? If the later , say
> > that in the first paragraph.
> >
> ok, sure. it is meant for the attach op.
> >
> >> + * @detach: detach a given device from this buffer. [optional]
> >> + * @map_dma_buf: returns list of scatter pages allocated, increases usecount
> >> + * ? ? ? ? ? ?of the buffer. Requires atleast one attach to be called
> >> + * ? ? ? ? ? ?before. Returned sg list should already be mapped into
> >> + * ? ? ? ? ? ?_device_ address space. This call may sleep. May also return
> >
> > Ok, there is some __might_sleep macro you should put on the function.
> >
> That's a nice suggestion; I will add it to the wrapper function for
> map_dma_buf().
> 
> >> + * ? ? ? ? ? ?-EINTR.
> >
> > Ok. What is the return code if attach has _not_ been called?
> Will document it to return -EINVAL if atleast on attach() hasn't been called.
> 
> >
> >> + * @unmap_dma_buf: decreases usecount of buffer, might deallocate scatter
> >> + * ? ? ? ? ? ? ?pages.
> >> + * @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.
> >
> > Not seeing those two.
> Oops; removed in v3 - will correct.
> 
> >> + */
> <snip>
> >> + ? ? /* TODO: Add try_map_dma_buf version, to return immed with -EBUSY
> >
> > Ewww. Why? Why not just just the 'map_dma_buf' and return that?
> Requirement is to allow for blocking and non-blocking versions of
> map_dma_buf. try_map_dma_buf could be used for the non-blocking
> version.

Ok. What about just adding that in as a nop function operation. And have an
initial implementation in the code base that does .. well, nothing except 
return -ENOSPC and move the TODO to the code instead of it being in the header?

> 
> >
> <snip>
> >> +/**
> >> + * struct dma_buf - shared buffer object
> >
> > Missing the 'size'.
> Will add.
> >
> >> + * @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
> >
> >
> > Can you elaborate on this? Is this the "exporter" using this? Or is
> > it for the "user" using it? If so, why provide it? Wouldn't the
> > user of this have something like this:
> >
> > struct my_dma_bufs {
> > ? ? ? ?struct dma_buf[20];
> > ? ? ? ?void *priv;
> > }
> >
> > Anyhow?
> My bad - it is meant for the exporter - exporter gives this as one of
> the parameters to 'dma_buf_export()' API. I will correct the comment.
> >
> Thanks for your review!

Sure. And please put 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>'.
> Best regards,
> ~Sumit.