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.

Sign up for GitHub

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

Add check owner support to DMA buffers #2337

Merged
merged 5 commits into from
Nov 2, 2024
Merged

Add check owner support to DMA buffers #2337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d3fa0ba
Add check owner support to DMA buffers
Oct 10, 2024
7056160
Merge remote-tracking branch 'upstream/main' into check_owner
Oct 16, 2024
a1ce29d
More docs
Oct 16, 2024
fd174b4
Explicit check owner bit setting
Oct 22, 2024
70d7340
Merge remote-tracking branch 'refs/remotes/upstream/main' into check_…
Oct 22, 2024
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
43 changes: 42 additions & 1 deletion esp-hal/src/dma/buffers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,30 @@ pub struct Preparation {
/// memory?)
#[cfg_attr(not(esp32s3), allow(dead_code))]
pub(super) block_size: Option<DmaBufBlkSize>,
// burst_mode, alignment, check_owner, etc.

/// Most DMA channels allow software to configure whether the hardware
Dominaezzz marked this conversation as resolved.
Show resolved Hide resolved
/// checks that [DmaDescriptor::owner] is set to [Owner::Dma] before
/// consuming the descriptor. If this check fails, the channel stops
/// operating and fires
/// [DmaRxInterrupt::DescriptorError]/[DmaTxInterrupt::DescriptorError].
///
/// This field allows buffer implementation to configure this behaviour.
///
/// Some buffer implementations may require that the DMA channel performs
/// this check before consuming the descriptor to ensure correct
/// behaviour. e.g. To prevent wrap-around in a circular transfer.
///
/// Some buffer implementations may require that the DMA channel does NOT
/// perform this check as the ownership bit will not be set before the
/// channel tries to consume the descriptor.
///
/// Most implementations won't have any such requirements and will work
/// correctly regardless of whether the DMA channel checks or not.
///
/// Note: If the DMA channel doesn't support the provided option,
/// preparation will fail.
pub(super) check_owner: Option<bool>,
Copy link
Contributor

Choose a reason for hiding this comment

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

WHy is this optional, what's the difference between None and Some(false)? The comment uses a lot of words to not explain this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • Some(true): DMA channel must the owner bit
  • Some(false): DMA channel must NOT the owner bit
  • None: DMA channel can do whatever it likes/supports.

I can add this after the "This field allows buffer implementation to configure this behaviour." comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

None is somewhat problematic, though, isn't it? It preserves some random previous setting in the current implementation, can't that cause problems?

Copy link
Collaborator Author

@Dominaezzz Dominaezzz Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ehhh, it does preserve some random previous setting, which I admit is a bit weird but I couldn't decide on a default haha. Like the doc says, for some buffer implementations, it doesn't matter whether the DMA channel checks or not. (And besides, the buffer trait is unsafe so we're trusting the user to pick the right option)

The main reason this None option exists is because SPI DMA (at least, I haven't checked all the other PDMA channels) doesn't support configuring check_owner, and I wanted a way for buffers to say "I don't care about this setting".

Copy link
Contributor

Choose a reason for hiding this comment

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

(And besides, the buffer trait is unsafe so we're trusting the user to pick the right option)

The user can't implement the buffer traits anyway because the fields of Preparation aren't pub 🙃

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah 😅 I'm working towards that. (Still have to decide on builder pattern vs plain struct)

Copy link
Contributor

@bugadani bugadani Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IDK I think "Some(false): DMA channel must NOT [check?] the owner bit" sounds like a reasonable default instead of "None = whatever". You obviously know more here, so I would appreciate you explaining why accidentally owner-checking based on past use is a better option.

I think if the answer is "it makes no difference" then less options might be better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So there's two parts of the equation:

  1. What behavior the buffer implementation needs
  2. What the DMA channel can provide

For 1, None needs to exist as an option because choosing either true or false means some DMA channels just won't work with the buffer implementation. If the buffer says true, DMA channels that cannot check will fail to prepare the buffer. If the buffer says false, DMA channels that can't not check will fail to prepare the buffer.
Having None means the buffer can work with any channel.

For 2, SPI DMA doesn't support configuring the "check owner" bit, so it'll reject false. Every other DMA channel (that is in the hal atm 😄) supports configuring this. In the None case, I left it as "past use" for simplicity but I can set it to true by default I suppose.

To eliminate the "accidentally owner-checking based on past use" I can configure the "check owner" bit to true when the buffer says it doesn't care. Then the behavior would be consistent.

// burst_mode, alignment, etc.
}

/// [DmaTxBuffer] is a DMA descriptor + memory combo that can be used for
Expand Down Expand Up @@ -290,6 +313,7 @@ unsafe impl DmaTxBuffer for DmaTxBuf {
Preparation {
start: self.descriptors.head(),
block_size: self.block_size,
check_owner: None,
}
}

Expand Down Expand Up @@ -436,6 +460,7 @@ unsafe impl DmaRxBuffer for DmaRxBuf {
Preparation {
start: self.descriptors.head(),
block_size: None,
check_owner: None,
}
}

Expand Down Expand Up @@ -560,6 +585,7 @@ unsafe impl DmaTxBuffer for DmaRxTxBuf {
Preparation {
start: self.tx_descriptors.head(),
block_size: None, // TODO: support block size!
check_owner: None,
}
}

Expand Down Expand Up @@ -587,6 +613,7 @@ unsafe impl DmaRxBuffer for DmaRxTxBuf {
Preparation {
start: self.rx_descriptors.head(),
block_size: None, // TODO: support block size!
check_owner: None,
}
}

Expand Down Expand Up @@ -723,6 +750,12 @@ unsafe impl DmaRxBuffer for DmaRxStreamBuf {
Preparation {
start: self.descriptors.as_mut_ptr(),
block_size: None,

// 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,
}
}

Expand Down Expand Up @@ -927,6 +960,10 @@ unsafe impl DmaTxBuffer for EmptyBuf {
Preparation {
start: unsafe { core::ptr::addr_of_mut!(EMPTY).cast() },
block_size: None,

// As we don't give ownership of the descriptor to the DMA, it's important that the DMA
// channel does *NOT* check for ownership, otherwise the channel will return an error.
check_owner: Some(false),
}
}

Expand All @@ -951,6 +988,10 @@ unsafe impl DmaRxBuffer for EmptyBuf {
Preparation {
start: unsafe { core::ptr::addr_of_mut!(EMPTY).cast() },
block_size: None,

// As we don't give ownership of the descriptor to the DMA, it's important that the DMA
// channel does *NOT* check for ownership, otherwise the channel will return an error.
check_owner: Some(false),
}
}

Expand Down
12 changes: 12 additions & 0 deletions esp-hal/src/dma/gdma.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,12 @@ impl<C: GdmaChannel> RegisterAccess for ChannelTxImpl<C> {
.modify(|_, w| w.outlink_restart().set_bit());
}

fn set_check_owner(&self, check_owner: bool) {
self.ch()
.out_conf1()
.modify(|_, w| w.out_check_owner().bit(check_owner));
}

#[cfg(esp32s3)]
fn set_ext_mem_block_size(&self, size: DmaExtMemBKSize) {
self.ch()
Expand Down Expand Up @@ -321,6 +327,12 @@ impl<C: GdmaChannel> RegisterAccess for ChannelRxImpl<C> {
.modify(|_, w| w.inlink_restart().set_bit());
}

fn set_check_owner(&self, check_owner: bool) {
self.ch()
.in_conf1()
.modify(|_, w| w.in_check_owner().bit(check_owner));
}

#[cfg(esp32s3)]
fn set_ext_mem_block_size(&self, size: DmaExtMemBKSize) {
self.ch()
Expand Down
12 changes: 12 additions & 0 deletions esp-hal/src/dma/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1704,6 +1704,10 @@ where
return Err(DmaError::InvalidAlignment);
}

if let Some(check_owner) = preparation.check_owner {
self.rx_impl.set_check_owner(check_owner);
}

compiler_fence(core::sync::atomic::Ordering::SeqCst);

self.rx_impl.clear_all();
Expand Down Expand Up @@ -1920,6 +1924,10 @@ where
);
// TODO: Get burst mode from DmaBuf.

if let Some(check_owner) = preparation.check_owner {
self.tx_impl.set_check_owner(check_owner);
}

compiler_fence(core::sync::atomic::Ordering::SeqCst);

self.tx_impl.clear_all();
Expand Down Expand Up @@ -2013,6 +2021,10 @@ pub trait RegisterAccess: crate::private::Sealed {
/// Mount a new descriptor.
fn restart(&self);

/// Configure the bit to enable checking the owner attribute of the
/// descriptor.
fn set_check_owner(&self, check_owner: bool);

#[cfg(esp32s3)]
fn set_ext_mem_block_size(&self, size: DmaExtMemBKSize);

Expand Down
26 changes: 26 additions & 0 deletions esp-hal/src/dma/pdma.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@ impl<C: PdmaChannel<RegisterBlock = SpiRegisterBlock>> RegisterAccess for SpiDma
.modify(|_, w| w.outlink_restart().set_bit());
}

fn set_check_owner(&self, check_owner: bool) {
if check_owner {
panic!("SPI DMA does not support checking descriptor ownership");
}
}

fn is_compatible_with(&self, peripheral: &impl PeripheralMarker) -> bool {
self.0.is_compatible_with(peripheral)
}
Expand Down Expand Up @@ -217,6 +223,12 @@ impl<C: PdmaChannel<RegisterBlock = SpiRegisterBlock>> RegisterAccess for SpiDma
.modify(|_, w| w.inlink_restart().set_bit());
}

fn set_check_owner(&self, check_owner: bool) {
if check_owner {
panic!("SPI DMA does not support checking descriptor ownership");
}
}

fn is_compatible_with(&self, peripheral: &impl PeripheralMarker) -> bool {
self.0.is_compatible_with(peripheral)
}
Expand Down Expand Up @@ -492,6 +504,13 @@ impl<C: PdmaChannel<RegisterBlock = I2sRegisterBlock>> RegisterAccess for I2sDma
.modify(|_, w| w.outlink_restart().set_bit());
}

fn set_check_owner(&self, check_owner: bool) {
let reg_block = self.0.register_block();
reg_block
.lc_conf()
.modify(|_, w| w.check_owner().bit(check_owner));
}

fn is_compatible_with(&self, peripheral: &impl PeripheralMarker) -> bool {
self.0.is_compatible_with(peripheral)
}
Expand Down Expand Up @@ -634,6 +653,13 @@ impl<C: PdmaChannel<RegisterBlock = I2sRegisterBlock>> RegisterAccess for I2sDma
.modify(|_, w| w.inlink_restart().set_bit());
}

fn set_check_owner(&self, check_owner: bool) {
let reg_block = self.0.register_block();
reg_block
.lc_conf()
.modify(|_, w| w.check_owner().bit(check_owner));
}

fn is_compatible_with(&self, peripheral: &impl PeripheralMarker) -> bool {
self.0.is_compatible_with(peripheral)
}
Expand Down
Loading