Message-id: <20130607152718.4b458087@amdc308.digital.local>
Date:	Fri, 07 Jun 2013 15:27:18 +0200
From:	Lukasz Majewski <l.majewski@...sung.com>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	"Rafael J. Wysocky" <rjw@...k.pl>,
	"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
	Linux PM list <linux-pm@...r.kernel.org>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Jonghwa Lee <jonghwa3.lee@...sung.com>,
	Myungjoo Ham <myungjoo.ham@...sung.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Lukasz Majewski <l.majewski@...ess.pl>,
	Andre Przywara <andre.przywara@...aro.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Lists linaro-kernel <linaro-kernel@...ts.linaro.org>
Subject: Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU
 frequency boost

Hi Viresh,

> On 6 June 2013 17:19, Lukasz Majewski <l.majewski@...sung.com> wrote:
> >> On 6 June 2013 12:37, Lukasz Majewski <l.majewski@...sung.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.

I would prefer to have following fields in the cpufreq_boost structure:
struct cpufreq_boost {
	unsigned int	max_boost_freq; /*boost max freq*/
	unsigned int 	max_normal_freq; /*max normal freq
	int (*low_level_boost) (int state);
	bool boost_en;  /* indicate if boost is enabled */
}

The max_{boost|normal}_freq fields will be filed at 
ret = cpufreq_driver->init(policy);

Thanks to them I will avoid calling many times routine, which extracts
from freq_table maximal boost and normal frequencies.

I could define those variables in the exynos-cpufreq.c driver, but I
think, that they are more suitable to be embedded at cpufreq_boost
structure.


> 
> > *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.



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists