On Thu, Jan 11, 2018 at 09:42:57AM +0000, Quentin Perret wrote:

Hi Eduardo,

On Wednesday 10 Jan 2018 at 11:34:33 (-0800), Eduardo Valentin wrote:

quoted

On Tue, Jan 09, 2018 at 11:02:50AM +0000, Quentin Perret wrote:

quoted

Currently, IPA estimates the power dissipated by a CPU at each available OPP
using its capacitance (the dynamic-power-coefficient DT binding). This series
relocates this feature into the OPP library as a preparation for future
changes. More specifically:

   1. The current DT-based approach for power estimation will need deep
      changes to support SCMI-provided power values. While the thermal
      subsystem is not necessarily the best place to hide multiple power
      estimation methods, the OPP library appears to be a good candidate to
      implement the required platform abstraction.
   2. The energy models of CPUs will be needed by other clients in the future
      (such as the task scheduler or CPUFreq governors for example) in order
      to make energy-aware decisions. The relocation to the OPP library will
      enable code re-use and all clients will benefit form the platform
      abstraction mentioned previously.

To be quite frank, I am happy to see this leaving thermal subsystem.
However, a few concerns with the patch set as it is. First, I am not
convinced PM OPP is the right place to put this, nor I see a good
explanation put in the patch set why it must be part of PM OPP.

IPA already uses the OPP library to build its power model (to retrieve
the voltage and frequency). The power model itself is a per-OPP
data-structure. As such, moving it to the OPP library felt like a
natural extension of the framework. That said, if you have any
suggestions of better places to put it, please do let me know -- I'm just
hopping to make the power model available to other clients, but it
doesn't necessarily have to be through the OPP library. In the meantime,
I'll rework the cover letter to better explain the choice of PM OPP.

quoted

Second, looks like we are following ARM "good" practice of fixing
problems of the future. I would only really sign off for this series
when we see real "other future users"

The first reason behind this posting is that I actually have scheduler
patches depending on this infrastructure that I intend to send on the
list in the coming weeks. I would appreciate to keep the two postings
separated because:
   1. I'd like to avoid sending a patch-bomb to the task scheduler
      maintainers that also touches a number of different sub-systems.
      The purpose of those scheduler patches is to introduce some form
      of energy awareness inside the scheduler, and I'm trying to make
      sure this message won't be lost in the middle of too many
      infrastructure patches.
   2. This infrastructure is not tied only to the aforementioned
      scheduler patches as it can also help another user, namely the
      transition to SCMI (which is currently being discussed on the list).

quoted

otherwise we end up with the
infamous static power scenario in 2-3 years down the row. If we
currently do not have users of this IN MAINLINE KERNEL, then the series
is not for upstream.

Again, this patchset is the first step of an upstreaming work, and as such,
and I hope it can make its way to getting merged.

I would rather see a single series showing all users of the new API
instead. If you want to split the series and add a link to the users
of the new API into the series that takes it out of thermal subsystem,
I am also fine, as long as I see the other users. Otherwise, this is a
light nack, reason: no real new users of the new API, even though I
can surely see how the scheduler could use it. But if you do not really
present the code of the new users, this is just speculation.

Presenting the entire code of the new API + all its users can help us to
judge if: (1) a new API is really needed, (2) where it should be place.

Cheers,

Thank you very much,
Quentin