Skip to content

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

Draft
bugadani wants to merge 19 commits intoesp-rs:mainfrom
bugadani:spi-dma-nocopy
Draft

SpiDma read/write: copy as little as possible#5290
bugadani wants to merge 19 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
@bugadani bugadani force-pushed the spi-dma-nocopy branch 2 times, most recently from f66878d to 09ff5b1 Compare April 13, 2026 10:32
Comment on lines +725 to +739
#[cfg(esp32s2)]
if _direction == TransferDirection::In
&& !((buffer.as_ptr() as usize).is_multiple_of(16)
&& buffer.len().is_multiple_of(16))
{
// When EDMA accesses external RAM, fields in linked list descriptors should be
// aligned with block size. Specifically, size and buffer
// address pointer in receive descriptors should be 16-byte, 32-byte or 64-byte
// aligned. For data frame whose length is not a multiple of 16 bytes, 32 bytes,
// or 64 bytes, EDMA adds padding bytes to the end.
//
// FIXME: the above should be handled by `prepare_for_rx`, but for now we just
// skip the no-copy optimization.
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.

Makes me wonder why AES doesn't have this issue - are we lucky with the tests?

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

1 participant