Hi Sumit,
Thanks for your work! Sorry for being absent recently, but Real Life (tm) interfered with my normal review duties in the past 3-4 weeks.
So I've been reading the backlog today and I have a few questions and remarks below.
On Tuesday, September 13, 2011 16:29:09 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
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
<cut>
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,
+};
+/**
- 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,
+};
+/**
- 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.
- @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 *);
+};
OK, it is not clear to me what the purpose is of the attachments.
If I understand the discussion from this list correctly, then the idea is that each device that wants to use this buffer first attaches itself by calling dma_buf_attach().
Then at some point the application asks some driver to export the buffer. So the driver calls dma_buf_export() and passes its own dma_buf_ops. In other words, this becomes the driver that 'controls' the memory, right?
Another driver that receives the fd will call dma_buf_get() and can then call e.g. get_scatterlist from dma_buf->ops. (As an aside: I would make inline functions that take a dma_buf pointer and call the corresponding op, rather than requiring drivers to go through ops directly)
But what I miss in this picture is the role of dma_buf_attachment. I'm passing it to get_scatterlist, but which attachment is that? That of the calling driver? And what is the get_scatterlist implementation supposed to do with it?
I also read some discussion about what is supposed to happen if another device is attached after get_scatterlist was already called. Apparently the idea was that the old scatterlist is somehow migrated to a new one if that should be necessary? Although I got the impression that that involved a lot of hand-waving with a pinch of wishful thinking. But I may be wrong about that.
Anyway, I guess my main point is that this patch does not explain the role of the attachments and how they should be used (and who uses them).
One other thing: once you call REQBUFS on a V4L device the V4L spec says that the memory should be allocated at that time. Because V4L often needs a lot of memory that behavior makes sense: you know immediately if you can get the memory or not. In addition, that memory is mmap-ed before the DMA is started.
This behavior may pose a problem if the idea is to wait with actually allocating memory until the pipeline is started.
Hmm, I'm rambling a bit, but I hope the gist of my mail is clear.
Regards,
Hans
+/**
- 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__ */