Re: [RESEND][PATCH v2 3/4] PM: EM: Add em_dev_update_chip_binning()

[Lukasz Luba]

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@xxxxxxx
> > > >
> > > > 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@xxxxxxx
> > > >
> > > > 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.