On Wed, 16 Jun 2021 at 19:24, Dietmar Eggemann [off-list ref] wrote:

On 15/06/2021 18:09, Lukasz Luba wrote:

quoted

On 6/15/21 4:31 PM, Dietmar Eggemann wrote:

quoted

On 14/06/2021 21:11, Lukasz Luba wrote:

[...]

quoted

quoted

It's important to highlight that this will only fix this issue between
schedutil and EAS when it's due to `thermal pressure` (today only via
CPU cooling). There are other places which could restrict policy->max
via freq_qos_update_request() and EAS will be unaware of it.

True, but for this I have some other plans.

As long as people are aware of the fact that this was developed to be
beneficial for `EAS - IPA` integration, I'm fine with this.

I don't think it's only for EAS - IPA. Thermal_pressure can be used by
HW throttling like here:
https://lkml.org/lkml/2021/6/8/1791

EAS is involved but not IPA

[...]

quoted

quoted

IMHO, this means that this is catered for the IPA governor then. I'm not
sure if this would be beneficial when another thermal governor is used?

Yes, it will be, the cpufreq_set_cur_state() is called by
thermal exported function:
thermal_cdev_update()
  __thermal_cdev_update()
    thermal_cdev_set_cur_state()
      cdev->ops->set_cur_state(cdev, target)

So it can be called not only by IPA. All governors call it, because
that's the default mechanism.

True, but I'm still not convinced that it is useful outside `EAS - IPA`.

quoted

quoted

The mechanical side of the code would allow for such benefits, I just
don't know if their CPU cooling device + thermal zone setups would cater
for this?

Yes, it's possible. Even for custom vendor governors (modified clones
of IPA)

Let's stick to mainline here ;-) It's complicated enough ...

[...]

quoted

quoted

Maybe shorter?

         struct cpumask *pd_mask = perf_domain_span(pd);
-       unsigned long cpu_cap =
arch_scale_cpu_capacity(cpumask_first(pd_mask));
+       int cpu = cpumask_first(pd_mask);
+       unsigned long cpu_cap = arch_scale_cpu_capacity(cpu);
+       unsigned long _cpu_cap = cpu_cap -
arch_scale_thermal_pressure(cpu);
         unsigned long max_util = 0, sum_util = 0;
-       unsigned long _cpu_cap = cpu_cap;
-       int cpu;
-
-       _cpu_cap -= arch_scale_thermal_pressure(cpumask_first(pd_mask));

Could be, but still, the definitions should be sorted from longest on
top, to shortest at the bottom. I wanted to avoid modifying too many
lines with this simple patch.

Only if there are no dependencies, but here we have already `cpu_cap ->
pd_mask`. OK, not a big deal.

[...]

quoted

quoted

There is IPA specific code in cpufreq_set_cur_state() ->
get_state_freq() which accesses the EM:

     ...
     return cpufreq_cdev->em->table[idx].frequency;
     ...

Has it been discussed that the `per-PD max (allowed) CPU capacity` (1)
could be stored in the EM from there so that code like the EAS wakeup
code (compute_energy()) could retrieve this information from the EM?

No, we haven't think about this approach in these patch sets.
The EM structure given to the cpufreq_cooling device and stored in:
cpufreq_cdev->em should not be modified. There are a few places which
receive the EM, but they all should not touch it. For those clients
it's a read-only data structure.

quoted

And there wouldn't be any need to pass (1) into the EM (like now via
em_cpu_energy()).
This would be signalling within the EM compared to external signalling
via `CPU cooling -> thermal pressure <- EAS wakeup -> EM`.

I see what you mean, but this might cause some issues in the design
(per-cpu scmi cpu perf control). Let's use this EM pointer gently ;)

OK, with the requirement that clients see the EM as ro:

Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>