Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost

[Viresh Kumar <viresh.kumar@linaro.org>]

On 6 June 2013 17:19, Lukasz Majewski <l.majewski@samsung.com> wrote:
>> On 6 June 2013 12:37, Lukasz Majewski <l.majewski@samsung.com> wrote:

>> > cpufreq_driver->boost->attr))
>>
>> Why is this required? Why do we want platforms to add some files
>> in sysfs?
>
> There are two kinds of attributes, which are exported by boost:
>
> 1. global boost (/sys/devices/system/cpu/cpufreq/boost)
>
> 2. attributes describing cpufreq abilities when boost is available
> (/sys/devices/syste/cpu/cpu0/cpufreq/):
>         - scaling_boost_frequencies - which will show over clocked
>           frequencies
>         - the scaling_available_frequencies will also display boosted
>           frequency (when boost enabled)
>
> Information from 2. is cpufreq_driver dependent. And that information
> shouldn't been displayed when boost is not available

This is not how I wanted this to be coded. Lets keep things simple:
- Implement it in the way cpufreq_freq_attr_scaling_available_freqs
  is implemented. And then drivers which need to see boost freqs
  can add it in their attr.

>> > +       policy->boost = cpufreq_boost = cpufreq_driver->boost;
>>
>> Why are we copying same pointer to policy->boost? Driver is
>> passing pointer to a single memory location, just save it globally.
>
> This needs some explanation.
>
> The sysfs entry presented at [1] doesn't bring any useful information
> to reuse (like *policy). For this reason the global cpufreq_boost
> pointer is needed.
>
> However to efficiently manage the boost, it is necessary to keep per
> policy pointer to the only struct cpufreq_boost instance (defined at
> cpufreq_driver code).

No we don't need to screw struct cpufreq_policy with it.
Just need two variables here:
- cpufreq_driver->boost: If driver supports boost or not.
- If above is true then a global bool variable that will say boost is
  enabled from sysfs or not.

>> > +       /* disable boost for newly created policy - as we e.g.
>> > change
>> > +          governor */
>> > +       policy->boost->status = CPUFREQ_BOOST_DIS;
>>
>> Drivers supporting boost may want boost to be enabled by default,
>> maybe without any sysfs calls.
>
> This can be done by setting the struct cpufreq_boost status field to
> CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is
> defined)

This really isn't driver dependent.. But user dependent. Maybe lets
keep it disabled and people can enable it from sysfs.

>> Why do we need to maintain a list of boost here? notifiers? complex :(
>
> Notifier is needed to disable boost when policy is changed (for
> example when we change from ondemand/lab to performance governor).
>
> I wanted to avoid the situation when boost stays enabled across
> different governors.
>
> The list of in system available policies is defined to allow boost
> enable/disable for all policies available (by changing for example
> policy->max field).
>
> If we decide, that we will support only one policy (as it is now at
> e.g. Exynos), the list is unnecessary here.

What we discussed last in your earlier patchset was:
- Keep boost feature separate from governors.
- If it is enabled, then any governor can use it (if they want).
- Lets not disable it if governor is changed. user must do it explicitly.

>> > +static int cpufreq_frequency_table_skip_boost(struct
>> > cpufreq_policy *policy,
>> > +                                             unsigned int index)
>> > +{
>> > +       if (index == CPUFREQ_BOOST)
>> > +               if (!policy->boost ||
>>
>> This shouldn't be true. If index has got CPUFREQ_BOOST, then driver
>> has to support boost.
>
> Correct me if I'm wrong here, but in my understanding the boost shall be
> only supported when both CPUFREQ_BOOST index is defined in a freq_table
> and boost.state = CPUFREQ_BOOST_EN is set.
>
> Setting of CPUFREQ_BOOST shouldn't by default allow to use over
> clocking frequency.

For cpufreq core boost is enabled as soon as cpufreq_driver->boost is
true. We can have additional checks to see if there is atleast one
boost frequency but can skip this too.

>> why do we need this?
>
> To evaluate the maximal boost frequency from the frequency table. It is
> then used as a delimiter (when LAB cooperates with thermal framework).

Introduce this with LAB then.. Lets keep it as simple as possible for now.
One step at a time.

>> > +struct cpufreq_boost {
>> > +       unsigned int            max_boost_freq; /* maximum value of
>> > +                                                * boosted freq */
>> > +       unsigned int            max_normal_freq; /* non boost max
>> > freq */
>> > +       int                     status; /* status of boost */
>> > +
>> > +       /* boost sysfs attributies */
>> > +       struct freq_attr        **attr;
>> > +
>> > +       /* low-level trigger for boost */
>> > +       int (*low_level_boost) (struct cpufreq_policy *policy, int
>> > state); +
>> > +       struct list_head        policies;
>> > +};
>>
>> We don't need it. Just add two more fields to cpufreq_driver:
>> - have_boost_freqs and low_level_boost (maybe a better name.
>> What's its use?)
>
> The separate struct cpufreq_boost was created to explicitly separate
> boost fields from cpufreq_driver structure.
>
> If in your opinion this structure is redundant, I can remove it and
> extend cpufreq_driver structure.

I am not against a structure (as putting related stuff in a struct is always
better), but against so many fields in it to make things complicated.

I will only keep have_boost_freqs and low_level_boost. Remove
everything else.

> *boost pointer is necessary when one wants to enable/disable boost from
> e.g governor code (which operates mostly on struct cpufreq_policy
> *policy pointers).

We don't need to do this. boost can only be disabled from userspace by
user. No intervention from governor.