From: John Stultz <john.stultz@linaro.org> To: Brian Starkey <Brian.Starkey@arm.com> Cc: lkml <linux-kernel@vger.kernel.org>, "Andrew F. Davis" <afd@ti.com>, Laura Abbott <labbott@redhat.com>, Benjamin Gaignard <benjamin.gaignard@linaro.org>, Greg KH <gregkh@linuxfoundation.org>, Sumit Semwal <sumit.semwal@linaro.org>, Liam Mark <lmark@codeaurora.org>, Chenbo Feng <fengc@google.com>, Alistair Strachan <astrachan@google.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, nd <nd@arm.com> Subject: Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework Date: Thu, 21 Mar 2019 14:16:34 -0700 [thread overview] Message-ID: <CALAqxLUoXF-M1bGtDPLPg5qJ++e9gpjpnXrJ1_vHg1uLhWQDmA@mail.gmail.com> (raw) In-Reply-To: <20190319120825.3mvdxp5saluboy7o@DESKTOP-E1NTVVP.localdomain> On Tue, Mar 19, 2019 at 5:08 AM Brian Starkey <Brian.Starkey@arm.com> wrote: > > Hi John, > > On Tue, Mar 05, 2019 at 12:54:29PM -0800, John Stultz wrote: > > From: "Andrew F. Davis" <afd@ti.com> > > [snip] > > > + > > +#define NUM_HEAP_MINORS 128 > > +static DEFINE_IDR(dma_heap_idr); > > +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */ > > I saw that Matthew Wilcox is trying to nuke idr: > https://patchwork.freedesktop.org/series/57073/ > > Perhaps a different data structure could be considered? (I don't have > an informed opinion on which). Thanks for pointing this out! I've just switched to using the Xarray implementation in my tree. > > +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, > > + unsigned int flags) > > +{ > > + len = PAGE_ALIGN(len); > > + if (!len) > > + return -EINVAL; > > I think aligning len to pages only makes sense if heaps are going to > allocate aligned to pages too. Perhaps that's an implicit assumption? > If so, lets document it. I've added a comment as such (or do you have more thoughts on where it should be documented?), and for consistency removed the PAGE_ALIGN usage in the heap allocator hooks. > Why not let the heaps take care of aligning len however they want > though? As Andrew already said, It seems page granularity would have to be the finest allocation granularity for dmabufs. If heaps want to implement their own larger granularity alignment, I don't see any reason they would be limited there. And for me, its mostly because I stubbed my toe implementing the heap code w/ the first patch that didn't have the page alignment in the generic code. :) > > + /* 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); > > + 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); > > Shouldn't this be s/dma_heap_devt/heap->heap_devt/ and a count of 1? > > Also would it be better to have cdev_add/device_create the other way > around? First create the char device, then once it's all set up > register it with sysfs. Thanks for catching that! Much appreciated! Reworked as suggested. Though I realized last week I have not figured out a consistent way to have the heaps show up in /dev/dma_heaps/<device> on both Android and classic Linux environments. I need to go stare at the /dev/input/ setup code some more. > > + 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); > > Until we've figured out how modules are going to work, I still think > it would be a good idea to not export this. Done! thanks -john
WARNING: multiple messages have this Message-ID (diff)
From: John Stultz <john.stultz@linaro.org> To: Brian Starkey <Brian.Starkey@arm.com> Cc: nd <nd@arm.com>, Alistair Strachan <astrachan@google.com>, Greg KH <gregkh@linuxfoundation.org>, Chenbo Feng <fengc@google.com>, lkml <linux-kernel@vger.kernel.org>, Liam Mark <lmark@codeaurora.org>, "Andrew F. Davis" <afd@ti.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org> Subject: Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework Date: Thu, 21 Mar 2019 14:16:34 -0700 [thread overview] Message-ID: <CALAqxLUoXF-M1bGtDPLPg5qJ++e9gpjpnXrJ1_vHg1uLhWQDmA@mail.gmail.com> (raw) In-Reply-To: <20190319120825.3mvdxp5saluboy7o@DESKTOP-E1NTVVP.localdomain> On Tue, Mar 19, 2019 at 5:08 AM Brian Starkey <Brian.Starkey@arm.com> wrote: > > Hi John, > > On Tue, Mar 05, 2019 at 12:54:29PM -0800, John Stultz wrote: > > From: "Andrew F. Davis" <afd@ti.com> > > [snip] > > > + > > +#define NUM_HEAP_MINORS 128 > > +static DEFINE_IDR(dma_heap_idr); > > +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */ > > I saw that Matthew Wilcox is trying to nuke idr: > https://patchwork.freedesktop.org/series/57073/ > > Perhaps a different data structure could be considered? (I don't have > an informed opinion on which). Thanks for pointing this out! I've just switched to using the Xarray implementation in my tree. > > +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, > > + unsigned int flags) > > +{ > > + len = PAGE_ALIGN(len); > > + if (!len) > > + return -EINVAL; > > I think aligning len to pages only makes sense if heaps are going to > allocate aligned to pages too. Perhaps that's an implicit assumption? > If so, lets document it. I've added a comment as such (or do you have more thoughts on where it should be documented?), and for consistency removed the PAGE_ALIGN usage in the heap allocator hooks. > Why not let the heaps take care of aligning len however they want > though? As Andrew already said, It seems page granularity would have to be the finest allocation granularity for dmabufs. If heaps want to implement their own larger granularity alignment, I don't see any reason they would be limited there. And for me, its mostly because I stubbed my toe implementing the heap code w/ the first patch that didn't have the page alignment in the generic code. :) > > + /* 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); > > + 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); > > Shouldn't this be s/dma_heap_devt/heap->heap_devt/ and a count of 1? > > Also would it be better to have cdev_add/device_create the other way > around? First create the char device, then once it's all set up > register it with sysfs. Thanks for catching that! Much appreciated! Reworked as suggested. Though I realized last week I have not figured out a consistent way to have the heaps show up in /dev/dma_heaps/<device> on both Android and classic Linux environments. I need to go stare at the /dev/input/ setup code some more. > > + 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); > > Until we've figured out how modules are going to work, I still think > it would be a good idea to not export this. Done! thanks -john _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-03-21 21:16 UTC|newest] Thread overview: 98+ 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 ` John Stultz 2019-03-05 20:54 ` [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework John Stultz 2019-03-05 20:54 ` 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 16:27 ` Andrew F. Davis 2019-03-06 19:03 ` John Stultz 2019-03-06 21:45 ` Andrew F. Davis 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:24 ` Andrew F. Davis 2019-03-15 20:18 ` Laura Abbott 2019-03-15 20:49 ` Andrew F. Davis 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-18 4:41 ` Sumit Semwal 2019-03-19 12:08 ` Brian Starkey 2019-03-19 15:24 ` Andrew F. Davis 2019-03-19 15:24 ` Andrew F. Davis 2019-03-21 21:16 ` John Stultz [this message] 2019-03-21 21:16 ` John Stultz 2019-03-27 14:53 ` Greg KH 2019-03-27 14:53 ` Greg KH 2019-03-28 6:09 ` John Stultz 2019-03-05 20:54 ` [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers John Stultz 2019-03-05 20:54 ` John Stultz 2019-03-13 20:18 ` Liam Mark 2019-03-13 20:18 ` Liam Mark 2019-03-13 21:48 ` Andrew F. Davis 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-13 23:42 ` Andrew F. Davis 2019-03-15 9:06 ` Christoph Hellwig 2019-03-19 15:03 ` Andrew F. Davis 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:11 ` John Stultz 2019-03-21 20:35 ` Andrew F. Davis 2019-03-21 20:43 ` 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-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-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-05 20:54 ` John Stultz 2019-03-06 16:14 ` Benjamin Gaignard 2019-03-06 16:35 ` Andrew F. Davis 2019-03-06 16:35 ` Andrew F. Davis 2019-03-06 18:19 ` John Stultz 2019-03-06 18:19 ` John Stultz 2019-03-06 18:32 ` Andrew F. Davis 2019-03-06 18:32 ` Andrew F. Davis 2019-03-06 17:01 ` John Stultz 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-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 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 16:59 ` Andrew F. Davis 2019-03-19 21:58 ` Rob Clark 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 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=CALAqxLUoXF-M1bGtDPLPg5qJ++e9gpjpnXrJ1_vHg1uLhWQDmA@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=nd@arm.com \ --cc=sumit.semwal@linaro.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.