lkml.org 
[lkml]   [2022]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/3] powercap: arm_scmi: Add SCMI Powercap based driver
From


On 9/4/22 16:41, Cristian Marussi wrote:
> On Tue, Aug 30, 2022 at 02:16:42PM +0100, Lukasz Luba wrote:
>> Hi Cristian,
>>
>
> Hi Lukasz,
>

[snip]

>>> +static int scmi_powercap_get_max_power_range_uw(struct powercap_zone *pz,
>>> + u64 *max_power_range_uw)
>>> +{
>>> + *max_power_range_uw = U32_MAX;
>>
>> Shouldn't be calculated based on pai info from the platform FW?
>> e.g.
>> *max_power_range_uw = spz->info->max_power_cap - spz->info->min_power_cap
>>
>> (but with uW conversion in mind if needed)
>>
>
> I double checked this and in include/linux/powercap.h these
> powercap_zone_ops are defined as:
>
> * @get_max_power_range_uw: Get maximum range of power counter in
> * micro-watts.
> * @get_power_uw: Get current power counter in micro-watts.
>
> so these are really data related to average power consumed, i.e. in SCMI
> parlance, info counters I can retrieve for a powercapping domain with
> POWERCAP_MEASUREMENTS_GET, which returns a uint32 representing the
> "average power consumption of the powercapping domain in the last PAI"
>
> It seemed to me that this was unrelated to min/max powercap but more
> something used to report actual powercap domain consumption, so I use
> UINT32_MAX to represent the max range...on the other side in Linux these
> powercap ops may seem more to expect to report a sort of progressive
> accumulated comsuption value while I can only expose the average consumption
> as calculated and reported by fw across the last PAI. (SCMI 4.10.3.10)
>
> Looking again at this, I'm not sure really if this is ok for the powercap
> Linux framework or should I instead try to keep a running accumulated value
> inside this driver (built from the values I get from
> POWERCAP_MEASUREMENTS_GET) and expose that....
>
> ... so thanks for pointing this out because it triggered more doubts :D
> ...any hint about this welcome.

I recalled this code in DTPM [1]. Although, I have checked the
documentation of Powercap sysfs for this file [2]. This 'range'
for power (or energy) describes the values for related: 'power_uw'
or 'energy_uj'. Which means the 'power_uw' value can be actually
lower that setting in 'min_power_cap' (e.g. due to lightly loaded CPU).
I'm not sure for the upper bound: 'max_power_cap'. In real world
we can get a power spike which is bigger than that, so probably
your original U32_MAX is OK.

Therefore, probably the DTPM [1] could be adjusted not your aproach.


[snip]

>>> + for (i = 0, spz = pr->spzones; i < pr->num_zones; i++, spz++) {
>>> + /*
>>> + * Powercap domains are validate by the protocol layer, i.e.
>>> + * when only non-NULL domains are returned here, whose
>>> + * parent_id is assured to point to another valid domain.
>>> + */
>>> + spz->info = powercap_ops->info_get(ph, i);
>>> +
>>> + spz->dev = dev;
>>> + spz->ph = ph;
>>> + spz->spzones = pr->spzones;
>>> + INIT_LIST_HEAD(&spz->node);
>>> + INIT_LIST_HEAD(&pr->registered_zones[i]);
>>> +
>>> + /*
>>> + * Forcibly skip powercap domains using an abstract scale.
>>> + * Note that only leaves domains can be skipped, so this could
>>> + * lead later to a global failure.
>>> + */
>>> + if (!spz->info->powercap_scale_uw &&
>>> + !spz->info->powercap_scale_mw) {
>>> + dev_warn(dev,
>>> + "Abstract power scale not supported. Skip %s.\n",
>>> + spz->info->name);
>>> + spz->info = NULL;
>>> + continue;
>>> + }
>>
>> We can say that the power scale should be consistent in
>> a platform. Then we can bail out when abstract scale has
>> been found. This could also simplify code by a bit.
>>
>
> I do NOT agree on this since I do NOT think from the SCMI spec we can
> assume this semplification: Linux powercap has indeed this limitation on
> scales BUT other non-Linux agents could indeed support abstract scales and
> the SCMI server could advertise a well built hierarchy of powercap domains
> including some abstract scale ones tha, if placed as leaves of the hierarchy,
> could be ignored by Linux but used instead by other agents...or in the future
> used by Linux too ?

This diversity makes me a headache ;) I would hope the SCMI spec would
restrict the span of variety. Although, I cannot find in the spec that
all powercap domains must use the same power scale...

It looks like, you will have to implement it this way.

>
> I'll double check with Archs since I had already an internal exchange on
> this and seemed to me that the current approach (of only bailing out when
> non-leaves abstract scale domains are found) was fine, i.e. that I could
> not just assume to receive only uw/mv scale domains.
>
>> Can we also validate here some those lines, which are
>> checked in many callback funcitons?
>>
>
> Partially yes....see below...
>
>> These are the lines, which could be then removed if we bail
>> out here earlier:
>> if (!spz->info)
>> return -ENODEV;
>
> I can remove this surely from everywhere since I really never register a
> zone with NULL spx->info, this check all-around, my bad, is redundant.
>
>> if (!spz->info->powercap_pai_config)
>> return -EINVAL;
>> if (!spz->info->powercap_monitoring)
>> return -EINVAL;
>>
>
> Instead I cannot see why a powercap domain missing this capabilities
> (PAI configuration and power consumption monitoring) should be
> excluded as a whole...for this reason (if valid from the scale
> perspective as said above) I currently register these powercap SCMI
> zones even if lacking these supports and then return -EINVAL only for
> the related Powercap unsupported callbacks...while still supporting as
> an example setting min/max powercaps.

It's a bit more complicated than I thought. We cannot simplify too much
and make weak assumption. You're right, please keep your approach.



[1]
https://elixir.bootlin.com/linux/latest/source/drivers/powercap/dtpm.c#L54
[2]
https://elixir.bootlin.com/linux/latest/source/Documentation/power/powercap/powercap.rst#L206

\
 
 \ /
  Last update: 2022-09-05 10:48    [W:0.044 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site