Messages in this thread | | | Subject | Re: [PATCH 13/14] staging: android: ion: Do not sync CPU cache on map/unmap | From | Laura Abbott <> | Date | Fri, 18 Jan 2019 12:31:34 -0800 |
| |
On 1/17/19 8:13 AM, Andrew F. Davis wrote: > On 1/16/19 4:48 PM, Liam Mark wrote: >> On Wed, 16 Jan 2019, Andrew F. Davis wrote: >> >>> On 1/15/19 1:05 PM, Laura Abbott wrote: >>>> On 1/15/19 10:38 AM, Andrew F. Davis wrote: >>>>> On 1/15/19 11:45 AM, Liam Mark wrote: >>>>>> On Tue, 15 Jan 2019, Andrew F. Davis wrote: >>>>>> >>>>>>> On 1/14/19 11:13 AM, Liam Mark wrote: >>>>>>>> On Fri, 11 Jan 2019, Andrew F. Davis wrote: >>>>>>>> >>>>>>>>> Buffers may not be mapped from the CPU so skip cache maintenance >>>>>>>>> here. >>>>>>>>> Accesses from the CPU to a cached heap should be bracketed with >>>>>>>>> {begin,end}_cpu_access calls so maintenance should not be needed >>>>>>>>> anyway. >>>>>>>>> >>>>>>>>> Signed-off-by: Andrew F. Davis <afd@ti.com> >>>>>>>>> --- >>>>>>>>> drivers/staging/android/ion/ion.c | 7 ++++--- >>>>>>>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/staging/android/ion/ion.c >>>>>>>>> b/drivers/staging/android/ion/ion.c >>>>>>>>> index 14e48f6eb734..09cb5a8e2b09 100644 >>>>>>>>> --- a/drivers/staging/android/ion/ion.c >>>>>>>>> +++ b/drivers/staging/android/ion/ion.c >>>>>>>>> @@ -261,8 +261,8 @@ static struct sg_table *ion_map_dma_buf(struct >>>>>>>>> dma_buf_attachment *attachment, >>>>>>>>> table = a->table; >>>>>>>>> - if (!dma_map_sg(attachment->dev, table->sgl, table->nents, >>>>>>>>> - direction)) >>>>>>>>> + if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents, >>>>>>>>> + direction, DMA_ATTR_SKIP_CPU_SYNC)) >>>>>>>> >>>>>>>> Unfortunately I don't think you can do this for a couple reasons. >>>>>>>> You can't rely on {begin,end}_cpu_access calls to do cache >>>>>>>> maintenance. >>>>>>>> If the calls to {begin,end}_cpu_access were made before the call to >>>>>>>> dma_buf_attach then there won't have been a device attached so the >>>>>>>> calls >>>>>>>> to {begin,end}_cpu_access won't have done any cache maintenance. >>>>>>>> >>>>>>> >>>>>>> That should be okay though, if you have no attachments (or all >>>>>>> attachments are IO-coherent) then there is no need for cache >>>>>>> maintenance. Unless you mean a sequence where a non-io-coherent device >>>>>>> is attached later after data has already been written. Does that >>>>>>> sequence need supporting? >>>>>> >>>>>> Yes, but also I think there are cases where CPU access can happen before >>>>>> in Android, but I will focus on later for now. >>>>>> >>>>>>> DMA-BUF doesn't have to allocate the backing >>>>>>> memory until map_dma_buf() time, and that should only happen after all >>>>>>> the devices have attached so it can know where to put the buffer. So we >>>>>>> shouldn't expect any CPU access to buffers before all the devices are >>>>>>> attached and mapped, right? >>>>>>> >>>>>> >>>>>> Here is an example where CPU access can happen later in Android. >>>>>> >>>>>> Camera device records video -> software post processing -> video device >>>>>> (who does compression of raw data) and writes to a file >>>>>> >>>>>> In this example assume the buffer is cached and the devices are not >>>>>> IO-coherent (quite common). >>>>>> >>>>> >>>>> This is the start of the problem, having cached mappings of memory that >>>>> is also being accessed non-coherently is going to cause issues one way >>>>> or another. On top of the speculative cache fills that have to be >>>>> constantly fought back against with CMOs like below; some coherent >>>>> interconnects behave badly when you mix coherent and non-coherent access >>>>> (snoop filters get messed up). >>>>> >>>>> The solution is to either always have the addresses marked non-coherent >>>>> (like device memory, no-map carveouts), or if you really want to use >>>>> regular system memory allocated at runtime, then all cached mappings of >>>>> it need to be dropped, even the kernel logical address (area as painful >>>>> as that would be). >>>>> >>>> >>>> I agree it's broken, hence my desire to remove it :) >>>> >>>> The other problem is that uncached buffers are being used for >>>> performance reason so anything that would involve getting >>>> rid of the logical address would probably negate any performance >>>> benefit. >>>> >>> >>> I wouldn't go as far as to remove them just yet.. Liam seems pretty >>> adamant that they have valid uses. I'm just not sure performance is one >>> of them, maybe in the case of software locks between devices or >>> something where there needs to be a lot of back and forth interleaved >>> access on small amounts of data? >>> >> >> I wasn't aware that ARM considered this not supported, I thought it was >> supported but they advised against it because of the potential performance >> impact. >> > > Not sure what you mean by "this" being not supported, do you mean mixed > attribute mappings? If so, it will certainly cause problems, and the > problems will change from platform to platform, avoid at all costs is my > understanding of ARM's position. > >> This is after all supported in the DMA APIs and up until now devices have >> been successfully commercializing with this configurations, and I think >> they will continue to commercialize with these configurations for quite a >> while. >> > > Use of uncached memory mappings are almost always wrong in my experience > and are used to work around some bug or because the user doesn't want to > implement proper CMOs. Counter examples welcome. > >> It would be really unfortunate if support was removed as I think that >> would drive clients away from using upstream ION. >> > > I'm not petitioning to remove support, but at very least lets reverse > the ION_FLAG_CACHED flag. Ion should hand out cached normal memory by > default, to get uncached you should need to add a flag to your > allocation command pointing out you know what you are doing. >
I thought about doing that, the problem is it becomes an ABI break for existing users which I really didn't want to do again. If it ends up being the last thing we do before moving out of staging, I'd consider doing it.
>>>>>> ION buffer is allocated. >>>>>> >>>>>> //Camera device records video >>>>>> dma_buf_attach >>>>>> dma_map_attachment (buffer needs to be cleaned) >>>>> >>>>> Why does the buffer need to be cleaned here? I just got through reading >>>>> the thread linked by Laura in the other reply. I do like +Brian's >>>>> suggestion of tracking if the buffer has had CPU access since the last >>>>> time and only flushing the cache if it has. As unmapped heaps never get >>>>> CPU mapped this would never be the case for unmapped heaps, it solves my >>>>> problem. >>>>> >>>>>> [camera device writes to buffer] >>>>>> dma_buf_unmap_attachment (buffer needs to be invalidated) >>>>> >>>>> It doesn't know there will be any further CPU access, it could get freed >>>>> after this for all we know, the invalidate can be saved until the CPU >>>>> requests access again. >>>>> >>>>>> dma_buf_detach (device cannot stay attached because it is being sent >>>>>> down >>>>>> the pipeline and Camera doesn't know the end of the use case) >>>>>> >>>>> >>>>> This seems like a broken use-case, I understand the desire to keep >>>>> everything as modular as possible and separate the steps, but at this >>>>> point no one owns this buffers backing memory, not the CPU or any >>>>> device. I would go as far as to say DMA-BUF should be free now to >>>>> de-allocate the backing storage if it wants, that way it could get ready >>>>> for the next attachment, which may change the required backing memory >>>>> completely. >>>>> >>>>> All devices should attach before the first mapping, and only let go >>>>> after the task is complete, otherwise this buffers data needs copied off >>>>> to a different location or the CPU needs to take ownership in-between. >>>>> >>>> >>>> Maybe it's broken but it's the status quo and we spent a good >>>> amount of time at plumbers concluding there isn't a great way >>>> to fix it :/ >>>> >>> >>> Hmm, guess that doesn't prove there is not a great way to fix it either.. :/ >>> >>> Perhaps just stronger rules on sequencing of operations? I'm not saying >>> I have a good solution either, I just don't see any way forward without >>> some use-case getting broken, so better to fix now over later. >>> >> >> I can see the benefits of Android doing things the way they do, I would >> request that changes we make continue to support Android, or we find a way >> to convice them to change, as they are the main ION client and I assume >> other ION clients in the future will want to do this as well. >> > > Android may be the biggest user today (makes sense, Ion come out of the > Android project), but that can change, and getting changes into Android > will be easier that the upstream kernel once Ion is out of staging. > > Unlike some other big ARM vendors, we (TI) do not primarily build mobile > chips targeting Android, our core offerings target more traditional > Linux userspaces, and I'm guessing others will start to do the same as > ARM tries to push more into desktop, server, and other spaces again. > >> I am concerned that if you go with a solution which enforces what you >> mention above, and bring ION out of staging that way, it will make it that >> much harder to solve this for Android and therefore harder to get >> Android clients to move to the upstream ION (and get everybody off their >> vendor modified Android versions). >> > > That would be an Android problem, reducing functionality in upstream to > match what some evil vendor trees do to support Android is not the way > forward on this. At least for us we are going to try to make all our > software offerings follow proper buffer ownership (including our Android > offering). >
I don't think this is reducing functionality, it's about not breaking what already works. There is some level of Android testing on a mainline tree (hikey boards). I would say if we can come to an agreement on a correct API, we could always merge the 'correct' version out of staging and keep a legacy driver around for some time as a transition.
Thanks, Laura
| |