Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230320095620.7480-1-di.shen@unisoc.com> <6055bc39-5c00-d12f-b5c3-fa21a9649d63@arm.com>
 <CAHYJL4qL+nJuiN8vXGaiPQuuaPx6BA+yjRq2TRaBgb+qXi8-yw@mail.gmail.com>
 <637a3bb1-ba1c-e707-01b7-06c1358583ca@linaro.org> <CAHYJL4rnfVp+X3imbxWzUd9ixTFAPe4ioLyi-t50PwhL0y5v8A@mail.gmail.com>
 <da59b4ef-1532-1b3a-7a73-9a095d8c9390@linaro.org> <CAHYJL4qJwKHFsCPUvLzmUEAJtEfHDAO23D5=0zAXOYSCABJ8_g@mail.gmail.com>
 <6aad180f-410c-5b11-b30b-c7bc02cbe054@linaro.org> <e31da1fa-168d-9a85-cdb3-66192d887d83@arm.com>
 <ce9b3b01-e496-9e02-5583-41893b7154c7@linaro.org> <6022d391-9ae8-2bb4-0f81-2c99466dc556@arm.com>
In-Reply-To: <6022d391-9ae8-2bb4-0f81-2c99466dc556@arm.com>
From:   Di Shen <cindygm567@gmail.com>
Date:   Thu, 25 May 2023 14:39:30 +0800
Message-ID: <CAHYJL4r0yxLDHsTDKdcny6F7Lbzo-D48RGWyax07YUUFuzC2mg@mail.gmail.com>
Subject: Re: [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can
 not be reset
To:     Lukasz Luba <lukasz.luba@arm.com>
Cc:     Daniel Lezcano <daniel.lezcano@linaro.org>,
        Di Shen <di.shen@unisoc.com>, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, xuewen.yan@unisoc.com,
        jeson.gao@unisoc.com, zhanglyra@gmail.com, orsonzhai@gmail.com,
        rui.zhang@intel.com, amitk@kernel.org, rafael@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Apr 14, 2023 at 11:21=E2=80=AFPM Lukasz Luba <lukasz.luba@arm.com> =
wrote:
>
>
>
> On 4/14/23 16:06, Daniel Lezcano wrote:
> > On 14/04/2023 16:18, Lukasz Luba wrote:
> >>
> >>
> >> On 4/14/23 12:12, Daniel Lezcano wrote:
> >>> On 13/04/2023 10:40, Di Shen wrote:
> >>>> We have discussed this question in patch-v1:
> >>>> https://lore.kernel.org/all/f6aaa5f1-495d-a158-14d8-ddb2bffbd9c2@arm=
.com/
> >>>>
> >>>> Simply put, we use the trip_temp in the Android System; set differen=
t
> >>>> trip_temp for thermal control of different scenarios.
> >>>
> >>> The changes are dealing with the trip points and trying to detect the
> >>> threshold. That part should be handled in the thermal core or thermal
> >>> trip side, not in the governor.
> >>>
> >>> AFAICT, if a trip point is changed, then the power allocator should
> >>> be reset, including the cdev state.
> >>>
> >>> It would be more convenient to add an ops to the governor ops
> >>> structure to reset the governor and then call it when a trip point is
> >>> changed in thermal_zone_set_trip()
> >>>
> >>>
> >>
> >> Sounds reasonable to have a proper API and fwk handling this corner
> >> case scenario.
> >> Although, if there is a need for a 'easy-to-backport' fix for IPA only=
,
> >> I agree with this patch, since it's straight forward to put in some
> >> Android kernel. We can later fix the framework to handle this properly=
.
> >> Anyway, both ways are OK to me.
> >
> > Unfortunately, we can not do the maintenance of the Linux kernel based
> > on an 'easy-to-backport' policy to Android.
> >
> > This patch could be applied from-list to Android as a hotfix. But for
> > Linux the fix should be more elaborated. One solution is to add a
> > 'reset' ops and call it from the trip point update function.
>
> Fair enough.
>
> >
> > Did you double check the issue is not impacting the other governors too=
 ?
>
> No, unfortunately, I haven't checked other governors.

Hi Lukasz and Daniel,
I rethought about this issue, and have tried three ways to solve it.
Finally, I realized that the root cause might be the cdev->state
update and notify. We should get back to take cdev->state into
account.

The three ways:
1.From trips updating perspective:
As your suggestion=EF=BC=8Cadd an ops function for thermal_governor
structure=EF=BC=8Cdefine it in IPA governor, and call it when trips are
changed by userspace(sysfs node).

2.From cdev->state updating perspective:
For example, for gov_power_allocator there are two branches reached to
__thermal_cdev_update.

power_allocator_trottle
        |
allow_maximum_power()[gov_power_allocator.c]
        |
__thermal_cdev_update()[thermal_helpers.c]<<<<<<<(1)
        |
thermal_cdev_set_cur_state()
        |
cdev->ops->set_cur_state()
        |
thermal_notify_cdev_state_update()
        |
     .......


power_allocator_throttle()[gov_power_allocator.c]
        |
allocate_power()
        |
power_actor_set_power()
        |
__thermal_cdev_update()[thermal_helpers.c]<<<<<<(2)
        |
      ......

Add a variable last_state for thermal_cooling_device structure to
record the last target cooling state, and before
thermal_notify_cdev_state_update, determine whether the last_state is
equal to current state. If not equal, then
thermal_notify_cdev_state_update.

static void thermal_cdev_set_cur_state(struct thermal_cooling_device
*cdev,
~                                        unsigned long target)
{
        if (cdev->ops->set_cur_state(cdev, target))
                return;

~       if (cdev->last_state !=3D target)
+               thermal_notify_cdev_state_update(cdev->id, target);
+
+       cdev->last_state =3D target;
+
        thermal_cooling_device_stats_update(cdev, target);
}

In this way, it will only update and notify when the state is changed
which means there's no need to use update flag to make sure it updates
cdev->state only once.

It can avoid a lot of unnecessary notifications, not only when the
temperature drops below the first trip point(at this situation
cdev->state is always 0) but also when the policy is working.

3.Similar to the second method, but an easier one.
Actually, in the set_cur_state ops of every cooling device, it has
already checked whether the last cooling state is equal to current
value or not, and returns 0. Like cpufreq cooling device:
static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev,
                                unsigned long state)
{
        //.......
        /* Request state should be less than max_level */
        if (state > cpufreq_cdev->max_level)
                return -EINVAL;

        /* Check if the old cooling action is same as new cooling
action */
        if (cpufreq_cdev->cpufreq_state =3D=3D state)
                return 0; //return -EAGAIN;
}

What if return a non-zero value? 1 or -EAGAIN(means thy again)? Then
thermal_cdev_set_cur_state() in __thermal_cdev_update() can return
directly without update and notify.

I prefer method 3. Because there's no more new variable or function
compared to 1 and 2, and it can make code more brief.

Well, what do you think about the three ways? Look forward to your
comments. Thank you!

Best regards,
Di