Messages in this thread | | | From | Ulf Hansson <> | Date | Fri, 4 Jun 2021 12:57:59 +0200 | Subject | Re: [PATCH v2 0/4] PM: domains: Avoid boilerplate code for DVFS in subsystem/drivers |
| |
On Fri, 4 Jun 2021 at 10:23, Stephan Gerhold <stephan@gerhold.net> wrote: > > On Fri, Jun 04, 2021 at 09:18:45AM +0200, Ulf Hansson wrote: > > On Thu, 3 Jun 2021 at 19:16, Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > On Thu, Jun 03, 2021 at 05:27:30PM +0200, Ulf Hansson wrote: > > > > On Thu, 3 Jun 2021 at 13:13, Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > I think this might also go into the direction of my problem with the OPP > > > > > core for CPU DVFS [1] since the OPP core currently does not "power-on" > > > > > the power domains, it just sets a performance state. I got kind of stuck > > > > > with all the complexity of power domains in Linux so I think we never > > > > > solved that. > > > > > > > > Hmm, that issue is in a way related. > > > > > > > > Although, if I understand correctly, that was rather about at what > > > > layer it makes best sense to activate the device (from runtime PM > > > > point of view). And this was needed due to the fact that the > > > > corresponding genpd provider, requires the PM domain to be power on to > > > > allow changing a performance state for it. Did I get that correct? > > > > > > > > > > Yes, mostly. But I guess I keep coming back to the same question: > > > > > > When/why does it make sense to vote for a "performance state" of > > > a power domain that is or might be powered off? > > > > > > "Powered off" sounds like the absolutely lowest possible performance > > > state to me, it's just not on at all. And if suddenly a device comes and > > > says "I want performance state X", nothing can change until the power > > > domain is also "powered on". > > > > > > I think my "CPU DVFS" problem only exists because in many other > > > situations it's possible to rely on one of the following side effects: > > > > > > 1. The genpd provider does not care if it's powered on or not. > > > (i.e. it's always-on or implicitly powers on if state > 0). > > > 2. There is some other device that votes to keep the power domain on. > > > > > > And that's how the problem relates to my comment for this patch series ... > > > > > > > > > > > > > > > > > Do I understand your patch set correctly that you basically make the > > > > > performance state votes conditional to the "power-on" vote of the device > > > > > (which is automatically toggled during runtime/system PM)? > > > > > > > > The series can be considered as a step in that direction, but no, this > > > > series doesn't change that behaviour. > > > > > > > > Users of dev_pm_genpd_set_performance_state() are still free to set a > > > > performance state, orthogonally to whether the PM domain is powered on > > > > or off. > > > > > > > > > > > > > > If yes, I think that's a good thing. It was always really confusing to me > > > > > that a device can make performance state votes if it doesn't actually > > > > > want the power domain to be powered on. > > > > > > > > I share your view, it's a bit confusing. > > > > > > > > Just adding the condition internally to genpd to prevent the caller of > > > > dev_pm_genpd_set_performance() from succeeding to set a new state, > > > > unless the genpd is powered on, should be a rather simple thing to > > > > add. > > > > > > > > However, to change this, we first need to double check that all the > > > > callers are making sure they have turned on the PM domain (typically > > > > via runtime PM). > > > > > > > > > > ... because if performance state votes would be conditional to the > > > "power-on" vote of the device, it would no longer be possible > > > to rely on the side effects mentioned above. So this would most > > > certainly break some code that (incorrectly?) relies on these side > > > effects, but would also prevent such code. > > > > Right. I understand your point and I am open to discuss an > > implementation. Although, I suggest we continue that separately from > > the $subject series. > > > > > > > > My (personal) feeling so far is that just dropping performance votes > > > during runtime/system suspend just makes the entire situation even more > > > confusing. > > > > Well, that's what most subsystems/drivers need to do. > > > > Moreover, we have specific devices that only use one default OPP [1]. > > > > > > > > > > > > > > > What happens if a driver calls dev_pm_genpd_set_performance_state(...) > > > > > while the device is suspended? Will that mess up the performance state > > > > > when the device resumes? > > > > > > > > Good question. The idea is: > > > > > > > > If genpd in genpd_runtime_suspend() are able to drop an existing vote > > > > for a performance state, it should restore the vote in > > > > genpd_runtime_resume(). This also means, if there is no vote to drop > > > > in genpd_runtime_suspend(), genpd should just leave the vote as is in > > > > genpd_runtime_resume(). > > > > > > > > > > But the next time the device enters runtime suspend that vote would be > > > dropped, wouldn't it? That feels kind of strange to me. > > > > What do you mean by "next time"? > > > > Basically just like: > > <device runtime-suspended> > driver does dev_pm_genpd_set_performance_state(...) > - performance state is applied immediately, even though device does > apparently not actually want the power domain to be powered on > <device runtime resumes> > - performance state is kept > <device runtime suspends> > - performance state is dropped
Yep, this is what would happen.
> ... > > I'm not saying this example makes sense (it doesn't for me). It doesn't > make sense to vote for a performance state while runtime suspended. > > But with this patch series we still allow that, and it will kind of > produce inconsistent behavior that the performance state is applied > immediately, even though the device is currently runtime-suspended. > But once it runtime suspends again, suddenly it is dropped.
Yes.
Note that, I have been looking at the existing callers of dev_pm_genpd_set_performance_state() in the kernel as of today. It should not be an issue, at least as far as I can tell.
> > And when you say: > > > My main point is, if the device enters runtime suspend state, why > > should we keep the vote for an OPP for the device? I mean, the device > > isn't going to be used anyway. > > > > A very similar point would be: "If the device *is* in runtime suspend > state, why should we take a vote for an OPP for the device?" > > But I understand that this might be something we should address > separately in a follow-up patch/discussion. Don't get me wrong, I agree > this patch set is good, I just think we should go one step further and > finally make this consistent and less prone to side effects.
I agree. We should look into how to change the behaviour. I intend to have a look at it in a while.
> > A good first step might be something like a WARN_ON_ONCE(...) if a > device tries to vote for a performance state while runtime suspended. > Then we might get a clearer picture which drivers do that currently.
That's an idea we could try, even if the number of users are quite limited today. I can try the "git grep" analyze-method, I will probably find most of them.
> > Stephan
That said, are you okay that we move forward with the $subject series (except patch4)?
Kind regards Uffe
| |