| Message ID | 20240322110850.77086-1-lukasz.luba@arm.com |
|---|---|
| Headers | show |
| Series | Update Energy Model after chip binning adjusted voltages | expand |
On 3/26/24 10:09, Dietmar Eggemann wrote: > On 22/03/2024 12:08, Lukasz Luba wrote: > >> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c >> index 6960dd7393b2d..f7f7ae34ec552 100644 >> --- a/kernel/power/energy_model.c >> +++ b/kernel/power/energy_model.c >> @@ -808,3 +808,54 @@ static void em_update_workfn(struct work_struct *work) >> { >> em_check_capacity_update(); >> } >> + >> +/** >> + * em_dev_update_chip_binning() - Update Energy Model with new values after > > s/with new values// ... IMHO this should be obvious ? Make sense > >> + * the new voltage information is present in the OPPs. >> + * @dev : Device for which the Energy Model has to be updated. >> + * >> + * This function allows to update easily the EM with new values available in >> + * the OPP framework and DT. It can be used after the chip has been properly >> + * verified by device drivers and the voltages adjusted for the 'chip binning'. >> + * It uses the "dynamic-power-coefficient" DT property to calculate the power >> + * values for EM. For power calculation it uses the new adjusted voltage >> + * values known for OPPs, which might be changed after boot. > > The last two sentences describe what dev_pm_opp_calc_power() is doing. > Maybe this can be made clearer here? Or I can just remove this, since it's too detailed description. > >> + */ >> +int em_dev_update_chip_binning(struct device *dev) > > This is the old dev_pm_opp_of_update_em() right? Yes, it is similar. > >> +{ >> + struct em_perf_table __rcu *em_table; >> + struct em_perf_domain *pd; >> + int i, ret; >> + >> + if (IS_ERR_OR_NULL(dev)) >> + return -EINVAL; > > When do you use if '(IS_ERR_OR_NULL(dev))' and when 'if(!dev)' for EM > interface functions? Sometimes IS_ERR_OR_NULL is used, especially for API function other that register function. > >> + pd = em_pd_get(dev); >> + if (!pd) { >> + dev_warn(dev, "Couldn't find Energy Model\n"); >> + return -EINVAL; >> + } >> + >> + em_table = em_table_dup(pd); >> + if (!em_table) { >> + dev_warn(dev, "EM: allocation failed\n"); >> + return -ENOMEM; >> + } >> + >> + /* Update power values which might change due to new voltage in OPPs */ >> + for (i = 0; i < pd->nr_perf_states; i++) { >> + unsigned long freq = em_table->state[i].frequency; >> + unsigned long power; >> + >> + ret = dev_pm_opp_calc_power(dev, &power, &freq); >> + if (ret) { >> + em_table_free(em_table); >> + return ret; >> + } >> + >> + em_table->state[i].power = power; >> + } >> + >> + return em_recalc_and_update(dev, pd, em_table); >> +} >> +EXPORT_SYMBOL_GPL(em_dev_update_chip_binning); > > In the previous version of 'chip-binning' you were using the new EM > interface em_dev_compute_costs() (1) which is now replaced by > em_recalc_and_update() -> em_compute_costs(). > > https://lkml.kernel.org/r/20231220110339.1065505-2-lukasz.luba@arm.com > > Which leaves (1) still unused. > > That was why my concern back then that we shouldn't introduce EM > interfaces without a user: > > https://lkml.kernel.org/r/8fc499cf-fca1-4465-bff7-a93dfd36f3c8@arm.com > > What happens now with em_dev_compute_costs()? > For now it's not used, but modules which will create new EMs with custom power values will use it. When such a module have e.g. 5 EMs for one PD and only switches on one of them, then this em_dev_compute_costs() will be used at setup for those 5 EMs. Later it won't be used. I don't wanted to combine the registration of new EM with the compute cost, because that will create overhead in the switching to new EM code path. Now we have only ~3us, which was the main goal. When our scmi-cpufreq get the support for EM update this compute cost will be used there.
On 26/03/2024 21:32, Lukasz Luba wrote: > > > On 3/26/24 10:09, Dietmar Eggemann wrote: >> On 22/03/2024 12:08, Lukasz Luba wrote: [...] >>> + return em_recalc_and_update(dev, pd, em_table); >>> +} >>> +EXPORT_SYMBOL_GPL(em_dev_update_chip_binning); >> >> In the previous version of 'chip-binning' you were using the new EM >> interface em_dev_compute_costs() (1) which is now replaced by >> em_recalc_and_update() -> em_compute_costs(). >> >> https://lkml.kernel.org/r/20231220110339.1065505-2-lukasz.luba@arm.com >> >> Which leaves (1) still unused. >> >> That was why my concern back then that we shouldn't introduce EM >> interfaces without a user: >> >> https://lkml.kernel.org/r/8fc499cf-fca1-4465-bff7-a93dfd36f3c8@arm.com >> >> What happens now with em_dev_compute_costs()? >> > > For now it's not used, but modules which will create new EMs > with custom power values will use it. When such a module have > e.g. 5 EMs for one PD and only switches on one of them, then > this em_dev_compute_costs() will be used at setup for those > 5 EMs. Later it won't be used. > I don't wanted to combine the registration of new EM with > the compute cost, because that will create overhead in the > switching to new EM code path. Now we have only ~3us, which > was the main goal. > > When our scmi-cpufreq get the support for EM update this > compute cost will be used there. OK, I see. I checked the reloadable EM test module and em_dev_compute_costs() is used there like you described it.
On 27/03/2024 13:55, Dietmar Eggemann wrote: > On 26/03/2024 21:32, Lukasz Luba wrote: >> >> >> On 3/26/24 10:09, Dietmar Eggemann wrote: >>> On 22/03/2024 12:08, Lukasz Luba wrote: > > [...] > >>>> + return em_recalc_and_update(dev, pd, em_table); >>>> +} >>>> +EXPORT_SYMBOL_GPL(em_dev_update_chip_binning); >>> >>> In the previous version of 'chip-binning' you were using the new EM >>> interface em_dev_compute_costs() (1) which is now replaced by >>> em_recalc_and_update() -> em_compute_costs(). >>> >>> https://lkml.kernel.org/r/20231220110339.1065505-2-lukasz.luba@arm.com >>> >>> Which leaves (1) still unused. >>> >>> That was why my concern back then that we shouldn't introduce EM >>> interfaces without a user: >>> >>> https://lkml.kernel.org/r/8fc499cf-fca1-4465-bff7-a93dfd36f3c8@arm.com >>> >>> What happens now with em_dev_compute_costs()? >>> >> >> For now it's not used, but modules which will create new EMs >> with custom power values will use it. When such a module have >> e.g. 5 EMs for one PD and only switches on one of them, then >> this em_dev_compute_costs() will be used at setup for those >> 5 EMs. Later it won't be used. >> I don't wanted to combine the registration of new EM with >> the compute cost, because that will create overhead in the >> switching to new EM code path. Now we have only ~3us, which >> was the main goal. >> >> When our scmi-cpufreq get the support for EM update this >> compute cost will be used there. > > OK, I see. I checked the reloadable EM test module and > em_dev_compute_costs() is used there like you described it. I had a second look and IMHO the layout is like this: Internal (1) and external (2,3) 'reloadable EM' use cases: (EM alloc and free not depicted) 1. Late CPUs booting (CPU capacity adjustment) e3f1164fc9ee PM: EM: Support late CPUs booting and capacity adjustment schedule_delayed_work(&em_update_work, ...) em_update_workfn() em_check_capacity_update() em_adjust_new_capacity() em_recalc_and_update() <-- (i) em_compute_costs() <-- (ii) em_dev_update_perf_domain() <-- external 2. Reload EM from driver 22ea02848c07 PM: EM: Add em_dev_compute_costs() 977230d5d503 PM: EM: Introduce em_dev_update_perf_domain() for EM updates em_dev_compute_costs() <-- external em_compute_costs() <-- (ii) em_dev_update_perf_domain() <-- external 3. Chip binning this patchset PM: EM: Add em_dev_update_chip_binning() em_dev_update_chip_binning() <-- external em_recalc_and_update() <-- (i) em_compute_costs() <-- (ii) em_dev_update_perf_domain() <-- external
On 3/28/24 07:21, Dietmar Eggemann wrote: > On 27/03/2024 13:55, Dietmar Eggemann wrote: >> On 26/03/2024 21:32, Lukasz Luba wrote: >>> >>> >>> On 3/26/24 10:09, Dietmar Eggemann wrote: >>>> On 22/03/2024 12:08, Lukasz Luba wrote: >> >> [...] >> >>>>> + return em_recalc_and_update(dev, pd, em_table); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(em_dev_update_chip_binning); >>>> >>>> In the previous version of 'chip-binning' you were using the new EM >>>> interface em_dev_compute_costs() (1) which is now replaced by >>>> em_recalc_and_update() -> em_compute_costs(). >>>> >>>> https://lkml.kernel.org/r/20231220110339.1065505-2-lukasz.luba@arm.com >>>> >>>> Which leaves (1) still unused. >>>> >>>> That was why my concern back then that we shouldn't introduce EM >>>> interfaces without a user: >>>> >>>> https://lkml.kernel.org/r/8fc499cf-fca1-4465-bff7-a93dfd36f3c8@arm.com >>>> >>>> What happens now with em_dev_compute_costs()? >>>> >>> >>> For now it's not used, but modules which will create new EMs >>> with custom power values will use it. When such a module have >>> e.g. 5 EMs for one PD and only switches on one of them, then >>> this em_dev_compute_costs() will be used at setup for those >>> 5 EMs. Later it won't be used. >>> I don't wanted to combine the registration of new EM with >>> the compute cost, because that will create overhead in the >>> switching to new EM code path. Now we have only ~3us, which >>> was the main goal. >>> >>> When our scmi-cpufreq get the support for EM update this >>> compute cost will be used there. >> >> OK, I see. I checked the reloadable EM test module and >> em_dev_compute_costs() is used there like you described it. > > I had a second look and IMHO the layout is like this: > > Internal (1) and external (2,3) 'reloadable EM' use cases: > (EM alloc and free not depicted) > > 1. Late CPUs booting (CPU capacity adjustment) > > e3f1164fc9ee PM: EM: Support late CPUs booting and capacity adjustment > > schedule_delayed_work(&em_update_work, ...) > > em_update_workfn() > em_check_capacity_update() > em_adjust_new_capacity() > em_recalc_and_update() <-- (i) > em_compute_costs() <-- (ii) > em_dev_update_perf_domain() <-- external > > 2. Reload EM from driver > > 22ea02848c07 PM: EM: Add em_dev_compute_costs() > 977230d5d503 PM: EM: Introduce em_dev_update_perf_domain() for EM > updates > > em_dev_compute_costs() <-- external > em_compute_costs() <-- (ii) > em_dev_update_perf_domain() <-- external > > 3. Chip binning > > this patchset PM: EM: Add em_dev_update_chip_binning() > > em_dev_update_chip_binning() <-- external > em_recalc_and_update() <-- (i) > em_compute_costs() <-- (ii) > em_dev_update_perf_domain() <-- external > > > > Yes, that's correct.