Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework

[John Stultz <john.stultz@linaro.org>]

On Wed, Mar 6, 2019 at 10:18 AM Andrew F. Davis <afd@ti.com> wrote:
>
> On 3/5/19 2:54 PM, John Stultz wrote:
> > From: "Andrew F. Davis" <afd@ti.com>
> >
> > 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 <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: Andrew F. Davis <afd@ti.com>
> > [jstultz: reworded commit message, and lots of cleanups]
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> > 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 <labbott@redhat.com>
> > +R:   Liam Mark <lmark@codeaurora.org>
> > +R:   Brian Starkey <Brian.Starkey@arm.com>
> > +R:   "Andrew F. Davis" <afd@ti.com>
>
> 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