Re: [RFC][PATCH 1/2] WIP: Devicetree bindings for Ion
From: Andrew
Date: Wed Oct 07 2015 - 06:37:04 EST
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@xxxxxxxxxxxxxxxxx> wrote:
From: Laura Abbott <laura@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
Signed-off-by: Andrew Andrianov <andrew@xxxxxxxxxx>
---
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)
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);
}
...
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@80180000 {
compatible = "acme,h266dec";
reg = <0x80180000 0x20000>,
reg-names = "registers";
interrupts = <12>;
interrupt-parent = <&vic1>;
ion-heaps-for-buffers = <&ion-decoder-region>
};
An alternative might be to have each heap just be a compatible string
and pull everything (id, type etc.) into C files for setup. I debated
doing that but decided to try putting everything in DT for my first
pass.
+ };
+
+ ion-camera-region {
+ linux,ion-heap-id = <1>;
+ linux,ion-heap-type = <ION_DMA_HEAP_TYPE>;
+ linux,ion-heap-name = "camera"
+ memory-region = <&camera_region>;
Couldn't the memory-region node with addition properties or some
standardization of existing ones provide enough information for ION's
needs?
I think we could probably derive most of it from the memory-region
right
now. If it's reusable, it's DMA, if not it goes to a carveout. Name can
come from the node name. heap ID and whether or not a region is a chunk
heap could be added as properties.
We'd still need to be able to get the same information for heaps that
don't correspond to a specific region like the system heap.
+ };
+
+ ion-fb-region {
+ linux,ion-heap-id = <2>;
+ linux,ion-heap-type = <ION_DMA_HEAP_TYPE>;
+ linux,ion-heap-name = "fb"
+ memory-region = <&fb_region>;
+ };
+ }
--
2.4.3
--
Regards,
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/