| Message ID | 20201020180413.32225-3-ilina@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Better domain idle from device wakeup patterns | expand |
On Tue, Oct 20, 2020 at 8:05 PM Lina Iyer <ilina@codeaurora.org> wrote: > > 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 <ilina@codeaurora.org> A few nits follow. > --- > 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 | 84 ++++++++++++++++++++++++++-- > include/linux/pm_domain.h | 1 + > 2 files changed, 80 insertions(+), 5 deletions(-) > > diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c > index 490ed7deb99a..092927b60dc0 100644 > --- a/drivers/base/power/domain_governor.c > +++ b/drivers/base/power/domain_governor.c > @@ -117,6 +117,49 @@ 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; > + 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; > + } > + > + /* 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; > + } This is assuming that the lists will remain stable during the walks above, but is that guaranteed? > + > + genpd->next_wakeup = domain_wakeup; > +} > + > +static bool next_wakeup_allows_state(struct generic_pm_domain *genpd, > + unsigned int state, ktime_t now) > +{ > + s64 idle_time_ns, min_sleep_ns; > + ktime_t domain_wakeup = genpd->next_wakeup; I'd move the second line to the top (it looks odd now IMO). > + > + min_sleep_ns = genpd->states[state].power_off_latency_ns + > + genpd->states[state].power_on_latency_ns + > + genpd->states[state].residency_ns; > + > + idle_time_ns = ktime_to_ns(ktime_sub(domain_wakeup, now)); > + if (idle_time_ns < min_sleep_ns) > + return false; > + > + return true; Why not + return idle_time_ns >= min_sleep_ns; > +} > + > static bool __default_power_down_ok(struct dev_pm_domain *pd, > unsigned int state) > { > @@ -210,6 +253,33 @@ static bool default_power_down_ok(struct dev_pm_domain *pd) > { > struct generic_pm_domain *genpd = pd_to_genpd(pd); > struct gpd_link *link; > + int state_idx; Why not initialize it right away? > + ktime_t now = ktime_get(); > + > + /* > + * 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); > + state_idx = genpd->state_count - 1; > + 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; > + } > + state_idx--; > + } > + > + if (state_idx < 0) { > + state_idx = 0; > + genpd->cached_power_down_ok = false; > + goto done; > + } > + } > > if (!genpd->max_off_time_changed) { > genpd->state_idx = genpd->cached_power_down_state_idx; > @@ -228,17 +298,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; > } > - 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; > } > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index e00c77b1efd8..205b750a2e56 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -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; > --
Thanks for your time Rafael. On Thu, Nov 05 2020 at 11:56 -0700, Rafael J. Wysocki wrote: >On Tue, Oct 20, 2020 at 8:05 PM Lina Iyer <ilina@codeaurora.org> wrote: >> >> 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 <ilina@codeaurora.org> > >A few nits follow. > >> --- >> 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 | 84 ++++++++++++++++++++++++++-- >> include/linux/pm_domain.h | 1 + >> 2 files changed, 80 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c >> index 490ed7deb99a..092927b60dc0 100644 >> --- a/drivers/base/power/domain_governor.c >> +++ b/drivers/base/power/domain_governor.c >> @@ -117,6 +117,49 @@ 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; >> + 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; >> + } >> + >> + /* 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; >> + } > >This is assuming that the lists will remain stable during the walks >above, but is that guaranteed? > I would expect so. We are looking up the child domains, which should be powered down and therefore their domain::next_wakeup should be locked in (because we update next_wakeup only in this governor). >> + >> + genpd->next_wakeup = domain_wakeup; >> +} >> + >> +static bool next_wakeup_allows_state(struct generic_pm_domain *genpd, >> + unsigned int state, ktime_t now) >> +{ >> + s64 idle_time_ns, min_sleep_ns; >> + ktime_t domain_wakeup = genpd->next_wakeup; > >I'd move the second line to the top (it looks odd now IMO). > I agree and prefer that as well but I missed this. Will update in the next spin. >> + >> + min_sleep_ns = genpd->states[state].power_off_latency_ns + >> + genpd->states[state].power_on_latency_ns + >> + genpd->states[state].residency_ns; >> + >> + idle_time_ns = ktime_to_ns(ktime_sub(domain_wakeup, now)); >> + if (idle_time_ns < min_sleep_ns) >> + return false; >> + >> + return true; > >Why not > >+ return idle_time_ns >= min_sleep_ns; > OK. >> +} >> + >> static bool __default_power_down_ok(struct dev_pm_domain *pd, >> unsigned int state) >> { >> @@ -210,6 +253,33 @@ static bool default_power_down_ok(struct dev_pm_domain *pd) >> { >> struct generic_pm_domain *genpd = pd_to_genpd(pd); >> struct gpd_link *link; >> + int state_idx; > >Why not initialize it right away? > OK. Thanks, Lina >> + ktime_t now = ktime_get(); >> + >> + /* >> + * 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); >> + state_idx = genpd->state_count - 1; >> + 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; >> + } >> + state_idx--; >> + } >> + >> + if (state_idx < 0) { >> + state_idx = 0; >> + genpd->cached_power_down_ok = false; >> + goto done; >> + } >> + } >> >> if (!genpd->max_off_time_changed) { >> genpd->state_idx = genpd->cached_power_down_state_idx; >> @@ -228,17 +298,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; >> } >> - 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; >> } >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index e00c77b1efd8..205b750a2e56 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -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; >> --
diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c index 490ed7deb99a..092927b60dc0 100644 --- a/drivers/base/power/domain_governor.c +++ b/drivers/base/power/domain_governor.c @@ -117,6 +117,49 @@ 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; + 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; + } + + /* 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; +} + +static bool next_wakeup_allows_state(struct generic_pm_domain *genpd, + unsigned int state, ktime_t now) +{ + s64 idle_time_ns, min_sleep_ns; + ktime_t domain_wakeup = genpd->next_wakeup; + + min_sleep_ns = genpd->states[state].power_off_latency_ns + + genpd->states[state].power_on_latency_ns + + genpd->states[state].residency_ns; + + idle_time_ns = ktime_to_ns(ktime_sub(domain_wakeup, now)); + if (idle_time_ns < min_sleep_ns) + return false; + + return true; +} + static bool __default_power_down_ok(struct dev_pm_domain *pd, unsigned int state) { @@ -210,6 +253,33 @@ static bool default_power_down_ok(struct dev_pm_domain *pd) { struct generic_pm_domain *genpd = pd_to_genpd(pd); struct gpd_link *link; + int state_idx; + ktime_t now = ktime_get(); + + /* + * 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); + state_idx = genpd->state_count - 1; + 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; + } + state_idx--; + } + + if (state_idx < 0) { + state_idx = 0; + genpd->cached_power_down_ok = false; + goto done; + } + } if (!genpd->max_off_time_changed) { genpd->state_idx = genpd->cached_power_down_state_idx; @@ -228,17 +298,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; } - 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; } diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index e00c77b1efd8..205b750a2e56 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -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;
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 <ilina@codeaurora.org> --- 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 | 84 ++++++++++++++++++++++++++-- include/linux/pm_domain.h | 1 + 2 files changed, 80 insertions(+), 5 deletions(-)