dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: John Stultz <john.stultz@linaro.org>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
	"Andrew F. Davis" <afd@ti.com>, Laura Abbott <labbott@redhat.com>,
	Benjamin Gaignard <benjamin.gaignard@linaro.org>,
	Sumit Semwal <sumit.semwal@linaro.org>,
	Liam Mark <lmark@codeaurora.org>,
	Brian Starkey <Brian.Starkey@arm.com>,
	Chenbo Feng <fengc@google.com>,
	Alistair Strachan <astrachan@google.com>,
	dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework
Date: Wed, 27 Mar 2019 23:09:39 -0700	[thread overview]
Message-ID: <CALAqxLWmdJ0KnwEsLjNOwZ0DQ=D0H7Smu7KBBCtF+zwbqY+y5g@mail.gmail.com> (raw)
In-Reply-To: <20190327145354.GA4514@kroah.com>

On Wed, Mar 27, 2019 at 11:25 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 05, 2019 at 12:54:29PM -0800, 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!
>
> Comments just on the user/kernel api and how it interacts with the
> driver model.  Not on the "real" functionality of this code :)

Thanks so much for the feedback! In some cases Andrew and I have
already made the changes you've suggested, and hopefully will have a
new version to share soon.


> > +#define DEVNAME "dma_heap"
> > +
> > +#define NUM_HEAP_MINORS 128
>
> Why a max?

Mostly because other drivers do. I'll see if this can be removed with
the Xarray bits.


> > +static DEFINE_IDR(dma_heap_idr);
> > +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
>
> Move to use xarray now so that Matthew doesn't have to send patches
> converting this code later :)
>
> It also allows you to drop the mutex.

Yep. Already converted to Xarray, it is nicer!


> > +dev_t dma_heap_devt;
> > +struct class *dma_heap_class;
> > +struct list_head dma_heap_list;
> > +struct dentry *dma_heap_debug_root;
>
> Global variables?

Oops. Will make those static. Thanks!

> > +
> > +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> > +                              unsigned int flags)
> > +{
> > +     len = PAGE_ALIGN(len);
> > +     if (!len)
> > +             return -EINVAL;
> > +
> > +     return heap->ops->allocate(heap, len, flags);
> > +}
> > +
> > +static int dma_heap_open(struct inode *inode, struct file *filp)
> > +{
> > +     struct dma_heap *heap;
> > +
> > +     mutex_lock(&minor_lock);
> > +     heap = idr_find(&dma_heap_idr, iminor(inode));
> > +     mutex_unlock(&minor_lock);
> > +     if (!heap) {
> > +             pr_err("dma_heap: minor %d unknown.\n", iminor(inode));
> > +             return -ENODEV;
> > +     }
> > +
> > +     /* instance data as context */
> > +     filp->private_data = heap;
> > +     nonseekable_open(inode, filp);
> > +
> > +     return 0;
> > +}
> > +
> > +static int dma_heap_release(struct inode *inode, struct file *filp)
> > +{
> > +     filp->private_data = NULL;
>
> Why does this matter?  release should only be called on the way out of
> here, no need to do anything as nothing else can be called, right?
>
> release shouldn't be needed from what I can tell.

Yep. Christoph suggested the same and its been removed already.


> > +             if (heap_allocation.flags & ~DMA_HEAP_VALID_FLAGS) {
> > +                     pr_warn_once("dma_heap: flags has invalid or unsupported flags set\n");
> > +                     return -EINVAL;
> > +             }
> > +
> > +             fd = dma_heap_buffer_alloc(heap, heap_allocation.len,
> > +                                        heap_allocation.flags);
>
> No max value checking for .len?  Can you really ask for anything?

So I think any length caps would be heap specific, so we want to pass
them on here.

> > +static const struct file_operations dma_heap_fops = {
> > +     .owner          = THIS_MODULE,
> > +     .open           = dma_heap_open,
> > +     .release        = dma_heap_release,
> > +     .unlocked_ioctl = dma_heap_ioctl,
> > +#ifdef CONFIG_COMPAT
> > +     .compat_ioctl   = dma_heap_ioctl,
> > +#endif
>
> Why is compat_ioctl even needed?

Probably my mistake. I didn't realize if we're running 32bit on 64bit
and there's no compat, the unlocked_ioctl gets called.


> > +     /* Find unused minor number */
> > +     mutex_lock(&minor_lock);
> > +     ret = idr_alloc(&dma_heap_idr, heap, 0, NUM_HEAP_MINORS, GFP_KERNEL);
> > +     mutex_unlock(&minor_lock);
>
> Again, xarray.

Ack.

> But I will ask you to back up, why need a major number at all?  Why not
> just use the misc subsystem?  How many of these are you going to have
> over time in a "normal" system?  How about a "abnormal system"?

So early implementations did use misc, but in order to get the
/dev/heap/cma_heap style directories, in both Android and classic udev
linux systems I had to create a class.

This v2 patch didn't get it quite right (got it working properly in
android but not on classic systems), but the next version does get the
subdir created properly (similar to how the input layer does it).

As for number of heaps, I wouldn't expect there to be a ton on any
given system. Most likely less then 16, but possibly up to 32. 128
seemed like a safe "crazy out there" cap. But perspectives on crazy
shift over time :)

> We have seen people running Android in "containers" such that they
> needed binderfs to handle huge numbers of individual android systems
> running at the same time.  Will this api break those systems if you have
> a tiny maximum number you an allocate?

I'd have to think some more on this. Right now I'd expect that you'd
not be trying to virtualize the heaps in a container so you'd not have
m heaps * n containers on the system. Instead the containers would
mount/link in the devnode (I'm a bit fuzzy on how containers handle
devnode creation/restrictions) as appropriate (the nice part with this
over ION is we have per heap dev nodes, so the set shared can be
limited).  But I'd have to think more about the risks of how multiple
containers might share things like cma heaps.


> > +     if (ret < 0) {
> > +             pr_err("dma_heap: Unable to get minor number for heap\n");
> > +             return ret;
> > +     }
> > +     heap->minor = ret;
> > +
> > +     /* Create device */
> > +     heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor);
> > +     dev_ret = device_create(dma_heap_class,
> > +                             NULL,
> > +                             heap->heap_devt,
> > +                             NULL,
> > +                             heap->name);
>
> No parent?  Can't hang this off of anything?  Ok, having it show up in
> /sys/devices/virtual/ is probably good enough.
>
> > +     if (IS_ERR(dev_ret)) {
> > +             pr_err("dma_heap: Unable to create char device\n");
> > +             return PTR_ERR(dev_ret);
> > +     }
> > +
> > +     /* Add device */
> > +     cdev_init(&heap->heap_cdev, &dma_heap_fops);
> > +     ret = cdev_add(&heap->heap_cdev, dma_heap_devt, NUM_HEAP_MINORS);
> > +     if (ret < 0) {
> > +             device_destroy(dma_heap_class, heap->heap_devt);
> > +             pr_err("dma_heap: Unable to add char device\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(dma_heap_add);
>
> EXPORT_SYMBOL_GPL() please?  For core stuff like this it's good.

Actually, removed the export completely for now since its probably not
ready for modules yet. But will be sure to tag it GPL when we do
re-add it.


> > +
> > +static int dma_heap_init(void)
> > +{
> > +     int ret;
> > +
> > +     ret = alloc_chrdev_region(&dma_heap_devt, 0, NUM_HEAP_MINORS, DEVNAME);
> > +     if (ret)
> > +             return ret;
> > +
> > +     dma_heap_class = class_create(THIS_MODULE, DEVNAME);
> > +     if (IS_ERR(dma_heap_class)) {
> > +             unregister_chrdev_region(dma_heap_devt, NUM_HEAP_MINORS);
> > +             return PTR_ERR(dma_heap_class);
> > +     }
> > +
> > +     return 0;
> > +}
> > +subsys_initcall(dma_heap_init);
>
> Overall, looks sane, the comments above are all really minor.

Very much appreciate the review!


>
> > --- /dev/null
> > +++ b/include/uapi/linux/dma-heap.h
> > @@ -0,0 +1,52 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> Wrong license for a uapi .h file :(

Ack. Fixed.

Thanks so much again!
-john

  reply	other threads:[~2019-03-28  6:09 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-05 20:54 [RFC][PATCH 0/5 v2] DMA-BUF Heaps (destaging ION) John Stultz
2019-03-05 20:54 ` [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework John Stultz
2019-03-06 16:12   ` Benjamin Gaignard
2019-03-06 16:57     ` John Stultz
2019-03-15  8:55     ` Christoph Hellwig
2019-03-06 16:27   ` Andrew F. Davis
2019-03-06 19:03     ` John Stultz
2019-03-06 21:45       ` Andrew F. Davis
2019-03-15  8:54   ` Christoph Hellwig
2019-03-15 20:24     ` Andrew F. Davis
2019-03-15 20:18   ` Laura Abbott
2019-03-15 20:49     ` Andrew F. Davis
2019-03-15 21:29     ` John Stultz
2019-03-15 22:44       ` Laura Abbott
2019-03-18  4:38         ` Sumit Semwal
2019-03-18  4:41         ` Sumit Semwal
2019-03-19 12:08   ` Brian Starkey
2019-03-19 15:24     ` Andrew F. Davis
2019-03-21 21:16     ` John Stultz
2019-03-27 14:53   ` Greg KH
2019-03-28  6:09     ` John Stultz [this message]
2019-03-05 20:54 ` [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers John Stultz
2019-03-13 20:18   ` Liam Mark
2019-03-13 21:48     ` Andrew F. Davis
2019-03-13 22:57       ` Liam Mark
2019-03-13 23:42         ` Andrew F. Davis
2019-03-15  9:06   ` Christoph Hellwig
2019-03-19 15:03     ` Andrew F. Davis
2019-03-21 20:01     ` John Stultz
2019-03-19 14:26   ` Brian Starkey
2019-03-21 20:11     ` John Stultz
2019-03-21 20:35     ` Andrew F. Davis
2019-03-21 20:43   ` Andrew F. Davis
2019-03-05 20:54 ` [RFC][PATCH 3/5 v2] dma-buf: heaps: Add system heap to dmabuf heaps John Stultz
2019-03-06 16:01   ` Benjamin Gaignard
2019-03-11  5:48     ` John Stultz
2019-03-13 20:20   ` Liam Mark
2019-03-13 22:49     ` John Stultz
2019-03-15  9:06   ` Christoph Hellwig
2019-03-05 20:54 ` [RFC][PATCH 4/5 v2] dma-buf: heaps: Add CMA heap to dmabuf heapss John Stultz
2019-03-06 16:05   ` Benjamin Gaignard
2019-03-21 20:15     ` John Stultz
2019-03-15  9:06   ` Christoph Hellwig
2019-03-15 20:08     ` John Stultz
2019-03-19 14:53   ` Brian Starkey
2019-03-05 20:54 ` [RFC][PATCH 5/5 v2] kselftests: Add dma-heap test John Stultz
2019-03-06 16:14   ` Benjamin Gaignard
2019-03-06 16:35     ` Andrew F. Davis
2019-03-06 18:19       ` John Stultz
2019-03-06 18:32         ` Andrew F. Davis
2019-03-06 17:01     ` John Stultz
2019-03-15 20:07       ` Laura Abbott
2019-03-15 20:13         ` John Stultz
2019-03-15 20:49           ` Laura Abbott
2019-03-13 20:23   ` Liam Mark
2019-03-13 20:11 ` [RFC][PATCH 0/5 v2] DMA-BUF Heaps (destaging ION) Liam Mark
2019-03-13 22:30   ` John Stultz
2019-03-13 23:29     ` Liam Mark
2019-03-19 16:54     ` Benjamin Gaignard
2019-03-19 16:59       ` Andrew F. Davis
2019-03-19 21:58         ` Rob Clark
2019-03-19 22:36           ` John Stultz
2019-03-20  9:16             ` Benjamin Gaignard
2019-03-20 14:44               ` Andrew F. Davis
2019-03-20 15:59                 ` Benjamin Gaignard
2019-03-20 16:11               ` John Stultz
2019-03-15 20:34 ` Laura Abbott
2019-03-15 23:15 ` Jerome Glisse
2019-03-16  0:16   ` John Stultz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CALAqxLWmdJ0KnwEsLjNOwZ0DQ=D0H7Smu7KBBCtF+zwbqY+y5g@mail.gmail.com' \
    --to=john.stultz@linaro.org \
    --cc=Brian.Starkey@arm.com \
    --cc=afd@ti.com \
    --cc=astrachan@google.com \
    --cc=benjamin.gaignard@linaro.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=fengc@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=labbott@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lmark@codeaurora.org \
    --cc=sumit.semwal@linaro.org \
    --subject='Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).