From a41e037f0e2dacd0983e8bb4faf31a55f01cd28c Mon Sep 17 00:00:00 2001 From: KonstantinKondrashov Date: Sat, 25 Nov 2023 23:32:47 +0800 Subject: [PATCH 1/2] feat(ipc): Adds a new no blocking IPC call --- components/app_trace/gcov/gcov_rtio.c | 5 +- components/esp_system/esp_ipc.c | 77 +++++++++---------- .../esp_system/include/esp_private/esp_ipc.h | 47 +++++++++++ components/esp_system/test/test_ipc.c | 49 +++++++++++- 4 files changed, 132 insertions(+), 46 deletions(-) create mode 100644 components/esp_system/include/esp_private/esp_ipc.h diff --git a/components/app_trace/gcov/gcov_rtio.c b/components/app_trace/gcov/gcov_rtio.c index bde24d4cf825..f8d124f70c39 100644 --- a/components/app_trace/gcov/gcov_rtio.c +++ b/components/app_trace/gcov/gcov_rtio.c @@ -15,7 +15,7 @@ #include "esp_app_trace.h" #include "esp_freertos_hooks.h" #include "esp_private/dbg_stubs.h" -#include "esp_ipc.h" +#include "esp_private/esp_ipc.h" #include "hal/wdt_hal.h" #if CONFIG_IDF_TARGET_ESP32 #include "esp32/rom/libc_stubs.h" @@ -82,9 +82,8 @@ void gcov_create_task(void *arg) void gcov_create_task_tick_hook(void) { - extern esp_err_t esp_ipc_start_gcov_from_isr(uint32_t cpu_id, esp_ipc_func_t func, void* arg); if (s_create_gcov_task) { - if (esp_ipc_start_gcov_from_isr(xPortGetCoreID(), &gcov_create_task, NULL) == ESP_OK) { + if (esp_ipc_call_nonblocking(xPortGetCoreID(), &gcov_create_task, NULL) == ESP_OK) { s_create_gcov_task = false; } } diff --git a/components/esp_system/esp_ipc.c b/components/esp_system/esp_ipc.c index 8a1870b8d219..6d451edfcb85 100644 --- a/components/esp_system/esp_ipc.c +++ b/components/esp_system/esp_ipc.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -12,11 +12,14 @@ #include "esp_ipc.h" #include "esp_private/esp_ipc_isr.h" #include "esp_attr.h" +#include "esp_cpu.h" #include "freertos/FreeRTOS.h" #include "freertos/task.h" #include "freertos/semphr.h" +#define IPC_MAX_PRIORITY (configMAX_PRIORITIES - 1) + #if !defined(CONFIG_FREERTOS_UNICORE) || defined(CONFIG_APPTRACE_GCOV_ENABLE) #if CONFIG_COMPILER_OPTIMIZATION_NONE @@ -45,10 +48,9 @@ static volatile esp_ipc_wait_t s_ipc_wait[portNUM_PROCESSORS];// This variable t // s_ipc_ack semaphore: before s_func is called, or // after it returns -#if CONFIG_APPTRACE_GCOV_ENABLE -static volatile esp_ipc_func_t s_gcov_func = NULL; // Gcov dump starter function which should be called by high priority task -static void * volatile s_gcov_func_arg; // Argument to pass into s_gcov_func -#endif +static volatile esp_ipc_func_t s_no_block_func[portNUM_PROCESSORS] = { 0 }; +static volatile bool s_no_block_func_and_arg_are_ready[portNUM_PROCESSORS] = { 0 }; +static void * volatile s_no_block_func_arg[portNUM_PROCESSORS]; static void IRAM_ATTR ipc_task(void* arg) { @@ -66,20 +68,19 @@ static void IRAM_ATTR ipc_task(void* arg) abort(); } -#if CONFIG_APPTRACE_GCOV_ENABLE - if (s_gcov_func) { - (*s_gcov_func)(s_gcov_func_arg); - s_gcov_func = NULL; - /* we can not interfer with IPC calls so no need for further processing */ - continue; + if (s_no_block_func_and_arg_are_ready[cpuid] && s_no_block_func[cpuid]) { + (*s_no_block_func[cpuid])(s_no_block_func_arg[cpuid]); + s_no_block_func_and_arg_are_ready[cpuid] = false; + s_no_block_func[cpuid] = NULL; + // esp_ipc API and esp_ipc_call_nonblocking APIs can be processed together if they came at the same time } -#endif + if (s_func[cpuid]) { // we need to cache s_func, s_func_arg and s_ipc_wait variables locally because they can be changed by a subsequent IPC call. esp_ipc_func_t func = s_func[cpuid]; - s_func[cpuid] = NULL; void* arg = s_func_arg[cpuid]; esp_ipc_wait_t ipc_wait = s_ipc_wait[cpuid]; + s_func[cpuid] = NULL; if (ipc_wait == IPC_WAIT_FOR_START) { xSemaphoreGive(s_ipc_ack[cpuid]); @@ -121,7 +122,7 @@ static void esp_ipc_init(void) s_ipc_ack[i] = xSemaphoreCreateBinaryStatic(&s_ipc_ack_buffer[i]); s_ipc_sem[i] = xSemaphoreCreateBinaryStatic(&s_ipc_sem_buffer[i]); portBASE_TYPE res = xTaskCreatePinnedToCore(ipc_task, task_name, IPC_STACK_SIZE, (void*) i, - configMAX_PRIORITIES - 1, &s_ipc_task_handle[i], i); + IPC_MAX_PRIORITY, &s_ipc_task_handle[i], i); assert(res == pdTRUE); (void)res; } @@ -150,9 +151,10 @@ static esp_err_t esp_ipc_call_and_wait(uint32_t cpu_id, esp_ipc_func_t func, voi xSemaphoreTake(s_ipc_mutex[0], portMAX_DELAY); #endif - s_func[cpu_id] = func; s_func_arg[cpu_id] = arg; s_ipc_wait[cpu_id] = wait_for; + // s_func must be set after all other parameters. The ipc_task use this as indicator of the IPC is prepared. + s_func[cpu_id] = func; xSemaphoreGive(s_ipc_sem[cpu_id]); xSemaphoreTake(s_ipc_ack[cpu_id], portMAX_DELAY); #ifdef CONFIG_ESP_IPC_USES_CALLERS_PRIORITY @@ -173,40 +175,31 @@ esp_err_t esp_ipc_call_blocking(uint32_t cpu_id, esp_ipc_func_t func, void* arg) return esp_ipc_call_and_wait(cpu_id, func, arg, IPC_WAIT_FOR_END); } -// currently this is only called from gcov component -#if CONFIG_APPTRACE_GCOV_ENABLE -esp_err_t esp_ipc_start_gcov_from_isr(uint32_t cpu_id, esp_ipc_func_t func, void* arg) +esp_err_t esp_ipc_call_nonblocking(uint32_t cpu_id, esp_ipc_func_t func, void* arg) { - portBASE_TYPE ret = pdFALSE; - - if (xTaskGetSchedulerState() != taskSCHEDULER_RUNNING) { - return ESP_ERR_INVALID_STATE; + if (cpu_id >= portNUM_PROCESSORS || s_ipc_task_handle[cpu_id] == NULL) { + return ESP_ERR_INVALID_ARG; } - - /* Lock IPC to avoid interferring with normal IPC calls, e.g. - avoid situation when esp_ipc_start_gcov_from_isr() is called from IRQ - in the middle of IPC call between `s_func` and `s_func_arg` modification. See esp_ipc_call_and_wait() */ -#ifdef CONFIG_ESP_IPC_USES_CALLERS_PRIORITY - ret = xSemaphoreTakeFromISR(s_ipc_mutex[cpu_id], NULL); -#else - ret = xSemaphoreTakeFromISR(s_ipc_mutex[0], NULL); -#endif - if (ret != pdTRUE) { - return ESP_ERR_TIMEOUT; + if (cpu_id == xPortGetCoreID() && xTaskGetSchedulerState() != taskSCHEDULER_RUNNING) { + return ESP_ERR_INVALID_STATE; } - s_gcov_func = func; - s_gcov_func_arg = arg; - ret = xSemaphoreGiveFromISR(s_ipc_sem[cpu_id], NULL); + // Since it can be called from an interrupt or Scheduler is Suspened, it can not wait for a mutex to be released. + if (esp_cpu_compare_and_set((volatile uint32_t *)&s_no_block_func[cpu_id], 0, (uint32_t)func)) { + s_no_block_func_arg[cpu_id] = arg; + s_no_block_func_and_arg_are_ready[cpu_id] = true; + if (xPortInIsrContext()) { + xSemaphoreGiveFromISR(s_ipc_sem[cpu_id], NULL); + } else { #ifdef CONFIG_ESP_IPC_USES_CALLERS_PRIORITY - xSemaphoreGiveFromISR(s_ipc_mutex[cpu_id], NULL); -#else - xSemaphoreGiveFromISR(s_ipc_mutex[0], NULL); + vTaskPrioritySet(s_ipc_task_handle[cpu_id], IPC_MAX_PRIORITY); #endif - - return ret == pdTRUE ? ESP_OK : ESP_FAIL; + xSemaphoreGive(s_ipc_sem[cpu_id]); + } + return ESP_OK; + } + return ESP_FAIL; } -#endif // CONFIG_APPTRACE_GCOV_ENABLE #endif // !defined(CONFIG_FREERTOS_UNICORE) || defined(CONFIG_APPTRACE_GCOV_ENABLE) diff --git a/components/esp_system/include/esp_private/esp_ipc.h b/components/esp_system/include/esp_private/esp_ipc.h new file mode 100644 index 000000000000..a71c9088cc0d --- /dev/null +++ b/components/esp_system/include/esp_private/esp_ipc.h @@ -0,0 +1,47 @@ +/* + * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#pragma once + +#include "../esp_ipc.h" +#include "sdkconfig.h" + +#ifdef __cplusplus +extern "C" { +#endif + +#if !defined(CONFIG_FREERTOS_UNICORE) || defined(CONFIG_APPTRACE_GCOV_ENABLE) + +/** + * @brief Execute a callback on a given CPU without any blocking operations for the caller + * + * Since it does not have any blocking operations it is suitable to be run from interrupts + * or even when the Scheduler on the current core is suspended. + * + * The function: + * - does not wait for the callback to begin or complete execution, + * - does not change the IPC priority. + * The function returns after sending a notification to the IPC task to execute the callback. + * + * @param[in] cpu_id CPU where the given function should be executed (0 or 1) + * @param[in] func Pointer to a function of type void func(void* arg) to be executed + * @param[in] arg Arbitrary argument of type void* to be passed into the function + * + * @return + * - ESP_ERR_INVALID_ARG if cpu_id is invalid + * - ESP_ERR_INVALID_STATE 1. IPC tasks have not been initialized yet, + * 2. cpu_id requests IPC on the current core, but the FreeRTOS scheduler is not running on it + * (the IPC task cannot be executed). + * - ESP_FAIL IPC is busy due to a previous call was not completed. + * - ESP_OK otherwise + */ +esp_err_t esp_ipc_call_nonblocking(uint32_t cpu_id, esp_ipc_func_t func, void* arg); + +#endif // !defined(CONFIG_FREERTOS_UNICORE) || defined(CONFIG_APPTRACE_GCOV_ENABLE) + +#ifdef __cplusplus +} +#endif diff --git a/components/esp_system/test/test_ipc.c b/components/esp_system/test/test_ipc.c index 0dce94125cfb..4b403310db19 100644 --- a/components/esp_system/test/test_ipc.c +++ b/components/esp_system/test/test_ipc.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -12,6 +12,7 @@ #include "unity.h" #if !CONFIG_FREERTOS_UNICORE #include "esp_ipc.h" +#include "esp_private/esp_ipc.h" #endif #include "esp_log.h" #include "esp_rom_sys.h" @@ -157,4 +158,50 @@ TEST_CASE("Test ipc_task can not wake up blocking task early", "[ipc]") TEST_ASSERT_EQUAL(31, val2); } +TEST_CASE("Test ipc call nonblocking", "[ipc]") +{ + int val_for_1_call = 20; + TEST_ESP_OK(esp_ipc_call_nonblocking(1, test_func_ipc_cb2, (void*)&val_for_1_call)); + TEST_ASSERT_EQUAL(20, val_for_1_call); + + int val_for_2_call = 30; + TEST_ESP_ERR(ESP_FAIL, esp_ipc_call_nonblocking(1, test_func_ipc_cb3, (void*)&val_for_2_call)); + + vTaskDelay(150 / portTICK_PERIOD_MS); + TEST_ASSERT_EQUAL(21, val_for_1_call); + + TEST_ESP_OK(esp_ipc_call_nonblocking(1, test_func_ipc_cb3, (void*)&val_for_2_call)); + + vTaskDelay(550 / portTICK_PERIOD_MS); + TEST_ASSERT_EQUAL(31, val_for_2_call); +} + +static void test_func_ipc_cb4(void *arg) +{ + int *val = (int *)arg; + *val = *val + 1; +} + +TEST_CASE("Test ipc call nonblocking when FreeRTOS Scheduler is suspended", "[ipc]") +{ + #ifdef CONFIG_FREERTOS_SMP + //Note: Scheduler suspension behavior changed in FreeRTOS SMP + vTaskPreemptionDisable(NULL); + #else + // Disable scheduler on the current CPU + vTaskSuspendAll(); + #endif // CONFIG_FREERTOS_SMP + + volatile int value = 20; + TEST_ESP_OK(esp_ipc_call_nonblocking(1, test_func_ipc_cb4, (void*)&value)); + while (value == 20) { }; + TEST_ASSERT_EQUAL(21, value); + + #ifdef CONFIG_FREERTOS_SMP + //Note: Scheduler suspension behavior changed in FreeRTOS SMP + vTaskPreemptionEnable(NULL); + #else + xTaskResumeAll(); + #endif +} #endif /* !CONFIG_FREERTOS_UNICORE */ From 955341db9858303b268558d314871ce6cdb1f463 Mon Sep 17 00:00:00 2001 From: KonstantinKondrashov Date: Thu, 28 Dec 2023 16:50:55 +0800 Subject: [PATCH 2/2] fix(spi_flash): Fix stuck during flash operation When a task was not pinned to a certain CPU. --- components/spi_flash/cache_utils.c | 60 ++++++++++++++++++------------ 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/components/spi_flash/cache_utils.c b/components/spi_flash/cache_utils.c index e91a5c061ea3..6d7f58f5a2d7 100644 --- a/components/spi_flash/cache_utils.c +++ b/components/spi_flash/cache_utils.c @@ -43,7 +43,7 @@ #include #include "sdkconfig.h" #ifndef CONFIG_FREERTOS_UNICORE -#include "esp_ipc.h" +#include "esp_private/esp_ipc.h" #endif #include "esp_attr.h" #include "esp_memory_utils.h" @@ -144,8 +144,8 @@ void IRAM_ATTR spi_flash_disable_interrupts_caches_and_other_cpu(void) spi_flash_op_lock(); - const int cpuid = xPortGetCoreID(); - const uint32_t other_cpuid = (cpuid == 0) ? 1 : 0; + int cpuid = xPortGetCoreID(); + uint32_t other_cpuid = (cpuid == 0) ? 1 : 0; #ifndef NDEBUG // For sanity check later: record the CPU which has started doing flash operation assert(s_flash_op_cpu == -1); @@ -160,33 +160,45 @@ void IRAM_ATTR spi_flash_disable_interrupts_caches_and_other_cpu(void) // esp_intr_noniram_disable. assert(other_cpuid == 1); } else { - // Temporarily raise current task priority to prevent a deadlock while - // waiting for IPC task to start on the other CPU - prvTaskSavedPriority_t SavedPriority; - prvTaskPriorityRaise(&SavedPriority, configMAX_PRIORITIES - 1); - - // Signal to the spi_flash_op_block_task on the other CPU that we need it to - // disable cache there and block other tasks from executing. - s_flash_op_can_start = false; - ESP_ERROR_CHECK(esp_ipc_call(other_cpuid, &spi_flash_op_block_func, (void *) other_cpuid)); + bool ipc_call_was_send_to_other_cpu; + do { + #ifdef CONFIG_FREERTOS_SMP + //Note: Scheduler suspension behavior changed in FreeRTOS SMP + vTaskPreemptionDisable(NULL); + #else + // Disable scheduler on the current CPU + vTaskSuspendAll(); + #endif // CONFIG_FREERTOS_SMP + + cpuid = xPortGetCoreID(); + other_cpuid = (cpuid == 0) ? 1 : 0; + #ifndef NDEBUG + s_flash_op_cpu = cpuid; + #endif + + s_flash_op_can_start = false; + + ipc_call_was_send_to_other_cpu = esp_ipc_call_nonblocking(other_cpuid, &spi_flash_op_block_func, (void *) other_cpuid) == ESP_OK; + + if (!ipc_call_was_send_to_other_cpu) { + // IPC call was not send to other cpu because another nonblocking API is running now. + // Enable the Scheduler again will not help the IPC to speed it up + // but there is a benefit to schedule to a higher priority task before the nonblocking running IPC call is done. + #ifdef CONFIG_FREERTOS_SMP + //Note: Scheduler suspension behavior changed in FreeRTOS SMP + vTaskPreemptionEnable(NULL); + #else + xTaskResumeAll(); + #endif // CONFIG_FREERTOS_SMP + } + } while (!ipc_call_was_send_to_other_cpu); while (!s_flash_op_can_start) { // Busy loop and wait for spi_flash_op_block_func to disable cache // on the other CPU } -#ifdef CONFIG_FREERTOS_SMP - //Note: Scheduler suspension behavior changed in FreeRTOS SMP - vTaskPreemptionDisable(NULL); -#else - // Disable scheduler on the current CPU - vTaskSuspendAll(); -#endif // CONFIG_FREERTOS_SMP - // Can now set the priority back to the normal one - prvTaskPriorityRestore(&SavedPriority); - // This is guaranteed to run on CPU because the other CPU is now - // occupied by highest priority task - assert(xPortGetCoreID() == cpuid); } + // Kill interrupts that aren't located in IRAM esp_intr_noniram_disable(); // This CPU executes this routine, with non-IRAM interrupts and the scheduler