dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
@ 2019-02-25 14:36 Andrew F. Davis
  2019-02-25 18:41 ` John Stultz
                   ` (7 more replies)
  0 siblings, 8 replies; 27+ messages in thread
From: Andrew F. Davis @ 2019-02-25 14:36 UTC (permalink / raw)
  To: John Stultz, Laura Abbott, Sumit Semwal, Benjamin Gaignard,
	Liam Mark, Brian Starkey, Chenbo Feng, Alistair Strachan
  Cc: dri-devel, Andrew F . Davis

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;
+};
+
+/*
+ * This should be called by the heap exporter when a buffers dma-buf
+ * handle is released so the core can update the stats and release the
+ * buffer handle memory.
+ *
+ * Or we could find a way to hook into the fd or struct dma_buf itself?
+ */
+void dma_heap_buffer_free(struct dma_heap_buffer *buffer)
+{
+	spin_lock(&buffer->heap->stat_lock);
+	buffer->heap->num_of_buffers--;
+	buffer->heap->num_of_alloc_bytes -= buffer->size;
+	spin_unlock(&buffer->heap->stat_lock);
+
+	kfree(buffer);
+}
+
+static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, unsigned int flags)
+{
+	struct dma_heap_buffer *buffer;
+	int fd, ret;
+
+	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	buffer->heap = heap;
+	ret = heap->ops->allocate(heap, buffer, len, flags);
+	if (ret) {
+		kfree(buffer);
+		return ret;
+	}
+
+	fd = dma_buf_fd(buffer->dmabuf, O_CLOEXEC);
+	if (fd < 0) {
+		dma_buf_put(buffer->dmabuf);
+		return fd;
+	}
+
+	spin_lock(&heap->stat_lock);
+	heap->num_of_buffers++;
+	heap->num_of_alloc_bytes += buffer->size;
+	if (heap->num_of_alloc_bytes > heap->alloc_bytes_wm)
+		heap->alloc_bytes_wm = heap->num_of_alloc_bytes;
+	spin_unlock(&heap->stat_lock);
+
+	return fd;
+}
+
+static int dma_heap_open(struct inode *inode, struct file *filp)
+{
+	struct dma_heap *heap;
+
+	mutex_lock(&minor_lock);
+	heap = idr_find(&dma_heap_idr, iminor(inode));
+	mutex_unlock(&minor_lock);
+	if (!heap) {
+		pr_err("dma_heap: minor %d unknown.\n", iminor(inode));
+		return -ENODEV;
+	}
+
+	/* instance data as context */
+	filp->private_data = heap;
+	nonseekable_open(inode, filp);
+
+	return 0;
+}
+
+static int dma_heap_release(struct inode *inode, struct file *filp)
+{
+	filp->private_data = NULL;
+
+	return 0;
+}
+
+static long dma_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	switch (cmd) {
+	case DMA_HEAP_IOC_ALLOC:
+	{
+		struct dma_heap_allocation_data heap_allocation;
+		struct dma_heap *heap = filp->private_data;
+		int fd;
+
+		if (copy_from_user(&heap_allocation, (void __user *)arg, _IOC_SIZE(cmd)))
+			return -EFAULT;
+
+		if (heap_allocation.reserved0 ||
+		    heap_allocation.reserved1) {
+			pr_warn_once("dma_heap: ioctl data not valid\n");
+			return -EINVAL;
+		}
+
+		fd = dma_heap_buffer_alloc(heap, heap_allocation.len, heap_allocation.flags);
+		if (fd < 0)
+			return fd;
+
+		heap_allocation.fd = fd;
+
+		if (copy_to_user((void __user *)arg, &heap_allocation, _IOC_SIZE(cmd)))
+			return -EFAULT;
+
+		break;
+	}
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
+}
+
+static const struct file_operations dma_heap_fops = {
+	.owner          = THIS_MODULE,
+	.open		= dma_heap_open,
+	.release	= dma_heap_release,
+	.unlocked_ioctl = dma_heap_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= dma_heap_ioctl,
+#endif
+};
+
+int dma_heap_add(struct dma_heap_info *heap_info)
+{
+	struct dma_heap *heap;
+	struct device *dev_ret;
+	struct dentry *heap_root;
+	int ret;
+
+	if (!heap_info->name || !strcmp(heap_info->name, "")) {
+		pr_err("dma_heap: Cannot add heap without a name\n");
+		return -EINVAL;
+	}
+
+	if (!heap_info->ops || !heap_info->ops->allocate) {
+		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
+		return -EINVAL;
+	}
+
+	heap = kzalloc(sizeof(*heap), GFP_KERNEL);
+	if (!heap)
+		return -ENOMEM;
+
+	heap->name = heap_info->name;
+	memcpy(heap->ops, heap_info->ops, sizeof(*heap->ops));
+
+	/* Find unused minor number */
+	mutex_lock(&minor_lock);
+	ret = idr_alloc(&dma_heap_idr, heap, 0, NUM_HEAP_MINORS, GFP_KERNEL);
+	mutex_unlock(&minor_lock);
+	if (ret < 0) {
+		pr_err("dma_heap: Unable to get minor number for heap\n");
+		return ret;
+	}
+	heap->minor = ret;
+
+	/* Create device */
+	heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
+	dev_ret = device_create(dma_heap_class,
+				NULL,
+				heap->heap_devt,
+				NULL,
+				heap->name);
+	if (IS_ERR(dev_ret)) {
+		pr_err("dma_heap: Unable to create char device\n");
+		return PTR_ERR(dev_ret);
+	}
+
+	/* Add device */
+	cdev_init(&heap->heap_cdev, &dma_heap_fops);
+	ret = cdev_add(&heap->heap_cdev, dma_heap_devt, NUM_HEAP_MINORS);
+	if (ret < 0) {
+		device_destroy(dma_heap_class, heap->heap_devt);
+		pr_err("dma_heap: Unable to add char device\n");
+		return ret;
+	}
+
+	heap->num_of_buffers = 0;
+	heap->num_of_alloc_bytes = 0;
+	heap->alloc_bytes_wm = 0;
+	spin_lock_init(&heap->stat_lock);
+	heap_root = debugfs_create_dir(heap->name, dma_heap_debug_root);
+	debugfs_create_u64("num_of_buffers", 0444, heap_root, &heap->num_of_buffers);
+	debugfs_create_u64("num_of_alloc_bytes", 0444, heap_root, &heap->num_of_alloc_bytes);
+	debugfs_create_u64("alloc_bytes_wm", 0444, heap_root, &heap->alloc_bytes_wm);
+
+	return 0;
+}
+EXPORT_SYMBOL(dma_heap_add);
+
+static int dma_heap_init(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&dma_heap_devt, 0, NUM_HEAP_MINORS, DEVNAME);
+	if (ret)
+		return ret;
+
+	dma_heap_class = class_create(THIS_MODULE, DEVNAME);
+	if (IS_ERR(dma_heap_class)) {
+		unregister_chrdev_region(dma_heap_devt, NUM_HEAP_MINORS);
+		return PTR_ERR(dma_heap_class);
+	}
+
+	dma_heap_debug_root = debugfs_create_dir("dma_heap", NULL);
+
+	return 0;
+}
+subsys_initcall(dma_heap_init);
diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
new file mode 100644
index 000000000000..09eab3105118
--- /dev/null
+++ b/include/linux/dma-heap.h
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DMABUF Heaps Allocation Infrastructure
+ *
+ * Copyright (C) 2011 Google, Inc.
+ * Copyright (C) 2019 Linaro Ltd.
+ */
+
+#ifndef _DMA_HEAPS_H
+#define _DMA_HEAPS_H
+
+#include <linux/types.h>
+
+/**
+ * struct dma_heap_buffer - metadata for a particular buffer
+ * @heap:		back pointer to the heap the buffer came from
+ * @dmabuf:		backing dma-buf for this buffer
+ * @size:		size of the buffer
+ * @flags:		buffer specific flags
+ */
+struct dma_heap_buffer {
+	struct dma_heap *heap;
+	struct dma_buf *dmabuf;
+	size_t size;
+	unsigned long flags;
+};
+
+/**
+ * struct dma_heap_ops - ops to operate on a given heap
+ * @allocate:		allocate buffer
+ *
+ * allocate returns 0 on success, -errno on error.
+ */
+struct dma_heap_ops {
+	int (*allocate)(struct dma_heap *heap,
+			struct dma_heap_buffer *buffer,
+			unsigned long len,
+			unsigned long flags);
+};
+
+/**
+ * struct dma_heap_info - holds information needed to create a DMA-heap
+ * @ops:		ops struct for this heap
+ * @name:		used for debugging/device-node name
+ */
+struct dma_heap_info {
+	const char *name;
+	struct dma_heap_ops *ops;
+};
+
+/**
+ * dma_heap_add - adds a heap to dmabuf heaps
+ * @heap:		the heap to add
+ */
+int dma_heap_add(struct dma_heap_info *heap_info);
+
+#endif /* _DMA_HEAPS_H */
diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
new file mode 100644
index 000000000000..7dcbb98e29ec
--- /dev/null
+++ b/include/uapi/linux/dma-heap.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * DMABUF Heaps Userspace API
+ *
+ * Copyright (C) 2011 Google, Inc.
+ * Copyright (C) 2019 Linaro Ltd.
+ */
+#ifndef _UAPI_LINUX_DMABUF_POOL_H
+#define _UAPI_LINUX_DMABUF_POOL_H
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+/**
+ * DOC: DMABUF Heaps Userspace API
+ *
+ */
+
+/*
+ * mappings of this buffer should be un-cached, otherwise dmabuf heaps will
+ * need cache maintenance using DMA_BUF_SYNC_* when the buffer is mapped for dma
+ */
+#define DMA_HEAP_FLAG_COHERENT 1
+
+/**
+ * struct dma_heap_allocation_data - metadata passed from userspace for
+ *                                      allocations
+ * @len:		size of the allocation
+ * @flags:		flags passed to pool
+ * @fd:			will be populated with a fd which provdes the
+ *			handle to the allocated dma-buf
+ *
+ * Provided by userspace as an argument to the ioctl
+ */
+struct dma_heap_allocation_data {
+	__u64 len;
+	__u32 flags;
+	__u32 fd;
+	__u32 reserved0;
+	__u32 reserved1;
+};
+
+#define DMA_HEAP_IOC_MAGIC		'H'
+
+/**
+ * DOC: DMA_HEAP_IOC_ALLOC - allocate memory from pool
+ *
+ * Takes an dma_heap_allocation_data struct and returns it with the fd field
+ * populated with the dmabuf handle of the allocation.
+ */
+#define DMA_HEAP_IOC_ALLOC	_IOWR(DMA_HEAP_IOC_MAGIC, 0, \
+				      struct dma_heap_allocation_data)
+
+#endif /* _UAPI_LINUX_DMABUF_POOL_H */
-- 
2.19.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-25 14:36 [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework Andrew F. Davis
@ 2019-02-25 18:41 ` John Stultz
  2019-02-26  0:20 ` John Stultz
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 27+ messages in thread
From: John Stultz @ 2019-02-25 18:41 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

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.

Overall looks pretty good!  I'll take a stab today at reworking my
stack on top of this starting patch.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-25 14:36 [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework Andrew F. Davis
  2019-02-25 18:41 ` John Stultz
@ 2019-02-26  0:20 ` John Stultz
  2019-02-26 14:02   ` Andrew F. Davis
  2019-02-26  0:55 ` John Stultz
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 27+ messages in thread
From: John Stultz @ 2019-02-26  0:20 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

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);

Plus the dma_heap duplicates the the dma_heap_info fields.

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().

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-25 14:36 [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework Andrew F. Davis
  2019-02-25 18:41 ` John Stultz
  2019-02-26  0:20 ` John Stultz
@ 2019-02-26  0:55 ` John Stultz
  2019-02-26 14:04   ` Andrew F. Davis
  2019-02-26  3:53 ` Sumit Semwal
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 27+ messages in thread
From: John Stultz @ 2019-02-26  0:55 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On Mon, Feb 25, 2019 at 6:36 AM Andrew F. Davis <afd@ti.com> wrote:
> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, unsigned int flags)
> +{
> +       struct dma_heap_buffer *buffer;
> +       int fd, ret;
> +
> +       buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +       if (!buffer)
> +               return -ENOMEM;
> +
> +       buffer->heap = heap;
> +       ret = heap->ops->allocate(heap, buffer, len, flags);
> +       if (ret) {
> +               kfree(buffer);
> +               return ret;
> +       }

Similarly, I think the struct dma_heap_buffer, should be allocated and
returned by the heap's allocate function.

That way it can allocate its own larger structure (with the
dma_heap_buffer as part of it) with private fields, and use
container_of() to traverse from the buffer to the private data.

Once I get things building, I'll share my changes.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-25 14:36 [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework Andrew F. Davis
                   ` (2 preceding siblings ...)
  2019-02-26  0:55 ` John Stultz
@ 2019-02-26  3:53 ` Sumit Semwal
  2019-02-26 14:28   ` Andrew F. Davis
  2019-02-26  6:20 ` John Stultz
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 27+ messages in thread
From: Sumit Semwal @ 2019-02-26  3:53 UTC (permalink / raw)
  To: Andrew F. Davis
  Cc: Chenbo Feng, Alistair Strachan, Liam Mark, DRI mailing list

On Mon, 25 Feb 2019 at 20:06, 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.

As an idea, I really like the direction for this. It gives a good
amount of flexibilty for exporters. So definitely thanks to John and
you for taking the plunge there :)

>
> 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.
>
Other than current heaps, the secure heaps have been talked about
quite a bit in the past, so I will check with Linaro's security group
on them trying out the next version as well.
We also would need to do a performance comparison, so that's another
activity to be added.
> Thanks,
> Andrew
Best,
Sumit.
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-25 14:36 [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework Andrew F. Davis
                   ` (3 preceding siblings ...)
  2019-02-26  3:53 ` Sumit Semwal
@ 2019-02-26  6:20 ` John Stultz
  2019-02-26 14:46   ` Andrew F. Davis
  2019-02-26 14:12 ` Benjamin Gaignard
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 27+ messages in thread
From: John Stultz @ 2019-02-26  6:20 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

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.

I took your patch here, made some small reworks as I mentioned
earlier, and tried to add some generic helpers and the system and cma
heaps integrated.

You can find the changes added on top of your patch here:
https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap

I propose we should squish down the first three changes down into the
core patch if you agree.

The helper functions need some work and cleanup, right now I'm only
exposing the dmabuf ops and realistically folks will probably want to
fill part of the ops with custom things.

It boots w/ AOSP, and allocations seem to work, but there's something
wrong with the dmabuf mmaping which casues the homescreen to crash
over and over.
(userland patches updated here:
https://android-review.googlesource.com/c/device/linaro/hikey/+/909436)

But that's about as far as I can do tonight, so I'm crashing and will
see what you think tomorrow. :)

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-26  0:20 ` John Stultz
@ 2019-02-26 14:02   ` Andrew F. Davis
  0 siblings, 0 replies; 27+ messages in thread
From: Andrew F. Davis @ 2019-02-26 14:02 UTC (permalink / raw)
  To: John Stultz; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

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
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-26  0:55 ` John Stultz
@ 2019-02-26 14:04   ` Andrew F. Davis
  0 siblings, 0 replies; 27+ messages in thread
From: Andrew F. Davis @ 2019-02-26 14:04 UTC (permalink / raw)
  To: John Stultz; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On 2/25/19 6:55 PM, John Stultz wrote:
> On Mon, Feb 25, 2019 at 6:36 AM Andrew F. Davis <afd@ti.com> wrote:
>> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, unsigned int flags)
>> +{
>> +       struct dma_heap_buffer *buffer;
>> +       int fd, ret;
>> +
>> +       buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
>> +       if (!buffer)
>> +               return -ENOMEM;
>> +
>> +       buffer->heap = heap;
>> +       ret = heap->ops->allocate(heap, buffer, len, flags);
>> +       if (ret) {
>> +               kfree(buffer);
>> +               return ret;
>> +       }
> 
> Similarly, I think the struct dma_heap_buffer, should be allocated and
> returned by the heap's allocate function.
> 

That sound good, we could also then remove the free in buffer_free(),
just wasn't sure about object lifetimes causing issues if it was freed
early and the framework still needed something from it.

Andrew

> That way it can allocate its own larger structure (with the
> dma_heap_buffer as part of it) with private fields, and use
> container_of() to traverse from the buffer to the private data.
> 
> Once I get things building, I'll share my changes.
> 
> thanks
> -john
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-25 14:36 [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework Andrew F. Davis
                   ` (4 preceding siblings ...)
  2019-02-26  6:20 ` John Stultz
@ 2019-02-26 14:12 ` Benjamin Gaignard
  2019-02-26 15:05   ` Andrew F. Davis
  2019-02-26 15:22   ` Linus Walleij
  2019-02-27 23:22 ` Laura Abbott
  2019-03-01 12:06 ` Brian Starkey
  7 siblings, 2 replies; 27+ messages in thread
From: Benjamin Gaignard @ 2019-02-26 14:12 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, ML dri-devel

Le lun. 25 févr. 2019 à 15:36, Andrew F. Davis <afd@ti.com> a écrit :
>
> 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

Overall  I like the idea but I think "dma_heap" will be confusing like
dma_buf is because it isn't
related to DMA engine.
I would like to see how this could be used with exiting allocator like
CMA or genalloc.

Benjamin

>
>  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

Why do you put MMU dependency ?

> +       select GENERIC_ALLOCATOR

Maybe I have miss it but I don't see the need to 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;
> +};
> +
> +/*
> + * This should be called by the heap exporter when a buffers dma-buf
> + * handle is released so the core can update the stats and release the
> + * buffer handle memory.
> + *
> + * Or we could find a way to hook into the fd or struct dma_buf itself?

If you remove stats from your code you will not need dma_heap_buffer structure
and this function will also because useless.

> + */
> +void dma_heap_buffer_free(struct dma_heap_buffer *buffer)
> +{
> +       spin_lock(&buffer->heap->stat_lock);
> +       buffer->heap->num_of_buffers--;
> +       buffer->heap->num_of_alloc_bytes -= buffer->size;
> +       spin_unlock(&buffer->heap->stat_lock);
> +
> +       kfree(buffer);
> +}
> +
> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, unsigned int flags)
> +{
> +       struct dma_heap_buffer *buffer;
> +       int fd, ret;
> +
> +       buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +       if (!buffer)
> +               return -ENOMEM;
> +
> +       buffer->heap = heap;
> +       ret = heap->ops->allocate(heap, buffer, len, flags);
> +       if (ret) {
> +               kfree(buffer);
> +               return ret;
> +       }
> +
> +       fd = dma_buf_fd(buffer->dmabuf, O_CLOEXEC);
> +       if (fd < 0) {
> +               dma_buf_put(buffer->dmabuf);
kfree(buffer);
> +               return fd;
> +       }
> +
> +       spin_lock(&heap->stat_lock);
> +       heap->num_of_buffers++;
> +       heap->num_of_alloc_bytes += buffer->size;
> +       if (heap->num_of_alloc_bytes > heap->alloc_bytes_wm)
> +               heap->alloc_bytes_wm = heap->num_of_alloc_bytes;
> +       spin_unlock(&heap->stat_lock);
> +
> +       return fd;
> +}
> +
> +static int dma_heap_open(struct inode *inode, struct file *filp)
> +{
> +       struct dma_heap *heap;
> +
> +       mutex_lock(&minor_lock);
> +       heap = idr_find(&dma_heap_idr, iminor(inode));
> +       mutex_unlock(&minor_lock);
> +       if (!heap) {
> +               pr_err("dma_heap: minor %d unknown.\n", iminor(inode));
> +               return -ENODEV;
> +       }
> +
> +       /* instance data as context */
> +       filp->private_data = heap;
> +       nonseekable_open(inode, filp);
> +
> +       return 0;
> +}
> +
> +static int dma_heap_release(struct inode *inode, struct file *filp)
> +{
> +       filp->private_data = NULL;
> +
> +       return 0;
> +}
> +
> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +       switch (cmd) {
> +       case DMA_HEAP_IOC_ALLOC:
> +       {
> +               struct dma_heap_allocation_data heap_allocation;
> +               struct dma_heap *heap = filp->private_data;
> +               int fd;
> +
> +               if (copy_from_user(&heap_allocation, (void __user *)arg, _IOC_SIZE(cmd)))
> +                       return -EFAULT;
> +
> +               if (heap_allocation.reserved0 ||
> +                   heap_allocation.reserved1) {
> +                       pr_warn_once("dma_heap: ioctl data not valid\n");
> +                       return -EINVAL;
> +               }
> +
> +               fd = dma_heap_buffer_alloc(heap, heap_allocation.len, heap_allocation.flags);
> +               if (fd < 0)
> +                       return fd;
> +
> +               heap_allocation.fd = fd;
> +
> +               if (copy_to_user((void __user *)arg, &heap_allocation, _IOC_SIZE(cmd)))
> +                       return -EFAULT;
> +
> +               break;
> +       }
> +       default:
> +               return -ENOTTY;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct file_operations dma_heap_fops = {
> +       .owner          = THIS_MODULE,
> +       .open           = dma_heap_open,
> +       .release        = dma_heap_release,
> +       .unlocked_ioctl = dma_heap_ioctl,
> +#ifdef CONFIG_COMPAT
> +       .compat_ioctl   = dma_heap_ioctl,
> +#endif
> +};
> +
> +int dma_heap_add(struct dma_heap_info *heap_info)
> +{
> +       struct dma_heap *heap;
> +       struct device *dev_ret;
> +       struct dentry *heap_root;
> +       int ret;
> +
> +       if (!heap_info->name || !strcmp(heap_info->name, "")) {
> +               pr_err("dma_heap: Cannot add heap without a name\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!heap_info->ops || !heap_info->ops->allocate) {
> +               pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
> +               return -EINVAL;
> +       }
> +
> +       heap = kzalloc(sizeof(*heap), GFP_KERNEL);
> +       if (!heap)
> +               return -ENOMEM;
> +
> +       heap->name = heap_info->name;
> +       memcpy(heap->ops, heap_info->ops, sizeof(*heap->ops));
> +
> +       /* Find unused minor number */
> +       mutex_lock(&minor_lock);
> +       ret = idr_alloc(&dma_heap_idr, heap, 0, NUM_HEAP_MINORS, GFP_KERNEL);
> +       mutex_unlock(&minor_lock);
> +       if (ret < 0) {
> +               pr_err("dma_heap: Unable to get minor number for heap\n");
kfree(heap);
> +               return ret;
> +       }
> +       heap->minor = ret;
> +
> +       /* Create device */
> +       heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
> +       dev_ret = device_create(dma_heap_class,
> +                               NULL,
> +                               heap->heap_devt,
> +                               NULL,
> +                               heap->name);
> +       if (IS_ERR(dev_ret)) {
> +               pr_err("dma_heap: Unable to create char device\n");
kfree(heap);
> +               return PTR_ERR(dev_ret);
> +       }
> +
> +       /* Add device */
> +       cdev_init(&heap->heap_cdev, &dma_heap_fops);
> +       ret = cdev_add(&heap->heap_cdev, dma_heap_devt, NUM_HEAP_MINORS);
> +       if (ret < 0) {
> +               device_destroy(dma_heap_class, heap->heap_devt);
> +               pr_err("dma_heap: Unable to add char device\n");
kfree(heap);
> +               return ret;
> +       }
> +
> +       heap->num_of_buffers = 0;
> +       heap->num_of_alloc_bytes = 0;
> +       heap->alloc_bytes_wm = 0;
> +       spin_lock_init(&heap->stat_lock);
> +       heap_root = debugfs_create_dir(heap->name, dma_heap_debug_root);
> +       debugfs_create_u64("num_of_buffers", 0444, heap_root, &heap->num_of_buffers);
> +       debugfs_create_u64("num_of_alloc_bytes", 0444, heap_root, &heap->num_of_alloc_bytes);
> +       debugfs_create_u64("alloc_bytes_wm", 0444, heap_root, &heap->alloc_bytes_wm);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(dma_heap_add);
> +
> +static int dma_heap_init(void)
> +{
> +       int ret;
> +
> +       ret = alloc_chrdev_region(&dma_heap_devt, 0, NUM_HEAP_MINORS, DEVNAME);
> +       if (ret)
> +               return ret;
> +
> +       dma_heap_class = class_create(THIS_MODULE, DEVNAME);
> +       if (IS_ERR(dma_heap_class)) {
> +               unregister_chrdev_region(dma_heap_devt, NUM_HEAP_MINORS);
> +               return PTR_ERR(dma_heap_class);
> +       }
> +
> +       dma_heap_debug_root = debugfs_create_dir("dma_heap", NULL);
> +
> +       return 0;
> +}
> +subsys_initcall(dma_heap_init);
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> new file mode 100644
> index 000000000000..09eab3105118
> --- /dev/null
> +++ b/include/linux/dma-heap.h
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF Heaps Allocation Infrastructure
> + *
> + * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2019 Linaro Ltd.
> + */
> +
> +#ifndef _DMA_HEAPS_H
> +#define _DMA_HEAPS_H
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct dma_heap_buffer - metadata for a particular buffer
> + * @heap:              back pointer to the heap the buffer came from
> + * @dmabuf:            backing dma-buf for this buffer
> + * @size:              size of the buffer
> + * @flags:             buffer specific flags
> + */
> +struct dma_heap_buffer {
> +       struct dma_heap *heap;
> +       struct dma_buf *dmabuf;
> +       size_t size;
> +       unsigned long flags;
> +};
> +
> +/**
> + * struct dma_heap_ops - ops to operate on a given heap
> + * @allocate:          allocate buffer
> + *
> + * allocate returns 0 on success, -errno on error.
> + */
> +struct dma_heap_ops {
> +       int (*allocate)(struct dma_heap *heap,
> +                       struct dma_heap_buffer *buffer,
> +                       unsigned long len,
> +                       unsigned long flags);
> +};
> +
> +/**
> + * struct dma_heap_info - holds information needed to create a DMA-heap
> + * @ops:               ops struct for this heap
> + * @name:              used for debugging/device-node name
> + */
> +struct dma_heap_info {
> +       const char *name;
> +       struct dma_heap_ops *ops;
> +};
> +
> +/**
> + * dma_heap_add - adds a heap to dmabuf heaps
> + * @heap:              the heap to add
> + */
> +int dma_heap_add(struct dma_heap_info *heap_info);
> +
> +#endif /* _DMA_HEAPS_H */
> diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
> new file mode 100644
> index 000000000000..7dcbb98e29ec
> --- /dev/null
> +++ b/include/uapi/linux/dma-heap.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * DMABUF Heaps Userspace API
> + *
> + * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2019 Linaro Ltd.
> + */
> +#ifndef _UAPI_LINUX_DMABUF_POOL_H
> +#define _UAPI_LINUX_DMABUF_POOL_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/**
> + * DOC: DMABUF Heaps Userspace API
> + *
> + */
> +
> +/*
> + * mappings of this buffer should be un-cached, otherwise dmabuf heaps will
> + * need cache maintenance using DMA_BUF_SYNC_* when the buffer is mapped for dma
> + */
> +#define DMA_HEAP_FLAG_COHERENT 1
> +
> +/**
> + * struct dma_heap_allocation_data - metadata passed from userspace for
> + *                                      allocations
> + * @len:               size of the allocation
> + * @flags:             flags passed to pool
> + * @fd:                        will be populated with a fd which provdes the
> + *                     handle to the allocated dma-buf
> + *
> + * Provided by userspace as an argument to the ioctl
> + */
> +struct dma_heap_allocation_data {
you could add an __u64 version field that could help if API change in the future

> +       __u64 len;
> +       __u32 flags;
> +       __u32 fd;
> +       __u32 reserved0;
> +       __u32 reserved1;
> +};
> +
> +#define DMA_HEAP_IOC_MAGIC             'H'
> +
> +/**
> + * DOC: DMA_HEAP_IOC_ALLOC - allocate memory from pool
> + *
> + * Takes an dma_heap_allocation_data struct and returns it with the fd field
> + * populated with the dmabuf handle of the allocation.
> + */
> +#define DMA_HEAP_IOC_ALLOC     _IOWR(DMA_HEAP_IOC_MAGIC, 0, \
> +                                     struct dma_heap_allocation_data)
> +
> +#endif /* _UAPI_LINUX_DMABUF_POOL_H */
> --
> 2.19.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-26  3:53 ` Sumit Semwal
@ 2019-02-26 14:28   ` Andrew F. Davis
  0 siblings, 0 replies; 27+ messages in thread
From: Andrew F. Davis @ 2019-02-26 14:28 UTC (permalink / raw)
  To: Sumit Semwal; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, DRI mailing list

On 2/25/19 9:53 PM, Sumit Semwal wrote:
> On Mon, 25 Feb 2019 at 20:06, 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.
> 
> As an idea, I really like the direction for this. It gives a good
> amount of flexibilty for exporters. So definitely thanks to John and
> you for taking the plunge there :)
> 
>>
>> 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.
>>
> Other than current heaps, the secure heaps have been talked about
> quite a bit in the past, so I will check with Linaro's security group
> on them trying out the next version as well.

As I also moonlight as a Linaro member engineer as part of SWG
(andrew.davis@linaro.org) I've talked with Joakim and Etienne about the
secure (unmapped) heaps, adding those are already baked into my plans
here :)

Andrew

> We also would need to do a performance comparison, so that's another
> activity to be added.
>> Thanks,
>> Andrew
> Best,
> Sumit.
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-26  6:20 ` John Stultz
@ 2019-02-26 14:46   ` Andrew F. Davis
  2019-02-26 19:21     ` John Stultz
  0 siblings, 1 reply; 27+ messages in thread
From: Andrew F. Davis @ 2019-02-26 14:46 UTC (permalink / raw)
  To: John Stultz; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On 2/26/19 12:20 AM, 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.
> 
> I took your patch here, made some small reworks as I mentioned
> earlier, and tried to add some generic helpers and the system and cma
> heaps integrated.
> 
> You can find the changes added on top of your patch here:
> https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap
> 
> I propose we should squish down the first three changes down into the
> core patch if you agree.
> 

All looks good to me. One thing I'm still unsure of is moving struct
dma_heap out into the public interface header. That struct contains
framework internal only info (device nodes, stats, etc.) that should not
be exposed to the outside world. I think of them as private members of
our class (too much C++ work lately), if we need to expose any members
of that struct then we can add accessors as needed.

> The helper functions need some work and cleanup, right now I'm only
> exposing the dmabuf ops and realistically folks will probably want to
> fill part of the ops with custom things.
> 

Yes, the current set of helper dma-buf-ops work well for pre-allocated
page-struct-backed memory buffers (basically sg list). For buffers
allocated outside of normal RAM, secure (unmapped) heaps, and attach
time allocated heaps all kinda break down with the current helpers. But
that's the best part of helpers, you don't need to use them if you don't
want :)

> It boots w/ AOSP, and allocations seem to work, but there's something
> wrong with the dmabuf mmaping which casues the homescreen to crash
> over and over.
> (userland patches updated here:
> https://android-review.googlesource.com/c/device/linaro/hikey/+/909436)
> 

Interesting, I wonder if the caching stuff is not right here, I'll see
if I can get this working on my side (AOSP on Beagle x15).

Thanks,
Andrew

> But that's about as far as I can do tonight, so I'm crashing and will
> see what you think tomorrow. :)
> 
> thanks
> -john
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-26 14:12 ` Benjamin Gaignard
@ 2019-02-26 15:05   ` Andrew F. Davis
  2019-02-26 15:22   ` Linus Walleij
  1 sibling, 0 replies; 27+ messages in thread
From: Andrew F. Davis @ 2019-02-26 15:05 UTC (permalink / raw)
  To: Benjamin Gaignard; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, ML dri-devel

On 2/26/19 8:12 AM, Benjamin Gaignard wrote:
> Le lun. 25 févr. 2019 à 15:36, Andrew F. Davis <afd@ti.com> a écrit :
>>
>> 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
> 
> Overall  I like the idea but I think "dma_heap" will be confusing like
> dma_buf is because it isn't
> related to DMA engine.

The intention was to keep things consistent within dma_buf and dma_fence
names, originally this was dmabuf_heap, right before I posted I did a
mass rename, so if there is push back here then it can always be mass
renamed again.

> I would like to see how this could be used with exiting allocator like
> CMA or genalloc.
> 
> Benjamin
> 
>>
>>  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
> 
> Why do you put MMU dependency ?
> 
>> +       select GENERIC_ALLOCATOR
> 
> Maybe I have miss it but I don't see the need to select GENERIC_ALLOCATOR
> 

For the two above they are leftover from ION when the heaps where more
tightly coupled with the core, these can now be moved out to the
individual heaps that use them.

>> +       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;
>> +};
>> +
>> +/*
>> + * This should be called by the heap exporter when a buffers dma-buf
>> + * handle is released so the core can update the stats and release the
>> + * buffer handle memory.
>> + *
>> + * Or we could find a way to hook into the fd or struct dma_buf itself?
> 
> If you remove stats from your code you will not need dma_heap_buffer structure
> and this function will also because useless.
> 

The issue is that at some point we will probably want to keep more
per-buffer data in the core framework than just stats, and I didn't want
to block that in the design so I kept stats as an example of per-buffer
data tracking. If we don't think we will need to keep track of the
buffers inside the heaps and only track the heaps themselves then yes
this can be removed.

>> + */
>> +void dma_heap_buffer_free(struct dma_heap_buffer *buffer)
>> +{
>> +       spin_lock(&buffer->heap->stat_lock);
>> +       buffer->heap->num_of_buffers--;
>> +       buffer->heap->num_of_alloc_bytes -= buffer->size;
>> +       spin_unlock(&buffer->heap->stat_lock);
>> +
>> +       kfree(buffer);
>> +}
>> +
>> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, unsigned int flags)
>> +{
>> +       struct dma_heap_buffer *buffer;
>> +       int fd, ret;
>> +
>> +       buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
>> +       if (!buffer)
>> +               return -ENOMEM;
>> +
>> +       buffer->heap = heap;
>> +       ret = heap->ops->allocate(heap, buffer, len, flags);
>> +       if (ret) {
>> +               kfree(buffer);
>> +               return ret;
>> +       }
>> +
>> +       fd = dma_buf_fd(buffer->dmabuf, O_CLOEXEC);
>> +       if (fd < 0) {
>> +               dma_buf_put(buffer->dmabuf);
> kfree(buffer);
>> +               return fd;
>> +       }
>> +
>> +       spin_lock(&heap->stat_lock);
>> +       heap->num_of_buffers++;
>> +       heap->num_of_alloc_bytes += buffer->size;
>> +       if (heap->num_of_alloc_bytes > heap->alloc_bytes_wm)
>> +               heap->alloc_bytes_wm = heap->num_of_alloc_bytes;
>> +       spin_unlock(&heap->stat_lock);
>> +
>> +       return fd;
>> +}
>> +
>> +static int dma_heap_open(struct inode *inode, struct file *filp)
>> +{
>> +       struct dma_heap *heap;
>> +
>> +       mutex_lock(&minor_lock);
>> +       heap = idr_find(&dma_heap_idr, iminor(inode));
>> +       mutex_unlock(&minor_lock);
>> +       if (!heap) {
>> +               pr_err("dma_heap: minor %d unknown.\n", iminor(inode));
>> +               return -ENODEV;
>> +       }
>> +
>> +       /* instance data as context */
>> +       filp->private_data = heap;
>> +       nonseekable_open(inode, filp);
>> +
>> +       return 0;
>> +}
>> +
>> +static int dma_heap_release(struct inode *inode, struct file *filp)
>> +{
>> +       filp->private_data = NULL;
>> +
>> +       return 0;
>> +}
>> +
>> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> +{
>> +       switch (cmd) {
>> +       case DMA_HEAP_IOC_ALLOC:
>> +       {
>> +               struct dma_heap_allocation_data heap_allocation;
>> +               struct dma_heap *heap = filp->private_data;
>> +               int fd;
>> +
>> +               if (copy_from_user(&heap_allocation, (void __user *)arg, _IOC_SIZE(cmd)))
>> +                       return -EFAULT;
>> +
>> +               if (heap_allocation.reserved0 ||
>> +                   heap_allocation.reserved1) {
>> +                       pr_warn_once("dma_heap: ioctl data not valid\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               fd = dma_heap_buffer_alloc(heap, heap_allocation.len, heap_allocation.flags);
>> +               if (fd < 0)
>> +                       return fd;
>> +
>> +               heap_allocation.fd = fd;
>> +
>> +               if (copy_to_user((void __user *)arg, &heap_allocation, _IOC_SIZE(cmd)))
>> +                       return -EFAULT;
>> +
>> +               break;
>> +       }
>> +       default:
>> +               return -ENOTTY;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct file_operations dma_heap_fops = {
>> +       .owner          = THIS_MODULE,
>> +       .open           = dma_heap_open,
>> +       .release        = dma_heap_release,
>> +       .unlocked_ioctl = dma_heap_ioctl,
>> +#ifdef CONFIG_COMPAT
>> +       .compat_ioctl   = dma_heap_ioctl,
>> +#endif
>> +};
>> +
>> +int dma_heap_add(struct dma_heap_info *heap_info)
>> +{
>> +       struct dma_heap *heap;
>> +       struct device *dev_ret;
>> +       struct dentry *heap_root;
>> +       int ret;
>> +
>> +       if (!heap_info->name || !strcmp(heap_info->name, "")) {
>> +               pr_err("dma_heap: Cannot add heap without a name\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!heap_info->ops || !heap_info->ops->allocate) {
>> +               pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       heap = kzalloc(sizeof(*heap), GFP_KERNEL);
>> +       if (!heap)
>> +               return -ENOMEM;
>> +
>> +       heap->name = heap_info->name;
>> +       memcpy(heap->ops, heap_info->ops, sizeof(*heap->ops));
>> +
>> +       /* Find unused minor number */
>> +       mutex_lock(&minor_lock);
>> +       ret = idr_alloc(&dma_heap_idr, heap, 0, NUM_HEAP_MINORS, GFP_KERNEL);
>> +       mutex_unlock(&minor_lock);
>> +       if (ret < 0) {
>> +               pr_err("dma_heap: Unable to get minor number for heap\n");
> kfree(heap);
>> +               return ret;
>> +       }
>> +       heap->minor = ret;
>> +
>> +       /* Create device */
>> +       heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
>> +       dev_ret = device_create(dma_heap_class,
>> +                               NULL,
>> +                               heap->heap_devt,
>> +                               NULL,
>> +                               heap->name);
>> +       if (IS_ERR(dev_ret)) {
>> +               pr_err("dma_heap: Unable to create char device\n");
> kfree(heap);
>> +               return PTR_ERR(dev_ret);
>> +       }
>> +
>> +       /* Add device */
>> +       cdev_init(&heap->heap_cdev, &dma_heap_fops);
>> +       ret = cdev_add(&heap->heap_cdev, dma_heap_devt, NUM_HEAP_MINORS);
>> +       if (ret < 0) {
>> +               device_destroy(dma_heap_class, heap->heap_devt);
>> +               pr_err("dma_heap: Unable to add char device\n");
> kfree(heap);

ACK for all the kfree()'s

>> +               return ret;
>> +       }
>> +
>> +       heap->num_of_buffers = 0;
>> +       heap->num_of_alloc_bytes = 0;
>> +       heap->alloc_bytes_wm = 0;
>> +       spin_lock_init(&heap->stat_lock);
>> +       heap_root = debugfs_create_dir(heap->name, dma_heap_debug_root);
>> +       debugfs_create_u64("num_of_buffers", 0444, heap_root, &heap->num_of_buffers);
>> +       debugfs_create_u64("num_of_alloc_bytes", 0444, heap_root, &heap->num_of_alloc_bytes);
>> +       debugfs_create_u64("alloc_bytes_wm", 0444, heap_root, &heap->alloc_bytes_wm);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(dma_heap_add);
>> +
>> +static int dma_heap_init(void)
>> +{
>> +       int ret;
>> +
>> +       ret = alloc_chrdev_region(&dma_heap_devt, 0, NUM_HEAP_MINORS, DEVNAME);
>> +       if (ret)
>> +               return ret;
>> +
>> +       dma_heap_class = class_create(THIS_MODULE, DEVNAME);
>> +       if (IS_ERR(dma_heap_class)) {
>> +               unregister_chrdev_region(dma_heap_devt, NUM_HEAP_MINORS);
>> +               return PTR_ERR(dma_heap_class);
>> +       }
>> +
>> +       dma_heap_debug_root = debugfs_create_dir("dma_heap", NULL);
>> +
>> +       return 0;
>> +}
>> +subsys_initcall(dma_heap_init);
>> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
>> new file mode 100644
>> index 000000000000..09eab3105118
>> --- /dev/null
>> +++ b/include/linux/dma-heap.h
>> @@ -0,0 +1,57 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * DMABUF Heaps Allocation Infrastructure
>> + *
>> + * Copyright (C) 2011 Google, Inc.
>> + * Copyright (C) 2019 Linaro Ltd.
>> + */
>> +
>> +#ifndef _DMA_HEAPS_H
>> +#define _DMA_HEAPS_H
>> +
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct dma_heap_buffer - metadata for a particular buffer
>> + * @heap:              back pointer to the heap the buffer came from
>> + * @dmabuf:            backing dma-buf for this buffer
>> + * @size:              size of the buffer
>> + * @flags:             buffer specific flags
>> + */
>> +struct dma_heap_buffer {
>> +       struct dma_heap *heap;
>> +       struct dma_buf *dmabuf;
>> +       size_t size;
>> +       unsigned long flags;
>> +};
>> +
>> +/**
>> + * struct dma_heap_ops - ops to operate on a given heap
>> + * @allocate:          allocate buffer
>> + *
>> + * allocate returns 0 on success, -errno on error.
>> + */
>> +struct dma_heap_ops {
>> +       int (*allocate)(struct dma_heap *heap,
>> +                       struct dma_heap_buffer *buffer,
>> +                       unsigned long len,
>> +                       unsigned long flags);
>> +};
>> +
>> +/**
>> + * struct dma_heap_info - holds information needed to create a DMA-heap
>> + * @ops:               ops struct for this heap
>> + * @name:              used for debugging/device-node name
>> + */
>> +struct dma_heap_info {
>> +       const char *name;
>> +       struct dma_heap_ops *ops;
>> +};
>> +
>> +/**
>> + * dma_heap_add - adds a heap to dmabuf heaps
>> + * @heap:              the heap to add
>> + */
>> +int dma_heap_add(struct dma_heap_info *heap_info);
>> +
>> +#endif /* _DMA_HEAPS_H */
>> diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
>> new file mode 100644
>> index 000000000000..7dcbb98e29ec
>> --- /dev/null
>> +++ b/include/uapi/linux/dma-heap.h
>> @@ -0,0 +1,54 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * DMABUF Heaps Userspace API
>> + *
>> + * Copyright (C) 2011 Google, Inc.
>> + * Copyright (C) 2019 Linaro Ltd.
>> + */
>> +#ifndef _UAPI_LINUX_DMABUF_POOL_H
>> +#define _UAPI_LINUX_DMABUF_POOL_H
>> +
>> +#include <linux/ioctl.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * DOC: DMABUF Heaps Userspace API
>> + *
>> + */
>> +
>> +/*
>> + * mappings of this buffer should be un-cached, otherwise dmabuf heaps will
>> + * need cache maintenance using DMA_BUF_SYNC_* when the buffer is mapped for dma
>> + */
>> +#define DMA_HEAP_FLAG_COHERENT 1
>> +
>> +/**
>> + * struct dma_heap_allocation_data - metadata passed from userspace for
>> + *                                      allocations
>> + * @len:               size of the allocation
>> + * @flags:             flags passed to pool
>> + * @fd:                        will be populated with a fd which provdes the
>> + *                     handle to the allocated dma-buf
>> + *
>> + * Provided by userspace as an argument to the ioctl
>> + */
>> +struct dma_heap_allocation_data {
> you could add an __u64 version field that could help if API change in the future
> 

This was brought up before when the same was attempted in ION [0], seems
versioning ioctls is frowned upon, something to do with it enabling the
bad practice of making backwards incompatible changes easier.

[0] https://patchwork.kernel.org/patch/10816693/

Thanks,
Andrew

>> +       __u64 len;
>> +       __u32 flags;
>> +       __u32 fd;
>> +       __u32 reserved0;
>> +       __u32 reserved1;
>> +};
>> +
>> +#define DMA_HEAP_IOC_MAGIC             'H'
>> +
>> +/**
>> + * DOC: DMA_HEAP_IOC_ALLOC - allocate memory from pool
>> + *
>> + * Takes an dma_heap_allocation_data struct and returns it with the fd field
>> + * populated with the dmabuf handle of the allocation.
>> + */
>> +#define DMA_HEAP_IOC_ALLOC     _IOWR(DMA_HEAP_IOC_MAGIC, 0, \
>> +                                     struct dma_heap_allocation_data)
>> +
>> +#endif /* _UAPI_LINUX_DMABUF_POOL_H */
>> --
>> 2.19.1
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-26 14:12 ` Benjamin Gaignard
  2019-02-26 15:05   ` Andrew F. Davis
@ 2019-02-26 15:22   ` Linus Walleij
  1 sibling, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2019-02-26 15:22 UTC (permalink / raw)
  To: Benjamin Gaignard
  Cc: ML dri-devel, Chenbo Feng, Alistair Strachan, Liam Mark, Andrew F. Davis

On Tue, Feb 26, 2019 at 3:12 PM Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:

> Overall  I like the idea but I think "dma_heap" will be confusing like
> dma_buf is because it isn't
> related to DMA engine.

It wouldn't be the only place, in Device Tree we use "dma-ranges" in the
PCI bus host bindings to define how the PCI devices see the memory
on the host system, and it is called like that since the PCI devices indeed
perform direct memory access. And this is even system agnostic
and cannot be changed.

In the kernel we use dma_alloc_coherent() for allocating framebuffers
though what we want is physically contiguous.

I think it's a lost cause, we just have to remind everyone once a week
that DMA means "some device or component accessing the memory
directly without involvement of the CPU. Such as a display scan-out,
not to be confused with DMA engines".

FWIW it might be easier to rename DMA engines to "memmove and
stream" or "blitters". :/

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-26 14:46   ` Andrew F. Davis
@ 2019-02-26 19:21     ` John Stultz
  2019-02-26 23:40       ` John Stultz
  0 siblings, 1 reply; 27+ messages in thread
From: John Stultz @ 2019-02-26 19:21 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On Tue, Feb 26, 2019 at 6:47 AM Andrew F. Davis <afd@ti.com> wrote:
> On 2/26/19 12:20 AM, John Stultz wrote:
> > On Mon, Feb 25, 2019 at 6:36 AM Andrew F. Davis <afd@ti.com> wrote:
> >> 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.
> >
> > I took your patch here, made some small reworks as I mentioned
> > earlier, and tried to add some generic helpers and the system and cma
> > heaps integrated.
> >
> > You can find the changes added on top of your patch here:
> > https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap
> >
> > I propose we should squish down the first three changes down into the
> > core patch if you agree.
> >
>
> All looks good to me. One thing I'm still unsure of is moving struct
> dma_heap out into the public interface header. That struct contains
> framework internal only info (device nodes, stats, etc.) that should not
> be exposed to the outside world. I think of them as private members of
> our class (too much C++ work lately), if we need to expose any members
> of that struct then we can add accessors as needed.

Hrm. The trouble is that the heaps need some way to index back to
their own per-heap data (similar to how the heaps also need their own
per-buffer data) like the cma struct for the specific cma region, for
instance. Maybe this doesn't address your concern, but one way I've
dealt with "subsystem internal" structures is to have local .h files,
so only the heap implementations can see it.  I may be missing a
different approach that your thinking of, so please let me know. :)

> > The helper functions need some work and cleanup, right now I'm only
> > exposing the dmabuf ops and realistically folks will probably want to
> > fill part of the ops with custom things.
> >
>
> Yes, the current set of helper dma-buf-ops work well for pre-allocated
> page-struct-backed memory buffers (basically sg list). For buffers
> allocated outside of normal RAM, secure (unmapped) heaps, and attach
> time allocated heaps all kinda break down with the current helpers. But
> that's the best part of helpers, you don't need to use them if you don't
> want :)
>
> > It boots w/ AOSP, and allocations seem to work, but there's something
> > wrong with the dmabuf mmaping which casues the homescreen to crash
> > over and over.
> > (userland patches updated here:
> > https://android-review.googlesource.com/c/device/linaro/hikey/+/909436)
> >
>
> Interesting, I wonder if the caching stuff is not right here, I'll see
> if I can get this working on my side (AOSP on Beagle x15).
>

Let me know if you figure anything out, I'll also be looking at this today.

I also realized some of Benajmin's error path improvements are going
to be hard to fix w/ my current code, specifically having the allocate
op do the allocation of the dma_heap_buffer (since we don't have a
free op, if something fails creating the dmabuf fd in the core, we
don't have a hook to release the dma_heap_buffer and heap private
data).

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-26 19:21     ` John Stultz
@ 2019-02-26 23:40       ` John Stultz
  2019-02-27 16:38         ` Andrew F. Davis
  0 siblings, 1 reply; 27+ messages in thread
From: John Stultz @ 2019-02-26 23:40 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On Tue, Feb 26, 2019 at 11:21 AM John Stultz <john.stultz@linaro.org> wrote:
>
> On Tue, Feb 26, 2019 at 6:47 AM Andrew F. Davis <afd@ti.com> wrote:
> > On 2/26/19 12:20 AM, John Stultz wrote:
> > > It boots w/ AOSP, and allocations seem to work, but there's something
> > > wrong with the dmabuf mmaping which casues the homescreen to crash
> > > over and over.
> > > (userland patches updated here:
> > > https://android-review.googlesource.com/c/device/linaro/hikey/+/909436)
> > >
> >
> > Interesting, I wonder if the caching stuff is not right here, I'll see
> > if I can get this working on my side (AOSP on Beagle x15).
> >
>
> Let me know if you figure anything out, I'll also be looking at this today.
>

Ok. Figured out the issue. There was a missing:
  len = PAGE_ALIGN(len)
assignment that the core used to do before calling the heap alloc op.

Adding that into the heap alloc op got it booting ok w/ AOSP.

I've updated the patches here:
kernel: https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap
userland: https://android-review.googlesource.com/c/device/linaro/hikey/+/909436


> I also realized some of Benajmin's error path improvements are going
> to be hard to fix w/ my current code, specifically having the allocate
> op do the allocation of the dma_heap_buffer (since we don't have a
> free op, if something fails creating the dmabuf fd in the core, we
> don't have a hook to release the dma_heap_buffer and heap private
> data).

I also realized doing my development and testing against my
hikey960-mainline-WIP branch, I accidentally folded in an ion hack I
was using to reduce cache operations, so I'll need to undo that in the
heap-helpers.c.  But at least we have a rough validation point for the
design.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-26 23:40       ` John Stultz
@ 2019-02-27 16:38         ` Andrew F. Davis
  2019-02-27 21:55           ` John Stultz
  0 siblings, 1 reply; 27+ messages in thread
From: Andrew F. Davis @ 2019-02-27 16:38 UTC (permalink / raw)
  To: John Stultz; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On 2/26/19 5:40 PM, John Stultz wrote:
> On Tue, Feb 26, 2019 at 11:21 AM John Stultz <john.stultz@linaro.org> wrote:
>>
>> On Tue, Feb 26, 2019 at 6:47 AM Andrew F. Davis <afd@ti.com> wrote:
>>> On 2/26/19 12:20 AM, John Stultz wrote:
>>>> It boots w/ AOSP, and allocations seem to work, but there's something
>>>> wrong with the dmabuf mmaping which casues the homescreen to crash
>>>> over and over.
>>>> (userland patches updated here:
>>>> https://android-review.googlesource.com/c/device/linaro/hikey/+/909436)
>>>>
>>>
>>> Interesting, I wonder if the caching stuff is not right here, I'll see
>>> if I can get this working on my side (AOSP on Beagle x15).
>>>
>>
>> Let me know if you figure anything out, I'll also be looking at this today.
>>
> 
> Ok. Figured out the issue. There was a missing:
>   len = PAGE_ALIGN(len)
> assignment that the core used to do before calling the heap alloc op.
> 

Ah, that was my bad then, I dropped that line thinking the heaps should
take care of it, forcing allocation to the page length does make sense
though, we cant pass any less back to userspace.

> Adding that into the heap alloc op got it booting ok w/ AOSP.
> 
> I've updated the patches here:
> kernel: https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap
> userland: https://android-review.googlesource.com/c/device/linaro/hikey/+/909436
> 
> 
>> I also realized some of Benajmin's error path improvements are going
>> to be hard to fix w/ my current code, specifically having the allocate
>> op do the allocation of the dma_heap_buffer (since we don't have a
>> free op, if something fails creating the dmabuf fd in the core, we
>> don't have a hook to release the dma_heap_buffer and heap private
>> data).
> 

We can always add back the free op, the alternative is to have the heap
export the fd.

I'm not sure either is needed though, when we
dma_buf_put(buffer->dmabuf) on the error path it should trigger the
release op, and that can cleanup the allocations in the heap.

> I also realized doing my development and testing against my
> hikey960-mainline-WIP branch, I accidentally folded in an ion hack I
> was using to reduce cache operations, so I'll need to undo that in the
> heap-helpers.c.  But at least we have a rough validation point for the
> design.
> 

Great! The details of the heap-helpers can always get fixed up at a
later point, validation of the core working is really good to hear.

Thanks,
Andrew

> thanks
> -john
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-27 16:38         ` Andrew F. Davis
@ 2019-02-27 21:55           ` John Stultz
  2019-02-28 15:20             ` Andrew F. Davis
  0 siblings, 1 reply; 27+ messages in thread
From: John Stultz @ 2019-02-27 21:55 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On Wed, Feb 27, 2019 at 8:38 AM Andrew F. Davis <afd@ti.com> wrote:
>
> On 2/26/19 5:40 PM, John Stultz wrote:
> > On Tue, Feb 26, 2019 at 11:21 AM John Stultz <john.stultz@linaro.org> wrote:
> > I've updated the patches here:
> > kernel: https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap
> > userland: https://android-review.googlesource.com/c/device/linaro/hikey/+/909436
> >
> >
> >> I also realized some of Benajmin's error path improvements are going
> >> to be hard to fix w/ my current code, specifically having the allocate
> >> op do the allocation of the dma_heap_buffer (since we don't have a
> >> free op, if something fails creating the dmabuf fd in the core, we
> >> don't have a hook to release the dma_heap_buffer and heap private
> >> data).
> >
>
> We can always add back the free op, the alternative is to have the heap
> export the fd.
>
> I'm not sure either is needed though, when we
> dma_buf_put(buffer->dmabuf) on the error path it should trigger the
> release op, and that can cleanup the allocations in the heap.

Good point, but I worry that's a bit subtle.

Also doing the stuff with the helpers where we have to register a free
callback is kind of ugly, and I personally like the symmetry of having
free hooks if we have allocation hooks (even if the dmabuf release
hook initiates the free call).

> > I also realized doing my development and testing against my
> > hikey960-mainline-WIP branch, I accidentally folded in an ion hack I
> > was using to reduce cache operations, so I'll need to undo that in the
> > heap-helpers.c.  But at least we have a rough validation point for the
> > design.
> >
>
> Great! The details of the heap-helpers can always get fixed up at a
> later point, validation of the core working is really good to hear.
>

Let me know if you have any further feedback or changes to integrate.
I've got to get back to some other work, but will try to take a
cleanup pass in the next few days.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-25 14:36 [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework Andrew F. Davis
                   ` (5 preceding siblings ...)
  2019-02-26 14:12 ` Benjamin Gaignard
@ 2019-02-27 23:22 ` Laura Abbott
  2019-02-28  0:14   ` John Stultz
  2019-03-01 12:06 ` Brian Starkey
  7 siblings, 1 reply; 27+ messages in thread
From: Laura Abbott @ 2019-02-27 23:22 UTC (permalink / raw)
  To: Andrew F. Davis, John Stultz, Sumit Semwal, Benjamin Gaignard,
	Liam Mark, Brian Starkey, Chenbo Feng, Alistair Strachan
  Cc: dri-devel

On 2/25/19 6:36 AM, Andrew F. Davis 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.
> 

I like this concept and I'm happy to see it go forward. Some high level
comments

> 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;

Can we make sure this gets reviewed by Greg sooner rather than
later when we drop the RFC? I think the use of this here
is fine with the device model but I'd rather not find out at
the end.

> +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;
> +};
> +
> +/*
> + * This should be called by the heap exporter when a buffers dma-buf
> + * handle is released so the core can update the stats and release the
> + * buffer handle memory.
> + *
> + * Or we could find a way to hook into the fd or struct dma_buf itself?
> + */
> +void dma_heap_buffer_free(struct dma_heap_buffer *buffer)
> +{
> +	spin_lock(&buffer->heap->stat_lock);
> +	buffer->heap->num_of_buffers--;
> +	buffer->heap->num_of_alloc_bytes -= buffer->size;
> +	spin_unlock(&buffer->heap->stat_lock);
> +
> +	kfree(buffer);
> +}
> +
> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, unsigned int flags)
> +{
> +	struct dma_heap_buffer *buffer;
> +	int fd, ret;
> +
> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	buffer->heap = heap;
> +	ret = heap->ops->allocate(heap, buffer, len, flags);
> +	if (ret) {
> +		kfree(buffer);
> +		return ret;
> +	}
> +
> +	fd = dma_buf_fd(buffer->dmabuf, O_CLOEXEC);
> +	if (fd < 0) {
> +		dma_buf_put(buffer->dmabuf);
> +		return fd;
> +	}
> +
> +	spin_lock(&heap->stat_lock);
> +	heap->num_of_buffers++;
> +	heap->num_of_alloc_bytes += buffer->size;
> +	if (heap->num_of_alloc_bytes > heap->alloc_bytes_wm)
> +		heap->alloc_bytes_wm = heap->num_of_alloc_bytes;
> +	spin_unlock(&heap->stat_lock);
> +
> +	return fd;
> +}
> +
> +static int dma_heap_open(struct inode *inode, struct file *filp)
> +{
> +	struct dma_heap *heap;
> +
> +	mutex_lock(&minor_lock);
> +	heap = idr_find(&dma_heap_idr, iminor(inode));
> +	mutex_unlock(&minor_lock);
> +	if (!heap) {
> +		pr_err("dma_heap: minor %d unknown.\n", iminor(inode));
> +		return -ENODEV;
> +	}
> +
> +	/* instance data as context */
> +	filp->private_data = heap;
> +	nonseekable_open(inode, filp);
> +
> +	return 0;
> +}
> +
> +static int dma_heap_release(struct inode *inode, struct file *filp)
> +{
> +	filp->private_data = NULL;
> +
> +	return 0;
> +}
> +
> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +	case DMA_HEAP_IOC_ALLOC:
> +	{
> +		struct dma_heap_allocation_data heap_allocation;
> +		struct dma_heap *heap = filp->private_data;
> +		int fd;
> +
> +		if (copy_from_user(&heap_allocation, (void __user *)arg, _IOC_SIZE(cmd)))
> +			return -EFAULT;
> +
> +		if (heap_allocation.reserved0 ||
> +		    heap_allocation.reserved1) {
> +			pr_warn_once("dma_heap: ioctl data not valid\n");
> +			return -EINVAL;
> +		}
> +
> +		fd = dma_heap_buffer_alloc(heap, heap_allocation.len, heap_allocation.flags);
> +		if (fd < 0)
> +			return fd;
> +
> +		heap_allocation.fd = fd;
> +
> +		if (copy_to_user((void __user *)arg, &heap_allocation, _IOC_SIZE(cmd)))
> +			return -EFAULT;
> +
> +		break;
> +	}
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct file_operations dma_heap_fops = {
> +	.owner          = THIS_MODULE,
> +	.open		= dma_heap_open,
> +	.release	= dma_heap_release,
> +	.unlocked_ioctl = dma_heap_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl	= dma_heap_ioctl,
> +#endif
> +};
> +
> +int dma_heap_add(struct dma_heap_info *heap_info)
> +{
> +	struct dma_heap *heap;
> +	struct device *dev_ret;
> +	struct dentry *heap_root;
> +	int ret;
> +
> +	if (!heap_info->name || !strcmp(heap_info->name, "")) {
> +		pr_err("dma_heap: Cannot add heap without a name\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!heap_info->ops || !heap_info->ops->allocate) {
> +		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
> +		return -EINVAL;
> +	}
> +
> +	heap = kzalloc(sizeof(*heap), GFP_KERNEL);
> +	if (!heap)
> +		return -ENOMEM;
> +
> +	heap->name = heap_info->name;
> +	memcpy(heap->ops, heap_info->ops, sizeof(*heap->ops));
> +
> +	/* Find unused minor number */
> +	mutex_lock(&minor_lock);
> +	ret = idr_alloc(&dma_heap_idr, heap, 0, NUM_HEAP_MINORS, GFP_KERNEL);
> +	mutex_unlock(&minor_lock);
> +	if (ret < 0) {
> +		pr_err("dma_heap: Unable to get minor number for heap\n");
> +		return ret;
> +	}
> +	heap->minor = ret;
> +
> +	/* Create device */
> +	heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
> +	dev_ret = device_create(dma_heap_class,
> +				NULL,
> +				heap->heap_devt,
> +				NULL,
> +				heap->name);
> +	if (IS_ERR(dev_ret)) {
> +		pr_err("dma_heap: Unable to create char device\n");
> +		return PTR_ERR(dev_ret);
> +	}
> +
> +	/* Add device */
> +	cdev_init(&heap->heap_cdev, &dma_heap_fops);
> +	ret = cdev_add(&heap->heap_cdev, dma_heap_devt, NUM_HEAP_MINORS);
> +	if (ret < 0) {
> +		device_destroy(dma_heap_class, heap->heap_devt);
> +		pr_err("dma_heap: Unable to add char device\n");
> +		return ret;
> +	}
> +
> +	heap->num_of_buffers = 0;
> +	heap->num_of_alloc_bytes = 0;
> +	heap->alloc_bytes_wm = 0;
> +	spin_lock_init(&heap->stat_lock);
> +	heap_root = debugfs_create_dir(heap->name, dma_heap_debug_root);
> +	debugfs_create_u64("num_of_buffers", 0444, heap_root, &heap->num_of_buffers);
> +	debugfs_create_u64("num_of_alloc_bytes", 0444, heap_root, &heap->num_of_alloc_bytes);
> +	debugfs_create_u64("alloc_bytes_wm", 0444, heap_root, &heap->alloc_bytes_wm);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(dma_heap_add);
> +
> +static int dma_heap_init(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&dma_heap_devt, 0, NUM_HEAP_MINORS, DEVNAME);
> +	if (ret)
> +		return ret;
> +
> +	dma_heap_class = class_create(THIS_MODULE, DEVNAME);
> +	if (IS_ERR(dma_heap_class)) {
> +		unregister_chrdev_region(dma_heap_devt, NUM_HEAP_MINORS);
> +		return PTR_ERR(dma_heap_class);
> +	}
> +
> +	dma_heap_debug_root = debugfs_create_dir("dma_heap", NULL);
> +
> +	return 0;
> +}
> +subsys_initcall(dma_heap_init);
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> new file mode 100644
> index 000000000000..09eab3105118
> --- /dev/null
> +++ b/include/linux/dma-heap.h
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF Heaps Allocation Infrastructure
> + *
> + * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2019 Linaro Ltd.
> + */
> +
> +#ifndef _DMA_HEAPS_H
> +#define _DMA_HEAPS_H
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct dma_heap_buffer - metadata for a particular buffer
> + * @heap:		back pointer to the heap the buffer came from
> + * @dmabuf:		backing dma-buf for this buffer
> + * @size:		size of the buffer
> + * @flags:		buffer specific flags
> + */
> +struct dma_heap_buffer {
> +	struct dma_heap *heap;
> +	struct dma_buf *dmabuf;
> +	size_t size;
> +	unsigned long flags;
> +};
> +
> +/**
> + * struct dma_heap_ops - ops to operate on a given heap
> + * @allocate:		allocate buffer
> + *
> + * allocate returns 0 on success, -errno on error.
> + */
> +struct dma_heap_ops {
> +	int (*allocate)(struct dma_heap *heap,
> +			struct dma_heap_buffer *buffer,
> +			unsigned long len,
> +			unsigned long flags);
> +};
> +
> +/**
> + * struct dma_heap_info - holds information needed to create a DMA-heap
> + * @ops:		ops struct for this heap
> + * @name:		used for debugging/device-node name
> + */
> +struct dma_heap_info {
> +	const char *name;
> +	struct dma_heap_ops *ops;
> +};
> +
> +/**
> + * dma_heap_add - adds a heap to dmabuf heaps
> + * @heap:		the heap to add
> + */
> +int dma_heap_add(struct dma_heap_info *heap_info);
> +
> +#endif /* _DMA_HEAPS_H */
> diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
> new file mode 100644
> index 000000000000..7dcbb98e29ec
> --- /dev/null
> +++ b/include/uapi/linux/dma-heap.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * DMABUF Heaps Userspace API
> + *
> + * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2019 Linaro Ltd.
> + */
> +#ifndef _UAPI_LINUX_DMABUF_POOL_H
> +#define _UAPI_LINUX_DMABUF_POOL_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/**
> + * DOC: DMABUF Heaps Userspace API
> + *
> + */
> +
> +/*
> + * mappings of this buffer should be un-cached, otherwise dmabuf heaps will
> + * need cache maintenance using DMA_BUF_SYNC_* when the buffer is mapped for dma
> + */
> +#define DMA_HEAP_FLAG_COHERENT 1
> +
> +/**
> + * struct dma_heap_allocation_data - metadata passed from userspace for
> + *                                      allocations
> + * @len:		size of the allocation
> + * @flags:		flags passed to pool
> + * @fd:			will be populated with a fd which provdes the
> + *			handle to the allocated dma-buf
> + *
> + * Provided by userspace as an argument to the ioctl
> + */
> +struct dma_heap_allocation_data {
> +	__u64 len;
> +	__u32 flags;
> +	__u32 fd;
> +	__u32 reserved0;
> +	__u32 reserved1;
> +};
> +

Did you have anything in particular for the reserved fields?
I don't object to having them, was just curious.

It might be worth considering bumping up the flags to u64
and/or adding an align field back in. I originally removed
the align field because nothing was actually using it and
it seemed to cause more confusion than anything. I suggested
if heaps really wanted an alignment they could pass it in
the flags.

Did you have thoughts on declaring which bits of the flags
can be used by heaps for their own private use? That was
a request I got several times.

> +#define DMA_HEAP_IOC_MAGIC		'H'
> +
> +/**
> + * DOC: DMA_HEAP_IOC_ALLOC - allocate memory from pool
> + *
> + * Takes an dma_heap_allocation_data struct and returns it with the fd field
> + * populated with the dmabuf handle of the allocation.
> + */
> +#define DMA_HEAP_IOC_ALLOC	_IOWR(DMA_HEAP_IOC_MAGIC, 0, \
> +				      struct dma_heap_allocation_data)
> +
> +#endif /* _UAPI_LINUX_DMABUF_POOL_H */
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-27 23:22 ` Laura Abbott
@ 2019-02-28  0:14   ` John Stultz
  0 siblings, 0 replies; 27+ messages in thread
From: John Stultz @ 2019-02-28  0:14 UTC (permalink / raw)
  To: Laura Abbott
  Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel, Andrew F. Davis

On Wed, Feb 27, 2019 at 3:22 PM Laura Abbott <labbott@redhat.com> wrote:
> On 2/25/19 6:36 AM, Andrew F. Davis wrote:
> > +
> > +dev_t dma_heap_devt;
> > +struct class *dma_heap_class;
>
> Can we make sure this gets reviewed by Greg sooner rather than
> later when we drop the RFC? I think the use of this here
> is fine with the device model but I'd rather not find out at
> the end.

Agreed!

> > +/**
> > + * struct dma_heap_allocation_data - metadata passed from userspace for
> > + *                                      allocations
> > + * @len:             size of the allocation
> > + * @flags:           flags passed to pool
> > + * @fd:                      will be populated with a fd which provdes the
> > + *                   handle to the allocated dma-buf
> > + *
> > + * Provided by userspace as an argument to the ioctl
> > + */
> > +struct dma_heap_allocation_data {
> > +     __u64 len;
> > +     __u32 flags;
> > +     __u32 fd;
> > +     __u32 reserved0;
> > +     __u32 reserved1;
> > +};
> > +
>
> Did you have anything in particular for the reserved fields?
> I don't object to having them, was just curious.

As we have thinned down some of the ion fields, I'm mostly suspecting
some additions may be needed as we start integrating more vendor
implemented heaps. But I don't have any plans myself.

> It might be worth considering bumping up the flags to u64
> and/or adding an align field back in. I originally removed
> the align field because nothing was actually using it and
> it seemed to cause more confusion than anything. I suggested
> if heaps really wanted an alignment they could pass it in
> the flags.

Yea, I think a u64 for flags is a good idea. If we don't have an
immediate use for an alignment field, it might be good to just hold
one of the reserved fields for that for now before complicating the
interface w/o users.

> Did you have thoughts on declaring which bits of the flags
> can be used by heaps for their own private use? That was
> a request I got several times.

Hrm.  I sort of feel like if folks can create their own heaps, we
shouldn't need too many options on how to allocate from those heaps.
I'd rather try to force a more standard interface so that userland
allocators can be more easily switched to use different heaps (just by
changing the pathname) w/o extra considerations.

But maybe I'm being a short-sighted. I guess if we really need this,
I'd probably just add an explicit heap_private field instead? I guess
that's another potential use for the reserved fields?

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-27 21:55           ` John Stultz
@ 2019-02-28 15:20             ` Andrew F. Davis
  2019-03-01 23:49               ` John Stultz
  0 siblings, 1 reply; 27+ messages in thread
From: Andrew F. Davis @ 2019-02-28 15:20 UTC (permalink / raw)
  To: John Stultz; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On 2/27/19 3:55 PM, John Stultz wrote:
> On Wed, Feb 27, 2019 at 8:38 AM Andrew F. Davis <afd@ti.com> wrote:
>>
>> On 2/26/19 5:40 PM, John Stultz wrote:
>>> On Tue, Feb 26, 2019 at 11:21 AM John Stultz <john.stultz@linaro.org> wrote:
>>> I've updated the patches here:
>>> kernel: https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap
>>> userland: https://android-review.googlesource.com/c/device/linaro/hikey/+/909436
>>>
>>>
>>>> I also realized some of Benajmin's error path improvements are going
>>>> to be hard to fix w/ my current code, specifically having the allocate
>>>> op do the allocation of the dma_heap_buffer (since we don't have a
>>>> free op, if something fails creating the dmabuf fd in the core, we
>>>> don't have a hook to release the dma_heap_buffer and heap private
>>>> data).
>>>
>>
>> We can always add back the free op, the alternative is to have the heap
>> export the fd.
>>
>> I'm not sure either is needed though, when we
>> dma_buf_put(buffer->dmabuf) on the error path it should trigger the
>> release op, and that can cleanup the allocations in the heap.
> 
> Good point, but I worry that's a bit subtle.
> 
> Also doing the stuff with the helpers where we have to register a free
> callback is kind of ugly, and I personally like the symmetry of having
> free hooks if we have allocation hooks (even if the dmabuf release
> hook initiates the free call).
> 

I do like the symmetry of a free op, just not sure how or what should be
done in it that couldn't be taken care of in the dmabuf.release op..

>>> I also realized doing my development and testing against my
>>> hikey960-mainline-WIP branch, I accidentally folded in an ion hack I
>>> was using to reduce cache operations, so I'll need to undo that in the
>>> heap-helpers.c.  But at least we have a rough validation point for the
>>> design.
>>>
>>
>> Great! The details of the heap-helpers can always get fixed up at a
>> later point, validation of the core working is really good to hear.
>>
> 
> Let me know if you have any further feedback or changes to integrate.
> I've got to get back to some other work, but will try to take a
> cleanup pass in the next few days.
> 

I've got no other feedback right now. I'm guessing we will try a first
non-RFC sometime in the next couple weeks for the sake of getting Greg's
eyes on this, can see where it goes from there.

Thanks,
Andrew

> thanks
> -john
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-25 14:36 [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework Andrew F. Davis
                   ` (6 preceding siblings ...)
  2019-02-27 23:22 ` Laura Abbott
@ 2019-03-01 12:06 ` Brian Starkey
  2019-03-04 14:53   ` Andrew F. Davis
  7 siblings, 1 reply; 27+ messages in thread
From: Brian Starkey @ 2019-03-01 12:06 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: nd, Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

Hi Andrew,

Sorry for not managing to comment on this sooner, I've had a crazy few
days.

As the others have said, I quite like the direction here.

On Mon, Feb 25, 2019 at 08:36:04AM -0600, Andrew F. Davis 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;
> +};
> +
> +/*
> + * This should be called by the heap exporter when a buffers dma-buf
> + * handle is released so the core can update the stats and release the
> + * buffer handle memory.
> + *
> + * Or we could find a way to hook into the fd or struct dma_buf itself?
> + */
> +void dma_heap_buffer_free(struct dma_heap_buffer *buffer)
> +{
> +	spin_lock(&buffer->heap->stat_lock);
> +	buffer->heap->num_of_buffers--;
> +	buffer->heap->num_of_alloc_bytes -= buffer->size;
> +	spin_unlock(&buffer->heap->stat_lock);
> +
> +	kfree(buffer);
> +}
> +
> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, unsigned int flags)
> +{
> +	struct dma_heap_buffer *buffer;
> +	int fd, ret;
> +
> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	buffer->heap = heap;
> +	ret = heap->ops->allocate(heap, buffer, len, flags);
> +	if (ret) {
> +		kfree(buffer);
> +		return ret;
> +	}
> +
> +	fd = dma_buf_fd(buffer->dmabuf, O_CLOEXEC);
> +	if (fd < 0) {
> +		dma_buf_put(buffer->dmabuf);
> +		return fd;
> +	}
> +
> +	spin_lock(&heap->stat_lock);
> +	heap->num_of_buffers++;
> +	heap->num_of_alloc_bytes += buffer->size;
> +	if (heap->num_of_alloc_bytes > heap->alloc_bytes_wm)
> +		heap->alloc_bytes_wm = heap->num_of_alloc_bytes;
> +	spin_unlock(&heap->stat_lock);
> +
> +	return fd;
> +}
> +
> +static int dma_heap_open(struct inode *inode, struct file *filp)
> +{
> +	struct dma_heap *heap;
> +
> +	mutex_lock(&minor_lock);
> +	heap = idr_find(&dma_heap_idr, iminor(inode));
> +	mutex_unlock(&minor_lock);
> +	if (!heap) {
> +		pr_err("dma_heap: minor %d unknown.\n", iminor(inode));
> +		return -ENODEV;
> +	}
> +
> +	/* instance data as context */
> +	filp->private_data = heap;
> +	nonseekable_open(inode, filp);
> +
> +	return 0;
> +}
> +
> +static int dma_heap_release(struct inode *inode, struct file *filp)
> +{
> +	filp->private_data = NULL;
> +
> +	return 0;
> +}
> +
> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	switch (cmd) {
> +	case DMA_HEAP_IOC_ALLOC:
> +	{
> +		struct dma_heap_allocation_data heap_allocation;
> +		struct dma_heap *heap = filp->private_data;
> +		int fd;
> +
> +		if (copy_from_user(&heap_allocation, (void __user *)arg, _IOC_SIZE(cmd)))
> +			return -EFAULT;

Am I right in thinking "cmd" comes from userspace? It might be a good
idea to not trust _IOC_SIZE(cmd) and use our own. I'm looking at
Documentation/ioctl/botching-up-ioctls.txt and drm_ioctl()

> +
> +		if (heap_allocation.reserved0 ||
> +		    heap_allocation.reserved1) {
> +			pr_warn_once("dma_heap: ioctl data not valid\n");
> +			return -EINVAL;
> +		}
> +
> +		fd = dma_heap_buffer_alloc(heap, heap_allocation.len, heap_allocation.flags);
> +		if (fd < 0)
> +			return fd;
> +
> +		heap_allocation.fd = fd;
> +
> +		if (copy_to_user((void __user *)arg, &heap_allocation, _IOC_SIZE(cmd)))
> +			return -EFAULT;
> +
> +		break;
> +	}
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct file_operations dma_heap_fops = {
> +	.owner          = THIS_MODULE,
> +	.open		= dma_heap_open,
> +	.release	= dma_heap_release,
> +	.unlocked_ioctl = dma_heap_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl	= dma_heap_ioctl,
> +#endif
> +};
> +
> +int dma_heap_add(struct dma_heap_info *heap_info)
> +{
> +	struct dma_heap *heap;
> +	struct device *dev_ret;
> +	struct dentry *heap_root;
> +	int ret;
> +
> +	if (!heap_info->name || !strcmp(heap_info->name, "")) {
> +		pr_err("dma_heap: Cannot add heap without a name\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!heap_info->ops || !heap_info->ops->allocate) {
> +		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
> +		return -EINVAL;
> +	}
> +
> +	heap = kzalloc(sizeof(*heap), GFP_KERNEL);
> +	if (!heap)
> +		return -ENOMEM;
> +
> +	heap->name = heap_info->name;
> +	memcpy(heap->ops, heap_info->ops, sizeof(*heap->ops));
> +
> +	/* Find unused minor number */
> +	mutex_lock(&minor_lock);
> +	ret = idr_alloc(&dma_heap_idr, heap, 0, NUM_HEAP_MINORS, GFP_KERNEL);
> +	mutex_unlock(&minor_lock);
> +	if (ret < 0) {
> +		pr_err("dma_heap: Unable to get minor number for heap\n");
> +		return ret;
> +	}
> +	heap->minor = ret;
> +
> +	/* Create device */
> +	heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
> +	dev_ret = device_create(dma_heap_class,
> +				NULL,
> +				heap->heap_devt,
> +				NULL,
> +				heap->name);
> +	if (IS_ERR(dev_ret)) {
> +		pr_err("dma_heap: Unable to create char device\n");
> +		return PTR_ERR(dev_ret);
> +	}
> +
> +	/* Add device */
> +	cdev_init(&heap->heap_cdev, &dma_heap_fops);
> +	ret = cdev_add(&heap->heap_cdev, dma_heap_devt, NUM_HEAP_MINORS);
> +	if (ret < 0) {
> +		device_destroy(dma_heap_class, heap->heap_devt);
> +		pr_err("dma_heap: Unable to add char device\n");
> +		return ret;
> +	}
> +
> +	heap->num_of_buffers = 0;
> +	heap->num_of_alloc_bytes = 0;
> +	heap->alloc_bytes_wm = 0;
> +	spin_lock_init(&heap->stat_lock);
> +	heap_root = debugfs_create_dir(heap->name, dma_heap_debug_root);
> +	debugfs_create_u64("num_of_buffers", 0444, heap_root, &heap->num_of_buffers);
> +	debugfs_create_u64("num_of_alloc_bytes", 0444, heap_root, &heap->num_of_alloc_bytes);
> +	debugfs_create_u64("alloc_bytes_wm", 0444, heap_root, &heap->alloc_bytes_wm);

I can see it being useful to be able to reset this one.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(dma_heap_add);
> +
> +static int dma_heap_init(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&dma_heap_devt, 0, NUM_HEAP_MINORS, DEVNAME);
> +	if (ret)
> +		return ret;
> +
> +	dma_heap_class = class_create(THIS_MODULE, DEVNAME);
> +	if (IS_ERR(dma_heap_class)) {
> +		unregister_chrdev_region(dma_heap_devt, NUM_HEAP_MINORS);
> +		return PTR_ERR(dma_heap_class);
> +	}
> +
> +	dma_heap_debug_root = debugfs_create_dir("dma_heap", NULL);
> +
> +	return 0;
> +}
> +subsys_initcall(dma_heap_init);
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> new file mode 100644
> index 000000000000..09eab3105118
> --- /dev/null
> +++ b/include/linux/dma-heap.h
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF Heaps Allocation Infrastructure
> + *
> + * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2019 Linaro Ltd.
> + */
> +
> +#ifndef _DMA_HEAPS_H
> +#define _DMA_HEAPS_H
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct dma_heap_buffer - metadata for a particular buffer
> + * @heap:		back pointer to the heap the buffer came from
> + * @dmabuf:		backing dma-buf for this buffer
> + * @size:		size of the buffer
> + * @flags:		buffer specific flags
> + */
> +struct dma_heap_buffer {
> +	struct dma_heap *heap;
> +	struct dma_buf *dmabuf;
> +	size_t size;
> +	unsigned long flags;
> +};
> +
> +/**
> + * struct dma_heap_ops - ops to operate on a given heap
> + * @allocate:		allocate buffer
> + *
> + * allocate returns 0 on success, -errno on error.
> + */
> +struct dma_heap_ops {
> +	int (*allocate)(struct dma_heap *heap,
> +			struct dma_heap_buffer *buffer,
> +			unsigned long len,
> +			unsigned long flags);
> +};
> +
> +/**
> + * struct dma_heap_info - holds information needed to create a DMA-heap
> + * @ops:		ops struct for this heap
> + * @name:		used for debugging/device-node name
> + */
> +struct dma_heap_info {
> +	const char *name;
> +	struct dma_heap_ops *ops;
> +};
> +
> +/**
> + * dma_heap_add - adds a heap to dmabuf heaps
> + * @heap:		the heap to add
> + */
> +int dma_heap_add(struct dma_heap_info *heap_info);
> +

For me, the main thing that's missing is a way to unregister a heap.
If drivers/modules are registering heaps, then they also need to be
able to remove them again when they go away (rmmod or whatever).

I think that starts to be quite a can of worms, as you might end up
with buffers which outlive their allocator (that's true today as well
afaik, but is in drivers rather than something more central).

> +#endif /* _DMA_HEAPS_H */
> diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
> new file mode 100644
> index 000000000000..7dcbb98e29ec
> --- /dev/null
> +++ b/include/uapi/linux/dma-heap.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * DMABUF Heaps Userspace API
> + *
> + * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2019 Linaro Ltd.
> + */
> +#ifndef _UAPI_LINUX_DMABUF_POOL_H
> +#define _UAPI_LINUX_DMABUF_POOL_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/**
> + * DOC: DMABUF Heaps Userspace API
> + *
> + */
> +
> +/*
> + * mappings of this buffer should be un-cached, otherwise dmabuf heaps will
> + * need cache maintenance using DMA_BUF_SYNC_* when the buffer is mapped for dma
> + */
> +#define DMA_HEAP_FLAG_COHERENT 1

I'm not really clear on the intention of this flag, and I do think
that "COHERENT" is a confusing/overloaded term.

To me, if the buffer is actually coherent with DMA masters, then
there's no need for any cache maintenance - which is the opposite of
what the comment says.

Cheers,
-Brian

> +
> +/**
> + * struct dma_heap_allocation_data - metadata passed from userspace for
> + *                                      allocations
> + * @len:		size of the allocation
> + * @flags:		flags passed to pool
> + * @fd:			will be populated with a fd which provdes the
> + *			handle to the allocated dma-buf
> + *
> + * Provided by userspace as an argument to the ioctl
> + */
> +struct dma_heap_allocation_data {
> +	__u64 len;
> +	__u32 flags;
> +	__u32 fd;
> +	__u32 reserved0;
> +	__u32 reserved1;
> +};
> +
> +#define DMA_HEAP_IOC_MAGIC		'H'
> +
> +/**
> + * DOC: DMA_HEAP_IOC_ALLOC - allocate memory from pool
> + *
> + * Takes an dma_heap_allocation_data struct and returns it with the fd field
> + * populated with the dmabuf handle of the allocation.
> + */
> +#define DMA_HEAP_IOC_ALLOC	_IOWR(DMA_HEAP_IOC_MAGIC, 0, \
> +				      struct dma_heap_allocation_data)
> +
> +#endif /* _UAPI_LINUX_DMABUF_POOL_H */
> -- 
> 2.19.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-02-28 15:20             ` Andrew F. Davis
@ 2019-03-01 23:49               ` John Stultz
  0 siblings, 0 replies; 27+ messages in thread
From: John Stultz @ 2019-03-01 23:49 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On Thu, Feb 28, 2019 at 9:21 AM Andrew F. Davis <afd@ti.com> wrote:
>
> On 2/27/19 3:55 PM, John Stultz wrote:
> > On Wed, Feb 27, 2019 at 8:38 AM Andrew F. Davis <afd@ti.com> wrote:
> >>
> >> We can always add back the free op, the alternative is to have the heap
> >> export the fd.
> >>
> >> I'm not sure either is needed though, when we
> >> dma_buf_put(buffer->dmabuf) on the error path it should trigger the
> >> release op, and that can cleanup the allocations in the heap.
> >
> > Good point, but I worry that's a bit subtle.
> >
> > Also doing the stuff with the helpers where we have to register a free
> > callback is kind of ugly, and I personally like the symmetry of having
> > free hooks if we have allocation hooks (even if the dmabuf release
> > hook initiates the free call).
> >
>
> I do like the symmetry of a free op, just not sure how or what should be
> done in it that couldn't be taken care of in the dmabuf.release op..

I came around on this. I've reworked the top core allocate function to
basically just return the result of heap->allocate(), which now
returns the dmabuf fd. Any error handling/freeing is then kept to the
heap allocate function and we don't need a heap free op.

I still have to have a free callback for the helper functions, but I
may do some more cleanup there.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-03-01 12:06 ` Brian Starkey
@ 2019-03-04 14:53   ` Andrew F. Davis
  2019-03-05  1:16     ` John Stultz
  2019-03-05 18:22     ` Brian Starkey
  0 siblings, 2 replies; 27+ messages in thread
From: Andrew F. Davis @ 2019-03-04 14:53 UTC (permalink / raw)
  To: Brian Starkey; +Cc: nd, Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On 3/1/19 6:06 AM, Brian Starkey wrote:
> Hi Andrew,
> 
> Sorry for not managing to comment on this sooner, I've had a crazy few
> days.
> 
> As the others have said, I quite like the direction here.
> 
> On Mon, Feb 25, 2019 at 08:36:04AM -0600, Andrew F. Davis 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;
>> +};
>> +
>> +/*
>> + * This should be called by the heap exporter when a buffers dma-buf
>> + * handle is released so the core can update the stats and release the
>> + * buffer handle memory.
>> + *
>> + * Or we could find a way to hook into the fd or struct dma_buf itself?
>> + */
>> +void dma_heap_buffer_free(struct dma_heap_buffer *buffer)
>> +{
>> +	spin_lock(&buffer->heap->stat_lock);
>> +	buffer->heap->num_of_buffers--;
>> +	buffer->heap->num_of_alloc_bytes -= buffer->size;
>> +	spin_unlock(&buffer->heap->stat_lock);
>> +
>> +	kfree(buffer);
>> +}
>> +
>> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, unsigned int flags)
>> +{
>> +	struct dma_heap_buffer *buffer;
>> +	int fd, ret;
>> +
>> +	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
>> +	if (!buffer)
>> +		return -ENOMEM;
>> +
>> +	buffer->heap = heap;
>> +	ret = heap->ops->allocate(heap, buffer, len, flags);
>> +	if (ret) {
>> +		kfree(buffer);
>> +		return ret;
>> +	}
>> +
>> +	fd = dma_buf_fd(buffer->dmabuf, O_CLOEXEC);
>> +	if (fd < 0) {
>> +		dma_buf_put(buffer->dmabuf);
>> +		return fd;
>> +	}
>> +
>> +	spin_lock(&heap->stat_lock);
>> +	heap->num_of_buffers++;
>> +	heap->num_of_alloc_bytes += buffer->size;
>> +	if (heap->num_of_alloc_bytes > heap->alloc_bytes_wm)
>> +		heap->alloc_bytes_wm = heap->num_of_alloc_bytes;
>> +	spin_unlock(&heap->stat_lock);
>> +
>> +	return fd;
>> +}
>> +
>> +static int dma_heap_open(struct inode *inode, struct file *filp)
>> +{
>> +	struct dma_heap *heap;
>> +
>> +	mutex_lock(&minor_lock);
>> +	heap = idr_find(&dma_heap_idr, iminor(inode));
>> +	mutex_unlock(&minor_lock);
>> +	if (!heap) {
>> +		pr_err("dma_heap: minor %d unknown.\n", iminor(inode));
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* instance data as context */
>> +	filp->private_data = heap;
>> +	nonseekable_open(inode, filp);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dma_heap_release(struct inode *inode, struct file *filp)
>> +{
>> +	filp->private_data = NULL;
>> +
>> +	return 0;
>> +}
>> +
>> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> +{
>> +	switch (cmd) {
>> +	case DMA_HEAP_IOC_ALLOC:
>> +	{
>> +		struct dma_heap_allocation_data heap_allocation;
>> +		struct dma_heap *heap = filp->private_data;
>> +		int fd;
>> +
>> +		if (copy_from_user(&heap_allocation, (void __user *)arg, _IOC_SIZE(cmd)))
>> +			return -EFAULT;
> 
> Am I right in thinking "cmd" comes from userspace? It might be a good
> idea to not trust _IOC_SIZE(cmd) and use our own. I'm looking at
> Documentation/ioctl/botching-up-ioctls.txt and drm_ioctl()
> 

Yes cmd is from userspace, but we are in a switch-case that has already
matched cmd == DMA_HEAP_IOC_ALLOC which is sized correctly.

>> +
>> +		if (heap_allocation.reserved0 ||
>> +		    heap_allocation.reserved1) {
>> +			pr_warn_once("dma_heap: ioctl data not valid\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		fd = dma_heap_buffer_alloc(heap, heap_allocation.len, heap_allocation.flags);
>> +		if (fd < 0)
>> +			return fd;
>> +
>> +		heap_allocation.fd = fd;
>> +
>> +		if (copy_to_user((void __user *)arg, &heap_allocation, _IOC_SIZE(cmd)))
>> +			return -EFAULT;
>> +
>> +		break;
>> +	}
>> +	default:
>> +		return -ENOTTY;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct file_operations dma_heap_fops = {
>> +	.owner          = THIS_MODULE,
>> +	.open		= dma_heap_open,
>> +	.release	= dma_heap_release,
>> +	.unlocked_ioctl = dma_heap_ioctl,
>> +#ifdef CONFIG_COMPAT
>> +	.compat_ioctl	= dma_heap_ioctl,
>> +#endif
>> +};
>> +
>> +int dma_heap_add(struct dma_heap_info *heap_info)
>> +{
>> +	struct dma_heap *heap;
>> +	struct device *dev_ret;
>> +	struct dentry *heap_root;
>> +	int ret;
>> +
>> +	if (!heap_info->name || !strcmp(heap_info->name, "")) {
>> +		pr_err("dma_heap: Cannot add heap without a name\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!heap_info->ops || !heap_info->ops->allocate) {
>> +		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	heap = kzalloc(sizeof(*heap), GFP_KERNEL);
>> +	if (!heap)
>> +		return -ENOMEM;
>> +
>> +	heap->name = heap_info->name;
>> +	memcpy(heap->ops, heap_info->ops, sizeof(*heap->ops));
>> +
>> +	/* Find unused minor number */
>> +	mutex_lock(&minor_lock);
>> +	ret = idr_alloc(&dma_heap_idr, heap, 0, NUM_HEAP_MINORS, GFP_KERNEL);
>> +	mutex_unlock(&minor_lock);
>> +	if (ret < 0) {
>> +		pr_err("dma_heap: Unable to get minor number for heap\n");
>> +		return ret;
>> +	}
>> +	heap->minor = ret;
>> +
>> +	/* Create device */
>> +	heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
>> +	dev_ret = device_create(dma_heap_class,
>> +				NULL,
>> +				heap->heap_devt,
>> +				NULL,
>> +				heap->name);
>> +	if (IS_ERR(dev_ret)) {
>> +		pr_err("dma_heap: Unable to create char device\n");
>> +		return PTR_ERR(dev_ret);
>> +	}
>> +
>> +	/* Add device */
>> +	cdev_init(&heap->heap_cdev, &dma_heap_fops);
>> +	ret = cdev_add(&heap->heap_cdev, dma_heap_devt, NUM_HEAP_MINORS);
>> +	if (ret < 0) {
>> +		device_destroy(dma_heap_class, heap->heap_devt);
>> +		pr_err("dma_heap: Unable to add char device\n");
>> +		return ret;
>> +	}
>> +
>> +	heap->num_of_buffers = 0;
>> +	heap->num_of_alloc_bytes = 0;
>> +	heap->alloc_bytes_wm = 0;
>> +	spin_lock_init(&heap->stat_lock);
>> +	heap_root = debugfs_create_dir(heap->name, dma_heap_debug_root);
>> +	debugfs_create_u64("num_of_buffers", 0444, heap_root, &heap->num_of_buffers);
>> +	debugfs_create_u64("num_of_alloc_bytes", 0444, heap_root, &heap->num_of_alloc_bytes);
>> +	debugfs_create_u64("alloc_bytes_wm", 0444, heap_root, &heap->alloc_bytes_wm);
> 
> I can see it being useful to be able to reset this one.
> 

Agree, looks like these will be pulled into the heaps themselves in the
next rev John is working on, so shouldn't matter here.

What we are moving to is a better (I think) ownership model. 'DMA-heaps'
only tracks 'heaps', 'heaps' track their 'buffers'. In the above we have
'DMA-heaps' tracking info on 'buffers', bypassing the 'heaps' layer, so
in the next rev will be the 'DMA-heaps' core asks the 'heaps' to report
back stats.

>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(dma_heap_add);
>> +
>> +static int dma_heap_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = alloc_chrdev_region(&dma_heap_devt, 0, NUM_HEAP_MINORS, DEVNAME);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dma_heap_class = class_create(THIS_MODULE, DEVNAME);
>> +	if (IS_ERR(dma_heap_class)) {
>> +		unregister_chrdev_region(dma_heap_devt, NUM_HEAP_MINORS);
>> +		return PTR_ERR(dma_heap_class);
>> +	}
>> +
>> +	dma_heap_debug_root = debugfs_create_dir("dma_heap", NULL);
>> +
>> +	return 0;
>> +}
>> +subsys_initcall(dma_heap_init);
>> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
>> new file mode 100644
>> index 000000000000..09eab3105118
>> --- /dev/null
>> +++ b/include/linux/dma-heap.h
>> @@ -0,0 +1,57 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * DMABUF Heaps Allocation Infrastructure
>> + *
>> + * Copyright (C) 2011 Google, Inc.
>> + * Copyright (C) 2019 Linaro Ltd.
>> + */
>> +
>> +#ifndef _DMA_HEAPS_H
>> +#define _DMA_HEAPS_H
>> +
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct dma_heap_buffer - metadata for a particular buffer
>> + * @heap:		back pointer to the heap the buffer came from
>> + * @dmabuf:		backing dma-buf for this buffer
>> + * @size:		size of the buffer
>> + * @flags:		buffer specific flags
>> + */
>> +struct dma_heap_buffer {
>> +	struct dma_heap *heap;
>> +	struct dma_buf *dmabuf;
>> +	size_t size;
>> +	unsigned long flags;
>> +};
>> +
>> +/**
>> + * struct dma_heap_ops - ops to operate on a given heap
>> + * @allocate:		allocate buffer
>> + *
>> + * allocate returns 0 on success, -errno on error.
>> + */
>> +struct dma_heap_ops {
>> +	int (*allocate)(struct dma_heap *heap,
>> +			struct dma_heap_buffer *buffer,
>> +			unsigned long len,
>> +			unsigned long flags);
>> +};
>> +
>> +/**
>> + * struct dma_heap_info - holds information needed to create a DMA-heap
>> + * @ops:		ops struct for this heap
>> + * @name:		used for debugging/device-node name
>> + */
>> +struct dma_heap_info {
>> +	const char *name;
>> +	struct dma_heap_ops *ops;
>> +};
>> +
>> +/**
>> + * dma_heap_add - adds a heap to dmabuf heaps
>> + * @heap:		the heap to add
>> + */
>> +int dma_heap_add(struct dma_heap_info *heap_info);
>> +
> 
> For me, the main thing that's missing is a way to unregister a heap.
> If drivers/modules are registering heaps, then they also need to be
> able to remove them again when they go away (rmmod or whatever).
> 
> I think that starts to be quite a can of worms, as you might end up
> with buffers which outlive their allocator (that's true today as well
> afaik, but is in drivers rather than something more central).
> 

Yeah, was wanting to avoid dealing with all that, at least to start out
with. At a high level memory shouldn't ever just disappear, we should
block removing a heap driver until all buffers have been released.

>> +#endif /* _DMA_HEAPS_H */
>> diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
>> new file mode 100644
>> index 000000000000..7dcbb98e29ec
>> --- /dev/null
>> +++ b/include/uapi/linux/dma-heap.h
>> @@ -0,0 +1,54 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * DMABUF Heaps Userspace API
>> + *
>> + * Copyright (C) 2011 Google, Inc.
>> + * Copyright (C) 2019 Linaro Ltd.
>> + */
>> +#ifndef _UAPI_LINUX_DMABUF_POOL_H
>> +#define _UAPI_LINUX_DMABUF_POOL_H
>> +
>> +#include <linux/ioctl.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * DOC: DMABUF Heaps Userspace API
>> + *
>> + */
>> +
>> +/*
>> + * mappings of this buffer should be un-cached, otherwise dmabuf heaps will
>> + * need cache maintenance using DMA_BUF_SYNC_* when the buffer is mapped for dma
>> + */
>> +#define DMA_HEAP_FLAG_COHERENT 1
> 
> I'm not really clear on the intention of this flag, and I do think
> that "COHERENT" is a confusing/overloaded term.
> 

It is, I wanted to use the term as used by the other DMA frameworks, but
I don't really like it either.

> To me, if the buffer is actually coherent with DMA masters, then
> there's no need for any cache maintenance - which is the opposite of
> what the comment says.
> 

Buffers themselves can't be (non)coherent, they are just memory, masters
on a bus can have coherent interactions depending on which masters are
talking and how the buffer in question is managed. So really the term
isn't right almost anywhere as it only applies from the perspective of
the local core (running Linux) and only if simply not locally caching
the buffer is enough to make it "coherent". But that is a rant best
saved for another time.

For us lets drop that flag, if you want to allocate from a
non-locally-cached buffer then it can be its own heap that only provides
that type of allocation. Or even the same heap exporter providing both
types, just registers itself twice with the DMA-heaps core, once for an
interface that gives out cached buffers and one for uncached.

Andrew

> Cheers,
> -Brian
> 
>> +
>> +/**
>> + * struct dma_heap_allocation_data - metadata passed from userspace for
>> + *                                      allocations
>> + * @len:		size of the allocation
>> + * @flags:		flags passed to pool
>> + * @fd:			will be populated with a fd which provdes the
>> + *			handle to the allocated dma-buf
>> + *
>> + * Provided by userspace as an argument to the ioctl
>> + */
>> +struct dma_heap_allocation_data {
>> +	__u64 len;
>> +	__u32 flags;
>> +	__u32 fd;
>> +	__u32 reserved0;
>> +	__u32 reserved1;
>> +};
>> +
>> +#define DMA_HEAP_IOC_MAGIC		'H'
>> +
>> +/**
>> + * DOC: DMA_HEAP_IOC_ALLOC - allocate memory from pool
>> + *
>> + * Takes an dma_heap_allocation_data struct and returns it with the fd field
>> + * populated with the dmabuf handle of the allocation.
>> + */
>> +#define DMA_HEAP_IOC_ALLOC	_IOWR(DMA_HEAP_IOC_MAGIC, 0, \
>> +				      struct dma_heap_allocation_data)
>> +
>> +#endif /* _UAPI_LINUX_DMABUF_POOL_H */
>> -- 
>> 2.19.1
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-03-04 14:53   ` Andrew F. Davis
@ 2019-03-05  1:16     ` John Stultz
  2019-03-05 18:05       ` Andrew F. Davis
  2019-03-05 18:22     ` Brian Starkey
  1 sibling, 1 reply; 27+ messages in thread
From: John Stultz @ 2019-03-05  1:16 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: nd, Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On Mon, Mar 4, 2019 at 6:53 AM Andrew F. Davis <afd@ti.com> wrote:
> On 3/1/19 6:06 AM, Brian Starkey wrote:
> > On Mon, Feb 25, 2019 at 08:36:04AM -0600, Andrew F. Davis wrote:
> >> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >> +{
> >> +    switch (cmd) {
> >> +    case DMA_HEAP_IOC_ALLOC:
> >> +    {
> >> +            struct dma_heap_allocation_data heap_allocation;
> >> +            struct dma_heap *heap = filp->private_data;
> >> +            int fd;
> >> +
> >> +            if (copy_from_user(&heap_allocation, (void __user *)arg, _IOC_SIZE(cmd)))
> >> +                    return -EFAULT;
> >
> > Am I right in thinking "cmd" comes from userspace? It might be a good
> > idea to not trust _IOC_SIZE(cmd) and use our own. I'm looking at
> > Documentation/ioctl/botching-up-ioctls.txt and drm_ioctl()
> >
>
> Yes cmd is from userspace, but we are in a switch-case that has already
> matched cmd == DMA_HEAP_IOC_ALLOC which is sized correctly.
>

Well, even so, I went through and made this cleanup over the weekend,
as sizeof(heap_allocation) is probably more straight forward.

The current patchset against v5.0 (with hikey960 patches), which
includes the flags and other suggested changes is here:
  https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap

W/ userland support here:
  https://android-review.googlesource.com/c/device/linaro/hikey/+/909436

I'm hoping to send this out for a real RFC in the next few days. So
Andrew, if you can check it out and make sure it suits you ok I'd
appreciate it!

> >> +    heap->num_of_buffers = 0;
> >> +    heap->num_of_alloc_bytes = 0;
> >> +    heap->alloc_bytes_wm = 0;
> >> +    spin_lock_init(&heap->stat_lock);
> >> +    heap_root = debugfs_create_dir(heap->name, dma_heap_debug_root);
> >> +    debugfs_create_u64("num_of_buffers", 0444, heap_root, &heap->num_of_buffers);
> >> +    debugfs_create_u64("num_of_alloc_bytes", 0444, heap_root, &heap->num_of_alloc_bytes);
> >> +    debugfs_create_u64("alloc_bytes_wm", 0444, heap_root, &heap->alloc_bytes_wm);
> >
> > I can see it being useful to be able to reset this one.
> >
>
> Agree, looks like these will be pulled into the heaps themselves in the
> next rev John is working on, so shouldn't matter here.

Yea. Sort of half-way done on this. I yanked the stats, but haven't
re-added them back to the heaps yet.

> What we are moving to is a better (I think) ownership model. 'DMA-heaps'
> only tracks 'heaps', 'heaps' track their 'buffers'. In the above we have
> 'DMA-heaps' tracking info on 'buffers', bypassing the 'heaps' layer, so
> in the next rev will be the 'DMA-heaps' core asks the 'heaps' to report
> back stats.

Yea. This matches my plan.

> >> +/*
> >> + * mappings of this buffer should be un-cached, otherwise dmabuf heaps will
> >> + * need cache maintenance using DMA_BUF_SYNC_* when the buffer is mapped for dma
> >> + */
> >> +#define DMA_HEAP_FLAG_COHERENT 1
> >
> > I'm not really clear on the intention of this flag, and I do think
> > that "COHERENT" is a confusing/overloaded term.
> >
>
> It is, I wanted to use the term as used by the other DMA frameworks, but
> I don't really like it either.
>
> > To me, if the buffer is actually coherent with DMA masters, then
> > there's no need for any cache maintenance - which is the opposite of
> > what the comment says.
> >
>
> Buffers themselves can't be (non)coherent, they are just memory, masters
> on a bus can have coherent interactions depending on which masters are
> talking and how the buffer in question is managed. So really the term
> isn't right almost anywhere as it only applies from the perspective of
> the local core (running Linux) and only if simply not locally caching
> the buffer is enough to make it "coherent". But that is a rant best
> saved for another time.
>
> For us lets drop that flag, if you want to allocate from a
> non-locally-cached buffer then it can be its own heap that only provides
> that type of allocation. Or even the same heap exporter providing both
> types, just registers itself twice with the DMA-heaps core, once for an
> interface that gives out cached buffers and one for uncached.

So I've not removed this yet. My only concern is that if its a
reasonable common attribute for heaps to implement, we probably should
keep it, rather then pushing for new heaps for coherent/non-coherent.
This comes from my experience creating the CLOCK_REALTIME_ALARM,
CLOCK_BOOTTIME_ALARM clockids, then later realizing  ALARM should have
been just a attribute flag on the REALTIME/BOOTTIME clockids.  I'd
rather not rework the heaps to have system and system-coherent  and
cma and cma-coherent, if its a general thing.

That said, I did find the flag's meaning confusing myself initially,
so maybe holding off on it for now (if we don't have a clear user) is
a good idea?

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-03-05  1:16     ` John Stultz
@ 2019-03-05 18:05       ` Andrew F. Davis
  2019-03-05 18:45         ` John Stultz
  0 siblings, 1 reply; 27+ messages in thread
From: Andrew F. Davis @ 2019-03-05 18:05 UTC (permalink / raw)
  To: John Stultz; +Cc: nd, Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On 3/4/19 7:16 PM, John Stultz wrote:
> On Mon, Mar 4, 2019 at 6:53 AM Andrew F. Davis <afd@ti.com> wrote:
>> On 3/1/19 6:06 AM, Brian Starkey wrote:
>>> On Mon, Feb 25, 2019 at 08:36:04AM -0600, Andrew F. Davis wrote:
>>>> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>>> +{
>>>> +    switch (cmd) {
>>>> +    case DMA_HEAP_IOC_ALLOC:
>>>> +    {
>>>> +            struct dma_heap_allocation_data heap_allocation;
>>>> +            struct dma_heap *heap = filp->private_data;
>>>> +            int fd;
>>>> +
>>>> +            if (copy_from_user(&heap_allocation, (void __user *)arg, _IOC_SIZE(cmd)))
>>>> +                    return -EFAULT;
>>>
>>> Am I right in thinking "cmd" comes from userspace? It might be a good
>>> idea to not trust _IOC_SIZE(cmd) and use our own. I'm looking at
>>> Documentation/ioctl/botching-up-ioctls.txt and drm_ioctl()
>>>
>>
>> Yes cmd is from userspace, but we are in a switch-case that has already
>> matched cmd == DMA_HEAP_IOC_ALLOC which is sized correctly.
>>
> 
> Well, even so, I went through and made this cleanup over the weekend,
> as sizeof(heap_allocation) is probably more straight forward.
> 
> The current patchset against v5.0 (with hikey960 patches), which
> includes the flags and other suggested changes is here:
>   https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap
> 
> W/ userland support here:
>   https://android-review.googlesource.com/c/device/linaro/hikey/+/909436
> 
> I'm hoping to send this out for a real RFC in the next few days. So
> Andrew, if you can check it out and make sure it suits you ok I'd
> appreciate it!
> 

Nothing at a high level, couple little things I can just point out when
you RFC, I think this is in good shape for a real RFC.

>>>> +    heap->num_of_buffers = 0;
>>>> +    heap->num_of_alloc_bytes = 0;
>>>> +    heap->alloc_bytes_wm = 0;
>>>> +    spin_lock_init(&heap->stat_lock);
>>>> +    heap_root = debugfs_create_dir(heap->name, dma_heap_debug_root);
>>>> +    debugfs_create_u64("num_of_buffers", 0444, heap_root, &heap->num_of_buffers);
>>>> +    debugfs_create_u64("num_of_alloc_bytes", 0444, heap_root, &heap->num_of_alloc_bytes);
>>>> +    debugfs_create_u64("alloc_bytes_wm", 0444, heap_root, &heap->alloc_bytes_wm);
>>>
>>> I can see it being useful to be able to reset this one.
>>>
>>
>> Agree, looks like these will be pulled into the heaps themselves in the
>> next rev John is working on, so shouldn't matter here.
> 
> Yea. Sort of half-way done on this. I yanked the stats, but haven't
> re-added them back to the heaps yet.
> 
>> What we are moving to is a better (I think) ownership model. 'DMA-heaps'
>> only tracks 'heaps', 'heaps' track their 'buffers'. In the above we have
>> 'DMA-heaps' tracking info on 'buffers', bypassing the 'heaps' layer, so
>> in the next rev will be the 'DMA-heaps' core asks the 'heaps' to report
>> back stats.
> 
> Yea. This matches my plan.
> 
>>>> +/*
>>>> + * mappings of this buffer should be un-cached, otherwise dmabuf heaps will
>>>> + * need cache maintenance using DMA_BUF_SYNC_* when the buffer is mapped for dma
>>>> + */
>>>> +#define DMA_HEAP_FLAG_COHERENT 1
>>>
>>> I'm not really clear on the intention of this flag, and I do think
>>> that "COHERENT" is a confusing/overloaded term.
>>>
>>
>> It is, I wanted to use the term as used by the other DMA frameworks, but
>> I don't really like it either.
>>
>>> To me, if the buffer is actually coherent with DMA masters, then
>>> there's no need for any cache maintenance - which is the opposite of
>>> what the comment says.
>>>
>>
>> Buffers themselves can't be (non)coherent, they are just memory, masters
>> on a bus can have coherent interactions depending on which masters are
>> talking and how the buffer in question is managed. So really the term
>> isn't right almost anywhere as it only applies from the perspective of
>> the local core (running Linux) and only if simply not locally caching
>> the buffer is enough to make it "coherent". But that is a rant best
>> saved for another time.
>>
>> For us lets drop that flag, if you want to allocate from a
>> non-locally-cached buffer then it can be its own heap that only provides
>> that type of allocation. Or even the same heap exporter providing both
>> types, just registers itself twice with the DMA-heaps core, once for an
>> interface that gives out cached buffers and one for uncached.
> 
> So I've not removed this yet. My only concern is that if its a
> reasonable common attribute for heaps to implement, we probably should
> keep it, rather then pushing for new heaps for coherent/non-coherent.
> This comes from my experience creating the CLOCK_REALTIME_ALARM,
> CLOCK_BOOTTIME_ALARM clockids, then later realizing  ALARM should have
> been just a attribute flag on the REALTIME/BOOTTIME clockids.  I'd
> rather not rework the heaps to have system and system-coherent  and
> cma and cma-coherent, if its a general thing.
> 
> That said, I did find the flag's meaning confusing myself initially,
> so maybe holding off on it for now (if we don't have a clear user) is
> a good idea?
> 

I'd say hold off on it for now. My problem is that caching is not
something you can really switch at runtime. A heap's backing memory is
either cached or not. This an issue Brian and I had discussed a bit on
the other thread.

Basically if you have a carveout heap for instance, if in DT you have
the reserved-memory node marked "no-map" then it will be non-cached and
you cannot ever make it cached, you could try to hand out cached
userspace mappings on mmap() but those pages will not have a valid
struct *page so most of the dma_sync_* stuff will break. Same is true
the other way, if it is cached, the kernel will always have valid cached
entries of that memory as the kernel logic address area, so handing out
non-cached mappings to userspace breaks aliased mapping attribute rules
on ARM (and probably dangerous on other ARCHs).

In the end you only have heaps that have cached or heaps with non-cached
memory, but cannot switch based on a flag at runtime.

Thanks,
Andrew

> thanks
> -john
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-03-04 14:53   ` Andrew F. Davis
  2019-03-05  1:16     ` John Stultz
@ 2019-03-05 18:22     ` Brian Starkey
  1 sibling, 0 replies; 27+ messages in thread
From: Brian Starkey @ 2019-03-05 18:22 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: nd, Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On Mon, Mar 04, 2019 at 08:53:42AM -0600, Andrew F. Davis wrote:
> On 3/1/19 6:06 AM, Brian Starkey wrote:
> > 
> > Am I right in thinking "cmd" comes from userspace? It might be a good
> > idea to not trust _IOC_SIZE(cmd) and use our own. I'm looking at
> > Documentation/ioctl/botching-up-ioctls.txt and drm_ioctl()
> > 
> 
> Yes cmd is from userspace, but we are in a switch-case that has already
> matched cmd == DMA_HEAP_IOC_ALLOC which is sized correctly.
> 

Ah yeah, sorry I missed that. It's fine until the struct gets
extended, which can be handled if/when.

[snip]

> > 
> > For me, the main thing that's missing is a way to unregister a heap.
> > If drivers/modules are registering heaps, then they also need to be
> > able to remove them again when they go away (rmmod or whatever).
> > 
> > I think that starts to be quite a can of worms, as you might end up
> > with buffers which outlive their allocator (that's true today as well
> > afaik, but is in drivers rather than something more central).
> > 
> 
> Yeah, was wanting to avoid dealing with all that, at least to start out
> with. At a high level memory shouldn't ever just disappear, we should
> block removing a heap driver until all buffers have been released.
> 

Correct me if I'm wrong, but I don't think there's a mechanism in the
kernel to do that right now is there? If my DRM driver registers a
heap, and then gets rmmod-ed, then kaboom. Just not exporting the
symbol might be OK to start with.

Thanks,
-Brian
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework
  2019-03-05 18:05       ` Andrew F. Davis
@ 2019-03-05 18:45         ` John Stultz
  0 siblings, 0 replies; 27+ messages in thread
From: John Stultz @ 2019-03-05 18:45 UTC (permalink / raw)
  To: Andrew F. Davis; +Cc: nd, Chenbo Feng, Alistair Strachan, Liam Mark, dri-devel

On Tue, Mar 5, 2019 at 10:05 AM Andrew F. Davis <afd@ti.com> wrote:
> On 3/4/19 7:16 PM, John Stultz wrote:
> > The current patchset against v5.0 (with hikey960 patches), which
> > includes the flags and other suggested changes is here:
> >   https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap
> >
> > W/ userland support here:
> >   https://android-review.googlesource.com/c/device/linaro/hikey/+/909436
> >
> > I'm hoping to send this out for a real RFC in the next few days. So
> > Andrew, if you can check it out and make sure it suits you ok I'd
> > appreciate it!
> >
>
> Nothing at a high level, couple little things I can just point out when
> you RFC, I think this is in good shape for a real RFC.

Ok.  Hopefully I can get it sent out later today.

> I'd say hold off on it for now. My problem is that caching is not
> something you can really switch at runtime. A heap's backing memory is
> either cached or not. This an issue Brian and I had discussed a bit on
> the other thread.
>
> Basically if you have a carveout heap for instance, if in DT you have
> the reserved-memory node marked "no-map" then it will be non-cached and
> you cannot ever make it cached, you could try to hand out cached
> userspace mappings on mmap() but those pages will not have a valid
> struct *page so most of the dma_sync_* stuff will break. Same is true
> the other way, if it is cached, the kernel will always have valid cached
> entries of that memory as the kernel logic address area, so handing out
> non-cached mappings to userspace breaks aliased mapping attribute rules
> on ARM (and probably dangerous on other ARCHs).
>
> In the end you only have heaps that have cached or heaps with non-cached
> memory, but cannot switch based on a flag at runtime.

Ok. This is convincing. I'll drop that flag for now.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2019-03-05 18:46 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-25 14:36 [EARLY RFC][PATCH] dma-buf: Add dma-buf heaps framework Andrew F. Davis
2019-02-25 18:41 ` John Stultz
2019-02-26  0:20 ` John Stultz
2019-02-26 14:02   ` Andrew F. Davis
2019-02-26  0:55 ` John Stultz
2019-02-26 14:04   ` Andrew F. Davis
2019-02-26  3:53 ` Sumit Semwal
2019-02-26 14:28   ` Andrew F. Davis
2019-02-26  6:20 ` John Stultz
2019-02-26 14:46   ` Andrew F. Davis
2019-02-26 19:21     ` John Stultz
2019-02-26 23:40       ` John Stultz
2019-02-27 16:38         ` Andrew F. Davis
2019-02-27 21:55           ` John Stultz
2019-02-28 15:20             ` Andrew F. Davis
2019-03-01 23:49               ` John Stultz
2019-02-26 14:12 ` Benjamin Gaignard
2019-02-26 15:05   ` Andrew F. Davis
2019-02-26 15:22   ` Linus Walleij
2019-02-27 23:22 ` Laura Abbott
2019-02-28  0:14   ` John Stultz
2019-03-01 12:06 ` Brian Starkey
2019-03-04 14:53   ` Andrew F. Davis
2019-03-05  1:16     ` John Stultz
2019-03-05 18:05       ` Andrew F. Davis
2019-03-05 18:45         ` John Stultz
2019-03-05 18:22     ` Brian Starkey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).