Messages in this thread | | | From | Ulf Hansson <> | Date | Thu, 21 Oct 2021 20:36:08 +0200 | Subject | Re: [PATCH 2/2] PM: sleep: Fix runtime PM based cpuidle support |
| |
[...]
> > > > > > > > > > > > > Additionally, since find_deepest_state() is being called for > > > > > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > > > > > > > > > No, it already checks "disabled". > > > > > > > > > > Yes, but that would be wrong. > > > > > > > > Hmmm. > > > > > > > > > The use case I want to support, for cpuidle-psci, is to allow all idle > > > > > states in suspend-to-idle, > > > > > > > > So does PM-runtime work in suspend-to-idle? How? > > > > > > > > > but prevent those that rely on runtime PM > > > > > (after it has been disabled) for the regular idle path. > > > > > > > > Do you have a special suspend-to-idle handling of those states that > > > > doesn't require PM-runtime? > > > > > > Regardless, pausing cpuidle in the suspend-to-idle path simply doesn't > > > make sense at all, so this needs to be taken care of in the first > > > place. > > > > Right, I do agree, don't get me wrong. But, do we really want to treat > > s2-to-idle differently, compared to s2-to-ram in regards to this? > > > > Wouldn't it be a lot easier to let cpuidle drivers to opt-out for > > cpuidle_pause|resume(), no matter whether it's for s2-to-idle or > > s2-to-ram? > > I don't think so. > > Suspend-to-idle resume cpuidle after pausing it which is just plain > confusing and waste of energy and the fact that the system-wide > suspend flow interferes with using PM-runtime for implementing cpuidle > callbacks at the low level really is an orthogonal problem.
It's certainly an orthogonal problem, I agree. However, trying to solve it in two different ways, may not really be worth the effort, in my opinion.
As I kind of pointed out in the earlier reply, I am not sure there are any other relatively easy solutions available, to fix the problem for runtime PM based cpuidle drivers. We probably need to call cpuidle_pause() (or similar) in some way.
> > > > > > > The problem with PM-runtime being unavailable after dpm_suspend() > > > needs to be addressed in a different way IMO, because it only affects > > > one specific use case. > > > > It's one specific case so far, but we have the riscv driver on its > > way, which would suffer from the same problem. > > So perhaps they should be advised about this issue.
Yes, I will let them know - and hopefully I will soon also be able to provide them with a fix. :-)
> > > Anyway, an option is to figure out what platforms and cpuidle drivers, > > that really needs cpuidle_pause|resume() at this point and make an > > opt-in solution instead. > > None of them need to pause cpuidle for suspend-to-idle AFAICS.
I assume so too, otherwise things would have been broken when cpuidle_resume() is called in s2idle_enter(). But, it's still a bit unclear.
> > Some may want it in the non-s2idle suspend path, but I'm not sure > about the exact point where cpuidle needs to be paused in this case. > Possibly before offlining the nonboot CPUs.
Okay.
Note that, I assume it would be okay to also pause cpuidle a bit earlier in these cases, like in dpm_suspend() for example. The point is, it's really a limited short period of time for when cpuidle would be paused, so I doubt it would have any impact on the consumed energy. Right?
> > > This could then be used by runtime PM based > > cpuidle drivers as well. Would that be a way forward? > > The PM-runtime case should be addressed directly IMO, we only need to > figure out how to do that.
If you have any other suggestions, I am listening. :-)
> > I'm wondering how you are dealing with the case when user space > prevents pd_dev from suspending via sysfs, for that matter.
That should work fine during runtime - because runtime PM is enabled for the device.
Kind regards Uffe
| |