You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/03/08 16:42:20 UTC
[GitHub] [incubator-nuttx] Ouss4 opened a new pull request #3003: ESP32: Re-organise the different heap regions
Ouss4 opened a new pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003
## Summary
This PR re-organises the ESP32 heaps:
1. Move the internal heap to "region 2", region 1 could end up with too little memory if static allocation is too large.
2. Part of the ROM bootloader memory at the end of 0x3ffae6f0 is not used. This is can be added to the static memory space.
3. Other regions in the middle of DRAM that are not used by the ROM bootloader were added as heap regions.
## Impact
Impacts ESP32 only:
- More heap available, ~10KB for non-SMP and ~35KB for SMP.
- Internal heap can be configured to a larger value.
## Testing
Testing on different boards and QEMU with and without SMP.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590071027
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -72,9 +72,23 @@
*
* When an internal heap is enabled this region starts at an offset equal to
* the size of the internal heap.
+ *
+ * The QEMU bootloader image is slightly different than the chip's one.
+ * The ROM on PRO and APP uses different regions for static data. In QEMU,
+ * however, we load only one ROM binary, taken from the PRO CPU, and it is
+ * used by both CPUs. So, in QEMU, if we allocate this part early, it will
+ * be clobbered once the APP CPU starts.
+ * We can delay the allocation to when everything has started through the
+ * board_late_initiliaze hook, as is done for the APP data, however this
+ * should be fixed from QEMU side. The following macros, then, just skip
+ * PRO CPU's data when a QEMU image generation is enabled with SMP.
*/
-#define HEAP_REGION2_START 0x3ffe0450
+#if defined(CONFIG_ESP32_QEMU_IMAGE) && defined(CONFIG_SMP)
Review comment:
I agree, it does feel awkward to me too.
The thing is this workaround has to be applied whenever a QEMU image is generated with SMP, otherwise the board won't boot. So if we add a new one it will end up being auto selected.
I'm also operating with the assumption that this should be fixed in QEMU (don't know exactly when, though.)
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#issuecomment-800136105
@masayuki2009 I have seen some instability in QEMU before but I've fixed them, or at least so I thought.
This can be another case of the differences in bootloader image used in QEMU and hardware.
I'll check again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r589853261
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -80,18 +83,16 @@
#endif
#ifdef CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP
-#define XTENSA_IMEM_REGION_SIZE CONFIG_XTENSA_IMEM_REGION_SIZE
+# define XTENSA_IMEM_REGION_SIZE CONFIG_XTENSA_IMEM_REGION_SIZE
#else
-#define XTENSA_IMEM_REGION_SIZE 0
+# define XTENSA_IMEM_REGION_SIZE 0
#endif
-/* If CONFIG_XTENSA_IMEM_MAXIMIZE_HEAP_REGION is defined, it means
- * using maximum separate heap for internal memory, but part of
- * the available memory is reserved for the Region 1 heap.
- */
+/* Internal heap starts at the end of the ROM data. */
-#ifdef CONFIG_XTENSA_IMEM_MAXIMIZE_HEAP_REGION
-#ifndef HEAP_REGION_OFFSET
-#define HEAP_REGION_OFFSET 0x2000
-#endif
+#ifndef CONFIG_SMP
+# define ESP32_IMEM_START 0x3ffe1330
Review comment:
how about:
```
#define ESP32_IMEM_START HEAP_REGION2_START
```
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -96,3 +96,8 @@
# define ESP32_IMEM_START 0x3ffe7e40
#endif
+/* Region of unused ROM App data */
+
+#define HEAP_REGION_ROMAPP_START 0x3ffe4360
+#define HEAP_REGION_ROMAPP_END 0x3ffe5230
Review comment:
are there public doc or code from which one can infer these values?
##########
File path: arch/xtensa/src/esp32/esp32_allocateheap.c
##########
@@ -96,8 +96,15 @@ void xtensa_add_region(void)
{
size_t region2_start = HEAP_REGION2_START + XTENSA_IMEM_REGION_SIZE;
+#ifndef CONFIG_SMP
umm_addregion((FAR void *)region2_start,
(size_t)(uintptr_t)&_eheap - region2_start);
+#else
+ umm_addregion((FAR void *)region2_start,
+ (size_t)HEAP_REGION2_END - region2_start);
+ umm_addregion((FAR void *)HEAP_REGION3_START,
+ (size_t)&_eheap - HEAP_REGION3_START);
Review comment:
this region3 stuff seems to belong to the other commit ("ESP32: Re-organise the different heap regions"), not this one ("arch/xtensa/esp32: Part of the ROM regions in middle of DRAM are not")
##########
File path: boards/xtensa/esp32/esp32-devkitc/scripts/esp32.template.ld
##########
@@ -50,7 +50,7 @@ MEMORY
* be used.
*/
- dram0_0_seg (RW) : org = 0x3ffb0000 + CONFIG_ESP32_BT_RESERVE_DRAM,
+ dram0_0_seg (RW) : org = 0x3ffae6f0 + CONFIG_ESP32_BT_RESERVE_DRAM,
Review comment:
is there public doc or code from which one can infer these values?
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -72,9 +72,23 @@
*
* When an internal heap is enabled this region starts at an offset equal to
* the size of the internal heap.
+ *
+ * The QEMU bootloader image is slightly different than the chip's one.
+ * The ROM on PRO and APP uses different regions for static data. In QEMU,
+ * however, we load only one ROM binary, taken from the PRO CPU, and it is
+ * used by both CPUs. So, in QEMU, if we allocate this part early, it will
+ * be clobbered once the APP CPU starts.
+ * We can delay the allocation to when everything has started through the
+ * board_late_initiliaze hook, as is done for the APP data, however this
+ * should be fixed from QEMU side. The following macros, then, just skip
+ * PRO CPU's data when a QEMU image generation is enabled with SMP.
*/
-#define HEAP_REGION2_START 0x3ffe0450
+#if defined(CONFIG_ESP32_QEMU_IMAGE) && defined(CONFIG_SMP)
Review comment:
can you consider to introduce a separate kconfig for this?
maybe we can follow this precedent.
```
config TIVA_WITH_QEMU
bool "Workaround for tiva with qemu"
default n
```
how do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590063841
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -72,9 +72,23 @@
*
* When an internal heap is enabled this region starts at an offset equal to
* the size of the internal heap.
+ *
+ * The QEMU bootloader image is slightly different than the chip's one.
+ * The ROM on PRO and APP uses different regions for static data. In QEMU,
+ * however, we load only one ROM binary, taken from the PRO CPU, and it is
+ * used by both CPUs. So, in QEMU, if we allocate this part early, it will
+ * be clobbered once the APP CPU starts.
+ * We can delay the allocation to when everything has started through the
+ * board_late_initiliaze hook, as is done for the APP data, however this
+ * should be fixed from QEMU side. The following macros, then, just skip
+ * PRO CPU's data when a QEMU image generation is enabled with SMP.
*/
-#define HEAP_REGION2_START 0x3ffe0450
+#if defined(CONFIG_ESP32_QEMU_IMAGE) && defined(CONFIG_SMP)
Review comment:
i feel it's awkward to change the behavior with the output binary/image options. but it might be just me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#issuecomment-800017210
@yamt can you merge it please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r591150127
##########
File path: arch/xtensa/src/esp32/esp32_allocateheap.c
##########
@@ -96,8 +96,15 @@ void xtensa_add_region(void)
{
size_t region2_start = HEAP_REGION2_START + XTENSA_IMEM_REGION_SIZE;
+#ifndef CONFIG_SMP
umm_addregion((FAR void *)region2_start,
(size_t)(uintptr_t)&_eheap - region2_start);
+#else
+ umm_addregion((FAR void *)region2_start,
+ (size_t)HEAP_REGION2_END - region2_start);
+ umm_addregion((FAR void *)HEAP_REGION3_START,
+ (size_t)&_eheap - HEAP_REGION3_START);
Review comment:
I was going to edit the commit, but I remembered that it was part of it in the first place.
> my suggestion is do nothing.
I'll take your suggestion. ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r591644492
##########
File path: boards/xtensa/esp32/esp32-devkitc/scripts/esp32.template.ld
##########
@@ -50,7 +50,7 @@ MEMORY
* be used.
*/
- dram0_0_seg (RW) : org = 0x3ffb0000 + CONFIG_ESP32_BT_RESERVE_DRAM,
+ dram0_0_seg (RW) : org = 0x3ffae6f0 + CONFIG_ESP32_BT_RESERVE_DRAM,
Review comment:
@yamt I dropped this commit. I released that when Bluetooth is enabled things will get complicated if we keep this region as static memory.
Bluetooth uses a fixed region at the start of 0x3ffb0000, controlled by `CONFIG_ESP32_BT_RESERVER_DRAM` but it should be around 64KB, maybe a bit less.
So I reverted the start of dram0_0_seg back to 0x3ffb0000 and used the 6KB region before that as heap, called REGION0, but open to better naming suggestions ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#issuecomment-800123362
@Ouss4
I noticed that ostest with esp32-devkitc:smp (QEMU) is unstable, although I added ```CONFIG_ESP32_QEMU_IMAGE=y```.
```
sender_thread: mq_send succeeded on msg 9
receiver_thread: mq_receive succeeded on msg 9
sender_thread: returning nerrors=0
xtensa_user_panic: User Exception: EXCCAUSE=001d task: ostest
xtensa_dumpstate: CPU1:
xtensa_registerdump: PC: 400d47ad PS: 00060430
xtensa_registerdump: A0: 800d453b A1: 3ffe3c90 A2: 3ffb1330 A3: 3ffe0858
xtensa_registerdump: A4: 3ffafe40 A5: 00060423 A6: 00060423 A7: 000000ff
xtensa_registerdump: A8: 3ffe3f08 A9: 00000000 A10: 00000000 A11: 3ffe3ee8
xtensa_registerdump: A12: 00060420 A13: 00000002 A14: 00000009 A15: 00000007
xtensa_registerdump: SAR: 0000001f CAUSE: 0000001d VADDR: 00000008
xtensa_registerdump: LBEG: 4000c46c LEND: 4000c477 LCNT: 00000000
xtensa_registerdump: TMP0: 400807da TMP1: 3ffe3a70
xtensa_btdump: Backtrace0: 400d4e4f:3ffe3a10
xtensa_btdump: Backtrace1: 400d4f5d:3ffe3a30
xtensa_btdump: Backtrace2: 400d4ddf:3ffe3a50
xtensa_btdump: Backtrace3: 400807f4:3ffe3a70
xtensa_btdump: Backtrace4: 40020:3ffe3a90
xtensa_btdump: Backtrace5: 400d5130:3ffe0858
xtensa_btdump: Backtrace6: 3ffb132d:2
xtensa_btdump: BACKTRACE CORRUPTED!
xtensa_dumpstate: sp: 3ffe39c0
xtensa_dumpstate: stack base: 3ffe3dd0
xtensa_dumpstate: stack size: 00002020
...
````
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r589604251
##########
File path: arch/xtensa/src/esp32/esp32_allocateheap.c
##########
@@ -96,17 +96,41 @@ void xtensa_add_region(void)
{
void *start;
size_t size;
+ int availregions;
+ int nregions = CONFIG_MM_REGIONS - 1;
+
+#ifdef CONFIG_SMP
+ availregions = 2;
+# ifdef CONFIG_BOARD_LATE_INITIALIZE
+ availregions++;
+# else
+ minfo("A ~3KB heap region can be added to the heap by enabling"
+ " CONFIG_BOARD_LATE_INITLIAZE\n");
Review comment:
```suggestion
" CONFIG_BOARD_LATE_INITIALIZE\n");
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590022179
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -80,18 +83,16 @@
#endif
#ifdef CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP
-#define XTENSA_IMEM_REGION_SIZE CONFIG_XTENSA_IMEM_REGION_SIZE
+# define XTENSA_IMEM_REGION_SIZE CONFIG_XTENSA_IMEM_REGION_SIZE
#else
-#define XTENSA_IMEM_REGION_SIZE 0
+# define XTENSA_IMEM_REGION_SIZE 0
#endif
-/* If CONFIG_XTENSA_IMEM_MAXIMIZE_HEAP_REGION is defined, it means
- * using maximum separate heap for internal memory, but part of
- * the available memory is reserved for the Region 1 heap.
- */
+/* Internal heap starts at the end of the ROM data. */
-#ifdef CONFIG_XTENSA_IMEM_MAXIMIZE_HEAP_REGION
-#ifndef HEAP_REGION_OFFSET
-#define HEAP_REGION_OFFSET 0x2000
-#endif
+#ifndef CONFIG_SMP
+# define ESP32_IMEM_START 0x3ffe1330
Review comment:
It's not always the start of region2, in SMP it's the start of region3.
I can change it to:
```C
#ifndef CONFIG_SMP
# define ESP32_IMEM_START HEAP_REGION2_START
#else
# define ESP32_IMEM_START HEAP_REGION3_START
#endif
```
Which I think is better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r591644492
##########
File path: boards/xtensa/esp32/esp32-devkitc/scripts/esp32.template.ld
##########
@@ -50,7 +50,7 @@ MEMORY
* be used.
*/
- dram0_0_seg (RW) : org = 0x3ffb0000 + CONFIG_ESP32_BT_RESERVE_DRAM,
+ dram0_0_seg (RW) : org = 0x3ffae6f0 + CONFIG_ESP32_BT_RESERVE_DRAM,
Review comment:
@yamt I dropped this commit. I realised that when Bluetooth is enabled things will get complicated if we keep this region as static memory.
Bluetooth uses a fixed region at the start of 0x3ffb0000, controlled by `CONFIG_ESP32_BT_RESERVER_DRAM` but it should be around 64KB, maybe a bit less.
So I reverted the start of dram0_0_seg back to 0x3ffb0000 and used the 6KB region before that as heap, called REGION0, but open to better naming suggestions ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590016890
##########
File path: boards/xtensa/esp32/esp32-devkitc/scripts/esp32.template.ld
##########
@@ -50,7 +50,7 @@ MEMORY
* be used.
*/
- dram0_0_seg (RW) : org = 0x3ffb0000 + CONFIG_ESP32_BT_RESERVE_DRAM,
+ dram0_0_seg (RW) : org = 0x3ffae6f0 + CONFIG_ESP32_BT_RESERVE_DRAM,
Review comment:
Please see https://github.com/espressif/esp-idf/blob/master/components/soc/esp32/soc_memory_layout.c#L168
IDF uses this as a seperate heap region, but because it's adjacent to 0x3ffb0000 I couldn't think of any reason why not extend static memory.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r593010032
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -32,66 +32,121 @@
*
* CONFIG_HEAP2_BASE eg. 3f80 0000
* :
- * : g_mmheap region3 (CONFIG_ESP32_SPIRAM)
+ * : g_mmheap (CONFIG_ESP32_SPIRAM)
* :
* CONFIG_HEAP2_BASE + CONFIG_HEAP2_SIZE eg. 3fc0 0000
*
- * _sheap eg. 3ffc 8c6c
+ * HEAP_REGION0_START 3ffa e6f0
* :
- * : g_iheap (CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP)
+ * : g_mmheap region0
* :
- * _sheap + CONFIG_XTENSA_IMEM_REGION_SIZE eg. 3ffd ebfc
+ * HEAP_REGION0_END 3ffa fff0
+ * :
+ * _sheap eg. 3ffc 8c6c
* :
* : g_mmheap region1
* :
* HEAP_REGION1_END 3ffd fff0
* :
* : ROM data
* :
- * HEAP_REGION2_START 3ffe 1330 or 3ffe 7e40
+ * HEAP_REGION2_START 3ffe 0450
+ * :
+ * : g_iheap (CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP)
Review comment:
I did so, thanks for the suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#issuecomment-793480245
@yamt for the docs, I'm not aware of anything public, but most of the information can be found [here](https://github.com/espressif/esp-idf/blob/master/components/soc/esp32/soc_memory_layout.c#L147-L168). These values just came from analysing the binaries of the ROM bootloader.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt merged pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
yamt merged pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r592985326
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -32,66 +32,121 @@
*
* CONFIG_HEAP2_BASE eg. 3f80 0000
* :
- * : g_mmheap region3 (CONFIG_ESP32_SPIRAM)
+ * : g_mmheap (CONFIG_ESP32_SPIRAM)
* :
* CONFIG_HEAP2_BASE + CONFIG_HEAP2_SIZE eg. 3fc0 0000
*
- * _sheap eg. 3ffc 8c6c
+ * HEAP_REGION0_START 3ffa e6f0
* :
- * : g_iheap (CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP)
+ * : g_mmheap region0
* :
- * _sheap + CONFIG_XTENSA_IMEM_REGION_SIZE eg. 3ffd ebfc
+ * HEAP_REGION0_END 3ffa fff0
+ * :
+ * _sheap eg. 3ffc 8c6c
* :
* : g_mmheap region1
* :
* HEAP_REGION1_END 3ffd fff0
* :
* : ROM data
* :
- * HEAP_REGION2_START 3ffe 1330 or 3ffe 7e40
+ * HEAP_REGION2_START 3ffe 0450
+ * :
+ * : g_iheap (CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP)
Review comment:
this iheap region is only for !SMP, right?
i guess it's more straightforward to make the `if CONFIG_SMP` block below
start from `HEAP_REGION2_START`.
that is:
```
------------
if !CONFIG_SMP
HEAP_REGION2_START
:
(omit)
:
------------
if CONFIG_SMP
HEAP_REGION2_START
:
(omit)
:
------------
_eheap
```
how do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r592976425
##########
File path: boards/xtensa/esp32/esp32-devkitc/scripts/esp32.template.ld
##########
@@ -50,7 +50,7 @@ MEMORY
* be used.
*/
- dram0_0_seg (RW) : org = 0x3ffb0000 + CONFIG_ESP32_BT_RESERVE_DRAM,
+ dram0_0_seg (RW) : org = 0x3ffae6f0 + CONFIG_ESP32_BT_RESERVE_DRAM,
Review comment:
unfortunately i don't have any better name suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590026412
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -80,18 +83,16 @@
#endif
#ifdef CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP
-#define XTENSA_IMEM_REGION_SIZE CONFIG_XTENSA_IMEM_REGION_SIZE
+# define XTENSA_IMEM_REGION_SIZE CONFIG_XTENSA_IMEM_REGION_SIZE
#else
-#define XTENSA_IMEM_REGION_SIZE 0
+# define XTENSA_IMEM_REGION_SIZE 0
#endif
-/* If CONFIG_XTENSA_IMEM_MAXIMIZE_HEAP_REGION is defined, it means
- * using maximum separate heap for internal memory, but part of
- * the available memory is reserved for the Region 1 heap.
- */
+/* Internal heap starts at the end of the ROM data. */
-#ifdef CONFIG_XTENSA_IMEM_MAXIMIZE_HEAP_REGION
-#ifndef HEAP_REGION_OFFSET
-#define HEAP_REGION_OFFSET 0x2000
-#endif
+#ifndef CONFIG_SMP
+# define ESP32_IMEM_START 0x3ffe1330
Review comment:
ok. when i made this comment, i was looking at the individual commit https://github.com/apache/incubator-nuttx/pull/3003/commits/7911aa57372e6f6002a2cc98fc5d420d379f968a which didn't have region3 yet.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r589784034
##########
File path: arch/xtensa/src/esp32/esp32_allocateheap.c
##########
@@ -96,17 +96,41 @@ void xtensa_add_region(void)
{
void *start;
size_t size;
+ int availregions;
+ int nregions = CONFIG_MM_REGIONS - 1;
+
+#ifdef CONFIG_SMP
+ availregions = 2;
+# ifdef CONFIG_BOARD_LATE_INITIALIZE
+ availregions++;
+# else
+ minfo("A ~3KB heap region can be added to the heap by enabling"
+ " CONFIG_BOARD_LATE_INITLIAZE\n");
Review comment:
Done. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590871509
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -72,9 +72,23 @@
*
* When an internal heap is enabled this region starts at an offset equal to
* the size of the internal heap.
+ *
+ * The QEMU bootloader image is slightly different than the chip's one.
+ * The ROM on PRO and APP uses different regions for static data. In QEMU,
+ * however, we load only one ROM binary, taken from the PRO CPU, and it is
+ * used by both CPUs. So, in QEMU, if we allocate this part early, it will
+ * be clobbered once the APP CPU starts.
+ * We can delay the allocation to when everything has started through the
+ * board_late_initiliaze hook, as is done for the APP data, however this
+ * should be fixed from QEMU side. The following macros, then, just skip
+ * PRO CPU's data when a QEMU image generation is enabled with SMP.
*/
-#define HEAP_REGION2_START 0x3ffe0450
+#if defined(CONFIG_ESP32_QEMU_IMAGE) && defined(CONFIG_SMP)
Review comment:
for me, a separate config is more convenient.
* CONFIG_ESP32_WITH_QEMU without CONFIG_ESP32_QEMU_IMAGE makes sense when building qemu images manually from nuttx or nuttx.bin. (i almost always do this when using qemu for xtensa)
* CONFIG_ESP32_QEMU_IMAGE without CONFIG_ESP32_WITH_QEMU might makes sense when using a fixed qemu in future.
having said that, i can live with the single CONFIG_ESP32_QEMU_IMAGE config. it isn't a blocker of the PR at all.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590027640
##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -72,9 +72,23 @@
*
* When an internal heap is enabled this region starts at an offset equal to
* the size of the internal heap.
+ *
+ * The QEMU bootloader image is slightly different than the chip's one.
+ * The ROM on PRO and APP uses different regions for static data. In QEMU,
+ * however, we load only one ROM binary, taken from the PRO CPU, and it is
+ * used by both CPUs. So, in QEMU, if we allocate this part early, it will
+ * be clobbered once the APP CPU starts.
+ * We can delay the allocation to when everything has started through the
+ * board_late_initiliaze hook, as is done for the APP data, however this
+ * should be fixed from QEMU side. The following macros, then, just skip
+ * PRO CPU's data when a QEMU image generation is enabled with SMP.
*/
-#define HEAP_REGION2_START 0x3ffe0450
+#if defined(CONFIG_ESP32_QEMU_IMAGE) && defined(CONFIG_SMP)
Review comment:
Isn't `CONFIG_ESP32_QEMU_IMAGE` enough? Or you want another one that's going to be auto selected if `CONFIG_ESP32_QEMU_IMAGE` and `CONFIG_SMP` are enabled?
`CONFIG_ESP32_QEMU_IMAGE` is supposed to generate a QEMU image, so maybe here it's being used for more than that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590043536
##########
File path: arch/xtensa/src/esp32/esp32_allocateheap.c
##########
@@ -96,8 +96,15 @@ void xtensa_add_region(void)
{
size_t region2_start = HEAP_REGION2_START + XTENSA_IMEM_REGION_SIZE;
+#ifndef CONFIG_SMP
umm_addregion((FAR void *)region2_start,
(size_t)(uintptr_t)&_eheap - region2_start);
+#else
+ umm_addregion((FAR void *)region2_start,
+ (size_t)HEAP_REGION2_END - region2_start);
+ umm_addregion((FAR void *)HEAP_REGION3_START,
+ (size_t)&_eheap - HEAP_REGION3_START);
Review comment:
~Hmmm... I guess this came from a bad editing of the commit when rebasing..~
Ah no, I think this was here with the "arch/xtensa/esp32: Part of the ROM regions in middle of DRAM are not" commit, but when I did it I didn't have "memory_layout.h", so the macro wasn't added until later.
What do you suggest?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590043536
##########
File path: arch/xtensa/src/esp32/esp32_allocateheap.c
##########
@@ -96,8 +96,15 @@ void xtensa_add_region(void)
{
size_t region2_start = HEAP_REGION2_START + XTENSA_IMEM_REGION_SIZE;
+#ifndef CONFIG_SMP
umm_addregion((FAR void *)region2_start,
(size_t)(uintptr_t)&_eheap - region2_start);
+#else
+ umm_addregion((FAR void *)region2_start,
+ (size_t)HEAP_REGION2_END - region2_start);
+ umm_addregion((FAR void *)HEAP_REGION3_START,
+ (size_t)&_eheap - HEAP_REGION3_START);
Review comment:
Hmmm... I guess this came from a bad editing of the commit when rebasing..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3003: ESP32: Re-organise the different heap regions
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r590875068
##########
File path: arch/xtensa/src/esp32/esp32_allocateheap.c
##########
@@ -96,8 +96,15 @@ void xtensa_add_region(void)
{
size_t region2_start = HEAP_REGION2_START + XTENSA_IMEM_REGION_SIZE;
+#ifndef CONFIG_SMP
umm_addregion((FAR void *)region2_start,
(size_t)(uintptr_t)&_eheap - region2_start);
+#else
+ umm_addregion((FAR void *)region2_start,
+ (size_t)HEAP_REGION2_END - region2_start);
+ umm_addregion((FAR void *)HEAP_REGION3_START,
+ (size_t)&_eheap - HEAP_REGION3_START);
Review comment:
you can history-edit with git to make it cleaner if you want. but i'm not sure if it's worth the effort. it's up to you.
my suggestion is do nothing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org