From: Di Shen <cindygm567@gmail.com>
To: Lukasz Luba <lukasz.luba@arm.com>
Cc: Di Shen <di.shen@unisoc.com>,
amitk@kernel.org, linux-pm@vger.kernel.org, rui.zhang@intel.com,
daniel.lezcano@linaro.org, linux-kernel@vger.kernel.org,
xuewen.yan@unisoc.com, jeson.gao@unisoc.com, zhanglyra@gmail.com,
orsonzhai@gmail.com, rafael@kernel.org
Subject: Re: [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed
Date: Wed, 21 Jun 2023 12:51:00 +0800 [thread overview]
Message-ID: <CAHYJL4rKE+2TiYLEYUE3VO7Ws+uPazD0Fd-9EqZ1pGB_eU2Lvw@mail.gmail.com> (raw)
In-Reply-To: <1a921c36-1959-65e0-aaf7-da2683cdb8c4@arm.com>
On Tue, Jun 20, 2023 at 6:39 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Di,
>
> I have missed your v4 because it landed below your v3 thread.
>
> On 6/19/23 07:35, Di Shen wrote:
> > When the thermal trip point is changed, the governor should
> > be reset so that the policy algorithm be updated to adapt to the
> > new trip point.
> >
> > This patch adds an ops for thermal the governor structure to reset
>
> s/ops/callback
>
> > the governor. The ops is called when the trip point is changed.
> > For power allocator, the parameters of pid controller and the states
> > of power cooling devices can be reset when the passive trip point
> > is changed.
> >
> > Signed-off-by: Di Shen <di.shen@unisoc.com>
> >
> > ---
> > V4:
> > - Compared to V3, handle it in thermal core instead of in governor.
> >
> > - Add an ops to the governor structure, and call it when a trip
> > point is changed.
> >
> > - Define reset ops for power allocator.
> >
> > V3:
> > - Add fix tag.
> >
> > V2:
> > - Compared to v1, do not revert.
> >
> > - Add a variable(last_switch_on_temp) in power_allocator_params
> > to record the last switch_on_temp value.
> >
> > - Adds a function to renew the update flag and update the
> > last_switch_on_temp when thermal trips are writable.
> >
> > V1:
> > - Revert commit 0952177f2a1f.
> > ---
> > ---
> > drivers/thermal/gov_power_allocator.c | 21 +++++++++++++++++++++
> > drivers/thermal/thermal_trip.c | 6 ++++++
> > include/linux/thermal.h | 1 +
> > 3 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> > index 8642f1096b91..41d155adc616 100644
> > --- a/drivers/thermal/gov_power_allocator.c
> > +++ b/drivers/thermal/gov_power_allocator.c
> > @@ -729,10 +729,31 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
> > return allocate_power(tz, trip.temperature);
> > }
> >
> > +static int power_allocator_reset(struct thermal_zone_device *tz, int trip_id)
> > +{
> > + int ret = 0;
> > + struct thermal_trip trip;
> > + struct power_allocator_params *params = tz->governor_data;
> > +
> > + ret = __thermal_zone_get_trip(tz, trip_id, &trip);
> > + if (ret)
> > + return ret;
> > +
> > + /* Only need reset for passive trips */
> > + if (trip.type != THERMAL_TRIP_PASSIVE)
> > + return -EINVAL;
> > +
> > + reset_pid_controller(params);
> > + allow_maximum_power(tz, true);
> > +
> > + return ret;
> > +}
> > +
> > static struct thermal_governor thermal_gov_power_allocator = {
> > .name = "power_allocator",
> > .bind_to_tz = power_allocator_bind,
> > .unbind_from_tz = power_allocator_unbind,
> > .throttle = power_allocator_throttle,
> > + .reset = power_allocator_reset,
> > };
> > THERMAL_GOVERNOR_DECLARE(thermal_gov_power_allocator);
> > diff --git a/drivers/thermal/thermal_trip.c b/drivers/thermal/thermal_trip.c
> > index 907f3a4d7bc8..52eb768fada8 100644
> > --- a/drivers/thermal/thermal_trip.c
> > +++ b/drivers/thermal/thermal_trip.c
> > @@ -173,6 +173,12 @@ int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
> > if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
> > tz->trips[trip_id] = *trip;
> >
> > + if (t.temperature != trip->temperature && tz->governor && tz->governor->reset) {
> > + ret = tz->governor->reset(tz, trip_id);
> > + if (ret)
> > + pr_warn_once("Failed to reset thermal governor\n");
> > + }
>
> I agree with Rafael. Maybe change that to debug print, so that can be
> checked during the product testing. We cannot do much if that happens.
>
Right.
> > +
> > thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
> > trip->temperature, trip->hysteresis);
> >
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index 87837094d549..155ce2291fa5 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -204,6 +204,7 @@ struct thermal_governor {
> > int (*bind_to_tz)(struct thermal_zone_device *tz);
> > void (*unbind_from_tz)(struct thermal_zone_device *tz);
> > int (*throttle)(struct thermal_zone_device *tz, int trip);
> > + int (*reset)(struct thermal_zone_device *tz, int trip);
> > struct list_head governor_list;
> > };
> >
>
> That thermal_governor::reset() callback is what I had im mind while
> giving you the feedback for the v1. Now it's much cleaner what is going
> on and why.
>
Yes, it is necessary to do something for the governor if the trip point
is changed, especially for the governors that their trips are strongly
related to
the policy.
> Apart from some small bits, LGTM. Please adjust the comment in the patch
> header and this debug print and you can add:
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
>
> Please send the next version as separate new thread.
>
> Regards,
> Lukasz
Thank you Lukasz !
I couldn't agree with you more about your comments. What you have said is what
I want to express.
I'd love to send the next version. Thanks again.
Best regards,
Di
prev parent reply other threads:[~2023-06-21 4:51 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-20 9:56 [PATCH V3] thermal/core/power_allocator: avoid thermal cdev can not be reset Di Shen
2023-03-20 12:24 ` Lukasz Luba
2023-04-10 2:09 ` Di Shen
2023-04-10 19:51 ` Daniel Lezcano
2023-04-11 7:17 ` Lukasz Luba
2023-04-13 4:51 ` Di Shen
2023-04-13 7:37 ` Daniel Lezcano
2023-04-13 8:40 ` Di Shen
2023-04-14 11:12 ` Daniel Lezcano
2023-04-14 14:18 ` Lukasz Luba
2023-04-14 15:06 ` Daniel Lezcano
2023-04-14 15:21 ` Lukasz Luba
2023-05-25 6:39 ` Di Shen
2023-06-19 6:35 ` [PATCH V4] thermal/core/power_allocator: reset thermal governor when trip point is changed Di Shen
2023-06-20 9:46 ` Rafael J. Wysocki
2023-06-20 10:07 ` Rafael J. Wysocki
2023-06-20 10:20 ` Lukasz Luba
2023-06-20 10:39 ` Rafael J. Wysocki
2023-06-20 11:56 ` Lukasz Luba
2023-06-22 18:27 ` Rafael J. Wysocki
2023-06-23 7:43 ` Lukasz Luba
2023-06-23 16:55 ` Rafael J. Wysocki
2023-06-23 17:34 ` Lukasz Luba
2023-06-25 8:40 ` Di Shen
2023-06-25 8:39 ` Di Shen
2023-06-26 7:45 ` Lukasz Luba
2023-06-20 10:38 ` Lukasz Luba
2023-06-21 4:51 ` Di Shen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHYJL4rKE+2TiYLEYUE3VO7Ws+uPazD0Fd-9EqZ1pGB_eU2Lvw@mail.gmail.com \
--to=cindygm567@gmail.com \
--cc=amitk@kernel.org \
--cc=daniel.lezcano@linaro.org \
--cc=di.shen@unisoc.com \
--cc=jeson.gao@unisoc.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lukasz.luba@arm.com \
--cc=orsonzhai@gmail.com \
--cc=rafael@kernel.org \
--cc=rui.zhang@intel.com \
--cc=xuewen.yan@unisoc.com \
--cc=zhanglyra@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).