ISP (work in progress) #319

eiln · 2023-08-14T17:56:52Z

No description provided.

povik · 2023-08-17T12:29:01Z

src/dapf.c

+    {"/arm-io/dart-aop", 1},
+    {"/arm-io/dart-mtp", 1},
+    {"/arm-io/dart-pmp", 1},
+    {"/arm-io/dart-isp", 5},


We should consider deriving the register range index from ADT. I think it was the instance property that held that info? Would that work here?

instance are names of each dapf entry (DAPF, SMMU, TLL, DART). I couldn't find another prop either. But for all of my test cases (t8103 ane/aop/isp; mtp/pmp gets dropped anyway) it's the last range index, which we could get from reg_len / ((a_cells + s_cells) * 4). If this holds up I can revert this & add adt_get_reg_count() instead.

Also enable power if domain exists. Signed-off-by: Eileen Yoon <[email protected]>

Signed-off-by: Eileen Yoon <[email protected]>

Handle /arm-io/isp0 and diff CTRR writes. Signed-off-by: Eileen Yoon <[email protected]>

Signed-off-by: Eileen Yoon <[email protected]>

marcan · 2023-09-07T10:27:50Z

src/isp.c

+
+/* ISP DART has some quirks we must work around */
+
+#define DART_T8020_ENABLED_STREAMS 0xfc


Why do we need to do all this DART stuff? Linux should be in charge of DARTs, and this is unlikely to work on M2+ as they use a different DART type. dart,t8110 uses very different registers, I don't think it's going to work with the stream selects for dart,t8020.

We've generally gotten away without these DART tunables, just by describing things properly in the DT. Do we know what they do? In particular the TCRs should be managed by Linux, so it shouldn't be necessary to poke them. If we need to apply the tunables in m1n1 due to "fun special stuff" that's fine, but we should try to figure out the minimum viable init sequence.

This should also probably be in common DART code, not specific to ISP, since as far as I know it's all part of the DART nodes anyway. There's lots of regs/etc defined in dart.c you can use. If we only need it for ISP we'll only call it for ISP, but it's common support code after all.

For normal DARTs, its register range count is 1 or 2, with the optional second range specifying DAPF. Walking ADT, the DART nodes with a reg count > 2:

dart-isp 6, dart-ane 4, dart-ave 4

Conveniently, both the ANE & AVE are derivatives of ISP. I also briefly looked at AVE for this exact reason. All three have the same DMA setup, involving an n-number of "fake" dart domains referenced by its periperal DMA channels. For the ISP, the (actual) dart is index 0, the next 2 are the fake/reference darts, and the last range is DAPF. ANE is n=2, AVE is n=1. These ranges look like real darts, and when poked, the registers somewhat behave like a real one, but they cannot substitute the main dart nor provide a new address space.

Instead, certain values must be manually "synced" to the main domain for DMA to work, notably TTBR and TLB invalidation. E.g. without syncing TTBR, the AVE/ISP firmware boots fine (ASC PTE is ok) but will fail when asked to do work (for the ISP, frames are dropped); the ANE specifically cannot DMA to/from L2 without TTBR synced; its internal command DMA channel works fine. Also, both ANE/ISP cannot reuse pages without also calling TLB invalidation on the fake domains beforehand (DART IRQ gets stuck). TTBR we can call once after init (though not quite a tunable because the main TTBR isn't initialized yet), but TLB invalidation must follow the main dart call every time, so that's really not a tunable.

Currently isp-iommu.c hacks around this, mainly because I can and without bothering sven (though I'm the only one outside of iommu calling iommu_flush_iotlb_all()), but clearly we need changes to core DART code. Especially if we're gonna use it again ;)

marcan · 2023-09-07T10:29:52Z

src/kboot.c

+    if (fdt_node < 0)
+        bail("FDT: '%s' node not found\n", path);
+
+    if (fdt_appendprop_u64(dt, fdt_node, "apple,isp-heap-base", heap_base))


Does this really need to be fixed here? It kind of sounds like the heap memory is supposed to be dynamically allocated depending on what the firmware requests, or some other metadata somewhere? If it's not in Apple's DT then it has to come from somewhere else...

It was once revealed to me in a dream that the firmware sizes are hardcoded in their kext. It also varies per device on the same the OS version (there's a firmware for each device). I'm pretty sure ah-'s t6000 issue was this.

marcan · 2023-09-07T10:31:21Z

src/kboot.c

+
+    int count = segments_len / sizeof(*segments);
+    for (int i = 0; i < count; i++)
+        segments[i].remap = segments[i].iova; // match sio segment-ranges


This is mutating the ADT in-place, that's not a good idea.

It is mutated to match the sio segment-ranges struct (for dt_reserve_asc_firmware()). Should I not mutate in-place (call adt_setprop()with full struct) or not mutate at all (fix dt_reserve_asc_firmware() to accept an index?)

marcan · 2023-09-07T10:33:09Z

src/kboot.c

+            bail("FDT: couldn't get ISP CTRR size (%d)\n", os_firmware.version);
+    }
+
+    u64 heap_base = segments[count - 1].iova + segments[count - 1].size;


Is this right? Taking an iova, adding a size, yields an iova... then ctrr_size minus that? So the base iova is effectively 0? But then calling this size CTRR makes no sense, CTRR is a read-only region that should cover the firmware text and rodata regions only, definitely not a heap after that. It kind of sounds like what you call "ctrr_size" is more like the entire ISP low iova area?

AppleH13CamIn::mapFwCTRRRegion - FW __TEXT segment phy: 0x10000a58000, virt: 0x0, remap: 0x10000a58000, size: 0x980000 AppleH13CamIn::mapFwCTRRRegion - FW __DATA segment phy: 0x10001b60000, virt: 0x980000, remap: 0x1f000980000, size: 0x324000 AppleH13CamIn::mapFwCTRRRegion - physical start: 0xca4000, heap size: 0x25c000 AppleH13CamIn::start - CTRR firmware is loaded, size: 0xf00000

The "heap" is where the firmware code executes. The problem is that there isn't a mechanism to separate the address space. We can set DMA dst to the heap all we want. XNU has some (kernel-level) page protections. Also the "physical start" is a typo, as the heap is allocated at probe and can be discontiguous.

Signed-off-by: Hector Martin <[email protected]>

The iova field is different, handle it with a flag. Signed-off-by: Hector Martin <[email protected]>

For ISP stuff which is not declared in the ADT Signed-off-by: Hector Martin <[email protected]>

Allocating this in m1n1 means we can treat it as a reserved-memory node and deal with knowing the size in m1n1, instead of the kernel. It also means we can deal with the firmware version dependency mess here. Signed-off-by: Hector Martin <[email protected]>

Signed-off-by: Hector Martin <[email protected]>

This means we can use it with pre-created nodes in the DT. Signed-off-by: Hector Martin <[email protected]>

So of_iommu can detect it. Signed-off-by: Eileen Yoon <[email protected]>

Signed-off-by: Eileen Yoon <[email protected]>

Still needed to fill out the fw bootargs. Signed-off-by: Eileen Yoon <[email protected]>

jannau · 2023-10-01T15:24:29Z

src/kboot.c

@@ -2052,6 +2113,9 @@ int kboot_prepare_dt(void *fdt)
        dt = NULL;
    }

+    /* Need to init ISP early to carve out heap */
+    isp_init();


this either needs if (fdt_get_alias(dt, "isp")) or isp_init should silently fail if the ADT misses isp/isp0

povik reviewed Aug 17, 2023

View reviewed changes

eiln force-pushed the isp-dapf branch from 0ddcc4b to 912691e Compare August 18, 2023 18:13

eiln added 2 commits August 23, 2023 04:46

dapf: Support reg indexing + add ISP

9ef073a

Also enable power if domain exists. Signed-off-by: Eileen Yoon <[email protected]>

kboot: Reserve ISP firmware

336dfb0

Signed-off-by: Eileen Yoon <[email protected]>

eiln force-pushed the isp-dapf branch from 6a72bf2 to 336dfb0 Compare August 22, 2023 19:48

eiln added 2 commits August 23, 2023 04:53

isp: tmp - Add t8020 ISP DART workaround

0098f4a

Signed-off-by: Eileen Yoon <[email protected]>

isp/kboot: Prelim T6000 support

d8cecca

Handle /arm-io/isp0 and diff CTRR writes. Signed-off-by: Eileen Yoon <[email protected]>

eiln force-pushed the isp-dapf branch from 6a268da to d8cecca Compare September 2, 2023 04:15

eiln added 2 commits September 2, 2023 22:45

isp: t6000 13c index 0 flag

caa4021

Signed-off-by: Eileen Yoon <[email protected]>

isp: t6000 stream select mask

f81b2c3

Signed-off-by: Eileen Yoon <[email protected]>

marcan requested changes Sep 7, 2023

View reviewed changes

eiln force-pushed the isp-dapf branch from 1cc6aa1 to 11c9c71 Compare September 10, 2023 11:25

marcan and others added 9 commits September 10, 2023 20:27

kboot: Use adt_segment_ranges struct instead of open-coding

a3cb742

Signed-off-by: Hector Martin <[email protected]>

kboot: Handle ISP/SIO reserved firmware differences

e709f9a

The iova field is different, handle it with a flag. Signed-off-by: Hector Martin <[email protected]>

pmgr: Expose pmgr_set_mode

ca3ca11

For ISP stuff which is not declared in the ADT Signed-off-by: Hector Martin <[email protected]>

isp: Allocate heap carveout

3b549bd

Allocating this in m1n1 means we can treat it as a reserved-memory node and deal with knowing the size in m1n1, instead of the kernel. It also means we can deal with the firmware version dependency mess here. Signed-off-by: Hector Martin <[email protected]>

kboot: Handle ISP heap carveout

bb2d532

Signed-off-by: Hector Martin <[email protected]>

kboot: Make dt_get_or_add_reserved_mem mutate existing nodes

067f61b

This means we can use it with pre-created nodes in the DT. Signed-off-by: Hector Martin <[email protected]>

kboot: Add isp-heap mem region to phandle

bf07bd0

So of_iommu can detect it. Signed-off-by: Eileen Yoon <[email protected]>

kboot: Remove old ISP segment-ranges struct

4511899

Signed-off-by: Eileen Yoon <[email protected]>

kboot/isp: Pass ctrr_size to FDT

b5c2fcb

Still needed to fill out the fw bootargs. Signed-off-by: Eileen Yoon <[email protected]>

eiln force-pushed the isp-dapf branch from 11c9c71 to b5c2fcb Compare September 10, 2023 11:28

jannau reviewed Oct 1, 2023

View reviewed changes

marcan force-pushed the main branch from 91442e2 to 5cecf53 Compare November 2, 2023 09:13

marcan force-pushed the main branch 3 times, most recently from 4f95305 to 95d67cf Compare April 9, 2024 03:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISP (work in progress) #319

ISP (work in progress) #319

eiln commented Aug 14, 2023

povik Aug 17, 2023

eiln Aug 17, 2023

marcan Sep 7, 2023

eiln Sep 7, 2023

marcan Sep 7, 2023

eiln Sep 7, 2023

marcan Sep 7, 2023

eiln Sep 7, 2023

marcan Sep 7, 2023

eiln Sep 7, 2023

jannau Oct 1, 2023


		/* ISP DART has some quirks we must work around */

		#define DART_T8020_ENABLED_STREAMS 0xfc

ISP (work in progress) #319

Are you sure you want to change the base?

ISP (work in progress) #319

Conversation

eiln commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment