Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] staging: android: ion: Add requested allocation alignment
To:     Laura Abbott <labbott@redhat.com>, sumit.semwal@linaro.org,
        gregkh@linuxfoundation.org, arve@android.com, tkjos@android.com,
        maco@android.com, devel@driverdev.osuosl.org
Cc:     linux-kernel@vger.kernel.org, Liam Mark <lmark@codeaurora.org>
References: <1518257863-6903-1-git-send-email-alexey.skidanov@intel.com>
 <ae413008-6c8b-5e53-9251-d11f4ae9713a@redhat.com>
From:   Alexey Skidanov <alexey.skidanov@intel.com>
Message-ID: <8284b2ba-a532-23fd-4c52-7ac556d63918@intel.com>
Date:   Mon, 12 Feb 2018 21:11:40 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <ae413008-6c8b-5e53-9251-d11f4ae9713a@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@intel.com>
>> ---
>>   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
>>
>