RE: [PATCH v5 5/7] thermal:boost: Automatic enable/disable of BOOSTfeature

[R, Durgadoss]

Hi Lukasz,

> -----Original Message-----
> From: Lukasz Majewski [mailto:l.majewski@xxxxxxxxx]
> Sent: Friday, July 05, 2013 2:28 AM
> To: R, Durgadoss
> Cc: Lukasz Majewski; Viresh Kumar; Rafael J. Wysocki; Zhang, Rui; Eduardo
> Valentin; cpufreq@xxxxxxxxxxxxxxx; Linux PM list; Jonghwa Lee; linux-kernel;
> Andre Przywara; Daniel Lezcano; Kukjin Kim; Myungjoo Ham
> Subject: Re: [PATCH v5 5/7] thermal:boost: Automatic enable/disable of BOOST
> feature
> 
> On Thu, 4 Jul 2013 17:19:04 +0000
> "R, Durgadoss" <durgadoss.r@xxxxxxxxx> wrote:
> Hi,
> 

[Cut.]

> > > @@ -326,6 +327,15 @@ static void monitor_thermal_zone(struct
> > > thermal_zone_device *tz)
> > >  static void handle_non_critical_trips(struct thermal_zone_device
> > > *tz, int trip, enum thermal_trip_type trip_type)
> > >  {
> > > +	if (cpufreq_boost_supported()) {
> > > +		tz->overheated = true;
> > > +		cpufreq_boost_trigger_state(0);
> > > +		if (!tz->polling_delay) {
> > > +			tz->boost_polling = true;
> > > +			tz->polling_delay = 1000;
> > > +		}
> > > +	}
> > > +
> > >  	if (tz->governor)
> > >  		tz->governor->throttle(tz, trip);
> > >  }
> > > @@ -453,6 +463,27 @@ 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);
> > > +	long trip_temp;
> > > +
> > > +	if (cpufreq_boost_supported() && tz->overheated) {
> >
> > Not all thermal drivers support trip points. So, we first need a
> > if (tz->ops->get_trip_temp) check here.
> 
> Ok, thanks for tip. Bluntly speaking, I thought, that all SoCs supported
> by thermal set trip points.

We would wish to get there. But not the reality today ;)

> 
> >
> > > +		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) {
> >
> > Another way would be to use the get_trend APIs for this thermal zone.
> > If the trend is cooling we can re-enable boost otherwise not.
> 
> Trend evaluation seems like a good complementary idea.
> 
> However, I would also like to have the relative temperature drop
> measurement (if possible) like above (to 75% of the first trip point).
> 
> Then I would be more confident, that everything cooled down and that I
> can start boost again.
> 
> >
> > > +			tz->overheated = false;
> > > +			if (tz->boost_polling) {
> > > +				tz->boost_polling = false;
> > > +				tz->polling_delay = 0;
> > > +				monitor_thermal_zone(tz);
> > > +			}
> >
> > Overall, I believe this will work well only if the thermal zone is
> > CPU.
> 
> My assumption:
> 
> When I enable boost at CPU, then I also shall cool down the CPU. And
> the CPU zone seemed a natural choice.
> 
> However I might be missing something, so hints are welcome.
> 
> >
> > Another suggestion is: We tried hard to remove all throttling logic
> > from thermal_core.c.
> 
> By throttling logic you mean:
> if ((tz->temperature + (trip_temp >> 2)) and other conditions (like
> trend measurement)?

Yes. That is correct.

> 
> > May be we should include this kind of logic in
> > step_wise.c ?
> 
> It sounds interesting (since ->throttle at thermal_core.c is called
> always when needed), but I'm afraid of a code duplication when one
> use Boost with fair_share or other thermal governor.

right. So, for the time being, (as part of this patch series)
I am Okay to have this code in thermal_core.c. From the thermal
subsystem perspective, we will (need to) work out a better/
cleaner/easier approach for this later.

Thanks,
Durga

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/