gh-raspberrypi/pico-sdk

Watch

Fork 0

dma_channel_abort does not wait for CHAN_ABORT to go zero #923

New issue

Open

opened 2022-07-17 11:43:13 -05:00 by alastairpatrick · 7 comments

alastairpatrick commented

2022-07-17 11:43:13 -05:00

(Migrated from github.com)

The source for dma_channel_abort from SDK 1.4.0:

static inline void dma_channel_abort(uint channel) {
    check_dma_channel_param(channel);
    dma_hw->abort = 1u << channel;
    // Bit will go 0 once channel has reached safe state
    // (i.e. any in-flight transfers have retired)
    while (dma_hw->ch[channel].ctrl_trig & DMA_CH0_CTRL_TRIG_BUSY_BITS) tight_loop_contents();
}

Note that while it waits for the channel's BUSY flag to go zero, it does not wait for CHAN_ABORT to go zero.

From the datasheet, emphasis mine:

DMA: CHAN_ABORT Register
Offset: 0x444
Description
Abort an in-progress transfer sequence on one or more channels

Each bit corresponds to a channel. Writing a 1 aborts whatever transfer sequence is in progress on that channel. The bit will remain high until any inflight transfers have been flushed through the address and data FIFOs.

After writing, this register must be polled until it returns all-zero. Until this point, it is unsafe to restart the channel.

Also from the datasheet:

At the time an abort is triggered, a channel may have bus transfers currently in flight between the read and write master,
and these transfers cannot be revoked. The CTRL.BUSY flag stays high until these transfers complete, and the channel
reaches a safe state, which generally takes only a few cycles. The channel must not be restarted until its CTRL.BUSY flag
deasserts. Starting a new sequence of transfers whilst transfers from an old sequence are still in flight can lead to
unpredictable behaviour.

The datasheet does not indicate whether it is necessary to wait for both BUSY to go zero and CHAN_ABORT to go zero or if waiting for either condition is sufficient. It also does not indicate in which order code should wait for them to go zero. As mentioned, dma_channel_abort only waits for BUSY to go zero.

I suggest there is either a bug in dma_channel_abort (if it ought to be waiting for both) or the datasheet would benefit from clarification.

The source for `dma_channel_abort` from SDK 1.4.0: ```c static inline void dma_channel_abort(uint channel) { check_dma_channel_param(channel); dma_hw->abort = 1u << channel; // Bit will go 0 once channel has reached safe state // (i.e. any in-flight transfers have retired) while (dma_hw->ch[channel].ctrl_trig & DMA_CH0_CTRL_TRIG_BUSY_BITS) tight_loop_contents(); } ``` Note that while it waits for the channel's `BUSY` flag to go zero, it does _not_ wait for `CHAN_ABORT` to go zero. From the datasheet, emphasis mine: > DMA: CHAN_ABORT Register > Offset: 0x444 > Description > Abort an in-progress transfer sequence on one or more channels > > Each bit corresponds to a channel. Writing a 1 aborts whatever transfer sequence is in progress on that channel. The bit will remain high until any inflight transfers have been flushed through the address and data FIFOs. > > **After writing, this register must be polled until it returns all-zero. Until this point, it is unsafe to restart the channel.** Also from the datasheet: > At the time an abort is triggered, a channel may have bus transfers currently in flight between the read and write master, > and these transfers cannot be revoked. The CTRL.BUSY flag stays high until these transfers complete, and the channel > reaches a safe state, which generally takes only a few cycles. **The channel must not be restarted until its CTRL.BUSY flag > deasserts.** Starting a new sequence of transfers whilst transfers from an old sequence are still in flight can lead to > unpredictable behaviour. The datasheet does not indicate whether it is necessary to wait for both `BUSY` to go zero _and_ `CHAN_ABORT` to go zero or if waiting for either condition is sufficient. It also does not indicate in which order code should wait for them to go zero. As mentioned, dma_channel_abort only waits for `BUSY` to go zero. I suggest there is either a bug in `dma_channel_abort` (if it ought to be waiting for both) or the datasheet would benefit from clarification.

lurch commented

2022-07-17 13:55:05 -05:00

(Migrated from github.com)

See also Errata RP2040-E13 in the RP2040 datasheet, if you haven't already.

See also Errata RP2040-E13 in the [RP2040 datasheet](https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf), if you haven't already.

alastairpatrick commented

2022-07-17 15:12:03 -05:00

(Migrated from github.com)

Given RP2040-E13, I agree that this is documentation.

The workaround recommends "Before aborting a channel, clear its interrupt enable. After aborting a channel, poll the CTRL.BUSY bit to wait for completion (not the ABORT bit), clear the spurious IRQ, and restore the interrupt enable."

I suggest this could be improved too. CHAN_ABORT can be used to abort multiple channels with a single write to the register. In fact this seems like the best way to abort multiple channels that trigger each other, with other methods likely to suffer from race conditions.

The workaround recommendation assumes that only a single channel is being aborted. It would be good to confirm that, even when multiple channels are aborted, it is adequate and necessary to check only the CTRL.BUSY bits for each channel in turn and not to check CHAN_ABORT. Without that clarification, it is not clear that checking only CTRL.BUSY is free of race conditions when aborting multiple channels.

Given RP2040-E13, I agree that this is documentation. The workaround recommends "Before aborting a channel, clear its interrupt enable. After aborting a channel, poll the CTRL.BUSY bit to wait for completion (not the ABORT bit), clear the spurious IRQ, and restore the interrupt enable." I suggest this could be improved too. CHAN_ABORT can be used to abort _multiple_ channels with a single write to the register. In fact this seems like the best way to abort multiple channels that trigger each other, with other methods likely to suffer from race conditions. The workaround recommendation assumes that only a _single_ channel is being aborted. It would be good to confirm that, even when _multiple_ channels are aborted, it is adequate and necessary to check only the CTRL.BUSY bits for each channel in turn and not to check CHAN_ABORT. Without that clarification, it is not clear that checking only CTRL.BUSY is free of race conditions when aborting multiple channels.

kilograham commented

2023-05-26 09:25:33 -05:00

(Migrated from github.com)

moving to 1.5.1 so it doesn't get forgotten (even if the docs aren't fixed there)

tommie commented

2024-04-29 09:35:35 -05:00

(Migrated from github.com)

In fact this seems like the best way to abort multiple channels that trigger each other, with other methods likely to suffer from race conditions.

Related, and for posterity, I have a DMA control+data setup, with the data channel re-starting control at the end, rather than issuing a null transfer, emulating a ring. This required me to do two CHAN_ABORT on both channels to ensure it stops. I'm thinking the DMA trigger register may already be in the DMA FIFO, making this racy even with CHAN_ABORT. To be safe, I have it in a loop:

while (dma_channel_is_busy(dma_ctrl) || dma_channel_is_busy(dma_data)) {
    dma_channel_abort_mask((1u << dma_ctrl) | (1u << dma_data));
}

> In fact this seems like the best way to abort multiple channels that trigger each other, with other methods likely to suffer from race conditions. Related, and for posterity, I have a DMA control+data setup, with the data channel re-starting control at the end, rather than issuing a null transfer, emulating a ring. This required me to do two CHAN_ABORT on both channels to ensure it stops. I'm thinking the DMA trigger register may already be in the DMA FIFO, making this racy even with CHAN_ABORT. To be safe, I have it in a loop: ```c while (dma_channel_is_busy(dma_ctrl) || dma_channel_is_busy(dma_data)) { dma_channel_abort_mask((1u << dma_ctrl) | (1u << dma_data)); } ```

👍 1

glopesdev commented

2025-05-28 04:42:44 -05:00

(Migrated from github.com)

@tommie I have the exact same problem and would like to try your approach, but coming up against dma_channel_abort_mask is undefined. A quick search turned out nothing, so I was wondering if this was your own local definition, or whether the API actually exists anywhere?

P.S.: For future reference, replacing this call with two single-channel dma_channel_abort calls also works, so leaving it for now.

@tommie I have the exact same problem and would like to try your approach, but coming up against `dma_channel_abort_mask` is undefined. A quick search turned out nothing, so I was wondering if this was your own local definition, or whether the API actually exists anywhere? P.S.: For future reference, replacing this call with two single-channel `dma_channel_abort` calls also works, so leaving it for now.

tommie commented

2025-05-28 06:34:32 -05:00

(Migrated from github.com)

@glopesdev Yepp, you're right.

// Aborts several DMA channels at once, and waits for their
// completion. If one channel is triggering another, it may be
// necessary to call this multiple times to resolve the race condition
// of a trigger register being loaded into the DMA FIFO already.
static inline void dma_channel_abort_mask(uint mask) {
  hw_set_bits(&dma_hw->abort, mask);

  while (dma_hw->abort & mask) {
    tight_loop_contents();
  }
}

@glopesdev Yepp, you're right. ```c // Aborts several DMA channels at once, and waits for their // completion. If one channel is triggering another, it may be // necessary to call this multiple times to resolve the race condition // of a trigger register being loaded into the DMA FIFO already. static inline void dma_channel_abort_mask(uint mask) { hw_set_bits(&dma_hw->abort, mask); while (dma_hw->abort & mask) { tight_loop_contents(); } } ```

abouatef commented

2026-04-21 17:05:35 -05:00

(Migrated from github.com)

Any updates on this yet ?, facing the same issue in 2.2.0 !
@kilograham

#include "stdio.h"
#include "pico/stdlib.h"
#include "hardware/dma.h"
#include "hardware/pio.h"
#include "hardware/clocks.h"

// Minimal PIO program — just pulls from TX FIFO and outputs to pins
// Equivalent to a 1-instruction waveform output loop
static const uint16_t pio_prog_instructions[] = {
    0x6008u  // out pins, 8
};

static const struct pio_program pio_prog = {
    .instructions = pio_prog_instructions,
    .length = 1,
    .origin = -1
};

// Test buffer — 256 bytes of arbitrary data
#define BUF_SIZE 256
static uint8_t s_buf[BUF_SIZE] __attribute__((aligned(4)));
static uint32_t s_loop_addr[1];

static PIO  s_pio;
static uint s_sm;
static uint s_prog_offset;
static int  s_dma_data;
static int  s_dma_ctrl;

static void setup_pio(void) {
    s_pio = pio0;
    bool ok = pio_claim_free_sm_and_add_program_for_gpio_range(
        &pio_prog, &s_pio, &s_sm, &s_prog_offset, 0, 8, true
    );
    hard_assert(ok);

    pio_sm_set_consecutive_pindirs(s_pio, s_sm, 0, 8, true);
    for (uint i = 0; i < 8; i++) pio_gpio_init(s_pio, i);

    pio_sm_config c = pio_get_default_sm_config();
    sm_config_set_wrap(&c, s_prog_offset, s_prog_offset);
    sm_config_set_out_pins(&c, 0, 8);
    sm_config_set_out_shift(&c, true, true, 32);
    sm_config_set_fifo_join(&c, PIO_FIFO_JOIN_TX);
    sm_config_set_clkdiv(&c, 36.0f);
    pio_sm_init(s_pio, s_sm, s_prog_offset, &c);
}

static void start_dma_loop(void) {
    // Data channel
    dma_channel_config cfg_data = dma_channel_get_default_config(s_dma_data);
    channel_config_set_transfer_data_size(&cfg_data, DMA_SIZE_32);
    channel_config_set_read_increment(&cfg_data, true);
    channel_config_set_write_increment(&cfg_data, false);
    channel_config_set_dreq(&cfg_data, pio_get_dreq(s_pio, s_sm, true));
    channel_config_set_chain_to(&cfg_data, s_dma_ctrl);

    dma_channel_configure(
        s_dma_data, &cfg_data,
        &s_pio->txf[s_sm],
        s_buf,
        BUF_SIZE / 4,
        false
    );

    // Control channel — reloads data channel read address
    dma_channel_config cfg_ctrl = dma_channel_get_default_config(s_dma_ctrl);
    channel_config_set_transfer_data_size(&cfg_ctrl, DMA_SIZE_32);
    channel_config_set_read_increment(&cfg_ctrl, false);
    channel_config_set_write_increment(&cfg_ctrl, false);
    channel_config_set_chain_to(&cfg_ctrl, s_dma_data);

    s_loop_addr[0] = (uint32_t)s_buf;

    dma_channel_configure(
        s_dma_ctrl, &cfg_ctrl,
        &dma_hw->ch[s_dma_data].read_addr,
        s_loop_addr,
        1,
        false
    );

    pio_sm_set_enabled(s_pio, s_sm, true);
    dma_channel_start(s_dma_data);
}

static void stop_dma_loop_broken(void) {
    // Method 1 — BROKEN on RP2350: abort does not reliably complete
    dma_channel_abort(s_dma_ctrl);
    dma_channel_abort(s_dma_data);
    pio_sm_set_enabled(s_pio, s_sm, false);
    pio_sm_clear_fifos(s_pio, s_sm);
    __compiler_memory_barrier();
}

static void stop_dma_loop_fixed(void) {
    // Method 2 — FIXED: clear enable bits before abort
    hw_clear_bits(&dma_hw->ch[s_dma_ctrl].al1_ctrl, DMA_CH0_CTRL_TRIG_EN_BITS);
    hw_clear_bits(&dma_hw->ch[s_dma_data].al1_ctrl, DMA_CH0_CTRL_TRIG_EN_BITS);
    dma_channel_abort(s_dma_ctrl);
    dma_channel_abort(s_dma_data);
    pio_sm_set_enabled(s_pio, s_sm, false);
    pio_sm_clear_fifos(s_pio, s_sm);
    __compiler_memory_barrier();
}

int main(void) {
    stdio_init_all();
    sleep_ms(5000);
    printf("RP2350 chained DMA abort test\n");

    // Fill buffer with test pattern
    for (int i = 0; i < BUF_SIZE; i++) s_buf[i] = (uint8_t)i;

    setup_pio();
    s_dma_data = dma_claim_unused_channel(true);
    s_dma_ctrl = dma_claim_unused_channel(true);

    printf("Testing broken method (dma_channel_abort only):\n");
    for (int iter = 0; iter < 5; iter++) {
        start_dma_loop();
        sleep_ms(10);  // let it run briefly

        stop_dma_loop_broken();

        bool data_busy = dma_channel_is_busy(s_dma_data);
        bool ctrl_busy = dma_channel_is_busy(s_dma_ctrl);
        printf("  iter %d: data_busy=%d ctrl_busy=%d %s\n",
               iter, data_busy, ctrl_busy,
               (data_busy || ctrl_busy) ? "<-- ABORT FAILED" : "ok");
    }

    printf("\nTesting fixed method (clear enable then abort):\n");
    for (int iter = 0; iter < 5; iter++) {
        start_dma_loop();
        sleep_ms(10);

        stop_dma_loop_fixed();

        bool data_busy = dma_channel_is_busy(s_dma_data);
        bool ctrl_busy = dma_channel_is_busy(s_dma_ctrl);
        printf("  iter %d: data_busy=%d ctrl_busy=%d %s\n",
               iter, data_busy, ctrl_busy,
               (data_busy || ctrl_busy) ? "<-- ABORT FAILED" : "ok");
    }

    printf("\nDone.\n");
    while (1) tight_loop_contents();
}

One more -maybe related- issue, is that changing any of the two stop_dma_loop() functions to try and disable the PIO before aborting the dma channels results in CPU getting completely stuck.

Any updates on this yet ?, facing the same issue in 2.2.0 ! @kilograham ```c #include "stdio.h" #include "pico/stdlib.h" #include "hardware/dma.h" #include "hardware/pio.h" #include "hardware/clocks.h" // Minimal PIO program — just pulls from TX FIFO and outputs to pins // Equivalent to a 1-instruction waveform output loop static const uint16_t pio_prog_instructions[] = { 0x6008u // out pins, 8 }; static const struct pio_program pio_prog = { .instructions = pio_prog_instructions, .length = 1, .origin = -1 }; // Test buffer — 256 bytes of arbitrary data #define BUF_SIZE 256 static uint8_t s_buf[BUF_SIZE] __attribute__((aligned(4))); static uint32_t s_loop_addr[1]; static PIO s_pio; static uint s_sm; static uint s_prog_offset; static int s_dma_data; static int s_dma_ctrl; static void setup_pio(void) { s_pio = pio0; bool ok = pio_claim_free_sm_and_add_program_for_gpio_range( &pio_prog, &s_pio, &s_sm, &s_prog_offset, 0, 8, true ); hard_assert(ok); pio_sm_set_consecutive_pindirs(s_pio, s_sm, 0, 8, true); for (uint i = 0; i < 8; i++) pio_gpio_init(s_pio, i); pio_sm_config c = pio_get_default_sm_config(); sm_config_set_wrap(&c, s_prog_offset, s_prog_offset); sm_config_set_out_pins(&c, 0, 8); sm_config_set_out_shift(&c, true, true, 32); sm_config_set_fifo_join(&c, PIO_FIFO_JOIN_TX); sm_config_set_clkdiv(&c, 36.0f); pio_sm_init(s_pio, s_sm, s_prog_offset, &c); } static void start_dma_loop(void) { // Data channel dma_channel_config cfg_data = dma_channel_get_default_config(s_dma_data); channel_config_set_transfer_data_size(&cfg_data, DMA_SIZE_32); channel_config_set_read_increment(&cfg_data, true); channel_config_set_write_increment(&cfg_data, false); channel_config_set_dreq(&cfg_data, pio_get_dreq(s_pio, s_sm, true)); channel_config_set_chain_to(&cfg_data, s_dma_ctrl); dma_channel_configure( s_dma_data, &cfg_data, &s_pio->txf[s_sm], s_buf, BUF_SIZE / 4, false ); // Control channel — reloads data channel read address dma_channel_config cfg_ctrl = dma_channel_get_default_config(s_dma_ctrl); channel_config_set_transfer_data_size(&cfg_ctrl, DMA_SIZE_32); channel_config_set_read_increment(&cfg_ctrl, false); channel_config_set_write_increment(&cfg_ctrl, false); channel_config_set_chain_to(&cfg_ctrl, s_dma_data); s_loop_addr[0] = (uint32_t)s_buf; dma_channel_configure( s_dma_ctrl, &cfg_ctrl, &dma_hw->ch[s_dma_data].read_addr, s_loop_addr, 1, false ); pio_sm_set_enabled(s_pio, s_sm, true); dma_channel_start(s_dma_data); } static void stop_dma_loop_broken(void) { // Method 1 — BROKEN on RP2350: abort does not reliably complete dma_channel_abort(s_dma_ctrl); dma_channel_abort(s_dma_data); pio_sm_set_enabled(s_pio, s_sm, false); pio_sm_clear_fifos(s_pio, s_sm); __compiler_memory_barrier(); } static void stop_dma_loop_fixed(void) { // Method 2 — FIXED: clear enable bits before abort hw_clear_bits(&dma_hw->ch[s_dma_ctrl].al1_ctrl, DMA_CH0_CTRL_TRIG_EN_BITS); hw_clear_bits(&dma_hw->ch[s_dma_data].al1_ctrl, DMA_CH0_CTRL_TRIG_EN_BITS); dma_channel_abort(s_dma_ctrl); dma_channel_abort(s_dma_data); pio_sm_set_enabled(s_pio, s_sm, false); pio_sm_clear_fifos(s_pio, s_sm); __compiler_memory_barrier(); } int main(void) { stdio_init_all(); sleep_ms(5000); printf("RP2350 chained DMA abort test\n"); // Fill buffer with test pattern for (int i = 0; i < BUF_SIZE; i++) s_buf[i] = (uint8_t)i; setup_pio(); s_dma_data = dma_claim_unused_channel(true); s_dma_ctrl = dma_claim_unused_channel(true); printf("Testing broken method (dma_channel_abort only):\n"); for (int iter = 0; iter < 5; iter++) { start_dma_loop(); sleep_ms(10); // let it run briefly stop_dma_loop_broken(); bool data_busy = dma_channel_is_busy(s_dma_data); bool ctrl_busy = dma_channel_is_busy(s_dma_ctrl); printf(" iter %d: data_busy=%d ctrl_busy=%d %s\n", iter, data_busy, ctrl_busy, (data_busy || ctrl_busy) ? "<-- ABORT FAILED" : "ok"); } printf("\nTesting fixed method (clear enable then abort):\n"); for (int iter = 0; iter < 5; iter++) { start_dma_loop(); sleep_ms(10); stop_dma_loop_fixed(); bool data_busy = dma_channel_is_busy(s_dma_data); bool ctrl_busy = dma_channel_is_busy(s_dma_ctrl); printf(" iter %d: data_busy=%d ctrl_busy=%d %s\n", iter, data_busy, ctrl_busy, (data_busy || ctrl_busy) ? "<-- ABORT FAILED" : "ok"); } printf("\nDone.\n"); while (1) tight_loop_contents(); } ``` One more -maybe related- issue, is that changing any of the two stop_dma_loop() functions to try and disable the PIO before aborting the dma channels results in CPU getting completely stuck.