Messages in this thread | | | Date | Mon, 13 Mar 2023 11:18:07 +0000 | Subject | Re: [PATCH] thermal/core/power_allocator: avoid cdev->state can not be reset | From | Lukasz Luba <> |
| |
On 3/13/23 11:10, Xuewen Yan wrote: > Hi Lukasz > > On Mon, Mar 13, 2023 at 5:35 PM Lukasz Luba <lukasz.luba@arm.com> wrote: >> >> Hi Xuewen, >> >> On 3/13/23 01:40, Xuewen Yan wrote: >>> Hi Lukasz >>> >>> On Fri, Mar 10, 2023 at 11:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote: >>>> >>>> Hi Di, >>>> >>>> On 3/9/23 13:55, Di Shen wrote: >>>>> Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low) >>>>> add a update flag to update cooling device only once when temp is low. >>>>> But when the switch_on_temp is set to be a higher value, the cooling device state >>>>> may not be reset to max, because the last_temp is smaller than the switch_on_temp. >>>>> >>>>> For example: >>>>> First: >>>>> swicth_on_temp=70 control_temp=85; >>>>> >>>>> Then userspace change the trip_temp: >>>>> swicth_on_temp=45 control_temp=55 cur_temp=54 >>>>> >>>>> Then userspace reset the trip_temp: >>>>> swicth_on_temp=70 control_temp=85 cur_temp=57 last_temp=54 >>>>> >>>>> At this time, the cooling device state should be reset to be max. >>>>> However, because cur_temp(57) < switch_on_temp(70) >>>>> last_temp(54) < swicth_on_temp(70) --> update = false >>>>> When update is false, the cooling device state can not be reset. >>>> >>>> That's a tricky use case. How is that now possible, >>> >>> We use the trip_temp in the Android System. Often, we set different >>> control temperatures in different scenarios, >>> and when we change the switch_on_temp from small to bigger, we find >>> the power can not be reset to be max. >> >> I see, thanks for letting me know that this is Android. >> >>> >>> >>>>> >>>>> So delete the update condition, so that the cooling device state >>>>> could be reset. >>>> >>>> IMO this is not the desired solution. Daniel reported the issue that >>>> IPA triggers the event sent to user-space even when there is no need. >>>> That's the motivation for the 0952177f2a1f change. >>>> >>>> To address your scenario properly, we need an interface which allows >>>> to respond properly for such situation when someone from user-space >>>> writes a new value to something fundamental as trip point. >>>> >>>> You also have a kernel config enabled: >>>> CONFIG_THERMAL_WRITABLE_TRIPS >>>> which IMO is only for debug kernels for system integrator (according >>>> to the Kconfig description). >>> >>> Yes, we use it to meet the temperature control needs of different scenarios. >>> And now in android with google's GKI2.0, the config must be opened. >> >> OK >> >>> >>>> >>>> When you disable this config in your deploy/product kernel >>>> than this issue would disappear. >>>> >>>>> >>>>> Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low) >>>>> Signed-off-by: Di Shen <di.shen@unisoc.com> >>>>> --- >>>>> drivers/thermal/gov_power_allocator.c | 9 +++------ >>>>> 1 file changed, 3 insertions(+), 6 deletions(-) >>>>> >>>> >>>> That's why IMO this is not the solution. >>> >>> Yes, but I think we should fix the bug, although the >>> CONFIG_THERMAL_WRITABLE_TRIPS is just for debugging. >>> How about record the last_trip_temp, and when the last_temp > >>> last_trip_temp, set the update tobe true? >> >> Yes, if that config is used in Android then we must fix it. >> >> That last_trip_temp makes sense (but maybe name it last_switch_on_temp). >> Please put that new field into the IPA local >> struct power_allocator_params. We should store the trip temp >> value there every time power_allocator_throttle() is called. >> That can be called due to a write from user-space w/ a new trip point >> value, so should be OK. > > Thanks for the suggestion. We would send the patch-v2 as soon as possible.
Thanks! I'll review that and check on my board. BTW, which board/device you use with this IPA? Maybe I can buy it and also test to capture such regression in future.
| |