Re: [PATCH v3 3/6] cpufreq: Add an interface to mark inefficient frequencies

[Lukasz Luba <lukasz.luba@arm.com>]

On 6/16/21 8:35 AM, Viresh Kumar wrote:
> On 15-06-21, 18:15, Vincent Donnefort wrote:
>> On Tue, Jun 15, 2021 at 03:47:06PM +0530, Viresh Kumar wrote:
>>> The point is that I don't want cpufreq to carry this for users, we
>>> have EM today, tomorrow we may want to mark a frequency as inefficient
>>> from somewhere else. The call need to initiate from EM core.
>>
>> In the current version of this patchset, any driver can mark inefficiencies
>> without relying on the EM, just by adding the flag CPUFREQ_INEFFICIENT_FREQ in
>> cpufreq_frequency_table.
> 
> Yeah, I wasn't really talking about cpufreq drivers but external
> entities, like EM.
> 
>> Populating cpufreq_frequency_table from the EM in cpufreq was just an attempt to
>> a less intrusive set of changes.
>   
>>> And this isn't a cpufreq only thing, but is going to be generic along
>>> with other device types.
>>>
>>> This is exactly why I asked you earlier to play with OPP core for
>>> this. That is the central place for data for all such users.
>>>
>>> If this information is present at the OPP table (somehow), then we can
>>> just fix dev_pm_opp_init_cpufreq_table() to set this for cpufreq core
>>> as well.
>>>
>>> This is the sequence that is followed in cpufreq drivers today:
>>>
>>> dev_pm_opp_of_cpumask_add_table();
>>> dev_pm_opp_init_cpufreq_table();
>>> dev_pm_opp_of_register_em();
>>>
>>> What about changing this to:
>>>
>>> dev_pm_opp_of_cpumask_add_table();
>>>
>>> /* Mark OPPs are inefficient here */
>>> dev_pm_opp_of_register_em();
>>>
>>> /* This should automatically pick the right set */
>>> dev_pm_opp_init_cpufreq_table();
>>>
>>> Will this break anything ?
>>
>> Probably not, but with this approach I'll have to modify all the cpufreq drivers
>> that are registering the EM, which I tried to avoid as much as possible so far.
> 
> Hmm. You are right as well, but I just want to get the right API in
> place which lives a longer life :)
> 
>> But if we sum-up:
>>
>> 1. em_dev_register_perf_domain() find inefficiencies
>>
>> 2. dev_pm_opp_of_register_em() apply EM inefficiencies into the OPP structures
> 
> I was looking to add a new API to the OPP core
> (dev_pm_opp_mark_inefficient()) to mark an OPP inefficient. And then
> get it called from em_create_perf_table().
> 
> But I now see that EM core rather has callbacks to call into and with

Exactly, that's what I was trying to stress.

> that I think you should rather add another callback
> (.mark_inefficient()) in struct em_data_callback, to set inefficient
> frequencies.

I disagree. That's why I prefer Vincent's approach in this patch set.
This proposal would cause more mess.

Vincent proposed a small and clean modification. This modification
can be done easily in this cpufreq place because we have EM in
device cpu struct.

Let's don't over-engineering. The inefficient information is only valid
for schedutil, thus IMHO it can live like this patch set made - in the
cpufreq table.

Compare the v1 (I still don't understand why it was blocked), this v3
and your proposal.