Re: [PATCH] staging: android: ion: Add requested allocation alignment

[Alexey Skidanov]

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 <alexey.skidanov@xxxxxxxxx>
>> ---
>> Â 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
>>
>