Re: [PATCH v2 1/3] cpufreq:boost: CPU frequency boost code unification for software and hardware solutions

[Lukasz Majewski <l.majewski@samsung.com>]

On Wed, 12 Jun 2013 14:55:45 +0530, Viresh Kumar wrote:
> On 12 June 2013 14:39, Lukasz Majewski <l.majewski@samsung.com> wrote:
> > On Wed, 12 Jun 2013 13:39:18 +0530, Viresh Kumar wrote:
> >> On 12 June 2013 13:09, Lukasz Majewski <l.majewski@samsung.com>
> >> wrote:
> 
> >> You don't have to add an extra "---" line. Just write your
> >> changelog after "---" added by git and give a blank line between
> >> your last changelog line and below ones (probably just to make it
> >> more readable and not a must, but i am not sure).
> >
> > I got your point. If you prefer, I will stick to it. No problem.
> 
> Its not how I prefer it, but how everybody does it :)

Ok, :-)

> 
> > As a side note:
> >
> > In the other open source project (u-boot) we use the pattern which
> > I've used previously. It has one big advantage - I can edit change
> > log at git gui (just below sign-of-by). It is simply more
> > convenient for me :-). Those changes between "---" are simply
> > skipped by git am afterwards.
> 
> Yes, I am still asking you to follow the same steps:
> 
> git send-email --annotate ***patches***
> 
> and then write whatever you don't want to be logged by git am after
> the "---" already present in the patch.

Ok,

> 
> >> >> >  drivers/cpufreq/cpufreq.c    |   69
> >> >> > ++++++++++++++++++++++++++++++++++++++++++
> >> >> > drivers/cpufreq/freq_table.c |   57
> >> >> > ++++++++++++++++++++++++++++++++--
> >> >> > include/linux/cpufreq.h      | 12 ++++++++ 3 files changed,
> >> >> > 136 insertions(+), 2 deletions(-)
> >>
> >> > I think that we shall give users some flexibility and don't
> >> > assume that low_level_boost is only used for one solution/vendor.
> >> >
> >> > It is also needed with software controlled boost. Please refer to
> >> > patch 3/3.
> >>
> >> You didn't get me. I am not asking to keep it only for Intel. But
> >> keep this variable as is (s/low_level_boost/set_boost_freq), and
> >> make it optional. So, few drivers can implement it but not
> >> everybody is required to.
> >
> > The low_level_boost (set_boost_freq)[*] is optional. However it
> > seems to me, that the burden of changing available set of
> > frequencies (when boost is enabled) must be put to cpufreq driver
> > anyway.
> 
> Driver shouldn't play with boost freqs at runtime. This has to be
> handled by cpufreq core. The only thing cpufreq driver must be doing
> in low_level_boost or set_boost_freq (as what I suggested it to be),

So you want the set_boost_freq to not be used with software based
boosting. Ok.

> is to set the boost frequency requested by core. And so this routine
> would only be defined by drivers that have a special way of setting
> boost frequencies. For others ->target() routine should be able to
> set all freqs, boost or non boost.
> 
> > Without this function [*] defined, we cannot enable frequency
> > boosting.
> 
> why?

Hmm, please read my further comment.

> 
> >> So, Add another variable: boost_supported, which will tell cpufreq
> >> core that boost is supported by governor or not.
> >                                   ^^^^^^^shouldn't it be cpufreq
> > driver?
> 
> Yeah, sorry :(

Ok.

> 
> > Ok, boost_supported seems needed. In my opinion it shall be defined
> > at cpufreq_driver structure (since it provides boosting
> > infrastructure anyway).
> 
> that's what I asked.

Ok :-) .

> 
> >> And a global variable in cpufreq.c boost_enabled to track status of
> >> what user has requested.
> >
> > I think, that boost_enable shall be also defined at cpufreq driver
> > (as proposed in the patch).
> 
> Not really. Driver should only care about if it supports boost or not.
> If it is enabled/disabled by sysfs or not should be kept inside core
> in a global variable.

Ok,

> 
> Moreover, if we have 5-6 cpufreq drivers compiled in (for multi-arch
> compiled kernels), we will save memory wasted by this variable if
> it is present in cpufreq_driver.

Ok. I didn't thought about this use case. Agree.

> 
> > Moreover, boost_enable flag is already defined at
> > acpi-cpufreq.c (as static). We will have two flags for the same
> > purpose.
> 
> No, we don't have to. Just expose another API from cpufreq core
> to get status of boost.

Ok.

> 
> > So we want as follows:
> > show_boost = 1 ---> show only frequencies tagged as
> > CPUFREQ_BOOST_FREQ show_boost = 0 ---> show only "normal" (non
> > boost) frequencies
> 
> Yes.

Ok,

> 
> >> You are just talking about showing boost freqs in sysfs. I am
> >> talking about the frequencies that governors will select when
> >> boost is disabled from sysfs. Because we don't skip boost
> >> frequencies in target() routines, we will set them as and when
> >> governor requests them.
> >
> > I think, that it is the main issue here and it shall be cleared out:
> >
> > Frequencies marked as: CPUFREQ_BOOST_FREQ are added permanently to
> > the freq_table.
> > That is the distinction to the original overclocking patch posted
> > with
> >> LAB, where freq_boost structure was modified and boost frequencies
> >> were
> > either valid or invalid.
> 
> Yes.
> 
> > Then we can in SW control boost in two ways:
> > 1. change policy->max value (to the maximal boost frequency) - as
> > it is done now (v3) at Exynos. This is the simple solution (patch
> > 3/3)
> 
> Drivers aren't supposed to set policy->max. It should be taken care
> of by core or freq_table.c file.

This seems the right thing, but it would require some functions defined
at freq_table.c to be extended to accept bool "boost" parameter (which
would not consider freqs tagged as CPUFREQ_BOOST_FREQ).
Then each per cpu policy, shall be read and its max freq shall be
updated when someone enables boost from sysfs (at
cpufreq_boost_trigger_state).

This is my idea to manage boost mode from core code. Am I right?



> 
> > 2. Modify all freq_table helper functions to be aware of boost and
> > skip boost frequencies when boost_enable = 0. (as it was done at
> > v2). This requires code modification at freq_table.c and
> > reevaluation of policy.
> 
> Yes, the core must be aware of it and must take the right decision
> here. So, we need to take care of boost freq in freq_table.c

Ok. Then we agreed :-).

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group