-
Notifications
You must be signed in to change notification settings - Fork 6
gpdma: add GPDMA driver #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
f5cb2a7
gpdma: add GPDMA driver
astapleton c65d457
Fixes, simpler dma example
astapleton 69eab2a
more fixes
astapleton 5d9fc22
missed some defmt derives
astapleton 6f3aa81
use ReadBuffer, WriteBuffer to force immovable buffers; fix example
astapleton 95e3f78
fix example again
astapleton 793a9d5
doc clarification
astapleton 336a8c4
Use mutable reference to channels when creating transfers
astapleton 9bbaca7
fix data width check; remove incomplete documentation
astapleton 5c587f3
Hold onto source/destination; Remove UB from example
astapleton cd32400
mute type complexity clippy
astapleton 5b0d9be
rename fields
astapleton 100bf1b
Add free method, don't consume self in abort/wait_for_transfer_complete
astapleton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
// #![deny(warnings)] | ||
#![no_main] | ||
#![no_std] | ||
|
||
mod utilities; | ||
|
||
use cortex_m::singleton; | ||
use cortex_m_rt::entry; | ||
use cortex_m_semihosting::debug; | ||
use stm32h5xx_hal::{ | ||
gpdma::{config::transform::*, DmaConfig, DmaTransfer}, | ||
pac, | ||
prelude::*, | ||
}; | ||
|
||
#[entry] | ||
fn main() -> ! { | ||
utilities::logger::init(); | ||
|
||
let dp = pac::Peripherals::take().unwrap(); | ||
|
||
let pwr = dp.PWR.constrain(); | ||
let pwrcfg = pwr.vos0().freeze(); | ||
|
||
// Constrain and Freeze clock | ||
let rcc = dp.RCC.constrain(); | ||
let ccdr = rcc.sys_ck(250.MHz()).freeze(pwrcfg, &dp.SBS); | ||
|
||
let channels = dp.GPDMA1.channels(ccdr.peripheral.GPDMA1); | ||
|
||
log::info!("u8 to u8"); | ||
let src = | ||
singleton!(: [u8; 40] = core::array::from_fn(|i| i as u8)).unwrap(); | ||
let dest = singleton!(: [u8; 40] = [0u8; 40]).unwrap(); | ||
let source_copy = unsafe { &*(src.as_ptr() as *const [u8; 40]) }; | ||
let dest_copy = unsafe { &*(dest.as_ptr() as *const [u8; 40]) }; | ||
|
||
let channel = channels.0; | ||
let config = DmaConfig::new(); | ||
let transfer = DmaTransfer::memory_to_memory(config, &channel, src, dest); | ||
transfer.start().unwrap(); | ||
transfer.wait_for_transfer_complete().unwrap(); | ||
assert_eq!(source_copy, dest_copy); | ||
|
||
log::info!("u32 to u32 with data transform"); | ||
let src = singleton!(: [u32; 10] = [0x12345678u32; 10]).unwrap(); | ||
let dest = singleton!(: [u32; 10] = [0u32; 10]).unwrap(); | ||
|
||
let dest_copy = unsafe { &*(dest.as_ptr() as *const [u32; 10]) }; | ||
|
||
let config = DmaConfig::new().with_data_transform( | ||
DataTransform::builder() | ||
.swap_destination_half_words() | ||
.swap_destination_half_word_byte_order(), | ||
); | ||
|
||
let transfer = DmaTransfer::memory_to_memory(config, &channel, src, dest); | ||
|
||
transfer.start().unwrap(); | ||
transfer.wait_for_transfer_complete().unwrap(); | ||
let expected = [0x78563412; 10]; | ||
assert_eq!(expected, *dest_copy); | ||
|
||
log::info!("u32 to u16 with truncate"); | ||
let src = singleton!(: [u32; 10] = [0x12345678u32; 10]).unwrap(); | ||
let dest = singleton!(: [u16; 20] = [0u16; 20]).unwrap(); | ||
let dest_copy = unsafe { &*(dest.as_ptr() as *const [u16; 20]) }; | ||
let config = DmaConfig::new().with_data_transform( | ||
DataTransform::builder().left_align_right_truncate(), | ||
); | ||
let transfer = DmaTransfer::memory_to_memory(config, &channel, src, dest); | ||
|
||
transfer.start().unwrap(); | ||
transfer.wait_for_transfer_complete().unwrap(); | ||
let expected = [0x1234; 10]; | ||
assert_eq!(expected, (*dest_copy)[0..10]); | ||
|
||
log::info!("u32 to u8 with unpack"); | ||
let src = singleton!(: [u32; 10] = [0x12345678u32; 10]).unwrap(); | ||
let dest = singleton!(: [u8; 40] = [0u8; 40]).unwrap(); | ||
|
||
let dest_copy = unsafe { &*(dest.as_ptr() as *const [u8; 40]) }; | ||
|
||
let config = | ||
DmaConfig::new().with_data_transform(DataTransform::builder().unpack()); | ||
let transfer = DmaTransfer::memory_to_memory(config, &channel, src, dest); | ||
|
||
transfer.start().unwrap(); | ||
transfer.wait_for_transfer_complete().unwrap(); | ||
let expected = [0x78, 0x56, 0x34, 0x12]; | ||
assert_eq!(expected, (*dest_copy)[0..4]); | ||
assert_eq!(expected, (*dest_copy)[36..40]); | ||
|
||
log::info!("u8 to u32 with pack"); | ||
let src = singleton!(: [u8; 40] = [0u8; 40]).unwrap(); | ||
let dest = singleton!(: [u32; 10] = [0u32; 10]).unwrap(); | ||
|
||
for chunk in src.chunks_mut(4) { | ||
chunk.copy_from_slice(&[0x78, 0x56, 0x34, 0x12]); | ||
} | ||
|
||
let dest_copy = unsafe { &*(dest.as_ptr() as *const [u32; 10]) }; | ||
|
||
let config = | ||
DmaConfig::new().with_data_transform(DataTransform::builder().pack()); | ||
let transfer = DmaTransfer::memory_to_memory(config, &channel, src, dest); | ||
|
||
transfer.start().unwrap(); | ||
transfer.wait_for_transfer_complete().unwrap(); | ||
let expected = [0x12345678; 10]; | ||
assert_eq!(expected, (*dest_copy)); | ||
assert_eq!(expected, (*dest_copy)); | ||
|
||
log::info!("All tests passed!"); | ||
loop { | ||
debug::exit(debug::EXIT_SUCCESS) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this? You have a
src: &mut [u8; 40]
, take a pointer to it (*const [u8; 40]
), deref that ([u8; 40]
) and then take a reference to that&[u8; 40]
? Is the point to get another reference to the buffer?I think that since you now have both a shared and an exclusive reference to the buffer, we have UB. (Assuming I am not missing something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the point is to verify the contents of the buffer after the DMA transfer is complete. Requiring a static reference to the buffer in the DmaTransfer API and only getting a reference from the
singleton
macro, instead of a value, means I can't access thesrc
reference to the buffer after passing it to thememory_to_memory
function (the reference gets moved). It's UB, but IMO it is safe because the reference is not used until the DMA transfer is stopped. Is that incorrect?This does reflect the pain of using this API now. It's really tricky to use without some sort of unsafe usage, and is why I initially avoided requiring static references to buffers in the first place. The alternative I've seen (e.g. stm32f4xx-hal) to this is to return the source & destination values (whether they be
'static
references or otherwise when the transfer completes (from afree
method or whenwait_for_transfer_complete
returns), but I generally don't like that API because it now forces users to use options to store these references in user code, or use unsafe themselves. Maybe that reflects the challenges of using DMA in general?I've also been figuring out the peripheral API and using
Future
s based on top of this API, and those both become more painful with this change too. But, if you think that is a more sound API, I can try out the alternative above and see how that interacts with Future/peripheral usage.You can see the WIP Futures and SPI DMA in #65 and #66. Would love feedback if you have ideas for improving/settling on an API!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While attempting to ensure the API is sound, and putting the onus on the user to ensure that their buffer use is sound seems like the appropriate approach, by requiring
'static
source/destination seems appropriate, this example also demonstrates how user can easily get around the requirement to have static buffers, by doing this exact operation).So perhaps the requirement for
'static
lifetimes is actually counterproductive. The user can introduce UB by leakingDmaTransfer
with the old API, or by copying references like I've done here. Dealer's choice. I felt encouraged to use this hack to meet the requirement to use a'static
lifetime, which feels like bad API design. Again, happy to learn how to do this in a safer way!Leaking
DmaTransfer
seems like a much more deliberately degenerate thing to do, perhaps we could instead take the approach a similar approach embassy takes, and mark the transfer initialization functions as unsafe. We could then include a warning to not leak theDmaTransfer
struct in a safety note.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AdinAck have you had the opportunity to think about it any more since we spoke last in the G4 issue about DMA? Do you have any input here or just in general?
Also @astapleton, as to
Please correct if I am wrong but I am pretty sure that strictly speaking it is instant UB the moment the second reference is created . Due to the UB the resulting binary is just something that may or may not happen to seem to do the right thing or some thing completely different which may or may not change depending on compiler version/options/alignment of stars/nasal demons etc. Yet I am not at all sure what we are allowed to do instead for that case... Perhaps a raw pointer that is only turned into a reference after the DMA is done?
I have tried(with not so great results) to read up on some recent discussion on the subject of DMA, volatile read/write etc. Here is some info. With, I think, this being most relevant ('2' being the DMA case):
Then there is this stating
Then there is this, stating
And this comment
I am getting more confused the more I read :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have! My plans are to begin implementing safe DMA in the next few weeks.
I don't have any code to share yet, nor any concrete decisions, but here are some thoughts...
I believe a DMA transfer can simply be split into two sides: the source and the destination. Either side can either hold a contiguous region of software memory, or a handle to a specific peripheral field.
The contract of these transfer endpoint types, is that the existence of the type implies an active transfer, so no need dealing with
'static
lifetimes, as long as the transfer endpoint type exists it holds the necessary resources and performing a DMA transfer is sound.A simple example of pseudo-code:
Since DMA is a highly dynamic process, I don't think we should even try to represent the stage of the transfer with the type-system, as it is likely not static enough. Maybe as I learn more my opinion on this will change, but I doubt it.
So then, each endpoint has methods like
.start()
,.cancel()
,.with(FnOnce(BufferAccess)
. The latter of which providing conditional access to a provided buffer only when the transfer is not active, and holding access to the field handles which control the transfer activity so a transfer cannot be started while the closure is running.BufferAccess
would be a&mut ...
if the endpoint was "source" and a& ...
if the endpoint was "destination".Related to what you guys were talking about with compiler fencing, I think the most reasonable solution would be to provide the buffer reference to this closure by
read_volatile
ing the buffer, even though we could just reference it directly, because that forces the compiler to properly order the reads/writes. Otherwise, it would have no reason to make sure that "read status flag and make sure transfer is inactive" precedes "read src/dest buffer".For a oneshot transfer, the transfer endpoints would be deconstructed:
So I'll be working on this in the coming weeks.
Just to reiterate, I believe the simplest solution is to make sure we use volatile read/writes to any in-memory buffers involved in a DMA transfer, because then we know the MMIO volatile access and the buffer volatile access will be in the correct order. Even though we have the buffer and could just reference it, according to all those resources you sent, the compiler could reorder volatile and non-volatile access. But I would consider -- since DMA is touching our memory -- access to our in-memory buffers is immediately now volatile memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💀 Well, that's quite the thorn in my side huh. I'll keep thinking but I'm pretty sure you're right and that destroys any and all invariances I would want to establish with the DMA transfer type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, really hope they find some solution to the leak thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a lighter note, this section of the embedonomicon brought me some peace of mind. The usage of the compiler fence and memory barrier instructions (or lack thereof) is very clear. I was especially happy to read:
So I will feel comfortable using compiler fences (and maybe a
dsb
just to be safe) to ensure the ordering of DMA operations is enforced.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much hope you are right but I think I read somewhere suggestions that it was not enough and that an asm block might be needed(again, I did not relly get it so I wont get into it much further). Perhaps somewhere here where there is also is discussion about dma writing to a &mut
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I had the same realization :)