2019-05-13 19:50:47

by John Stultz

[permalink] [raw]
Subject: [RFC][PATCH 4/5 v4] dma-buf: heaps: Add CMA heap to dmabuf heaps

This adds a CMA heap, which allows userspace to allocate
a dma-buf of contiguous memory out of a CMA region.

This code is an evolution of the Android ION implementation, so
thanks to its original author and maintainters:
Benjamin Gaignard, Laura Abbott, and others!

Cc: Laura Abbott <[email protected]>
Cc: Benjamin Gaignard <[email protected]>
Cc: Sumit Semwal <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Pratik Patel <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Vincent Donnefort <[email protected]>
Cc: Sudipto Paul <[email protected]>
Cc: Andrew F. Davis <[email protected]>
Cc: Xu YiPing <[email protected]>
Cc: "Chenfeng (puck)" <[email protected]>
Cc: butao <[email protected]>
Cc: "Xiaqing (A)" <[email protected]>
Cc: Yudongbin <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Chenbo Feng <[email protected]>
Cc: Alistair Strachan <[email protected]>
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
v2:
* Switch allocate to return dmabuf fd
* Simplify init code
* Checkpatch fixups
v3:
* Switch to inline function for to_cma_heap()
* Minor cleanups suggested by Brian
* Fold in new registration style from Andrew
* Folded in changes from Andrew to use simplified page list
from the heap helpers
* Use the fd_flags when creating dmabuf fd (Suggested by
Benjamin)
* Use precalculated pagecount (Suggested by Andrew)
---
drivers/dma-buf/heaps/Kconfig | 8 ++
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/cma_heap.c | 169 +++++++++++++++++++++++++++++++
3 files changed, 178 insertions(+)
create mode 100644 drivers/dma-buf/heaps/cma_heap.c

diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index 205052744169..a5eef06c4226 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -4,3 +4,11 @@ config DMABUF_HEAPS_SYSTEM
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.
+
+config DMABUF_HEAPS_CMA
+ bool "DMA-BUF CMA Heap"
+ depends on DMABUF_HEAPS && DMA_CMA
+ help
+ Choose this option to enable dma-buf CMA heap. This heap is backed
+ by the Contiguous Memory Allocator (CMA). If your system has these
+ regions, you should say Y here.
diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
index d1808eca2581..6e54cdec3da0 100644
--- a/drivers/dma-buf/heaps/Makefile
+++ b/drivers/dma-buf/heaps/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
obj-y += heap-helpers.o
obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
+obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
new file mode 100644
index 000000000000..3d0ffbbd0a34
--- /dev/null
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DMABUF CMA heap exporter
+ *
+ * Copyright (C) 2012, 2019 Linaro Ltd.
+ * Author: <[email protected]> for ST-Ericsson.
+ */
+
+#include <linux/device.h>
+#include <linux/dma-buf.h>
+#include <linux/dma-heap.h>
+#include <linux/slab.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/cma.h>
+#include <linux/scatterlist.h>
+#include <linux/highmem.h>
+
+#include "heap-helpers.h"
+
+struct cma_heap {
+ struct dma_heap *heap;
+ struct cma *cma;
+};
+
+static void cma_heap_free(struct heap_helper_buffer *buffer)
+{
+ struct cma_heap *cma_heap = dma_heap_get_data(buffer->heap_buffer.heap);
+ unsigned long nr_pages = buffer->pagecount;
+ struct page *pages = buffer->priv_virt;
+
+ /* free page list */
+ kfree(buffer->pages);
+ /* release memory */
+ cma_release(cma_heap->cma, pages, nr_pages);
+ kfree(buffer);
+}
+
+/* dmabuf heap CMA operations functions */
+static int cma_heap_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags)
+{
+ struct cma_heap *cma_heap = dma_heap_get_data(heap);
+ struct heap_helper_buffer *helper_buffer;
+ struct page *pages;
+ size_t size = PAGE_ALIGN(len);
+ unsigned long nr_pages = size >> PAGE_SHIFT;
+ unsigned long align = get_order(size);
+ DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+ struct dma_buf *dmabuf;
+ int ret = -ENOMEM;
+ pgoff_t pg;
+
+ if (align > CONFIG_CMA_ALIGNMENT)
+ align = CONFIG_CMA_ALIGNMENT;
+
+ helper_buffer = kzalloc(sizeof(*helper_buffer), GFP_KERNEL);
+ if (!helper_buffer)
+ return -ENOMEM;
+
+ INIT_HEAP_HELPER_BUFFER(helper_buffer, cma_heap_free);
+ helper_buffer->heap_buffer.flags = heap_flags;
+ helper_buffer->heap_buffer.heap = heap;
+ helper_buffer->heap_buffer.size = len;
+
+ pages = cma_alloc(cma_heap->cma, nr_pages, align, false);
+ if (!pages)
+ goto free_buf;
+
+ if (PageHighMem(pages)) {
+ unsigned long nr_clear_pages = nr_pages;
+ struct page *page = pages;
+
+ while (nr_clear_pages > 0) {
+ void *vaddr = kmap_atomic(page);
+
+ memset(vaddr, 0, PAGE_SIZE);
+ kunmap_atomic(vaddr);
+ page++;
+ nr_clear_pages--;
+ }
+ } else {
+ memset(page_address(pages), 0, size);
+ }
+
+ helper_buffer->pagecount = nr_pages;
+ helper_buffer->pages = kmalloc_array(helper_buffer->pagecount,
+ sizeof(*helper_buffer->pages),
+ GFP_KERNEL);
+ if (!helper_buffer->pages) {
+ ret = -ENOMEM;
+ goto free_cma;
+ }
+
+ for (pg = 0; pg < helper_buffer->pagecount; pg++) {
+ helper_buffer->pages[pg] = &pages[pg];
+ if (!helper_buffer->pages[pg])
+ goto free_pages;
+ }
+
+ /* create the dmabuf */
+ exp_info.ops = &heap_helper_ops;
+ exp_info.size = len;
+ exp_info.flags = fd_flags;
+ exp_info.priv = &helper_buffer->heap_buffer;
+ dmabuf = dma_buf_export(&exp_info);
+ if (IS_ERR(dmabuf)) {
+ ret = PTR_ERR(dmabuf);
+ goto free_pages;
+ }
+
+ helper_buffer->heap_buffer.dmabuf = dmabuf;
+ helper_buffer->priv_virt = pages;
+
+ ret = dma_buf_fd(dmabuf, fd_flags);
+ if (ret < 0) {
+ dma_buf_put(dmabuf);
+ /* just return, as put will call release and that will free */
+ return ret;
+ }
+
+ return ret;
+
+free_pages:
+ kfree(helper_buffer->pages);
+free_cma:
+ cma_release(cma_heap->cma, pages, nr_pages);
+free_buf:
+ kfree(helper_buffer);
+ return ret;
+}
+
+static struct dma_heap_ops cma_heap_ops = {
+ .allocate = cma_heap_allocate,
+};
+
+static int __add_cma_heap(struct cma *cma, void *data)
+{
+ struct cma_heap *cma_heap;
+ struct dma_heap_export_info exp_info;
+
+ cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
+ if (!cma_heap)
+ return -ENOMEM;
+ cma_heap->cma = cma;
+
+ exp_info.name = cma_get_name(cma);
+ exp_info.ops = &cma_heap_ops;
+ exp_info.priv = cma_heap;
+
+ cma_heap->heap = dma_heap_add(&exp_info);
+ if (IS_ERR(cma_heap->heap)) {
+ int ret = PTR_ERR(cma_heap->heap);
+
+ kfree(cma_heap);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int add_cma_heaps(void)
+{
+ cma_for_each_area(__add_cma_heap, NULL);
+ return 0;
+}
+device_initcall(add_cma_heaps);
--
2.17.1


2019-05-14 10:42:29

by Xiaqing (A)

[permalink] [raw]
Subject: Re: [RFC][PATCH 4/5 v4] dma-buf: heaps: Add CMA heap to dmabuf heaps



On 2019/5/14 2:37, John Stultz wrote:
> This adds a CMA heap, which allows userspace to allocate
> a dma-buf of contiguous memory out of a CMA region.
>
> This code is an evolution of the Android ION implementation, so
> thanks to its original author and maintainters:
> Benjamin Gaignard, Laura Abbott, and others!
>
> Cc: Laura Abbott <[email protected]>
> Cc: Benjamin Gaignard <[email protected]>
> Cc: Sumit Semwal <[email protected]>
> Cc: Liam Mark <[email protected]>
> Cc: Pratik Patel <[email protected]>
> Cc: Brian Starkey <[email protected]>
> Cc: Vincent Donnefort <[email protected]>
> Cc: Sudipto Paul <[email protected]>
> Cc: Andrew F. Davis <[email protected]>
> Cc: Xu YiPing <[email protected]>
> Cc: "Chenfeng (puck)" <[email protected]>
> Cc: butao <[email protected]>
> Cc: "Xiaqing (A)" <[email protected]>
> Cc: Yudongbin <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Chenbo Feng <[email protected]>
> Cc: Alistair Strachan <[email protected]>
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> v2:
> * Switch allocate to return dmabuf fd
> * Simplify init code
> * Checkpatch fixups
> v3:
> * Switch to inline function for to_cma_heap()
> * Minor cleanups suggested by Brian
> * Fold in new registration style from Andrew
> * Folded in changes from Andrew to use simplified page list
> from the heap helpers
> * Use the fd_flags when creating dmabuf fd (Suggested by
> Benjamin)
> * Use precalculated pagecount (Suggested by Andrew)
> ---
> drivers/dma-buf/heaps/Kconfig | 8 ++
> drivers/dma-buf/heaps/Makefile | 1 +
> drivers/dma-buf/heaps/cma_heap.c | 169 +++++++++++++++++++++++++++++++
> 3 files changed, 178 insertions(+)
> create mode 100644 drivers/dma-buf/heaps/cma_heap.c
>
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index 205052744169..a5eef06c4226 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -4,3 +4,11 @@ config DMABUF_HEAPS_SYSTEM
> 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.
> +
> +config DMABUF_HEAPS_CMA
> + bool "DMA-BUF CMA Heap"
> + depends on DMABUF_HEAPS && DMA_CMA
> + help
> + Choose this option to enable dma-buf CMA heap. This heap is backed
> + by the Contiguous Memory Allocator (CMA). If your system has these
> + regions, you should say Y here.
> diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> index d1808eca2581..6e54cdec3da0 100644
> --- a/drivers/dma-buf/heaps/Makefile
> +++ b/drivers/dma-buf/heaps/Makefile
> @@ -1,3 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-y += heap-helpers.o
> obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
> +obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> new file mode 100644
> index 000000000000..3d0ffbbd0a34
> --- /dev/null
> +++ b/drivers/dma-buf/heaps/cma_heap.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DMABUF CMA heap exporter
> + *
> + * Copyright (C) 2012, 2019 Linaro Ltd.
> + * Author: <[email protected]> for ST-Ericsson.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/dma-buf.h>
> +#include <linux/dma-heap.h>
> +#include <linux/slab.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/cma.h>
> +#include <linux/scatterlist.h>
> +#include <linux/highmem.h>
> +
> +#include "heap-helpers.h"
> +
> +struct cma_heap {
> + struct dma_heap *heap;
> + struct cma *cma;
> +};
> +
> +static void cma_heap_free(struct heap_helper_buffer *buffer)
> +{
> + struct cma_heap *cma_heap = dma_heap_get_data(buffer->heap_buffer.heap);
> + unsigned long nr_pages = buffer->pagecount;
> + struct page *pages = buffer->priv_virt;
> +
> + /* free page list */
> + kfree(buffer->pages);
> + /* release memory */
> + cma_release(cma_heap->cma, pages, nr_pages);
> + kfree(buffer);
> +}
> +
> +/* dmabuf heap CMA operations functions */
> +static int cma_heap_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags)
> +{
> + struct cma_heap *cma_heap = dma_heap_get_data(heap);
> + struct heap_helper_buffer *helper_buffer;
> + struct page *pages;
> + size_t size = PAGE_ALIGN(len);
> + unsigned long nr_pages = size >> PAGE_SHIFT;
> + unsigned long align = get_order(size);
> + DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> + struct dma_buf *dmabuf;
> + int ret = -ENOMEM;
> + pgoff_t pg;
> +
> + if (align > CONFIG_CMA_ALIGNMENT)
> + align = CONFIG_CMA_ALIGNMENT;
> +
> + helper_buffer = kzalloc(sizeof(*helper_buffer), GFP_KERNEL);
> + if (!helper_buffer)
> + return -ENOMEM;
> +
> + INIT_HEAP_HELPER_BUFFER(helper_buffer, cma_heap_free);
> + helper_buffer->heap_buffer.flags = heap_flags;
> + helper_buffer->heap_buffer.heap = heap;
> + helper_buffer->heap_buffer.size = len;
> +
> + pages = cma_alloc(cma_heap->cma, nr_pages, align, false);
> + if (!pages)
> + goto free_buf;
> +
> + if (PageHighMem(pages)) {
> + unsigned long nr_clear_pages = nr_pages;
> + struct page *page = pages;
> +
> + while (nr_clear_pages > 0) {
> + void *vaddr = kmap_atomic(page);
> +
> + memset(vaddr, 0, PAGE_SIZE);
> + kunmap_atomic(vaddr);
> + page++;
> + nr_clear_pages--;
> + }
> + } else {
> + memset(page_address(pages), 0, size);
> + }
> +
> + helper_buffer->pagecount = nr_pages;
> + helper_buffer->pages = kmalloc_array(helper_buffer->pagecount,
> + sizeof(*helper_buffer->pages),
> + GFP_KERNEL);
> + if (!helper_buffer->pages) {
> + ret = -ENOMEM;
> + goto free_cma;
> + }
> +
> + for (pg = 0; pg < helper_buffer->pagecount; pg++) {
> + helper_buffer->pages[pg] = &pages[pg];
> + if (!helper_buffer->pages[pg])
> + goto free_pages;
> + }
> +
> + /* create the dmabuf */
> + exp_info.ops = &heap_helper_ops;
> + exp_info.size = len;
> + exp_info.flags = fd_flags;
> + exp_info.priv = &helper_buffer->heap_buffer;
> + dmabuf = dma_buf_export(&exp_info);
> + if (IS_ERR(dmabuf)) {
> + ret = PTR_ERR(dmabuf);
> + goto free_pages;
> + }
Can the dmabuf be created in the framework layer?
each heap needs to add the same code here, which is not very good.

> +
> + helper_buffer->heap_buffer.dmabuf = dmabuf;
> + helper_buffer->priv_virt = pages;
> +
> + ret = dma_buf_fd(dmabuf, fd_flags);
> + if (ret < 0) {
> + dma_buf_put(dmabuf);
> + /* just return, as put will call release and that will free */
> + return ret;
> + }
> +
> + return ret;
> +
> +free_pages:
> + kfree(helper_buffer->pages);
> +free_cma:
> + cma_release(cma_heap->cma, pages, nr_pages);
> +free_buf:
> + kfree(helper_buffer);
> + return ret;
> +}
> +
> +static struct dma_heap_ops cma_heap_ops = {
> + .allocate = cma_heap_allocate,
> +};
> +
> +static int __add_cma_heap(struct cma *cma, void *data)
> +{
> + struct cma_heap *cma_heap;
> + struct dma_heap_export_info exp_info;
> +
> + cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
> + if (!cma_heap)
> + return -ENOMEM;
> + cma_heap->cma = cma;
> +
> + exp_info.name = cma_get_name(cma);
> + exp_info.ops = &cma_heap_ops;
> + exp_info.priv = cma_heap;
> +
> + cma_heap->heap = dma_heap_add(&exp_info);
> + if (IS_ERR(cma_heap->heap)) {
> + int ret = PTR_ERR(cma_heap->heap);
> +
> + kfree(cma_heap);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int add_cma_heaps(void)
> +{
> + cma_for_each_area(__add_cma_heap, NULL);
> + return 0;
> +}
> +device_initcall(add_cma_heaps);
>

2019-05-14 11:57:22

by Benjamin Gaignard

[permalink] [raw]
Subject: Re: [RFC][PATCH 4/5 v4] dma-buf: heaps: Add CMA heap to dmabuf heaps

Le mar. 14 mai 2019 à 12:40, Xiaqing (A) <[email protected]> a écrit :
>
>
>
> On 2019/5/14 2:37, John Stultz wrote:
> > This adds a CMA heap, which allows userspace to allocate
> > a dma-buf of contiguous memory out of a CMA region.
> >
> > This code is an evolution of the Android ION implementation, so
> > thanks to its original author and maintainters:
> > Benjamin Gaignard, Laura Abbott, and others!
> >
> > Cc: Laura Abbott <[email protected]>
> > Cc: Benjamin Gaignard <[email protected]>
> > Cc: Sumit Semwal <[email protected]>
> > Cc: Liam Mark <[email protected]>
> > Cc: Pratik Patel <[email protected]>
> > Cc: Brian Starkey <[email protected]>
> > Cc: Vincent Donnefort <[email protected]>
> > Cc: Sudipto Paul <[email protected]>
> > Cc: Andrew F. Davis <[email protected]>
> > Cc: Xu YiPing <[email protected]>
> > Cc: "Chenfeng (puck)" <[email protected]>
> > Cc: butao <[email protected]>
> > Cc: "Xiaqing (A)" <[email protected]>
> > Cc: Yudongbin <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Chenbo Feng <[email protected]>
> > Cc: Alistair Strachan <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > v2:
> > * Switch allocate to return dmabuf fd
> > * Simplify init code
> > * Checkpatch fixups
> > v3:
> > * Switch to inline function for to_cma_heap()
> > * Minor cleanups suggested by Brian
> > * Fold in new registration style from Andrew
> > * Folded in changes from Andrew to use simplified page list
> > from the heap helpers
> > * Use the fd_flags when creating dmabuf fd (Suggested by
> > Benjamin)
> > * Use precalculated pagecount (Suggested by Andrew)
> > ---
> > drivers/dma-buf/heaps/Kconfig | 8 ++
> > drivers/dma-buf/heaps/Makefile | 1 +
> > drivers/dma-buf/heaps/cma_heap.c | 169 +++++++++++++++++++++++++++++++
> > 3 files changed, 178 insertions(+)
> > create mode 100644 drivers/dma-buf/heaps/cma_heap.c
> >
> > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> > index 205052744169..a5eef06c4226 100644
> > --- a/drivers/dma-buf/heaps/Kconfig
> > +++ b/drivers/dma-buf/heaps/Kconfig
> > @@ -4,3 +4,11 @@ config DMABUF_HEAPS_SYSTEM
> > 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.
> > +
> > +config DMABUF_HEAPS_CMA
> > + bool "DMA-BUF CMA Heap"
> > + depends on DMABUF_HEAPS && DMA_CMA
> > + help
> > + Choose this option to enable dma-buf CMA heap. This heap is backed
> > + by the Contiguous Memory Allocator (CMA). If your system has these
> > + regions, you should say Y here.
> > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> > index d1808eca2581..6e54cdec3da0 100644
> > --- a/drivers/dma-buf/heaps/Makefile
> > +++ b/drivers/dma-buf/heaps/Makefile
> > @@ -1,3 +1,4 @@
> > # SPDX-License-Identifier: GPL-2.0
> > obj-y += heap-helpers.o
> > obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
> > +obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
> > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> > new file mode 100644
> > index 000000000000..3d0ffbbd0a34
> > --- /dev/null
> > +++ b/drivers/dma-buf/heaps/cma_heap.c
> > @@ -0,0 +1,169 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DMABUF CMA heap exporter
> > + *
> > + * Copyright (C) 2012, 2019 Linaro Ltd.
> > + * Author: <[email protected]> for ST-Ericsson.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/dma-buf.h>
> > +#include <linux/dma-heap.h>
> > +#include <linux/slab.h>
> > +#include <linux/errno.h>
> > +#include <linux/err.h>
> > +#include <linux/cma.h>
> > +#include <linux/scatterlist.h>
> > +#include <linux/highmem.h>
> > +
> > +#include "heap-helpers.h"
> > +
> > +struct cma_heap {
> > + struct dma_heap *heap;
> > + struct cma *cma;
> > +};
> > +
> > +static void cma_heap_free(struct heap_helper_buffer *buffer)
> > +{
> > + struct cma_heap *cma_heap = dma_heap_get_data(buffer->heap_buffer.heap);
> > + unsigned long nr_pages = buffer->pagecount;
> > + struct page *pages = buffer->priv_virt;
> > +
> > + /* free page list */
> > + kfree(buffer->pages);
> > + /* release memory */
> > + cma_release(cma_heap->cma, pages, nr_pages);
> > + kfree(buffer);
> > +}
> > +
> > +/* dmabuf heap CMA operations functions */
> > +static int cma_heap_allocate(struct dma_heap *heap,
> > + unsigned long len,
> > + unsigned long fd_flags,
> > + unsigned long heap_flags)
> > +{
> > + struct cma_heap *cma_heap = dma_heap_get_data(heap);
> > + struct heap_helper_buffer *helper_buffer;
> > + struct page *pages;
> > + size_t size = PAGE_ALIGN(len);
> > + unsigned long nr_pages = size >> PAGE_SHIFT;
> > + unsigned long align = get_order(size);
> > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > + struct dma_buf *dmabuf;
> > + int ret = -ENOMEM;
> > + pgoff_t pg;
> > +
> > + if (align > CONFIG_CMA_ALIGNMENT)
> > + align = CONFIG_CMA_ALIGNMENT;
> > +
> > + helper_buffer = kzalloc(sizeof(*helper_buffer), GFP_KERNEL);
> > + if (!helper_buffer)
> > + return -ENOMEM;
> > +
> > + INIT_HEAP_HELPER_BUFFER(helper_buffer, cma_heap_free);
> > + helper_buffer->heap_buffer.flags = heap_flags;
> > + helper_buffer->heap_buffer.heap = heap;
> > + helper_buffer->heap_buffer.size = len;
> > +
> > + pages = cma_alloc(cma_heap->cma, nr_pages, align, false);
> > + if (!pages)
> > + goto free_buf;
> > +
> > + if (PageHighMem(pages)) {
> > + unsigned long nr_clear_pages = nr_pages;
> > + struct page *page = pages;
> > +
> > + while (nr_clear_pages > 0) {
> > + void *vaddr = kmap_atomic(page);
> > +
> > + memset(vaddr, 0, PAGE_SIZE);
> > + kunmap_atomic(vaddr);
> > + page++;
> > + nr_clear_pages--;
> > + }
> > + } else {
> > + memset(page_address(pages), 0, size);
> > + }
> > +
> > + helper_buffer->pagecount = nr_pages;
> > + helper_buffer->pages = kmalloc_array(helper_buffer->pagecount,
> > + sizeof(*helper_buffer->pages),
> > + GFP_KERNEL);
> > + if (!helper_buffer->pages) {
> > + ret = -ENOMEM;
> > + goto free_cma;
> > + }
> > +
> > + for (pg = 0; pg < helper_buffer->pagecount; pg++) {
> > + helper_buffer->pages[pg] = &pages[pg];
> > + if (!helper_buffer->pages[pg])
> > + goto free_pages;
> > + }
> > +
> > + /* create the dmabuf */
> > + exp_info.ops = &heap_helper_ops;
> > + exp_info.size = len;
> > + exp_info.flags = fd_flags;
> > + exp_info.priv = &helper_buffer->heap_buffer;
> > + dmabuf = dma_buf_export(&exp_info);
> > + if (IS_ERR(dmabuf)) {
> > + ret = PTR_ERR(dmabuf);
> > + goto free_pages;
> > + }
> Can the dmabuf be created in the framework layer?
> each heap needs to add the same code here, which is not very good.

No that would make errors cases more difficult to handle because
you will need to call a new heap's function to clean everything.

>
> > +
> > + helper_buffer->heap_buffer.dmabuf = dmabuf;
> > + helper_buffer->priv_virt = pages;
> > +
> > + ret = dma_buf_fd(dmabuf, fd_flags);
> > + if (ret < 0) {
> > + dma_buf_put(dmabuf);
> > + /* just return, as put will call release and that will free */
> > + return ret;
> > + }
> > +
> > + return ret;
> > +
> > +free_pages:
> > + kfree(helper_buffer->pages);
> > +free_cma:
> > + cma_release(cma_heap->cma, pages, nr_pages);
> > +free_buf:
> > + kfree(helper_buffer);
> > + return ret;
> > +}
> > +
> > +static struct dma_heap_ops cma_heap_ops = {
> > + .allocate = cma_heap_allocate,
> > +};
> > +
> > +static int __add_cma_heap(struct cma *cma, void *data)
> > +{
> > + struct cma_heap *cma_heap;
> > + struct dma_heap_export_info exp_info;
> > +
> > + cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
> > + if (!cma_heap)
> > + return -ENOMEM;
> > + cma_heap->cma = cma;
> > +
> > + exp_info.name = cma_get_name(cma);
> > + exp_info.ops = &cma_heap_ops;
> > + exp_info.priv = cma_heap;
> > +
> > + cma_heap->heap = dma_heap_add(&exp_info);
> > + if (IS_ERR(cma_heap->heap)) {
> > + int ret = PTR_ERR(cma_heap->heap);
> > +
> > + kfree(cma_heap);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int add_cma_heaps(void)
> > +{
> > + cma_for_each_area(__add_cma_heap, NULL);
> > + return 0;
> > +}
> > +device_initcall(add_cma_heaps);
> >
>

2019-05-17 00:24:16

by John Stultz

[permalink] [raw]
Subject: Re: [RFC][PATCH 4/5 v4] dma-buf: heaps: Add CMA heap to dmabuf heaps

On Tue, May 14, 2019 at 3:40 AM Xiaqing (A) <[email protected]> wrote:
>
>
>
> On 2019/5/14 2:37, John Stultz wrote:
> > This adds a CMA heap, which allows userspace to allocate
> > a dma-buf of contiguous memory out of a CMA region.
> >
> > This code is an evolution of the Android ION implementation, so
> > thanks to its original author and maintainters:
> > Benjamin Gaignard, Laura Abbott, and others!
> >
> > Cc: Laura Abbott <[email protected]>
> > Cc: Benjamin Gaignard <[email protected]>
> > Cc: Sumit Semwal <[email protected]>
> > Cc: Liam Mark <[email protected]>
> > Cc: Pratik Patel <[email protected]>
> > Cc: Brian Starkey <[email protected]>
> > Cc: Vincent Donnefort <[email protected]>
> > Cc: Sudipto Paul <[email protected]>
> > Cc: Andrew F. Davis <[email protected]>
> > Cc: Xu YiPing <[email protected]>
> > Cc: "Chenfeng (puck)" <[email protected]>
> > Cc: butao <[email protected]>
> > Cc: "Xiaqing (A)" <[email protected]>
> > Cc: Yudongbin <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Chenbo Feng <[email protected]>
> > Cc: Alistair Strachan <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > v2:
> > * Switch allocate to return dmabuf fd
> > * Simplify init code
> > * Checkpatch fixups
> > v3:
> > * Switch to inline function for to_cma_heap()
> > * Minor cleanups suggested by Brian
> > * Fold in new registration style from Andrew
> > * Folded in changes from Andrew to use simplified page list
> > from the heap helpers
> > * Use the fd_flags when creating dmabuf fd (Suggested by
> > Benjamin)
> > * Use precalculated pagecount (Suggested by Andrew)
> > ---
> > drivers/dma-buf/heaps/Kconfig | 8 ++
> > drivers/dma-buf/heaps/Makefile | 1 +
> > drivers/dma-buf/heaps/cma_heap.c | 169 +++++++++++++++++++++++++++++++
> > 3 files changed, 178 insertions(+)
> > create mode 100644 drivers/dma-buf/heaps/cma_heap.c
> >
> > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> > index 205052744169..a5eef06c4226 100644
> > --- a/drivers/dma-buf/heaps/Kconfig
> > +++ b/drivers/dma-buf/heaps/Kconfig
> > @@ -4,3 +4,11 @@ config DMABUF_HEAPS_SYSTEM
> > 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.
> > +
> > +config DMABUF_HEAPS_CMA
> > + bool "DMA-BUF CMA Heap"
> > + depends on DMABUF_HEAPS && DMA_CMA
> > + help
> > + Choose this option to enable dma-buf CMA heap. This heap is backed
> > + by the Contiguous Memory Allocator (CMA). If your system has these
> > + regions, you should say Y here.
> > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
> > index d1808eca2581..6e54cdec3da0 100644
> > --- a/drivers/dma-buf/heaps/Makefile
> > +++ b/drivers/dma-buf/heaps/Makefile
> > @@ -1,3 +1,4 @@
> > # SPDX-License-Identifier: GPL-2.0
> > obj-y += heap-helpers.o
> > obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
> > +obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
> > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> > new file mode 100644
> > index 000000000000..3d0ffbbd0a34
> > --- /dev/null
> > +++ b/drivers/dma-buf/heaps/cma_heap.c
> > @@ -0,0 +1,169 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DMABUF CMA heap exporter
> > + *
> > + * Copyright (C) 2012, 2019 Linaro Ltd.
> > + * Author: <[email protected]> for ST-Ericsson.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/dma-buf.h>
> > +#include <linux/dma-heap.h>
> > +#include <linux/slab.h>
> > +#include <linux/errno.h>
> > +#include <linux/err.h>
> > +#include <linux/cma.h>
> > +#include <linux/scatterlist.h>
> > +#include <linux/highmem.h>
> > +
> > +#include "heap-helpers.h"
> > +
> > +struct cma_heap {
> > + struct dma_heap *heap;
> > + struct cma *cma;
> > +};
> > +
> > +static void cma_heap_free(struct heap_helper_buffer *buffer)
> > +{
> > + struct cma_heap *cma_heap = dma_heap_get_data(buffer->heap_buffer.heap);
> > + unsigned long nr_pages = buffer->pagecount;
> > + struct page *pages = buffer->priv_virt;
> > +
> > + /* free page list */
> > + kfree(buffer->pages);
> > + /* release memory */
> > + cma_release(cma_heap->cma, pages, nr_pages);
> > + kfree(buffer);
> > +}
> > +
> > +/* dmabuf heap CMA operations functions */
> > +static int cma_heap_allocate(struct dma_heap *heap,
> > + unsigned long len,
> > + unsigned long fd_flags,
> > + unsigned long heap_flags)
> > +{
> > + struct cma_heap *cma_heap = dma_heap_get_data(heap);
> > + struct heap_helper_buffer *helper_buffer;
> > + struct page *pages;
> > + size_t size = PAGE_ALIGN(len);
> > + unsigned long nr_pages = size >> PAGE_SHIFT;
> > + unsigned long align = get_order(size);
> > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> > + struct dma_buf *dmabuf;
> > + int ret = -ENOMEM;
> > + pgoff_t pg;
> > +
> > + if (align > CONFIG_CMA_ALIGNMENT)
> > + align = CONFIG_CMA_ALIGNMENT;
> > +
> > + helper_buffer = kzalloc(sizeof(*helper_buffer), GFP_KERNEL);
> > + if (!helper_buffer)
> > + return -ENOMEM;
> > +
> > + INIT_HEAP_HELPER_BUFFER(helper_buffer, cma_heap_free);
> > + helper_buffer->heap_buffer.flags = heap_flags;
> > + helper_buffer->heap_buffer.heap = heap;
> > + helper_buffer->heap_buffer.size = len;
> > +
> > + pages = cma_alloc(cma_heap->cma, nr_pages, align, false);
> > + if (!pages)
> > + goto free_buf;
> > +
> > + if (PageHighMem(pages)) {
> > + unsigned long nr_clear_pages = nr_pages;
> > + struct page *page = pages;
> > +
> > + while (nr_clear_pages > 0) {
> > + void *vaddr = kmap_atomic(page);
> > +
> > + memset(vaddr, 0, PAGE_SIZE);
> > + kunmap_atomic(vaddr);
> > + page++;
> > + nr_clear_pages--;
> > + }
> > + } else {
> > + memset(page_address(pages), 0, size);
> > + }
> > +
> > + helper_buffer->pagecount = nr_pages;
> > + helper_buffer->pages = kmalloc_array(helper_buffer->pagecount,
> > + sizeof(*helper_buffer->pages),
> > + GFP_KERNEL);
> > + if (!helper_buffer->pages) {
> > + ret = -ENOMEM;
> > + goto free_cma;
> > + }
> > +
> > + for (pg = 0; pg < helper_buffer->pagecount; pg++) {
> > + helper_buffer->pages[pg] = &pages[pg];
> > + if (!helper_buffer->pages[pg])
> > + goto free_pages;
> > + }
> > +
> > + /* create the dmabuf */
> > + exp_info.ops = &heap_helper_ops;
> > + exp_info.size = len;
> > + exp_info.flags = fd_flags;
> > + exp_info.priv = &helper_buffer->heap_buffer;
> > + dmabuf = dma_buf_export(&exp_info);
> > + if (IS_ERR(dmabuf)) {
> > + ret = PTR_ERR(dmabuf);
> > + goto free_pages;
> > + }
> Can the dmabuf be created in the framework layer?
> each heap needs to add the same code here, which is not very good.

Benjamin's point is true, that it might not be best to try to handle
it in the framework layer, but there is a fair amount of boilerplate
that I'll see if I can refactor into some helper functions.

thanks for the feedback!
-john