On 11/29/18 3:30 AM, Laura Abbott wrote:
> 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@start" kernel boot parameter.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Alexey Skidanov <alexey.skidanov@xxxxxxxxx>
>>>>>>>>>> ---
>>>>>>>>>> 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
>
Excellent. I will prepare v2 of the patch and submit it for review.
Thanks,
Alexey
>> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
