From 4f03e0241e27015d2256526027cccc1c406cb69e Mon Sep 17 00:00:00 2001 From: KonstantinKondrashov Date: Thu, 28 Dec 2023 16:50:55 +0800 Subject: [PATCH 1/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 862e57e5b55..b61a06c63a9 100644 --- a/components/spi_flash/cache_utils.c +++ b/components/spi_flash/cache_utils.c @@ -48,7 +48,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" @@ -157,8 +157,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); @@ -173,33 +173,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 From cbba78ee760452dd3d0c251b6ca1f56e1f00eca1 Mon Sep 17 00:00:00 2001 From: KonstantinKondrashov Date: Sat, 25 Nov 2023 23:32:47 +0800 Subject: [PATCH 2/2] feat(ipc): Adds a new no blocking IPC call --- components/app_trace/gcov/gcov_rtio.c | 5 +- components/esp_system/esp_ipc.c | 73 ++++++++++--------- components/esp_system/include/esp_ipc.h | 2 +- .../esp_system/include/esp_private/esp_ipc.h | 47 ++++++++++++ .../esp_system_unity_tests/main/test_ipc.c | 49 ++++++++++++- 5 files changed, 137 insertions(+), 39 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 62d5398ecd3..21a25cad1fb 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 "esp_attr.h" #include "hal/wdt_hal.h" #if CONFIG_IDF_TARGET_ESP32 @@ -84,9 +84,8 @@ void gcov_create_task(void *arg) static IRAM_ATTR 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 984fb89e04e..e0924575a83 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 @@ -39,10 +42,11 @@ typedef enum { IPC_WAIT_FOR_END, } esp_ipc_wait_t; -#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 esp_ipc_wait_t volatile s_wait_for[portNUM_PROCESSORS]; + +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) { @@ -54,29 +58,23 @@ static void IRAM_ATTR ipc_task(void* arg) #endif while (true) { - uint32_t ipc_wait; - xTaskNotifyWait(0, ULONG_MAX, &ipc_wait, portMAX_DELAY); - -#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 */ - // esp_ipc API and gcov_from_isr APIs can be processed together if they came at the same time - if (ipc_wait == IPC_WAIT_NO) { - continue; - } + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + + 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; } -#endif // CONFIG_APPTRACE_GCOV_ENABLE #ifndef CONFIG_FREERTOS_UNICORE if (s_func[cpuid]) { // we need to cache s_func, s_func_arg and ipc_ack variables locally // because they can be changed by a subsequent IPC call (after xTaskNotify(caller_task_handle)). esp_ipc_func_t func = s_func[cpuid]; - s_func[cpuid] = NULL; void* func_arg = s_func_arg[cpuid]; + esp_ipc_wait_t ipc_wait = s_wait_for[cpuid]; SemaphoreHandle_t ipc_ack = s_ipc_ack[cpuid]; + s_func[cpuid] = NULL; if (ipc_wait == IPC_WAIT_FOR_START) { xSemaphoreGive(ipc_ack); @@ -119,7 +117,7 @@ static void esp_ipc_init(void) s_ipc_mutex[i] = xSemaphoreCreateMutexStatic(&s_ipc_mutex_buffer[i]); s_ipc_ack[i] = xSemaphoreCreateBinaryStatic(&s_ipc_ack_buffer[i]); BaseType_t 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; } @@ -151,9 +149,11 @@ 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; - xTaskNotify(s_ipc_task_handle[cpu_id], wait_for, eSetValueWithOverwrite); + s_wait_for[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; + xTaskNotifyGive(s_ipc_task_handle[cpu_id]); xSemaphoreTake(s_ipc_ack[cpu_id], portMAX_DELAY); #ifdef CONFIG_ESP_IPC_USES_CALLERS_PRIORITY @@ -174,28 +174,33 @@ 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 -// the top level guarantees that the next call will be only after the previous one has completed -#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) { - if (xTaskGetSchedulerState() != taskSCHEDULER_RUNNING) { + if (cpu_id >= portNUM_PROCESSORS || s_ipc_task_handle[cpu_id] == NULL) { + return ESP_ERR_INVALID_ARG; + } + if (cpu_id == xPortGetCoreID() && xTaskGetSchedulerState() != taskSCHEDULER_RUNNING) { return ESP_ERR_INVALID_STATE; } - // Since it is called from an interrupt, it can not wait for a mutex to be released. - if (s_gcov_func == NULL) { - s_gcov_func_arg = arg; - s_gcov_func = func; + // 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 the target task already has a notification pending then its notification value is not updated (WithoutOverwrite). - xTaskNotifyFromISR(s_ipc_task_handle[cpu_id], IPC_WAIT_NO, eSetValueWithoutOverwrite, NULL); + if (xPortInIsrContext()) { + vTaskNotifyGiveFromISR(s_ipc_task_handle[cpu_id], NULL); + } else { +#ifdef CONFIG_ESP_IPC_USES_CALLERS_PRIORITY + vTaskPrioritySet(s_ipc_task_handle[cpu_id], IPC_MAX_PRIORITY); +#endif + xTaskNotifyGive(s_ipc_task_handle[cpu_id]); + } return ESP_OK; } // the previous call was not completed 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_ipc.h b/components/esp_system/include/esp_ipc.h index 3be0620d009..853463043d7 100644 --- a/components/esp_system/include/esp_ipc.h +++ b/components/esp_system/include/esp_ipc.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ 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 00000000000..a71c9088cc0 --- /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_apps/esp_system_unity_tests/main/test_ipc.c b/components/esp_system/test_apps/esp_system_unity_tests/main/test_ipc.c index da09e901075..78214b2ff2e 100644 --- a/components/esp_system/test_apps/esp_system_unity_tests/main/test_ipc.c +++ b/components/esp_system/test_apps/esp_system_unity_tests/main/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" @@ -162,4 +163,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 */