| Message ID | 20201012223400.23609-3-ilina@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Better domain idle from device wakeup patterns | expand |
Hi Lina,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on pm/linux-next]
[also build test WARNING on linus/master v5.9 next-20201013]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lina-Iyer/Better-domain-idle-from-device-wakeup-patterns/20201013-063543
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-m021-20201013 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
smatch warnings:
drivers/base/power/domain_governor.c:269 default_power_down_ok() warn: always true condition '(state_idx >= 0) => (0-u32max >= 0)'
drivers/base/power/domain_governor.c:269 default_power_down_ok() warn: always true condition '(state_idx >= 0) => (0-u32max >= 0)'
drivers/base/power/domain_governor.c:277 default_power_down_ok() warn: unsigned 'state_idx' is never less than zero.
vim +269 drivers/base/power/domain_governor.c
245
246 /**
247 * default_power_down_ok - Default generic PM domain power off governor routine.
248 * @pd: PM domain to check.
249 *
250 * This routine must be executed under the PM domain's lock.
251 */
252 static bool default_power_down_ok(struct dev_pm_domain *pd)
253 {
254 struct generic_pm_domain *genpd = pd_to_genpd(pd);
255 struct gpd_link *link;
256 unsigned int state_idx;
257 ktime_t now = ktime_get();
258
259 /*
260 * Find the next wakeup from devices that can determine their own wakeup
261 * to find when the domain would wakeup and do it for every device down
262 * the hierarchy. It is not worth while to sleep if the state's residency
263 * cannot be met.
264 */
265 update_domain_next_wakeup(genpd, now);
266 state_idx = genpd->state_count - 1;
267 if (genpd->next_wakeup != KTIME_MAX) {
268 /* Let's find out the deepest domain idle state, the devices prefer */
> 269 while (state_idx >= 0) {
270 if (next_wakeup_allows_state(genpd, state_idx, now)) {
271 genpd->max_off_time_changed = true;
272 break;
273 }
274 state_idx--;
275 }
276
> 277 if (state_idx < 0) {
278 state_idx = 0;
279 genpd->cached_power_down_ok = false;
280 goto done;
281 }
282 }
283
284 if (!genpd->max_off_time_changed) {
285 genpd->state_idx = genpd->cached_power_down_state_idx;
286 return genpd->cached_power_down_ok;
287 }
288
289 /*
290 * We have to invalidate the cached results for the parents, so
291 * use the observation that default_power_down_ok() is not
292 * going to be called for any parent until this instance
293 * returns.
294 */
295 list_for_each_entry(link, &genpd->child_links, child_node)
296 link->parent->max_off_time_changed = true;
297
298 genpd->max_off_time_ns = -1;
299 genpd->max_off_time_changed = false;
300 genpd->cached_power_down_ok = true;
301
302 /* Find a state to power down to, starting from the state
303 * determined by the next wakeup.
304 */
305 while (!__default_power_down_ok(pd, state_idx)) {
306 if (state_idx == 0) {
307 genpd->cached_power_down_ok = false;
308 break;
309 }
310 state_idx--;
311 }
312
313 done:
314 genpd->state_idx = state_idx;
315 genpd->cached_power_down_state_idx = genpd->state_idx;
316 return genpd->cached_power_down_ok;
317 }
318
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c index 490ed7deb99a..77b4928aa389 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 = READ_ONCE(pdd->dev->power.next_event); + 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; + unsigned 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,20 @@ 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 ee11502a575b..9ea6f666967b 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -119,6 +119,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; bool max_off_time_changed; bool cached_power_down_ok; bool cached_power_down_state_idx;
If the device's next event is known, determine if it is worthwhile entering a domain idle state. To find the next wakeup, traverse a domain for all child devices and find out the earliest wakeup value. A parent domain's next wakeup is the earliest of all its child devices and domains. The next wakeup is specified by devices in the domains before they devices are suspended. Update the domain governor logic to determine if it is worthwhile to enter an idle state based on the next wakeup for the domain along with other existing constraints. Signed-off-by: Lina Iyer <ilina@codeaurora.org> --- drivers/base/power/domain_governor.c | 83 ++++++++++++++++++++++++++-- include/linux/pm_domain.h | 1 + 2 files changed, 79 insertions(+), 5 deletions(-)