mbox series

[EARLY,RFC,0/4] dmabuf pools infrastructure (destaging ION)

Message ID 1550734830-23499-1-git-send-email-john.stultz@linaro.org (mailing list archive)
Headers show
Series dmabuf pools infrastructure (destaging ION) | expand

Message

John Stultz Feb. 21, 2019, 7:40 a.m. UTC
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!

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

Comments

John Stultz Feb. 22, 2019, 7:19 a.m. UTC | #1
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
Andrew Davis Feb. 22, 2019, 4:55 p.m. UTC | #2
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
>
John Stultz Feb. 22, 2019, 5:24 p.m. UTC | #3
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
John Stultz Feb. 22, 2019, 8:45 p.m. UTC | #4
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
Laura Abbott Feb. 22, 2019, 10:30 p.m. UTC | #5
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
John Stultz Feb. 23, 2019, 6:21 a.m. UTC | #6
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