Re: [RFC][PATCH 3/5 v2] dma-buf: heaps: Add system heap to dmabuf heaps

[Liam Mark <lmark@codeaurora.org>]

On Tue, 5 Mar 2019, John Stultz wrote:

> This patch adds system heap to the dma-buf heaps framework.
> 
> This allows applications to get a page-allocator backed dma-buf
> for non-contiguous memory.
> 
> This code is an evolution of the Android ION implementation, so
> thanks to its original authors and maintainters:
>   Rebecca Schultz Zavin, Colin Cross, Laura Abbott, and others!
> 
> 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: John Stultz <john.stultz@linaro.org>
> ---
> v2:
> * Switch allocate to return dmabuf fd
> * Simplify init code
> * Checkpatch fixups
> * Droped dead system-contig code
> ---
>  drivers/dma-buf/Kconfig             |   2 +
>  drivers/dma-buf/heaps/Kconfig       |   6 ++
>  drivers/dma-buf/heaps/Makefile      |   1 +
>  drivers/dma-buf/heaps/system_heap.c | 132 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 141 insertions(+)
>  create mode 100644 drivers/dma-buf/heaps/Kconfig
>  create mode 100644 drivers/dma-buf/heaps/system_heap.c
> 
> diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
> index 09c61db..63c139d 100644
> --- a/drivers/dma-buf/Kconfig
> +++ b/drivers/dma-buf/Kconfig
> @@ -47,4 +47,6 @@ menuconfig DMABUF_HEAPS
>  	  this allows userspace to allocate dma-bufs that can be shared between
>  	  drivers.
>  
> +source "drivers/dma-buf/heaps/Kconfig"
> +
>  endmenu
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> new file mode 100644
> index 0000000..2050527
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -0,0 +1,6 @@
> +config DMABUF_HEAPS_SYSTEM
> +	bool "DMA-BUF System Heap"
> +	depends on DMABUF_HEAPS
> +	help
> +	  Choose this option to enable the system dmabuf heap. The system heap
> +	  is backed by pages from the buddy allocator. If in doubt, say Y.
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index de49898..d1808ec 100644
> --- a/drivers/dma-buf/heaps/Makefile
> +++ b/drivers/dma-buf/heaps/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-y					+= heap-helpers.o
> +obj-$(CONFIG_DMABUF_HEAPS_SYSTEM)	+= system_heap.o
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> new file mode 100644
> index 0000000..e001661
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF System heap exporter
> + *
> + * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2019 Linaro Ltd.
> + */
> +
> +#include <asm/page.h>
> +#include <linux/dma-buf.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dma-heap.h>
> +#include <linux/err.h>
> +#include <linux/highmem.h>
> +#include <linux/mm.h>
> +#include <linux/scatterlist.h>
> +#include <linux/slab.h>
> +
> +#include "heap-helpers.h"
> +
> +
> +struct system_heap {
> +	struct dma_heap heap;
> +};
> +
> +
> +static void system_heap_free(struct heap_helper_buffer *buffer)
> +{
> +	int i;
> +	struct scatterlist *sg;
> +	struct sg_table *table = buffer->sg_table;
> +
> +	for_each_sg(table->sgl, sg, table->nents, i)
> +		__free_page(sg_page(sg));
> +
> +	sg_free_table(table);
> +	kfree(table);
> +	kfree(buffer);
> +}
> +
> +static int system_heap_allocate(struct dma_heap *heap,
> +				unsigned long len,
> +				unsigned long flags)
> +{
> +	struct heap_helper_buffer *helper_buffer;
> +	struct sg_table *table;
> +	struct scatterlist *sg;
> +	int i, j;
> +	int npages = PAGE_ALIGN(len) / PAGE_SIZE;
> +	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> +	struct dma_buf *dmabuf;
> +	int ret = -ENOMEM;
> +
> +	helper_buffer = kzalloc(sizeof(*helper_buffer), GFP_KERNEL);
> +	if (!helper_buffer)
> +		return -ENOMEM;
> +
> +	INIT_HEAP_HELPER_BUFFER(helper_buffer, system_heap_free);
> +	helper_buffer->heap_buffer.flags = flags;
> +	helper_buffer->heap_buffer.heap = heap;
> +	helper_buffer->heap_buffer.size = len;
> +
> +	table = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
> +	if (!table)
> +		goto err0;
> +
> +	i = sg_alloc_table(table, npages, GFP_KERNEL);
> +	if (i)
> +		goto err1;
> +	for_each_sg(table->sgl, sg, table->nents, i) {
> +		struct page *page;
> +
> +		page = alloc_page(GFP_KERNEL);

Need to zero the allocation (add __GFP_ZERO)

> +		if (!page)
> +			goto err2;
> +		sg_set_page(sg, page, PAGE_SIZE, 0);
> +	}
> +

Can always be done later, but it may be helpful to also move this common 
code from here (and from the cma heap) to the heap helpers file as it 
reduces code but will also make it easier to introduce future debug 
features such as making the dma buf names unique
to help make it easier to track down the source of memory leaks.

> +	 /* create the dmabuf */ 
> +	 exp_info.ops = &heap_helper_ops; 
> +	exp_info.size = len;
> +	exp_info.flags = O_RDWR;
> +	exp_info.priv = &helper_buffer->heap_buffer;
> +	dmabuf = dma_buf_export(&exp_info);
> +	if (IS_ERR(dmabuf)) {
> +		ret = PTR_ERR(dmabuf);
> +		goto err2;
> +	}
> +
> +	helper_buffer->heap_buffer.dmabuf = dmabuf;
> +	helper_buffer->sg_table = table;
> +
> +	ret = dma_buf_fd(dmabuf, O_CLOEXEC);
> +	if (ret < 0) {
> +		dma_buf_put(dmabuf);
> +		/* just return, as put will call release and that will free */
> +		return ret;
> +	}
> +
> +	return ret;
> +
> +err2:
> +	for_each_sg(table->sgl, sg, i, j)
> +		__free_page(sg_page(sg));
> +	sg_free_table(table);
> +err1:
> +	kfree(table);
> +err0:
> +	kfree(helper_buffer);
> +	return -ENOMEM;
> +}
> +
> +
> +static struct dma_heap_ops system_heap_ops = {
> +	.allocate = system_heap_allocate,
> +};
> +
> +static int system_heap_create(void)
> +{
> +	struct system_heap *sys_heap;
> +
> +	sys_heap = kzalloc(sizeof(*sys_heap), GFP_KERNEL);
> +	if (!sys_heap)
> +		return -ENOMEM;
> +	sys_heap->heap.name = "system_heap";
> +	sys_heap->heap.ops = &system_heap_ops;
> +
> +	dma_heap_add(&sys_heap->heap);
> +
> +	return 0;
> +}
> +device_initcall(system_heap_create);
> -- 
> 2.7.4
> 
> 

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project