RE: [Linaro-mm-sig] [RFC][PATCH 1/3] staging: ion: Move away from the DMA APIs for cache flushing
Hi Laura, It seems that outer cache is needed to be handled after __cpuc_flush_dcache_area() for ARM platform. Please correct me if I am wrong. Thanks Xiaoquan -Original Message- From: Linaro-mm-sig [mailto:linaro-mm-sig-boun...@lists.linaro.org] On Behalf Of Laura Abbott Sent: 2016年5月26日 3:48 To: Sumit Semwal; John Stultz; Arve Hjønnevåg; Riley Andrews Cc: de...@driverdev.osuosl.org; Jon Medhurst; Android Kernel Team; Greg Kroah-Hartman; Will Deacon; Russell King; linux-ker...@vger.kernel.org; linaro-mm-...@lists.linaro.org; Rohit kumar; Jeremy Gebben; Eun Taik Lee; Catalin Marinas; Liviu Dudau; Laura Abbott; linux-arm-ker...@lists.infradead.org Subject: [Linaro-mm-sig] [RFC][PATCH 1/3] staging: ion: Move away from the DMA APIs for cache flushing Ion is currently using the DMA APIs in non-compliant ways for cache maintaince. The issue is Ion needs to do cache operations outside of the regular DMA model. The Ion model matches more closely with the DRM model which calls cache APIs directly. Add an appropriate abstraction layer for Ion to call cache operations outside of the DMA API. Signed-off-by: Laura Abbott <labb...@redhat.com> --- drivers/staging/android/ion/Kconfig | 14 - drivers/staging/android/ion/Makefile| 3 + drivers/staging/android/ion/ion-arm.c | 83 + drivers/staging/android/ion/ion-arm64.c | 46 ++ drivers/staging/android/ion/ion-x86.c | 34 ++ drivers/staging/android/ion/ion.c | 59 ++ drivers/staging/android/ion/ion_carveout_heap.c | 5 +- drivers/staging/android/ion/ion_chunk_heap.c| 7 +-- drivers/staging/android/ion/ion_page_pool.c | 3 +- drivers/staging/android/ion/ion_priv.h | 14 ++--- drivers/staging/android/ion/ion_system_heap.c | 5 +- 11 files changed, 210 insertions(+), 63 deletions(-) create mode 100644 drivers/staging/android/ion/ion-arm.c create mode 100644 drivers/staging/android/ion/ion-arm64.c create mode 100644 drivers/staging/android/ion/ion-x86.c diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig index 19c1572..c1b1813 100644 --- a/drivers/staging/android/ion/Kconfig +++ b/drivers/staging/android/ion/Kconfig @@ -1,6 +1,6 @@ menuconfig ION bool "Ion Memory Manager" - depends on HAVE_MEMBLOCK && HAS_DMA && MMU + depends on HAVE_MEMBLOCK && HAS_DMA && MMU && (X86 || ARM || ARM64) select GENERIC_ALLOCATOR select DMA_SHARED_BUFFER ---help--- @@ -10,6 +10,18 @@ menuconfig ION If you're not using Android its probably safe to say N here. +config ION_ARM + depends on ION && ARM + def_bool y + +config ION_ARM64 + depends on ION && ARM64 + def_bool y + +config ION_X86 + depends on ION && X86 + def_bool y + config ION_TEST tristate "Ion Test Device" depends on ION diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile index 18cc2aa..1b82bb5 100644 --- a/drivers/staging/android/ion/Makefile +++ b/drivers/staging/android/ion/Makefile @@ -1,5 +1,8 @@ obj-$(CONFIG_ION) += ion.o ion_heap.o ion_page_pool.o ion_system_heap.o \ ion_carveout_heap.o ion_chunk_heap.o ion_cma_heap.o +obj-$(CONFIG_ION_X86) += ion-x86.o +obj-$(CONFIG_ION_ARM) += ion-arm.o +obj-$(CONFIG_ION_ARM64) += ion-arm64.o obj-$(CONFIG_ION_TEST) += ion_test.o ifdef CONFIG_COMPAT obj-$(CONFIG_ION) += compat_ion.o diff --git a/drivers/staging/android/ion/ion-arm.c b/drivers/staging/android/ion/ion-arm.c new file mode 100644 index 000..b91287f --- /dev/null +++ b/drivers/staging/android/ion/ion-arm.c @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2016 Laura Abbott <la...@labbott.name> + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include +#include +#include +#include + +#include + +#include "ion_priv.h" + + +void ion_clean_page(struct page *page, size_t size) +{ + unsigned long pfn; + size_t left = size; + + pfn = page_to_pfn(page); + + /* +* A single sg entry may refer to multiple physically contiguous +* pages. But we still need to process highmem pages individually. +* If highmem is not configured then the bulk of this loop gets +* optimized out. +*/ + do { + size_t len = left; +
Re: [RFC][PATCH 1/3] staging: ion: Move away from the DMA APIs for cache flushing
On Thu, May 26, 2016 at 11:59:22AM +0100, Russell King - ARM Linux wrote: > I _really_ hate seeing architecture internal functions being abused in > drivers - architecture internal functions are there to implement the > official kernel APIs and are not for drivers to poke about with. Exactly - if drivers like drm and ion want to do something out side the current DMA API we need to add a proper API, not hack around the lack of one. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC][PATCH 1/3] staging: ion: Move away from the DMA APIs for cache flushing
On Thu, May 26, 2016 at 10:58:35AM +0100, Liviu Dudau wrote: > On Wed, May 25, 2016 at 12:48:02PM -0700, Laura Abbott wrote: > > > > Ion is currently using the DMA APIs in non-compliant ways for cache > > maintaince. The issue is Ion needs to do cache operations outside of > > the regular DMA model. The Ion model matches more closely with the > > DRM model which calls cache APIs directly. Add an appropriate > > abstraction layer for Ion to call cache operations outside of the > > DMA API. I _really_ hate seeing architecture internal functions being abused in drivers - architecture internal functions are there to implement the official kernel APIs and are not for drivers to poke about with. I've had this happen several times, and each time it makes maintanence of architecture code harder than it should be. In any case, the functions you are using are probably not appropriate - the way I've defined the architecture internal functions is for each to have a specific purpose. Eg, if caches need flushing when page tables change, then the function gets implemented, otherwise it's a no-op. Using a function which _seems_ to do the right thing today in a way which is against its purpose is a recipe for your code breaking. If you need something from the architecture which isn't already provided, then you need to talk to architecture people about proposing an official interface to that functionality, rather than trying to bolt per- architecture shims into drivers. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC][PATCH 1/3] staging: ion: Move away from the DMA APIs for cache flushing
On Wed, May 25, 2016 at 12:48:02PM -0700, Laura Abbott wrote: > > Ion is currently using the DMA APIs in non-compliant ways for cache > maintaince. The issue is Ion needs to do cache operations outside of > the regular DMA model. The Ion model matches more closely with the > DRM model which calls cache APIs directly. Add an appropriate > abstraction layer for Ion to call cache operations outside of the > DMA API. > > Signed-off-by: Laura AbbottHi Laura, By no means a full review, just some comments: > --- > drivers/staging/android/ion/Kconfig | 14 - > drivers/staging/android/ion/Makefile| 3 + > drivers/staging/android/ion/ion-arm.c | 83 > + > drivers/staging/android/ion/ion-arm64.c | 46 ++ > drivers/staging/android/ion/ion-x86.c | 34 ++ > drivers/staging/android/ion/ion.c | 59 ++ > drivers/staging/android/ion/ion_carveout_heap.c | 5 +- > drivers/staging/android/ion/ion_chunk_heap.c| 7 +-- > drivers/staging/android/ion/ion_page_pool.c | 3 +- > drivers/staging/android/ion/ion_priv.h | 14 ++--- > drivers/staging/android/ion/ion_system_heap.c | 5 +- > 11 files changed, 210 insertions(+), 63 deletions(-) > create mode 100644 drivers/staging/android/ion/ion-arm.c > create mode 100644 drivers/staging/android/ion/ion-arm64.c > create mode 100644 drivers/staging/android/ion/ion-x86.c > > diff --git a/drivers/staging/android/ion/Kconfig > b/drivers/staging/android/ion/Kconfig > index 19c1572..c1b1813 100644 > --- a/drivers/staging/android/ion/Kconfig > +++ b/drivers/staging/android/ion/Kconfig > @@ -1,6 +1,6 @@ > menuconfig ION > bool "Ion Memory Manager" > - depends on HAVE_MEMBLOCK && HAS_DMA && MMU > + depends on HAVE_MEMBLOCK && HAS_DMA && MMU && (X86 || ARM || ARM64) > select GENERIC_ALLOCATOR > select DMA_SHARED_BUFFER > ---help--- > @@ -10,6 +10,18 @@ menuconfig ION > If you're not using Android its probably safe to > say N here. > > +config ION_ARM > + depends on ION && ARM > + def_bool y > + > +config ION_ARM64 > + depends on ION && ARM64 > + def_bool y > + > +config ION_X86 > + depends on ION && X86 > + def_bool y If you are going to make this arch specific, can I suggest that you make CONFIG_ION depend on CONFIG_ARCH_ION (or any name you like) and then in each arch select it? I don't particularly like the enumeration of all ION enabled arches above (is the list exhaustive?) > + > config ION_TEST > tristate "Ion Test Device" > depends on ION > diff --git a/drivers/staging/android/ion/Makefile > b/drivers/staging/android/ion/Makefile > index 18cc2aa..1b82bb5 100644 > --- a/drivers/staging/android/ion/Makefile > +++ b/drivers/staging/android/ion/Makefile > @@ -1,5 +1,8 @@ > obj-$(CONFIG_ION) += ion.o ion_heap.o ion_page_pool.o ion_system_heap.o \ > ion_carveout_heap.o ion_chunk_heap.o ion_cma_heap.o > +obj-$(CONFIG_ION_X86) += ion-x86.o > +obj-$(CONFIG_ION_ARM) += ion-arm.o > +obj-$(CONFIG_ION_ARM64) += ion-arm64.o > obj-$(CONFIG_ION_TEST) += ion_test.o > ifdef CONFIG_COMPAT > obj-$(CONFIG_ION) += compat_ion.o > diff --git a/drivers/staging/android/ion/ion-arm.c > b/drivers/staging/android/ion/ion-arm.c > new file mode 100644 > index 000..b91287f > --- /dev/null > +++ b/drivers/staging/android/ion/ion-arm.c > @@ -0,0 +1,83 @@ > +/* > + * Copyright (C) 2016 Laura Abbott > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include > +#include > +#include > +#include > + > +#include > + > +#include "ion_priv.h" > + > + > +void ion_clean_page(struct page *page, size_t size) > +{ > + unsigned long pfn; > + size_t left = size; > + > + pfn = page_to_pfn(page); > + > + /* > + * A single sg entry may refer to multiple physically contiguous > + * pages. But we still need to process highmem pages individually. > + * If highmem is not configured then the bulk of this loop gets > + * optimized out. > + */ > + do { > + size_t len = left; > + void *vaddr; > + > + page = pfn_to_page(pfn); > + > + if (PageHighMem(page)) { > + if (len > PAGE_SIZE) > + len = PAGE_SIZE; > + > + if (cache_is_vipt_nonaliasing()) { > + vaddr = kmap_atomic(page); > +
[RFC][PATCH 1/3] staging: ion: Move away from the DMA APIs for cache flushing
Ion is currently using the DMA APIs in non-compliant ways for cache maintaince. The issue is Ion needs to do cache operations outside of the regular DMA model. The Ion model matches more closely with the DRM model which calls cache APIs directly. Add an appropriate abstraction layer for Ion to call cache operations outside of the DMA API. Signed-off-by: Laura Abbott--- drivers/staging/android/ion/Kconfig | 14 - drivers/staging/android/ion/Makefile| 3 + drivers/staging/android/ion/ion-arm.c | 83 + drivers/staging/android/ion/ion-arm64.c | 46 ++ drivers/staging/android/ion/ion-x86.c | 34 ++ drivers/staging/android/ion/ion.c | 59 ++ drivers/staging/android/ion/ion_carveout_heap.c | 5 +- drivers/staging/android/ion/ion_chunk_heap.c| 7 +-- drivers/staging/android/ion/ion_page_pool.c | 3 +- drivers/staging/android/ion/ion_priv.h | 14 ++--- drivers/staging/android/ion/ion_system_heap.c | 5 +- 11 files changed, 210 insertions(+), 63 deletions(-) create mode 100644 drivers/staging/android/ion/ion-arm.c create mode 100644 drivers/staging/android/ion/ion-arm64.c create mode 100644 drivers/staging/android/ion/ion-x86.c diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig index 19c1572..c1b1813 100644 --- a/drivers/staging/android/ion/Kconfig +++ b/drivers/staging/android/ion/Kconfig @@ -1,6 +1,6 @@ menuconfig ION bool "Ion Memory Manager" - depends on HAVE_MEMBLOCK && HAS_DMA && MMU + depends on HAVE_MEMBLOCK && HAS_DMA && MMU && (X86 || ARM || ARM64) select GENERIC_ALLOCATOR select DMA_SHARED_BUFFER ---help--- @@ -10,6 +10,18 @@ menuconfig ION If you're not using Android its probably safe to say N here. +config ION_ARM + depends on ION && ARM + def_bool y + +config ION_ARM64 + depends on ION && ARM64 + def_bool y + +config ION_X86 + depends on ION && X86 + def_bool y + config ION_TEST tristate "Ion Test Device" depends on ION diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile index 18cc2aa..1b82bb5 100644 --- a/drivers/staging/android/ion/Makefile +++ b/drivers/staging/android/ion/Makefile @@ -1,5 +1,8 @@ obj-$(CONFIG_ION) += ion.o ion_heap.o ion_page_pool.o ion_system_heap.o \ ion_carveout_heap.o ion_chunk_heap.o ion_cma_heap.o +obj-$(CONFIG_ION_X86) += ion-x86.o +obj-$(CONFIG_ION_ARM) += ion-arm.o +obj-$(CONFIG_ION_ARM64) += ion-arm64.o obj-$(CONFIG_ION_TEST) += ion_test.o ifdef CONFIG_COMPAT obj-$(CONFIG_ION) += compat_ion.o diff --git a/drivers/staging/android/ion/ion-arm.c b/drivers/staging/android/ion/ion-arm.c new file mode 100644 index 000..b91287f --- /dev/null +++ b/drivers/staging/android/ion/ion-arm.c @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2016 Laura Abbott + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include +#include +#include +#include + +#include + +#include "ion_priv.h" + + +void ion_clean_page(struct page *page, size_t size) +{ + unsigned long pfn; + size_t left = size; + + pfn = page_to_pfn(page); + + /* +* A single sg entry may refer to multiple physically contiguous +* pages. But we still need to process highmem pages individually. +* If highmem is not configured then the bulk of this loop gets +* optimized out. +*/ + do { + size_t len = left; + void *vaddr; + + page = pfn_to_page(pfn); + + if (PageHighMem(page)) { + if (len > PAGE_SIZE) + len = PAGE_SIZE; + + if (cache_is_vipt_nonaliasing()) { + vaddr = kmap_atomic(page); + __cpuc_flush_dcache_area(vaddr, len); + kunmap_atomic(vaddr); + } else { + vaddr = kmap_high_get(page); + if (vaddr) { + __cpuc_flush_dcache_area(vaddr, len); + kunmap_high(page); + } + } + } else { + vaddr = page_address(page); +