Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance

[Viresh Kumar]

On 27-08-20, 12:27, Ionela Voinescu wrote:
> I don't see it as anyone registering for freq invariance, rather the
> freq invariance framework chooses its source of information (AMU, CPPC,
> cpufreq).

Yeah, either way is fine for me.

> > i.e. if CPPC registers for it first then there is no need to check
> > AMUs further (as CPPC will be using AMUs anyway), else we will
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Not necessarily. Even if AMUs are present, they are only used for CPPC's
> delivered and reference performance counters if the ACPI _CPC entry
> specifies FFH as method:
> 
>   ResourceTemplate(){Register(FFixedHW, 0x40, 0, 1, 0x4)},
>   ResourceTemplate(){Register(FFixedHW, 0x40, 0, 0, 0x4)},

Right.

> While I understand your point (accessing AMUs through CPPC's read
> functions to implement invariance) I don't think it's worth tying the
> two together.
> 
> I see the two functionalities as independent:
>  - frequency invariance with whichever source of information is valid
>    (AMUs, cpufreq, etc) is separate from
>  - CPPC's delivered and reference performance counters, which currently
>    are used in cpufreq's .get() function.
> 
> Therefore, taking each of the scenarios one by one:
>  - All CPUs support AMUs: the freq invariance initialisation code will
>    find AMUs valid and it will use them to set the scale factor;
>    completely independently, if the FFH method is specified for CPPC's
>    delivered and reference performance counters, it will also use
>    AMUs, even if, let's say, invariance is disabled.
> 
>  - None of the CPUs support AMUs, but the _CPC entry specifies some
>    platform specific counters for delivered and reference performance.
>    With the current mainline code neither cpufreq or counter based
>    invariance is supported, but the CPPC counters can be used in the
>    cppc_cpufreq driver for the .get() function.
> 
>    But with the above new functionality we can detect that AMUs are not
>    supported and expose the CPPC counters to replace them in
>    implementing invariance.
> 
>  - Mixed scenarios are also supported if we play our cards right and
>    implement the above per-cpu.
> 
> 
> I'm thinking that having some well defined invariance sources might work
> well: it will simplify the init function (go through all registered
> sources and choose (per-cpu) the one that's valid) and allow for
> otherwise generic invariance support. Something like:
> 
> enum freq_inv_source {INV_CPUFREQ, INV_AMU_COUNTERS, INV_CPPC_COUNTERS};
> 
> struct freq_inv_source {
> 	enum freq_inv_source source;
> 	bool (*valid)(int cpu);
> 	u64 (*read_corecnt)(int cpu);
> 	u64 (*read_constcnt)(int cpu);
> 	u64 (*max_rate)(int cpu);
> 	u64 (*ref_rate)(int cpu);
> }
> 
> I am in the middle of unifying AMU counter and cpufreq invariance through
> something like this, so if you like the idea and you don't think I'm
> stepping too much on your toes with this, I can consider the usecase in
> my (what should be) generic support. So in the end this might end up
> being just a matter of adding a new invariance source (CPPC counters).

Okay, if you have already started working on that, no issues from my
side. I can just get the relevant stuff from CPPC added once you
provide that layer..

> My only worry is that while I know how a cpufreq source behaves and how
> AMU counters behave, I'm not entirely sure what to expect from CPPC

Neither do I :)

> counters: if they are always appropriate for updates on the tick (not
> blocking),

The update stuff may sleep here and so I had to do stuff in the
irq-work handler in my patch.

> if they both stop during idle, if there is save/restore
> functionality before/after idle, etc.

This I will check.

-- 
viresh