Re: [PATCH 1/2] PM / Domains: Add time accounting to various genpd states.

[Ulf Hansson <ulf.hansson@linaro.org>]

On 13 June 2017 at 22:17, Thara Gopinath <thara.gopinath@linaro.org> wrote:
> On 06/07/2017 09:28 AM, Ulf Hansson wrote:
> Hello Ulf,
>
>> On 26 May 2017 at 01:51, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>>> This patch adds support to calculate the time spent by the generic
>>> power domains in on and various idle states.
>>>
>>> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
>>> ---
>>>  drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
>>>  include/linux/pm_domain.h   |  4 ++++
>>>  2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index da49a83..e733796 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -207,6 +207,25 @@ static void genpd_sd_counter_inc(struct generic_pm_domain *genpd)
>>>         smp_mb__after_atomic();
>>>  }
>>>
>>> +static void genpd_update_accounting(struct generic_pm_domain *genpd)
>>
>> I suggest we have all this within #ifdef CONFIG_DEBUG_FS and add a
>> stub for !CONFIG_DEBUG_FS.
>>
>>> +{
>>> +       ktime_t delta, now;
>>> +
>>> +       now = ktime_get();
>>> +       delta = ktime_sub(now, genpd->accounting_time);
>>> +
>>> +       if (genpd->status == GPD_STATE_ACTIVE) {
>>> +               genpd->on_time = ktime_add(genpd->on_time, delta);
>>> +       } else {
>>> +               int state_idx = genpd->state_idx;
>>> +
>>> +               genpd->states[state_idx].active_time =
>>
>> "active_time" seems to be a misleading, can we rename it to "idle_time" instead?
>>
>>> +                       ktime_add(genpd->states[state_idx].active_time, delta);
>>> +               genpd->off_time = ktime_add(genpd->off_time, delta);
>>
>> Isn't the off_time the summary of the states' genpd->states[i].active_time?
>>
>> Then we could do that computation quite easily at the point when
>> producing the debugfs output instead.
>>
>>> +       }
>>> +       genpd->accounting_time = now;
>>> +}
>>> +
>>>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>>  {
>>>         unsigned int state_idx = genpd->state_idx;
>>> @@ -358,6 +377,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>>>                         return ret;
>>>         }
>>>
>>> +       genpd_update_accounting(genpd);
>>
>> It find it a bit odd to update accounting just before changing the
>> status. Can we re-work this and update accounting right afterwards
>> instead? At least to me, that makes this easier to understand.
>
> The accounting is being updated for the previous state and not the
> current state. For eg if the domain has been previously off and just
> turned on, the time spent in off state is updated. So it
> upadte_accounting has to be before changing the status.
> Sorry I did not mention this earlier as I just realized this as
> I was reworking the patches.
>

Yes, I understand.

However, of course changing this also means that you need to change
how genpd_update_accounting works. That's was my main point, sorry if
that wasn't clear.

Kind regards
Uffe