Re: [PATCH 00/14] Misc ION cleanups and adding unmapped heap

From: Laura Abbott
Date: Fri Jan 18 2019 - 15:19:45 EST

On 1/16/19 8:05 AM, Andrew F. Davis wrote:
On 1/15/19 12:58 PM, Laura Abbott wrote:
On 1/15/19 9:47 AM, Andrew F. Davis wrote:
On 1/14/19 8:39 PM, Laura Abbott wrote:
On 1/11/19 10:05 AM, Andrew F. Davis wrote:
Hello all,

This is a set of (hopefully) non-controversial cleanups for the ION
framework and current set of heaps. These were found as I start to
familiarize myself with the framework to help in whatever way I
can in getting all this up to the standards needed for de-staging.

I would like to get some ideas of what is left to work on to get ION
out of staging. Has there been some kind of agreement on what ION
should
eventually end up being? To me it looks like it is being whittled
away at
to it's most core functions. To me that is looking like being a DMA-BUF
user-space front end, simply advertising available memory backings in a
system and providing allocations as DMA-BUF handles. If this is the
case
then it looks close to being ready to me at least, but I would love to
hear any other opinions and concerns.


Yes, at this point the only functionality that people are really
depending on is the ability to allocate a dma_buf easily from userspace.

Back to this patchset, the last patch may be a bit different than the
others, it adds an unmapped heaps type and creation helper. I wanted to
get this in to show off another heap type and maybe some issues we may
have with the current ION framework. The unmapped heap is used when the
backing memory should not (or cannot) be touched. Currently this kind
of heap is used for firewalled secure memory that can be allocated like
normal heap memory but only used by secure devices (OP-TEE, crypto HW,
etc). It is basically just copied from the "carveout" heap type with
the
only difference being it is not mappable to userspace and we do not
clear
the memory (as we should not map it either). So should this really be a
new heap type? Or maybe advertised as a carveout heap but with an
additional allocation flag? Perhaps we do away with "types" altogether
and just have flags, coherent/non-coherent, mapped/unmapped, etc.

Maybe more thinking will be needed afterall..


So the cleanup looks okay (I need to finish reviewing) but I'm not a
fan of adding another heaptype without solving the problem of adding
some sort of devicetree binding or other method of allocating and
placing Ion heaps. That plus uncached buffers are one of the big
open problems that need to be solved for destaging Ion. See
https://lore.kernel.org/lkml/20181120164636.jcw7li2uaa3cmwc3@xxxxxxxxxxxxxxxxxxxxxxxxxxx/


for some background on that problem.


I'm under the impression that adding heaps like carveouts/chunk will be
rather system specific and so do not lend themselves well to a universal
DT style exporter. For instance a carveout memory space can be reported
by a device at runtime, then the driver managing that device should go
and use the carveout heap helpers to export that heap. If this is the
case then I'm not sure it is a problem for the ION core framework to
solve, but rather the users of it to figure out how best to create the
various heaps. All Ion needs to do is allow exporting and advertising
them IMHO.


I think it is a problem for the Ion core framework to take care of.
Ion is useless if you don't actually have the heaps. Nobody has
actually gotten a full Ion solution end-to-end with a carveout heap
working in mainline because any proposals have been rejected. I think
we need at least one example in mainline of how creating a carveout
heap would work.

In our evil vendor trees we have several examples. The issue being that
Ion is still staging and attempts for generic DT heap definitions
haven't seemed to go so well. So for now we just keep it specific to our
platforms until upstream makes a direction decision.


Yeah, it's been a bit of a chicken and egg in that this has been
blocking Ion getting out of staging but we don't actually have
in-tree users because it's still in staging.


Thanks for the background thread link, I've been looking for some info
on current status of all this and "ion" is a bit hard to search the
lists for. The core reason for posting this cleanup series is to throw
my hat into the ring of all this Ion work and start getting familiar
with the pending issues. The last two patches are not all that important
to get in right now.

In that thread you linked above, it seems we may have arrived at a
similar problem for different reasons. I think the root issue is the Ion
core makes too many assumptions about the heap memory. My proposal would
be to allow the heap exporters more control over the DMA-BUF ops, maybe
even going as far as letting them provide their own complete struct
dma_buf_ops.

Let me give an example where I think this is going to be useful. We have
the classic constraint solving problem on our SoCs. Our SoCs are full of
various coherent and non-coherent devices, some require contiguous
memory allocations, others have in-line IOMMUs so can operate on
non-contiguous, etc..

DMA-BUF has a solution designed in for this we can use, namely
allocation at map time after all the attachments have come in. The
checking of each attached device to find the right backing memory is
something the DMA-BUF exporter has to do, and so this SoC specific logic
would have to be added to each exporting framework (DRM, V4L2, etc),
unless we have one unified system exporter everyone uses, Ion.


That's how dmabuf is supposed to work in theory but in practice we
also have the case of userspace allocates memory, mmaps, and then
a device attaches to it. The issue is we end up having to do work
and make decisions before all devices are actually attached.


That just seems wrong, DMA-BUF should be used for, well, DMA-able
buffers.. Userspace should not be using these buffers without devices
attached, otherwise why not use a regular buffer. If you need to fill
the buffer then you should attach/map it first so the DMA-BUF exporter
can pick the appropriate backing memory first.

Maybe a couple more rules on the ordering of DMA-BUF operations are
needed to prevent having to deal with all these non-useful permutations.

Sumit? ^^


I'd love to just say "don't do that" but it's existing userspace
behavior and it's really hard to change that.

Then each system can define one (maybe typeless) heap, the correct
backing type is system specific anyway, so let the system specific
backing logic in the unified system exporter heap handle picking that.
To allow that heaps need direct control of dma_buf_ops.

Direct heap control of dma_buf_ops also fixes the cache/non-cache issue,
and my unmapped memory issue, each heap type handles the quirks of its
backing storage in its own way, instead of trying to find some one size
fits all memory operations like we are doing now.


I don't think this is an issue of one-size fits all. We have flags
to differentiate between cached and uncached paths, the issue is
that doing the synchronization for uncached buffers is difficult.


It is difficult, hence why letting an uncached heap exporter do all the
heavy work, instead of trying to deal with all these cases in the Ion
core framework.

I'm just not sure how an extra set of dma_buf ops actually solves
the problem of needing to synchronize alias mappings.


It doesn't solve it, it just moves the work out of the framework. There
are going to be a lot more interesting problems than this with some
types heaps we will have in the future, dealing with all the logic in
the framework core is not going to scale.


That is a good point. My immediate concern though is getting Ion out
of staging. If the per heap dma_buf ops will help with that I'd
certainly like to see them.

Thanks,
Laura

Thanks,
Andrew

Thanks,
Laura

We can provide helpers for the simple heap types still, but with this
much of the heavy lifting moves out of the Ion core framework making it
much more simple, something I think it will need for de-staging.

Anyway, I might be completely off base in my direction here, just let me
know :)