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 2022/05/09 09:15:00 UTC
[GitHub] [incubator-nuttx] zhuyanlinzyl opened a new pull request, #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
zhuyanlinzyl opened a new pull request, #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222
1 move fpu register to XCP_REGS
2 move save & restore fpu register to context_save/restore
Consistency with other archs.
Signed-off-by: zhuyanlin <zh...@xiaomi.com>
## Summary
This PR move fpu register to XCP, in Consistency with other archs
## Impact
No
## Testing
Test in xtensa board, with fpu coprocosser
#define XCHAL_CP_NUM 1 /* number of coprocessors */
#define XCHAL_CP_MAX 2 /* max CP ID + 1 (0 if none) */
#define XCHAL_CP_MASK 0x02 /* bitmask of all CPs by ID */
ostest test OK
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1125693281
`curl: (28) Failed to connect to 1.phyplusinc.com port 80: Connection timed out` . Network connection failed. Please retrigger the CI @xiaoxiang781216
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1126224670
BTW, does ESP-IDF's FreeRTOS hook FPU exception every time to support the full lazy save/restore?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1121080021
> Yes, It increase stack size. But it decrease xcp struct size(heap size). And with the commit `arch/xtensa: Replace the xcp context with stack context to improve context switching`. It will decrease time in fpu context switch.
That's true. But it's important to note that the heap is set and managed by the system, where the thread stack is user selectable. There is no problem saving the state of the coprocessors in the stack, but users should be aware of this when selecting the size of the stack, especially if they have more than one coprocessor.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r869082706
##########
arch/xtensa/src/common/xtensa_coproc.S:
##########
@@ -112,77 +113,75 @@ _xtensa_coproc_savestate:
/* Move the address of the thread state save area to R15 */
- mov a15, a2 /* A15 is now the address of the save area */
-
+ mov a3, a2 /* A15 is now the address of the save area */
Review Comment:
```suggestion
mov a3, a2 /* A3 is now the address of the save area */
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1120960574
One more thing, if we want to keep this approach (i.e. coprocessor state along side the integer registers), we can then restore the signatures for the save, restore and switch context functions. We don't need the address of the saving area anymore, we can work just with its value.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1126111660
> > Here is the current implementation of armv7-m/armv8-m:
>
> > If the task use float point, FPU will be saved in the exception entry, otherwise only integer context is saved
> > If the next selected task use float point, FPU will be restored before exception return, otherwise only integer context is restored
>
> Is this still in place for all of the FPU registers or just for those automatically saved by the architecture? I remember seeing a commit that removed lazy context switching for ARM.
The old ARM lazy switch is different from ESP-IDF:
1. Skip save/restore FPU context if no thread switch happen in the interrupt handler
2. Save/restore FPU context regardless whether the interrupted thread use FPU or not if the switch happen
Three problems with this implementation:
1. Compiler may generate NEON(use FPU register) instruction automatically(see the discussion in PR #5662)
2. It's very inefficient to save/restore all thread's FPU context since only a few thread in a system will really use FPU
3. The thread most likely will be switched out after interrupt finish process in the tickless mode
Item 1 must be fixed to avoid the wrong behavior, item 3 make the gain from skipping save/restore is small. So the new implementation:
1. Don't delay the save/restore of FPU context
2. Only save/restore FPU context if the thread really use FPU
RiSCV has one additional optimization:
- Skip save the FPU context if FPU is still in clean state
- Don't restore the FPU context if FPU isn't dirty
So, RISCV is very close what ESP-IDF do for xtensa on FreeRTOS.
>
> > Since RISCV FPU record whether it's content is changed, FPU save/restore can be skipped in this case:
> > https://github.com/apache/incubator-nuttx/blob/master/arch/risc-v/src/common/riscv_macros.S#L105-L108
>
> I am aware of this part, but isn't the FPU state specific to each CPU? Or can we copy one CPU's FPU state from another?
We didn't tested FPU on SMP before, but it's very normal to copy the FPU register from the save context(on the stack) to the next CPU.
>
> > Basically, we need a new array g_lazy_regs[NCPU](just like g_current_regs[NCPU]) to track the task who's fpu context doesn't save yet.
>
> Thanks for the idea, I'll see how we can do this for NuttX.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r867949392
##########
arch/xtensa/include/irq.h:
##########
@@ -155,12 +168,6 @@ struct xcptcontext
uint32_t *regs;
-#if XCHAL_CP_NUM > 0
- /* Co-processor save area */
-
- struct xtensa_cpstate_s cpstate;
Review Comment:
And I don't think we need `struct xtensa_cpstate_s cpstate` to save the coprocessors state in every task. I think just xtensa_set_cpenable in up_initialize is enough now. Until we realize the LAZY coprocessors context switch
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r869248972
##########
arch/xtensa/src/common/xtensa_context.S:
##########
@@ -200,6 +200,15 @@ _xtensa_context_save:
#endif
+#if XCHAL_CP_NUM > 0
+ s32i a0, sp, (4 * REG_TMP0) /* Save return address */
+
+ mov a2, sp
+ call0 _xtensa_coproc_savestate
Review Comment:
should we inline _xtensa_coproc_savestate here or change it to macro?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1126646638
But to implement the real lazy save/restore(only when the new task really use FPU), I think you have to disable coprocessor every time when the schedule happen.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1125787938
@Ouss4 could you please approve and merge if you agree.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1126582583
> BTW, does ESP-IDF's FreeRTOS hook FPU exception every time to support the full lazy save/restore?
The exception triggers only if a coprocessor was disabled and used. So managing the CPENEABLE register and few variables to set the owner of a particular coprocessor, we can reduce the exceptions and the saving/restoring.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1126213529
Thanks for the comment @xiaoxiang781216
> We didn't tested FPU on SMP before, but it's very normal to copy the FPU register from the save context(on the stack) to the next CPU.
Yes, the part of copying from the stack to the CPU register is simple, but the opposite is problematic. If we want to lazily copy registers from CPU to stack this has to be done from the same CPU because we can't access others CPUs.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1122289491
I have test in Qemu esp32, `ostest` run success. @Ouss4
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r870048764
##########
arch/xtensa/src/common/xtensa_coproc.S:
##########
@@ -112,77 +113,75 @@ _xtensa_coproc_savestate:
/* Move the address of the thread state save area to R15 */
- mov a15, a2 /* A15 is now the address of the save area */
-
+ mov a3, a2 /* A15 is now the address of the save area */
+ addi a3, a3, (4 * XCPTCONTEXT_REGS)
Review Comment:
Thanks for the explanation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r869128661
##########
arch/xtensa/src/common/xtensa_coproc.S:
##########
@@ -112,77 +113,75 @@ _xtensa_coproc_savestate:
/* Move the address of the thread state save area to R15 */
- mov a15, a2 /* A15 is now the address of the save area */
-
+ mov a3, a2 /* A15 is now the address of the save area */
+ addi a3, a3, (4 * XCPTCONTEXT_REGS)
Review Comment:
You can review my modify in arch/xtensa/include/irq.h.
The stack when interrupt happend:
* -----------------------------------
* | Reserve area (0x20) |
* -----------------------------------
* | FPU(coproc) context |
* -----------------------------------
* | XCP common REGS |
* ----------------------------------| <- SP
so, SP pointer add `4 * XCPTCONTEXT_REGS`, will pointer to the `FPU(coproc) context` area first address.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1125842200
>Here is the current implementation of armv7-m/armv8-m:
> If the task use float point, FPU will be saved in the exception entry, otherwise only integer context is saved
> If the next selected task use float point, FPU will be restored before exception return, otherwise only integer context is restored
Is this still in place for all of the FPU registers or just for those automatically saved by the architecture? I remember seeing a commit that removed lazy context switching for ARM.
> Since RISCV FPU record whether it's content is changed, FPU save/restore can be skipped in this case:
https://github.com/apache/incubator-nuttx/blob/master/arch/risc-v/src/common/riscv_macros.S#L105-L108
I am aware of this part, but isn't the FPU state specific to each CPU? Or can we copy one CPU's FPU state from another?
> Basically, we need a new array g_lazy_regs[NCPU](just like g_current_regs[NCPU]) to track the task who's fpu context doesn't save yet.
Thanks for the idea, I'll see how we can do this for NuttX.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r869082599
##########
arch/xtensa/src/common/xtensa_coproc.S:
##########
@@ -112,77 +113,75 @@ _xtensa_coproc_savestate:
/* Move the address of the thread state save area to R15 */
- mov a15, a2 /* A15 is now the address of the save area */
-
+ mov a3, a2 /* A15 is now the address of the save area */
+ addi a3, a3, (4 * XCPTCONTEXT_REGS)
Review Comment:
Why `4 * XCPTCONTEXT_REGS` and not `XCPTCONTEXT_SIZE`? `XCPTCONTEXT_SIZE` is the full interrupt frame and it contains extra bytes required by the ABI, is that taken under consideration when saving/restoring?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1125297087
> @zhuyanlinzyl @xiaoxiang781216 A bit unrelated, but one thing regarding lazy context switching is this issue: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/freertos-smp.html#floating-point-usage
ESP-IDF FreeRTOS delay the save/restore until there is new task try to use floating point, it's great but:
1. It's hard to implement
2. Interrupt can't use the floating point
3. Task need to pin to a specific cpu
Here is the current implementation of armv7-m/armv8-m:
1. If the task use float point, FPU will be saved in the exception entry, otherwise only integer context is saved
2. If the next selected task use float point, FPU will be restored before exception return, otherwise only integer context is restored
It isn't optimized as ESP-IDF in one case: no any task use FPU when the task who use FPU get running again.
>
> If we gonna have to do that, we'll need to find a way to have the TCB in the context code(or only the address of xcp->regs and then offset), we talked elsewhere that this is somewhat cumbersome.
Basically, we need a new array g_lazy_regs[NCPU](just like g_current_regs[NCPU]) to track the task who's fpu context doesn't save yet.
> How is this managed for RISC-V for instance? Do you have any multi-core RISC-V chips with FPU?
RISC-V is much easier than other arch since the designer keep the lazy save/restore in mind when he define the spec. Please reference "3.1.6.6 Extension Context Status in mstatus Register" here:
https://github.com/riscv/riscv-isa-manual/releases/download/Priv-v1.12/riscv-privileged-20211203.pdf
Since RISCV FPU record whether it's content is changed, FPU save/restore can be skipped in this case:
https://github.com/apache/incubator-nuttx/blob/master/arch/risc-v/src/common/riscv_macros.S#L105-L108
> For Xtensa, I couldn't find a lot of other references, even in Linux the fast coprocessor handling is marked experimental.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 merged pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 merged PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r869976515
##########
arch/xtensa/src/common/xtensa_coproc.S:
##########
@@ -112,77 +113,75 @@ _xtensa_coproc_savestate:
/* Move the address of the thread state save area to R15 */
- mov a15, a2 /* A15 is now the address of the save area */
-
+ mov a3, a2 /* A15 is now the address of the save area */
+ addi a3, a3, (4 * XCPTCONTEXT_REGS)
Review Comment:
let's copy this to the assemble code to help the understanding.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r867949392
##########
arch/xtensa/include/irq.h:
##########
@@ -155,12 +168,6 @@ struct xcptcontext
uint32_t *regs;
-#if XCHAL_CP_NUM > 0
- /* Co-processor save area */
-
- struct xtensa_cpstate_s cpstate;
Review Comment:
And I don't think we need `struct xtensa_cpstate_s cpstate` to save the coprocessors state in every task. I think just xtensa_set_cpenable in up_initialize is enough now. Until we realize the LAZY coprocessors registers
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1125699093
> `curl: (28) Failed to connect to 1.phyplusinc.com port 80: Connection timed out` . Network connection failed. Please retrigger the CI @xiaoxiang781216
Let' me trigger it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1120916995
Please note that one significant impact of this is an increase of the stack size. This moves all the coprocessors saving area to the stack not just the FPU. For SoCs where they have more than just one coprocessor, like the ESP32-S3, users will need to be careful when selecting their stack sizes.
I have more or less the same change, although for me I moved the saving area to the beginning of the stack and not in the interrupt frame, this is to experiment with lazy context switching.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r867862588
##########
arch/xtensa/include/irq.h:
##########
@@ -155,12 +168,6 @@ struct xcptcontext
uint32_t *regs;
-#if XCHAL_CP_NUM > 0
- /* Co-processor save area */
-
- struct xtensa_cpstate_s cpstate;
Review Comment:
It looks like this is fixed for only one owner of the coprocessors and saving/restoring in any context switch. `struct xtensa_cpstate_s` also contained other members to help managing different owners of coprocessors.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r869241827
##########
arch/xtensa/src/common/xtensa_context.S:
##########
@@ -200,6 +200,15 @@ _xtensa_context_save:
#endif
+#if XCHAL_CP_NUM > 0
+ s32i a0, sp, (4 * REG_TMP0) /* Save return address */
+
+ mov a2, sp
+ call0 _xtensa_coproc_savestate
Review Comment:
aglin _xtensa_coproc_savestate
##########
arch/xtensa/include/irq.h:
##########
@@ -118,12 +118,25 @@
/* Storage for overlay state */
# error Overlays not supported
-# define XCPTCONTEXT_REGS _REG_OVLY_START
+# define _REG_CP_START _REG_OVLY_START
#else
-# define XCPTCONTEXT_REGS _REG_OVLY_START
+# define _REG_CP_START _REG_OVLY_START
#endif
-#define XCPTCONTEXT_SIZE ((4 * XCPTCONTEXT_REGS) + 0x20)
+#if XCHAL_CP_NUM > 0
+# if (XCHAL_TOTAL_SA_ALIGN == 8) && ((_REG_CP_START & 1) == 1)
+ /* Fpu first address must align to cp align size. */
+
+# define REG_CP_DUMMY (_REG_CP_START + 0)
+# define XCPTCONTEXT_REGS (_REG_CP_START + 1)
+# else
+# define XCPTCONTEXT_REGS _REG_CP_START
Review Comment:
add extra space
##########
arch/xtensa/src/common/xtensa_context.S:
##########
@@ -239,6 +248,14 @@ _xtensa_context_save:
_xtensa_context_restore:
+#if XCHAL_CP_NUM > 0
+ s32i a0, a2, (4 * REG_TMP0) /* Save return address */
+
+ call0 _xtensa_coproc_restorestate
Review Comment:
ditto
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1122219847
hi @Ouss4 : I update the patch move fpu to XCP_REGS.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1123930183
LGTM, @Ouss4 please take a look.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1125842780
> @Ouss4 could you please approve and merge if you agree.
Let me please update the internal branch to run the tests, I'll merge it after if all good.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1126220287
> Thanks for the comment @xiaoxiang781216
>
> > We didn't tested FPU on SMP before, but it's very normal to copy the FPU register from the save context(on the stack) to the next CPU.
>
> Yes, the part of copying from the stack to the CPU register is simple, but the opposite is problematic. If we want to lazily copy registers from CPU to stack this has to be done from the same CPU because we can't access others CPUs.
Yes, it's really hard to copy FPU register directly between different CPU core. I don't know whether the full lazy context save/restore is good if the thread has to be pinned to a specific core once it use FPU operation, or the current arm/riscv implementation is enough.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1124887380
@zhuyanlinzyl @xiaoxiang781216 A bit unrelated, but one thing regarding lazy context switching is this issue: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/freertos-smp.html#floating-point-usage
If we gonna have to do that, we'll need to find a way to have the TCB in the context code(or only the address of xcp->regs and then offset), we talked elsewhere that this is somewhat cumbersome.
How is this managed for RISC-V for instance? Do you have any multi-core RISC-V chips with FPU?
For Xtensa, I couldn't find a lot of other references, even in Linux the fast coprocessor handling is marked experimental.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r871785422
##########
arch/xtensa/src/common/xtensa_coproc.S:
##########
@@ -112,77 +113,87 @@ _xtensa_coproc_savestate:
/* Move the address of the thread state save area to R15 */
- mov a15, a2 /* A15 is now the address of the save area */
+ mov a3, a2 /* A3 is now the address of the save area */
+
+ /* The stack when interrupt happened (the register a2)
+ * ----------------------------------------------------
+ * | Reserve area (0x20) |
+ * ----------------------------------------------------
+ * | Coproc context |
+ * ----------------------------------------------------
+ * | Xtensa common regs |
+ * ---------------------------------------------------| <- SP
+ */
+
+ addi a3, a3, (4 * XCPTCONTEXT_REGS)
/* CPENABLE should show which CPs are enabled. */
- rsr a2, CPENABLE /* a2 = which CPs are enabled */
- beqz a2, .Ldone1 /* Quick exit if none */
+ rsr a2, CPENABLE /* a2 = which CPs are enabled */
+ beqz a2, .Ldone1 /* Quick exit if none */
- s16i a2, a15, XTENSA_CPSTORED /* Save mask of CPs being stored */
- movi a13, _xtensa_coproc_saoffsets /* Array of CP save offsets */
- addi a15, a15, XTENSA_CPASA /* a15 = base of aligned save area */
+ movi a13, _xtensa_coproc_saoffsets /* Array of CP save offsets */
#if XCHAL_CP0_SA_SIZE > 0
- bbci.l a2, 0, 2f /* CP 0 not enabled */
- l32i a14, a13, 0 /* a14 = _xtensa_coproc_saoffsets[0] */
- add a3, a14, a15 /* a3 = save area for CP 0 */
+ bbci.l a2, 0, 2f /* CP 0 not enabled */
+ l32i a14, a13, 0 /* a14 = _xtensa_coproc_saoffsets[0] */
+ add a3, a14, a3 /* a3 = save area for CP 0 */
Review Comment:
Can you please align code with TABs? Here and other similar places
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1125628955
Please review again @Ouss4 @pkarashchenko
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1121249636
> One more thing, if we want to keep this approach (i.e. coprocessor state along side the integer registers), we can then restore the signatures for the save, restore and switch context functions. We don't need the address of the saving area anymore, we can work just with its value.
Yes, @zhuyanlinzyl please restore these function prototype and enable FPU test, so we can do some basic cp state testing.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1122229798
@zhuyanlinzyl how about `up_saveusercontext` (found in `xtensa_saveusercontext.c`), it's that function that's somewhat problematic since it's public. The change should straightforward as in xtensa_contextrestore.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1121016687
> Please note that one significant impact of this is an increase of the stack size. This moves all the coprocessors saving area to the stack not just the FPU. For SoCs where they have more than just one coprocessor, like the ESP32-S3, users will need to be careful when selecting their stack sizes.
>
Yes, It increase stack size. But it decrease xcp struct size(heap size). And with the commit `arch/xtensa: Replace the xcp context with stack context to improve context switching`. It will decrease time in fpu context switch.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r867990438
##########
arch/xtensa/include/irq.h:
##########
@@ -155,12 +168,6 @@ struct xcptcontext
uint32_t *regs;
-#if XCHAL_CP_NUM > 0
- /* Co-processor save area */
-
- struct xtensa_cpstate_s cpstate;
Review Comment:
Yes, that is correct. Right now we don't have any lazy context switching implemented, I just wanted to point out that those members are now gone.
BTW, xtensa_context.h will need to be adapted too and the `struct xtensa_cpstate_s` can be commented out or deleted.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r867947463
##########
arch/xtensa/include/irq.h:
##########
@@ -155,12 +168,6 @@ struct xcptcontext
uint32_t *regs;
-#if XCHAL_CP_NUM > 0
- /* Co-processor save area */
-
- struct xtensa_cpstate_s cpstate;
Review Comment:
The reason I move the `struct xtensa_cpstate_s cpstate` is: lazy coprocessors registers context switching is not support now, and the `cpenable` and `cpstored` is always enable and same in every task.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a diff in pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on code in PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#discussion_r869082596
##########
arch/xtensa/src/common/xtensa_coproc.S:
##########
@@ -112,77 +113,75 @@ _xtensa_coproc_savestate:
/* Move the address of the thread state save area to R15 */
- mov a15, a2 /* A15 is now the address of the save area */
-
+ mov a3, a2 /* A15 is now the address of the save area */
+ addi a3, a3, (4 * XCPTCONTEXT_REGS)
Review Comment:
Why `4 * XCPTCONTEXT_REGS` and not `XCPTCONTEXT_SIZE`? `XCPTCONTEXT_SIZE` is the full interrupt frame and it contains extra bytes required by the ABI, is that taken under consideration when saving/restoring?
##########
arch/xtensa/src/common/xtensa_coproc.S:
##########
@@ -112,77 +113,75 @@ _xtensa_coproc_savestate:
/* Move the address of the thread state save area to R15 */
- mov a15, a2 /* A15 is now the address of the save area */
-
+ mov a3, a2 /* A15 is now the address of the save area */
Review Comment:
```suggestion
mov a3, a2 /* A3 is now the address of the save area */
```
```suggestion
mov a3, a2 /* A15 is now the address of the save area */
```
##########
arch/xtensa/src/common/xtensa_coproc.S:
##########
@@ -112,77 +113,75 @@ _xtensa_coproc_savestate:
/* Move the address of the thread state save area to R15 */
- mov a15, a2 /* A15 is now the address of the save area */
-
+ mov a3, a2 /* A15 is now the address of the save area */
Review Comment:
```suggestion
mov a3, a2 /* A3 is now the address of the save area */
```
```suggestion
mov a3, a2 /* A15 is now the address of the save area */
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1124880206
> ok, I can remove struct `xtensa_cpstate_s`. How about void xtensa_coproc_enable(struct xtensa_cpstate_s *cpstate, int cpset) function ? modify to void xtensa_coproc_enable(int cpset) instead ? @Ouss4
I forgot about this function. We don't use it anywhere anyway, there is one commented out call in esp32_cpustart.c. I suggest we remove that call, but also keep the functions for future use.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1124704317
@xiaoxiang781216 @zhuyanlinzyl how about the `xtensa_cpstate_s` struct we still have in `xtensa_coproc.h`. There is no use for it now. I think we can just `#if 0` it with a comment that it will be used when lazy context switching is implemented.
BTW, I pushed a branch internally with this change, all the defconfigs passed the tests.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhuyanlinzyl commented on pull request #6222: xtensa: move fpu register to XCPTCONTEXT_REGS
Posted by GitBox <gi...@apache.org>.
zhuyanlinzyl commented on PR #6222:
URL: https://github.com/apache/incubator-nuttx/pull/6222#issuecomment-1124823186
ok, I can removew struct `xtensa_cpstate_s`. How about void xtensa_coproc_enable(struct xtensa_cpstate_s *cpstate, int cpset) function ? modify to void xtensa_coproc_enable(int cpset) instead ? @Ouss4
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org