lkml.org 
[lkml]   [2018]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V2 5/5] PM / Domains: Propagate performance state updates
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)



> 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().

I know things are repetitive here, but that's the right way of doing it IMHO.
What do you say ?

--
viresh

\
 
 \ /
  Last update: 2018-11-30 12:07    [W:0.089 / U:9.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site