Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework

On 2/25/19 6:20 PM, John Stultz wrote:

On Mon, Feb 25, 2019 at 6:36 AM Andrew F. Davis afd@ti.com wrote:

...

This framework allows a unified userspace interface for dma-buf exporters, allowing userland to allocate specific types of memory for use in dma-buf sharing.

Each heap is given its own device node, which a user can allocate a dma-buf fd from using the DMA_HEAP_IOC_ALLOC.

Signed-off-by: Andrew F. Davis afd@ti.com

Hello all,

I had a little less time over the weekend than I thought I would to clean this up more and finish the first set of user heaps, but wanted to get this out anyway.

ION in its current form assumes a lot about the memory it exports and these assumptions lead to restrictions on the full range of operations dma-buf's can produce. Due to this if we are to add this as an extension of the core dma-buf then it should only be the user-space advertisement and allocation front-end. All dma-buf exporting and operation need to be placed in the control of the exporting heap. The core becomes rather small, just a few hundred lines you see here. This is not to say we should not provide helpers to make the heap exporters more simple, but they should only be helpers and not enforced by the core framework.

Basic use model here is an exporter (dedicated heap memory driver, CMA, System, etc.) registers with the framework by providing a struct dma_heap_info which gives us the needed info to export this heap to userspace as a device node. Next a user will request an allocation, the IOCTL is parsed and the request made to a heap provided alloc() op. The heap should return the filled out struct dma_heap_buffer, including exporting the buffer as a dma-buf. This dma-buf we then return to the user as a fd. Buffer free is still a bit open as we need to update some stats and free some memory, but the release operation is controlled by the heap exporter, so some hook will have to be created.

It all needs a bit more work, and I'm sure I'll find missing parts when I add some more heaps, but hopefully this framework is simple enough that it does not impede the implementation of all functionality once provided by ION (shrinker, delayed free), nor any new functionality needed for future heap exporting devices.

Thanks, Andrew

drivers/dma-buf/Kconfig | 12 ++ drivers/dma-buf/Makefile | 1 + drivers/dma-buf/dma-heap.c | 268 ++++++++++++++++++++++++++++++++++ include/linux/dma-heap.h | 57 ++++++++ include/uapi/linux/dma-heap.h | 54 +++++++ 5 files changed, 392 insertions(+) create mode 100644 drivers/dma-buf/dma-heap.c create mode 100644 include/linux/dma-heap.h create mode 100644 include/uapi/linux/dma-heap.h

diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 2e5a0faa2cb1..30b0d7c83945 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -39,4 +39,16 @@ config UDMABUF A driver to let userspace turn memfd regions into dma-bufs. Qemu can use this to create host dmabufs for guest framebuffers.

+menuconfig DMABUF_HEAPS

•   bool "DMA-BUF Userland Memory Heaps"
  

•   depends on HAS_DMA && MMU
  

•   select GENERIC_ALLOCATOR
  

•   select DMA_SHARED_BUFFER
  

•   help
  

•     Choose this option to enable the DMA-BUF userland memory heaps,
  

•     this allows userspace to allocate dma-bufs that can be shared between
  

•     drivers.
  

+source "drivers/dma-buf/heaps/Kconfig"

endmenu diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 0913a6ccab5a..b0332f143413 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,4 +1,5 @@ obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o seqno-fence.o +obj-$(CONFIG_DMABUF_HEAPS) += dma-heap.o obj-$(CONFIG_SYNC_FILE) += sync_file.o obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o obj-$(CONFIG_UDMABUF) += udmabuf.o diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c new file mode 100644 index 000000000000..72ed225fa892 --- /dev/null +++ b/drivers/dma-buf/dma-heap.c @@ -0,0 +1,268 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Framework for userspace DMA-BUF allocations
• • Copyright (C) 2011 Google, Inc.
• • Copyright (C) 2019 Linaro Ltd.
• */

+#include <linux/cdev.h> +#include <linux/debugfs.h> +#include <linux/device.h> +#include <linux/dma-buf.h> +#include <linux/err.h> +#include <linux/idr.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/uaccess.h>

+#include <linux/dma-heap.h> +#include <uapi/linux/dma-heap.h>

+#define DEVNAME "dma_heap"

+#define NUM_HEAP_MINORS 128 +static DEFINE_IDR(dma_heap_idr); +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */

+dev_t dma_heap_devt; +struct class *dma_heap_class; +struct list_head dma_heap_list; +struct dentry *dma_heap_debug_root;

+/**

• • struct dma_heap - represents a dmabuf heap in the system
• • @name: used for debugging/device-node name
• • @ops: ops struct for this heap
• • @minor minor number of this heap device
• • @heap_devt heap device node
• • @heap_cdev heap char device
• • @num_of_buffers the number of currently allocated buffers
• • @num_of_alloc_bytes the number of allocated bytes
• • @alloc_bytes_wm the number of allocated bytes watermark
• • @stat_lock lock for heap statistics
• • Represents a heap of memory from which buffers can be made.
• */

+struct dma_heap {

•   const char *name;
  

•   struct dma_heap_ops *ops;
  

•   unsigned int minor;
  

•   dev_t heap_devt;
  

•   struct cdev heap_cdev;
  

•   /* heap statistics */
  

•   u64 num_of_buffers;
  

•   u64 num_of_alloc_bytes;
  

•   u64 alloc_bytes_wm;
  

•   spinlock_t stat_lock;
  

+};

One issue I've run into here implementing the heaps on top of this:

So.. you've defined the dma_heap here in the dma-heap.c, but this structure needs to be visible to the heap implementations, as their allocate functions look like: int (*allocate)(struct dma_heap *heap, struct dma_heap_buffer *buffer, unsigned long len, unsigned long flags);

dma_heap is meant to be an opaque struct pointer, it is passed in as a token to keep the interface consistent and so the heap can identify itself (when I had the free() op it also started with dma_heap so the heaps could track themselves, now with only allocate() it probably doesn't matter much until we add something else).

Plus the dma_heap duplicates the the dma_heap_info fields.

Some of them, as above dma_heap is meant to be strictly internal, heaps provide dma_heap_info into the framework and produce dma_heap_buffers.

It seems like the dma_heap is what the heap implementation should allocate register, so it can traverse via container_of up to its own data structure, rather then doing it in dma_heap_add().

Hmm, let me look at how this ended up in the code.

Thanks, Andrew

thanks -john