[PATCH] staging: android: ion: Add chunk heap initialization

Laura Abbott labbott at redhat.com
Thu Nov 29 01:30:29 UTC 2018


On 11/27/18 12:07 PM, Alexey Skidanov wrote:
> 
> 
> On 11/27/18 9:20 PM, Laura Abbott wrote:
>> On 11/26/18 10:43 AM, Alexey Skidanov wrote:
>>>
>>>
>>> On 11/26/18 6:39 PM, Laura Abbott wrote:
>>>> On 11/25/18 2:02 PM, Alexey Skidanov wrote:
>>>>>
>>>>>
>>>>> On 11/25/18 11:40 PM, Laura Abbott wrote:
>>>>>> On 11/25/18 1:22 PM, Alexey Skidanov wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 11/25/18 10:51 PM, Laura Abbott wrote:
>>>>>>>> On 11/11/18 11:29 AM, Alexey Skidanov wrote:
>>>>>>>>> Create chunk heap of specified size and base address by adding
>>>>>>>>> "ion_chunk_heap=size at start" kernel boot parameter.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alexey Skidanov <alexey.skidanov at intel.com>
>>>>>>>>> ---
>>>>>>>>>       drivers/staging/android/ion/ion_chunk_heap.c | 40
>>>>>>>>> ++++++++++++++++++++++++++++
>>>>>>>>>       1 file changed, 40 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/staging/android/ion/ion_chunk_heap.c
>>>>>>>>> b/drivers/staging/android/ion/ion_chunk_heap.c
>>>>>>>>> index 159d72f..67573aa4 100644
>>>>>>>>> --- a/drivers/staging/android/ion/ion_chunk_heap.c
>>>>>>>>> +++ b/drivers/staging/android/ion/ion_chunk_heap.c
>>>>>>>>> @@ -135,6 +135,7 @@ struct ion_heap *ion_chunk_heap_create(struct
>>>>>>>>> ion_platform_heap *heap_data)
>>>>>>>>>           }
>>>>>>>>>           chunk_heap->base = heap_data->base;
>>>>>>>>>           chunk_heap->size = heap_data->size;
>>>>>>>>> +    chunk_heap->heap.name = heap_data->name;
>>>>>>>>>           chunk_heap->allocated = 0;
>>>>>>>>>             gen_pool_add(chunk_heap->pool, chunk_heap->base,
>>>>>>>>> heap_data->size, -1);
>>>>>>>>> @@ -151,3 +152,42 @@ struct ion_heap *ion_chunk_heap_create(struct
>>>>>>>>> ion_platform_heap *heap_data)
>>>>>>>>>           return ERR_PTR(ret);
>>>>>>>>>       }
>>>>>>>>>       +static u64 base;
>>>>>>>>> +static u64 size;
>>>>>>>>> +
>>>>>>>>> +static int __init setup_heap(char *param)
>>>>>>>>> +{
>>>>>>>>> +    char *p, *pp;
>>>>>>>>> +
>>>>>>>>> +    size = memparse(param, &p);
>>>>>>>>> +    if (param == p)
>>>>>>>>> +        return -EINVAL;
>>>>>>>>> +
>>>>>>>>> +    if (*p == '@')
>>>>>>>>> +        base = memparse(p + 1, &pp);
>>>>>>>>> +    else
>>>>>>>>> +        return -EINVAL;
>>>>>>>>> +
>>>>>>>>> +    if (p == pp)
>>>>>>>>> +        return -EINVAL;
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +__setup("ion_chunk_heap=", setup_heap);
>>>>>>>>> +
>>>>>>>>> +static int ion_add_chunk_heap(void)
>>>>>>>>> +{
>>>>>>>>> +    struct ion_heap *heap;
>>>>>>>>> +    struct ion_platform_heap plat_heap = {.base = base,
>>>>>>>>> +                          .size = size,
>>>>>>>>> +                          .name = "chunk_heap",
>>>>>>>>> +                          .priv = (void *)PAGE_SIZE};
>>>>>>>>> +    heap = ion_chunk_heap_create(&plat_heap);
>>>>>>>>> +    if (heap)
>>>>>>>>> +        ion_device_add_heap(heap);
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>> +device_initcall(ion_add_chunk_heap);
>>>>>>>>> +
>>>>>>>>>
>>>>>>>>
>>>>>>>> This solves a problem but not enough of the problem.
>>>>>>>>
>>>>>>>> We need to be able to support more than one chunk/carveout
>>>>>>>> heap.
>>>>>>> This is easy to support.
>>>>>>> This also assumes that the memory has already been
>>>>>>>> reserved/placed and that you know the base and size to
>>>>>>>> pass on the command line. Part of the issue with the carveout
>>>>>>>> heaps is that we need a way to tell the kernel to reserve
>>>>>>>> the memory early enough and then get that information to
>>>>>>>> Ion. Hard coding memory locations tends to be buggy from
>>>>>>>> my past experience with Ion.
>>>>>>> memmap= kernel option marks the memory region(s) as reserved (Zone
>>>>>>> Allocator doesn't use this memory region(s)). So the heap(s) may
>>>>>>> manage
>>>>>>> this memory region(s).
>>>>>>
>>>>>> memmap= is x86 only. I really don't like using the command line for
>>>>>> specifying the base/size as it seems likely to conflict with platforms
>>>>>> that rely on devicetree for reserving memory regions.
>>>>>>
>>>>>> Thanks,
>>>>>> Laura
>>>>>>
>>>>> I see ... So probably the better way is the one similar to this
>>>>> https://elixir.bootlin.com/linux/latest/source/kernel/dma/contiguous.c#L245
>>>>>
>>>>>
>>>>> ?
>>>>>
>>>>
>>>> Correct. For platforms that need devicetree, we need a way to specify
>>>> that a region should become an Ion heap. I went through a similar
>>>> exercise with CMA heaps before I kind of gave up on figuring out a
>>>> binding and just had Ion enumerate all CMA heaps. We do still need
>>>> a solution to work with non-DT platforms as well so whatever we
>>>> come up with needs to plausibly work for both cases. Your solution
>>>> would cover the non-DT case but I'd really like to make sure we
>>>> at least have a path forward for the devicetree case as well.
>>>
>>> I would say that we have the following steps to consider:
>>>
>>> 1. Memory reservation. The suggested solution doesn't care how it's done.
>>>
>>> 2. Per-heap information passing to the Kernel. It's different for DT and
>>> non-DT cases.
>>>
>>> 3. Heap objects instantiation. The DT and non-DT cases have different
>>> ways/formats to pass this per-heap information. But once the parsing is
>>> done, the rest of the code is common.
>>>
>>> I think it clearly defines the steps covering both cases. What do you
>>> think?
>>>
>>
>> Yes, that sounds about right.
>>
> 
> So, in this patch step #2 - is setup_heap() and step #3 - is
> ion_add_chunk_heap(). The only missing part is to support several heap
> instances creation, correct?
> 

Mostly yes. I'd like to see struct ion_platform_heap go away since
it really isn't used for anything else but we need another
way to get the reserved memory information into Ion.

Thanks,
Laura

> Thanks,
> Alexey
> 
> 
>>> Thanks,
>>> Alexey
>>>>
>>>> Thanks,
>>>> Laura
>>>>
>>>>> Thanks,
>>>>> Alexey
>>>>>
>>>>>>>>
>>>>>>>> If you'd like to see about coming up with a complete solution,
>>>>>>>> feel free to resubmit but I'm still strongly considering
>>>>>>>> removing these heaps.
>>>>>>>>
>>>>>>> I will add the multiple heaps support and resubmit the patch
>>>>>>>> Thanks,
>>>>>>>> Laura
>>>>>>> Thanks,
>>>>>>> Alexey
>>>>>>>
>>>>>>
>>>>
>>



More information about the devel mailing list