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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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?

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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux