Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework
To:     John Stultz <john.stultz@linaro.org>,
        lkml <linux-kernel@vger.kernel.org>
CC:     Laura Abbott <labbott@redhat.com>,
        Benjamin Gaignard <benjamin.gaignard@linaro.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Liam Mark <lmark@codeaurora.org>,
        Brian Starkey <Brian.Starkey@arm.com>,
        Chenbo Feng <fengc@google.com>,
        Alistair Strachan <astrachan@google.com>,
        <dri-devel@lists.freedesktop.org>
References: <1551819273-640-1-git-send-email-john.stultz@linaro.org>
 <1551819273-640-2-git-send-email-john.stultz@linaro.org>
From:   "Andrew F. Davis" <afd@ti.com>
Message-ID: <a4fd1783-c0d8-8ed5-5cd0-522b0789998c@ti.com>
Date:   Wed, 6 Mar 2019 10:27:34 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <1551819273-640-2-git-send-email-john.stultz@linaro.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 3/5/19 2:54 PM, John Stultz wrote:
> From: "Andrew F. Davis" <afd@ti.com>
> 
> 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 <labbott@redhat.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Chenbo Feng <fengc@google.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> [jstultz: reworded commit message, and lots of cleanups]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> 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 <labbott@redhat.com>
> +R:	Liam Mark <lmark@codeaurora.org>
> +R:	Brian Starkey <Brian.Starkey@arm.com>
> +R:	"Andrew F. Davis" <afd@ti.com>

Quotes not needed in maintainers file.

> +R:	John Stultz <john.stultz@linaro.org>
> +S:	Maintained
> +L:	linux-media@vger.kernel.org
> +L:	dri-devel@lists.freedesktop.org
> +L:	linaro-mm-sig@lists.linaro.org (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 <vkoul@kernel.org>
>  L:	dmaengine@vger.kernel.org
> 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 */
>