Re: [RFC][PATCH 6/6] staging: android: ion: Introduce new ioctls for dynamic heaps
On Mon, Jun 06, 2016 at 11:23:33AM -0700, Laura Abbott wrote: > From: Laura Abbott > > > The Ion ABI for heaps is limiting to work with for more complex systems. > Heaps have to be registered at boot time with known ids available to > userspace. This becomes a tight ABI which is prone to breakage. > > Introduce a new mechanism for registering heap ids dynamically. A base > set of heap ids are registered at boot time but there is no knowledge > of fallbacks. Fallback information can be remapped and changed > dynamically. Information about available heaps can also be queried with > an ioctl to avoid the need to have heap ids be an ABI with userspace. > > Signed-off-by: Laura Abbott > --- > drivers/staging/android/ion/ion-ioctl.c | 109 +-- > drivers/staging/android/ion/ion.c | 184 > ++-- > drivers/staging/android/ion/ion_priv.h | 15 +++ > drivers/staging/android/uapi/ion.h | 135 +++ > 4 files changed, 426 insertions(+), 17 deletions(-) > > diff --git a/drivers/staging/android/ion/ion-ioctl.c > b/drivers/staging/android/ion/ion-ioctl.c > index 45b89e8..169cad8 100644 > --- a/drivers/staging/android/ion/ion-ioctl.c > +++ b/drivers/staging/android/ion/ion-ioctl.c > @@ -22,6 +22,49 @@ > #include "ion_priv.h" > #include "compat_ion.h" > > +union ion_ioctl_arg { > + struct ion_fd_data fd; > + struct ion_allocation_data allocation; > + struct ion_handle_data handle; > + struct ion_custom_data custom; > + struct ion_abi_version abi_version; > + struct ion_new_alloc_data allocation2; > + struct ion_usage_id_map id_map; > + struct ion_usage_cnt usage_cnt; > + struct ion_heap_query query; > +}; > + > +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) > +{ > + int ret = 0; > + > + switch (cmd) { > + case ION_IOC_ABI_VERSION: > + ret = arg->abi_version.reserved != 0; > + break; > + case ION_IOC_ALLOC2: > + ret = arg->allocation2.reserved0 != 0; > + ret |= arg->allocation2.reserved1 != 0; > + ret |= arg->allocation2.reserved2 != 0; > + break; > + case ION_IOC_ID_MAP: > + ret = arg->id_map.reserved0 != 0; > + ret |= arg->id_map.reserved1 != 0; > + break; > + case ION_IOC_USAGE_CNT: > + ret = arg->usage_cnt.reserved != 0; > + break; > + case ION_IOC_HEAP_QUERY: > + ret = arg->query.reserved0 != 0; > + ret |= arg->query.reserved1 != 0; > + ret |= arg->query.reserved2 != 0; > + break; > + default: > + break; > + } > + return ret ? -EINVAL : 0; > +} > + > /* fix up the cases where the ioctl direction bits are incorrect */ > static unsigned int ion_ioctl_dir(unsigned int cmd) > { > @@ -42,14 +85,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > struct ion_handle *cleanup_handle = NULL; > int ret = 0; > unsigned int dir; > - > - union { > - struct ion_fd_data fd; > - struct ion_allocation_data allocation; > - struct ion_handle_data handle; > - struct ion_custom_data custom; > - struct ion_abi_version abi_version; > - } data; > + union ion_ioctl_arg data; > > dir = ion_ioctl_dir(cmd); > > @@ -60,7 +96,12 @@ long ion_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) > return -EFAULT; > > + ret = validate_ioctl_arg(cmd, &data); > + if (ret) > + return ret; > + > switch (cmd) { > + /* Old ioctl */ I can see the value of this comment, given ION_IOC_ALLOC2, but not for the other cases. > case ION_IOC_ALLOC: > { > struct ion_handle *handle; > @@ -77,6 +118,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > cleanup_handle = handle; > break; > } > + /* Old ioctl */ > case ION_IOC_FREE: > { > struct ion_handle *handle; > @@ -92,6 +134,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > mutex_unlock(&client->lock); > break; > } > + /* Old ioctl */ > case ION_IOC_SHARE: > case ION_IOC_MAP: > { > @@ -106,6 +149,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > ret = data.fd.fd; > break; > } > + /* Old ioctl */ > case ION_IOC_IMPORT: > { > struct ion_handle *handle; > @@ -117,11 +161,13 @@ long ion_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > data.handle.handle = handle->id; > break; > } > + /* Old ioctl */ >
Re: [RFC][PATCH 4/6] staging: android: ion: Pull out ion ioctls to a separate file
On Mon, Jun 06, 2016 at 11:23:31AM -0700, Laura Abbott wrote: > > The number of Ion ioctls may continue to grow along with necessary > validation. Pull it out into a separate file for easier management > and review. > > Signed-off-by: Laura Abbott Reviewed-by: Liviu Dudau > --- > drivers/staging/android/ion/Makefile| 3 +- > drivers/staging/android/ion/ion-ioctl.c | 144 ++ > drivers/staging/android/ion/ion.c | 208 > +--- > drivers/staging/android/ion/ion_priv.h | 92 ++ > 4 files changed, 244 insertions(+), 203 deletions(-) > create mode 100644 drivers/staging/android/ion/ion-ioctl.c > > diff --git a/drivers/staging/android/ion/Makefile > b/drivers/staging/android/ion/Makefile > index 18cc2aa..376c2b2 100644 > --- a/drivers/staging/android/ion/Makefile > +++ b/drivers/staging/android/ion/Makefile > @@ -1,4 +1,5 @@ > -obj-$(CONFIG_ION) += ion.o ion_heap.o ion_page_pool.o ion_system_heap.o \ > +obj-$(CONFIG_ION) += ion.o ion-ioctl.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_TEST) += ion_test.o > ifdef CONFIG_COMPAT > diff --git a/drivers/staging/android/ion/ion-ioctl.c > b/drivers/staging/android/ion/ion-ioctl.c > new file mode 100644 > index 000..341ba7d > --- /dev/null > +++ b/drivers/staging/android/ion/ion-ioctl.c > @@ -0,0 +1,144 @@ > +/* > + * > + * Copyright (C) 2011 Google, Inc. > + * > + * 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 "ion.h" > +#include "ion_priv.h" > +#include "compat_ion.h" > + > +/* fix up the cases where the ioctl direction bits are incorrect */ > +static unsigned int ion_ioctl_dir(unsigned int cmd) > +{ > + switch (cmd) { > + case ION_IOC_SYNC: > + case ION_IOC_FREE: > + case ION_IOC_CUSTOM: > + return _IOC_WRITE; > + default: > + return _IOC_DIR(cmd); > + } > +} > + > +long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{ > + struct ion_client *client = filp->private_data; > + struct ion_device *dev = client->dev; > + struct ion_handle *cleanup_handle = NULL; > + int ret = 0; > + unsigned int dir; > + > + union { > + struct ion_fd_data fd; > + struct ion_allocation_data allocation; > + struct ion_handle_data handle; > + struct ion_custom_data custom; > + } data; > + > + dir = ion_ioctl_dir(cmd); > + > + if (_IOC_SIZE(cmd) > sizeof(data)) > + return -EINVAL; > + > + if (dir & _IOC_WRITE) > + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) > + return -EFAULT; > + > + switch (cmd) { > + case ION_IOC_ALLOC: > + { > + struct ion_handle *handle; > + > + handle = ion_alloc(client, data.allocation.len, > + data.allocation.align, > + data.allocation.heap_id_mask, > + data.allocation.flags); > + if (IS_ERR(handle)) > + return PTR_ERR(handle); > + > + data.allocation.handle = handle->id; > + > + cleanup_handle = handle; > + break; > + } > + case ION_IOC_FREE: > + { > + struct ion_handle *handle; > + > + mutex_lock(&client->lock); > + handle = ion_handle_get_by_id_nolock(client, > data.handle.handle); > + if (IS_ERR(handle)) { > + mutex_unlock(&client->lock); > + return PTR_ERR(handle); > + } > + ion_free_nolock(client, handle); > + ion_handle_put_nolock(handle); > + mutex_unlock(&client->lock); > + break; > + } > + case ION_IOC_SHARE: > + case ION_IOC_MAP: > + { > + struct ion_ha
Re: [RFC][PATCH 2/6] staging: android: ion: Switch to using an idr to manage heaps
bug_shrink_get, > int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap) > { > struct dentry *debug_file; > + int ret; > > if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma || > !heap->ops->unmap_dma) { > @@ -1606,12 +1626,15 @@ int ion_device_add_heap(struct ion_device *dev, > struct ion_heap *heap) > > heap->dev = dev; > down_write(&dev->lock); > - /* > - * use negative heap->id to reverse the priority -- when traversing > - * the list later attempt higher id numbers first > - */ > - plist_node_init(&heap->node, -heap->id); > - plist_add(&heap->node, &dev->heaps); > + > + ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1, GFP_KERNEL); > + if (ret < 0 || ret != heap->id) { > + pr_info("%s: Failed to add heap id, expected %d got %d\n", > + __func__, heap->id, ret); > + up_write(&dev->lock); > + return ret < 0 ? ret : -EINVAL; > + } > + > debug_file = debugfs_create_file(heap->name, 0664, > dev->heaps_debug_root, heap, > &debug_heap_fops); > @@ -1639,7 +1662,7 @@ int ion_device_add_heap(struct ion_device *dev, struct > ion_heap *heap) > path, debug_name); > } > } > - > + dev->heap_cnt++; > up_write(&dev->lock); > return 0; > } > @@ -1689,7 +1712,7 @@ debugfs_done: > idev->buffers = RB_ROOT; > mutex_init(&idev->buffer_lock); > init_rwsem(&idev->lock); > - plist_head_init(&idev->heaps); > + idr_init(&idev->idr); > idev->clients = RB_ROOT; > ion_root_client = &idev->clients; > mutex_init(&debugfs_mutex); > -- > 2.5.5 > Reviewed-by: Liviu Dudau -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC][PATCH 1/6] staging: android: ion: return error value for ion_device_add_heap
On Mon, Jun 06, 2016 at 11:23:28AM -0700, Laura Abbott wrote: > From: Laura Abbott > > > ion_device_add_heap doesn't return an error value. Change it to return > information to callers. > > Signed-off-by: Laura Abbott Reviewed-by: Liviu Dudau > --- > drivers/staging/android/ion/ion.c | 7 +-- > drivers/staging/android/ion/ion_priv.h | 2 +- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/android/ion/ion.c > b/drivers/staging/android/ion/ion.c > index a2cf93b..306340a 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -1584,14 +1584,16 @@ static int debug_shrink_get(void *data, u64 *val) > DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get, > debug_shrink_set, "%llu\n"); > > -void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap) > +int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap) > { > struct dentry *debug_file; > > if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma || > - !heap->ops->unmap_dma) > + !heap->ops->unmap_dma) { > pr_err("%s: can not add heap with invalid ops struct.\n", > __func__); > + return -EINVAL; > + } > > spin_lock_init(&heap->free_lock); > heap->free_list_size = 0; > @@ -1639,6 +1641,7 @@ void ion_device_add_heap(struct ion_device *dev, struct > ion_heap *heap) > } > > up_write(&dev->lock); > + return 0; > } > EXPORT_SYMBOL(ion_device_add_heap); > > diff --git a/drivers/staging/android/ion/ion_priv.h > b/drivers/staging/android/ion/ion_priv.h > index 0239883..35726ae 100644 > --- a/drivers/staging/android/ion/ion_priv.h > +++ b/drivers/staging/android/ion/ion_priv.h > @@ -221,7 +221,7 @@ void ion_device_destroy(struct ion_device *dev); > * @dev: the device > * @heap:the heap to add > */ > -void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap); > +int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap); > > /** > * some helpers for common operations on buffers using the sg_table > -- > 2.5.5 > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ ___ 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 Abbott Hi 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); > + __cpuc_flush_dcache