Skip to content

SpiDma read/write: copy as little as possible#5290

Merged
MabezDev merged 18 commits intoesp-rs:mainfrom
bugadani:spi-dma-nocopy
May 6, 2026
Merged

SpiDma read/write: copy as little as possible#5290
MabezDev merged 18 commits intoesp-rs:mainfrom
bugadani:spi-dma-nocopy

Conversation

@bugadani
Copy link
Copy Markdown
Contributor

@bugadani bugadani commented Mar 31, 2026

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.

@github-actions github-actions Bot added the merge-conflict Merge conflict detected. Automatically added/removed by CI. label Mar 31, 2026
@github-actions
Copy link
Copy Markdown

New commits in main has made this PR unmergable. Please resolve the conflicts.

Comment thread esp-hal/src/spi/master/dma.rs
Comment thread esp-hal/src/spi/master/dma.rs
Comment thread esp-hal/src/spi/master/dma.rs Outdated
Comment on lines +501 to +504
if copy_before > 0 {
self.write_async_impl(MaybeCopyTxBuf::Copy(tx_buffer), &words[..copy_before])
.await?;
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bugadani

This comment was marked as outdated.

@bugadani bugadani force-pushed the spi-dma-nocopy branch 2 times, most recently from 38fd120 to 85d8576 Compare April 1, 2026 09:33
@github-actions github-actions Bot removed the merge-conflict Merge conflict detected. Automatically added/removed by CI. label Apr 1, 2026
@bugadani bugadani force-pushed the spi-dma-nocopy branch 5 times, most recently from 5e99f7f to 38a1636 Compare April 1, 2026 12:36
@bugadani
Copy link
Copy Markdown
Contributor Author

bugadani commented Apr 1, 2026

/hil full

@bugadani
Copy link
Copy Markdown
Contributor Author

bugadani commented Apr 1, 2026

/test-size embassy_spi esp32s3

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

Binary Size Report

esp32s3

Diff (PR vs. Base)
VM SIZE    
-------------- 
+0.7%    +428    .bss
-1.5% -1.02Ki    .text
-0.4%    -612    TOTAL
Filtering enabled (source_filter); omitted  341Ki of entries

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

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).

Comment on lines +722 to +725
#[cfg(esp32)]
if !(buffer.as_ptr() as usize).is_multiple_of(4) {
return false;
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 2, 2026

New commits in main has made this PR unmergable. Please resolve the conflicts.

@github-actions github-actions Bot added the merge-conflict Merge conflict detected. Automatically added/removed by CI. label Apr 2, 2026
@github-actions github-actions Bot added merge-conflict Merge conflict detected. Automatically added/removed by CI. and removed merge-conflict Merge conflict detected. Automatically added/removed by CI. labels Apr 7, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 8, 2026

New commits in main has made this PR unmergable. Please resolve the conflicts.

@github-actions github-actions Bot removed the merge-conflict Merge conflict detected. Automatically added/removed by CI. label Apr 13, 2026
@github-actions github-actions Bot removed the merge-conflict Merge conflict detected. Automatically added/removed by CI. label May 4, 2026
@bugadani
Copy link
Copy Markdown
Contributor Author

bugadani commented May 4, 2026

/hil full

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

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).

@bugadani bugadani marked this pull request as ready for review May 5, 2026 09:58
Copilot AI review requested due to automatic review settings May 5, 2026 09:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SpiDma slice-based read/write/transfer (+ async variants) to use prepared descriptor chains for direct DMA where possible, minimizing copies.
  • Introduced ScopedDma{Rx,Tx}Buf and refactored Dma{Rx,Tx}Buf to 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.

Comment thread esp-hal/src/spi/master/dma.rs Outdated
Copy link
Copy Markdown
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MabezDev MabezDev added this pull request to the merge queue May 6, 2026
Merged via the queue into esp-rs:main with commit 054a366 May 6, 2026
23 checks passed
@bugadani bugadani deleted the spi-dma-nocopy branch May 6, 2026 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow DMA buffer disassociation to prevent memory duplication

3 participants