| Message ID | 1551819273-640-4-git-send-email-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | DMA-BUF Heaps (destaging ION) | expand |
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
On Wed, Mar 13, 2019 at 1:20 PM Liam Mark <lmark@codeaurora.org> wrote: > On Tue, 5 Mar 2019, John Stultz wrote: > > + > > + page = alloc_page(GFP_KERNEL); > > Need to zero the allocation (add __GFP_ZERO) Ah! Thanks! Fixed now. > > + 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. I think this is a good suggestion, but I do want to be careful to try to make sure we add debugging tools to the larger dmabuf infrastructure, rather then just the heap code (though having some heap specific usage info would still be good). I think there's a separate patchset to dmabufs originally from Greg Hackmann that provides names that is supposed to help with what your suggesting. https://lists.freedesktop.org/archives/dri-devel/2019-February/208759.html thanks! -john
> + 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); > + if (!page) > + goto err2; > + sg_set_page(sg, page, PAGE_SIZE, 0); > + } Given that one if not the primary intent here is to DMA map the memory this is a bad idea, as it might require bounce buffering. What we really want here is something like this: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-noncoherent-allocator.3 And I wonder if the S/G building should also move into common code instead of being duplicated everywhere. > +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); Why is this dynamically allocated?
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); + if (!page) + goto err2; + sg_set_page(sg, page, PAGE_SIZE, 0); + } + + /* 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);
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 -- 2.7.4