From 28db901d77085efa8e9458226f39cf23da4d1f55 Mon Sep 17 00:00:00 2001 From: morris Date: Tue, 9 Jan 2024 10:12:29 +0800 Subject: [PATCH 1/3] fix(mcp): align the descriptor to cache line size for DMA memory copy --- .../esp_hw_support/dma/async_memcpy_gdma.c | 16 ++++++++++------ .../test_apps/dma/main/test_async_memcpy.c | 4 ++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/components/esp_hw_support/dma/async_memcpy_gdma.c b/components/esp_hw_support/dma/async_memcpy_gdma.c index eacb73b29d..8b8fa19930 100644 --- a/components/esp_hw_support/dma/async_memcpy_gdma.c +++ b/components/esp_hw_support/dma/async_memcpy_gdma.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2020-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2020-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -68,8 +68,9 @@ typedef struct async_memcpy_transaction_t { /// @note - Number of transaction objects are determined by the backlog parameter typedef struct { async_memcpy_context_t parent; // Parent IO interface - size_t sram_trans_align; // DMA transfer alignment (both in size and address) for SRAM memory - size_t psram_trans_align; // DMA transfer alignment (both in size and address) for PSRAM memory + size_t descriptor_align; // DMA descriptor alignment + size_t sram_trans_align; // DMA buffer alignment (both in size and address) for SRAM memory + size_t psram_trans_align; // DMA buffer alignment (both in size and address) for PSRAM memory size_t max_single_dma_buffer; // max DMA buffer size by a single descriptor int gdma_bus_id; // GDMA bus id (AHB, AXI, etc.) gdma_channel_handle_t tx_channel; // GDMA TX channel handle @@ -117,9 +118,12 @@ static esp_err_t esp_async_memcpy_install_gdma_template(const async_memcpy_confi ESP_GOTO_ON_FALSE(mcp_gdma, ESP_ERR_NO_MEM, err, TAG, "no mem for driver context"); uint32_t trans_queue_len = config->backlog ? config->backlog : DEFAULT_TRANSACTION_QUEUE_LENGTH; // allocate memory for transaction pool - mcp_gdma->transaction_pool = heap_caps_aligned_calloc(MCP_DMA_DESC_ALIGN, trans_queue_len, sizeof(async_memcpy_transaction_t), + uint32_t data_cache_line_size = cache_hal_get_cache_line_size(CACHE_LL_LEVEL_INT_MEM, CACHE_TYPE_DATA); + uint32_t alignment = MAX(data_cache_line_size, MCP_DMA_DESC_ALIGN); + mcp_gdma->transaction_pool = heap_caps_aligned_calloc(alignment, trans_queue_len, sizeof(async_memcpy_transaction_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT | MALLOC_CAP_DMA); ESP_GOTO_ON_FALSE(mcp_gdma->transaction_pool, ESP_ERR_NO_MEM, err, TAG, "no mem for transaction pool"); + mcp_gdma->descriptor_align = alignment; // create TX channel and RX channel, they should reside in the same DMA pair gdma_channel_alloc_config_t tx_alloc_config = { @@ -387,13 +391,13 @@ static esp_err_t mcp_gdma_memcpy(async_memcpy_context_t *ctx, void *dst, void *s size_t max_single_dma_buffer = mcp_gdma->max_single_dma_buffer; uint32_t num_desc_per_path = (n + max_single_dma_buffer - 1) / max_single_dma_buffer; // allocate DMA descriptors, descriptors need a strict alignment - trans->tx_desc_link = heap_caps_aligned_calloc(MCP_DMA_DESC_ALIGN, num_desc_per_path, sizeof(mcp_dma_descriptor_t), + trans->tx_desc_link = heap_caps_aligned_calloc(mcp_gdma->descriptor_align, num_desc_per_path, sizeof(mcp_dma_descriptor_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT | MALLOC_CAP_DMA); ESP_GOTO_ON_FALSE(trans->tx_desc_link, ESP_ERR_NO_MEM, err, TAG, "no mem for DMA descriptors"); trans->tx_desc_nc = (mcp_dma_descriptor_t *)MCP_GET_NON_CACHE_ADDR(trans->tx_desc_link); // don't have to allocate the EOF descriptor, we will use trans->eof_node as the RX EOF descriptor if (num_desc_per_path > 1) { - trans->rx_desc_link = heap_caps_aligned_calloc(MCP_DMA_DESC_ALIGN, num_desc_per_path - 1, sizeof(mcp_dma_descriptor_t), + trans->rx_desc_link = heap_caps_aligned_calloc(mcp_gdma->descriptor_align, num_desc_per_path - 1, sizeof(mcp_dma_descriptor_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT | MALLOC_CAP_DMA); ESP_GOTO_ON_FALSE(trans->rx_desc_link, ESP_ERR_NO_MEM, err, TAG, "no mem for DMA descriptors"); trans->rx_desc_nc = (mcp_dma_descriptor_t *)MCP_GET_NON_CACHE_ADDR(trans->rx_desc_link); diff --git a/components/esp_hw_support/test_apps/dma/main/test_async_memcpy.c b/components/esp_hw_support/test_apps/dma/main/test_async_memcpy.c index 9c0236a5a4..8a166b3be6 100644 --- a/components/esp_hw_support/test_apps/dma/main/test_async_memcpy.c +++ b/components/esp_hw_support/test_apps/dma/main/test_async_memcpy.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2021-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2021-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -274,7 +274,7 @@ TEST_CASE("memory copy by DMA on the fly", "[async mcp]") TEST_ESP_OK(esp_async_memcpy_uninstall(driver)); } -#define TEST_ASYNC_MEMCPY_BENCH_COUNTS (16) +#define TEST_ASYNC_MEMCPY_BENCH_COUNTS (8) static int s_count = 0; static IRAM_ATTR bool test_async_memcpy_isr_cb(async_memcpy_handle_t mcp_hdl, async_memcpy_event_t *event, void *cb_args) From 467dce2ffc7ab94c004d94922b48b21d206553b2 Mon Sep 17 00:00:00 2001 From: morris Date: Fri, 22 Dec 2023 12:23:17 +0800 Subject: [PATCH 2/3] change(dw_gdma): add a case to test memset from psram to sram --- .../test_apps/.build-test-rules.yml | 4 +- .../esp_hw_support/test_apps/dma/README.md | 4 +- .../test_apps/dma/main/test_dw_gdma.c | 80 ++++++++++++++++++- .../test_apps/dma/sdkconfig.defaults.esp32p4 | 3 + 4 files changed, 86 insertions(+), 5 deletions(-) create mode 100644 components/esp_hw_support/test_apps/dma/sdkconfig.defaults.esp32p4 diff --git a/components/esp_hw_support/test_apps/.build-test-rules.yml b/components/esp_hw_support/test_apps/.build-test-rules.yml index 6d22ad8850..b8118048f0 100644 --- a/components/esp_hw_support/test_apps/.build-test-rules.yml +++ b/components/esp_hw_support/test_apps/.build-test-rules.yml @@ -1,10 +1,10 @@ # Documentation: .gitlab/ci/README.md#manifest-file-to-control-the-buildtest-apps components/esp_hw_support/test_apps/dma: - disable_test: + disable: - if: IDF_TARGET in ["esp32"] temporary: false - reason: Neither GDMA nor CPDMA is supported on ESP32 + reason: No general DMA controller on ESP32 components/esp_hw_support/test_apps/esp_hw_support_unity_tests: disable: diff --git a/components/esp_hw_support/test_apps/dma/README.md b/components/esp_hw_support/test_apps/dma/README.md index bf47d80ec6..4fc69c16b6 100644 --- a/components/esp_hw_support/test_apps/dma/README.md +++ b/components/esp_hw_support/test_apps/dma/README.md @@ -1,2 +1,2 @@ -| Supported Targets | ESP32 | ESP32-C2 | ESP32-C3 | ESP32-C6 | ESP32-H2 | ESP32-P4 | ESP32-S2 | ESP32-S3 | -| ----------------- | ----- | -------- | -------- | -------- | -------- | -------- | -------- | -------- | +| Supported Targets | ESP32-C2 | ESP32-C3 | ESP32-C6 | ESP32-H2 | ESP32-P4 | ESP32-S2 | ESP32-S3 | +| ----------------- | -------- | -------- | -------- | -------- | -------- | -------- | -------- | diff --git a/components/esp_hw_support/test_apps/dma/main/test_dw_gdma.c b/components/esp_hw_support/test_apps/dma/main/test_dw_gdma.c index 3ffe52c112..4da9060d3b 100644 --- a/components/esp_hw_support/test_apps/dma/main/test_dw_gdma.c +++ b/components/esp_hw_support/test_apps/dma/main/test_dw_gdma.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -12,6 +12,7 @@ #include "esp_private/dw_gdma.h" #include "hal/dw_gdma_ll.h" #include "esp_cache.h" +#include "esp_private/esp_cache_private.h" TEST_CASE("DW_GDMA channel allocation", "[DW_GDMA]") { @@ -499,6 +500,8 @@ TEST_CASE("DW_GDMA M2M Test: Link-List Mode", "[DW_GDMA]") TEST_ESP_OK(dw_gdma_channel_enable_ctrl(m2m_chan, true)); TEST_ASSERT_EQUAL(pdTRUE, xSemaphoreTake(done_sem, pdMS_TO_TICKS(1000))); + // should only go into the block invalid callback for once + TEST_ASSERT_EQUAL_UINT8(1, user_data.count); printf("check the memory copy result\r\n"); #if CONFIG_IDF_TARGET_ESP32P4 @@ -515,3 +518,78 @@ TEST_CASE("DW_GDMA M2M Test: Link-List Mode", "[DW_GDMA]") free(dst_buf); vSemaphoreDelete(done_sem); } + +TEST_CASE("DW_GDMA M2M Test: memory set with fixed address", "[DW_GDMA]") +{ + printf("prepare the source and destination buffers\r\n"); + // memset: source in psram and destination in sram + size_t ext_mem_alignment = 0; + size_t int_mem_alignment = 0; + TEST_ESP_OK(esp_cache_get_alignment(ESP_CACHE_MALLOC_FLAG_PSRAM, &ext_mem_alignment)); + TEST_ESP_OK(esp_cache_get_alignment(0, &int_mem_alignment)); + uint8_t *src_buf = heap_caps_aligned_calloc(ext_mem_alignment, 1, 256, MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT); + uint8_t *dst_buf = heap_caps_aligned_calloc(int_mem_alignment, 1, 256, MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); + TEST_ASSERT_NOT_NULL(src_buf); + TEST_ASSERT_NOT_NULL(dst_buf); + // prepare the source buffer, only the first byte has a non-zero value + for (int i = 0; i < 256; i++) { + src_buf[i] = 0; + } + src_buf[0] = 66; +#if CONFIG_IDF_TARGET_ESP32P4 + // do write-back for the source data because it's in the cache + TEST_ESP_OK(esp_cache_msync((void *)src_buf, 256, ESP_CACHE_MSYNC_FLAG_DIR_C2M)); +#endif + + printf("allocate a channel for memory set\r\n"); + dw_gdma_channel_static_config_t static_config = { + .block_transfer_type = DW_GDMA_BLOCK_TRANSFER_CONTIGUOUS, + .role = DW_GDMA_ROLE_MEM, + .num_outstanding_requests = 1, + }; + dw_gdma_channel_alloc_config_t alloc_config = { + .src = static_config, + .dst = static_config, + .flow_controller = DW_GDMA_FLOW_CTRL_SELF, // DMA as the flow controller + .chan_priority = 1, + }; + dw_gdma_channel_handle_t m2m_chan = NULL; + TEST_ESP_OK(dw_gdma_new_channel(&alloc_config, &m2m_chan)); + + printf("set up memory set transaction\r\n"); + dw_gdma_block_transfer_config_t transfer_config = { + .src = { + .addr = (uint32_t)src_buf, + .burst_mode = DW_GDMA_BURST_MODE_FIXED, + .width = DW_GDMA_TRANS_WIDTH_8, + .burst_items = 4, + .burst_len = 1, // Note for ESP32P4, if the buffer is in PSRAM and the burst mode is fixed, we can't set the burst length larger than 1 + }, + .dst = { + .addr = (uint32_t)dst_buf, + .burst_mode = DW_GDMA_BURST_MODE_INCREMENT, + .width = DW_GDMA_TRANS_WIDTH_8, + .burst_items = 4, + .burst_len = 1, + }, + .size = 256, + }; + TEST_ESP_OK(dw_gdma_channel_config_transfer(m2m_chan, &transfer_config)); + + printf("start the DMA engine\r\n"); + TEST_ESP_OK(dw_gdma_channel_enable_ctrl(m2m_chan, true)); + vTaskDelay(pdMS_TO_TICKS(100)); + + printf("check the memory set result\r\n"); +#if CONFIG_IDF_TARGET_ESP32P4 + // the destination data are not reflected to the cache, so do an invalidate to ask the cache load new data + TEST_ESP_OK(esp_cache_msync((void *)dst_buf, 256, ESP_CACHE_MSYNC_FLAG_DIR_M2C)); +#endif + for (int i = 0; i < 256; i++) { + TEST_ASSERT_EQUAL_UINT8(66, dst_buf[i]); + } + + TEST_ESP_OK(dw_gdma_del_channel(m2m_chan)); + free(src_buf); + free(dst_buf); +} diff --git a/components/esp_hw_support/test_apps/dma/sdkconfig.defaults.esp32p4 b/components/esp_hw_support/test_apps/dma/sdkconfig.defaults.esp32p4 new file mode 100644 index 0000000000..9a2c70b27e --- /dev/null +++ b/components/esp_hw_support/test_apps/dma/sdkconfig.defaults.esp32p4 @@ -0,0 +1,3 @@ +CONFIG_SPIRAM=y +CONFIG_SPIRAM_MODE_HEX=y +CONFIG_SPIRAM_SPEED_20M=y From 002467d0f66295ed485e310da91dd10abfd90ce5 Mon Sep 17 00:00:00 2001 From: morris Date: Mon, 8 Jan 2024 11:37:52 +0800 Subject: [PATCH 3/3] fix(dw_gdma): free the interrupt handle when deleting the channel --- components/esp_hw_support/dma/dw_gdma.c | 14 ++++++++++---- .../esp_hw_support/include/esp_private/dw_gdma.h | 4 +++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/components/esp_hw_support/dma/dw_gdma.c b/components/esp_hw_support/dma/dw_gdma.c index 5d7755bd7d..7f6cdb43cd 100644 --- a/components/esp_hw_support/dma/dw_gdma.c +++ b/components/esp_hw_support/dma/dw_gdma.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -208,6 +208,9 @@ static esp_err_t channel_destroy(dw_gdma_channel_t *chan) if (chan->group) { channel_unregister_from_group(chan); } + if (chan->intr) { + esp_intr_free(chan->intr); + } free(chan); return ESP_OK; } @@ -379,9 +382,12 @@ esp_err_t dw_gdma_new_link_list(const dw_gdma_link_list_config_t *config, dw_gdm uint32_t num_items = config->num_items; list = heap_caps_calloc(1, sizeof(dw_gdma_link_list_t), DW_GDMA_MEM_ALLOC_CAPS); ESP_GOTO_ON_FALSE(list, ESP_ERR_NO_MEM, err, TAG, "no mem for link list"); - // the link list item has a strict alignment requirement, so we allocate it separately - items = heap_caps_aligned_calloc(DW_GDMA_LL_LINK_LIST_ALIGNMENT, num_items, - sizeof(dw_gdma_link_list_item_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_DMA); + // allocate memory for link list items, from SRAM + // the link list items has itw own alignment requirement + // also we should respect the data cache line size + uint32_t data_cache_line_size = cache_hal_get_cache_line_size(CACHE_LL_LEVEL_INT_MEM, CACHE_TYPE_DATA); + uint32_t alignment = MAX(DW_GDMA_LL_LINK_LIST_ALIGNMENT, data_cache_line_size); + items = heap_caps_aligned_calloc(alignment, num_items, sizeof(dw_gdma_link_list_item_t), MALLOC_CAP_INTERNAL | MALLOC_CAP_DMA); ESP_RETURN_ON_FALSE(items, ESP_ERR_NO_MEM, TAG, "no mem for link list items"); list->num_items = num_items; list->items = items; diff --git a/components/esp_hw_support/include/esp_private/dw_gdma.h b/components/esp_hw_support/include/esp_private/dw_gdma.h index 14968015b5..b672cdcda7 100644 --- a/components/esp_hw_support/include/esp_private/dw_gdma.h +++ b/components/esp_hw_support/include/esp_private/dw_gdma.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -338,6 +338,8 @@ esp_err_t dw_gdma_channel_use_link_list(dw_gdma_channel_handle_t chan, dw_gdma_l /** * @brief A helper function to return an item from a given link list, by index * + * @note The address of the returned item is not behind the cache + * * @param[in] list Link list handle, allocated by `dw_gdma_new_link_list` * @param[in] item_index Index of the item * @return