On 11/29/18 8:25 AM, Alexey Skidanov wrote:
>
>
> 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
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
Does it make sense to use unified device properties API
(https://elixir.bootlin.com/linux/v4.20-rc5/source/include/linux/property.h#L1)?
Looks like it's exactly what we are looking for.
Thanks,
Alexey
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
