Re: [PATCH v3 1/3] cpufreq: Add boost frequency support in core

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

On Mon, 17 Jun 2013 09:43:27 +0200, Viresh Kumar wrote:
> On 17 June 2013 12:45, Lukasz Majewski <l.majewski@samsung.com> wrote:
> > On Mon, 17 Jun 2013 11:13:18 +0530, Viresh Kumar wrote:
> >> On 14 June 2013 13:08, Lukasz Majewski <l.majewski@samsung.com>
> >> wrote:
> 
> >> > +int cpufreq_boost_trigger_state(int state) +{
> 
> >> > +       if (!cpufreq_driver->boost_supported)
> >> > +               return -ENODEV;
> >>
> >> This can't happen. sysfs directory is present only when we
> >> have boost supported.
> >
> > I know, that we shall not look into the future. But this method
> > will be used when somebody would like to enable boost from kernel.
> > Let's say new governor or such :-).
> 
> We don't know if that would be acceptable or not as of now.
> 
> > I can remove this and add it later, but I think, that it is safer to
> > add it straightaway.
> 
> Remove it for now.

Ok.

> 
> >>
> >> > +       if (cpufreq_boost_enabled != state) {
> >> > +               if (cpufreq_driver->set_boost_freq) {
> >> > +                       ret =
> >> > cpufreq_driver->set_boost_freq(state);
> >>
> >> I thought this routine was for setting boost frequency and not
> >> for enabling boost feature. If boost has to be enabled separately
> >> then this routine must take care of it while setting freq.
> >>
> >> So, in other words, this must not be called here.
> >
> > This function explicitly follows current logic of acpi-cpufreq.c.
> >
> > I would like to avoid modifying legacy/working code as much as
> > possible (especially when I hadn't yet received any feedback about
> > acpi-cpufreq.c file changes).
> 
> Hmm.. I saw that code now. You are talking about: boost_set_msrs ??
> 
> So, this function has nothing to do with set_boost_freq() but
> enable_boost(). Rename your function similarly and keep this code.

Ok.

> 
> >> > +                       if (ret) {
> >> > +                               pr_err("%s: BOOST cannot enable
> >> > (%d)\n",
> >> > +                                      __func__, ret);
> >> > +                               return ret;
> >> > +                       }
> >> > +               }
> >> > +
> >> > +               for_each_possible_cpu(cpu) {
> >> > +                       policy = cpufreq_cpu_get(cpu);
> >>
> >> In case this code is required, it would make more sense to keep
> >> list of all available policies, which we can iterate through.
> >
> > Maybe I don't get the big picture, but why we cannot iterate
> > through possible CPUs? At least one shall have valid policy and
> > freq_table combination.
> 
> Many cpus share same policy structure and so iterating over all of
> them isn't a very good idea. Either keep a mask of cpus already
> iterated, copy policy->cpus to it on each iteration. If a cpu is
> already done skip loop for it.

This is the situation, which I wanted to avoid. Policy has the
policy->cpus mask already implemented, but we don't know how/where to
hook to them (one solution would be to start from cpu0, but for some
reason I'm reluctant to do it in this way).

> 
> OR keep a list of policies. I would prefer the later (do it in a
> separate patch), as this can be used later too.

I will declare a boost_policy list at cpufreq.c. Then I will add policy
to it, when cpufreq_add_dev() is called.

> 
> > I've already proposed list of all policies (at v1), but we decided
> > to abandon this idea.
> 
> I don't remember why it was there but reason wasn't good enough to
> keep it.

:-). Lets try with list.

> 
> >> I am not sure if this is required at all. Or what complications
> >> can be there when we update max/min on the fly here.
> >
> > Correct me if I'm wrong, but I'm using scripts for tests which run
> > simultaneously and enables/disables boost feature. I don't recall if
> > there is a lock at sysfs, which would prevent from simultaneous
> > write to the "boost" sysfs attribute.
> >
> > I will doubble check that.
> 
> It isn't about a crash but how cpufreq subsystem works. I will think
> over it later too, for now you can keep it.

OK.

> 
> >> > +{
> >> > +       return cpufreq_boost_enabled;
> >>
> >> s/cpufreq_boost_enabled/boost_enabled
> 
> ??

I will change the name.

> 
> >> > @@ -318,6 +322,10 @@ __ATTR(_name, _perm, show_##_name, NULL)
> >> >  static struct freq_attr _name =                        \
> >> >  __ATTR(_name, 0644, show_##_name, store_##_name)
> >> >
> >> > +#define cpufreq_attr_available_freq(_name)       \
> >> > +struct freq_attr cpufreq_freq_attr_##_name##_freqs =     \
> >> > +__ATTR_RO(_name##_frequencies)
> >>
> >> What is this doing in cpufreq.h? It will only be used by
> >> freq_table.c file.
> >
> > I wanted to add those #defines to the place where other similar ones
> > are placed.
> 
> That's not how these or any other declaration should be placed. By
> adding these to cpufreq.h, you are inviting other parts of kernel to
> use them. Which we don't want.
> 
> > I can put it to freq_table.c.
> 
> Thanks.

Ok. I will keep the code local.

> 
> >> Again, I couldn't see how boost freqs are getting used? You have
> >> probably made them part of normal freq range here and so they
> >> might be used during normal operations. But we wanted it to be
> >> used only when we have few cpus on... etc.. Where is that logic?
> >
> > The logic is as follow:
> > - cpufreq_driver exports .attr field. When driver supports boost
> > then two attributes are exported (even when boost is not enabled,
> > but supported):
> >         - scaling_available_frequencies (only normal frequencies -
> > this attribute is unchanged)
> >         - scaling_boost_frequencies - list possible boost
> > frequencies.
> >
> > When boost is not supported - then only
> > scaling_available_frequencies is exported (as it is done now).
> 
> You are missing the whole point behind keeping this patchset. Its not
> about how to expose boost freqs to sysfs (that's what you are talking
> about) but to use these frequencies on the fly. 

Two separate things:

1. I think, that scaling_available_frequencies [*] attribute has to
   stay unchanged (this is how userspace sees the API). Linus would
   we very unhappy :-) if we had changed public API.

   x86 guys, please correct me, but I think that [*] will be not
   changed (expanded) when Intel's HW boost is enabled at acpi-cpufreq.c
   code.

   The scaling_boost_frequencies is only visible when driver (and
   probably soc maintainer) is ready to handle boosting. It shows over
   clocked frequencies.

2. How we would control boost?
   - To enable this you must mark IDs for some freqs as
     CPUFREQ_BOOST_FREQ.
   - The cpufreq_boost.boost_supported flag needs to be true (set at
     cpufreq driver code)
   - One needs to enable boosting by sysfs or call
     cpufreq_boost_trigger_state(1); The latter will not work when
     cpufreq_driver->boost_supported is not set.

I assume that when somebody takes those three above steps, then he is
aware that boost is working on his machine.

How one can control the boost? I'm now (on my setup) using thermal
subsystem. I set proper trip points and when one of them is met, then
boost is disabled. Moreover the thermal governor (stepwise) also
reduces frequency.

It works stable with v3.10 (with 3.8 there were some bugs - now they
are fixed).


The core acpi-cpufreq.c code hadn't been changed by me, so I assume
that it will work as before.

> Some part of kernel
> code would be setting these frequencies in real hardware. Who will
> set these frequencies? 

Those freqs are set by ->target() callback (when allowed)

> On what basis? How do we ensure we don't
> burn your chip?

Thermal subsystem is a good example here. Another may be a
governor, or even scheduler.


-- 
Best regards,

Lukasz Majewski

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