drivers: perf: arm: implement CPU_PM notifier

When a CPU is suspended (either through suspend-to-RAM or CPUidle), its PMU registers content can be lost, which means that counters registers values that were initialized on power down entry have to be reprogrammed on power-up to make sure the counters set-up is preserved (ie on power-up registers take the reset values on Cold or Warm reset, which can be architecturally UNKNOWN).

To guarantee seamless profiling conditions across a core power down this patch adds a CPU PM notifier to ARM pmus, that upon CPU PM entry/exit from low-power states saves/restores the pmu registers set-up (by using the ARM perf API), so that the power-down/up cycle does not affect the perf behaviour (apart from a black-out period between power-up/down CPU PM notifications that is unavoidable).

Signed-off-by Lorenzo Pieralisi <[email protected]>

Cc Ashwin Chaugule <[email protected]>

Cc Will Deacon <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Mathieu Poirier <[email protected]>

Cc Mark Rutland <[email protected]>

---
 drivers/perf/arm_pmu.c       | 95 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h |  1 +
 2 files changed, 96 insertions(+)

drivers/perf/arm_pmu.c +42 -0

index 166637f..12317a9 100644
@@ -13,6 +13,7 @@
 
 #include <linux/bitmap.h>
 #include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/of_device.h>

@@ -710,6 +711,93 @@ static int cpu_pmu_notify(struct notifier_block *b, unsigned long action,
        return NOTIFY_OK;
 }
 
+#ifdef CONFIG_CPU_PM
+static void cpu_pm_pmu_setup(struct arm_pmu *armpmu, unsigned long cmd)
+{
+       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+       struct perf_event *event;
+       int idx;
+
+       for (idx = 0; idx < armpmu->num_events; idx++) {
+               /*
+                * If the counter is not used skip it, there is no
+                * need of stopping/restarting it.
+                */
+               if (!test_bit(idx, hw_events->used_mask))
+                       continue;
+
+               event = hw_events->events[idx];
+
+               switch (cmd) {
+               case CPU_PM_ENTER:
+                       /*
+                        * Stop and update the counter
+                        */
+                       armpmu_stop(event, PERF_EF_UPDATE);
+                       break;
+               case CPU_PM_EXIT:
+               case CPU_PM_ENTER_FAILED:
+                        /* Restore and enable the counter */
+                       armpmu_start(event, PERF_EF_RELOAD);
+                       break;
+               default:
+                       break;
+               }
+       }
+}
+
+static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
+                            void *v)
+{
+       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
+       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);

drivers/perf/arm_pmu.c +43 -0

+
+       if (!enabled)
+               return NOTIFY_OK;
+
+       switch (cmd) {
+       case CPU_PM_ENTER:
+               armpmu->stop(armpmu);
+               cpu_pm_pmu_setup(armpmu, cmd);
+               break;
+       case CPU_PM_EXIT:
+               cpu_pm_pmu_setup(armpmu, cmd);
+       case CPU_PM_ENTER_FAILED:
+               armpmu->start(armpmu);
+               break;
+       default:
+               return NOTIFY_DONE;
+       }
+
+       return NOTIFY_OK;
+}
+
+static int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu)
+{
+       cpu_pmu->cpu_pm_nb.notifier_call = cpu_pm_pmu_notify;
+       return cpu_pm_register_notifier(&cpu_pmu->cpu_pm_nb);
+}
+
+static void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu)
+{
+       cpu_pm_unregister_notifier(&cpu_pmu->cpu_pm_nb);
+}
+#else
+static inline int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu) { return 0; }
+static inline void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu) { }
+#endif
+
 static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
 {
        int err;

@@ -725,6 +813,10 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
        if (err)
                goto out_hw_events;
 
+       err = cpu_pm_pmu_register(cpu_pmu);
+       if (err)
+               goto out_unregister;
+
        for_each_possible_cpu(cpu) {
                struct pmu_hw_events *events = per_cpu_ptr(cpu_hw_events, cpu);
                raw_spin_lock_init(&events->pmu_lock);

@@ -746,6 +838,8 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
 
        return 0;
 
+out_unregister:
+       unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
 out_hw_events:
        free_percpu(cpu_hw_events);
        return err;

@@ -753,6 +847,7 @@ out_hw_events:
 
 static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
 {
+       cpu_pm_pmu_unregister(cpu_pmu);
        unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
        free_percpu(cpu_pmu->hw_events);
 }

include/linux/perf/arm_pmu.h +1 -1

index 83b5e34..9ffa316 100644
@@ -107,6 +107,7 @@ struct arm_pmu {
        struct platform_device  *plat_device;
        struct pmu_hw_events    __percpu *hw_events;
        struct notifier_block   hotplug_nb;
+       struct notifier_block   cpu_pm_nb;
 };
 
 #define to_arm_pmu(p) (container_of(p, struct arm_pmu, pmu))
--

2.5.1

On 23 February 2016 at 11:22, Lorenzo Pieralisi

Quoted 14 lines

drivers/perf/arm_pmu.c +14 -0

+static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
+                            void *v)
+{
+       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
+       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
+
+       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
+               return NOTIFY_DONE;
+
+       /*
+        * Always reset the PMU registers on power-up even if
+        * there are no events running.
+        */

drivers/perf/arm_pmu.c static int cpu_pmu_notify(struct notifier_block *b, unsigned long action, · line 764

Quoted 2 lines

drivers/perf/arm_pmu.c +2 -0

+       if (cmd == CPU_PM_EXIT && armpmu->reset)
+               armpmu->reset(armpmu);

I think this patch does the right thing but I can't get the above reset. Wouldn't it be better to do the reset as part of the CPU_PM_EXIT case below? At this point nothing tells us the CPU won't go back down before the event is enabled, wasting the cycle needed to reset the PMU.

Thanks,
Mathieu

Quoted 82 lines

drivers/perf/arm_pmu.c +43 -0

+
+       if (!enabled)
+               return NOTIFY_OK;
+
+       switch (cmd) {
+       case CPU_PM_ENTER:
+               armpmu->stop(armpmu);
+               cpu_pm_pmu_setup(armpmu, cmd);
+               break;
+       case CPU_PM_EXIT:
+               cpu_pm_pmu_setup(armpmu, cmd);
+       case CPU_PM_ENTER_FAILED:
+               armpmu->start(armpmu);
+               break;
+       default:
+               return NOTIFY_DONE;
+       }
+
+       return NOTIFY_OK;
+}
+
+static int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu)
+{
+       cpu_pmu->cpu_pm_nb.notifier_call = cpu_pm_pmu_notify;
+       return cpu_pm_register_notifier(&cpu_pmu->cpu_pm_nb);
+}
+
+static void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu)
+{
+       cpu_pm_unregister_notifier(&cpu_pmu->cpu_pm_nb);
+}
+#else
+static inline int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu) { return 0; }
+static inline void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu) { }
+#endif
+
 static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
 {
        int err;

@@ -725,6 +813,10 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
        if (err)
                goto out_hw_events;
 
+       err = cpu_pm_pmu_register(cpu_pmu);
+       if (err)
+               goto out_unregister;
+
        for_each_possible_cpu(cpu) {
                struct pmu_hw_events *events = per_cpu_ptr(cpu_hw_events, cpu);
                raw_spin_lock_init(&events->pmu_lock);

@@ -746,6 +838,8 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
 
        return 0;
 
+out_unregister:
+       unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
 out_hw_events:
        free_percpu(cpu_hw_events);
        return err;

@@ -753,6 +847,7 @@ out_hw_events:
 
 static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
 {
+       cpu_pm_pmu_unregister(cpu_pmu);
        unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
        free_percpu(cpu_pmu->hw_events);
 }

include/linux/perf/arm_pmu.h +1 -1

index 83b5e34..9ffa316 100644
@@ -107,6 +107,7 @@ struct arm_pmu {
        struct platform_device  *plat_device;
        struct pmu_hw_events    __percpu *hw_events;
        struct notifier_block   hotplug_nb;
+       struct notifier_block   cpu_pm_nb;
 };
 
 #define to_arm_pmu(p) (container_of(p, struct arm_pmu, pmu))
--

2.5.1

Quoted 1 line · Mathieu Poirier

On 23 February 2016 at 11:22, Lorenzo Pieralisi

[...]

Quoted 14 lines

Quoted 14 lines

drivers/perf/arm_pmu.c +14 -0

+static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
+                            void *v)
+{
+       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
+       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
+
+       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
+               return NOTIFY_DONE;
+
+       /*
+        * Always reset the PMU registers on power-up even if
+        * there are no events running.
+        */

Quoted 8 lines

drivers/perf/arm_pmu.c static int cpu_pmu_notify(struct notifier_block *b, unsigned long action, · line 764

Quoted 2 lines

drivers/perf/arm_pmu.c +2 -0

+       if (cmd == CPU_PM_EXIT && armpmu->reset)
+               armpmu->reset(armpmu);

I think this patch does the right thing but I can't get the above reset. Wouldn't it be better to do the reset as part of the CPU_PM_EXIT case below? At this point nothing tells us the CPU won't go back down before the event is enabled, wasting the cycle needed to reset the PMU.

The logic goes, if the cpu is woken up and it has no events enabled, if we do not reset it (mind, ->reset here sets the PMU register values to a sane default, some of them are architecturally UNKNOWN on reset, it does NOT reset the PMU) _and_ we subsequently install an event on it we do have a problem, that's why whenever a core gets out of low-power we have to reset its pmu.

Hi Lorenzo,

Quoted 28 lines · Lorenzo Pieralisi

Quoted 1 line · Mathieu Poirier

On 23 February 2016 at 11:22, Lorenzo Pieralisi

[...]

Quoted 14 lines

Quoted 14 lines

drivers/perf/arm_pmu.c +14 -0

+static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
+                            void *v)
+{
+       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
+       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
+
+       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
+               return NOTIFY_DONE;
+
+       /*
+        * Always reset the PMU registers on power-up even if
+        * there are no events running.
+        */

Quoted 8 lines

drivers/perf/arm_pmu.c static int cpu_pmu_notify(struct notifier_block *b, unsigned long action, · line 764

Quoted 2 lines

drivers/perf/arm_pmu.c +2 -0

+       if (cmd == CPU_PM_EXIT && armpmu->reset)
+               armpmu->reset(armpmu);

I think this patch does the right thing but I can't get the above reset. Wouldn't it be better to do the reset as part of the CPU_PM_EXIT case below? At this point nothing tells us the CPU won't go back down before the event is enabled, wasting the cycle needed to reset the PMU.

Quoted 6 lines

The logic goes, if the cpu is woken up and it has no events enabled, if we do not reset it (mind, ->reset here sets the PMU register values to a sane default, some of them are architecturally UNKNOWN on reset, it does NOT reset the PMU) _and_ we subsequently install an event on it we do have a problem, that's why whenever a core gets out of low-power we have to reset its pmu.

Dont we blow out the previous value in the counter when installing an event? Or has that changed lately? IIRC, there was some initial value we'd program into the counter to avoid missing the overflow event. (sorry its been too long) ;)

Quoted 39 lines · Ashwin Chaugule

Hi Lorenzo,

Quoted 28 lines · Lorenzo Pieralisi

Quoted 1 line · Mathieu Poirier

On 23 February 2016 at 11:22, Lorenzo Pieralisi

[...]

Quoted 14 lines

Quoted 14 lines

drivers/perf/arm_pmu.c +14 -0

+static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
+                            void *v)
+{
+       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
+       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
+
+       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
+               return NOTIFY_DONE;
+
+       /*
+        * Always reset the PMU registers on power-up even if
+        * there are no events running.
+        */

Quoted 8 lines

drivers/perf/arm_pmu.c static int cpu_pmu_notify(struct notifier_block *b, unsigned long action, · line 764

Quoted 2 lines

drivers/perf/arm_pmu.c +2 -0

+       if (cmd == CPU_PM_EXIT && armpmu->reset)
+               armpmu->reset(armpmu);

I think this patch does the right thing but I can't get the above reset. Wouldn't it be better to do the reset as part of the CPU_PM_EXIT case below? At this point nothing tells us the CPU won't go back down before the event is enabled, wasting the cycle needed to reset the PMU.

Quoted 6 lines

The logic goes, if the cpu is woken up and it has no events enabled, if we do not reset it (mind, ->reset here sets the PMU register values to a sane default, some of them are architecturally UNKNOWN on reset, it does NOT reset the PMU) _and_ we subsequently install an event on it we do have a problem, that's why whenever a core gets out of low-power we have to reset its pmu.

Quoted 4 lines

Dont we blow out the previous value in the counter when installing an event? Or has that changed lately? IIRC, there was some initial value we'd program into the counter to avoid missing the overflow event. (sorry its been too long) ;)

If you mean there is no need of resetting the value since we are overwriting it anyway you should see ->reset from a PMU unit POW not just an event. If you look at the ->reset method for eg v8, you will see that the reset method operates on all counters and the PMU unit as a whole, that's the only sane way of setting up the PMU unit before enabling single counters (some registers are UNKNOWN at reset).

To make my reply to Mathieu clearer, the ->reset method contains operations (eg writing PMCR counters reset bits) that do carry out counters reset, what I wanted to say is that the ->reset method does not by itself drive the PMU HW reset signal, that's what the power controller does when it resets the CPU on power up.

The PMU ->reset method must be called on a cpu on power-up, to make sure PMU HW is set-up to sane default values and ready to be used (ie install counters), for instance on v8 all counters must be disabled (irq inclusive) and reset, that's what armv8pmu_reset() does.

I hope this makes things clearer.

Thanks,
Lorenzo

Quoted 1 line · Mathieu Poirier

On 23 February 2016 at 11:22, Lorenzo Pieralisi

[...]

Quoted 14 lines

Quoted 14 lines

drivers/perf/arm_pmu.c +14 -0

+static int cpu_pm_pmu_notify(struct notifier_block *b, unsigned long cmd,
+                            void *v)
+{
+       struct arm_pmu *armpmu = container_of(b, struct arm_pmu, cpu_pm_nb);
+       struct pmu_hw_events *hw_events = this_cpu_ptr(armpmu->hw_events);
+       int enabled = bitmap_weight(hw_events->used_mask, armpmu->num_events);
+
+       if (!cpumask_test_cpu(smp_processor_id(), &armpmu->supported_cpus))
+               return NOTIFY_DONE;
+
+       /*
+        * Always reset the PMU registers on power-up even if
+        * there are no events running.
+        */

Quoted 8 lines

drivers/perf/arm_pmu.c static int cpu_pmu_notify(struct notifier_block *b, unsigned long action, · line 764

Quoted 2 lines

drivers/perf/arm_pmu.c +2 -0

+       if (cmd == CPU_PM_EXIT && armpmu->reset)
+               armpmu->reset(armpmu);

I think this patch does the right thing but I can't get the above reset. Wouldn't it be better to do the reset as part of the CPU_PM_EXIT case below? At this point nothing tells us the CPU won't go back down before the event is enabled, wasting the cycle needed to reset the PMU.

The logic goes, if the cpu is woken up and it has no events enabled, if we do not reset it (mind, ->reset here sets the PMU register values to a sane default, some of them are architecturally UNKNOWN on reset, it does NOT reset the PMU) _and_ we subsequently install an event on it we do have a problem, that's why whenever a core gets out of low-power we have to reset its pmu.

Does it make sense ?

Thanks,
Lorenzo

Quoted 87 lines

Thanks,
Mathieu

Quoted 82 lines

drivers/perf/arm_pmu.c +43 -0

+
+       if (!enabled)
+               return NOTIFY_OK;
+
+       switch (cmd) {
+       case CPU_PM_ENTER:
+               armpmu->stop(armpmu);
+               cpu_pm_pmu_setup(armpmu, cmd);
+               break;
+       case CPU_PM_EXIT:
+               cpu_pm_pmu_setup(armpmu, cmd);
+       case CPU_PM_ENTER_FAILED:
+               armpmu->start(armpmu);
+               break;
+       default:
+               return NOTIFY_DONE;
+       }
+
+       return NOTIFY_OK;
+}
+
+static int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu)
+{
+       cpu_pmu->cpu_pm_nb.notifier_call = cpu_pm_pmu_notify;
+       return cpu_pm_register_notifier(&cpu_pmu->cpu_pm_nb);
+}
+
+static void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu)
+{
+       cpu_pm_unregister_notifier(&cpu_pmu->cpu_pm_nb);
+}
+#else
+static inline int cpu_pm_pmu_register(struct arm_pmu *cpu_pmu) { return 0; }
+static inline void cpu_pm_pmu_unregister(struct arm_pmu *cpu_pmu) { }
+#endif
+
 static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
 {
        int err;

@@ -725,6 +813,10 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
        if (err)
                goto out_hw_events;
 
+       err = cpu_pm_pmu_register(cpu_pmu);
+       if (err)
+               goto out_unregister;
+
        for_each_possible_cpu(cpu) {
                struct pmu_hw_events *events = per_cpu_ptr(cpu_hw_events, cpu);
                raw_spin_lock_init(&events->pmu_lock);

@@ -746,6 +838,8 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
 
        return 0;
 
+out_unregister:
+       unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
 out_hw_events:
        free_percpu(cpu_hw_events);
        return err;

@@ -753,6 +847,7 @@ out_hw_events:
 
 static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
 {
+       cpu_pm_pmu_unregister(cpu_pmu);
        unregister_cpu_notifier(&cpu_pmu->hotplug_nb);
        free_percpu(cpu_pmu->hw_events);
 }

include/linux/perf/arm_pmu.h +1 -1

index 83b5e34..9ffa316 100644
@@ -107,6 +107,7 @@ struct arm_pmu {
        struct platform_device  *plat_device;
        struct pmu_hw_events    __percpu *hw_events;
        struct notifier_block   hotplug_nb;
+       struct notifier_block   cpu_pm_nb;
 };
 
 #define to_arm_pmu(p) (container_of(p, struct arm_pmu, pmu))
--

2.5.1

When a CPU is suspended (either through suspend-to-RAM or CPUidle), its PMU registers content can be lost, which means that counters registers values that were initialized on power down entry have to be reprogrammed on power-up to make sure the counters set-up is preserved

Quoted 4 lines · Lorenzo Pieralisi

When a CPU is suspended (either through suspend-to-RAM or CPUidle), its PMU registers content can be lost, which means that counters registers values that were initialized on power down entry have to be reprogrammed on power-up to make sure the counters set-up is preserved

This assumes that the PMUs are always sharing a power rail with a specific CPU, not the cluster. Is that a reliable assumption?

Quoted 9 lines · Kevin Hilman

Quoted 4 lines · Lorenzo Pieralisi

When a CPU is suspended (either through suspend-to-RAM or CPUidle), its PMU registers content can be lost, which means that counters registers values that were initialized on power down entry have to be reprogrammed on power-up to make sure the counters set-up is preserved

This assumes that the PMUs are always sharing a power rail with a specific CPU, not the cluster. Is that a reliable assumption?

As reliable as assuming the GIC HW state needs save/restore on idle-state entry, if we have no power domains information linked to CPU PM notifiers saving/restoring everything every time an idle state deeper than wfi is entered is the best we can do.

As far as this patch is concerned, if the PMU is on a different power domain than the CPU, I agree that stopping/resetting/restoring the PMU registers is a waste of cycles (I will test it also by triggering the notifiers on wfi, to make sure the reset is not disruptive on a cpu that is not powered off), I see no alternative other than disabling idle to carry out profiling sanely (or implement CPU PM notifiers with runtime PM).

Thanks,
Lorenzo

Quoted 16 lines · Lorenzo Pieralisi

Quoted 9 lines · Kevin Hilman

Quoted 4 lines · Lorenzo Pieralisi

When a CPU is suspended (either through suspend-to-RAM or CPUidle), its PMU registers content can be lost, which means that counters registers values that were initialized on power down entry have to be reprogrammed on power-up to make sure the counters set-up is preserved

This assumes that the PMUs are always sharing a power rail with a specific CPU, not the cluster. Is that a reliable assumption?

As reliable as assuming the GIC HW state needs save/restore on idle-state entry, if we have no power domains information linked to CPU PM notifiers saving/restoring everything every time an idle state deeper than wfi is entered is the best we can do.

Quoted 7 lines

As far as this patch is concerned, if the PMU is on a different power domain than the CPU, I agree that stopping/resetting/restoring the PMU registers is a waste of cycles (I will test it also by triggering the notifiers on wfi, to make sure the reset is not disruptive on a cpu that is not powered off), I see no alternative other than disabling idle to carry out profiling sanely (or implement CPU PM notifiers with runtime PM).

I totally understand - I'm faced with the same problem on Coresight. To me the ultimate solution is still to integrate CPU power domain management with runtime PM (like we talked about many times before). I know Lina is working on something that will get us close to that but it's not finished yet.

Quoted 26 lines · Mathieu Poirier

Quoted 16 lines · Lorenzo Pieralisi

Quoted 9 lines · Kevin Hilman

Quoted 4 lines · Lorenzo Pieralisi

When a CPU is suspended (either through suspend-to-RAM or CPUidle), its PMU registers content can be lost, which means that counters registers values that were initialized on power down entry have to be reprogrammed on power-up to make sure the counters set-up is preserved

This assumes that the PMUs are always sharing a power rail with a specific CPU, not the cluster. Is that a reliable assumption?

As reliable as assuming the GIC HW state needs save/restore on idle-state entry, if we have no power domains information linked to CPU PM notifiers saving/restoring everything every time an idle state deeper than wfi is entered is the best we can do.

Quoted 7 lines

As far as this patch is concerned, if the PMU is on a different power domain than the CPU, I agree that stopping/resetting/restoring the PMU registers is a waste of cycles (I will test it also by triggering the notifiers on wfi, to make sure the reset is not disruptive on a cpu that is not powered off), I see no alternative other than disabling idle to carry out profiling sanely (or implement CPU PM notifiers with runtime PM).

Quoted 5 lines

I totally understand - I'm faced with the same problem on Coresight. To me the ultimate solution is still to integrate CPU power domain management with runtime PM (like we talked about many times before). I know Lina is working on something that will get us close to that but it's not finished yet.

I've been waiting for that code for *years* now...

Ultimately, Juno-r2 loses its PMU state over idle if you run mainline on it and, worse still, you end up getting junk numbers back to userspace, so it's not even obvious what happened.

I'm not aware of any flags that indicate loss of state.

Quoted 26 lines · Mathieu Poirier

Quoted 23 lines · Lorenzo Pieralisi

Quoted 9 lines · Kevin Hilman

Quoted 4 lines · Lorenzo Pieralisi

When a CPU is suspended (either through suspend-to-RAM or CPUidle), its PMU registers content can be lost, which means that counters registers values that were initialized on power down entry have to be reprogrammed on power-up to make sure the counters set-up is preserved

This assumes that the PMUs are always sharing a power rail with a specific CPU, not the cluster. Is that a reliable assumption?

As reliable as assuming the GIC HW state needs save/restore on idle-state entry, if we have no power domains information linked to CPU PM notifiers saving/restoring everything every time an idle state deeper than wfi is entered is the best we can do.

As far as this patch is concerned, if the PMU is on a different power domain than the CPU, I agree that stopping/resetting/restoring the PMU registers is a waste of cycles (I will test it also by triggering the notifiers on wfi, to make sure the reset is not disruptive on a cpu that is not powered off), I see no alternative other than disabling idle to carry out profiling sanely (or implement CPU PM notifiers with runtime PM).

Quoted 5 lines

I totally understand - I'm faced with the same problem on Coresight. To me the ultimate solution is still to integrate CPU power domain management with runtime PM (like we talked about many times before). I know Lina is working on something that will get us close to that but it's not finished yet.

I've been waiting for that code for *years* now...