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

[John Stultz]

On Fri, Feb 22, 2019 at 8:55 AM Andrew F. Davis <afd at 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