On Fri, Aug 24, 2018 at 11:26:19AM +0200, Ulf Hansson wrote: [...] > > That's a good question and it maybe gives a path towards a solution. > > > > AFAICS the genPD governor only selects the idle state parameter that > > determines the idle state at, say, GenPD cpumask level it does not touch > > the CPUidle decision, that works on a subset of idle states (at cpu > > level). > > > > That's my understanding, which can be wrong so please correct me > > if that's the case because that's a bit confusing. > > > > Let's imagine that we flattened out the list of idle states and feed > > CPUidle with it (all of them - cpu, cluster, package, system - as it is > > in the mainline _now_). Then the GenPD governor can run-through the > > CPUidle selection and _demote_ the idle state if necessary since it > > understands that some CPUs in the GenPD will wake up shortly and break > > the target residency hyphothesis the CPUidle governor is expecting. > > > > The whole idea about this series is improving CPUidle decision when > > the target idle state is _shared_ among groups of cpus (again, please > > do correct me if I am wrong). > > Absolutely, this is one of the main reason for the series! > > > > > It is obvious that a GenPD governor must only demote - never promote a > > CPU idle state selection given that hierarchy implies more power > > savings and higher target residencies required. > > Absolutely. I apologize if I have been using the word "promote" > wrongly, I realize it may be a bit confusing. > > > > > This whole series would become more generic and won't depend on > > PSCI OSI at all - actually that would become a hierarchical > > CPUidle governor. > > Well, to me we need a first user of the new infrastructure code in > genpd and PSCI is probably the easiest one to start with. An option > would be to start with an old ARM32 platform, but it seems a bit silly > to me. If the code can be structured as described above as a hierarchical (possibly optional through a Kconfig entry or sysfs tuning) idle decision you can apply it to _any_ PSCI based platform out there, provided that the new governor improves power savings. > In regards to OS-initiated mode vs platform coordinated mode, let's > discuss that in details in the other email thread instead. I think that's crystal clear by now that IMHO PSCI OS-initiated mode is a red-herring, it has nothing to do with this series, it is there just because QC firmware does not support PSCI platform coordinated suspend mode. You can apply the concept in this series to _any_ arch provided the power domains representation is correct (and again, I would sound like a broken record but the series must improve power savings over vanilla CPUidle menu governor). > > I still think that PSCI firmware and most certainly mwait() play the > > role the GenPD governor does since they can detect in FW/HW whether > > that's worthwhile to switch off a domain, the information is obviously > > there and the kernel would just add latency to the idle path in that > > case but let's gloss over this for the sake of this discussion. > > Yep, let's discuss that separately. > > That said, can I interpret your comments on the series up until this > change, that you seems rather happy with where the series is going? It is something we have been discussing with Daniel since generic idle was merged for Arm a long while back. I have nothing against describing idle states with power domains but it must improve idle decisions against the mainline. As I said before, runtime PM can also be used to get rid of CPU PM notifiers (because with power domains we KNOW what devices eg PMU are switched off on idle entry, we do not guess any longer; replacing CPU PM notifiers is challenging and can be tackled - if required - in a different series). Bottom line (talk is cheap, I know and apologise about that): this series (up until this change) adds complexity to the idle path and lots of code; if its usage is made optional and can be switched on on systems where it saves power that's fine by me as long as we keep PSCI OS-initiated idle states out of the equation, that's an orthogonal discussion as, I hope, I managed to convey. Thanks, Lorenzo
