> -----Original Message----- > From: Lukasz Majewski [mailto:l.majewski@xxxxxxxxxxx] > Sent: Tuesday, October 15, 2013 11:43 PM > To: Zhang, Rui > Cc: Viresh Kumar; Rafael J. Wysocki; Eduardo Valentin; > cpufreq@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxx> > > > Signed-off-by: Myungjoo Ham <myungjoo.ham@xxxxxxxxxxx> > > > > > > --- > > > 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 -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
