[RFC,1/5,v2] dma-buf: Add dma-buf heaps framework

Commit Message

John Stultz March 5, 2019, 8:54 p.m. UTC

From: "Andrew F. Davis" <[e-mail]>

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.

This code is an evoluiton of the Android ION implementation,
and a big thanks is due to its authors/maintainers over time
for their effort:
  Rebecca Schultz Zavin, Colin Cross, Benjamin Gaignard,
  Laura Abbott, and many other contributors!

Cc: Laura Abbott <[e-mail]>
Cc: Benjamin Gaignard <[e-mail]>
Cc: Greg KH <[e-mail]>
Cc: Sumit Semwal <[e-mail]>
Cc: Liam Mark <[e-mail]>
Cc: Brian Starkey <[e-mail]>
Cc: Andrew F. Davis <[e-mail]>
Cc: Chenbo Feng <[e-mail]>
Cc: Alistair Strachan <[e-mail]>
Cc: [e-mail]
Signed-off-by: Andrew F. Davis <[e-mail]>
[jstultz: reworded commit message, and lots of cleanups]
Signed-off-by: John Stultz <[e-mail]>
---
v2:
* Folded down fixes I had previously shared in implementing
  heaps
* Make flags a u64 (Suggested by Laura)
* Add PAGE_ALIGN() fix to the core alloc funciton
* IOCTL fixups suggested by Brian
* Added fixes suggested by Benjamin
* Removed core stats mgmt, as that should be implemented by
  per-heap code
* Changed alloc to return a dma-buf fd, rather then a buffer
  (as it simplifies error handling)
---
 MAINTAINERS                   |  16 ++++
 drivers/dma-buf/Kconfig       |   8 ++
 drivers/dma-buf/Makefile      |   1 +
 drivers/dma-buf/dma-heap.c    | 191 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-heap.h      |  65 ++++++++++++++
 include/uapi/linux/dma-heap.h |  52 ++++++++++++
 6 files changed, 333 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

Comments

Benjamin Gaignard March 6, 2019, 4:12 p.m. UTC | #1

Le mar. 5 mars 2019 à 21:54, John Stultz <[e-mail]> a écrit :
>
> From: "Andrew F. Davis" <[e-mail]>
>
> 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.
>
> This code is an evoluiton of the Android ION implementation,
> and a big thanks is due to its authors/maintainers over time
> for their effort:
>   Rebecca Schultz Zavin, Colin Cross, Benjamin Gaignard,
>   Laura Abbott, and many other contributors!
>
> Cc: Laura Abbott <[e-mail]>
> Cc: Benjamin Gaignard <[e-mail]>
> Cc: Greg KH <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: Liam Mark <[e-mail]>
> Cc: Brian Starkey <[e-mail]>
> Cc: Andrew F. Davis <[e-mail]>
> Cc: Chenbo Feng <[e-mail]>
> Cc: Alistair Strachan <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Andrew F. Davis <[e-mail]>
> [jstultz: reworded commit message, and lots of cleanups]
> Signed-off-by: John Stultz <[e-mail]>
> ---
> v2:
> * Folded down fixes I had previously shared in implementing
>   heaps
> * Make flags a u64 (Suggested by Laura)
> * Add PAGE_ALIGN() fix to the core alloc funciton
> * IOCTL fixups suggested by Brian
> * Added fixes suggested by Benjamin
> * Removed core stats mgmt, as that should be implemented by
>   per-heap code
> * Changed alloc to return a dma-buf fd, rather then a buffer
>   (as it simplifies error handling)
> ---
>  MAINTAINERS                   |  16 ++++
>  drivers/dma-buf/Kconfig       |   8 ++
>  drivers/dma-buf/Makefile      |   1 +
>  drivers/dma-buf/dma-heap.c    | 191 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-heap.h      |  65 ++++++++++++++
>  include/uapi/linux/dma-heap.h |  52 ++++++++++++
>  6 files changed, 333 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/MAINTAINERS b/MAINTAINERS
> index ac2e518..a661e19 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4621,6 +4621,22 @@ F:       include/linux/*fence.h
>  F:     Documentation/driver-api/dma-buf.rst
>  T:     git git://anongit.freedesktop.org/drm/drm-misc
>
> +DMA-BUF HEAPS FRAMEWORK
> +M:     Laura Abbott <[e-mail]>
> +R:     Liam Mark <[e-mail]>
> +R:     Brian Starkey <[e-mail]>
> +R:     "Andrew F. Davis" <[e-mail]>
> +R:     John Stultz <[e-mail]>
> +S:     Maintained
> +L:     [e-mail]
> +L:     [e-mail]
> +L:     [e-mail] (moderated for non-subscribers)
> +F:     include/uapi/linux/dma-heap.h
> +F:     include/linux/dma-heap.h
> +F:     drivers/dma-buf/dma-heap.c
> +F:     drivers/dma-buf/heaps/*
> +T:     git git://anongit.freedesktop.org/drm/drm-misc
> +
>  DMA GENERIC OFFLOAD ENGINE SUBSYSTEM
>  M:     Vinod Koul <[e-mail]>
>  L:     [e-mail]
> diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
> index 2e5a0fa..09c61db 100644
> --- a/drivers/dma-buf/Kconfig
> +++ b/drivers/dma-buf/Kconfig
> @@ -39,4 +39,12 @@ 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"
> +       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.
> +
>  endmenu
> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> index 0913a6c..b0332f1 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 0000000..14b3975
> --- /dev/null
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -0,0 +1,191 @@
> +// 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;
> +
> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +                                unsigned int flags)
> +{
> +       len = PAGE_ALIGN(len);
> +       if (!len)
> +               return -EINVAL;
> +
> +       return heap->ops->allocate(heap, len, flags);
> +}
> +
> +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,
> +                                  sizeof(heap_allocation)))
> +                       return -EFAULT;
> +
> +               if (heap_allocation.fd ||
> +                   heap_allocation.reserved0 ||
> +                   heap_allocation.reserved1 ||
> +                   heap_allocation.reserved2) {
> +                       pr_warn_once("dma_heap: ioctl data not valid\n");
> +                       return -EINVAL;
> +               }
> +               if (heap_allocation.flags & ~DMA_HEAP_VALID_FLAGS) {
> +                       pr_warn_once("dma_heap: flags has invalid or unsupported flags set\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,
> +                                sizeof(heap_allocation)))
> +                       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 *heap)
> +{
> +       struct device *dev_ret;
> +       int ret;
> +
> +       if (!heap->name || !strcmp(heap->name, "")) {
> +               pr_err("dma_heap: Cannot add heap without a name\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!heap->ops || !heap->ops->allocate) {
> +               pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
> +               return -EINVAL;
> +       }
> +
> +       /* 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;
> +       }
> +
> +       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);
> +       }
> +
> +       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 0000000..ed86a8e
> --- /dev/null
> +++ b/include/linux/dma-heap.h
> @@ -0,0 +1,65 @@
> +/* 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/cdev.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 - 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
> + *
> + * 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;
> +};
> +
> +/**
> + * struct dma_heap_ops - ops to operate on a given heap
> + * @allocate:          allocate dmabuf and return fd
> + *
> + * allocate returns dmabuf fd  on success, -errno on error.
> + */
> +struct dma_heap_ops {
> +       int (*allocate)(struct dma_heap *heap,
> +                       unsigned long len,
> +                       unsigned long flags);
> +};
> +
> +/**
> + * dma_heap_add - adds a heap to dmabuf heaps
> + * @heap:              the heap to add
> + */
> +int dma_heap_add(struct dma_heap *heap);
> +
> +#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 0000000..75c5d3f
> --- /dev/null
> +++ b/include/uapi/linux/dma-heap.h
> @@ -0,0 +1,52 @@
> +/* 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
> + *
> + */
> +
> +/* Currently no flags */
> +#define DMA_HEAP_VALID_FLAGS (0)

I think here you need to allow flags like O_RDWR or O_CLOEXEC else
mmap will fail.

Benjamin

> +
> +/**
> + * 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;
> +       __u64 flags;
> +       __u32 fd;
> +       __u32 reserved0;
> +       __u32 reserved1;
> +       __u32 reserved2;
> +};
> +
> +#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.7.4
>

Andrew Davis March 6, 2019, 4:27 p.m. UTC | #2

On 3/5/19 2:54 PM, John Stultz wrote:
> From: "Andrew F. Davis" <[e-mail]>
> 
> 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.
> 
> This code is an evoluiton of the Android ION implementation,
> and a big thanks is due to its authors/maintainers over time
> for their effort:
>   Rebecca Schultz Zavin, Colin Cross, Benjamin Gaignard,
>   Laura Abbott, and many other contributors!
> 
> Cc: Laura Abbott <[e-mail]>
> Cc: Benjamin Gaignard <[e-mail]>
> Cc: Greg KH <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: Liam Mark <[e-mail]>
> Cc: Brian Starkey <[e-mail]>
> Cc: Andrew F. Davis <[e-mail]>
> Cc: Chenbo Feng <[e-mail]>
> Cc: Alistair Strachan <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Andrew F. Davis <[e-mail]>
> [jstultz: reworded commit message, and lots of cleanups]
> Signed-off-by: John Stultz <[e-mail]>
> ---
> v2:
> * Folded down fixes I had previously shared in implementing
>   heaps
> * Make flags a u64 (Suggested by Laura)
> * Add PAGE_ALIGN() fix to the core alloc funciton
> * IOCTL fixups suggested by Brian
> * Added fixes suggested by Benjamin
> * Removed core stats mgmt, as that should be implemented by
>   per-heap code
> * Changed alloc to return a dma-buf fd, rather then a buffer
>   (as it simplifies error handling)
> ---
>  MAINTAINERS                   |  16 ++++
>  drivers/dma-buf/Kconfig       |   8 ++
>  drivers/dma-buf/Makefile      |   1 +
>  drivers/dma-buf/dma-heap.c    | 191 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-heap.h      |  65 ++++++++++++++
>  include/uapi/linux/dma-heap.h |  52 ++++++++++++
>  6 files changed, 333 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/MAINTAINERS b/MAINTAINERS
> index ac2e518..a661e19 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4621,6 +4621,22 @@ F:	include/linux/*fence.h
>  F:	Documentation/driver-api/dma-buf.rst
>  T:	git git://anongit.freedesktop.org/drm/drm-misc
>  
> +DMA-BUF HEAPS FRAMEWORK
> +M:	Laura Abbott <[e-mail]>
> +R:	Liam Mark <[e-mail]>
> +R:	Brian Starkey <[e-mail]>
> +R:	"Andrew F. Davis" <[e-mail]>

Quotes not needed in maintainers file.

> +R:	John Stultz <[e-mail]>
> +S:	Maintained
> +L:	[e-mail]
> +L:	[e-mail]
> +L:	[e-mail] (moderated for non-subscribers)
> +F:	include/uapi/linux/dma-heap.h
> +F:	include/linux/dma-heap.h
> +F:	drivers/dma-buf/dma-heap.c
> +F:	drivers/dma-buf/heaps/*
> +T:	git git://anongit.freedesktop.org/drm/drm-misc
> +
>  DMA GENERIC OFFLOAD ENGINE SUBSYSTEM
>  M:	Vinod Koul <[e-mail]>
>  L:	[e-mail]
> diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
> index 2e5a0fa..09c61db 100644
> --- a/drivers/dma-buf/Kconfig
> +++ b/drivers/dma-buf/Kconfig
> @@ -39,4 +39,12 @@ 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"
> +	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.
> +
>  endmenu
> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> index 0913a6c..b0332f1 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 0000000..14b3975
> --- /dev/null
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -0,0 +1,191 @@
> +// 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;
> +
> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				 unsigned int flags)
> +{
> +	len = PAGE_ALIGN(len);
> +	if (!len)
> +		return -EINVAL;
> +
> +	return heap->ops->allocate(heap, len, flags);
> +}
> +
> +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,
> +				   sizeof(heap_allocation)))
> +			return -EFAULT;
> +
> +		if (heap_allocation.fd ||
> +		    heap_allocation.reserved0 ||
> +		    heap_allocation.reserved1 ||
> +		    heap_allocation.reserved2) {

Seems too many reserved, I can understand one, but if we ever needed all
of these we would be better off just adding another alloc ioctl.

> +			pr_warn_once("dma_heap: ioctl data not valid\n");
> +			return -EINVAL;
> +		}
> +		if (heap_allocation.flags & ~DMA_HEAP_VALID_FLAGS) {
> +			pr_warn_once("dma_heap: flags has invalid or unsupported flags set\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,
> +				 sizeof(heap_allocation)))
> +			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 *heap)
> +{
> +	struct device *dev_ret;
> +	int ret;
> +
> +	if (!heap->name || !strcmp(heap->name, "")) {
> +		pr_err("dma_heap: Cannot add heap without a name\n");

As these names end up as the dev name in the file system we may want to
check for invalid names, there is probably a helper for that somewhere.

> +		return -EINVAL;
> +	}
> +
> +	if (!heap->ops || !heap->ops->allocate) {
> +		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
> +		return -EINVAL;
> +	}
> +
> +	/* 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;
> +	}
> +
> +	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);
> +	}
> +
> +	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 0000000..ed86a8e
> --- /dev/null
> +++ b/include/linux/dma-heap.h
> @@ -0,0 +1,65 @@
> +/* 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/cdev.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 - 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
> + *
> + * 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;
> +};

Still not sure about this, all of the members in this struct are
strictly internally used by the framework. The users of this framework
should not have access to them and only need to deal with an opaque
pointer for tracking themselves (can store it in a private struct of
their own then container_of to get back out their struct).

Anyway, not a big deal, and if it really bugs me enough I can always go
fix it later, it's all kernel internal so not a blocker here. :)

Andrew

> +
> +/**
> + * struct dma_heap_ops - ops to operate on a given heap
> + * @allocate:		allocate dmabuf and return fd
> + *
> + * allocate returns dmabuf fd  on success, -errno on error.
> + */
> +struct dma_heap_ops {
> +	int (*allocate)(struct dma_heap *heap,
> +			unsigned long len,
> +			unsigned long flags);
> +};
> +
> +/**
> + * dma_heap_add - adds a heap to dmabuf heaps
> + * @heap:		the heap to add
> + */
> +int dma_heap_add(struct dma_heap *heap);
> +
> +#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 0000000..75c5d3f
> --- /dev/null
> +++ b/include/uapi/linux/dma-heap.h
> @@ -0,0 +1,52 @@
> +/* 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
> + *
> + */
> +
> +/* Currently no flags */
> +#define DMA_HEAP_VALID_FLAGS (0)
> +
> +/**
> + * 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;
> +	__u64 flags;
> +	__u32 fd;
> +	__u32 reserved0;
> +	__u32 reserved1;
> +	__u32 reserved2;
> +};
> +
> +#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 */
>

John Stultz March 6, 2019, 4:57 p.m. UTC | #3

On Wed, Mar 6, 2019 at 8:12 AM Benjamin Gaignard
<[e-mail]> wrote:
> Le mar. 5 mars 2019 à 21:54, John Stultz <[e-mail]> a écrit :
> > +/**
> > + * DOC: DMABUF Heaps Userspace API
> > + *
> > + */
> > +
> > +/* Currently no flags */
> > +#define DMA_HEAP_VALID_FLAGS (0)
>
> I think here you need to allow flags like O_RDWR or O_CLOEXEC else
> mmap will fail.
>

Hm. So I meant for these to be just the bitmask of valid flags for the
allocate IOCTL (used to make sure no one is passing junk flags
accidentally), rather then valid flags for the heap open call.  But
this probably suggests I should call it something like
DMA_HEAP_ALLOC_VALID_FLAGS instead?

thanks
-john

John Stultz March 6, 2019, 7:03 p.m. UTC | #4

On Wed, Mar 6, 2019 at 10:18 AM Andrew F. Davis <[e-mail]> wrote:
>
> On 3/5/19 2:54 PM, John Stultz wrote:
> > From: "Andrew F. Davis" <[e-mail]>
> >
> > 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.
> >
> > This code is an evoluiton of the Android ION implementation,
> > and a big thanks is due to its authors/maintainers over time
> > for their effort:
> >   Rebecca Schultz Zavin, Colin Cross, Benjamin Gaignard,
> >   Laura Abbott, and many other contributors!
> >
> > Cc: Laura Abbott <[e-mail]>
> > Cc: Benjamin Gaignard <[e-mail]>
> > Cc: Greg KH <[e-mail]>
> > Cc: Sumit Semwal <[e-mail]>
> > Cc: Liam Mark <[e-mail]>
> > Cc: Brian Starkey <[e-mail]>
> > Cc: Andrew F. Davis <[e-mail]>
> > Cc: Chenbo Feng <[e-mail]>
> > Cc: Alistair Strachan <[e-mail]>
> > Cc: [e-mail]
> > Signed-off-by: Andrew F. Davis <[e-mail]>
> > [jstultz: reworded commit message, and lots of cleanups]
> > Signed-off-by: John Stultz <[e-mail]>
> > ---
> > v2:
> > * Folded down fixes I had previously shared in implementing
> >   heaps
> > * Make flags a u64 (Suggested by Laura)
> > * Add PAGE_ALIGN() fix to the core alloc funciton
> > * IOCTL fixups suggested by Brian
> > * Added fixes suggested by Benjamin
> > * Removed core stats mgmt, as that should be implemented by
> >   per-heap code
> > * Changed alloc to return a dma-buf fd, rather then a buffer
> >   (as it simplifies error handling)
> > ---
> >  MAINTAINERS                   |  16 ++++
> >  drivers/dma-buf/Kconfig       |   8 ++
> >  drivers/dma-buf/Makefile      |   1 +
> >  drivers/dma-buf/dma-heap.c    | 191 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/dma-heap.h      |  65 ++++++++++++++
> >  include/uapi/linux/dma-heap.h |  52 ++++++++++++
> >  6 files changed, 333 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/MAINTAINERS b/MAINTAINERS
> > index ac2e518..a661e19 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4621,6 +4621,22 @@ F:     include/linux/*fence.h
> >  F:   Documentation/driver-api/dma-buf.rst
> >  T:   git git://anongit.freedesktop.org/drm/drm-misc
> >
> > +DMA-BUF HEAPS FRAMEWORK
> > +M:   Laura Abbott <[e-mail]>
> > +R:   Liam Mark <[e-mail]>
> > +R:   Brian Starkey <[e-mail]>
> > +R:   "Andrew F. Davis" <[e-mail]>
>
> Quotes not needed in maintainers file.

Whatever you say, "Andrew F. Davis", or whomever you really are! ;)


> > +
> > +             if (heap_allocation.fd ||
> > +                 heap_allocation.reserved0 ||
> > +                 heap_allocation.reserved1 ||
> > +                 heap_allocation.reserved2) {
>
> Seems too many reserved, I can understand one, but if we ever needed all
> of these we would be better off just adding another alloc ioctl.

Well, we have to have one u32 for padding. And I figured if we needed
anything more then a u32, then we're in for 2 more.

And I think the potential of the alignment and heap-private flags, I
worry we might want to  have something, but I guess we could just add
a new ioctl and keep the support for the old one if folks prefer.

> > +int dma_heap_add(struct dma_heap *heap)
> > +{
> > +     struct device *dev_ret;
> > +     int ret;
> > +
> > +     if (!heap->name || !strcmp(heap->name, "")) {
> > +             pr_err("dma_heap: Cannot add heap without a name\n");
>
> As these names end up as the dev name in the file system we may want to
> check for invalid names, there is probably a helper for that somewhere.

Hrm. I'll have to look.

> > +struct dma_heap {
> > +     const char *name;
> > +     struct dma_heap_ops *ops;
> > +     unsigned int minor;
> > +     dev_t heap_devt;
> > +     struct cdev heap_cdev;
> > +};
>
> Still not sure about this, all of the members in this struct are
> strictly internally used by the framework. The users of this framework
> should not have access to them and only need to deal with an opaque
> pointer for tracking themselves (can store it in a private struct of
> their own then container_of to get back out their struct).
>
> Anyway, not a big deal, and if it really bugs me enough I can always go
> fix it later, it's all kernel internal so not a blocker here. :)

I guess I'd just move the include/linux/dma-heap.h to
drivers/dma-buf/heaps/ and keep it localized there.
But whichever. Feel free to also send a patch and I can fold it down.

thanks
-john

Andrew Davis March 6, 2019, 9:45 p.m. UTC | #5

On 3/6/19 1:03 PM, John Stultz wrote:
> On Wed, Mar 6, 2019 at 10:18 AM Andrew F. Davis <[e-mail]> wrote:
>>
>> On 3/5/19 2:54 PM, John Stultz wrote:
>>> From: "Andrew F. Davis" <[e-mail]>
>>>
>>> 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.
>>>
>>> This code is an evoluiton of the Android ION implementation,
>>> and a big thanks is due to its authors/maintainers over time
>>> for their effort:
>>>   Rebecca Schultz Zavin, Colin Cross, Benjamin Gaignard,
>>>   Laura Abbott, and many other contributors!
>>>
>>> Cc: Laura Abbott <[e-mail]>
>>> Cc: Benjamin Gaignard <[e-mail]>
>>> Cc: Greg KH <[e-mail]>
>>> Cc: Sumit Semwal <[e-mail]>
>>> Cc: Liam Mark <[e-mail]>
>>> Cc: Brian Starkey <[e-mail]>
>>> Cc: Andrew F. Davis <[e-mail]>
>>> Cc: Chenbo Feng <[e-mail]>
>>> Cc: Alistair Strachan <[e-mail]>
>>> Cc: [e-mail]
>>> Signed-off-by: Andrew F. Davis <[e-mail]>
>>> [jstultz: reworded commit message, and lots of cleanups]
>>> Signed-off-by: John Stultz <[e-mail]>
>>> ---
>>> v2:
>>> * Folded down fixes I had previously shared in implementing
>>>   heaps
>>> * Make flags a u64 (Suggested by Laura)
>>> * Add PAGE_ALIGN() fix to the core alloc funciton
>>> * IOCTL fixups suggested by Brian
>>> * Added fixes suggested by Benjamin
>>> * Removed core stats mgmt, as that should be implemented by
>>>   per-heap code
>>> * Changed alloc to return a dma-buf fd, rather then a buffer
>>>   (as it simplifies error handling)
>>> ---
>>>  MAINTAINERS                   |  16 ++++
>>>  drivers/dma-buf/Kconfig       |   8 ++
>>>  drivers/dma-buf/Makefile      |   1 +
>>>  drivers/dma-buf/dma-heap.c    | 191 ++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/dma-heap.h      |  65 ++++++++++++++
>>>  include/uapi/linux/dma-heap.h |  52 ++++++++++++
>>>  6 files changed, 333 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/MAINTAINERS b/MAINTAINERS
>>> index ac2e518..a661e19 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -4621,6 +4621,22 @@ F:     include/linux/*fence.h
>>>  F:   Documentation/driver-api/dma-buf.rst
>>>  T:   git git://anongit.freedesktop.org/drm/drm-misc
>>>
>>> +DMA-BUF HEAPS FRAMEWORK
>>> +M:   Laura Abbott <[e-mail]>
>>> +R:   Liam Mark <[e-mail]>
>>> +R:   Brian Starkey <[e-mail]>
>>> +R:   "Andrew F. Davis" <[e-mail]>
>>
>> Quotes not needed in maintainers file.
> 
> Whatever you say, "Andrew F. Davis", or whomever you really are! ;)
> 

 <_<
 >_>
> 
>>> +
>>> +             if (heap_allocation.fd ||
>>> +                 heap_allocation.reserved0 ||
>>> +                 heap_allocation.reserved1 ||
>>> +                 heap_allocation.reserved2) {
>>
>> Seems too many reserved, I can understand one, but if we ever needed all
>> of these we would be better off just adding another alloc ioctl.
> 
> Well, we have to have one u32 for padding. And I figured if we needed
> anything more then a u32, then we're in for 2 more.
> 
> And I think the potential of the alignment and heap-private flags, I
> worry we might want to  have something, but I guess we could just add
> a new ioctl and keep the support for the old one if folks prefer.
> 
>>> +int dma_heap_add(struct dma_heap *heap)
>>> +{
>>> +     struct device *dev_ret;
>>> +     int ret;
>>> +
>>> +     if (!heap->name || !strcmp(heap->name, "")) {
>>> +             pr_err("dma_heap: Cannot add heap without a name\n");
>>
>> As these names end up as the dev name in the file system we may want to
>> check for invalid names, there is probably a helper for that somewhere.
> 
> Hrm. I'll have to look.
> 
>>> +struct dma_heap {
>>> +     const char *name;
>>> +     struct dma_heap_ops *ops;
>>> +     unsigned int minor;
>>> +     dev_t heap_devt;
>>> +     struct cdev heap_cdev;
>>> +};
>>
>> Still not sure about this, all of the members in this struct are
>> strictly internally used by the framework. The users of this framework
>> should not have access to them and only need to deal with an opaque
>> pointer for tracking themselves (can store it in a private struct of
>> their own then container_of to get back out their struct).
>>
>> Anyway, not a big deal, and if it really bugs me enough I can always go
>> fix it later, it's all kernel internal so not a blocker here. :)
> 
> I guess I'd just move the include/linux/dma-heap.h to
> drivers/dma-buf/heaps/ and keep it localized there.
> But whichever. Feel free to also send a patch and I can fold it down.
> 

The dma-heap.h needs to stay where it is, I was thinking just move
struct dma_heap to inside drivers/dma-buf/dma-heap.c. I wouldn't worry
about changing anything right now though, I'll post a patch you can
squash in later one we confirm this whole dma-heap thing will get deemed
acceptable in the first place.

Thanks,
Andrew

> thanks
> -john
>

Laura Abbott March 15, 2019, 8:18 p.m. UTC | #6

On 3/5/19 12:54 PM, John Stultz wrote:
> +DMA-BUF HEAPS FRAMEWORK
> +M:	Laura Abbott<[e-mail]>
> +R:	Liam Mark<[e-mail]>
> +R:	Brian Starkey<[e-mail]>
> +R:	"Andrew F. Davis"<[e-mail]>
> +R:	John Stultz<[e-mail]>
> +S:	Maintained
> +L:	[e-mail]
> +L:	[e-mail]
> +L:	[e-mail]  (moderated for non-subscribers)
> +F:	include/uapi/linux/dma-heap.h
> +F:	include/linux/dma-heap.h
> +F:	drivers/dma-buf/dma-heap.c
> +F:	drivers/dma-buf/heaps/*
> +T:	git git://anongit.freedesktop.org/drm/drm-misc

So I talked about this with Sumit privately but I think
it might make sense to have me step down as maintainer when
this goes out of staging. I mostly worked on Ion at my
previous position and anything I do now is mostly a side
project. I still want to see it succeed which is why I
took on the maintainer role but I don't want to become blocking
for people who have a stronger vision about where this needs
to go (see also, I'm not working with this on a daily basis).

If you just want someone to help review or take patches
to be pulled, I'm happy to do so but I'd hate to become
the bottleneck on getting things done for people who
are attempting to do real work.

Thanks,
Laura

Andrew Davis March 15, 2019, 8:24 p.m. UTC | #7

On 3/15/19 3:54 AM, Christoph Hellwig wrote:
>> +static int dma_heap_release(struct inode *inode, struct file *filp)
>> +{
>> +	filp->private_data = NULL;
>> +
>> +	return 0;
>> +}
> 
> No point in clearing ->private_data, the file is about to be freed.
> 

This was leftover from when release had some memory to free, will remove.

>> +
>> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd,
>> +			   unsigned long arg)
> 
> Pleae don't use the weird legacy filp naming, file is a perfectly
> valid and readable default name for struct file pointers.
> 

Thanks for info, I saw both used and this was used where I found the
prototype so I used it too, will fix.

>> +{
>> +	switch (cmd) {
>> +	case DMA_HEAP_IOC_ALLOC:
>> +	{
>> +		struct dma_heap_allocation_data heap_allocation;
>> +		struct dma_heap *heap = filp->private_data;
>> +		int fd;
> 
> Please split each ioctl into a separate function from the very start,
> otherwise this will grow into a spaghetty mess sooner than you can
> see cheese.
> 

Good idea, will fix.

>> +	dev_ret = device_create(dma_heap_class,
>> +				NULL,
>> +				heap->heap_devt,
>> +				NULL,
>> +				heap->name);
> 
> No need this weird argument alignment.
> 

I kinda like it this way, if everything cant fit on one line then
everything gets its own line, seems more consistent. If there is strong
objection I can fix.

Andrew Davis March 15, 2019, 8:49 p.m. UTC | #8

On 3/15/19 3:18 PM, Laura Abbott wrote:
> On 3/5/19 12:54 PM, John Stultz wrote:
>> +DMA-BUF HEAPS FRAMEWORK
>> +M:    Laura Abbott<[e-mail]>
>> +R:    Liam Mark<[e-mail]>
>> +R:    Brian Starkey<[e-mail]>
>> +R:    "Andrew F. Davis"<[e-mail]>
>> +R:    John Stultz<[e-mail]>
>> +S:    Maintained
>> +L:    [e-mail]
>> +L:    [e-mail]
>> +L:    [e-mail]  (moderated for non-subscribers)
>> +F:    include/uapi/linux/dma-heap.h
>> +F:    include/linux/dma-heap.h
>> +F:    drivers/dma-buf/dma-heap.c
>> +F:    drivers/dma-buf/heaps/*
>> +T:    git git://anongit.freedesktop.org/drm/drm-misc
> 
> So I talked about this with Sumit privately but I think
> it might make sense to have me step down as maintainer when
> this goes out of staging. I mostly worked on Ion at my
> previous position and anything I do now is mostly a side
> project. I still want to see it succeed which is why I
> took on the maintainer role but I don't want to become blocking
> for people who have a stronger vision about where this needs
> to go (see also, I'm not working with this on a daily basis).
> 
> If you just want someone to help review or take patches
> to be pulled, I'm happy to do so but I'd hate to become
> the bottleneck on getting things done for people who
> are attempting to do real work.
> 

We could consider this as an "ION inspired" framework, and treat it like
an extension of DMA-BUF. In which case Sumit could become the default
Maintainer if he's up for it.

Andrew

> Thanks,
> Laura

John Stultz March 15, 2019, 9:29 p.m. UTC | #9

On Fri, Mar 15, 2019 at 1:18 PM Laura Abbott <[e-mail]> wrote:
>
> On 3/5/19 12:54 PM, John Stultz wrote:
> > +DMA-BUF HEAPS FRAMEWORK
> > +M:   Laura Abbott<[e-mail]>
> > +R:   Liam Mark<[e-mail]>
> > +R:   Brian Starkey<[e-mail]>
> > +R:   "Andrew F. Davis"<[e-mail]>
> > +R:   John Stultz<[e-mail]>
> > +S:   Maintained
> > +L:   [e-mail]
> > +L:   [e-mail]
> > +L:   [e-mail]  (moderated for non-subscribers)
> > +F:   include/uapi/linux/dma-heap.h
> > +F:   include/linux/dma-heap.h
> > +F:   drivers/dma-buf/dma-heap.c
> > +F:   drivers/dma-buf/heaps/*
> > +T:   git git://anongit.freedesktop.org/drm/drm-misc
>
> So I talked about this with Sumit privately but I think
> it might make sense to have me step down as maintainer when
> this goes out of staging. I mostly worked on Ion at my
> previous position and anything I do now is mostly a side
> project. I still want to see it succeed which is why I
> took on the maintainer role but I don't want to become blocking
> for people who have a stronger vision about where this needs
> to go (see also, I'm not working with this on a daily basis).
>
> If you just want someone to help review or take patches
> to be pulled, I'm happy to do so but I'd hate to become
> the bottleneck on getting things done for people who
> are attempting to do real work.

I worry this will make everyone to touch the side of their nose and
yell "NOT IT!" :)

First of all, thank you so much for your efforts maintaining ION along
with your attempts to drag out requirements from interested parties
and the numerous attempts to get collaborative discussion going at
countless conferences! Your persistence and continual nudging in the
face of apathetic private users of the code probably cannot be
appreciated enough!

Your past practical experience with ION and active work with the
upstream community made you a stand out pick for this, but I
understand not wanting to be eternally stuck with a maintainership if
your not active in the area.  I'm happy to volunteer as a neutral
party, but I worry my limited experience with some of the more
complicated usage would make my opinions less informed then they
probably need to be.  Further, as a neutral party, Sumit would
probably be a better pick since he's already maintaining the dmabuf
core.

So I'd nominate Andrew, Liam or Benjamin (or all three?) as they all
have more practical experience enabling past ION heaps on real devices
and have demonstrated active interest in working in the community.

So, in other words... NOT IT! :)
-john

Laura Abbott March 15, 2019, 10:44 p.m. UTC | #10

On 3/15/19 2:29 PM, John Stultz wrote:
> On Fri, Mar 15, 2019 at 1:18 PM Laura Abbott <[e-mail]> wrote:
>>
>> On 3/5/19 12:54 PM, John Stultz wrote:
>>> +DMA-BUF HEAPS FRAMEWORK
>>> +M:   Laura Abbott<[e-mail]>
>>> +R:   Liam Mark<[e-mail]>
>>> +R:   Brian Starkey<[e-mail]>
>>> +R:   "Andrew F. Davis"<[e-mail]>
>>> +R:   John Stultz<[e-mail]>
>>> +S:   Maintained
>>> +L:   [e-mail]
>>> +L:   [e-mail]
>>> +L:   [e-mail]  (moderated for non-subscribers)
>>> +F:   include/uapi/linux/dma-heap.h
>>> +F:   include/linux/dma-heap.h
>>> +F:   drivers/dma-buf/dma-heap.c
>>> +F:   drivers/dma-buf/heaps/*
>>> +T:   git git://anongit.freedesktop.org/drm/drm-misc
>>
>> So I talked about this with Sumit privately but I think
>> it might make sense to have me step down as maintainer when
>> this goes out of staging. I mostly worked on Ion at my
>> previous position and anything I do now is mostly a side
>> project. I still want to see it succeed which is why I
>> took on the maintainer role but I don't want to become blocking
>> for people who have a stronger vision about where this needs
>> to go (see also, I'm not working with this on a daily basis).
>>
>> If you just want someone to help review or take patches
>> to be pulled, I'm happy to do so but I'd hate to become
>> the bottleneck on getting things done for people who
>> are attempting to do real work.
> 
> I worry this will make everyone to touch the side of their nose and
> yell "NOT IT!" :)
> 
> First of all, thank you so much for your efforts maintaining ION along
> with your attempts to drag out requirements from interested parties
> and the numerous attempts to get collaborative discussion going at
> countless conferences! Your persistence and continual nudging in the
> face of apathetic private users of the code probably cannot be
> appreciated enough!
> 
> Your past practical experience with ION and active work with the
> upstream community made you a stand out pick for this, but I
> understand not wanting to be eternally stuck with a maintainership if
> your not active in the area.  I'm happy to volunteer as a neutral
> party, but I worry my limited experience with some of the more
> complicated usage would make my opinions less informed then they
> probably need to be.  Further, as a neutral party, Sumit would
> probably be a better pick since he's already maintaining the dmabuf
> core.
> 

Honestly if you're doing the work to re-write everything, I
think you're more than qualified to be the maintainer. I
would also support Sumit as well.

> So I'd nominate Andrew, Liam or Benjamin (or all three?) as they all
> have more practical experience enabling past ION heaps on real devices
> and have demonstrated active interest in working in the community.
> 

I do think this would benefit both from multiple maintainers and
from maintainers who are actively using the framework. Like I
said, I can still be a maintainer but I think having some comaintainers
would be very helpful (and I'd support any of the names you've
suggested)

> So, in other words... NOT IT! :)

I think you have to shout "Noes goes" first. :)

> -john
> 

Thanks,
Laura

P.S. For the benefit of anyone who's confused,
https://en.wikipedia.org/wiki/Nose_goes

Sumit Semwal March 18, 2019, 4:38 a.m. UTC | #11

On Sat, 16 Mar 2019 at 04:15, Laura Abbott <[e-mail]> wrote:

> On 3/15/19 2:29 PM, John Stultz wrote:
> > On Fri, Mar 15, 2019 at 1:18 PM Laura Abbott <[e-mail]> wrote:
> >>
> >> On 3/5/19 12:54 PM, John Stultz wrote:
> >>> +DMA-BUF HEAPS FRAMEWORK
> >>> +M:   Laura Abbott<[e-mail]>
> >>> +R:   Liam Mark<[e-mail]>
> >>> +R:   Brian Starkey<[e-mail]>
> >>> +R:   "Andrew F. Davis"<[e-mail]>
> >>> +R:   John Stultz<[e-mail]>
> >>> +S:   Maintained
> >>> +L:   [e-mail]
> >>> +L:   [e-mail]
> >>> +L:   [e-mail]  (moderated for non-subscribers)
> >>> +F:   include/uapi/linux/dma-heap.h
> >>> +F:   include/linux/dma-heap.h
> >>> +F:   drivers/dma-buf/dma-heap.c
> >>> +F:   drivers/dma-buf/heaps/*
> >>> +T:   git git://anongit.freedesktop.org/drm/drm-misc
> >>
> >> So I talked about this with Sumit privately but I think
> >> it might make sense to have me step down as maintainer when
> >> this goes out of staging. I mostly worked on Ion at my
> >> previous position and anything I do now is mostly a side
> >> project. I still want to see it succeed which is why I
> >> took on the maintainer role but I don't want to become blocking
> >> for people who have a stronger vision about where this needs
> >> to go (see also, I'm not working with this on a daily basis).
> >>
> >> If you just want someone to help review or take patches
> >> to be pulled, I'm happy to do so but I'd hate to become
> >> the bottleneck on getting things done for people who
> >> are attempting to do real work.
> >
> > I worry this will make everyone to touch the side of their nose and
> > yell "NOT IT!" :)
> >
> > First of all, thank you so much for your efforts maintaining ION along
> > with your attempts to drag out requirements from interested parties
> > and the numerous attempts to get collaborative discussion going at
> > countless conferences! Your persistence and continual nudging in the
> > face of apathetic private users of the code probably cannot be
> > appreciated enough!
>
I totally second John here - the persistence has been inspiring to me
personally as well :)

> >
> > Your past practical experience with ION and active work with the
> > upstream community made you a stand out pick for this, but I
> > understand not wanting to be eternally stuck with a maintainership if
> > your not active in the area.  I'm happy to volunteer as a neutral
> > party, but I worry my limited experience with some of the more
> > complicated usage would make my opinions less informed then they
> > probably need to be.  Further, as a neutral party, Sumit would
> > probably be a better pick since he's already maintaining the dmabuf
> > core.
> >
>
> Honestly if you're doing the work to re-write everything, I
> think you're more than qualified to be the maintainer. I
> would also support Sumit as well.
> + 1
> > So I'd nominate Andrew, Liam or Benjamin (or all three?) as they all
> > have more practical experience enabling past ION heaps on real devices
> > and have demonstrated active interest in working in the community.
> >
>
> I do think this would benefit both from multiple maintainers and
> from maintainers who are actively using the framework. Like I
> said, I can still be a maintainer but I think having some comaintainers
> would be very helpful (and I'd support any of the names you've
> suggested)
>

If it's required, I am happy to co-maintain - we could even follow the
drm-misc model of multiple co-maintainers. I will support any or all of the
names above as well :)

>
> > So, in other words... NOT IT! :)
>
> I think you have to shout "Noes goes" first. :)
>
> > -john
> >
>
> Thanks,
> Laura
>
> P.S. For the benefit of anyone who's confused,
> https://en.wikipedia.org/wiki/Nose_goes
>

Best,
Sumit.
<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 16 Mar 2019 at 04:15, Laura Abbott &lt;<a href="mailto:[e-mail]">[e-mail]</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 3/15/19 2:29 PM, John Stultz wrote:<br>
&gt; On Fri, Mar 15, 2019 at 1:18 PM Laura Abbott &lt;<a href="mailto:[e-mail]" target="_blank">[e-mail]</a>&gt; wrote:<br>
&gt;&gt;<br>
&gt;&gt; On 3/5/19 12:54 PM, John Stultz wrote:<br>
&gt;&gt;&gt; +DMA-BUF HEAPS FRAMEWORK<br>
&gt;&gt;&gt; +M:   Laura Abbott&lt;<a href="mailto:[e-mail]" target="_blank">[e-mail]</a>&gt;<br>
&gt;&gt;&gt; +R:   Liam Mark&lt;<a href="mailto:[e-mail]" target="_blank">[e-mail]</a>&gt;<br>
&gt;&gt;&gt; +R:   Brian Starkey&lt;<a href="mailto:[e-mail]" target="_blank">[e-mail]</a>&gt;<br>
&gt;&gt;&gt; +R:   &quot;Andrew F. Davis&quot;&lt;<a href="mailto:[e-mail]" target="_blank">[e-mail]</a>&gt;<br>
&gt;&gt;&gt; +R:   John Stultz&lt;<a href="mailto:[e-mail]" target="_blank">[e-mail]</a>&gt;<br>
&gt;&gt;&gt; +S:   Maintained<br>
&gt;&gt;&gt; +L:   <a href="mailto:[e-mail]" target="_blank">[e-mail]</a><br>
&gt;&gt;&gt; +L:   <a href="mailto:[e-mail]" target="_blank">[e-mail]</a><br>
&gt;&gt;&gt; +L:   <a href="mailto:[e-mail]" target="_blank">[e-mail]</a>  (moderated for non-subscribers)<br>
&gt;&gt;&gt; +F:   include/uapi/linux/dma-heap.h<br>
&gt;&gt;&gt; +F:   include/linux/dma-heap.h<br>
&gt;&gt;&gt; +F:   drivers/dma-buf/dma-heap.c<br>
&gt;&gt;&gt; +F:   drivers/dma-buf/heaps/*<br>
&gt;&gt;&gt; +T:   git git://<a href="http://anongit.freedesktop.org/drm/drm-misc" rel="noreferrer" target="_blank">anongit.freedesktop.org/drm/drm-misc</a><br>
&gt;&gt;<br>
&gt;&gt; So I talked about this with Sumit privately but I think<br>
&gt;&gt; it might make sense to have me step down as maintainer when<br>
&gt;&gt; this goes out of staging. I mostly worked on Ion at my<br>
&gt;&gt; previous position and anything I do now is mostly a side<br>
&gt;&gt; project. I still want to see it succeed which is why I<br>
&gt;&gt; took on the maintainer role but I don&#39;t want to become blocking<br>
&gt;&gt; for people who have a stronger vision about where this needs<br>
&gt;&gt; to go (see also, I&#39;m not working with this on a daily basis).<br>
&gt;&gt;<br>
&gt;&gt; If you just want someone to help review or take patches<br>
&gt;&gt; to be pulled, I&#39;m happy to do so but I&#39;d hate to become<br>
&gt;&gt; the bottleneck on getting things done for people who<br>
&gt;&gt; are attempting to do real work.<br>
&gt; <br>
&gt; I worry this will make everyone to touch the side of their nose and<br>
&gt; yell &quot;NOT IT!&quot; :)<br>
&gt; <br>
&gt; First of all, thank you so much for your efforts maintaining ION along<br>
&gt; with your attempts to drag out requirements from interested parties<br>
&gt; and the numerous attempts to get collaborative discussion going at<br>
&gt; countless conferences! Your persistence and continual nudging in the<br>
&gt; face of apathetic private users of the code probably cannot be<br>
&gt; appreciated enough!<br></blockquote><div>I totally second John here - the persistence has been inspiring to me personally as well :) </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
&gt; <br>
&gt; Your past practical experience with ION and active work with the<br>
&gt; upstream community made you a stand out pick for this, but I<br>
&gt; understand not wanting to be eternally stuck with a maintainership if<br>
&gt; your not active in the area.  I&#39;m happy to volunteer as a neutral<br>
&gt; party, but I worry my limited experience with some of the more<br>
&gt; complicated usage would make my opinions less informed then they<br>
&gt; probably need to be.  Further, as a neutral party, Sumit would<br>
&gt; probably be a better pick since he&#39;s already maintaining the dmabuf<br>
&gt; core.<br>
&gt; <br>
<br>
Honestly if you&#39;re doing the work to re-write everything, I<br>
think you&#39;re more than qualified to be the maintainer. I<br>
would also support Sumit as well.<br>
+ 1<br>
&gt; So I&#39;d nominate Andrew, Liam or Benjamin (or all three?) as they all<br>
&gt; have more practical experience enabling past ION heaps on real devices<br>
&gt; and have demonstrated active interest in working in the community.<br>
&gt; <br>
<br>
I do think this would benefit both from multiple maintainers and<br>
from maintainers who are actively using the framework. Like I<br>
said, I can still be a maintainer but I think having some comaintainers<br>
would be very helpful (and I&#39;d support any of the names you&#39;ve<br>
suggested)<br></blockquote><div><br></div><div>If it&#39;s required, I am happy to co-maintain - we could even follow the drm-misc model of multiple co-maintainers. I will support any or all of the names above as well :) </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
&gt; So, in other words... NOT IT! :)<br>
<br>
I think you have to shout &quot;Noes goes&quot; first. :)<br>
<br>
&gt; -john<br>
&gt; <br>
<br>
Thanks,<br>
Laura<br>
<br>
P.S. For the benefit of anyone who&#39;s confused,<br>
<a href="https://en.wikipedia.org/wiki/Nose_goes" rel="noreferrer" target="_blank">https://en.wikipedia.org/wiki/Nose_goes</a><br>
</blockquote></div><br><div>Best,</div><div>Sumit.</div></div>

Sumit Semwal March 18, 2019, 4:41 a.m. UTC | #12

On Sat, 16 Mar 2019 at 04:15, Laura Abbott <[e-mail]> wrote:
>
> On 3/15/19 2:29 PM, John Stultz wrote:
> > On Fri, Mar 15, 2019 at 1:18 PM Laura Abbott <[e-mail]> wrote:
> >>
> >> On 3/5/19 12:54 PM, John Stultz wrote:
> >>> +DMA-BUF HEAPS FRAMEWORK
> >>> +M:   Laura Abbott<[e-mail]>
> >>> +R:   Liam Mark<[e-mail]>
> >>> +R:   Brian Starkey<[e-mail]>
> >>> +R:   "Andrew F. Davis"<[e-mail]>
> >>> +R:   John Stultz<[e-mail]>
> >>> +S:   Maintained
> >>> +L:   [e-mail]
> >>> +L:   [e-mail]
> >>> +L:   [e-mail]  (moderated for non-subscribers)
> >>> +F:   include/uapi/linux/dma-heap.h
> >>> +F:   include/linux/dma-heap.h
> >>> +F:   drivers/dma-buf/dma-heap.c
> >>> +F:   drivers/dma-buf/heaps/*
> >>> +T:   git git://anongit.freedesktop.org/drm/drm-misc
> >>
> >> So I talked about this with Sumit privately but I think
> >> it might make sense to have me step down as maintainer when
> >> this goes out of staging. I mostly worked on Ion at my
> >> previous position and anything I do now is mostly a side
> >> project. I still want to see it succeed which is why I
> >> took on the maintainer role but I don't want to become blocking
> >> for people who have a stronger vision about where this needs
> >> to go (see also, I'm not working with this on a daily basis).
> >>
> >> If you just want someone to help review or take patches
> >> to be pulled, I'm happy to do so but I'd hate to become
> >> the bottleneck on getting things done for people who
> >> are attempting to do real work.
> >
> > I worry this will make everyone to touch the side of their nose and
> > yell "NOT IT!" :)
> >
> > First of all, thank you so much for your efforts maintaining ION along
> > with your attempts to drag out requirements from interested parties
> > and the numerous attempts to get collaborative discussion going at
> > countless conferences! Your persistence and continual nudging in the
> > face of apathetic private users of the code probably cannot be
> > appreciated enough!
I totally second John here - the persistence has been inspiring to me
personally as well :)

> >
> > Your past practical experience with ION and active work with the
> > upstream community made you a stand out pick for this, but I
> > understand not wanting to be eternally stuck with a maintainership if
> > your not active in the area.  I'm happy to volunteer as a neutral
> > party, but I worry my limited experience with some of the more
> > complicated usage would make my opinions less informed then they
> > probably need to be.  Further, as a neutral party, Sumit would
> > probably be a better pick since he's already maintaining the dmabuf
> > core.
> >
>
> Honestly if you're doing the work to re-write everything, I
> think you're more than qualified to be the maintainer. I
> would also support Sumit as well.
>
> > So I'd nominate Andrew, Liam or Benjamin (or all three?) as they all
> > have more practical experience enabling past ION heaps on real devices
> > and have demonstrated active interest in working in the community.
> >
>
> I do think this would benefit both from multiple maintainers and
> from maintainers who are actively using the framework. Like I
> said, I can still be a maintainer but I think having some comaintainers
> would be very helpful (and I'd support any of the names you've
> suggested)
If it's required, I am happy to co-maintain - we could even follow the
drm-misc model of multiple co-maintainers. I will support any or all
of the names above as well :)

>
> > So, in other words... NOT IT! :)
>
> I think you have to shout "Noes goes" first. :)
>
> > -john
> >
>
> Thanks,
> Laura
>
> P.S. For the benefit of anyone who's confused,
> https://en.wikipedia.org/wiki/Nose_goes


Best,
Sumit.

Brian Starkey March 19, 2019, 12:08 p.m. UTC | #13

Hi John,

On Tue, Mar 05, 2019 at 12:54:29PM -0800, John Stultz wrote:
> From: "Andrew F. Davis" <[e-mail]>

[snip]

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

I saw that Matthew Wilcox is trying to nuke idr:
https://patchwork.freedesktop.org/series/57073/

Perhaps a different data structure could be considered? (I don't have
an informed opinion on which).

> +
> +dev_t dma_heap_devt;
> +struct class *dma_heap_class;
> +struct list_head dma_heap_list;
> +struct dentry *dma_heap_debug_root;
> +
> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				 unsigned int flags)
> +{
> +	len = PAGE_ALIGN(len);
> +	if (!len)
> +		return -EINVAL;

I think aligning len to pages only makes sense if heaps are going to
allocate aligned to pages too. Perhaps that's an implicit assumption?
If so, lets document it.

Why not let the heaps take care of aligning len however they want
though?

...

> +
> +int dma_heap_add(struct dma_heap *heap)
> +{
> +	struct device *dev_ret;
> +	int ret;
> +
> +	if (!heap->name || !strcmp(heap->name, "")) {
> +		pr_err("dma_heap: Cannot add heap without a name\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!heap->ops || !heap->ops->allocate) {
> +		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
> +		return -EINVAL;
> +	}
> +
> +	/* 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);

Shouldn't this be s/dma_heap_devt/heap->heap_devt/ and a count of 1?

Also would it be better to have cdev_add/device_create the other way
around? First create the char device, then once it's all set up
register it with sysfs.

> +	if (ret < 0) {
> +		device_destroy(dma_heap_class, heap->heap_devt);
> +		pr_err("dma_heap: Unable to add char device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(dma_heap_add);

Until we've figured out how modules are going to work, I still think
it would be a good idea to not export this.

Cheers,
-Brian

Andrew Davis March 19, 2019, 3:24 p.m. UTC | #14

On 3/19/19 7:08 AM, Brian Starkey wrote:
> Hi John,
> 
> On Tue, Mar 05, 2019 at 12:54:29PM -0800, John Stultz wrote:
>> From: "Andrew F. Davis" <[e-mail]>
> 
> [snip]
> 
>> +
>> +#define NUM_HEAP_MINORS 128
>> +static DEFINE_IDR(dma_heap_idr);
>> +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
> 
> I saw that Matthew Wilcox is trying to nuke idr:
> https://patchwork.freedesktop.org/series/57073/
> 
> Perhaps a different data structure could be considered? (I don't have
> an informed opinion on which).
> 

Looks like XArray is the suggested replacement. Should be easy enough,
the minor number would just index to our heap struct directly, I'll give
it a shot and see.

>> +
>> +dev_t dma_heap_devt;
>> +struct class *dma_heap_class;
>> +struct list_head dma_heap_list;
>> +struct dentry *dma_heap_debug_root;
>> +
>> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
>> +				 unsigned int flags)
>> +{
>> +	len = PAGE_ALIGN(len);
>> +	if (!len)
>> +		return -EINVAL;
> 
> I think aligning len to pages only makes sense if heaps are going to
> allocate aligned to pages too. Perhaps that's an implicit assumption?
> If so, lets document it.
> 
> Why not let the heaps take care of aligning len however they want
> though?
> 

This is how I originally had it, but I think we couldn't find any case
where you would want an the start or end of a buffer to not fall on a
page boundary here. It would only lead to problems. As you say though,
nothing keeping us from moving that into the heaps themselves.

> ...
> 
>> +
>> +int dma_heap_add(struct dma_heap *heap)
>> +{
>> +	struct device *dev_ret;
>> +	int ret;
>> +
>> +	if (!heap->name || !strcmp(heap->name, "")) {
>> +		pr_err("dma_heap: Cannot add heap without a name\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!heap->ops || !heap->ops->allocate) {
>> +		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* 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);
> 
> Shouldn't this be s/dma_heap_devt/heap->heap_devt/ and a count of 1?
> 

Hmm, strange this ever worked before..

> Also would it be better to have cdev_add/device_create the other way
> around? First create the char device, then once it's all set up
> register it with sysfs.
> 

Yes that does seem to be more common, lets flip it.

>> +	if (ret < 0) {
>> +		device_destroy(dma_heap_class, heap->heap_devt);
>> +		pr_err("dma_heap: Unable to add char device\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(dma_heap_add);
> 
> Until we've figured out how modules are going to work, I still think
> it would be a good idea to not export this.
> 

Agree.

Andrew

> Cheers,
> -Brian
>

John Stultz March 21, 2019, 9:16 p.m. UTC | #15

On Tue, Mar 19, 2019 at 5:08 AM Brian Starkey <[e-mail]> wrote:
>
> Hi John,
>
> On Tue, Mar 05, 2019 at 12:54:29PM -0800, John Stultz wrote:
> > From: "Andrew F. Davis" <[e-mail]>
>
> [snip]
>
> > +
> > +#define NUM_HEAP_MINORS 128
> > +static DEFINE_IDR(dma_heap_idr);
> > +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
>
> I saw that Matthew Wilcox is trying to nuke idr:
> https://patchwork.freedesktop.org/series/57073/
>
> Perhaps a different data structure could be considered? (I don't have
> an informed opinion on which).

Thanks for pointing this out! I've just switched to using the Xarray
implementation in my tree.

> > +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> > +                              unsigned int flags)
> > +{
> > +     len = PAGE_ALIGN(len);
> > +     if (!len)
> > +             return -EINVAL;
>
> I think aligning len to pages only makes sense if heaps are going to
> allocate aligned to pages too. Perhaps that's an implicit assumption?
> If so, lets document it.

I've added a comment as such (or do you have more thoughts on where it
should be documented?), and for consistency removed the PAGE_ALIGN
usage in the heap allocator hooks.

> Why not let the heaps take care of aligning len however they want
> though?

As Andrew already said, It seems page granularity would have to be the
finest allocation granularity for dmabufs.  If heaps want to implement
their own larger granularity alignment, I don't see any reason they
would be limited there.

And for me, its mostly because I stubbed my toe implementing the heap
code w/ the first patch that didn't have the page alignment in the
generic code. :)

> > +     /* 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);
>
> Shouldn't this be s/dma_heap_devt/heap->heap_devt/ and a count of 1?
>
> Also would it be better to have cdev_add/device_create the other way
> around? First create the char device, then once it's all set up
> register it with sysfs.

Thanks for catching that! Much appreciated! Reworked as suggested.

Though I realized last week I have not figured out a consistent way to
have the heaps show up in /dev/dma_heaps/<device> on both Android and
classic Linux environments.  I need to go stare at the /dev/input/
setup code some more.

> > +     if (ret < 0) {
> > +             device_destroy(dma_heap_class, heap->heap_devt);
> > +             pr_err("dma_heap: Unable to add char device\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(dma_heap_add);
>
> Until we've figured out how modules are going to work, I still think
> it would be a good idea to not export this.

Done!

thanks
-john

Greg KH March 27, 2019, 2:53 p.m. UTC | #16

On Tue, Mar 05, 2019 at 12:54:29PM -0800, John Stultz wrote:
> From: "Andrew F. Davis" <[e-mail]>
> 
> 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.
> 
> This code is an evoluiton of the Android ION implementation,
> and a big thanks is due to its authors/maintainers over time
> for their effort:
>   Rebecca Schultz Zavin, Colin Cross, Benjamin Gaignard,
>   Laura Abbott, and many other contributors!

Comments just on the user/kernel api and how it interacts with the
driver model.  Not on the "real" functionality of this code :)

> +#define DEVNAME "dma_heap"
> +
> +#define NUM_HEAP_MINORS 128

Why a max?

> +static DEFINE_IDR(dma_heap_idr);
> +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */

Move to use xarray now so that Matthew doesn't have to send patches
converting this code later :)

It also allows you to drop the mutex.

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

Global variables?

> +
> +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				 unsigned int flags)
> +{
> +	len = PAGE_ALIGN(len);
> +	if (!len)
> +		return -EINVAL;
> +
> +	return heap->ops->allocate(heap, len, flags);
> +}
> +
> +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;

Why does this matter?  release should only be called on the way out of
here, no need to do anything as nothing else can be called, right?

release shouldn't be needed from what I can tell.

> +
> +	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,
> +				   sizeof(heap_allocation)))
> +			return -EFAULT;
> +
> +		if (heap_allocation.fd ||
> +		    heap_allocation.reserved0 ||
> +		    heap_allocation.reserved1 ||
> +		    heap_allocation.reserved2) {
> +			pr_warn_once("dma_heap: ioctl data not valid\n");
> +			return -EINVAL;
> +		}

Good job in forcing the reserved fields to be 0!

> +		if (heap_allocation.flags & ~DMA_HEAP_VALID_FLAGS) {
> +			pr_warn_once("dma_heap: flags has invalid or unsupported flags set\n");
> +			return -EINVAL;
> +		}
> +
> +		fd = dma_heap_buffer_alloc(heap, heap_allocation.len,
> +					   heap_allocation.flags);

No max value checking for .len?  Can you really ask for anything?

> +		if (fd < 0)
> +			return fd;
> +
> +		heap_allocation.fd = fd;
> +
> +		if (copy_to_user((void __user *)arg, &heap_allocation,
> +				 sizeof(heap_allocation)))
> +			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

Why is compat_ioctl even needed?

> +};
> +
> +int dma_heap_add(struct dma_heap *heap)
> +{
> +	struct device *dev_ret;
> +	int ret;
> +
> +	if (!heap->name || !strcmp(heap->name, "")) {
> +		pr_err("dma_heap: Cannot add heap without a name\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!heap->ops || !heap->ops->allocate) {
> +		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
> +		return -EINVAL;
> +	}
> +
> +	/* 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);

Again, xarray.

But I will ask you to back up, why need a major number at all?  Why not
just use the misc subsystem?  How many of these are you going to have
over time in a "normal" system?  How about a "abnormal system"?

We have seen people running Android in "containers" such that they
needed binderfs to handle huge numbers of individual android systems
running at the same time.  Will this api break those systems if you have
a tiny maximum number you an allocate?

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

No parent?  Can't hang this off of anything?  Ok, having it show up in
/sys/devices/virtual/ is probably good enough.

> +	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;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(dma_heap_add);

EXPORT_SYMBOL_GPL() please?  For core stuff like this it's good.

> +
> +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);
> +	}
> +
> +	return 0;
> +}
> +subsys_initcall(dma_heap_init);

Overall, looks sane, the comments above are all really minor.


> --- /dev/null
> +++ b/include/uapi/linux/dma-heap.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Wrong license for a uapi .h file :(

thanks,

greg k-h

John Stultz March 28, 2019, 6:09 a.m. UTC | #17

On Wed, Mar 27, 2019 at 11:25 AM Greg KH <[e-mail]> wrote:
>
> On Tue, Mar 05, 2019 at 12:54:29PM -0800, John Stultz wrote:
> > From: "Andrew F. Davis" <[e-mail]>
> >
> > 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.
> >
> > This code is an evoluiton of the Android ION implementation,
> > and a big thanks is due to its authors/maintainers over time
> > for their effort:
> >   Rebecca Schultz Zavin, Colin Cross, Benjamin Gaignard,
> >   Laura Abbott, and many other contributors!
>
> Comments just on the user/kernel api and how it interacts with the
> driver model.  Not on the "real" functionality of this code :)

Thanks so much for the feedback! In some cases Andrew and I have
already made the changes you've suggested, and hopefully will have a
new version to share soon.


> > +#define DEVNAME "dma_heap"
> > +
> > +#define NUM_HEAP_MINORS 128
>
> Why a max?

Mostly because other drivers do. I'll see if this can be removed with
the Xarray bits.


> > +static DEFINE_IDR(dma_heap_idr);
> > +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
>
> Move to use xarray now so that Matthew doesn't have to send patches
> converting this code later :)
>
> It also allows you to drop the mutex.

Yep. Already converted to Xarray, it is nicer!


> > +dev_t dma_heap_devt;
> > +struct class *dma_heap_class;
> > +struct list_head dma_heap_list;
> > +struct dentry *dma_heap_debug_root;
>
> Global variables?

Oops. Will make those static. Thanks!

> > +
> > +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> > +                              unsigned int flags)
> > +{
> > +     len = PAGE_ALIGN(len);
> > +     if (!len)
> > +             return -EINVAL;
> > +
> > +     return heap->ops->allocate(heap, len, flags);
> > +}
> > +
> > +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;
>
> Why does this matter?  release should only be called on the way out of
> here, no need to do anything as nothing else can be called, right?
>
> release shouldn't be needed from what I can tell.

Yep. Christoph suggested the same and its been removed already.


> > +             if (heap_allocation.flags & ~DMA_HEAP_VALID_FLAGS) {
> > +                     pr_warn_once("dma_heap: flags has invalid or unsupported flags set\n");
> > +                     return -EINVAL;
> > +             }
> > +
> > +             fd = dma_heap_buffer_alloc(heap, heap_allocation.len,
> > +                                        heap_allocation.flags);
>
> No max value checking for .len?  Can you really ask for anything?

So I think any length caps would be heap specific, so we want to pass
them on here.

> > +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
>
> Why is compat_ioctl even needed?

Probably my mistake. I didn't realize if we're running 32bit on 64bit
and there's no compat, the unlocked_ioctl gets called.


> > +     /* 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);
>
> Again, xarray.

Ack.

> But I will ask you to back up, why need a major number at all?  Why not
> just use the misc subsystem?  How many of these are you going to have
> over time in a "normal" system?  How about a "abnormal system"?

So early implementations did use misc, but in order to get the
/dev/heap/cma_heap style directories, in both Android and classic udev
linux systems I had to create a class.

This v2 patch didn't get it quite right (got it working properly in
android but not on classic systems), but the next version does get the
subdir created properly (similar to how the input layer does it).

As for number of heaps, I wouldn't expect there to be a ton on any
given system. Most likely less then 16, but possibly up to 32. 128
seemed like a safe "crazy out there" cap. But perspectives on crazy
shift over time :)

> We have seen people running Android in "containers" such that they
> needed binderfs to handle huge numbers of individual android systems
> running at the same time.  Will this api break those systems if you have
> a tiny maximum number you an allocate?

I'd have to think some more on this. Right now I'd expect that you'd
not be trying to virtualize the heaps in a container so you'd not have
m heaps * n containers on the system. Instead the containers would
mount/link in the devnode (I'm a bit fuzzy on how containers handle
devnode creation/restrictions) as appropriate (the nice part with this
over ION is we have per heap dev nodes, so the set shared can be
limited).  But I'd have to think more about the risks of how multiple
containers might share things like cma heaps.


> > +     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);
>
> No parent?  Can't hang this off of anything?  Ok, having it show up in
> /sys/devices/virtual/ is probably good enough.
>
> > +     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;
> > +     }
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(dma_heap_add);
>
> EXPORT_SYMBOL_GPL() please?  For core stuff like this it's good.

Actually, removed the export completely for now since its probably not
ready for modules yet. But will be sure to tag it GPL when we do
re-add it.


> > +
> > +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);
> > +     }
> > +
> > +     return 0;
> > +}
> > +subsys_initcall(dma_heap_init);
>
> Overall, looks sane, the comments above are all really minor.

Very much appreciate the review!


>
> > --- /dev/null
> > +++ b/include/uapi/linux/dma-heap.h
> > @@ -0,0 +1,52 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> Wrong license for a uapi .h file :(

Ack. Fixed.

Thanks so much again!
-john

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index ac2e518..a661e19 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4621,6 +4621,22 @@  F:	include/linux/*fence.h
 F:	Documentation/driver-api/dma-buf.rst
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 
+DMA-BUF HEAPS FRAMEWORK
+M:	Laura Abbott <[e-mail]>
+R:	Liam Mark <[e-mail]>
+R:	Brian Starkey <[e-mail]>
+R:	"Andrew F. Davis" <[e-mail]>
+R:	John Stultz <[e-mail]>
+S:	Maintained
+L:	[e-mail]
+L:	[e-mail]
+L:	[e-mail] (moderated for non-subscribers)
+F:	include/uapi/linux/dma-heap.h
+F:	include/linux/dma-heap.h
+F:	drivers/dma-buf/dma-heap.c
+F:	drivers/dma-buf/heaps/*
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+
 DMA GENERIC OFFLOAD ENGINE SUBSYSTEM
 M:	Vinod Koul <[e-mail]>
 L:	[e-mail]
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 2e5a0fa..09c61db 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -39,4 +39,12 @@  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"
+	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.
+
 endmenu
diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 0913a6c..b0332f1 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 0000000..14b3975
--- /dev/null
+++ b/drivers/dma-buf/dma-heap.c
@@ -0,0 +1,191 @@ 
+// 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;
+
+static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
+				 unsigned int flags)
+{
+	len = PAGE_ALIGN(len);
+	if (!len)
+		return -EINVAL;
+
+	return heap->ops->allocate(heap, len, flags);
+}
+
+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,
+				   sizeof(heap_allocation)))
+			return -EFAULT;
+
+		if (heap_allocation.fd ||
+		    heap_allocation.reserved0 ||
+		    heap_allocation.reserved1 ||
+		    heap_allocation.reserved2) {
+			pr_warn_once("dma_heap: ioctl data not valid\n");
+			return -EINVAL;
+		}
+		if (heap_allocation.flags & ~DMA_HEAP_VALID_FLAGS) {
+			pr_warn_once("dma_heap: flags has invalid or unsupported flags set\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,
+				 sizeof(heap_allocation)))
+			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 *heap)
+{
+	struct device *dev_ret;
+	int ret;
+
+	if (!heap->name || !strcmp(heap->name, "")) {
+		pr_err("dma_heap: Cannot add heap without a name\n");
+		return -EINVAL;
+	}
+
+	if (!heap->ops || !heap->ops->allocate) {
+		pr_err("dma_heap: Cannot add heap with invalid ops struct\n");
+		return -EINVAL;
+	}
+
+	/* 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;
+	}
+
+	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);
+	}
+
+	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 0000000..ed86a8e
--- /dev/null
+++ b/include/linux/dma-heap.h
@@ -0,0 +1,65 @@ 
+/* 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/cdev.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 - 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
+ *
+ * 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;
+};
+
+/**
+ * struct dma_heap_ops - ops to operate on a given heap
+ * @allocate:		allocate dmabuf and return fd
+ *
+ * allocate returns dmabuf fd  on success, -errno on error.
+ */
+struct dma_heap_ops {
+	int (*allocate)(struct dma_heap *heap,
+			unsigned long len,
+			unsigned long flags);
+};
+
+/**
+ * dma_heap_add - adds a heap to dmabuf heaps
+ * @heap:		the heap to add
+ */
+int dma_heap_add(struct dma_heap *heap);
+
+#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 0000000..75c5d3f
--- /dev/null
+++ b/include/uapi/linux/dma-heap.h
@@ -0,0 +1,52 @@ 
+/* 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
+ *
+ */
+
+/* Currently no flags */
+#define DMA_HEAP_VALID_FLAGS (0)
+
+/**
+ * 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;
+	__u64 flags;
+	__u32 fd;
+	__u32 reserved0;
+	__u32 reserved1;
+	__u32 reserved2;
+};
+
+#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 */