Signed-off-by: Colin Cross <[email protected]>
---
arch/arm/vfp/vfpmodule.c | 24 ++++++++++++++++++++++++
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 0797cb5..8b27c18 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -21,6 +21,7 @@
#include <asm/cputype.h>
#include <asm/thread_notify.h>
#include <asm/vfp.h>
+#include <asm/cpu_pm.h>
#include "vfpinstr.h"
#include "vfp.h"
@@ -149,6 +150,28 @@ static struct notifier_block vfp_notifier_block = {
.notifier_call = vfp_notifier,
};
+static int vfp_idle_notifier(struct notifier_block *self, unsigned long cmd,
+ void *v)
+{
+ u32 fpexc = fmrx(FPEXC);
+ unsigned int cpu = smp_processor_id();
+
+ if (cmd != CPU_PM_ENTER)
+ return NOTIFY_OK;
+
+ /* The VFP may be reset in idle, save the state */
+ if ((fpexc & FPEXC_EN) && last_VFP_context[cpu]) {
+ vfp_save_state(last_VFP_context[cpu], fpexc);
+ last_VFP_context[cpu]->hard.cpu = cpu;
+ }
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block vfp_idle_notifier_block = {
+ .notifier_call = vfp_idle_notifier,
+};
+
/*
* Raise a SIGFPE for the current process.
* sicode describes the signal being raised.
@@ -549,6 +572,7 @@ static int __init vfp_init(void)
vfp_vector = vfp_support_entry;
thread_register_notifier(&vfp_notifier_block);
+ cpu_pm_register_notifier(&vfp_idle_notifier_block);
vfp_pm_init();
/*
--
1.7.3.1
Colin,
On Thu, 2011-02-10 at 21:31 +0000, Colin Cross wrote:
> +static int vfp_idle_notifier(struct notifier_block *self, unsigned long cmd,
> + void *v)
> +{
> + u32 fpexc = fmrx(FPEXC);
> + unsigned int cpu = smp_processor_id();
> +
> + if (cmd != CPU_PM_ENTER)
> + return NOTIFY_OK;
> +
> + /* The VFP may be reset in idle, save the state */
> + if ((fpexc & FPEXC_EN) && last_VFP_context[cpu]) {
> + vfp_save_state(last_VFP_context[cpu], fpexc);
> + last_VFP_context[cpu]->hard.cpu = cpu;
> + }
Should we only handle the case where the VFP is enabled? At context
switch we disable the VFP and re-enable it when an application tries to
use it but it will remain disabled even the application hasn't used the
VFP. So switching to the idle thread would cause the VFP to be disabled
but the state not necessarily saved.
On SMP systems, we save the VFP at every context switch to deal with the
thread migration (though I have a plan to make this lazily on SMP as
well). On UP however, we don't save the VFP registers at context switch,
we just disable it and save it lazily if used later in a different task
Something like below (untested):
if (last_VFP_context[cpu]) {
vfp_save_state(last_VFP_context[cpu], fpexc);
/* force a reload when coming back from idle */
last_VFP_context[cpu] = NULL;
fmxr(FPEXC, fpexc & ~FPEXC_EN);
}
The last line (disabling) may not be necessary if we know that it comes
back from idle as disabled.
I wonder whether the current vfp_pm_suspend() function needs fixing for
UP systems as well. It is find if the hardware preserves the VFP
registers (which may not be the case).
--
Catalin
On Fri, Feb 11, 2011 at 12:12:25PM +0000, Catalin Marinas wrote:
> On SMP systems, we save the VFP at every context switch to deal with the
> thread migration (though I have a plan to make this lazily on SMP as
> well).
I'm not sure it's worth the complexity. You'd have to do an IPI to the
old CPU to provoke it to save the context from its VFP unit. You'd have
to do that in some kind of atomic way as the old CPU may be in the middle
of already saving it. You're also going to have to add locking to the
last_VFP_context[] array as other CPUs will be accessing non-local
entries, and that means doing locking in assembly. Yuck.
No, let's not go there. Stick with what we currently have which works
well.
On 11 February 2011 12:24, Russell King - ARM Linux
<[email protected]> wrote:
> On Fri, Feb 11, 2011 at 12:12:25PM +0000, Catalin Marinas wrote:
>> On SMP systems, we save the VFP at every context switch to deal with the
>> thread migration (though I have a plan to make this lazily on SMP as
>> well).
>
> I'm not sure it's worth the complexity. You'd have to do an IPI to the
> old CPU to provoke it to save the context from its VFP unit. You'd have
> to do that in some kind of atomic way as the old CPU may be in the middle
> of already saving it. You're also going to have to add locking to the
> last_VFP_context[] array as other CPUs will be accessing non-local
> entries, and that means doing locking in assembly. Yuck.
I wasn't thinking about that, too complex indeed. But it may be easier
to detect thread migration, possibly with some hooks into generic
scheduler and only save the VFP state at that point. I haven't looked
in detail but I heard the x86 people have patches for something
similar.
--
Catalin
On Fri, Feb 11, 2011 at 4:12 AM, Catalin Marinas
<[email protected]> wrote:
> Colin,
>
> On Thu, 2011-02-10 at 21:31 +0000, Colin Cross wrote:
>> +static int vfp_idle_notifier(struct notifier_block *self, unsigned long cmd,
>> + ? ? ? void *v)
>> +{
>> + ? ? ? u32 fpexc = fmrx(FPEXC);
>> + ? ? ? unsigned int cpu = smp_processor_id();
>> +
>> + ? ? ? if (cmd != CPU_PM_ENTER)
>> + ? ? ? ? ? ? ? return NOTIFY_OK;
>> +
>> + ? ? ? /* The VFP may be reset in idle, save the state */
>> + ? ? ? if ((fpexc & FPEXC_EN) && last_VFP_context[cpu]) {
>> + ? ? ? ? ? ? ? vfp_save_state(last_VFP_context[cpu], fpexc);
>> + ? ? ? ? ? ? ? last_VFP_context[cpu]->hard.cpu = cpu;
>> + ? ? ? }
>
> Should we only handle the case where the VFP is enabled? At context
> switch we disable the VFP and re-enable it when an application tries to
> use it but it will remain disabled even the application hasn't used the
> VFP. So switching to the idle thread would cause the VFP to be disabled
> but the state not necessarily saved.
Right
> On SMP systems, we save the VFP at every context switch to deal with the
> thread migration (though I have a plan to make this lazily on SMP as
> well). On UP however, we don't save the VFP registers at context switch,
> we just disable it and save it lazily if used later in a different task
>
> Something like below (untested):
>
> ? ? ? ?if (last_VFP_context[cpu]) {
> ? ? ? ? ? ? ? ?vfp_save_state(last_VFP_context[cpu], fpexc);
> ? ? ? ? ? ? ? ?/* force a reload when coming back from idle */
> ? ? ? ? ? ? ? ?last_VFP_context[cpu] = NULL;
> ? ? ? ? ? ? ? ?fmxr(FPEXC, fpexc & ~FPEXC_EN);
> ? ? ? ?}
>
> The last line (disabling) may not be necessary if we know that it comes
> back from idle as disabled.
It shouldn't be necessary, the context switch into the idle thread
should have disabled it, but it doesn't hurt. We should also disable
it when exiting idle.
> I wonder whether the current vfp_pm_suspend() function needs fixing for
> UP systems as well. It is find if the hardware preserves the VFP
> registers (which may not be the case).
I think there is a case where the VFP registers can be lost in suspend
on UP platforms that don't save the VFP registers in their platform
suspend. If a thread is using the VFP, and then context switches to a
thread that does not use VFP but triggers suspend by writing to
/sys/power/state, vfp_pm_suspend will be called with the VFP disabled
but the registers not saved. I think this would work:
/* save state for resumption */
if (last_VFP_context[ti->cpu]) {
printk(KERN_DEBUG "%s: saving vfp state\n", __func__);
vfp_save_state(last_VFP_context[ti->cpu], fpexc);
/* disable, just in case */
fmxr(FPEXC, fpexc & ~FPEXC_EN);
}
If the thread that wrote to /sys/power/state is using VFP,
last_VFP_context will be the same as ti->vfpstate, so we can always
save last_VFP_context.
On Fri, Feb 11, 2011 at 11:50 AM, Colin Cross <[email protected]> wrote:
>> Something like below (untested):
>>
>> ? ? ? ?if (last_VFP_context[cpu]) {
>> ? ? ? ? ? ? ? ?vfp_save_state(last_VFP_context[cpu], fpexc);
>> ? ? ? ? ? ? ? ?/* force a reload when coming back from idle */
>> ? ? ? ? ? ? ? ?last_VFP_context[cpu] = NULL;
>> ? ? ? ? ? ? ? ?fmxr(FPEXC, fpexc & ~FPEXC_EN);
>> ? ? ? ?}
One more fix is necessary, the VFP will usually not be enabled when
this is called. The VFP needs to be enabled before vfp_save_state,
and then disabled after.
> ? ? ? ?/* save state for resumption */
> ? ? ? ?if (last_VFP_context[ti->cpu]) {
> ? ? ? ? ? ? ? ?printk(KERN_DEBUG "%s: saving vfp state\n", __func__);
> ? ? ? ? ? ? ? ?vfp_save_state(last_VFP_context[ti->cpu], fpexc);
>
> ? ? ? ? ? ? ? ?/* disable, just in case */
> ? ? ? ? ? ? ? ?fmxr(FPEXC, fpexc & ~FPEXC_EN);
> ? ? ? ?}
Same fix is needed here.