diff --git a/components/esp_driver_i2s/i2s_common.c b/components/esp_driver_i2s/i2s_common.c index bcf15a342afa..c739dcb538ed 100644 --- a/components/esp_driver_i2s/i2s_common.c +++ b/components/esp_driver_i2s/i2s_common.c @@ -573,6 +573,62 @@ uint32_t i2s_get_source_clk_freq(i2s_clock_src_t clk_src, uint32_t mclk_freq_hz) return clk_freq; } +/** + * @brief Track which TX buffer the DMA is sending + * @note Called from the TX EOF interrupt. When `finish_desc` is done, the DMA has moved on to the + * next descriptor in the ring. If either of them holds the buffer that the writer is filling, + * data written to that buffer later would be dropped or sent out of order. + */ +static void IRAM_ATTR i2s_tx_update_dma_ptr(i2s_chan_handle_t handle, lldesc_t *finish_desc) +{ + void *finished_buf = (void *)finish_desc->buf; + void *started_buf = (void *)STAILQ_NEXT(finish_desc, qe)->buf; + portENTER_CRITICAL_ISR(&g_i2s.spinlock); + handle->dma.dma_ptr = started_buf; + if (handle->dma.curr_ptr == finished_buf || handle->dma.curr_ptr == started_buf) { + handle->dma.curr_started = true; + } + portEXIT_CRITICAL_ISR(&g_i2s.spinlock); +} + +/** + * @brief Set the TX buffer that the DMA is sending, outside of the EOF interrupt + */ +static void i2s_tx_set_dma_ptr(i2s_chan_handle_t handle, void *buf) +{ + portENTER_CRITICAL(&g_i2s.spinlock); + handle->dma.dma_ptr = buf; + if (buf != NULL && handle->dma.curr_ptr == buf) { + handle->dma.curr_started = true; + } + portEXIT_CRITICAL(&g_i2s.spinlock); +} + +/** + * @brief Make `buf` the TX buffer that is being filled + * @note The DMA may have started sending `buf` already: the writer can take the next buffer of + * the ring from the queue just before the EOF interrupt that moves the DMA to it. + */ +static void i2s_tx_set_curr_ptr(i2s_chan_handle_t handle, void *buf) +{ + portENTER_CRITICAL(&g_i2s.spinlock); + handle->dma.curr_ptr = buf; + handle->dma.curr_started = buf != NULL && buf == handle->dma.dma_ptr; + portEXIT_CRITICAL(&g_i2s.spinlock); + handle->dma.rw_pos = 0; +} + +/** + * @brief Whether the DMA has started sending the TX buffer that is being filled + */ +static bool i2s_tx_curr_started(i2s_chan_handle_t handle) +{ + portENTER_CRITICAL(&g_i2s.spinlock); + bool started = handle->dma.curr_started; + portEXIT_CRITICAL(&g_i2s.spinlock); + return started; +} + /* Temporary ignore the deprecated warning of i2s_event_data_t::data */ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" @@ -622,6 +678,7 @@ static bool IRAM_ATTR i2s_dma_tx_callback(gdma_channel_handle_t dma_chan, gdma_e finish_desc = (lldesc_t *)event_data->tx_eof_desc_addr; void *curr_buf = (void *)finish_desc->buf; + i2s_tx_update_dma_ptr(handle, finish_desc); i2s_event_data_t evt = { .data = &(finish_desc->buf), .dma_buf = curr_buf, @@ -719,6 +776,7 @@ static void IRAM_ATTR i2s_dma_tx_callback(void *arg) if (handle && (status & I2S_LL_EVENT_TX_EOF)) { i2s_hal_get_out_eof_des_addr(&(handle->controller->hal), (uint32_t *)&finish_desc); void *curr_buf = (void *)finish_desc->buf; + i2s_tx_update_dma_ptr(handle, finish_desc); evt.data = &(finish_desc->buf); evt.dma_buf = curr_buf; evt.size = handle->dma.buf_size; @@ -802,6 +860,15 @@ esp_err_t i2s_init_dma_intr(i2s_chan_handle_t handle, int intr_flag) /* Register a new GDMA tx channel */ ESP_RETURN_ON_ERROR(gdma_new_ahb_channel(&dma_cfg, &handle->dma.dma_chan), TAG, "Register tx dma channel error"); ESP_GOTO_ON_ERROR(gdma_connect(handle->dma.dma_chan, trig), err1, TAG, "Connect tx dma channel error"); + /* Raise the EOF event once the DMA has read the whole buffer, not once the data has left the DMA + * FIFO. By then the DMA has already prefetched the start of the next buffer, so the EOF would + * come too late to tell the writer that it can no longer append to that buffer. */ + gdma_strategy_config_t strategy = { + .owner_check = false, + .auto_update_desc = false, + .eof_till_data_popped = false, + }; + ESP_GOTO_ON_ERROR(gdma_apply_strategy(handle->dma.dma_chan, &strategy), err2, TAG, "Apply tx dma strategy failed"); gdma_tx_event_callbacks_t cb = {.on_trans_eof = i2s_dma_tx_callback}; /* Set callback function for GDMA, the interrupt is triggered by GDMA, then the GDMA ISR will call the callback function */ ESP_GOTO_ON_ERROR(gdma_register_tx_event_callbacks(handle->dma.dma_chan, &cb, handle), err2, TAG, "Register tx callback failed"); @@ -818,6 +885,9 @@ esp_err_t i2s_init_dma_intr(i2s_chan_handle_t handle, int intr_flag) intr_flag |= handle->intr_prio_flags; /* Initialize I2S module interrupt */ if (handle->dir == I2S_DIR_TX) { + /* Raise the EOF interrupt once the DMA has read the whole buffer, not once the data has left the + * FIFO. See the GDMA strategy above. */ + i2s_ll_dma_enable_eof_on_fifo_empty(handle->controller->hal.dev, false); esp_intr_alloc_intrstatus(i2s_periph_signal[port_id].irq, intr_flag, (uint32_t)i2s_ll_get_interrupt_status_reg(handle->controller->hal.dev), I2S_LL_TX_EVENT_MASK, i2s_dma_tx_callback, handle, &handle->dma.dma_chan); @@ -1172,6 +1242,10 @@ esp_err_t i2s_channel_enable(i2s_chan_handle_t handle) #if CONFIG_PM_ENABLE esp_pm_lock_acquire(handle->pm_lock); #endif + if (handle->dir == I2S_DIR_TX) { + /* The DMA starts with the first buffer, which preload may have left partially filled */ + i2s_tx_set_dma_ptr(handle, (void *)handle->dma.desc[0]->buf); + } handle->start(handle); handle->state = I2S_CHAN_STATE_RUNNING; if (handle->dir == I2S_DIR_RX) { @@ -1206,9 +1280,11 @@ esp_err_t i2s_channel_disable(i2s_chan_handle_t handle) xSemaphoreTake(handle->binary, portMAX_DELAY); /* Reset the descriptor pointer */ handle->dma.curr_ptr = NULL; + handle->dma.curr_started = false; handle->dma.rw_pos = 0; handle->stop(handle); if (handle->dir == I2S_DIR_TX) { + i2s_tx_set_dma_ptr(handle, NULL); /* TX queue is reset when the channel is disabled In case the queue is wrongly reset after preload the data */ xQueueReset(handle->msg_queue); @@ -1246,17 +1322,17 @@ esp_err_t i2s_channel_preload_data(i2s_chan_handle_t tx_handle, const void *src, ESP_GOTO_ON_FALSE(xQueueSend(tx_handle->msg_queue, &(tx_handle->dma.desc[i]->buf), 0) == pdTRUE, ESP_FAIL, err, TAG, "Failed to push the descriptor to the queue"); } - tx_handle->dma.curr_ptr = (void *)tx_handle->dma.desc[0]->buf; - tx_handle->dma.rw_pos = 0; + i2s_tx_set_curr_ptr(tx_handle, (void *)tx_handle->dma.desc[0]->buf); } /* Loop until no bytes in source buff remain or the descriptors are full */ while (remain_bytes) { if (tx_handle->dma.rw_pos == tx_handle->dma.buf_size) { - if (xQueueReceive(tx_handle->msg_queue, &(tx_handle->dma.curr_ptr), 0) == pdFALSE) { + void *next_buf; + if (xQueueReceive(tx_handle->msg_queue, &next_buf, 0) == pdFALSE) { break; } - tx_handle->dma.rw_pos = 0; + i2s_tx_set_curr_ptr(tx_handle, next_buf); } size_t bytes_can_load = remain_bytes > (tx_handle->dma.buf_size - tx_handle->dma.rw_pos) ? (tx_handle->dma.buf_size - tx_handle->dma.rw_pos) : remain_bytes; @@ -1299,12 +1375,26 @@ esp_err_t i2s_channel_write(i2s_chan_handle_t handle, const void *src, size_t si ESP_RETURN_ON_FALSE(xSemaphoreTake(handle->binary, pdMS_TO_TICKS(timeout_ms)) == pdTRUE, ESP_ERR_INVALID_STATE, TAG, "The channel is not enabled"); src_byte = (char *)src; while (size > 0 && handle->state == I2S_CHAN_STATE_RUNNING) { - if (handle->dma.rw_pos == handle->dma.buf_size || handle->dma.curr_ptr == NULL) { - if (xQueueReceive(handle->msg_queue, &(handle->dma.curr_ptr), pdMS_TO_TICKS(timeout_ms)) == pdFALSE) { + /* Don't write to a buffer that the DMA has started sending. For example, the previous write + * may have left the buffer partially filled, and the channel ran out of data since. Data + * written to it would be dropped or sent out of order. Leave the rest of that buffer as is + * (silent when auto clear is enabled) and continue with the next free buffer. */ + if (handle->dma.rw_pos == handle->dma.buf_size || handle->dma.curr_ptr == NULL || i2s_tx_curr_started(handle)) { + void *next_buf; + /* Wait without removing a buffer: a task switch after dequeue could let the DMA + * cycle through and requeue it before the writer records it as current. */ + if (xQueuePeek(handle->msg_queue, &next_buf, pdMS_TO_TICKS(timeout_ms)) == pdFALSE) { ret = ESP_ERR_TIMEOUT; break; } - handle->dma.rw_pos = 0; + vTaskSuspendAll(); + /* The ISR may have changed the queue since the peek. Only adopt a buffer that + * we actually dequeue now, and never block with the scheduler suspended. */ + if (xQueueReceive(handle->msg_queue, &next_buf, 0) == pdTRUE) { + i2s_tx_set_curr_ptr(handle, next_buf); + } + xTaskResumeAll(); + continue; } data_ptr = (char *)handle->dma.curr_ptr; data_ptr += handle->dma.rw_pos; @@ -1312,10 +1402,24 @@ esp_err_t i2s_channel_write(i2s_chan_handle_t handle, const void *src, size_t si if (bytes_can_write > size) { bytes_can_write = size; } - memcpy(data_ptr, src_byte, bytes_can_write); + /* Check again and copy without being preempted in between. The DMA sends at the sample rate, + * so a copy that starts before the DMA has reached `rw_pos` stays ahead of it. */ + vTaskSuspendAll(); + bool started = i2s_tx_curr_started(handle); + if (!started) { + memcpy(data_ptr, src_byte, bytes_can_write); #if SOC_CACHE_INTERNAL_MEM_VIA_L1CACHE - esp_cache_msync(handle->dma.curr_ptr, handle->dma.buf_size, ESP_CACHE_MSYNC_FLAG_DIR_C2M); + /* Publish to DMA before allowing task preemption. Critical context also prevents + * the optional chunked cache-sync implementation from taking a blocking mutex. */ + portENTER_CRITICAL(&g_i2s.spinlock); + esp_cache_msync(handle->dma.curr_ptr, handle->dma.buf_size, ESP_CACHE_MSYNC_FLAG_DIR_C2M); + portEXIT_CRITICAL(&g_i2s.spinlock); #endif + } + xTaskResumeAll(); + if (started) { + continue; + } size -= bytes_can_write; src_byte += bytes_can_write; handle->dma.rw_pos += bytes_can_write; diff --git a/components/esp_driver_i2s/i2s_private.h b/components/esp_driver_i2s/i2s_private.h index 792dfe2196eb..621ef5f52e63 100644 --- a/components/esp_driver_i2s/i2s_private.h +++ b/components/esp_driver_i2s/i2s_private.h @@ -121,6 +121,8 @@ typedef struct { bool auto_clear_before_cb; /*!< Set to auto clear DMA TX descriptor before callback, i2s will always send zero automatically if no data to send */ uint32_t rw_pos; /*!< reading/writing pointer position */ void *curr_ptr; /*!< Pointer to current dma buffer */ + bool curr_started; /*!< TX only: the DMA has started sending curr_ptr, so no more data can be written to it */ + void *dma_ptr; /*!< TX only: the buffer that the DMA is sending */ lldesc_t **desc; /*!< dma descriptor array */ uint8_t **bufs; /*!< dma buffer array */ } i2s_dma_t; diff --git a/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt b/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt index d90f51c9f47e..b5fb4129a5a8 100644 --- a/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt +++ b/components/esp_driver_i2s/test_apps/i2s/main/CMakeLists.txt @@ -1,5 +1,6 @@ set(srcs "test_app_main.c" "test_i2s.c" + "test_i2s_tx_timeout.c" "test_i2s_iram.c" "test_i2s_sleep.c") @@ -8,5 +9,7 @@ if(CONFIG_SOC_I2S_SUPPORTS_ETM AND CONFIG_SOC_GPIO_SUPPORT_ETM) endif() idf_component_register(SRCS ${srcs} - PRIV_REQUIRES unity esp_driver_pcnt spi_flash esp_driver_gpio esp_driver_i2s esp_driver_uart + PRIV_INCLUDE_DIRS "../../.." + PRIV_REQUIRES unity esp_driver_pcnt spi_flash esp_driver_gpio + esp_driver_i2s esp_driver_uart esp_pm WHOLE_ARCHIVE) diff --git a/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c b/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c new file mode 100644 index 000000000000..dd6d59172cff --- /dev/null +++ b/components/esp_driver_i2s/test_apps/i2s/main/test_i2s_tx_timeout.c @@ -0,0 +1,80 @@ +// Copyright (C) 2026 Toit contributors. +// SPDX-FileCopyrightText: 2026 Toit contributors +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#include "driver/i2s_std.h" +#include "esp_rom_gpio.h" +#include "soc/gpio_sig_map.h" +#include "soc/i2s_periph.h" +#include "unity.h" +#include "i2s_private.h" + +/* Hold the slave clocks low so there are no EOFs and the free queue cannot + * refill. No GPIO is driven. This makes partial writes and timeouts deterministic + * without assuming a particular interrupt latency or requiring loopback wiring. */ +TEST_CASE("I2S TX timeout preserves an in-flight tail", "[i2s]") +{ + for (unsigned clear_mode = 0; clear_mode < 3; clear_mode++) { + i2s_chan_config_t cfg = I2S_CHANNEL_DEFAULT_CONFIG(I2S_NUM_AUTO, I2S_ROLE_SLAVE); + cfg.dma_desc_num = 2; + cfg.dma_frame_num = 240; + cfg.auto_clear_before_cb = clear_mode == 1; + cfg.auto_clear_after_cb = clear_mode == 2; + i2s_chan_handle_t tx = NULL; + TEST_ESP_OK(i2s_new_channel(&cfg, &tx, NULL)); + i2s_std_config_t std = { + .clk_cfg = I2S_STD_CLK_DEFAULT_CONFIG(10000), + .slot_cfg = I2S_STD_MSB_SLOT_DEFAULT_CONFIG(I2S_DATA_BIT_WIDTH_16BIT, I2S_SLOT_MODE_STEREO), + .gpio_cfg = { + .mclk = I2S_GPIO_UNUSED, .bclk = I2S_GPIO_UNUSED, + .ws = I2S_GPIO_UNUSED, .dout = I2S_GPIO_UNUSED, .din = I2S_GPIO_UNUSED, + }, + }; + TEST_ESP_OK(i2s_channel_init_std_mode(tx, &std)); + int port = tx->controller->id; + esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ZERO_INPUT, i2s_periph_signal[port].s_tx_bck_sig, false); + esp_rom_gpio_connect_in_signal(GPIO_MATRIX_CONST_ZERO_INPUT, i2s_periph_signal[port].s_tx_ws_sig, false); + + size_t size = tx->dma.buf_size; + uint8_t *old = malloc(2 * size); + uint8_t *next = malloc(size + 4); + TEST_ASSERT_NOT_NULL(old); + TEST_ASSERT_NOT_NULL(next); + memset(old, 0xa5, 2 * size); + memset(next, 0x3c, size + 4); + size_t loaded = 0; + TEST_ESP_OK(i2s_channel_preload_data(tx, old, 2 * size, &loaded)); + TEST_ASSERT_EQUAL_UINT32(2 * size, loaded); + TEST_ESP_OK(i2s_channel_enable(tx)); + TEST_ESP_OK(i2s_channel_disable(tx)); + + /* Reset retains DMA storage. Partially preload descriptor 0 again; + * its unwritten tail represents data from an earlier ring pass. */ + TEST_ESP_OK(i2s_channel_preload_data(tx, next, 4, &loaded)); + TEST_ASSERT_EQUAL_UINT32(4, loaded); + TEST_ESP_OK(i2s_channel_enable(tx)); + + size_t written = SIZE_MAX; + TEST_ASSERT_EQUAL(ESP_ERR_TIMEOUT, i2s_channel_write(tx, next, size + 4, &written, 0)); + TEST_ASSERT_EQUAL_UINT32(size, written); + TEST_ASSERT_EQUAL_MEMORY(next, tx->dma.desc[0]->buf, 4); + TEST_ASSERT_EQUAL_MEMORY(old + 4, (const uint8_t *)tx->dma.desc[0]->buf + 4, size - 4); + TEST_ASSERT_EQUAL_MEMORY(next, tx->dma.desc[1]->buf, size); + + /* A subsequent timed-out call must reset bytes_written to zero and + * must not overwrite either buffer. Auto-clear only runs at EOF. */ + written = SIZE_MAX; + TEST_ASSERT_EQUAL(ESP_ERR_TIMEOUT, i2s_channel_write(tx, old, 4, &written, 20)); + TEST_ASSERT_EQUAL_UINT32(0, written); + TEST_ASSERT_EQUAL_MEMORY(next, tx->dma.desc[0]->buf, 4); + TEST_ASSERT_EQUAL_MEMORY(old + 4, (const uint8_t *)tx->dma.desc[0]->buf + 4, size - 4); + TEST_ASSERT_EQUAL_MEMORY(next, tx->dma.desc[1]->buf, size); + TEST_ESP_OK(i2s_channel_disable(tx)); + TEST_ESP_OK(i2s_del_channel(tx)); + free(next); + free(old); + printf("TX timeout accounting and untouched tail: clear_mode=%u PASS\n", clear_mode); + } +}