lkml.org 
[lkml]   [2019]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework
    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

    \
     
     \ /
      Last update: 2019-03-06 20:04    [W:3.067 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site