[PATCH] thermal: power_allocate: add upper and lower limits

[PATCH] thermal: power_allocate: add upper and lower limits

[Michael Kao]

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);
-- 
2.18.0

Re: [PATCH] thermal: power_allocate: add upper and lower limits

[Lukasz Luba]

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

Re: [PATCH] thermal: power_allocate: add upper and lower limits

[Michael Kao]

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.

Best Regards,
Michael

Re: [PATCH] thermal: power_allocate: add upper and lower limits

[Lukasz Luba]

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

Re: [PATCH] thermal: power_allocate: add upper and lower limits

[Michael Kao]

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

Re: [PATCH] thermal: power_allocate: add upper and lower limits

[Lukasz Luba]

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

Re: [PATCH] thermal: power_allocate: add upper and lower limits

[Lukasz Luba]

On 10/6/20 3:39 PM, Lukasz Luba wrote:
> 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

Please change also the commit message to something like:

thermal: core: add upper and lower limits to power_actor_set_power

The upper and lower limits of thermal throttle state in the
DT do not apply to the Intelligent Power Allocation (IPA) governor.
Add the clamping for cooling device upper and lower limits in the
power_actor_set_power() used by IPA.

Regards,
Lukasz