| Message ID | 1550734830-23499-1-git-send-email-john.stultz@linaro.org |
|---|---|
| Headers | show |
| Series | dmabuf pools infrastructure (destaging ION) | expand |
On Wed, Feb 20, 2019 at 11:40 PM John Stultz <john.stultz@linaro.org> wrote: > > Here is a very early peek at my dmabuf pools patchset, which > tries to destage a fair chunk of ION functionality. > > This build and boots, but I've not gotten to testing the actual > pool devices yet (need to write some kselftests)! I just wanted > some early feedback on the overall direction. > > The patchset implements per-pool devices (extending my ion > per-heap devices patchset from last week), which can be opened > directly and then an ioctl is used to allocate a dmabuf from the > pool. > > The interface is similar, but simpler then IONs, only providing > an ALLOC ioctl. > > Also, I've only destaged the system/system-contig and cma pools, > since the ION carveout and chunk heaps depended on out of tree > board files to initialize those heaps. I'll leave that to folks > who are actually using those heaps. > > Let me know what you think! I also managed to get this validated under AOSP w/ HiKey960 today. There were some bugs, so I've got updated patches here (on top of HiKey960 kernel changes - even includes the beginnings of a kselftest): https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-pools And the userland changes HiKey960's gralloc (so it can dynamically support legacy ion, modern ion and dmabuf pools) are here: https://android-review.googlesource.com/c/device/linaro/hikey/+/909436 I still need to flesh out the kselftest code some more to actually do some validation and error testing, and do a bunch of cleanups (and I'm sure find a few more bugs), but hopefully this can now be considered viable. thanks -john
On 2/21/19 1:40 AM, John Stultz wrote: > Here is a very early peek at my dmabuf pools patchset, which > tries to destage a fair chunk of ION functionality. > > This build and boots, but I've not gotten to testing the actual > pool devices yet (need to write some kselftests)! I just wanted > some early feedback on the overall direction. > > The patchset implements per-pool devices (extending my ion > per-heap devices patchset from last week), which can be opened > directly and then an ioctl is used to allocate a dmabuf from the > pool. > > The interface is similar, but simpler then IONs, only providing > an ALLOC ioctl. > > Also, I've only destaged the system/system-contig and cma pools, > since the ION carveout and chunk heaps depended on out of tree > board files to initialize those heaps. I'll leave that to folks > who are actually using those heaps. > > Let me know what you think! > +1 Was this source not pulled from -next, I have some fixes in next that I don't see in this code, so I won't review the code itself just yet (it is and early RFC after all). For the concept itself I have a couple small suggestions: I'm not sure I like the name. "Pool" in the context of DMA-BUF feels like it means something else, like some new feature of DMA-BUFs exporters/importers can use for making buffer pools. How about just keep the "heap" terminology to prevent too much re-wording. Maybe just call this dma-buf/heaps/ ? Although the differed free stuff is nice and should be available, I don't think it needs to be part of the first set of de-staged features. It is a bolt-on feature that can be added later, making this first patchset more simple. In the same way I would like to see the changes suggested in one of the other threads implemented. Basically let the heaps(pools?) provide their own struct dma_buf_ops. If this is to be an extension of dma-buf then it shouldn't be making forcing the use of its own dma_buf_ops like ION did. Instead it should just handle the userspace exporting API only. We can always provide helpers for the basic dma_buf_ops for consistency and code-reuse, but the heaps themselves should have full control if/when to use them. It might be easier to show this all by example, I'll put together my own rough RFC over the weekend if that is okay with you (not trying to walk over your work here or anything, just want to show what I'm thinking if any of the above doesn't make sense) :) Thanks, Andrew > thanks > -john > > Cc: Laura Abbott <labbott@redhat.com> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.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 > > John Stultz (4): > dma-buf: Add dma-buf pools framework > dma-buf: pools: Add page-pool for dma-buf pools > dma-buf: pools: Add system/system-contig pools to dmabuf pools > dma-buf: pools: Add CMA pool to dmabuf pools > > MAINTAINERS | 13 + > drivers/dma-buf/Kconfig | 2 + > drivers/dma-buf/Makefile | 1 + > drivers/dma-buf/pools/Kconfig | 25 ++ > drivers/dma-buf/pools/Makefile | 4 + > drivers/dma-buf/pools/cma_pool.c | 143 ++++++++ > drivers/dma-buf/pools/dmabuf-pools.c | 670 +++++++++++++++++++++++++++++++++++ > drivers/dma-buf/pools/dmabuf-pools.h | 295 +++++++++++++++ > drivers/dma-buf/pools/page_pool.c | 157 ++++++++ > drivers/dma-buf/pools/pool-helpers.c | 317 +++++++++++++++++ > drivers/dma-buf/pools/pool-ioctl.c | 94 +++++ > drivers/dma-buf/pools/system_pool.c | 374 +++++++++++++++++++ > include/uapi/linux/dmabuf-pools.h | 59 +++ > 13 files changed, 2154 insertions(+) > create mode 100644 drivers/dma-buf/pools/Kconfig > create mode 100644 drivers/dma-buf/pools/Makefile > create mode 100644 drivers/dma-buf/pools/cma_pool.c > create mode 100644 drivers/dma-buf/pools/dmabuf-pools.c > create mode 100644 drivers/dma-buf/pools/dmabuf-pools.h > create mode 100644 drivers/dma-buf/pools/page_pool.c > create mode 100644 drivers/dma-buf/pools/pool-helpers.c > create mode 100644 drivers/dma-buf/pools/pool-ioctl.c > create mode 100644 drivers/dma-buf/pools/system_pool.c > create mode 100644 include/uapi/linux/dmabuf-pools.h >
On Fri, Feb 22, 2019 at 8:55 AM Andrew F. Davis <afd@ti.com> wrote: > On 2/21/19 1:40 AM, John Stultz wrote: > > Here is a very early peek at my dmabuf pools patchset, which > > tries to destage a fair chunk of ION functionality. > > > > This build and boots, but I've not gotten to testing the actual > > pool devices yet (need to write some kselftests)! I just wanted > > some early feedback on the overall direction. > > > > The patchset implements per-pool devices (extending my ion > > per-heap devices patchset from last week), which can be opened > > directly and then an ioctl is used to allocate a dmabuf from the > > pool. > > > > The interface is similar, but simpler then IONs, only providing > > an ALLOC ioctl. > > > > Also, I've only destaged the system/system-contig and cma pools, > > since the ION carveout and chunk heaps depended on out of tree > > board files to initialize those heaps. I'll leave that to folks > > who are actually using those heaps. > > > > Let me know what you think! > > > > +1 > > Was this source not pulled from -next, I have some fixes in next that I > don't see in this code, so I won't review the code itself just yet (it > is and early RFC after all). For the concept itself I have a couple > small suggestions: Oh, no, I've missed those. I was working off -rc7. I'll try to re-integrate them in. > I'm not sure I like the name. "Pool" in the context of DMA-BUF feels > like it means something else, like some new feature of DMA-BUFs > exporters/importers can use for making buffer pools. How about just keep > the "heap" terminology to prevent too much re-wording. Maybe just call > this dma-buf/heaps/ ? The name changing was mostly as Laura noted that the term heap has caused confusion historically. I'm not really particular, and I do worry about the naming overlap between dmabuf-pools and the pagepool code was problematic. Due to that overlap, renaming things back will be a small chore, but I've got only myself to blame there :) > Although the differed free stuff is nice and should be available, I > don't think it needs to be part of the first set of de-staged features. > It is a bolt-on feature that can be added later, making this first > patchset more simple. Yea, I tried to split that out, but I included it as I didn't really want to do the surgery to the system heaps to remove it right off. > In the same way I would like to see the changes suggested in one of the > other threads implemented. Basically let the heaps(pools?) provide their > own struct dma_buf_ops. If this is to be an extension of dma-buf then it > shouldn't be making forcing the use of its own dma_buf_ops like ION did. Yea. For the most part, the primary goal of my efforts are to just get the userland ABI that folks can agree on established. Letting the heaps have their own dma_buf_ops sounds reasonable, but that's also something that hopefully won't have userspace impact, so can be done at any point. > Instead it should just handle the userspace exporting API only. We can > always provide helpers for the basic dma_buf_ops for consistency and > code-reuse, but the heaps themselves should have full control if/when to > use them. Sounds nice. > It might be easier to show this all by example, I'll put together my own > rough RFC over the weekend if that is okay with you (not trying to walk > over your work here or anything, just want to show what I'm thinking if > any of the above doesn't make sense) :) Please do! I just had a quiet last few days and after seeing your earlier patch figured I should do more then just handwave at it so we can make some progress so we can get it out of staging/changing-abi-limbo. thanks -john
On Fri, Feb 22, 2019 at 9:24 AM John Stultz <john.stultz@linaro.org> wrote: > > On Fri, Feb 22, 2019 at 8:55 AM Andrew F. Davis <afd@ti.com> wrote: > > On 2/21/19 1:40 AM, John Stultz wrote: > > > Here is a very early peek at my dmabuf pools patchset, which > > > tries to destage a fair chunk of ION functionality. > > > > > > This build and boots, but I've not gotten to testing the actual > > > pool devices yet (need to write some kselftests)! I just wanted > > > some early feedback on the overall direction. > > > > > > The patchset implements per-pool devices (extending my ion > > > per-heap devices patchset from last week), which can be opened > > > directly and then an ioctl is used to allocate a dmabuf from the > > > pool. > > > > > > The interface is similar, but simpler then IONs, only providing > > > an ALLOC ioctl. > > > > > > Also, I've only destaged the system/system-contig and cma pools, > > > since the ION carveout and chunk heaps depended on out of tree > > > board files to initialize those heaps. I'll leave that to folks > > > who are actually using those heaps. > > > > > > Let me know what you think! > > > > > > > +1 > > > > Was this source not pulled from -next, I have some fixes in next that I > > don't see in this code, so I won't review the code itself just yet (it > > is and early RFC after all). For the concept itself I have a couple > > small suggestions: > > Oh, no, I've missed those. I was working off -rc7. I'll try to > re-integrate them in. > > > I'm not sure I like the name. "Pool" in the context of DMA-BUF feels > > like it means something else, like some new feature of DMA-BUFs > > exporters/importers can use for making buffer pools. How about just keep > > the "heap" terminology to prevent too much re-wording. Maybe just call > > this dma-buf/heaps/ ? > > The name changing was mostly as Laura noted that the term heap has > caused confusion historically. I'm not really particular, and I do > worry about the naming overlap between dmabuf-pools and the pagepool > code was problematic. Due to that overlap, renaming things back will > be a small chore, but I've got only myself to blame there :) Ok, I've renamed things back to heaps, and updated the patches here (sorry, I didn't rename the git branch :): kernel: https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-pools userland: https://android-review.googlesource.com/c/device/linaro/hikey/+/909436 I'll get to integrating your changes in -next, and see about splitting the page pool/deferred freeing logic out to the end here soon. thanks -john
On 2/22/19 9:24 AM, John Stultz wrote: > On Fri, Feb 22, 2019 at 8:55 AM Andrew F. Davis <afd@ti.com> wrote: >> On 2/21/19 1:40 AM, John Stultz wrote: >>> Here is a very early peek at my dmabuf pools patchset, which >>> tries to destage a fair chunk of ION functionality. >>> >>> This build and boots, but I've not gotten to testing the actual >>> pool devices yet (need to write some kselftests)! I just wanted >>> some early feedback on the overall direction. >>> >>> The patchset implements per-pool devices (extending my ion >>> per-heap devices patchset from last week), which can be opened >>> directly and then an ioctl is used to allocate a dmabuf from the >>> pool. >>> >>> The interface is similar, but simpler then IONs, only providing >>> an ALLOC ioctl. >>> >>> Also, I've only destaged the system/system-contig and cma pools, >>> since the ION carveout and chunk heaps depended on out of tree >>> board files to initialize those heaps. I'll leave that to folks >>> who are actually using those heaps. >>> >>> Let me know what you think! >>> >> >> +1 >> >> Was this source not pulled from -next, I have some fixes in next that I >> don't see in this code, so I won't review the code itself just yet (it >> is and early RFC after all). For the concept itself I have a couple >> small suggestions: > > Oh, no, I've missed those. I was working off -rc7. I'll try to > re-integrate them in. > >> I'm not sure I like the name. "Pool" in the context of DMA-BUF feels >> like it means something else, like some new feature of DMA-BUFs >> exporters/importers can use for making buffer pools. How about just keep >> the "heap" terminology to prevent too much re-wording. Maybe just call >> this dma-buf/heaps/ ? > > The name changing was mostly as Laura noted that the term heap has > caused confusion historically. I'm not really particular, and I do > worry about the naming overlap between dmabuf-pools and the pagepool > code was problematic. Due to that overlap, renaming things back will > be a small chore, but I've got only myself to blame there :) > > Yeah I'm not set on changing the names. If everyone else finds heap to be descriptive enough, we can keep it. Thanks, Laura
On Fri, Feb 22, 2019 at 12:45 PM John Stultz <john.stultz@linaro.org> wrote: > Ok, I've renamed things back to heaps, and updated the patches here > (sorry, I didn't rename the git branch :): > kernel: https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-pools > userland: https://android-review.googlesource.com/c/device/linaro/hikey/+/909436 > > I'll get to integrating your changes in -next, and see about splitting > the page pool/deferred freeing logic out to the end here soon. Ok, I've gone through the -next changes and ported them over. I've also split out the shrinker/deferred freeing and page-pool logic into separate patches at the end (so we can skip them or include them). Though, while I have done some initial validation, I'd trust the page-pool verison of the system heap code that is closer to the more widely tested ION version then my paired down version (which roughly implements the original 3.4 era ION system heap). I've updated my tree here: https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-pools I still need to rework/improve the commit logs, and will probably wait to see what Andrew is working on before I resend it, but I wanted to share in case anyone is wanting to check it out. thanks -john