lkml.org 
[lkml]   [2013]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v9 3/7] thermal:boost: Automatic enable/disable of BOOST feature
    Date


    > -----Original Message-----
    > From: Lukasz Majewski [mailto:l.majewski@samsung.com]
    > Sent: Tuesday, October 15, 2013 11:43 PM
    > To: Zhang, Rui
    > Cc: Viresh Kumar; Rafael J. Wysocki; Eduardo Valentin;
    > cpufreq@vger.kernel.org; Linux PM list; Jonghwa Lee; Lukasz Majewski;
    > linux-kernel; Bartlomiej Zolnierkiewicz; Myungjoo Ham; R, Durgadoss
    > Subject: Re: [PATCH v9 3/7] thermal:boost: Automatic enable/disable of
    > BOOST feature
    > Importance: High
    >
    > Hi Zhang,
    >
    > > Hi, Lukasz,
    > >
    > > thanks for the patch, sorry that I didn't look into this one earlier.
    >
    > Yes, I would _really_ appreciate _earlier_ feedback from thermal
    > maintainers :-)
    >
    > >
    > > On Mon, 2013-10-14 at 14:17 +0200, Lukasz Majewski wrote:
    > > > This patch provides auto disable/enable operation for boost. When
    > > > any defined trip point is passed, the boost is disabled.
    > >
    > > Do you mean boost is disabled if the system is in a overheating state?
    >
    > In short - Yes.
    >
    >
    > To be more precise - the thermal here is a "safe" valve.
    >
    > Its role is to provide hysteresis similar to the one available for
    > Intel processors.
    >
    > Intel does it in HW. Here I'm trying to do the same with SW for ARM.
    >
    > >
    > > > In that moment thermal monitor workqueue is woken up and it
    > monitors
    > > > if the device temperature drops below 75% of the smallest trip
    > > > point.
    > >
    > > Just FYI, the smallest trip point does not equal the trip point with
    > > lowest temperature value.
    >
    > Thermal processors to which I've looked (exynos 4/5, ste-snowball) had
    > trip points defined monotonically with smallest value defined first.
    >
    > This was the rationale for choosing thermal trip point 0.
    >
    But this is not a hard rule for all thermal drivers, thus you can't make this assumption.

    >
    > >
    > > Say, here is a platform with an active trip point at 40C, and an
    > > critical trip point at 100C, you want to enable boost only if the
    > > temperature is under 30C, right?
    >
    > In short: no (please read below explanation).
    >
    >
    > The boost rough idea:
    > 1. I enable boost from cpufreq (no matter what is the state of thermal)
    > 2. If temperature is too high, then thermal interrupt would trigger and
    > disable boost 3. If device cools down - I enable the boost again
    >
    >
    > >
    > > > When device cools down, the boost is enabled again.
    > > >
    > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
    > > > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
    > > >
    > > > ---
    > > > Changes for v9:
    > > > - None
    > > >
    > > > Changes for v8:
    > > > - Move cpufreq_boost_* stub functions definition (needed when
    > > > cpufreq is not compiled in) to cpufreq.h at cpufreq core support
    > > > commit
    > > >
    > > > Changes for v7:
    > > > - None
    > > >
    > > > Changes for v6:
    > > > - Disable boost only when supported and enabled
    > > > - Protect boost related thermal_zone_device struct fields with
    > mutex
    > > > - Evaluate temperature trend during boost enable decision
    > > > - Create separate methods to handle boost enable/disable
    > > > (thermal_boost_{enable|disable}) operations
    > > > - Boost is disabled at any trip point passage (not only the non
    > > > critical one)
    > > > - Add stub definitions for cpufreq boost functions used when
    > > > CONFIG_CPU_FREQ is NOT defined.
    > > >
    > > > Changes for v5:
    > > > - Move boost disable code from cpu_cooling.c to thermal_core.c
    > > > (to handle_non_critical_trips)
    > > > - Extent struct thermal_zone_device by adding overheated bool flag
    > > > - Implement auto enable of boost after device cools down
    > > > - Introduce boost_polling flag, which indicates if thermal uses
    > it's
    > > > predefined pool delay or has woken up thermal workqueue only to
    > wait
    > > > until device cools down.
    > > >
    > > > Changes for v4:
    > > > - New patch
    > > >
    > > > drivers/thermal/thermal_core.c | 55
    > > > ++++++++++++++++++++++++++++++++++++++++
    > > > include/linux/thermal.h | 2 ++ 2 files changed, 57
    > > > insertions(+)
    > > >
    > > > diff --git a/drivers/thermal/thermal_core.c
    > > > b/drivers/thermal/thermal_core.c index 4962a6a..a167ab9 100644
    > > > --- a/drivers/thermal/thermal_core.c
    > > > +++ b/drivers/thermal/thermal_core.c
    > > > @@ -34,6 +34,7 @@
    > > > #include <linux/thermal.h>
    > > > #include <linux/reboot.h>
    > > > #include <linux/string.h>
    > > > +#include <linux/cpufreq.h>
    > >
    > > Actually, I do not like to see this as thermal_core.c.
    > > Because it is the platform thermal driver that owns the thermal
    > > policy, e.g. it tells the thermal core to take what action at what
    > > temperature.
    > > And this cpufreq boost support should be part of the thermal policy.
    >
    >
    > Boost is defined as policy independent at cpufreq.


    > So I believe that
    > it shall be also thermal policy independent.

    Boost mode support itself is policy independent, but when to use it is kind of a policy, right?
    Say, if you introduce boost support in cpufreq cooling code, either as a cooling device or as a special cooling state, it is thermal policy independent, but when to use this cooling device/state is surely part of thermal policy.

    > In the end thermal shall
    > help cpufreq to not burn the device.
    >
    >
    > >
    > > For example, here is a platform that supports boost. And it has a
    > > passive trip point at 40C, which means the platform driver wants to
    > > reduce the processor frequency when the temperature at 40C.
    > > And what you're trying to add in this patch is to turn on boost mode
    > > when the temperature is under 30C, right?
    >
    > In short: yes.
    >
    > I want to add code which would disable boost when detected temperature
    > is more than 40C.
    >
    > First, boost must be enabled at cpufreq. Only then it can be disabled
    > (if temp > 40C) at thermal.
    >
    > During boost disablement I also setup the thermal zone for
    > polling (if we already poll it - no settings are changed).
    >
    > The boost is re-enabled only when temperature drops to 30C AND the
    > tz->overheated is set (which means that we are at overheated state
    > caused by boost).
    >
    >
    > > If yes, then I'd prefer to
    > > 1. introduce a separate cpu cooling device that just has two cooling
    > > state, 0 means boost mode enabled, and 1 means boost mode disabled.
    > > 2. For any platform thermal driver that wants this support, introduce
    > > a new trip point (30C) to the platform thermal driver,
    > > and bind the
    > > cpufreq boost cooling device to this trip point.
    >
    > >
    > > And IMO, Step 1 can be an enhancement of cpufreq cooling feature. You
    > > just need to introduce two new APIs for registering/unregistering an
    > > cpu boost cooling device, without changing the current cpufreq
    > > cooling code.
    > >
    > > Further more, cpufreq_boost_trigger_state(1) just make it possible to
    > > enter boost mode, it does not mean the cpu will be put into boost
    > mode
    > > immediately, right?
    >
    > Yes, correct.
    >
    > > can we make it transparent to thermal core, say,
    > > always enable it when the cpu is in cooling state 0 (p0)?
    >
    > Thanks for presenting possible solution.
    >
    No problem.

    Thanks,
    rui
    > I will investigate it for boost.
    >
    Thanks,
    rui
    > >
    > > thanks,
    > > rui
    > > > #include <net/netlink.h>
    > > > #include <net/genetlink.h>
    > > >
    > > > @@ -366,9 +367,59 @@ static void handle_critical_trips(struct
    > > > thermal_zone_device *tz, }
    > > > }
    > > >
    > > > +static int thermal_boost_enable(struct thermal_zone_device *tz)
    > > > +{
    > > > + enum thermal_trend trend = get_tz_trend(tz, 0);
    > > > + long trip_temp;
    > > > +
    > > > + if (!tz->ops->get_trip_temp || !tz->overheated)
    > > > + return -EPERM;
    > > > + if (trend == THERMAL_TREND_RAISING || trend ==
    > > > THERMAL_TREND_RAISE_FULL)
    > > > + return -EBUSY;
    > > > +
    > > > + tz->ops->get_trip_temp(tz, 0, &trip_temp);
    > > > + /*
    > > > + * Enable boost again only when current temperature is less
    > > > + * than 75% of trip_temp[0]
    > > > + */
    > > > + if ((tz->temperature + (trip_temp >> 2)) < trip_temp) {
    > > > + mutex_lock(&tz->lock);
    > > > + tz->overheated = false;
    > > > + if (tz->boost_polling) {
    > > > + tz->boost_polling = false;
    > > > + tz->polling_delay = 0;
    > > > + }
    > > > + mutex_unlock(&tz->lock);
    > > > + cpufreq_boost_trigger_state(1);
    > > > + return 0;
    > > > + }
    > > > + return -EBUSY;
    > > > +}
    > > > +
    > > > +static void thermal_boost_disable(struct thermal_zone_device *tz)
    > > > +{
    > > > + cpufreq_boost_trigger_state(0);
    > > > +
    > > > + /*
    > > > + * If no workqueue for monitoring is running - start one
    > > > with
    > > > + * 1000 ms monitoring period
    > > > + * If workqueue already running - do not change its period
    > > > and only
    > > > + * test if target CPU has cooled down
    > > > + */
    > > > + mutex_lock(&tz->lock);
    > > > + if (!tz->polling_delay) {
    > > > + tz->boost_polling = true;
    > > > + tz->polling_delay = 1000;
    > > > + }
    > > > + tz->overheated = true;
    > > > + mutex_unlock(&tz->lock);
    > > > +}
    > > > +
    > > > static void handle_thermal_trip(struct thermal_zone_device *tz,
    > > > int trip) {
    > > > enum thermal_trip_type type;
    > > > + if (cpufreq_boost_supported() && cpufreq_boost_enabled())
    > > > + thermal_boost_disable(tz);
    > > >
    > > > tz->ops->get_trip_type(tz, trip, &type);
    > > >
    > > > @@ -467,6 +518,10 @@ static void thermal_zone_device_check(struct
    > > > work_struct *work) struct thermal_zone_device *tz =
    > > > container_of(work, struct thermal_zone_device,
    > > > poll_queue.work);
    > > > + if (cpufreq_boost_supported())
    > > > + if (!thermal_boost_enable(tz))
    > > > + return;
    > > > +
    > > > thermal_zone_device_update(tz);
    > > > }
    > > >
    > > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
    > > > index b268d3c..b316bdf 100644
    > > > --- a/include/linux/thermal.h
    > > > +++ b/include/linux/thermal.h
    > > > @@ -172,6 +172,8 @@ struct thermal_zone_device {
    > > > int emul_temperature;
    > > > int passive;
    > > > unsigned int forced_passive;
    > > > + bool overheated;
    > > > + bool boost_polling;
    > > > const struct thermal_zone_device_ops *ops;
    > > > const struct thermal_zone_params *tzp;
    > > > struct thermal_governor *governor;
    > >
    > >
    >
    >
    >
    > --
    > Best regards,
    >
    > Lukasz Majewski
    >
    > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group


    \
     
     \ /
      Last update: 2013-10-17 17:21    [W:4.646 / U:0.180 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site