Re: [PATCH v4 2/2] PM / Domains: use device's next wakeup to determine domain idle state

[Lina Iyer <ilina@codeaurora.org>]

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;
>> --