From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Stultz Subject: Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework Date: Wed, 6 Mar 2019 11:03:19 -0800 Message-ID: References: <1551819273-640-1-git-send-email-john.stultz@linaro.org> <1551819273-640-2-git-send-email-john.stultz@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: "Andrew F. Davis" Cc: lkml , Laura Abbott , Benjamin Gaignard , Greg KH , Sumit Semwal , Liam Mark , Brian Starkey , Chenbo Feng , Alistair Strachan , dri-devel List-Id: dri-devel@lists.freedesktop.org On Wed, Mar 6, 2019 at 10:18 AM Andrew F. Davis wrote: > > On 3/5/19 2:54 PM, John Stultz wrote: > > From: "Andrew F. Davis" > > > > 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 > > Cc: Benjamin Gaignard > > Cc: Greg KH > > Cc: Sumit Semwal > > Cc: Liam Mark > > Cc: Brian Starkey > > Cc: Andrew F. Davis > > Cc: Chenbo Feng > > Cc: Alistair Strachan > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Andrew F. Davis > > [jstultz: reworded commit message, and lots of cleanups] > > Signed-off-by: John Stultz > > --- > > 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 > > +R: Liam Mark > > +R: Brian Starkey > > +R: "Andrew F. Davis" > > Quotes not needed in maintainers file. Whatever you say, "Andrew F. Davis", or whomever you really are! ;) > > + > > + 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. Well, we have to have one u32 for padding. And I figured if we needed anything more then a u32, then we're in for 2 more. And I think the potential of the alignment and heap-private flags, I worry we might want to have something, but I guess we could just add a new ioctl and keep the support for the old one if folks prefer. > > +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. Hrm. I'll have to look. > > +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. :) I guess I'd just move the include/linux/dma-heap.h to drivers/dma-buf/heaps/ and keep it localized there. But whichever. Feel free to also send a patch and I can fold it down. thanks -john