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? 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