feat(D1): DMAC Driver 2: Electric Boogaloo #281

hawkw · 2023-09-17T03:32:09Z

This branch kinda rewrites a bunch of the DMAC code. Now, DMA channels
are capable of being dynamically allocated on demand using a cool atomic
bitset thingy, and released back to the DMAC pool when they're no longer
in use. This will allow us to have multiple drivers that consume
basically as many DMA channels as they want to, rather than having a
fixed assignment of DMA channels to drivers which has to be maintained
by manually updating the DMAC IRQ handler every time we add a new driver
that wants to do a DMA. A channel can be allocated from the pool once
and used for multiple transfers, if a driver needs to make a bunch of
transfers in sequence and doesn't want to have to round trip through the
channel pool, but they can also be grabbed for oneshot transfers and
immediately released.

I also fixed the cancel safety of the async Channel::transfer
(formerly Channel::run_descriptor) method, which would previously
leave the transfer running if the future is dropped. This means that if
a caller assumed dropping the transfer future meant that the transfer
descriptor's buffer could be freed, the transfer might complete later
and trample memory that has been reallocated. THAT SEEMS KINDA BAD LOL.
So, now, dropping a Channel::transfer future will cancel the transfer
if it hasn't finished yet, allowing fun stuff like attaching a timeout
to a transfer if you really want to. Of course, the transfer may still
have completed partially, and if we were writing to a device, the device
may be unhappy to have only gotten some of the data it wanted. But, at
least we don't have abandoned DMA transfers running around in random
parts of the heap you probably wanted to use for normal stuff like
having strings, or whatever it is that people do on the computer. So
that's better.

Unfortunately, while we can easily make the DMA transfers Drop-safe,
we can't really make them mem::forget-safe without passing the DMA
buffer into the future and leaking it if it gets forgetted. This is
probably a good idea but would require a nice higher-level interface
around constructing DMA descriptors from buffer-shaped memory regions,
which I didn't want to do tonight. So, the DMA transfer APIs are still
unsafe and maybe we can add a nicer thing on top later. In the
meantime, probably don't mem::forget DMA transfers. Honestly there is
no reason for normal people to do that so I don't feel super bad about
it.

This introduces an atomic allocator for up to 16 unique indices. I need this for the DMAC allocator, which has 16 channels. But, we can also add bigger bitfields later for use in pools of up to 64 (or 128) objects. Probably these could be macro-generated later. Whatever.

This branch kinda rewrites a bunch of the DMAC code. Now, DMA channels are capable of being dynamically allocated on demand using a cool atomic bitset thingy, and released back to the DMAC pool when they're no longer in use. This will allow us to have multiple drivers that consume basically as many DMA channels as they want to, rather than having a fixed assignment of DMA channels to drivers which has to be maintained by manually updating the DMAC IRQ handler every time we add a new driver that wants to do a DMA. A channel can be allocated from the pool once and used for multiple transfers, if a driver needs to make a bunch of transfers in sequence and doesn't want to have to round trip through the channel pool, but they can also be grabbed for oneshot transfers and immediately released. I also fixed the cancel safety of the async `Channel::transfer` (formerly `Channel::run_descriptor`) method, which would previously leave the transfer running if the future is dropped. This means that if a caller assumed dropping the transfer future meant that the transfer descriptor's buffer could be freed, the transfer might complete later and trample memory that has been reallocated. THAT SEEMS KINDA BAD LOL. So, now, dropping a `Channel::transfer` future will cancel the transfer if it hasn't finished yet, allowing fun stuff like attaching a timeout to a transfer if you really want to. Of course, the transfer may still have completed partially, and if we were writing to a device, the device may be unhappy to have only gotten some of the data it wanted. But, at least we don't have abandoned DMA transfers running around in random parts of the heap you probably wanted to use for normal stuff like having strings, or whatever it is that people do on the computer. So that's better. Unfortunately, while we can easily make the DMA transfers `Drop`-safe, we can't really make them `mem::forget`-safe without passing the DMA buffer into the future and leaking it if it gets forgetted. This is probably a good idea but would require a nice higher-level interface around constructing DMA descriptors from buffer-shaped memory regions, which I didn't want to do tonight. So, the DMA transfer APIs are still `unsafe` and maybe we can add a nicer thing on top later. In the meantime, probably don't `mem::forget` DMA transfers. Honestly there is no reason for normal people to do that so I don't feel super bad about it.

hawkw · 2023-09-17T04:45:15Z

platforms/allwinner-d1/src/dmac/mod.rs

+
+        ccu.enable_module(&mut dmac);
+
+        // enable the queue IRQ for all the channels, because use the other


Suggested change

// enable the queue IRQ for all the channels, because use the other

// enable the queue IRQ for all the channels, because we don't use the other

now it's much harder to violate the invariants around "don't start an xfer while one is already running" and register block access. if we need these to be public later, we can always make them public again...

because why not!

jamesmunns

This is hella hella cool.

I also really like that you can still reserve and take a DMA channel and re-use it, which addressed my one potential concern of "how do we handle 'high prio' dma cases"?

Looks like we're all the way ready for async fn copy(src: &[u8], dst: &mut [u8]) now :)

platforms/allwinner-d1/src/dmac/mod.rs

jamesmunns · 2023-09-17T16:28:48Z

platforms/allwinner-d1/src/dmac/mod.rs

+    /// probably wanted to use for normal stuff like having strings, or whatever
+    /// it is that people do on the computer.
+    ///
+    /// If the DMA transfer was a read rather than a write, cancelling a partial


I'd say this isn't TOTALLY true, depending on how broad you are talking about, reads can still have side effects in external hardware.

yeah, good point

source/bitslab/src/index.rs

Co-authored-by: James Munns <james@onevariable.com>

hawkw added 2 commits September 16, 2023 18:58

hawkw added kind: enhancement New feature or request platform: D1 Specific to the Allwinner D1 hardware platform area: drivers Related to driver implementation for hardware devices. labels Sep 17, 2023

hawkw requested a review from jamesmunns September 17, 2023 03:32

hawkw self-assigned this Sep 17, 2023

hawkw marked this pull request as ready for review September 17, 2023 03:32

endocumentify some stuff

66c4f64

hawkw force-pushed the eliza/dmac-allocator-2 branch from 3818b7f to 66c4f64 Compare September 17, 2023 04:04

hawkw commented Sep 17, 2023

View reviewed changes

hawkw added 5 commits September 17, 2023 09:09

more documentationeering

c0c3683

put public APIs first

df80ed0

make internal DMAC APIs private

78b69ca

now it's much harder to violate the invariants around "don't start an xfer while one is already running" and register block access. if we need these to be public later, we can always make them public again...

add fmt::Debug impls

41b8cc9

because why not!

misc internal refactors/cleanup

646c8ca

jamesmunns approved these changes Sep 17, 2023

View reviewed changes

hawkw and others added 3 commits September 17, 2023 09:37

Update platforms/allwinner-d1/src/dmac/mod.rs

9e89306

Co-authored-by: James Munns <james@onevariable.com>

feat(bitslab): method name feedback + more utils

cfaa12a

don't state that cancelled reads are "always fine"

5be0b27

hawkw merged commit c4db424 into main Sep 17, 2023
10 checks passed

hawkw deleted the eliza/dmac-allocator-2 branch September 17, 2023 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(D1): DMAC Driver 2: Electric Boogaloo #281

feat(D1): DMAC Driver 2: Electric Boogaloo #281

hawkw commented Sep 17, 2023

hawkw Sep 17, 2023

jamesmunns left a comment

jamesmunns Sep 17, 2023

hawkw Sep 17, 2023


		ccu.enable_module(&mut dmac);

		// enable the queue IRQ for all the channels, because use the other

feat(D1): DMAC Driver 2: Electric Boogaloo #281

feat(D1): DMAC Driver 2: Electric Boogaloo #281

Conversation

hawkw commented Sep 17, 2023

hawkw Sep 17, 2023

Choose a reason for hiding this comment

jamesmunns left a comment

Choose a reason for hiding this comment

jamesmunns Sep 17, 2023

Choose a reason for hiding this comment

hawkw Sep 17, 2023

Choose a reason for hiding this comment