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

[Laura Abbott <labbott@redhat.com>]

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@DESKTOP-E1NTVVP.localdomain/
>>>>
>>>>
>>>> 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 :)
>>>
>>