-
Notifications
You must be signed in to change notification settings - Fork 1.5k
arch/xtensa/esp32s3: Fix bug related to the PSRAM-allocated task stack #16756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
yamt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please let me test a bit.
this breaks my configuration. my configuration assumes spiram is available for text heap. (for wamr) (the configuration also happens to use spiflash, which was just working by luck i guess. maybe because LPWORK stack is allocated early in the boot. maybe it helps to have a debug option to make the malloc pick memory regions in an unusual order.) |
Can you apply the following diff and re-test your config? If not successful, can you please share (part of it, if preferable) your config? |
|
Sorry, closed by accident. |
Also, please note that kernel heap is enabled, it's only used by the idle thread and the work queues (about allocating the stack, I meant). So it makes sense not allocating the text heap from the kernel heap on ESP32-S3. |
this approach might work for my config. but i suspect it can break others.
i provide a full config here anyway. it isn't a secret. (ignore mbedtls and jsonsink stuff. they are wip.) |
06082f5
d848948 to
06082f5
Compare
06082f5 to
5678918
Compare
|
I'm sorry for the late response to this PR. As @yamt suggested, I continued testing this implementation and reached some conclusions. I'm convinced that we should allocate memory from the user heap (if available) for Just for the sake of sanity, this can be checked by building with Furthermore, it's easy to conclude that a code that should run from the userspace (just like a ELF file being loaded in a non-flat build), should allocate its text heap from the user heap. Talking about the ELF loader... After running some testing, I conclude that we couldn't run any ELF files from the external PSRAM on ESP32-S3 at all... we never caught this issue because the external PSRAM was being allocated along with the internal RAM in a single heap and the testing applications didn't require allocating from the external memory (internal memory was enough). This issue can be reproduced by just enabling the external PSRAM and enabling a separate heap for it. Also, to force the firmware to use the user heap, we need to apply the following patch: Built with: The above issue can be checked against the master branch: whenever we force the device to allocate from the PSRAM, it fails... that would happen even without the applied patch once the internal memory runs out... And, finally... The update PR (I also updated the description) fixes it: if the external PSRAM is disabled (or allocated to a common heap whenever this is possible), it doesn't matter from where the ELF file's allocates its memory. If we have a separate heap (which is mandatory when SPI flash or Wi-fi are enabled), it will allocate from the external PSRAM. Also, it will ensure that any data being copied to the memory is copied using the data bus. Finally, we still needed to ensure that the cache status is cleaned before running the ELF file. |
|
@yamt , @acassis and @xiaoxiang781216 , can you please reconsider this patch after #16756 (comment)? |
|
Fixing the failing jobs... (not directly related, but it's worth fixing it) |
5678918 to
51630fc
Compare
Done. There were missing APIs. Already implemented... |
|
please fix the build and check error: |
51630fc to
a8a9009
Compare
Fixed that, thanks! Now the sim-02 test is falling and I couldn't figure out why. Perhaps something related to the CI itself? |
This commit adds the missing `board_memorymap.h` file for all the ESP32-S3's boards. This header file is necessary whenever Wi-Fi is enabled, for instance. Signed-off-by: Tiago Medicci Serrano <tiago.medicci@espressif.com>
If both SPI Flash support (`CONFIG_ESP32S3_SPIFLASH`) and PSRAM (`CONFIG_ESP32S3_SPIRAM`) are enabled, the PSRAM can only be assigned to the user's heap (`CONFIG_ESP32S3_SPIRAM_USER_HEAP`). Additionaly, `CONFIG_ESP32S3_SPI_FLASH_SUPPORT_PSRAM_STACK` must be set because the system will end up allocating tasks' stacks from the external PSRAM. This has an impact when dealing with SPI flash operations because the cache must be disabled and the running task should not rely on any data from the PSRAM. To accomplish that, It offloads the SPI flash operation to a work queue (which, by definition, allocates its heap from the kernel heap). The same (assigning the PSRAM to the user's heap) is valid when the Wi-Fi is enabled because the lower-half driver requires data being allocated from the internal memory (which can only be achieved by allocating from the kernel heap when both the kernel and user heaps exists). Signed-off-by: Tiago Medicci Serrano <tiago.medicci@espressif.com>
Prior to this commit, it wasn't possible to load ELF modules from the external PSRAM. There were two main issues about it: 1) copying data using the instruction bus was being used instead of the data bus (this, per si, isn't a problem, but requires special attention regarding data alignment), and 2) the cache was not being properly cleaned and flushed to properly access the loaded data using the instruction bus. Signed-off-by: Tiago Medicci Serrano <tiago.medicci@espressif.com>
a8a9009 to
bc74366
Compare
Summary
Prior to this change, it wasn't possible to load ELF modules from the external PSRAM. There were two main issues about it: 1) copying data using the instruction bus was being used instead of the data bus (this, per se, isn't a problem, but requires special attention regarding data alignment), and 2) the cache was not being properly cleaned and flushed to properly access the loaded data using the instruction bus.
If both SPI Flash support (
CONFIG_ESP32S3_SPIFLASH) and PSRAM (CONFIG_ESP32S3_SPIRAM) are enabled, the PSRAM can only be assigned to the user's heap (CONFIG_ESP32S3_SPIRAM_USER_HEAP). Additionally,CONFIG_ESP32S3_SPI_FLASH_SUPPORT_PSRAM_STACKmust be set because the system will end up allocating tasks' stacks from the external PSRAM. This has an impact when dealing with SPI flash operations because the cache must be disabled and the running task should not rely on any data from the PSRAM. To accomplish that, It offloads the SPI flash operation to a work queue (which, by definition, allocates its heap from the kernel heap).The same (assigning the PSRAM to the user's heap) is valid when the Wi-Fi is enabled because the lower-half driver requires data being allocated from the internal memory (which can only be achieved by allocating from the kernel heap when both the kernel and user heaps exist).
It is necessary to add the missing
board_memorymap.hfile for all the ESP32-S3's boards. This header file is necessary whenever Wi-Fi is enabled because it now requires the kernel heap to be enabled (and this file is a dependency for it).Impact
Impact on user: Yes. It fixes a bug.
Impact on build: No.
Impact on hardware: Yes. It impacts ESP32-S3.
Impact on documentation: No.
Impact on security: No.
Impact on compatibility: No.
Testing
Testing can be performed by building the firmware and flashing it to a ESP32-S3 board with a PSRAM module (this example uses a ESP32-S3-DevKitC-1 v1.1 board with a ESP32-S3-WROOM-2 module). Before applying this patch, the device fails to boot.
Building
The firmware can be built and flashed with the following command:
Running
Check boot on UART0:
Results
Before
Boot failed...
After