Date:	Wed, 07 Oct 2015 22:23:46 +0300
From:	Andrew <andrew@...mnt.org>
To:	Rob Herring <robherring2@...il.com>
Cc:	Laura Abbott <laura@...bott.name>,
	Laura Abbott <labbott@...oraproject.org>,
	Rob Herring <robh+dt@...nel.org>,
	Frank Rowand <frowand.list@...il.com>,
	Sumit Semwal <sumit.semwal@...aro.org>, arve@...roid.com,
	Riley Andrews <riandrews@...roid.com>,
	John Stultz <john.stultz@...aro.org>,
	Grant Likely <grant.likely@...aro.org>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Tom Gall <tom.gall@...aro.org>,
	Colin Cross <ccross@...gle.com>, devel@...verdev.osuosl.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	romlem@...gle.com, Mitchel Humpherys <mitchelh@...eaurora.org>,
	linux-arm-kernel@...ts.infradead.org,
	Feng Tang <feng.tang@...el.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	a.makarov@...ule.ru, a.bogdanova@...ule.ru
Subject: Re: [RFC][PATCH 1/2] WIP: Devicetree bindings for Ion

On 2015-10-07 21:36, Rob Herring wrote:
> On Wed, Oct 7, 2015 at 5:36 AM, Andrew <andrew@...mnt.org> wrote:
>> On 2015-10-07 02:01, Laura Abbott wrote:
>>> 
>>> On 10/6/15 3:35 PM, Rob Herring wrote:
>>>> 
>>>> On Tue, Oct 6, 2015 at 3:47 PM, Laura Abbott 
>>>> <labbott@...oraproject.org>
>>>> wrote:
>>>>> 
>>>>> From: Laura Abbott <laura@...bott.name>
>>>>> 
>>>>> 
>>>>> This adds a base set of devicetree bindings for the Ion memory
>>>>> manager. This supports setting up the generic set of heaps and
>>>>> their properties.
>>>>> 
>>>>> Signed-off-by: Laura Abbott <laura@...bott.name>
>>>>> Signed-off-by: Andrew Andrianov <andrew@...mnt.org>
>>>>> ---
>>>>>   drivers/staging/android/ion/devicetree.txt | 53
>>>>> ++++++++++++++++++++++++++++++
>>>> 
>>>> 
>>>> I have no issue with this going in here, but I do have lots of 
>>>> issues
>>>> with this binding.
>>>> 
>>>>>   1 file changed, 53 insertions(+)
>>>>>   create mode 100644 drivers/staging/android/ion/devicetree.txt
>>>>> 
>>>>> diff --git a/drivers/staging/android/ion/devicetree.txt
>>>>> b/drivers/staging/android/ion/devicetree.txt
>>>>> new file mode 100644
>>>>> index 0000000..4a0c941
>>>>> --- /dev/null
>>>>> +++ b/drivers/staging/android/ion/devicetree.txt
>>>>> @@ -0,0 +1,53 @@
>>>>> +Ion Memory Manager
>>>>> +
>>>>> +Ion is a memory manager that allows for sharing of buffers via 
>>>>> dma-buf.
>>>>> +Ion allows for different types of allocation via an abstraction 
>>>>> called
>>>>> +a 'heap'. A heap represents a specific type of memory. Each heap 
>>>>> has
>>>>> +a different type. There can be multiple instances of the same heap
>>>>> +type.
>>>>> +
>>>>> +Required properties for Ion
>>>>> +
>>>>> +- compatible: "linux,ion"
>>>>> +
>>>>> +All child nodes of a linux,ion node are interpreted as heaps
>>>>> +
>>>>> +required properties for heaps
>>>>> +
>>>>> +- linux,ion-heap-id: The Ion heap id used for allocation selection
>>>>> +- linux,ion-heap-type: Ion heap type defined in ion.h
>>>>> +- linux,ion-heap-name: Human readble name of the heap
>>>>> +
>>>>> +
>>>>> +Optional properties
>>>>> +- memory-region: A phandle to a memory region. Required for DMA 
>>>>> heap
>>>>> type
>>>>> +(see reserved-memory.txt for details on the reservation)
>>>>> +- linux,ion-heap-align: Alignment for the heap.
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +       ion {
>>>>> +               compatbile = "linux,ion";
>>>>> +               #address-cells = <1>;
>>>>> +               #size-cells = <0>;
>>>>> +
>>>>> +               ion-system-heap {
>>>>> +                       linux,ion-heap-id = <0>;
>>>>> +                       linux,ion-heap-type = 
>>>>> <ION_SYSTEM_HEAP_TYPE>;
>>>>> +                       linux,ion-heap-name = "system";
>>>> 
>>>> 
>>>> How does this vary across platforms? Is all of this being pushed 
>>>> down
>>>> to DT, because there is no coordination of this at the kernel ABI
>>>> level across platforms. In other words, why can't heap 0 be 
>>>> hardcoded
>>>> as system heap in the driver. It seems to me any 1 of these 3
>>>> properties could be used to derive the other 2.
>>>> 
>>> 
>>> Right now there is no guarantee heap IDs will be the same across
>>> platforms. The heap IDs are currently part of the userspace ABI
>>> as well since userspace clients must pass in a mask of the heap
>>> IDs to allocate from. If we assume all existing clients could change,
>>> heaps such as the system heap could be mandated to have the same
>>> heap ID but we'd still run into problems if you have multiple
>>> heaps of the same type (e.g. multiple carveouts)
> 
> Vendors largely ignore the kernel-userspace ABI and anything in
> staging is not a ABI. So arguments about what the ABI is currently is
> pointless IMO.
> 
> Pushing an inconsistent kernel ABI to DT is not the answer.

Totally agree here.

> 
>> 
>> I don't really like the idea of enforcing any IDs here. As of now
>> heap ids are generally something VERY platform-specific
>> (or even product-specific). Personally I'd prefer something like this
>> for userspace apps:
>> 
>> int id1 = ion_get_heap_id("camera");
>> if (id1 < 0) {
>>       fprintf(stderr, "Invalid heap id");
>>       exit(1);
>> }
>> 
>> int id2 = ion_get_heap_id("backup-heap");
>> if (id2 < 0) {
>>       fprintf(stderr, "Invalid heap id");
>>       exit(1);
>> }
> 
> We've learned that creating number spaces like this are bad (irqs,
> gpios, /dev nodes). We should move away from that. Why should
> userspace care about IDs or what the IDs are? An ID is just encoding
> certain implicit requirements. So are the strings here. Users should
> express what capabilities, restrictions, etc. they have, and then the
> kernel can find the best heap.

I'd argue about that point, since sometimes kernel might NOT know
all the hardware details behind some of the heaps or how they are going 
to
be used. Hence it can't decide a proper heap. And that's where things 
get ugly.

Real-world example: There are several on-chip SRAM banks that make up 
several
heaps. Say, IM0, IM1, IM2.

Technically - they are all DMA, and all work. But the hardware guys hand 
you
a handful of weird recommendations, like:
* Decoder will work faster if you use bank IM2 for internal buffers,
and prefer DDR bank A for decoded frames.
* DSP should stick with IM1, and please prefer DDR bank B for buffers

When several such devices are involved in one chain - things may get
even weirder. Having manual control over where allocations are made 
allows
us to keep all these voodoo magicks away from the kernel and (hopefully) 
keep
vendors from dirty hacks into ion itself.

> 
>> ...
>> 
>> int ret = ion_alloc(fd, 0x100, 0x4,
>>               (id1 | id2),
>>               0, &handle);
>> 
>> 
>> What concerns kernel stuff, things are simpler - we may just pass the 
>> heap
>> to use
>> by a reference in devicetree node for that driver. Something like 
>> that:
>> 
>> ...
>>                ion-decoder-region : region_ddr {
>>                        linux,ion-heap-id = <1>;
>>                        linux,ion-heap-type = <ION_DMA_HEAP_TYPE>;
>>                        linux,ion-heap-name = "decoder_mem"
>>                        memory-region = <&camera_region>;
>>                 };
>> ...
>>                 video_decoder@...80000 {
>>                         compatible = "acme,h266dec";
>>                         reg = <0x80180000 0x20000>,
>>                         reg-names = "registers";
>>                         interrupts = <12>;
>>                         interrupt-parent = <&vic1>;
>>                         ion-heaps-for-buffers = <&ion-decoder-region>
> 
> This is how memory-region is supposed to work. I don't see why we need
> an additional level of indirection.

I don't see right now a clean way of getting ion heap id from driver 
when
having a memory-region referenced there. Correct me if if I'm all wrong 
about it.

> 
> Rob

-- 
Regards,
Andrew
RC Module
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists