Current ion defined allocation ioctl doesn't allow to specify the requested
allocation alignment. CMA heap allocates buffers aligned on buffer size
page order.
Sometimes, the alignment requirement is less restrictive. In such cases,
providing specific alignment may reduce the external memory fragmentation
and in some cases it may avoid the allocation request failure.
To fix this, we add an alignment parameter to the allocation ioctl.
Signed-off-by: Alexey Skidanov <[email protected]>
---
drivers/staging/android/ion/ion-ioctl.c | 3 ++-
drivers/staging/android/ion/ion.c | 14 +++++++++-----
drivers/staging/android/ion/ion.h | 9 ++++++---
drivers/staging/android/ion/ion_carveout_heap.c | 3 ++-
drivers/staging/android/ion/ion_chunk_heap.c | 3 ++-
drivers/staging/android/ion/ion_cma_heap.c | 18 ++++++++++++------
drivers/staging/android/ion/ion_system_heap.c | 6 ++++--
drivers/staging/android/uapi/ion.h | 7 +++----
8 files changed, 40 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index c789893..9fe022b 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
fd = ion_alloc(data.allocation.len,
data.allocation.heap_id_mask,
- data.allocation.flags);
+ data.allocation.flags,
+ data.allocation.align);
if (fd < 0)
return fd;
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index f480885..35ddc7a 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
struct ion_device *dev,
unsigned long len,
- unsigned long flags)
+ unsigned long flags,
+ unsigned int align)
{
struct ion_buffer *buffer;
int ret;
@@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
buffer->heap = heap;
buffer->flags = flags;
- ret = heap->ops->allocate(heap, buffer, len, flags);
+ ret = heap->ops->allocate(heap, buffer, len, flags, align);
if (ret) {
if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
goto err2;
ion_heap_freelist_drain(heap, 0);
- ret = heap->ops->allocate(heap, buffer, len, flags);
+ ret = heap->ops->allocate(heap, buffer, len, flags, align);
if (ret)
goto err2;
}
@@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
.unmap = ion_dma_buf_kunmap,
};
-int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
+int ion_alloc(size_t len,
+ unsigned int heap_id_mask,
+ unsigned int flags,
+ unsigned int align)
{
struct ion_device *dev = internal_dev;
struct ion_buffer *buffer = NULL;
@@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
/* if the caller didn't specify this heap id */
if (!((1 << heap->id) & heap_id_mask))
continue;
- buffer = ion_buffer_create(heap, dev, len, flags);
+ buffer = ion_buffer_create(heap, dev, len, flags, align);
if (!IS_ERR(buffer))
break;
}
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index f5f9cd6..0c161d2 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -123,8 +123,10 @@ struct ion_device {
*/
struct ion_heap_ops {
int (*allocate)(struct ion_heap *heap,
- struct ion_buffer *buffer, unsigned long len,
- unsigned long flags);
+ struct ion_buffer *buffer,
+ unsigned long len,
+ unsigned long flags,
+ unsigned int align);
void (*free)(struct ion_buffer *buffer);
void * (*map_kernel)(struct ion_heap *heap, struct ion_buffer *buffer);
void (*unmap_kernel)(struct ion_heap *heap, struct ion_buffer *buffer);
@@ -228,7 +230,8 @@ int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot);
int ion_alloc(size_t len,
unsigned int heap_id_mask,
- unsigned int flags);
+ unsigned int flags,
+ unsigned int align);
/**
* ion_heap_init_shrinker
diff --git a/drivers/staging/android/ion/ion_carveout_heap.c b/drivers/staging/android/ion/ion_carveout_heap.c
index fee7650..deae9dd 100644
--- a/drivers/staging/android/ion/ion_carveout_heap.c
+++ b/drivers/staging/android/ion/ion_carveout_heap.c
@@ -59,7 +59,8 @@ static void ion_carveout_free(struct ion_heap *heap, phys_addr_t addr,
static int ion_carveout_heap_allocate(struct ion_heap *heap,
struct ion_buffer *buffer,
unsigned long size,
- unsigned long flags)
+ unsigned long flags,
+ unsigned int align)
{
struct sg_table *table;
phys_addr_t paddr;
diff --git a/drivers/staging/android/ion/ion_chunk_heap.c b/drivers/staging/android/ion/ion_chunk_heap.c
index 102c093..15f9518 100644
--- a/drivers/staging/android/ion/ion_chunk_heap.c
+++ b/drivers/staging/android/ion/ion_chunk_heap.c
@@ -35,7 +35,8 @@ struct ion_chunk_heap {
static int ion_chunk_heap_allocate(struct ion_heap *heap,
struct ion_buffer *buffer,
unsigned long size,
- unsigned long flags)
+ unsigned long flags,
+ unsigned int align)
{
struct ion_chunk_heap *chunk_heap =
container_of(heap, struct ion_chunk_heap, heap);
diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
index 86196ff..f3f8deb 100644
--- a/drivers/staging/android/ion/ion_cma_heap.c
+++ b/drivers/staging/android/ion/ion_cma_heap.c
@@ -32,25 +32,31 @@ struct ion_cma_heap {
#define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
/* ION CMA heap operations functions */
-static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
+static int ion_cma_allocate(struct ion_heap *heap,
+ struct ion_buffer *buffer,
unsigned long len,
- unsigned long flags)
+ unsigned long flags,
+ unsigned int align)
{
struct ion_cma_heap *cma_heap = to_cma_heap(heap);
struct sg_table *table;
struct page *pages;
unsigned long size = PAGE_ALIGN(len);
unsigned long nr_pages = size >> PAGE_SHIFT;
- unsigned long align = get_order(size);
+ int order = get_order(align);
int ret;
- if (align > CONFIG_CMA_ALIGNMENT)
- align = CONFIG_CMA_ALIGNMENT;
+ /* CMA allocation alignment is in PAGE_SIZE order */
+ if (order > CONFIG_CMA_ALIGNMENT)
+ order = CONFIG_CMA_ALIGNMENT;
- pages = cma_alloc(cma_heap->cma, nr_pages, align, GFP_KERNEL);
+ pages = cma_alloc(cma_heap->cma, nr_pages, order, GFP_KERNEL);
if (!pages)
return -ENOMEM;
+ pr_debug("Allocated block of %lu pages starting at 0x%lX",
+ nr_pages, page_to_pfn(pages));
+
table = kmalloc(sizeof(*table), GFP_KERNEL);
if (!table)
goto err;
diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index 4dc5d7a..b5a1720 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -125,7 +125,8 @@ static struct page *alloc_largest_available(struct ion_system_heap *heap,
static int ion_system_heap_allocate(struct ion_heap *heap,
struct ion_buffer *buffer,
unsigned long size,
- unsigned long flags)
+ unsigned long flags,
+ unsigned int align)
{
struct ion_system_heap *sys_heap = container_of(heap,
struct ion_system_heap,
@@ -363,7 +364,8 @@ device_initcall(ion_system_heap_create);
static int ion_system_contig_heap_allocate(struct ion_heap *heap,
struct ion_buffer *buffer,
unsigned long len,
- unsigned long flags)
+ unsigned long flags,
+ unsigned int align)
{
int order = get_order(len);
struct page *page;
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index 9e21451..b093edd 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -70,9 +70,8 @@ enum ion_heap_type {
* @len: size of the allocation
* @heap_id_mask: mask of heap ids to allocate from
* @flags: flags passed to heap
- * @handle: pointer that will be populated with a cookie to use to
- * refer to this allocation
- *
+ * @fd: file descriptor associated with newly allocated buffer
+ * @align: allocation alignment
* Provided by userspace as an argument to the ioctl
*/
struct ion_allocation_data {
@@ -80,7 +79,7 @@ struct ion_allocation_data {
__u32 heap_id_mask;
__u32 flags;
__u32 fd;
- __u32 unused;
+ __u32 align;
};
#define MAX_HEAP_NAME 32
--
2.7.4
On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
> Current ion defined allocation ioctl doesn't allow to specify the requested
> allocation alignment. CMA heap allocates buffers aligned on buffer size
> page order.
>
> Sometimes, the alignment requirement is less restrictive. In such cases,
> providing specific alignment may reduce the external memory fragmentation
> and in some cases it may avoid the allocation request failure.
>
I really do not want to bring this back as part of the regular
ABI. Having an alignment parameter that gets used for exactly
one heap only leads to confusion (which is why it was removed
from the ABI in the first place).
The alignment came from the behavior of the DMA APIs. Do you
actually need to specify any alignment from userspace or do
you only need page size?
Thanks,
Laura
> To fix this, we add an alignment parameter to the allocation ioctl.
>
> Signed-off-by: Alexey Skidanov <[email protected]>
> ---
> drivers/staging/android/ion/ion-ioctl.c | 3 ++-
> drivers/staging/android/ion/ion.c | 14 +++++++++-----
> drivers/staging/android/ion/ion.h | 9 ++++++---
> drivers/staging/android/ion/ion_carveout_heap.c | 3 ++-
> drivers/staging/android/ion/ion_chunk_heap.c | 3 ++-
> drivers/staging/android/ion/ion_cma_heap.c | 18 ++++++++++++------
> drivers/staging/android/ion/ion_system_heap.c | 6 ++++--
> drivers/staging/android/uapi/ion.h | 7 +++----
> 8 files changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index c789893..9fe022b 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>
> fd = ion_alloc(data.allocation.len,
> data.allocation.heap_id_mask,
> - data.allocation.flags);
> + data.allocation.flags,
> + data.allocation.align);
> if (fd < 0)
> return fd;
>
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index f480885..35ddc7a 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
> static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
> struct ion_device *dev,
> unsigned long len,
> - unsigned long flags)
> + unsigned long flags,
> + unsigned int align)
> {
> struct ion_buffer *buffer;
> int ret;
> @@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
> buffer->heap = heap;
> buffer->flags = flags;
>
> - ret = heap->ops->allocate(heap, buffer, len, flags);
> + ret = heap->ops->allocate(heap, buffer, len, flags, align);
>
> if (ret) {
> if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
> goto err2;
>
> ion_heap_freelist_drain(heap, 0);
> - ret = heap->ops->allocate(heap, buffer, len, flags);
> + ret = heap->ops->allocate(heap, buffer, len, flags, align);
> if (ret)
> goto err2;
> }
> @@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
> .unmap = ion_dma_buf_kunmap,
> };
>
> -int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
> +int ion_alloc(size_t len,
> + unsigned int heap_id_mask,
> + unsigned int flags,
> + unsigned int align)
> {
> struct ion_device *dev = internal_dev;
> struct ion_buffer *buffer = NULL;
> @@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
> /* if the caller didn't specify this heap id */
> if (!((1 << heap->id) & heap_id_mask))
> continue;
> - buffer = ion_buffer_create(heap, dev, len, flags);
> + buffer = ion_buffer_create(heap, dev, len, flags, align);
> if (!IS_ERR(buffer))
> break;
> }
> diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
> index f5f9cd6..0c161d2 100644
> --- a/drivers/staging/android/ion/ion.h
> +++ b/drivers/staging/android/ion/ion.h
> @@ -123,8 +123,10 @@ struct ion_device {
> */
> struct ion_heap_ops {
> int (*allocate)(struct ion_heap *heap,
> - struct ion_buffer *buffer, unsigned long len,
> - unsigned long flags);
> + struct ion_buffer *buffer,
> + unsigned long len,
> + unsigned long flags,
> + unsigned int align);
> void (*free)(struct ion_buffer *buffer);
> void * (*map_kernel)(struct ion_heap *heap, struct ion_buffer *buffer);
> void (*unmap_kernel)(struct ion_heap *heap, struct ion_buffer *buffer);
> @@ -228,7 +230,8 @@ int ion_heap_pages_zero(struct page *page, size_t size, pgprot_t pgprot);
>
> int ion_alloc(size_t len,
> unsigned int heap_id_mask,
> - unsigned int flags);
> + unsigned int flags,
> + unsigned int align);
>
> /**
> * ion_heap_init_shrinker
> diff --git a/drivers/staging/android/ion/ion_carveout_heap.c b/drivers/staging/android/ion/ion_carveout_heap.c
> index fee7650..deae9dd 100644
> --- a/drivers/staging/android/ion/ion_carveout_heap.c
> +++ b/drivers/staging/android/ion/ion_carveout_heap.c
> @@ -59,7 +59,8 @@ static void ion_carveout_free(struct ion_heap *heap, phys_addr_t addr,
> static int ion_carveout_heap_allocate(struct ion_heap *heap,
> struct ion_buffer *buffer,
> unsigned long size,
> - unsigned long flags)
> + unsigned long flags,
> + unsigned int align)
> {
> struct sg_table *table;
> phys_addr_t paddr;
> diff --git a/drivers/staging/android/ion/ion_chunk_heap.c b/drivers/staging/android/ion/ion_chunk_heap.c
> index 102c093..15f9518 100644
> --- a/drivers/staging/android/ion/ion_chunk_heap.c
> +++ b/drivers/staging/android/ion/ion_chunk_heap.c
> @@ -35,7 +35,8 @@ struct ion_chunk_heap {
> static int ion_chunk_heap_allocate(struct ion_heap *heap,
> struct ion_buffer *buffer,
> unsigned long size,
> - unsigned long flags)
> + unsigned long flags,
> + unsigned int align)
> {
> struct ion_chunk_heap *chunk_heap =
> container_of(heap, struct ion_chunk_heap, heap);
> diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> index 86196ff..f3f8deb 100644
> --- a/drivers/staging/android/ion/ion_cma_heap.c
> +++ b/drivers/staging/android/ion/ion_cma_heap.c
> @@ -32,25 +32,31 @@ struct ion_cma_heap {
> #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
>
> /* ION CMA heap operations functions */
> -static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
> +static int ion_cma_allocate(struct ion_heap *heap,
> + struct ion_buffer *buffer,
> unsigned long len,
> - unsigned long flags)
> + unsigned long flags,
> + unsigned int align)
> {
> struct ion_cma_heap *cma_heap = to_cma_heap(heap);
> struct sg_table *table;
> struct page *pages;
> unsigned long size = PAGE_ALIGN(len);
> unsigned long nr_pages = size >> PAGE_SHIFT;
> - unsigned long align = get_order(size);
> + int order = get_order(align);
> int ret;
>
> - if (align > CONFIG_CMA_ALIGNMENT)
> - align = CONFIG_CMA_ALIGNMENT;
> + /* CMA allocation alignment is in PAGE_SIZE order */
> + if (order > CONFIG_CMA_ALIGNMENT)
> + order = CONFIG_CMA_ALIGNMENT;
>
> - pages = cma_alloc(cma_heap->cma, nr_pages, align, GFP_KERNEL);
> + pages = cma_alloc(cma_heap->cma, nr_pages, order, GFP_KERNEL);
> if (!pages)
> return -ENOMEM;
>
> + pr_debug("Allocated block of %lu pages starting at 0x%lX",
> + nr_pages, page_to_pfn(pages));
> +
> table = kmalloc(sizeof(*table), GFP_KERNEL);
> if (!table)
> goto err;
> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> index 4dc5d7a..b5a1720 100644
> --- a/drivers/staging/android/ion/ion_system_heap.c
> +++ b/drivers/staging/android/ion/ion_system_heap.c
> @@ -125,7 +125,8 @@ static struct page *alloc_largest_available(struct ion_system_heap *heap,
> static int ion_system_heap_allocate(struct ion_heap *heap,
> struct ion_buffer *buffer,
> unsigned long size,
> - unsigned long flags)
> + unsigned long flags,
> + unsigned int align)
> {
> struct ion_system_heap *sys_heap = container_of(heap,
> struct ion_system_heap,
> @@ -363,7 +364,8 @@ device_initcall(ion_system_heap_create);
> static int ion_system_contig_heap_allocate(struct ion_heap *heap,
> struct ion_buffer *buffer,
> unsigned long len,
> - unsigned long flags)
> + unsigned long flags,
> + unsigned int align)
> {
> int order = get_order(len);
> struct page *page;
> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
> index 9e21451..b093edd 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -70,9 +70,8 @@ enum ion_heap_type {
> * @len: size of the allocation
> * @heap_id_mask: mask of heap ids to allocate from
> * @flags: flags passed to heap
> - * @handle: pointer that will be populated with a cookie to use to
> - * refer to this allocation
> - *
> + * @fd: file descriptor associated with newly allocated buffer
> + * @align: allocation alignment
> * Provided by userspace as an argument to the ioctl
> */
> struct ion_allocation_data {
> @@ -80,7 +79,7 @@ struct ion_allocation_data {
> __u32 heap_id_mask;
> __u32 flags;
> __u32 fd;
> - __u32 unused;
> + __u32 align;
> };
>
> #define MAX_HEAP_NAME 32
>
On 02/12/2018 08:42 PM, Laura Abbott wrote:
> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>> Current ion defined allocation ioctl doesn't allow to specify the
>> requested
>> allocation alignment. CMA heap allocates buffers aligned on buffer size
>> page order.
>>
>> Sometimes, the alignment requirement is less restrictive. In such cases,
>> providing specific alignment may reduce the external memory fragmentation
>> and in some cases it may avoid the allocation request failure.
>>
>
> I really do not want to bring this back as part of the regular
> ABI.
Yes, I know it was removed in 4.12.
Having an alignment parameter that gets used for exactly
> one heap only leads to confusion (which is why it was removed
> from the ABI in the first place).
You are correct regarding the CMA heap. But, probably it may be used by
custom heap as well.
>
> The alignment came from the behavior of the DMA APIs. Do you
> actually need to specify any alignment from userspace or do
> you only need page size?
Yes. If CMA gives it for free, I would suggest to let the ion user to decide
>
> Thanks,
> Laura
>
Thanks,
Alexey
>> To fix this, we add an alignment parameter to the allocation ioctl.
>>
>> Signed-off-by: Alexey Skidanov <[email protected]>
>> ---
>> drivers/staging/android/ion/ion-ioctl.c | 3 ++-
>> drivers/staging/android/ion/ion.c | 14 +++++++++-----
>> drivers/staging/android/ion/ion.h | 9 ++++++---
>> drivers/staging/android/ion/ion_carveout_heap.c | 3 ++-
>> drivers/staging/android/ion/ion_chunk_heap.c | 3 ++-
>> drivers/staging/android/ion/ion_cma_heap.c | 18 ++++++++++++------
>> drivers/staging/android/ion/ion_system_heap.c | 6 ++++--
>> drivers/staging/android/uapi/ion.h | 7 +++----
>> 8 files changed, 40 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index c789893..9fe022b 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -83,7 +83,8 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>> fd = ion_alloc(data.allocation.len,
>> data.allocation.heap_id_mask,
>> - data.allocation.flags);
>> + data.allocation.flags,
>> + data.allocation.align);
>> if (fd < 0)
>> return fd;
>> diff --git a/drivers/staging/android/ion/ion.c
>> b/drivers/staging/android/ion/ion.c
>> index f480885..35ddc7a 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -78,7 +78,8 @@ static void ion_buffer_add(struct ion_device *dev,
>> static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
>> struct ion_device *dev,
>> unsigned long len,
>> - unsigned long flags)
>> + unsigned long flags,
>> + unsigned int align)
>> {
>> struct ion_buffer *buffer;
>> int ret;
>> @@ -90,14 +91,14 @@ static struct ion_buffer *ion_buffer_create(struct
>> ion_heap *heap,
>> buffer->heap = heap;
>> buffer->flags = flags;
>> - ret = heap->ops->allocate(heap, buffer, len, flags);
>> + ret = heap->ops->allocate(heap, buffer, len, flags, align);
>> if (ret) {
>> if (!(heap->flags & ION_HEAP_FLAG_DEFER_FREE))
>> goto err2;
>> ion_heap_freelist_drain(heap, 0);
>> - ret = heap->ops->allocate(heap, buffer, len, flags);
>> + ret = heap->ops->allocate(heap, buffer, len, flags, align);
>> if (ret)
>> goto err2;
>> }
>> @@ -390,7 +391,10 @@ static const struct dma_buf_ops dma_buf_ops = {
>> .unmap = ion_dma_buf_kunmap,
>> };
>> -int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int
>> flags)
>> +int ion_alloc(size_t len,
>> + unsigned int heap_id_mask,
>> + unsigned int flags,
>> + unsigned int align)
>> {
>> struct ion_device *dev = internal_dev;
>> struct ion_buffer *buffer = NULL;
>> @@ -417,7 +421,7 @@ int ion_alloc(size_t len, unsigned int
>> heap_id_mask, unsigned int flags)
>> /* if the caller didn't specify this heap id */
>> if (!((1 << heap->id) & heap_id_mask))
>> continue;
>> - buffer = ion_buffer_create(heap, dev, len, flags);
>> + buffer = ion_buffer_create(heap, dev, len, flags, align);
>> if (!IS_ERR(buffer))
>> break;
>> }
>> diff --git a/drivers/staging/android/ion/ion.h
>> b/drivers/staging/android/ion/ion.h
>> index f5f9cd6..0c161d2 100644
>> --- a/drivers/staging/android/ion/ion.h
>> +++ b/drivers/staging/android/ion/ion.h
>> @@ -123,8 +123,10 @@ struct ion_device {
>> */
>> struct ion_heap_ops {
>> int (*allocate)(struct ion_heap *heap,
>> - struct ion_buffer *buffer, unsigned long len,
>> - unsigned long flags);
>> + struct ion_buffer *buffer,
>> + unsigned long len,
>> + unsigned long flags,
>> + unsigned int align);
>> void (*free)(struct ion_buffer *buffer);
>> void * (*map_kernel)(struct ion_heap *heap, struct ion_buffer
>> *buffer);
>> void (*unmap_kernel)(struct ion_heap *heap, struct ion_buffer
>> *buffer);
>> @@ -228,7 +230,8 @@ int ion_heap_pages_zero(struct page *page, size_t
>> size, pgprot_t pgprot);
>> int ion_alloc(size_t len,
>> unsigned int heap_id_mask,
>> - unsigned int flags);
>> + unsigned int flags,
>> + unsigned int align);
>> /**
>> * ion_heap_init_shrinker
>> diff --git a/drivers/staging/android/ion/ion_carveout_heap.c
>> b/drivers/staging/android/ion/ion_carveout_heap.c
>> index fee7650..deae9dd 100644
>> --- a/drivers/staging/android/ion/ion_carveout_heap.c
>> +++ b/drivers/staging/android/ion/ion_carveout_heap.c
>> @@ -59,7 +59,8 @@ static void ion_carveout_free(struct ion_heap *heap,
>> phys_addr_t addr,
>> static int ion_carveout_heap_allocate(struct ion_heap *heap,
>> struct ion_buffer *buffer,
>> unsigned long size,
>> - unsigned long flags)
>> + unsigned long flags,
>> + unsigned int align)
>> {
>> struct sg_table *table;
>> phys_addr_t paddr;
>> diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
>> b/drivers/staging/android/ion/ion_chunk_heap.c
>> index 102c093..15f9518 100644
>> --- a/drivers/staging/android/ion/ion_chunk_heap.c
>> +++ b/drivers/staging/android/ion/ion_chunk_heap.c
>> @@ -35,7 +35,8 @@ struct ion_chunk_heap {
>> static int ion_chunk_heap_allocate(struct ion_heap *heap,
>> struct ion_buffer *buffer,
>> unsigned long size,
>> - unsigned long flags)
>> + unsigned long flags,
>> + unsigned int align)
>> {
>> struct ion_chunk_heap *chunk_heap =
>> container_of(heap, struct ion_chunk_heap, heap);
>> diff --git a/drivers/staging/android/ion/ion_cma_heap.c
>> b/drivers/staging/android/ion/ion_cma_heap.c
>> index 86196ff..f3f8deb 100644
>> --- a/drivers/staging/android/ion/ion_cma_heap.c
>> +++ b/drivers/staging/android/ion/ion_cma_heap.c
>> @@ -32,25 +32,31 @@ struct ion_cma_heap {
>> #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
>> /* ION CMA heap operations functions */
>> -static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer
>> *buffer,
>> +static int ion_cma_allocate(struct ion_heap *heap,
>> + struct ion_buffer *buffer,
>> unsigned long len,
>> - unsigned long flags)
>> + unsigned long flags,
>> + unsigned int align)
>> {
>> struct ion_cma_heap *cma_heap = to_cma_heap(heap);
>> struct sg_table *table;
>> struct page *pages;
>> unsigned long size = PAGE_ALIGN(len);
>> unsigned long nr_pages = size >> PAGE_SHIFT;
>> - unsigned long align = get_order(size);
>> + int order = get_order(align);
>> int ret;
>> - if (align > CONFIG_CMA_ALIGNMENT)
>> - align = CONFIG_CMA_ALIGNMENT;
>> + /* CMA allocation alignment is in PAGE_SIZE order */
>> + if (order > CONFIG_CMA_ALIGNMENT)
>> + order = CONFIG_CMA_ALIGNMENT;
>> - pages = cma_alloc(cma_heap->cma, nr_pages, align, GFP_KERNEL);
>> + pages = cma_alloc(cma_heap->cma, nr_pages, order, GFP_KERNEL);
>> if (!pages)
>> return -ENOMEM;
>> + pr_debug("Allocated block of %lu pages starting at 0x%lX",
>> + nr_pages, page_to_pfn(pages));
>> +
>> table = kmalloc(sizeof(*table), GFP_KERNEL);
>> if (!table)
>> goto err;
>> diff --git a/drivers/staging/android/ion/ion_system_heap.c
>> b/drivers/staging/android/ion/ion_system_heap.c
>> index 4dc5d7a..b5a1720 100644
>> --- a/drivers/staging/android/ion/ion_system_heap.c
>> +++ b/drivers/staging/android/ion/ion_system_heap.c
>> @@ -125,7 +125,8 @@ static struct page *alloc_largest_available(struct
>> ion_system_heap *heap,
>> static int ion_system_heap_allocate(struct ion_heap *heap,
>> struct ion_buffer *buffer,
>> unsigned long size,
>> - unsigned long flags)
>> + unsigned long flags,
>> + unsigned int align)
>> {
>> struct ion_system_heap *sys_heap = container_of(heap,
>> struct ion_system_heap,
>> @@ -363,7 +364,8 @@ device_initcall(ion_system_heap_create);
>> static int ion_system_contig_heap_allocate(struct ion_heap *heap,
>> struct ion_buffer *buffer,
>> unsigned long len,
>> - unsigned long flags)
>> + unsigned long flags,
>> + unsigned int align)
>> {
>> int order = get_order(len);
>> struct page *page;
>> diff --git a/drivers/staging/android/uapi/ion.h
>> b/drivers/staging/android/uapi/ion.h
>> index 9e21451..b093edd 100644
>> --- a/drivers/staging/android/uapi/ion.h
>> +++ b/drivers/staging/android/uapi/ion.h
>> @@ -70,9 +70,8 @@ enum ion_heap_type {
>> * @len: size of the allocation
>> * @heap_id_mask: mask of heap ids to allocate from
>> * @flags: flags passed to heap
>> - * @handle: pointer that will be populated with a cookie to
>> use to
>> - * refer to this allocation
>> - *
>> + * @fd: file descriptor associated with newly allocated buffer
>> + * @align: allocation alignment
>> * Provided by userspace as an argument to the ioctl
>> */
>> struct ion_allocation_data {
>> @@ -80,7 +79,7 @@ struct ion_allocation_data {
>> __u32 heap_id_mask;
>> __u32 flags;
>> __u32 fd;
>> - __u32 unused;
>> + __u32 align;
>> };
>> #define MAX_HEAP_NAME 32
>>
>
On 02/12/2018 11:11 AM, Alexey Skidanov wrote:
>
> On 02/12/2018 08:42 PM, Laura Abbott wrote:
>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>>> Current ion defined allocation ioctl doesn't allow to specify the
>>> requested
>>> allocation alignment. CMA heap allocates buffers aligned on buffer size
>>> page order.
>>>
>>> Sometimes, the alignment requirement is less restrictive. In such cases,
>>> providing specific alignment may reduce the external memory fragmentation
>>> and in some cases it may avoid the allocation request failure.
>>>
>> I really do not want to bring this back as part of the regular
>> ABI.
> Yes, I know it was removed in 4.12.
> Having an alignment parameter that gets used for exactly
>> one heap only leads to confusion (which is why it was removed
>> from the ABI in the first place).
> You are correct regarding the CMA heap. But, probably it may be used by
> custom heap as well.
I can think of a lot of instances where it could be used but
ultimately there needs to be an actual in kernel user who wants
it.
>> The alignment came from the behavior of the DMA APIs. Do you
>> actually need to specify any alignment from userspace or do
>> you only need page size?
> Yes. If CMA gives it for free, I would suggest to let the ion user to decide
I'm really not convinced changing the ABI yet again just to let
the user decide is actually worth it. If we can manage it, I'd
much rather see a proposal that doesn't change the ABI.
>> Thanks,
>> Laura
>>
> Thanks,
> Alexey
>
On 02/12/2018 09:52 PM, Laura Abbott wrote:
> On 02/12/2018 11:11 AM, Alexey Skidanov wrote:
>>
>> On 02/12/2018 08:42 PM, Laura Abbott wrote:
>>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>>>> Current ion defined allocation ioctl doesn't allow to specify the
>>>> requested
>>>> allocation alignment. CMA heap allocates buffers aligned on buffer size
>>>> page order.
>>>>
>>>> Sometimes, the alignment requirement is less restrictive. In such
>>>> cases,
>>>> providing specific alignment may reduce the external memory
>>>> fragmentation
>>>> and in some cases it may avoid the allocation request failure.
>>>>
>>> I really do not want to bring this back as part of the regular
>>> ABI.
>> Yes, I know it was removed in 4.12.
>> Having an alignment parameter that gets used for exactly
>>> one heap only leads to confusion (which is why it was removed
>>> from the ABI in the first place).
>> You are correct regarding the CMA heap. But, probably it may be used by
>> custom heap as well.
>
> I can think of a lot of instances where it could be used but
> ultimately there needs to be an actual in kernel user who wants
> it.
>
>>> The alignment came from the behavior of the DMA APIs. Do you
>>> actually need to specify any alignment from userspace or do
>>> you only need page size?
>> Yes. If CMA gives it for free, I would suggest to let the ion user to
>> decide
>
> I'm really not convinced changing the ABI yet again just to let
> the user decide is actually worth it. If we can manage it, I'd
> much rather see a proposal that doesn't change the ABI.
I didn't actually change the ABI - I just use the "unused" member:
struct ion_allocation_data {
@@ -80,7 +79,7 @@ struct ion_allocation_data {
__u32 heap_id_mask;
__u32 flags;
__u32 fd;
- __u32 unused;
+ __u32 align;
};
As an alternative, I may add __u64 heap_specific_param - but this will
change the ABI. But, probably it makes the ABI more generic?
>
>>> Thanks,
>>> Laura
>>>
>> Thanks,
>> Alexey
>>
>
Thanks,
Alexey
On 02/12/2018 12:22 PM, Alexey Skidanov wrote:
>
>
> On 02/12/2018 09:52 PM, Laura Abbott wrote:
>> On 02/12/2018 11:11 AM, Alexey Skidanov wrote:
>>>
>>> On 02/12/2018 08:42 PM, Laura Abbott wrote:
>>>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>>>>> Current ion defined allocation ioctl doesn't allow to specify the
>>>>> requested
>>>>> allocation alignment. CMA heap allocates buffers aligned on buffer size
>>>>> page order.
>>>>>
>>>>> Sometimes, the alignment requirement is less restrictive. In such
>>>>> cases,
>>>>> providing specific alignment may reduce the external memory
>>>>> fragmentation
>>>>> and in some cases it may avoid the allocation request failure.
>>>>>
>>>> I really do not want to bring this back as part of the regular
>>>> ABI.
>>> Yes, I know it was removed in 4.12.
>>> Having an alignment parameter that gets used for exactly
>>>> one heap only leads to confusion (which is why it was removed
>>>> from the ABI in the first place).
>>> You are correct regarding the CMA heap. But, probably it may be used by
>>> custom heap as well.
>>
>> I can think of a lot of instances where it could be used but
>> ultimately there needs to be an actual in kernel user who wants
>> it.
>>
>>>> The alignment came from the behavior of the DMA APIs. Do you
>>>> actually need to specify any alignment from userspace or do
>>>> you only need page size?
>>> Yes. If CMA gives it for free, I would suggest to let the ion user to
>>> decide
>>
>> I'm really not convinced changing the ABI yet again just to let
>> the user decide is actually worth it. If we can manage it, I'd
>> much rather see a proposal that doesn't change the ABI.
> I didn't actually change the ABI - I just use the "unused" member:
> struct ion_allocation_data {
> @@ -80,7 +79,7 @@ struct ion_allocation_data {
> __u32 heap_id_mask;
> __u32 flags;
> __u32 fd;
> - __u32 unused;
> + __u32 align;
> };
>
Something that was previously unused is now being used. That's a change
in how the structure is interpreted which is an ABI change, especially
since we haven't been checking that the __unused field is set
to 0.
> As an alternative, I may add __u64 heap_specific_param - but this will
> change the ABI. But, probably it makes the ABI more generic?
Why does the ABI need to be more generic? If we change the ABI
we're stuck with it and I'd like to approach it as the very last
resort.
Thanks,
Laura
On 02/12/2018 10:46 PM, Laura Abbott wrote:
> On 02/12/2018 12:22 PM, Alexey Skidanov wrote:
>>
>>
>> On 02/12/2018 09:52 PM, Laura Abbott wrote:
>>> On 02/12/2018 11:11 AM, Alexey Skidanov wrote:
>>>>
>>>> On 02/12/2018 08:42 PM, Laura Abbott wrote:
>>>>> On 02/10/2018 02:17 AM, Alexey Skidanov wrote:
>>>>>> Current ion defined allocation ioctl doesn't allow to specify the
>>>>>> requested
>>>>>> allocation alignment. CMA heap allocates buffers aligned on buffer
>>>>>> size
>>>>>> page order.
>>>>>>
>>>>>> Sometimes, the alignment requirement is less restrictive. In such
>>>>>> cases,
>>>>>> providing specific alignment may reduce the external memory
>>>>>> fragmentation
>>>>>> and in some cases it may avoid the allocation request failure.
>>>>>>
>>>>> I really do not want to bring this back as part of the regular
>>>>> ABI.
>>>> Yes, I know it was removed in 4.12.
>>>> Having an alignment parameter that gets used for exactly
>>>>> one heap only leads to confusion (which is why it was removed
>>>>> from the ABI in the first place).
>>>> You are correct regarding the CMA heap. But, probably it may be used by
>>>> custom heap as well.
>>>
>>> I can think of a lot of instances where it could be used but
>>> ultimately there needs to be an actual in kernel user who wants
>>> it.
>>>
>>>>> The alignment came from the behavior of the DMA APIs. Do you
>>>>> actually need to specify any alignment from userspace or do
>>>>> you only need page size?
>>>> Yes. If CMA gives it for free, I would suggest to let the ion user to
>>>> decide
>>>
>>> I'm really not convinced changing the ABI yet again just to let
>>> the user decide is actually worth it. If we can manage it, I'd
>>> much rather see a proposal that doesn't change the ABI.
>> I didn't actually change the ABI - I just use the "unused" member:
>> struct ion_allocation_data {
>> @@ -80,7 +79,7 @@ struct ion_allocation_data {
>> __u32 heap_id_mask;
>> __u32 flags;
>> __u32 fd;
>> - __u32 unused;
>> + __u32 align;
>> };
>>
>
> Something that was previously unused is now being used. That's a change
> in how the structure is interpreted which is an ABI change, especially
> since we haven't been checking that the __unused field is set
> to 0.
Yes you are correct. I just realized that it isn't even backward
compatible.
>
>> As an alternative, I may add __u64 heap_specific_param - but this will
>> change the ABI. But, probably it makes the ABI more generic?
>
> Why does the ABI need to be more generic? If we change the ABI
> we're stuck with it and I'd like to approach it as the very last
> resort.
>
> Thanks,
> Laura
It seems, that there is no way to do it without some ABI change?
Thanks,
Alexey