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@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