On Wed, 2020-04-29 at 21:24 +0100, Lukasz Luba wrote: > > On 4/29/20 11:39 AM, Michael Kao wrote: > > On Fri, 2020-04-24 at 10:22 +0100, Lukasz Luba wrote: > >> Hi Michael, > >> > >> On 4/24/20 8:16 AM, Michael Kao wrote: > >>> The upper and lower limits of thermal throttle state in the > >>> device tree do not apply to the power_allocate governor. > >>> Add the upper and lower limits to the power_allocate governor. > >>> > >>> Signed-off-by: Michael Kao <michael.kao@xxxxxxxxxxxx> > >>> --- > >>> drivers/thermal/thermal_core.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > >>> index 9a321dc548c8..f6feed2265bd 100644 > >>> --- a/drivers/thermal/thermal_core.c > >>> +++ b/drivers/thermal/thermal_core.c > >>> @@ -598,7 +598,7 @@ int power_actor_set_power(struct thermal_cooling_device *cdev, > >>> if (ret) > >>> return ret; > >>> > >>> - instance->target = state; > >>> + instance->target = clamp_val(state, instance->lower, instance->upper); > >>> mutex_lock(&cdev->lock); > >>> cdev->updated = false; > >>> mutex_unlock(&cdev->lock); > >>> > >> > >> Thank you for the patch and having to look at it. I have some concerns > >> with this approach. Let's analyze it further. > >> > >> In default the cooling devices in the thermal zone which is used by IPA > >> do not have this 'lower' and 'upper' limits. They are set to > >> THERMAL_NO_LIMIT in DT to give full control to IPA over the states. > >> > >> This the function 'power_actor_set_power' actually translates granted > >> power to the state that device will run for the next period. > >> The IPA algorithm has already split the power budget. > >> Now what happen when the 'lower' value will change the state to a state > >> which consumes more power than was calculated in the IPA alg... It will > >> became unstable. > >> > >> I would rather see a change which uses these 'lower' and 'upper' limits > >> before the IPA do the calculation of the power budget. But this wasn't > >> a requirement and we assumed that IPA has full control over the cooling > >> device (which I described above with this DT THERMAL_NO_LIMIT). > >> > >> Is there a problem with your platform that it has to provide some > >> minimal performance, so you tried to introduce this clamping? > >> > >> Regards, > >> Lukasz > > > > > > Hi Lukasz, > > > > I refer to the documentation settings of the thermal device tree > > (Documentation / devicetree / bindings / thermal / thermal.txt). > > > > It shows that cooling-device is a mandatory property, so max/min cooling > > state should be able to support in framework point of view. > > Otherwise, the limitation should be added in binding document. > > > > Different hardware mechanisms have different heat dissipation > > capabilities. > > Limiting the input heat source can slow down the heat accumulation and > > temperature burst. > > We want to reduce the accumulation of heat at high temperature by > > limiting the minimum gear of thermal throttle. > > I agree that these 'lower' and 'upper' limits shouldn't be just > ignored as is currently. This patch clamps the value at late stage, > though. > > Let me have a look how it could be taken into account in the early > stage, before the power calculation and split are done. Maybe there > is a clean way to inject this. > > Regards, > Lukasz Hi Lukasz, After the research, do you have any ideas or suggestions? Best Regards, Michael
