Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer

[Viresh Kumar]

On 04-02-16, 13:44, Gautham R Shenoy wrote:
> In a a two policy system, to run ondemand on one and conservative on the other,
>  won't the driver have CPUFREQ_HAVE_GOVERNOR_PER_POLICY set?  

No.

CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not about the facility of using
separate governor-type for each policy, that is always available to
the user.

CPUFREQ_HAVE_GOVERNOR_PER_POLICY was initially added for platforms
with different type of CPUs on the same chip, though others can
benefit from it as well.

For example, on a 4 core ARM big LITTLE platform, we will have:
- 2 A7 (low performance/low power)
- 2 A15 (high performance/high power)

The A7's share a policy and A15's share another one.

Without CPUFREQ_HAVE_GOVERNOR_PER_POLICY, if ondemand is selected for
both the policies, the we used to get a single directory (and a set of
tunables) at /sys/devices/system/cpu/cpufreq/ondemand/ .

That used to force us to use same tunables, like sampling rate, etc
for both the policies.

But because the CPUs were so different, we really wanted independent
control.

So, we designed CPUFREQ_HAVE_GOVERNOR_PER_POLICY, so that in such
cases, each policy will have a set of tunables for the same governor
type.

Hope that makes it clear.

If the below questionnaire is still valid, please let me know :)

> If yes, then the changes in this patch won't come into play.
> 
> Also in cpufreq_governor.c, we set cdata->gdbs_data only when 
> !have_governor_per_policy(). cdata->gdbs_data is NULL otherwise.
> A cursory inspection doesn't show any other place in the cpufreq codebase
>  where cdata->gdbs_data is set. Unless I have missed one such initialization, 
> based on my reading of the patch, instead of doing an extra hop to get the 
> governor data via cdata->gdbs_data, we can simply record it in a global
> variable.
> 
> Also, if your concern is regarding the use of show_##file_name##_gov_sys in
> case
> of governor per policy, then the existing code is also broken, since we would
> be 
> accessing _gov##_dbs_cdata.gdbs_data->tuners where _gov##_dbs_cdata.gdbs_data 
> will be NULL!.
> 
> What am I missing ?

-- 
viresh