MIME-Version: 1.0
References: <20230309135515.1232-1-di.shen@unisoc.com> <db539c1e-22d5-2261-1248-07883dac12ee@arm.com>
In-Reply-To: <db539c1e-22d5-2261-1248-07883dac12ee@arm.com>
From:   Xuewen Yan <xuewen.yan94@gmail.com>
Date:   Mon, 13 Mar 2023 09:40:58 +0800
Message-ID: <CAB8ipk_T5RUZxD42d9wg_i8-3UXHFP=4Ffa_NH8Nm7FnyW2Ppw@mail.gmail.com>
Subject: Re: [PATCH] thermal/core/power_allocator: avoid cdev->state can not
 be reset
To:     Lukasz Luba <lukasz.luba@arm.com>
Cc:     Di Shen <di.shen@unisoc.com>, daniel.lezcano@linaro.org,
        rafael@kernel.org, amitk@kernel.org, rui.zhang@intel.com,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        xuewen.yan@unisoc.com, Qais Yousef <qyousef@layalina.io>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Lukasz

On Fri, Mar 10, 2023 at 11:56=E2=80=AFPM 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 de=
vice state
> > may not be reset to max, because the last_temp is smaller than the swit=
ch_on_temp.
> >
> > For example:
> > First:
> > swicth_on_temp=3D70 control_temp=3D85;
> >
> > Then userspace change the trip_temp:
> > swicth_on_temp=3D45 control_temp=3D55 cur_temp=3D54
> >
> > Then userspace reset the trip_temp:
> > swicth_on_temp=3D70 control_temp=3D85 cur_temp=3D57 last_temp=3D54
> >
> > 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 =3D 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.


> >
> > 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 scenario=
s.
And now in android with google's GKI2.0, the config must be opened.

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

Thanks!
BR,
xuewen

>
> Regards,
> Lukasz