Skip to content
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

kernel: init: activate FPU for main thread #30455

Merged
merged 1 commit into from
Dec 5, 2020

Conversation

katsuster
Copy link
Member

This patch activates FPU feature for main thread if FPU related
configs (FPU, FPU_SHARING) are enabled.

Signed-off-by: Katsuhiro Suzuki katsuhiro@katsuster.net


This PR will fix the problem of issue #29349.

This patch activates FPU feature for main thread if FPU related
configs (FPU, FPU_SHARING) are enabled.

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
Copy link
Contributor

@andrewboie andrewboie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable.

@nashif nashif merged commit 8453a73 into zephyrproject-rtos:master Dec 5, 2020
@katsuster katsuster deleted the fpu_at_main branch December 5, 2020 10:52
@ioannisg
Copy link
Member

ioannisg commented Jan 26, 2021

@andrewboie I have some concerns about this (not crucial though).

If the main thread is not using the floating point registers, this change unnecessarily decreases the available main thread's stack area (due to a larger stack guard for floating point; that's in Cortex-M). I wonder if we could disallow the main thread to use the FP registers, since the main thread is not an optional one. (users may want FP sharing mode for their own threads, but not necessarily the main thread)

@katsuster
Copy link
Member Author

@ioannisg Indeed... So both forced enable / disable have a different problem.
Is it better to change the FPU availability in main thread by user specified way?
(Ex. CONFIG_USE_FPU_MAIN_THREAD...)

@ioannisg
Copy link
Member

I feel that the problem here is more generic: it's that the main thread is non-optional, but user cannot pass their desired configuration at the application code; that has to be done in init.c in a cross-arch fashion. Perhaps we should have an agreement on what the main thread is allowed to do (floating point usage, etc). Main thread is allowed to drop to user mode ( but we do that in main(), but the K_FP_REGS is passed at init).

Is it better to change the FPU availability in main thread by user specified way?
(Ex. CONFIG_USE_FPU_MAIN_THREAD...)

I feel this is getting too complex. I would prefer a rule that under FP_SHARING the main thread is always FP-capable or it never is.

Why does risc-v need FP capabilities on the main thread?

@katsuster
Copy link
Member Author

Originally, issue #29349 is motivation of this PR. But this is not RISC-V specialized issue.

I think it's an usual request that is an user want to use FPU in main thread.
And not only in main thread but also other system threads that includes work queue.

There are 2 types of Zephyr's thread; User-defined and System-defined (main, work queue, log thread, etc.).
Only System-defined threads never allow to use FPU even if an user set FP-capable config in Kconfig.
Is this intuitive...?

@ioannisg
Copy link
Member

There are 2 types of Zephyr's thread; User-defined and System-defined (main, work queue, log thread, etc.).
Only System-defined threads never allow to use FPU even if an user set FP-capable config in Kconfig.
Is this intuitive...?

That could be an option, yes. But we need to think more about it. Pre-tagging a thread with K_FP_REGS implies the thread is going to use the FP context, and I don't see why the main thread needs to hard-code this behavior.

@katsuster
Copy link
Member Author

Hmm... I don't want to hard-code, it's not purpose. Just I want to use FPU in system threads.
I think this PR can be removed if FPU can be enabled by user in main thread (or other system threads).

But in current Zephyr implementation, it seems that we can do that only on x86 platform (k_float_enable)...?

@ioannisg
Copy link
Member

Nope, we can do that for ARM Cortex-M as well.

@katsuster
Copy link
Member Author

@ioannisg
I want to clarify about conclusion. A acceptable solution for RISC-V and Cortex-M (and others?) is that we will implement like as k_float_enable() for RISC-V?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants