| Message ID | 20190724003656.59780-4-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DMA-BUF Heaps (destaging ION) | expand |
On 7/25/19 9:02 AM, Christoph Hellwig wrote: >> +struct system_heap { >> + struct dma_heap *heap; >> +} sys_heap; > > It seems like this structure could be removed and if would improve > the code flow. > >> +static struct dma_heap_ops system_heap_ops = { >> + .allocate = system_heap_allocate, >> +}; >> + >> +static int system_heap_create(void) >> +{ >> + struct dma_heap_export_info exp_info; >> + int ret = 0; >> + >> + exp_info.name = "system_heap"; >> + exp_info.ops = &system_heap_ops; >> + exp_info.priv = &sys_heap; >> + >> + sys_heap.heap = dma_heap_add(&exp_info); >> + if (IS_ERR(sys_heap.heap)) >> + ret = PTR_ERR(sys_heap.heap); >> + >> + return ret; > > The data structures here seem a little odd. I think you want to: > > - mark all dma_heap_ops instanes consts, as we generally do that for > all structures containing function pointers > - move the name into dma_heap_ops. > - remove the dma_heap_export_info structure, which is a bit pointless The idea here is to keep the struct dma_heap as an opaque pointer for everyone but the core framework. No one should be touching the guts of that struct (would be 'private' if we were using C++ but this is the best we can do with C), exposing it to the exporters or the importers will break this isolation. This export style also matches DMA-BUF: you pass in a filled out _export struct and it passes back a dma_buf handle. DMA-BUF unfortunately made the internals of that struct public so they are widely used directly (and abused in some cases) and that prevents the internals from being easily refactored when needed. Andrew > - don't bother setting a private data, as you don't need it. > If other heaps need private data I'd suggest to switch to embedding > the dma_heap structure into containing structure insted so that you > can use container_of to get at it. > - also why is the free callback passed as a callback rather than > kept in dma_heap_ops, next to the paired alloc one? >
On Thu, Jul 25, 2019 at 6:02 AM Christoph Hellwig <hch@infradead.org> wrote: > > > +struct system_heap { > > + struct dma_heap *heap; > > +} sys_heap; > > It seems like this structure could be removed and if would improve > the code flow. Good point. We actually keep a few things in the cma version of this, and I think I copied that over when I started here, but never cleaned it up. > > +static struct dma_heap_ops system_heap_ops = { > > + .allocate = system_heap_allocate, > > +}; > > + > > +static int system_heap_create(void) > > +{ > > + struct dma_heap_export_info exp_info; > > + int ret = 0; > > + > > + exp_info.name = "system_heap"; > > + exp_info.ops = &system_heap_ops; > > + exp_info.priv = &sys_heap; > > + > > + sys_heap.heap = dma_heap_add(&exp_info); > > + if (IS_ERR(sys_heap.heap)) > > + ret = PTR_ERR(sys_heap.heap); > > + > > + return ret; > > The data structures here seem a little odd. I think you want to: Yea. There is some awkwardness, and some is due to using the helper infrastructure, but some is just clutter and I'll revise that. > - mark all dma_heap_ops instanes consts, as we generally do that for > all structures containing function pointers Done. > - move the name into dma_heap_ops. I'm not sure this is useful, as there are cases where there are multiple heaps that use the same ops. Specifically the multiple CMA heaps. > - remove the dma_heap_export_info structure, which is a bit pointless Andrew and I went back and forth on this a bit. It looks like he just responded so I'll defer to his answer. > - don't bother setting a private data, as you don't need it. > If other heaps need private data I'd suggest to switch to embedding > the dma_heap structure into containing structure insted so that you > can use container_of to get at it. Fair. There is some cases where we use the priv data, but I'll try to see if I can minimize it. And again, I think having the dma_heap structure be internal/private to the heap implementations made it difficult to be a contained structure. So it goes back to the export_info structure point above. > - also why is the free callback passed as a callback rather than > kept in dma_heap_ops, next to the paired alloc one? This one is due to the optional heap helpers infrastructure. If a heap implements its own dma_buf_ops, it can have release directly call the buffer free function. However, since we tried to minimize the code we have the heap helpers infrastructure which implements a shared dma_buf_op, we need some way for the helper release function to call back to the heap specific free. We could put it in the dma_heaps_ops like you suggest, but that brings some confusion as well, as nothing in the dma-heaps core would call it, it would only be a tool for the helper infrastructure to trace back to the heap specific free call. This is why its passed to the heap_helper initializer. I agree it feels a little odd, so I'd welcome alternate approaches. Very much appreciate the review and feedback! I'll try to address as much of this as I can in the next revision. thanks -john
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 162e24e1e429..657ce743abda 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -48,4 +48,6 @@ menuconfig DMABUF_HEAPS allows userspace to use 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 000000000000..205052744169 --- /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 de49898112db..d1808eca2581 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 000000000000..5e3eb1fb3bcd --- /dev/null +++ b/drivers/dma-buf/heaps/system_heap.c @@ -0,0 +1,123 @@ +// 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; +} sys_heap; + +static void system_heap_free(struct heap_helper_buffer *buffer) +{ + pgoff_t pg; + + for (pg = 0; pg < buffer->pagecount; pg++) + __free_page(buffer->pages[pg]); + kfree(buffer->pages); + kfree(buffer); +} + +static int system_heap_allocate(struct dma_heap *heap, + unsigned long len, + unsigned long fd_flags, + unsigned long heap_flags) +{ + struct heap_helper_buffer *helper_buffer; + struct dma_buf *dmabuf; + int ret = -ENOMEM; + pgoff_t pg; + + 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 = heap_flags; + helper_buffer->heap_buffer.heap = heap; + helper_buffer->heap_buffer.size = len; + + helper_buffer->pagecount = len / PAGE_SIZE; + helper_buffer->pages = kmalloc_array(helper_buffer->pagecount, + sizeof(*helper_buffer->pages), + GFP_KERNEL); + if (!helper_buffer->pages) { + ret = -ENOMEM; + goto err0; + } + + for (pg = 0; pg < helper_buffer->pagecount; pg++) { + /* + * Avoid trying to allocate memory if the process + * has been killed by by SIGKILL + */ + if (fatal_signal_pending(current)) + goto err1; + + helper_buffer->pages[pg] = alloc_page(GFP_KERNEL | __GFP_ZERO); + if (!helper_buffer->pages[pg]) + goto err1; + } + + /* create the dmabuf */ + dmabuf = heap_helper_export_dmabuf(helper_buffer, fd_flags); + if (IS_ERR(dmabuf)) { + ret = PTR_ERR(dmabuf); + goto err1; + } + + helper_buffer->heap_buffer.dmabuf = dmabuf; + + 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; + +err1: + while (pg > 0) + __free_page(helper_buffer->pages[--pg]); + kfree(helper_buffer->pages); +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 dma_heap_export_info exp_info; + int ret = 0; + + exp_info.name = "system_heap"; + exp_info.ops = &system_heap_ops; + exp_info.priv = &sys_heap; + + sys_heap.heap = dma_heap_add(&exp_info); + if (IS_ERR(sys_heap.heap)) + ret = PTR_ERR(sys_heap.heap); + + return ret; +} +device_initcall(system_heap_create);