Re: [PATCH] thermal/core/power_allocator: avoid cdev->state can not be reset

[Xuewen Yan]

add Orson and lyra

Hi Lukasz

On Mon, Mar 13, 2023 at 7:18 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
>
>
>
> On 3/13/23 11:10, Xuewen Yan wrote:
> > Hi Lukasz
> >
> > On Mon, Mar 13, 2023 at 5:35 PM Lukasz Luba <lukasz.luba@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxx>
> >>>>> ---
> >>>>>     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.

We use our company(unisoc)'s mobile phone, and the development board
with the same chip should be available in the market. And Orson would
help and then reply to you.

Thanks!
BR
xuewen