From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thara Gopinath Subject: Re: [PATCH 1/2] PM / Domains: Add time accounting to various genpd states. Date: Wed, 14 Jun 2017 17:01:37 -0400 Message-ID: <5941A431.1090901@linaro.org> References: <1495756293-29534-1-git-send-email-thara.gopinath@linaro.org> <1495756293-29534-2-git-send-email-thara.gopinath@linaro.org> <59404863.1030608@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: Received: from mail-yw0-f182.google.com ([209.85.161.182]:34198 "EHLO mail-yw0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751664AbdFNVBk (ORCPT ); Wed, 14 Jun 2017 17:01:40 -0400 Received: by mail-yw0-f182.google.com with SMTP id e142so8187907ywa.1 for ; Wed, 14 Jun 2017 14:01:40 -0700 (PDT) In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Ulf Hansson Cc: "linux-pm@vger.kernel.org" , Kevin Hilman On 06/14/2017 05:18 AM, Ulf Hansson wrote: > On 13 June 2017 at 22:17, Thara Gopinath wrote: >> On 06/07/2017 09:28 AM, Ulf Hansson wrote: >> Hello Ulf, >> >>> On 26 May 2017 at 01:51, Thara Gopinath 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 >>>> --- >>>> 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. Ok. Sorry for all the confusion. It is possible to rework genpd_update_accounting. I sent out the V2 yesterday. I will wait for a couple of more days for any other review comments on V2 before sending the re-worked patch set. Regards Thara > > Kind regards > Uffe > -- Regards Thara