From: Ulf Hansson <ulf.hansson@linaro.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Kevin Hilman <khilman@kernel.org>, Pavel Machek <pavel@ucw.cz>,
Len Brown <len.brown@intel.com>,
Vincent Guittot <vincent.guittot@linaro.org>,
Rajendra Nayak <rnayak@codeaurora.org>,
Niklas Cassel <niklas.cassel@linaro.org>,
Linux PM <linux-pm@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Stephen Boyd <sboyd@kernel.org>,
Mike Turquette <mturquette@baylibre.com>,
grahamr@codeaurora.org
Subject: Re: [PATCH V2 5/5] PM / Domains: Propagate performance state updates
Date: Mon, 3 Dec 2018 14:38:35 +0100 [thread overview]
Message-ID: <CAPDyKFr_cG1if1Aozo+4hpQ6jY8sHD1wx91QCTwsnKcF5TwUhw@mail.gmail.com> (raw)
In-Reply-To: <20181130110650.6ni2ft3wud7gn6jz@vireshk-i7>
+ Stephen, Mike, Graham
On Fri, 30 Nov 2018 at 12:06, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 30-11-18, 11:18, Ulf Hansson wrote:
> > On Fri, 30 Nov 2018 at 10:59, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > Sure, but the ordering of locks is always subdomain first and then master.
> > > Considering the case of Qcom, we have two domains Cx (sub-domain) and Mx (master).
> > >
> > > On first genpd_power_on(Cx) call, we will first call genpd_power_on(Mx) which
> > > will just power it on as none of its master will have perf-state support. We
> > > then call _genpd_power_on(Cx) which will also not do anything with Mx as its own
> > > (Cx's) pstate would be 0 at that time. But even if it had a valid value, it will
> > > propagate just fine with all proper locking in place.
> >
> > Can you explain that, it's not super easy to follow the flow.
>
> Sorry, I somehow assumed you would know it already :)
>
> > So what will happen if Cx has a value that needs to be propagated?
> > What locks will be taken, and in what order?
> >
> > Following, what if we had a Bx domain, being the subdomain of Cx, and
> > it too had a value that needs to be propagated.
>
> Lets take the worst example, we have Bx (sub-domain of Cx), Cx (sub-domain of
> Mx) and Dx (master). Normal power-on/off will always have the values 0, so lets
> consider resume sequence where all the domains will have a value pstate value.
> And please forgive me for any bugs I have introduced in the following
> super-complex sequence :)
>
> genpd_runtime_resume(dev) //domain Bx
> -> genpd_lock(Bx)
> -> genpd_power_on(Bx)
>
> -> genpd_lock(Cx)
> -> genpd_power_on(Cx)
>
> -> genpd_lock(Dx)
> -> genpd_power_on(Dx)
>
> -> _genpd_power_on(Dx)
> -> _genpd_set_performance_state(Dx, Dxstate) {
> //Doesn't have any masters
> -> genpd->set_performance_state(Dx, Dxstate);
> }
>
> -> genpd_unlock(Dx)
>
> -> _genpd_power_on(Cx)
> -> _genpd_set_performance_state(Cx, Cxstate) {
> //have one master, Dx
> -> genpd_lock(Dx)
> -> _genpd_set_performance_state(Dx, Dxstate) {
> //Doesn't have any masters
> -> genpd->set_performance_state(Dx, Dxstate);
> }
>
> -> genpd_unlock(Dx)
>
> // Change Cx state
> -> genpd->set_performance_state(Cx, Cxstate);
> }
>
> -> genpd_unlock(Cx)
>
> -> _genpd_power_on(Bx)
> -> _genpd_set_performance_state(Bx, Bxstate) {
> //have one master, Cx
> -> genpd_lock(Cx)
> -> _genpd_set_performance_state(Cx, Cxstate) {
> //have one master, Dx
> -> genpd_lock(Dx)
> -> _genpd_set_performance_state(Dx, Dxstate) {
> //Doesn't have any masters
> -> genpd->set_performance_state(Dx, Dxstate);
> }
>
> -> genpd_unlock(Dx)
>
> // Change Cx state
> -> genpd->set_performance_state(Cx, Cxstate);
> }
> -> genpd_unlock(Cx)
>
> -> genpd->set_performance_state(Bx, Bxstate);
> }
>
> -> genpd_unlock(Bx)
>
>
Thanks for clarifying. This confirms my worries about the locking overhead.
>
> > It sounds like we will
> > do the propagation one time per level. Is that really necessary,
> > couldn't we just do it once, after the power on sequence have been
> > completed?
>
> It will be a BIG hack somewhere, isn't it ? How will we know when has the time
> come to shoot the final sequence of set_performance_state() ? And where will we
> do it? genpd_runtime_resume() ? And then we will have more problems, for example
> Rajendra earlier compared this stuff to clk framework where it is possible to do
> clk_set_rate() first and then only call clk_enable() and the same should be
> possible with genpd as well, i.e. set performance state first and then only
> enable the device/domain. And so we need this right within genpd_power_on().
There is one a big difference while comparing with clocks, which make
this more difficult.
That is, in dev_pm_genpd_set_performance_state(), we are *not* calling
->the set_performance_state() callback of the genpd, unless the genpd
is already powered on. Instead, for that case, we are only aggregating
the performance states votes, to defer to invoke
->set_performance_state() until the genpd becomes powered on. In some
way this makes sense, but for clock_set_rate(), the clock's rate can
be changed, no matter if the clock has been prepared/enabled or not.
I recall we discussed this behavior of genpd, while introducing the
performance states support to it. Reaching this point, introducing the
master-domain propagation of performance states votes, we may need to
re-consider the behavior, as there is evidently an overhead that grows
along with the hierarchy.
As a matter of fact, what I think this boils to, is to consider if we
want to temporary drop the performance state vote for a device from
genpd's ->runtime_suspend() callback. Thus, also restore the vote from
genpd's ->runtime_resume() callback. That's because, this is directly
related to whether genpd should care about whether it's powered on or
off, when calling the ->set_performance_state(). We have had
discussions at LKML already around this topic. It seems like we need
to pick them up to reach a consensus, before we can move forward with
this.
>
> I know things are repetitive here, but that's the right way of doing it IMHO.
> What do you say ?
As this point, honestly I don't know yet.
I have looped in Stephen, Mike and Graham, let's see if they have some
thoughts on the topic.
Kind regards
Uffe
next prev parent reply other threads:[~2018-12-03 13:39 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-26 8:09 [PATCH V2 0/5] PM / Domains: Allow performance state propagation Viresh Kumar
2018-11-26 8:10 ` [PATCH V2 1/5] OPP: Improve _find_table_of_opp_np() Viresh Kumar
2018-11-26 8:10 ` [PATCH V2 2/5] OPP: Add dev_pm_opp_xlate_performance_state() helper Viresh Kumar
2018-11-30 8:45 ` Ulf Hansson
2018-12-03 6:42 ` Viresh Kumar
2018-11-26 8:10 ` [PATCH V2 3/5] PM / Domains: Save OPP table pointer in genpd Viresh Kumar
2018-11-30 8:53 ` Ulf Hansson
2018-11-30 9:05 ` Viresh Kumar
2018-12-03 6:57 ` Viresh Kumar
2018-11-26 8:10 ` [PATCH V2 4/5] PM / Domains: Factorize dev_pm_genpd_set_performance_state() Viresh Kumar
2018-11-30 8:54 ` Ulf Hansson
2018-11-26 8:10 ` [PATCH V2 5/5] PM / Domains: Propagate performance state updates Viresh Kumar
2018-11-30 9:44 ` Ulf Hansson
2018-11-30 9:59 ` Viresh Kumar
2018-11-30 10:18 ` Ulf Hansson
2018-11-30 11:06 ` Viresh Kumar
2018-12-03 13:38 ` Ulf Hansson [this message]
2018-12-05 6:42 ` Stephen Boyd
2018-12-05 17:29 ` Ulf Hansson
2018-12-03 8:50 ` Viresh Kumar
2018-11-27 4:50 ` [PATCH V2 0/5] PM / Domains: Allow performance state propagation Rajendra Nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPDyKFr_cG1if1Aozo+4hpQ6jY8sHD1wx91QCTwsnKcF5TwUhw@mail.gmail.com \
--to=ulf.hansson@linaro.org \
--cc=grahamr@codeaurora.org \
--cc=khilman@kernel.org \
--cc=len.brown@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=niklas.cassel@linaro.org \
--cc=pavel@ucw.cz \
--cc=rjw@rjwysocki.net \
--cc=rnayak@codeaurora.org \
--cc=sboyd@kernel.org \
--cc=vincent.guittot@linaro.org \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).