Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
344 changes: 329 additions & 15 deletions esp-hal/src/dma/buffers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -995,6 +995,8 @@ unsafe impl DmaRxBuffer for DmaRxTxBuf {
///
/// See [DmaRxStreamBufView] for APIs available whilst a transfer is in
/// progress.
#[derive(Debug)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub struct DmaRxStreamBuf {
descriptors: &'static mut [DmaDescriptor],
buffer: &'static mut [u8],
Expand All @@ -1015,22 +1017,16 @@ impl DmaRxStreamBuf {
return Err(DmaBufError::UnsupportedMemoryRegion);
}

if descriptors.is_empty() {
// see https://github.com/esp-rs/esp-hal/issues/2269#issuecomment-4397953660
// we can lift that requirement once we sort out this issue
if descriptors.len() < 4 {
return Err(DmaBufError::InsufficientDescriptors);
}

// Evenly distribute the buffer between the descriptors.
let chunk_size = buffer.len() / descriptors.len();

if chunk_size > 4095 {
return Err(DmaBufError::InsufficientDescriptors);
}

// Check that the last descriptor can hold the excess
let excess = buffer.len() % descriptors.len();
if chunk_size + excess > 4095 {
return Err(DmaBufError::InsufficientDescriptors);
}
let chunk_size = Some(buffer.len() / descriptors.len())
.filter(|x| *x <= 4095)
.ok_or(DmaBufError::InsufficientDescriptors)?;

let mut chunks = buffer.chunks_exact_mut(chunk_size);
for (desc, chunk) in descriptors.iter_mut().zip(chunks.by_ref()) {
Expand All @@ -1039,12 +1035,15 @@ impl DmaRxStreamBuf {
}

let remainder = chunks.into_remainder();
debug_assert_eq!(remainder.len(), excess);

if !remainder.is_empty() {
// Append any excess to the last descriptor.
let last_descriptor = descriptors.last_mut().unwrap();
last_descriptor.set_size(last_descriptor.size() + remainder.len());
let size = last_descriptor.size() + remainder.len();
if size > 4095 {
return Err(DmaBufError::InsufficientDescriptors);
}
last_descriptor.set_size(size);
}

Ok(Self {
Expand Down Expand Up @@ -1133,6 +1132,9 @@ impl DmaRxStreamBufView {
let mut remaining = buf;
loop {
let available = self.peek();
if available.is_empty() {
break;
}
if available.len() >= remaining.len() {
remaining.copy_from_slice(&available[0..remaining.len()]);
self.consume(remaining.len());
Expand Down Expand Up @@ -1238,8 +1240,9 @@ impl DmaRxStreamBufView {
(&[], false)
} else {
let length = last_descriptor.len() - self.descriptor_offset;
let chunk_size = last_descriptor.size();
(
&self.buf.buffer[self.buf.buffer.len() - length..],
&self.buf.buffer[self.buf.buffer.len() - chunk_size..][..length],
Comment thread
Dominaezzz marked this conversation as resolved.
last_descriptor.flags.suc_eof(),
)
}
Expand Down Expand Up @@ -1273,6 +1276,317 @@ impl DmaRxStreamBufView {
}
}

/// DMA Streaming Transmit Buffer.
///
/// This is symmetric implementation to [DmaRxStreamBuf], used for continuously
/// streaming data to a peripheral's FIFO.
Comment thread
Dominaezzz marked this conversation as resolved.
///
/// The list starts out like so `A(full) -> B(full) -> C(full) -> D(full) -> NULL`.
///
/// As the DMA writes to FIFO, the list progresses like so:
/// - `A(full) -> B(full) -> C(full) -> D(full) -> NULL`
/// - `A(empty) -> B(full) -> C(full) -> D(full) -> NULL`
/// - `A(empty) -> B(empty) -> C(full) -> D(full) -> NULL`
/// - `A(empty) -> B(empty) -> C(empty) -> D(full) -> NULL`
///
/// As you call [DmaTxStreamBufView::push] the list (approximately) progresses like so:
/// - `A(empty) -> B(empty) -> C(empty) -> D(full) -> NULL`
/// - `B(empty) -> C(empty) -> D(full) -> A(full) -> NULL`
/// - `C(empty) -> D(full) -> A(full) -> B(full) -> NULL`
/// - `D(full) -> A(full) -> B(full) -> C(full) -> NULL`
///
/// If all the descriptors run out, the [DmaTxInterrupt::TotalEof] interrupt will fire and DMA
/// will stop writing, at which point it is up to you to resume/restart the transfer.
#[derive(Debug)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub struct DmaTxStreamBuf {
descriptors: &'static mut [DmaDescriptor],
buffer: &'static mut [u8],
burst: BurstConfig,
pre_filled: Option<usize>,
view_descriptor_idx: usize,
view_descriptor_offset: usize,
}

impl DmaTxStreamBuf {
/// Creates a new [DmaTxStreamBuf] evenly distributing the buffer between
/// the provided descriptors.
pub fn new(
descriptors: &'static mut [DmaDescriptor],
buffer: &'static mut [u8],
) -> Result<Self, DmaBufError> {
match () {
_ if !is_slice_in_dram(descriptors) => Err(DmaBufError::UnsupportedMemoryRegion)?,
_ if !is_slice_in_dram(buffer) => Err(DmaBufError::UnsupportedMemoryRegion)?,
_ if descriptors.len() < 4 => {
// see https://github.com/esp-rs/esp-hal/issues/2269#issuecomment-4397953660
// we can lift that requirement once we sort out this issue
Err(DmaBufError::InsufficientDescriptors)?
}
_ => (),
};

// Evenly distribute the buffer between the descriptors.
let chunk_size = Some(buffer.len() / descriptors.len())
.filter(|x| *x <= 4095)
.ok_or(DmaBufError::InsufficientDescriptors)?;

let mut chunks = buffer.chunks_exact_mut(chunk_size);
for (desc, chunk) in descriptors.iter_mut().zip(chunks.by_ref()) {
desc.buffer = chunk.as_mut_ptr();
desc.set_size(chunk.len());
desc.set_length(chunk.len());
}
let remainder = chunks.into_remainder();

if !remainder.is_empty() {
// Append any excess to the last descriptor.
let last_descriptor = descriptors.last_mut().unwrap();
let size = last_descriptor.size() + remainder.len();
if size > 4095 {
Err(DmaBufError::InsufficientDescriptors)?;
}
last_descriptor.set_size(size);
}

Ok(Self {
descriptors,
buffer,
burst: Default::default(),
pre_filled: None,
view_descriptor_idx: 0,
view_descriptor_offset: 0,
})
}

/// Consume the buf, returning the descriptors and buffer.
pub fn split(self) -> (&'static mut [DmaDescriptor], &'static mut [u8]) {
(self.descriptors, self.buffer)
}

/// Push the buffer with the given data before DMA transfer starts.
///
/// Otherwise the streaming buffer will transfer garbage data to
/// DMA so that CPU has enough time to fill the buffer after transfer starts.
pub fn push(&mut self, data: &[u8]) -> usize {
self.push_with(|buf| {
let len = buf.len().min(data.len());
buf[..len].copy_from_slice(&data[..len]);
len
})
}

/// Push the buffer with the given data before DMA transfer starts.
///
/// Returns the number of bytes filled.
pub fn push_with(&mut self, f: impl FnOnce(&mut [u8]) -> usize) -> usize {
let start = self.pre_filled.unwrap_or(0);
let bytes_pushed = f(&mut self.buffer[start..]);
self.pre_filled = Some(start + bytes_pushed);
bytes_pushed
}

fn setup_view_state(&mut self) {
self.view_descriptor_idx = 0;
self.view_descriptor_offset = 0;
let pre_filled = self.pre_filled.unwrap_or(self.buffer.len());
mark_tx_stream_descriptors_ready(
self.descriptors,
&mut self.view_descriptor_idx,
&mut self.view_descriptor_offset,
pre_filled,
);
}
}

/// Marks descriptors containing data that should be transmitted when the DMA
/// channel starts. Unlike [advance_tx_stream_descriptors], this does not modify
/// the `next` pointers because the linked list must remain intact until the
/// transfer is running.
fn mark_tx_stream_descriptors_ready(
descriptors: &mut [DmaDescriptor],
descriptor_idx: &mut usize,
descriptor_offset: &mut usize,
bytes_pushed: usize,
) {
if bytes_pushed == 0 {
return;
}

let mut bytes_filled = 0;
let num_descriptors = descriptors.len();

for i in 0..num_descriptors {
let d = (*descriptor_idx + i) % num_descriptors;
let desc = &mut descriptors[d];
let bytes_in_d = desc.size() - *descriptor_offset;

if bytes_in_d + bytes_filled > bytes_pushed {
*descriptor_idx = d;
*descriptor_offset = *descriptor_offset + bytes_pushed - bytes_filled;
desc.set_owner(Owner::Dma);
desc.set_length(*descriptor_offset);
desc.set_suc_eof(true);
return;
}

bytes_filled += bytes_in_d;
*descriptor_offset = 0;

desc.set_owner(Owner::Dma);
desc.set_length(desc.size());
desc.set_suc_eof(true);
}
}

fn advance_tx_stream_descriptors(
descriptors: &mut [DmaDescriptor],
descriptor_idx: &mut usize,
descriptor_offset: &mut usize,
bytes_pushed: usize,
) {
if bytes_pushed == 0 {
return;
}

let mut bytes_filled = 0;
let num_descriptors = descriptors.len();

for i in 0..num_descriptors {
let d = (*descriptor_idx + i) % num_descriptors;
let desc = &mut descriptors[d];
let bytes_in_d = desc.size() - *descriptor_offset;
if bytes_in_d + bytes_filled > bytes_pushed {
*descriptor_idx = d;
*descriptor_offset = *descriptor_offset + bytes_pushed - bytes_filled;
return;
}
bytes_filled += bytes_in_d;
*descriptor_offset = 0;

// Put the current descriptor at the end of the list
desc.set_owner(Owner::Dma);
desc.set_length(desc.size());
desc.set_suc_eof(true);
let p = d.checked_sub(1).unwrap_or(num_descriptors - 1);
if p != d {
let [prev, desc] = descriptors.get_disjoint_mut([p, d]).unwrap();
desc.next = null_mut();
prev.next = desc;
}
}
}

unsafe impl DmaTxBuffer for DmaTxStreamBuf {
type View = DmaTxStreamBufView;
type Final = Self;

fn prepare(&mut self) -> Preparation {
// Link up all the descriptors (but not in a circle).
let mut next = null_mut();
for desc in self.descriptors.iter_mut().rev() {
desc.next = next;
next = desc;

desc.set_owner(Owner::Cpu);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this should be Owner::Dma, otherwise available_bytes returns a value that is too high in the first pass.

If I'm understanding this buffer type correctly, it sets up the descriptors such that they are all linked together and the first half of the linked list contains the prefilled data and the second half contains empty descriptors.

When the user pushes more data after the transfer starts, it takes descriptors from the start of the linked list, instead of the start of the 2nd half.

The push function checks the descriptor ownership to see if it's available for filling.
Once the dma gets through the first half of the linked list, the push function is going to think the DMA has gotten through the second half as well, even if the DMA hasn't, due to these descriptors being marked as owned by the CPU.

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.

it should and it's overwritten as Dma in setup_view_state - removed since this is nothing but confusing

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The problem still exists because DMA descriptors are Owner::Cpu by default anyway, so the 2nd half of the list isn't set to Owner::Dma.

The new test you added doesn't cover the prefilled case.

You need another test that does:

  • Make a stream buf of 4096/1024
  • Push 2048 bytes or so.
  • Convert to view, then back to buf.
  • split the buf and assert that all descriptors are owned by DMA.

I think that last assertion will fail. (which is a bug)

Ideally we'd be able to use available_bytes for the assertion but that gets reset when the buf is recreated.
These buffers are tricky to test. Mem2Mem might be the way but not worth complicating this PR more.

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.

good catch - I forgot about prefilled buffers 👍

desc.set_length(0);
desc.set_suc_eof(false);
}

self.setup_view_state();

Preparation {
start: self.descriptors.as_mut_ptr(),
direction: TransferDirection::Out,
#[cfg(dma_can_access_psram)]
accesses_psram: false,
burst_transfer: self.burst,

// Whilst we give ownership of the descriptors the DMA, the correctness of this buffer
// implementation doesn't rely on the DMA checking for descriptor ownership.
// No descriptor is added back to the end of the stream before it's ready for the DMA
// to consume it.
check_owner: None,
auto_write_back: true,
}
}

fn into_view(self) -> Self::View {
DmaTxStreamBufView {
descriptor_idx: self.view_descriptor_idx,
descriptor_offset: self.view_descriptor_offset,
buf: self,
}
}

fn from_view(view: Self::View) -> Self {
view.buf
}
}

/// A view into a [DmaTxStreamBuf]
pub struct DmaTxStreamBufView {
buf: DmaTxStreamBuf,
descriptor_idx: usize,
descriptor_offset: usize,
}

impl DmaTxStreamBufView {
/// Returns the number of bytes available for writing.
pub fn available_bytes(&self) -> usize {
let (tail, head) = self.buf.descriptors.split_at(self.descriptor_idx);
head.iter()
.chain(tail)
.take_while(|d| d.owner() == Owner::Cpu)
.map(|d| d.size())
.sum::<usize>()
- self.descriptor_offset
}

/// Pushes a buffer into the stream buffer.
/// Returns the number of bytes pushed.
pub fn push_with(&mut self, f: impl FnOnce(&mut [u8]) -> usize) -> usize {
let chunk_size = self.buf.descriptors[0].size();
let dma_start = self.descriptor_idx * chunk_size + self.descriptor_offset;
let dma_end = (dma_start + self.available_bytes()).min(self.buf.buffer.len());
let bytes_pushed = f(&mut self.buf.buffer[dma_start..dma_end]);

self.advance(bytes_pushed);
bytes_pushed
}

/// Advances the first `n` bytes from the available data
pub fn advance(&mut self, bytes_pushed: usize) {
advance_tx_stream_descriptors(
self.buf.descriptors,
&mut self.descriptor_idx,
&mut self.descriptor_offset,
bytes_pushed,
);
}

/// Pushes a buffer into the stream buffer.
/// Returns the number of bytes pushed.
pub fn push(&mut self, data: &[u8]) -> usize {
let total_len = data.len();
let mut remaining = data;

while !remaining.is_empty() && self.available_bytes() > 0 {
let written = self.push_with(|buffer| {
let len = usize::min(buffer.len(), remaining.len());
buffer[..len].copy_from_slice(&remaining[..len]);
len
});
if written == 0 {
break;
}
remaining = &remaining[written..];
}

total_len - remaining.len()
}
}

static mut EMPTY: [DmaDescriptor; 1] = [DmaDescriptor::EMPTY];

/// An empty buffer that can be used when you don't need to transfer any data.
Expand Down
Loading
Loading