| Message ID | 20200424071601.2636-1-michael.kao@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | thermal: power_allocate: add upper and lower limits | expand |
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@mediatek.com> > >>> --- > >>> 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
Hi Michael, On 8/25/20 10:29 AM, Michael Kao wrote: > 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@mediatek.com> >>>>> --- >>>>> 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 > My apologies for the delay. I have done some experiments. Could you resend the patch, please make sure it is not encoded in base64 like this one. I am going to take your patch together with some other changes. Regards, Lukasz
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);
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@mediatek.com> --- drivers/thermal/thermal_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.18.0