Better domain idle from device wakeup patterns

Currently, a PM domain's idle state is determined based on whether the QoS requirements are met. This may not save power, if the idle state residency requirements are not met.

CPU PM domains use the next timer wakeup for the CPUs in the domain to determine the sleep duration of the domain. This is compared with the idle state residencies to determine the optimal idle state. For other PM domains, determining the sleep length is not that straight forward. But if the device's next_event is available, we can use that to determine the sleep duration of the PM domain.

Let's update the domain governor logic to check for idle state residency based on the next wakeup of devices as well as QoS constraints.

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v5:
- Minor code changes suggested by Rafel
Changes in v4:
- Update to use next_wakeup from struct generic_pm_domain_data.
Changes in v3:
- None
Changes in v2:
- Fix state_idx type to hold negative value.
- Update commit text.

---
 drivers/base/power/domain_governor.c | 81 ++++++++++++++++++++++++++--
 include/linux/pm_domain.h            |  1 +
 2 files changed, 77 insertions(+), 5 deletions(-)

drivers/base/power/domain_governor.c +3 -0

index 490ed7deb99a..fee050c6ea6a 100644
@@ -117,6 +117,47 @@ static bool default_suspend_ok(struct device *dev)
        return td->cached_suspend_ok;
 }
 
+static void update_domain_next_wakeup(struct generic_pm_domain *genpd, ktime_t now)
+{
+       ktime_t domain_wakeup = KTIME_MAX;

drivers/base/power/domain_governor.c +13 -0

+       }
+
+       /* Then find the earliest wakeup of from all the child domains */
+       list_for_each_entry(link, &genpd->parent_links, parent_node) {
+               next_wakeup = link->child->next_wakeup;
+               if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+                       if (ktime_before(next_wakeup, domain_wakeup))
+                               domain_wakeup = next_wakeup;
+       }
+
+       genpd->next_wakeup = domain_wakeup;
+}
+

drivers/base/power/domain_governor.c +21 -0

+       idle_time_ns = ktime_to_ns(ktime_sub(domain_wakeup, now));
+
+       return idle_time_ns >= min_sleep_ns;
+}
+
 static bool __default_power_down_ok(struct dev_pm_domain *pd,
                                     unsigned int state)
 {

@@ -209,8 +250,34 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd,
 static bool default_power_down_ok(struct dev_pm_domain *pd)
 {
        struct generic_pm_domain *genpd = pd_to_genpd(pd);
+       int state_idx = genpd->state_count - 1;
+       ktime_t now = ktime_get();
        struct gpd_link *link;
 
+       /*
+        * Find the next wakeup from devices that can determine their own wakeup
+        * to find when the domain would wakeup and do it for every device down
+        * the hierarchy. It is not worth while to sleep if the state's residency
+        * cannot be met.
+        */
+       update_domain_next_wakeup(genpd, now);
+       if (genpd->next_wakeup != KTIME_MAX) {
+               /* Let's find out the deepest domain idle state, the devices prefer */
+               while (state_idx >= 0) {
+                       if (next_wakeup_allows_state(genpd, state_idx, now)) {
+                               genpd->max_off_time_changed = true;
+                               break;
+                       }

drivers/base/power/domain_governor.c +6 -4

        if (!genpd->max_off_time_changed) {
                genpd->state_idx = genpd->cached_power_down_state_idx;
                return genpd->cached_power_down_ok;

@@ -228,17 +295,21 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
        genpd->max_off_time_ns = -1;
        genpd->max_off_time_changed = false;
        genpd->cached_power_down_ok = true;
-       genpd->state_idx = genpd->state_count - 1;
 
-       /* Find a state to power down to, starting from the deepest. */
-       while (!__default_power_down_ok(pd, genpd->state_idx)) {
-               if (genpd->state_idx == 0) {
+       /*
+        * Find a state to power down to, starting from the state
+        * determined by the next wakeup.
+        */
+       while (!__default_power_down_ok(pd, state_idx)) {
+               if (state_idx == 0) {
                        genpd->cached_power_down_ok = false;
                        break;

include/linux/pm_domain.h +1 -1

index d7875bfde7f4..49982cd58bfd 100644
@@ -130,6 +130,7 @@ struct generic_pm_domain {
                                     unsigned int state);
        struct gpd_dev_ops dev_ops;
        s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
+       ktime_t next_wakeup;    /* Maintained by the domain governor */
        bool max_off_time_changed;
        bool cached_power_down_ok;
        bool cached_power_down_state_idx;
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Quoted 1 line · Lina Iyer

drivers/base/power/domain_governor.c static bool default_suspend_ok(struct device *dev) · line 132

Quoted 13 lines

drivers/base/power/domain_governor.c +13 -0

+static void update_domain_next_wakeup(struct generic_pm_domain *genpd, ktime_t now)
+{
+       ktime_t domain_wakeup = KTIME_MAX;
+       ktime_t next_wakeup;
+       struct pm_domain_data *pdd;
+       struct gpd_link *link;
+
+       /* Find the earliest wakeup for all devices in the domain */
+       list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+               next_wakeup = to_gpd_data(pdd)->next_wakeup;
+               if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+                       if (ktime_before(next_wakeup, domain_wakeup))
+                               domain_wakeup = next_wakeup;

If it turns out that one of the device's next_wakeup is before "now", leading to ktime_before() above returns true - then I think you should bail out, no?

At least, we shouldn't just continue and ignore this case, right?

Quoted 13 lines

drivers/base/power/domain_governor.c +13 -0

+       }
+
+       /* Then find the earliest wakeup of from all the child domains */
+       list_for_each_entry(link, &genpd->parent_links, parent_node) {
+               next_wakeup = link->child->next_wakeup;
+               if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+                       if (ktime_before(next_wakeup, domain_wakeup))
+                               domain_wakeup = next_wakeup;
+       }
+
+       genpd->next_wakeup = domain_wakeup;
+}
+

drivers/base/power/domain_governor.c static bool default_suspend_ok(struct device *dev) · line 155

Quoted 10 lines

drivers/base/power/domain_governor.c +10 -0

+static bool next_wakeup_allows_state(struct generic_pm_domain *genpd,
+                                    unsigned int state, ktime_t now)
+{
+       ktime_t domain_wakeup = genpd->next_wakeup;
+       s64 idle_time_ns, min_sleep_ns;
+
+       min_sleep_ns = genpd->states[state].power_off_latency_ns +
+                      genpd->states[state].power_on_latency_ns +
+                      genpd->states[state].residency_ns;
+

I don't think you should include the power_on_latency_ns here.

The validation isn't about QoS constraints, but whether we can meet the residency time to make it worth entering the state, from an energy point of view. Right?

Quoted 30 lines

drivers/base/power/domain_governor.c +21 -0

+       idle_time_ns = ktime_to_ns(ktime_sub(domain_wakeup, now));
+
+       return idle_time_ns >= min_sleep_ns;
+}
+
 static bool __default_power_down_ok(struct dev_pm_domain *pd,
                                     unsigned int state)
 {

@@ -209,8 +250,34 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd,
 static bool default_power_down_ok(struct dev_pm_domain *pd)
 {
        struct generic_pm_domain *genpd = pd_to_genpd(pd);
+       int state_idx = genpd->state_count - 1;
+       ktime_t now = ktime_get();
        struct gpd_link *link;
 
+       /*
+        * Find the next wakeup from devices that can determine their own wakeup
+        * to find when the domain would wakeup and do it for every device down
+        * the hierarchy. It is not worth while to sleep if the state's residency
+        * cannot be met.
+        */
+       update_domain_next_wakeup(genpd, now);
+       if (genpd->next_wakeup != KTIME_MAX) {
+               /* Let's find out the deepest domain idle state, the devices prefer */
+               while (state_idx >= 0) {
+                       if (next_wakeup_allows_state(genpd, state_idx, now)) {
+                               genpd->max_off_time_changed = true;
+                               break;
+                       }

drivers/base/power/domain_governor.c static bool __default_power_down_ok(struct dev_pm_domain *pd, · line 280

Quoted 10 lines

drivers/base/power/domain_governor.c +10 -0

+                       state_idx--;
+               }
+
+               if (state_idx < 0) {
+                       state_idx = 0;
+                       genpd->cached_power_down_ok = false;
+                       goto done;
+               }
+       }
+

The above would introduce unnecessary overhead, as it may become executed in cases when it's not needed.

For example, there's no point doing the above, if the domain doesn't specify residency values for its idle states.

Additionally, we don't need to recompute the domain's next wakup, unless a device has got a new next_wakeup value set for it. In this case, we can just select a state based upon an previously computed value, thus avoiding the recomputation.

Quoted 20 lines

drivers/base/power/domain_governor.c +6 -4

        if (!genpd->max_off_time_changed) {
                genpd->state_idx = genpd->cached_power_down_state_idx;
                return genpd->cached_power_down_ok;

@@ -228,17 +295,21 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
        genpd->max_off_time_ns = -1;
        genpd->max_off_time_changed = false;
        genpd->cached_power_down_ok = true;
-       genpd->state_idx = genpd->state_count - 1;
 
-       /* Find a state to power down to, starting from the deepest. */
-       while (!__default_power_down_ok(pd, genpd->state_idx)) {
-               if (genpd->state_idx == 0) {
+       /*
+        * Find a state to power down to, starting from the state
+        * determined by the next wakeup.
+        */
+       while (!__default_power_down_ok(pd, state_idx)) {
+               if (state_idx == 0) {
                        genpd->cached_power_down_ok = false;
                        break;

drivers/base/power/domain_governor.c static bool default_power_down_ok(struct dev_pm_domain *pd) · line 315

Quoted 10 lines

drivers/base/power/domain_governor.c +3 -1

                }
-               genpd->state_idx--;
+               state_idx--;
        }
 
+done:
+       genpd->state_idx = state_idx;
        genpd->cached_power_down_state_idx = genpd->state_idx;
        return genpd->cached_power_down_ok;
 }

Another thing to consider for the above changes, is that the cpu_power_down_ok() calls into default_power_down_ok().

Even if I am fine with the approach taken in $subject patch, I think it's important to try to keep the path a slim as possible as it's also executed in the CPU idlepath.

For example, $subject change means also
that we end up calling ktime_get() twice in the same path, introducing unnecessary overhead. We can do better and avoid that by restructuring the code a bit, don't you think?

Quoted 2 lines · Ulf Hansson

Quoted 1 line · Lina Iyer

[...]

Quoted 18 lines

drivers/base/power/domain_governor.c static bool default_suspend_ok(struct device *dev) · line 132

Quoted 13 lines

drivers/base/power/domain_governor.c +13 -0

+static void update_domain_next_wakeup(struct generic_pm_domain *genpd, ktime_t now)
+{
+       ktime_t domain_wakeup = KTIME_MAX;
+       ktime_t next_wakeup;
+       struct pm_domain_data *pdd;
+       struct gpd_link *link;
+
+       /* Find the earliest wakeup for all devices in the domain */
+       list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+               next_wakeup = to_gpd_data(pdd)->next_wakeup;
+               if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+                       if (ktime_before(next_wakeup, domain_wakeup))
+                               domain_wakeup = next_wakeup;

If it turns out that one of the device's next_wakeup is before "now", leading to ktime_before() above returns true - then I think you should bail out, no?

Quoted 2 lines

At least, we shouldn't just continue and ignore this case, right?

No, that could be a very common case. Drivers are not expected to clean up the next wakeup by setting it to KTIME_MAX. The best we can do is to make a choice with the valid information we have. This will also map to the current behavior. Say if all next wakeup information provided to the devices were in the past, we would be no worse (or better) than what we do without this change.

Quoted 27 lines

Quoted 14 lines

drivers/base/power/domain_governor.c +13 -0

+       }
+
+       /* Then find the earliest wakeup of from all the child domains */
+       list_for_each_entry(link, &genpd->parent_links, parent_node) {
+               next_wakeup = link->child->next_wakeup;
+               if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+                       if (ktime_before(next_wakeup, domain_wakeup))
+                               domain_wakeup = next_wakeup;
+       }
+
+       genpd->next_wakeup = domain_wakeup;
+}
+

drivers/base/power/domain_governor.c static bool default_suspend_ok(struct device *dev) · line 155

Quoted 10 lines

drivers/base/power/domain_governor.c +10 -0

+static bool next_wakeup_allows_state(struct generic_pm_domain *genpd,
+                                    unsigned int state, ktime_t now)
+{
+       ktime_t domain_wakeup = genpd->next_wakeup;
+       s64 idle_time_ns, min_sleep_ns;
+
+       min_sleep_ns = genpd->states[state].power_off_latency_ns +
+                      genpd->states[state].power_on_latency_ns +
+                      genpd->states[state].residency_ns;
+

I don't think you should include the power_on_latency_ns here.

Quoted 4 lines

The validation isn't about QoS constraints, but whether we can meet the residency time to make it worth entering the state, from an energy point of view. Right?

Fair point. I will remove the power_on_latency_ns.

Quoted 44 lines

Quoted 30 lines

drivers/base/power/domain_governor.c +21 -0

+       idle_time_ns = ktime_to_ns(ktime_sub(domain_wakeup, now));
+
+       return idle_time_ns >= min_sleep_ns;
+}
+
 static bool __default_power_down_ok(struct dev_pm_domain *pd,
                                     unsigned int state)
 {

@@ -209,8 +250,34 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd,
 static bool default_power_down_ok(struct dev_pm_domain *pd)
 {
        struct generic_pm_domain *genpd = pd_to_genpd(pd);
+       int state_idx = genpd->state_count - 1;
+       ktime_t now = ktime_get();
        struct gpd_link *link;
 
+       /*
+        * Find the next wakeup from devices that can determine their own wakeup
+        * to find when the domain would wakeup and do it for every device down
+        * the hierarchy. It is not worth while to sleep if the state's residency
+        * cannot be met.
+        */
+       update_domain_next_wakeup(genpd, now);
+       if (genpd->next_wakeup != KTIME_MAX) {
+               /* Let's find out the deepest domain idle state, the devices prefer */
+               while (state_idx >= 0) {
+                       if (next_wakeup_allows_state(genpd, state_idx, now)) {
+                               genpd->max_off_time_changed = true;
+                               break;
+                       }

drivers/base/power/domain_governor.c static bool __default_power_down_ok(struct dev_pm_domain *pd, · line 280

Quoted 10 lines

drivers/base/power/domain_governor.c +10 -0

+                       state_idx--;
+               }
+
+               if (state_idx < 0) {
+                       state_idx = 0;
+                       genpd->cached_power_down_ok = false;
+                       goto done;
+               }
+       }
+

The above would introduce unnecessary overhead, as it may become executed in cases when it's not needed.

Quoted 3 lines

For example, there's no point doing the above, if the domain doesn't specify residency values for its idle states.

We would still need to ensure that the next wakeup is after the power_off_latency, if specified.

Quoted 6 lines

Additionally, we don't need to recompute the domain's next wakup, unless a device has got a new next_wakeup value set for it. In this case, we can just select a state based upon an previously computed value, thus avoiding the recomputation.

If the domain's next wakeup was in the past, then using our previously computed state may be incorrect.

Quoted 35 lines

Quoted 21 lines

drivers/base/power/domain_governor.c +6 -4

        if (!genpd->max_off_time_changed) {
                genpd->state_idx = genpd->cached_power_down_state_idx;
                return genpd->cached_power_down_ok;

@@ -228,17 +295,21 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
        genpd->max_off_time_ns = -1;
        genpd->max_off_time_changed = false;
        genpd->cached_power_down_ok = true;
-       genpd->state_idx = genpd->state_count - 1;
 
-       /* Find a state to power down to, starting from the deepest. */
-       while (!__default_power_down_ok(pd, genpd->state_idx)) {
-               if (genpd->state_idx == 0) {
+       /*
+        * Find a state to power down to, starting from the state
+        * determined by the next wakeup.
+        */
+       while (!__default_power_down_ok(pd, state_idx)) {
+               if (state_idx == 0) {
                        genpd->cached_power_down_ok = false;
                        break;

drivers/base/power/domain_governor.c static bool default_power_down_ok(struct dev_pm_domain *pd) · line 315

Quoted 10 lines

drivers/base/power/domain_governor.c +3 -1

                }
-               genpd->state_idx--;
+               state_idx--;
        }
 
+done:
+       genpd->state_idx = state_idx;
        genpd->cached_power_down_state_idx = genpd->state_idx;
        return genpd->cached_power_down_ok;
 }

Another thing to consider for the above changes, is that the cpu_power_down_ok() calls into default_power_down_ok().

Quoted 3 lines

Even if I am fine with the approach taken in $subject patch, I think it's important to try to keep the path a slim as possible as it's also executed in the CPU idlepath.

Wouldn't this be called only the last CPU is powering down and only if the domain is ready to power down?

Quoted 5 lines

For example, $subject change means also
that we end up calling ktime_get() twice in the same path, introducing unnecessary overhead. We can do better and avoid that by restructuring the code a bit, don't you think?

Hmmm, we could. I will submit a follow on patch to reorganize the code so the ktime_get() would be called only once for either of the power_down_ok callbacks.

Quoted 23 lines · Lina Iyer

Quoted 2 lines · Ulf Hansson

Quoted 1 line · Lina Iyer

[...]

Quoted 18 lines

drivers/base/power/domain_governor.c static bool default_suspend_ok(struct device *dev) · line 132

Quoted 13 lines

drivers/base/power/domain_governor.c +13 -0

+static void update_domain_next_wakeup(struct generic_pm_domain *genpd, ktime_t now)
+{
+       ktime_t domain_wakeup = KTIME_MAX;
+       ktime_t next_wakeup;
+       struct pm_domain_data *pdd;
+       struct gpd_link *link;
+
+       /* Find the earliest wakeup for all devices in the domain */
+       list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+               next_wakeup = to_gpd_data(pdd)->next_wakeup;
+               if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+                       if (ktime_before(next_wakeup, domain_wakeup))
+                               domain_wakeup = next_wakeup;

If it turns out that one of the device's next_wakeup is before "now", leading to ktime_before() above returns true - then I think you should bail out, no?

Quoted 8 lines

Quoted 2 lines

At least, we shouldn't just continue and ignore this case, right?

No, that could be a very common case. Drivers are not expected to clean up the next wakeup by setting it to KTIME_MAX. The best we can do is to make a choice with the valid information we have. This will also map to the current behavior. Say if all next wakeup information provided to the devices were in the past, we would be no worse (or better) than what we do without this change.

Well, I don't quite agree (at least not yet), but let me elaborate, as I think we can do better without having to add complexity.

Normally, I don't think a driver needs to clean up its device's next wakeup in between the remote wakeups, instead it should just set a new value.

That's because, even if the driver acts to a remote wakeup or deals with a request entering a queue, the driver needs to runtime resume its device during this period. This prevents genpd from power off the PM domain, hence also the genpd governor from potentially looking at "invalid" wakeup information for its attached devices.

Of course, I assume there are situations, where a driver actually needs to do a clean up of its device's next wakeup, but that should be less frequent and likely when a remote wakeup is disabled (for whatever reason).

Quoted 28 lines

Quoted 28 lines

Quoted 14 lines

drivers/base/power/domain_governor.c +14 -0

+       /*
+        * Find the next wakeup from devices that can determine their own wakeup
+        * to find when the domain would wakeup and do it for every device down
+        * the hierarchy. It is not worth while to sleep if the state's residency
+        * cannot be met.
+        */
+       update_domain_next_wakeup(genpd, now);
+       if (genpd->next_wakeup != KTIME_MAX) {
+               /* Let's find out the deepest domain idle state, the devices prefer */
+               while (state_idx >= 0) {
+                       if (next_wakeup_allows_state(genpd, state_idx, now)) {
+                               genpd->max_off_time_changed = true;
+                               break;
+                       }

drivers/base/power/domain_governor.c static bool __default_power_down_ok(struct dev_pm_domain *pd, · line 280

Quoted 10 lines

drivers/base/power/domain_governor.c +10 -0

+                       state_idx--;
+               }
+
+               if (state_idx < 0) {
+                       state_idx = 0;
+                       genpd->cached_power_down_ok = false;
+                       goto done;
+               }
+       }
+

The above would introduce unnecessary overhead, as it may become executed in cases when it's not needed.

Quoted 5 lines

Quoted 3 lines

For example, there's no point doing the above, if the domain doesn't specify residency values for its idle states.

We would still need to ensure that the next wakeup is after the power_off_latency, if specified.

Good point! Although, I would rather avoid adding the overhead, unless the residency is specified. Do you see a problem with this approach?

Another option is to add a new governor, but if possible, I would like to avoid that.

Quoted 6 lines

Quoted 6 lines

Additionally, we don't need to recompute the domain's next wakup, unless a device has got a new next_wakeup value set for it. In this case, we can just select a state based upon an previously computed value, thus avoiding the recomputation.

Quoted 2 lines

If the domain's next wakeup was in the past, then using our previously computed state may be incorrect.

I am not saying you should use the previously computed *idlestate*, but the previously computed next wakeup.

Quoted 46 lines

Quoted 35 lines

Quoted 21 lines

        if (!genpd->max_off_time_changed) {
                genpd->state_idx = genpd->cached_power_down_state_idx;
                return genpd->cached_power_down_ok;

drivers/base/power/domain_governor.c +6 -4

@@ -228,17 +295,21 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
        genpd->max_off_time_ns = -1;
        genpd->max_off_time_changed = false;
        genpd->cached_power_down_ok = true;
-       genpd->state_idx = genpd->state_count - 1;
 
-       /* Find a state to power down to, starting from the deepest. */
-       while (!__default_power_down_ok(pd, genpd->state_idx)) {
-               if (genpd->state_idx == 0) {
+       /*
+        * Find a state to power down to, starting from the state
+        * determined by the next wakeup.
+        */
+       while (!__default_power_down_ok(pd, state_idx)) {
+               if (state_idx == 0) {
                        genpd->cached_power_down_ok = false;
                        break;

drivers/base/power/domain_governor.c static bool default_power_down_ok(struct dev_pm_domain *pd) · line 315

Quoted 10 lines

drivers/base/power/domain_governor.c +3 -1

                }
-               genpd->state_idx--;
+               state_idx--;
        }
 
+done:
+       genpd->state_idx = state_idx;
        genpd->cached_power_down_state_idx = genpd->state_idx;
        return genpd->cached_power_down_ok;
 }

Another thing to consider for the above changes, is that the cpu_power_down_ok() calls into default_power_down_ok().

Quoted 3 lines

Even if I am fine with the approach taken in $subject patch, I think it's important to try to keep the path a slim as possible as it's also executed in the CPU idlepath.

Wouldn't this be called only the last CPU is powering down and only if the domain is ready to power down?

Quoted 5 lines

For example, $subject change means also
that we end up calling ktime_get() twice in the same path, introducing unnecessary overhead. We can do better and avoid that by restructuring the code a bit, don't you think?

Quoted 3 lines

Hmmm, we could. I will submit a follow on patch to reorganize the code so the ktime_get() would be called only once for either of the power_down_ok callbacks.

If possible, I would rather make it part of the series. Just fold in some "rework" patch before extending the governor, that should be possible I think.

Quoted 40 lines · Ulf Hansson

Quoted 23 lines · Lina Iyer

Quoted 2 lines · Ulf Hansson

Quoted 1 line · Lina Iyer

[...]

Quoted 18 lines

drivers/base/power/domain_governor.c static bool default_suspend_ok(struct device *dev) · line 132

Quoted 13 lines

drivers/base/power/domain_governor.c +13 -0

+static void update_domain_next_wakeup(struct generic_pm_domain *genpd, ktime_t now)
+{
+       ktime_t domain_wakeup = KTIME_MAX;
+       ktime_t next_wakeup;
+       struct pm_domain_data *pdd;
+       struct gpd_link *link;
+
+       /* Find the earliest wakeup for all devices in the domain */
+       list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+               next_wakeup = to_gpd_data(pdd)->next_wakeup;
+               if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+                       if (ktime_before(next_wakeup, domain_wakeup))
+                               domain_wakeup = next_wakeup;

If it turns out that one of the device's next_wakeup is before "now", leading to ktime_before() above returns true - then I think you should bail out, no?

Quoted 8 lines

Quoted 2 lines

At least, we shouldn't just continue and ignore this case, right?

No, that could be a very common case. Drivers are not expected to clean up the next wakeup by setting it to KTIME_MAX. The best we can do is to make a choice with the valid information we have. This will also map to the current behavior. Say if all next wakeup information provided to the devices were in the past, we would be no worse (or better) than what we do without this change.

Well, I don't quite agree (at least not yet), but let me elaborate, as I think we can do better without having to add complexity.

Normally, I don't think a driver needs to clean up its device's next wakeup in between the remote wakeups, instead it should just set a new value.

Quoted 6 lines

That's because, even if the driver acts to a remote wakeup or deals with a request entering a queue, the driver needs to runtime resume its device during this period. This prevents genpd from power off the PM domain, hence also the genpd governor from potentially looking at "invalid" wakeup information for its attached devices.

Could you elaborate a bit? Why would a remote wakeup affect the next wakeup. I'm afraid that I'm not getting the situation correctly.

Quoted 6 lines

Of course, I assume there are situations, where a driver actually needs to do a clean up of its device's next wakeup, but that should be less frequent and likely when a remote wakeup is disabled (for whatever reason).

A common case would be that the driver does not know when the usecase is being turned off and therefore may not be able to set the next wakeup to max. If the stale value continues to exist then we may never power off the domain.

Quoted 35 lines

Quoted 34 lines

Quoted 29 lines

Quoted 15 lines

drivers/base/power/domain_governor.c +14 -0

+       /*
+        * Find the next wakeup from devices that can determine their own wakeup
+        * to find when the domain would wakeup and do it for every device down
+        * the hierarchy. It is not worth while to sleep if the state's residency
+        * cannot be met.
+        */
+       update_domain_next_wakeup(genpd, now);
+       if (genpd->next_wakeup != KTIME_MAX) {
+               /* Let's find out the deepest domain idle state, the devices prefer */
+               while (state_idx >= 0) {
+                       if (next_wakeup_allows_state(genpd, state_idx, now)) {
+                               genpd->max_off_time_changed = true;
+                               break;
+                       }

drivers/base/power/domain_governor.c static bool __default_power_down_ok(struct dev_pm_domain *pd, · line 280

Quoted 10 lines

drivers/base/power/domain_governor.c +10 -0

+                       state_idx--;
+               }
+
+               if (state_idx < 0) {
+                       state_idx = 0;
+                       genpd->cached_power_down_ok = false;
+                       goto done;
+               }
+       }
+

The above would introduce unnecessary overhead, as it may become executed in cases when it's not needed.

Quoted 3 lines

For example, there's no point doing the above, if the domain doesn't specify residency values for its idle states.

We would still need to ensure that the next wakeup is after the power_off_latency, if specified.

Quoted 3 lines

Good point! Although, I would rather avoid adding the overhead, unless the residency is specified. Do you see a problem with this approach?

Hmmm, no strong objections. However, we still need to run through the states to make sure the residency is not set and have a variable track that.

The devices wouldn't know that and would still continue to set the next wakeup, unless we find a way to let them know we are not using this feature for the domain.

Quoted 4 lines

Another option is to add a new governor, but if possible, I would like to avoid that.

Absolutely, we should avoid that.

Quoted 42 lines · Lina Iyer

Quoted 40 lines · Ulf Hansson

Quoted 23 lines · Lina Iyer

Quoted 2 lines · Ulf Hansson

Quoted 1 line · Lina Iyer

[...]

Quoted 18 lines

drivers/base/power/domain_governor.c static bool default_suspend_ok(struct device *dev) · line 132

Quoted 13 lines

drivers/base/power/domain_governor.c +13 -0

+static void update_domain_next_wakeup(struct generic_pm_domain *genpd, ktime_t now)
+{
+       ktime_t domain_wakeup = KTIME_MAX;
+       ktime_t next_wakeup;
+       struct pm_domain_data *pdd;
+       struct gpd_link *link;
+
+       /* Find the earliest wakeup for all devices in the domain */
+       list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+               next_wakeup = to_gpd_data(pdd)->next_wakeup;
+               if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+                       if (ktime_before(next_wakeup, domain_wakeup))
+                               domain_wakeup = next_wakeup;

If it turns out that one of the device's next_wakeup is before "now", leading to ktime_before() above returns true - then I think you should bail out, no?

Quoted 8 lines

Quoted 2 lines

At least, we shouldn't just continue and ignore this case, right?

No, that could be a very common case. Drivers are not expected to clean up the next wakeup by setting it to KTIME_MAX. The best we can do is to make a choice with the valid information we have. This will also map to the current behavior. Say if all next wakeup information provided to the devices were in the past, we would be no worse (or better) than what we do without this change.

Well, I don't quite agree (at least not yet), but let me elaborate, as I think we can do better without having to add complexity.

Normally, I don't think a driver needs to clean up its device's next wakeup in between the remote wakeups, instead it should just set a new value.

Quoted 8 lines

Quoted 6 lines

That's because, even if the driver acts to a remote wakeup or deals with a request entering a queue, the driver needs to runtime resume its device during this period. This prevents genpd from power off the PM domain, hence also the genpd governor from potentially looking at "invalid" wakeup information for its attached devices.

Could you elaborate a bit? Why would a remote wakeup affect the next wakeup. I'm afraid that I'm not getting the situation correctly.

Let me try :-)

A remote wakeup is a wakeup irq that is triggered when the device is in runtime suspended state.

I was expecting that you would be arming a remote wakeup for the corresponding device that is attached to a genpd, when the use case becomes enabled. Additionally, to allow the driver to consume the wakeup irq, it needs to runtime resume its device (which means its PM domain via genpd must be powered on as well, if it's not on already).

Therefore, during the period of when the driver consumes the wakeup irq, its device's PM domain remains powered on. When this is completed, the driver allows its device to become runtime suspended again. At some point before the device becomes runtime suspended, the driver should set a new value of the "next wakeup" for its device.

Quoted 10 lines

Quoted 6 lines

Of course, I assume there are situations, where a driver actually needs to do a clean up of its device's next wakeup, but that should be less frequent and likely when a remote wakeup is disabled (for whatever reason).

A common case would be that the driver does not know when the usecase is being turned off and therefore may not be able to set the next wakeup to max. If the stale value continues to exist then we may never power off the domain.

Right.

But, how do you know that the use case starts and what prevents us from knowing that the use case has stopped?

Maybe if you add a user of the new APIs, this would help me to understand better?

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

For example, there's no point doing the above, if the domain doesn't specify residency values for its idle states.

We would still need to ensure that the next wakeup is after the power_off_latency, if specified.

Quoted 6 lines

Quoted 3 lines

Good point! Although, I would rather avoid adding the overhead, unless the residency is specified. Do you see a problem with this approach?

Hmmm, no strong objections. However, we still need to run through the states to make sure the residency is not set and have a variable track that.

Right.

The important part is that we can do that once and not for every call to the governor.

Quoted 3 lines

The devices wouldn't know that and would still continue to set the next wakeup, unless we find a way to let them know we are not using this feature for the domain.

Right.

To allow the driver to know, we could respond with an error code from the new dev_pm_genpd_set_performance_state() API (from patch1), in case the genpd+governor doesn't support it.

Would that be okay? Otherwise we will have to add a separate genpd API, asking explicitly if the "next wakeup" feature is supported.

Quoted 63 lines · Ulf Hansson

Quoted 42 lines · Lina Iyer

Quoted 40 lines · Ulf Hansson

Quoted 23 lines · Lina Iyer

Quoted 2 lines · Ulf Hansson

Quoted 1 line · Lina Iyer

[...]

Quoted 18 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+static void update_domain_next_wakeup(struct generic_pm_domain *genpd, ktime_t now)
+{
+       ktime_t domain_wakeup = KTIME_MAX;
+       ktime_t next_wakeup;
+       struct pm_domain_data *pdd;
+       struct gpd_link *link;
+
+       /* Find the earliest wakeup for all devices in the domain */
+       list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+               next_wakeup = to_gpd_data(pdd)->next_wakeup;
+               if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+                       if (ktime_before(next_wakeup, domain_wakeup))
+                               domain_wakeup = next_wakeup;

If it turns out that one of the device's next_wakeup is before "now", leading to ktime_before() above returns true - then I think you should bail out, no?

Quoted 8 lines

Quoted 2 lines

At least, we shouldn't just continue and ignore this case, right?

No, that could be a very common case. Drivers are not expected to clean up the next wakeup by setting it to KTIME_MAX. The best we can do is to make a choice with the valid information we have. This will also map to the current behavior. Say if all next wakeup information provided to the devices were in the past, we would be no worse (or better) than what we do without this change.

Well, I don't quite agree (at least not yet), but let me elaborate, as I think we can do better without having to add complexity.

Normally, I don't think a driver needs to clean up its device's next wakeup in between the remote wakeups, instead it should just set a new value.

Quoted 8 lines

Quoted 6 lines

That's because, even if the driver acts to a remote wakeup or deals with a request entering a queue, the driver needs to runtime resume its device during this period. This prevents genpd from power off the PM domain, hence also the genpd governor from potentially looking at "invalid" wakeup information for its attached devices.

Could you elaborate a bit? Why would a remote wakeup affect the next wakeup. I'm afraid that I'm not getting the situation correctly.

Let me try :-)

A remote wakeup is a wakeup irq that is triggered when the device is in runtime suspended state.

I was expecting that you would be arming a remote wakeup for the corresponding device that is attached to a genpd, when the use case becomes enabled. Additionally, to allow the driver to consume the wakeup irq, it needs to runtime resume its device (which means its PM domain via genpd must be powered on as well, if it's not on already).

Quoted 6 lines

Therefore, during the period of when the driver consumes the wakeup irq, its device's PM domain remains powered on. When this is completed, the driver allows its device to become runtime suspended again. At some point before the device becomes runtime suspended, the driver should set a new value of the "next wakeup" for its device.

Okay, that is clear. Thanks :)

Yes, we would expect the device to set up its next_wakeup before doing runtime suspend and if the next wakeup is in the past, we would possibly not have runtime suspended the device. That would keep the domain ON and we would not come to this governor at all. And if we still are doing it, then the device has not set the next wakeup correctly.

What you are suggesting is that, we should not power down the domain in such a case. This would be a really hard problem to debug when a device leaves a stale wakeup behind and we no longer power off the domain because of that. Tracking that back to the device will be a monumental effort. Ignoring the next wakeup though might involve a power/perf penalty (no worse than today), but we would not have a difficult problem to solve.

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Of course, I assume there are situations, where a driver actually needs to do a clean up of its device's next wakeup, but that should be less frequent and likely when a remote wakeup is disabled (for whatever reason).

A common case would be that the driver does not know when the usecase is being turned off and therefore may not be able to set the next wakeup to max. If the stale value continues to exist then we may never power off the domain.

Quoted 5 lines

Right.

But, how do you know that the use case starts and what prevents us from knowing that the use case has stopped?

Usually, the device is made aware of the usecase when it starts, but stopping the usecase might be something the device may or may not be aware of.

Quoted 4 lines

Maybe if you add a user of the new APIs, this would help me to understand better?

I have been asking some folks, but let's see.

[...]

Quoted 22 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

For example, there's no point doing the above, if the domain doesn't specify residency values for its idle states.

We would still need to ensure that the next wakeup is after the power_off_latency, if specified.

Quoted 6 lines

Quoted 3 lines

Good point! Although, I would rather avoid adding the overhead, unless the residency is specified. Do you see a problem with this approach?

Hmmm, no strong objections. However, we still need to run through the states to make sure the residency is not set and have a variable track that.

Right.

The important part is that we can do that once and not for every call to the governor.

Quoted 3 lines

The devices wouldn't know that and would still continue to set the next wakeup, unless we find a way to let them know we are not using this feature for the domain.

Quoted 6 lines

Right.

To allow the driver to know, we could respond with an error code from the new dev_pm_genpd_set_performance_state() API (from patch1), in case the genpd+governor doesn't support it.

It would an unnecessary work everytime a next wakeup is being set to iterate through the available states and figure out if the residency has been set or not. We could probably hold that result in a variable when we setup the genpd states. Expect the next_wake to be set from a critical path or an interrupt handler, so we have to be quick.

Quoted 4 lines

Would that be okay? Otherwise we will have to add a separate genpd API, asking explicitly if the "next wakeup" feature is supported.

Would like to avoid that as much as possible.

Quoted 13 lines · Lina Iyer

Quoted 9 lines · Ulf Hansson

Quoted 8 lines · Lina Iyer

Quoted 6 lines · Ulf Hansson

Quoted 5 lines · Lina Iyer

Quoted 2 lines · Ulf Hansson

Quoted 1 line · Lina Iyer

[...]

Quoted 7 lines

What you are suggesting is that, we should not power down the domain in such a case. This would be a really hard problem to debug when a device leaves a stale wakeup behind and we no longer power off the domain because of that. Tracking that back to the device will be a monumental effort. Ignoring the next wakeup though might involve a power/perf penalty (no worse than today), but we would not have a difficult problem to solve.

Hmm, you have a good point!

Additionally, I guess it should be a rather seldom situation, as in principle the wakeup irq should have been triggered already.

That said, I am okay to stick with your suggested approach.

Although, please add a comment in the code, to make it clear that the behaviour is deliberate. Perhaps we should also clarify the function description of dev_pm_genpd_set_next_wakeup() (in patch1) to make the behaviour more clear for the user.

Quoted 1 line

Quoted 39 lines · Lina Iyer

Quoted 7 lines · Ulf Hansson

Quoted 6 lines · Lina Iyer

Quoted 4 lines · Ulf Hansson

Quoted 3 lines · Lina Iyer

Quoted 1 line · Ulf Hansson

On Fri, 6 Nov 2020 at 17:48, Lina Iyer <[email protected]> wrote:

[...]

Quoted 22 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

For example, there's no point doing the above, if the domain doesn't specify residency values for its idle states.

We would still need to ensure that the next wakeup is after the power_off_latency, if specified.

Quoted 6 lines

Quoted 3 lines

Good point! Although, I would rather avoid adding the overhead, unless the residency is specified. Do you see a problem with this approach?

Hmmm, no strong objections. However, we still need to run through the states to make sure the residency is not set and have a variable track that.

Right.

The important part is that we can do that once and not for every call to the governor.

Quoted 3 lines

The devices wouldn't know that and would still continue to set the next wakeup, unless we find a way to let them know we are not using this feature for the domain.

Quoted 6 lines

Right.

To allow the driver to know, we could respond with an error code from the new dev_pm_genpd_set_performance_state() API (from patch1), in case the genpd+governor doesn't support it.

Quoted 5 lines

It would an unnecessary work everytime a next wakeup is being set to iterate through the available states and figure out if the residency has been set or not. We could probably hold that result in a variable when we setup the genpd states. Expect the next_wake to be set from a critical path or an interrupt handler, so we have to be quick.

Yes, that's the idea I had in mind.

Maybe it's not feasible, let's see. However, for sure I am looking at decreasing overhead, not to increase. :-)

Quoted 5 lines

Quoted 4 lines

Would that be okay? Otherwise we will have to add a separate genpd API, asking explicitly if the "next wakeup" feature is supported.

Would like to avoid that as much as possible.

Okay, good.

Kind regards
Uffe