[Linaro-mm-sig] [RFC v2] dma-buf: Add buffer sharing framework
Hans Verkuil
hverkuil at xs4all.nl
Tue Sep 27 13:24:24 UTC 2011
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 at arndb.de>, Rob Clark <rob at ti.com> and
> Daniel Vetter <daniel at ffwll.ch>.
>
> The implementation is inspired from proof-of-concept patch-set from
> Tomasz Stanislawski <t.stanislaws at 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 at linaro.org>
> Signed-off-by: Sumit Semwal <sumit.semwal at 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 at ti.com>
> + *
> + * Many thanks to linaro-mm-sig list, and specially
> + * Arnd Bergmann <arnd at arndb.de>, Rob Clark <rob at ti.com> and
> + * Daniel Vetter <daniel at 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__ */
>
More information about the Linaro-mm-sig
mailing list