SpiDma read/write: copy as little as possible#5290
Conversation
|
New commits in main has made this PR unmergable. Please resolve the conflicts. |
| if copy_before > 0 { | ||
| self.write_async_impl(MaybeCopyTxBuf::Copy(tx_buffer), &words[..copy_before]) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
This cries for a "block for short transfers" setting. Whether we want that even for CPU-controlled async transfers is a good question, I think really fast SPI busses justify it.
This comment was marked as outdated.
This comment was marked as outdated.
38fd120 to
85d8576
Compare
5e99f7f to
38a1636
Compare
|
/hil full |
|
/test-size embassy_spi esp32s3 |
Binary Size Report
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Triggered full HIL run for #5290. Run: https://github.com/esp-rs/esp-hal/actions/runs/23854189810 Status update: ❌ HIL (full) run failed (conclusion: failure). |
| #[cfg(esp32)] | ||
| if !(buffer.as_ptr() as usize).is_multiple_of(4) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I'll highlight this just to mention that I don't want to bother with this PR any more just to get it to work on ESP32. We still skip copying if the data is aligned, but resolving this here would take probably more changes than this PR needs to have. The solution would be to add a copy buffer to MaybeCopyTxBuffer::Direct, and pipe it through prepare_for_tx, but it can be done in the future if we really need to do it.
|
New commits in main has made this PR unmergable. Please resolve the conflicts. |
|
New commits in main has made this PR unmergable. Please resolve the conflicts. |
be7262a to
c3da565
Compare
|
/hil full |
|
Triggered full HIL run for #5290. Run: https://github.com/esp-rs/esp-hal/actions/runs/25317421035 Status update: ❌ HIL (full) run failed (conclusion: failure). |
There was a problem hiding this comment.
Pull request overview
This PR reduces CPU-side copying in the SPI master DMA slice-based API by attempting in-place DMA transfers from DMA-capable memory (DRAM/PSRAM), falling back to internal copy buffers only when required. It also refactors DMA buffer internals to support “scoped” buffer ownership and expands HIL coverage to exercise different memory regions and alignments.
Changes:
- Reworked
SpiDmaslice-basedread/write/transfer(+ async variants) to use prepared descriptor chains for direct DMA where possible, minimizing copies. - Introduced
ScopedDma{Rx,Tx}Bufand refactoredDma{Rx,Tx}Bufto wrap them, enabling safer internal buffer handling. - Updated SPI full-duplex HIL tests to run transfers across DRAM and (when available) PSRAM with multiple alignment offsets and sizes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hil-test/src/bin/spi_full_duplex.rs | Expands HIL tests to cover DRAM/PSRAM + alignment shifts and updates PCNT setup reuse. |
| esp-hal/src/spi/master/dma.rs | Implements copy-avoiding slice-based SPI DMA transfers via direct descriptor preparation and new helper types. |
| esp-hal/src/dma/buffers/scoped.rs | Adds scoped DMA RX/TX buffer types backing the public Dma{Rx,Tx}Buf wrappers. |
| esp-hal/src/dma/buffers/mod.rs | Refactors Dma{Rx,Tx}Buf to wrap scoped buffers; exposes NoBuffer internals and adjusts DMA prep helpers. |
Using code developed for AES DMA, this PR tries its best to avoid copying data before DMA-enabled SPI transfers. This PR enables us to send/receive data to/from DRAM and PSRAM without configuring copy buffers.
Non-copying transfers are disabled on ESP32 for unaligned data. ESP32 is the only device we have that requires a TX-side copy buffer due to alignment requirements, and the code we've had for AES-DMA doesn't implement that yet.
Closes #3125 by removing most of the copying that a display driver would do.