Skip to content
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

[DMA 8/8] Burst configuration #2543

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Nov 14, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

This PR adds burst mode configuration to the DMA buffers. The PR also describes burst mode constraints for both internal and PSRAM accesses, where applicable.

esp-hal/src/dma/buffers.rs Outdated Show resolved Hide resolved
@bugadani bugadani force-pushed the dma-burst branch 4 times, most recently from f2fae81 to 9b608fe Compare November 14, 2024 21:46
esp-hal/src/dma/mod.rs Outdated Show resolved Hide resolved
@Dominaezzz
Copy link
Collaborator

  • we can probably enable external memory support in DmaRxBuf

Easier said than done.

Before this can happen, the work to remove DmaRxBuffer::length (and less importantly DmaTxBuffer as well) has to be done (which mostly means the SPI DMA driver has to take the length explicitly, rather than infer from the DmaRxBuffer).

Why? Enabling extmem support in DmaRxBuf means conforming to the 16,32,64 byte alignment in size. The means DmaRxBuffer::length() would always be a multiple of 16 (at least).
What happens when someone wants to receive 120 bytes of data into extmem? 120 bytes can't be aligned to 16, 32 or 64. The solution here is a DmaRxBuf of size 128 bytes and an SPI transaction length of 120.

Another nice thing about length() going away is the SPI DMA can be used with all buffer types, not just the linear/basic DmaRxBuf/DmaTxBuf.

@bugadani
Copy link
Contributor Author

What happens when someone wants to receive 120 bytes of data into extmem?

Note that length is always unconstrained, alignment requirements are for address and size only. For reads, the DMA will pad the received data to 128 bytes, using zeroes. Then the DMA sets the length to the actually received amount. For writes, Size, length and buffer address pointer in transmit descriptors are not necessarily aligned with block size

@Dominaezzz
Copy link
Collaborator

What happens when someone wants to receive 120 bytes of data into extmem?

Note that length is always unconstrained, alignment requirements are for address and size only. For reads, the DMA will pad the received data to 128 bytes, using zeroes. Then the DMA sets the length to the actually received amount. For writes, Size, length and buffer address pointer in transmit descriptors are not necessarily aligned with block size

Yes but how do you tell the SPI to only read 120 bytes, instead of the 128 that DmaRxBuf has been set to for alignment reasons?

@bugadani
Copy link
Contributor Author

bugadani commented Nov 14, 2024

Ah okay I understand your issue, it's not a hardware-related one. Yeah this is a consequence of the design right now 🤷‍♂️

I'm guessing we could add a read_length (in spirit, not necessarily in name) field to DmaRxBuf, that we'd use to set up the peripherals. That value wouldn't affect the descriptors, but users could use it to set an upper limit to how much data they want to read.

@Dominaezzz
Copy link
Collaborator

Yeah this is a consequence of the design right now 🤷‍♂️

Indeed

I'm guessing we could add a read_length (in spirit, not necessarily in name) field to DmaRxBuf, that we'd use to set up the peripherals. That value wouldn't affect the descriptors, but users could use it to set an upper limit to how much data they want to read.

Yeah that would work but supporting that on other infinite-length descriptors would be awkward.

@bugadani
Copy link
Contributor Author

Yeah that would work but supporting that on other infinite-length descriptors would be awkward.

Please explain this a bit

@Dominaezzz
Copy link
Collaborator

Yeah that would work but supporting that on other infinite-length descriptors would be awkward.

Please explain this a bit

Oops typo. I meant buffers not descriptors. (I need to read my comments more)

DmaRxStreamBuf doesn't really have a length, you can use it to receive data forever so there's no such thing as a "max" length.

@bugadani

This comment was marked as outdated.

@bugadani bugadani force-pushed the dma-burst branch 2 times, most recently from 81286d6 to 4ea6b19 Compare November 15, 2024 10:02
@Dominaezzz
Copy link
Collaborator

Do you expect that to work with peripherals where the peripheral dictates how much it reads? Especially if there is no TxBuf pair associated to the transfers. I can see slave mode SPI using indeterminate length RX buffers by themselves.

Yes! I'm proposing that the peripheral takes the length. I.e. spi_dma.read(120, dma_rx_buf).

The use case I'm thinking of is the user wants to send (or read) 30000 bytes to an spi device but doesn't want to allocate a 30,000 byte buffer. They could make do with 4000 bytes only and stream the data with a stream buf.

SPI and parl io have special interrupts to let the user know when they were too slow.

@bugadani
Copy link
Contributor Author

Alright, that sounds to me like a problem for later. For now, if someone is fine with block aligned reads, they are free to allocate the RX buffer in PSRAM.

@@ -122,27 +122,27 @@ mod test {
let mut spi = Spi::new_with_config(
peripherals.SPI2,
Config {
frequency: 100.kHz(),
frequency: 10000.kHz(),
Copy link
Contributor Author

@bugadani bugadani Nov 15, 2024

Choose a reason for hiding this comment

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

Had to speed these (well, one but I didn't bother figuring out which one, probably SPI3) up, otherwise this test failed (but only with generic-queue). I think it's a setup time regression, and we ran out of the 1ms tick used by the interrupt task, thereby starving the main task. Considering we were transferring 64 bits at 100kHz, we were already close anyway.

@bugadani bugadani changed the title [DMA 7/N] Burst configuration [DMA 7/8] Burst configuration Nov 15, 2024
@bugadani bugadani changed the title [DMA 7/8] Burst configuration [DMA 8/8] Burst configuration Nov 23, 2024
@bugadani
Copy link
Contributor Author

Moved this PR back a spot, as this is probably the bigger and more complex change.

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.

2 participants