Re: [RFC][PATCH 0/5 v2] DMA-BUF Heaps (destaging ION)

[John Stultz]

On Wed, Mar 13, 2019 at 1:11 PM Liam Mark <lmark@xxxxxxxxxxxxxx> wrote:
> On Tue, 5 Mar 2019, John Stultz wrote:
> >
> > Eventual TODOS:
> > * Reimplement page-pool for system heap (working on this)
> > * Add stats accounting to system/cma heaps
> > * Make the kselftest actually useful
> > * Add other heaps folks see as useful (would love to get
> >   some help from actual carveout/chunk users)!
>
> We use a modified carveout heap for certain secure use cases.

Cool! It would be great to see if you have any concerns about adding
such a secure-carveout heap to this framework. I suspect it would be
fairly similar to how its integrated into ION, but particularly I'd be
interested in issues around the lack of private flags and other
allocation arguments like alignment.

> Although there would probably be some benefit in discssing how the dma-buf
> heap framework may want to support
> secure heaps in the future it is a large topic which I assume you don't
> want to tackle now.

So I suspect others (Benjamin?) would have a more informed opinion on
the details, but the intent is to allow secure heap implementations.
I'm not sure what areas of concern you have for this allocation
framework in particular?

> We don't have any non-secure carveout heap use cases but the client use
> case I have seen usually revolve around
> wanting large allocations to succeed very quickly.
> For example I have seen camera use cases which do very large allocations
> on camera bootup from the carveout heap, these allocations would come from
> the carveout heap and fallback to the system heap when the carveout heap
> was full.
> Actual non-secure carveout heap can perhaps provide more detail.

Yea, I'm aware that folks still see carveout as preferable to CMA due
to more consistent/predictable allocation latency.  I think we still
have the issue that we don't have bindings to establish/configure
carveout regions w/ dts, and I'm not really wanting to hold up the
allocation API on that issue.


> Since we are making some fundamental changes to how ION worked and since
> Android is likely also be the largest user of the dma-buf heaps framework
> I think it would be good
> to have a path to resolve the issues which are currently preventing
> commercial Android releases from moving to the upstream version of ION.

Yea, I do see solving the cache management efficiency issues as
critical for the dmabuf heaps to be actually usable (my previous
version of this patchset accidentally had my hacks to improve
performance rolled in!).  And there are discussions going on in
various channels to try to figure out how to either change Android to
use dma-bufs more in line with how upstream expects, or what more
generic dma-buf changes we may need to allow Android to use dmabufs
with the expected performance they need.

> I can understand if you don't necessarily want to put all/any of these
> changes into the dma-buf heaps framework as part of this series, but my
> hope is we can get
> the upstream community and the Android framework team to agree on what
> upstreamable changes to dma-buf heaps framework, and/or the Android
> framework, would be required in order for Android to move to the upstream
> dma-buf heaps framework for commercial devices.

Yes. Though I also don't want to get the bigger dma-buf usage
discussion (which really affects all dmabuf exporters) too tied up
with this patch sets attempt to provide a usable allocation interface.
Part of the problem that I think we've seen with ION is that there is
a nest of of related issues, and the entire thing is just too big to
address at once, which I think is part of why ION has sat in staging
for so long. This patchset just tries to provide an dmabuf allocation
interface, and a few example exporter heap types.

> I don't mean to make this specific to Android, but my assumption is that
> many of the ION/dma-buf heaps issues which affect Android would likely
> affect other new large users of the dma-buf heaps framework, so if we
> resolve it for Android we would be helping these future users as well.
> And I do understand that some the issues facing Android may need to be
> resolved by making changes to Android framework.

While true, I also think some of the assumptions in how the dma-bufs
are used (pre-attachment of all devices, etc) are maybe not so
realistic given how Android is using them.  I do want to explore if
Android can change how they use dma-bufs, but I also worry that we
need to think about how we could loosen the expectations for dma-bufs,
as well as trying to figure out how to support things folks have
brought up like partial cache maintenance.

> I think it would be helpful to try and get as much of this agreed upon as
> possible before the dma-buf heaps framework moves out of staging.
>
> As part of my review I will highlight some of the issues which would
> affect Android.
> In my comments I will apply them to the system heap since that is what
> Android currently uses for a lot of its use cases.
> I realize that this new framework provides more flexibility to heaps, so
> perhaps some of these issues can be solved by creating a new type of
> system heap which Android can use, but even if the solution involves
> creating a new system heap I would like to make sure that this "new"
> system heap is upstreamable.

So yea, I do realize I'm dodging the hard problem here, but I think
the cache-management/usage issue is far more generic.

You're right that this implementation give a lot of flexibility to the
exporter heaps in how they implement the dmabuf ops (just like how
other device drivers that are dmabuf exporters have the same
flexibility), but I very much agree we don't want to add a system and
then later a "system-android" heap. So yea, a reasonable amount of
caution is warranted here.

Thanks so much for the review and feedback! I'll try to address things
as I can as I'm traveling this week (so I may be a bit spotty).

thanks
-john