Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
MIME-Version: 1.0
References: <13365827.uLZWGnKmhe@kreacher> <3768557.kQq0lBPeGt@kreacher> <ecd3e95c-bfea-4f0e-94c8-28f056b6206a@arm.com>
In-Reply-To: <ecd3e95c-bfea-4f0e-94c8-28f056b6206a@arm.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Fri, 20 Oct 2023 18:41:35 +0200
Message-ID: <CAJZ5v0hCYqZ4qinY09NA05Zdw8N-6jZk0sMVN6epfH=dBX4eJg@mail.gmail.com>
Subject: Re: [PATCH v1 4/6] thermal: gov_power_allocator: Use trip pointers
 instead of trip indices
To:     Lukasz Luba <lukasz.luba@arm.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        LKML <linux-kernel@vger.kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Zhang Rui <rui.zhang@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Fri, Oct 20, 2023 at 6:36 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 10/6/23 18:47, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Modify the power allocator thermal governor to use trip pointers instead
> > of trip indices everywhere except for the power_allocator_throttle()
> > second argument that will be changed subsequently along with the
> > definition of the .throttle() governor callback.
> >
> > The general functionality is not expected to be changed.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/gov_power_allocator.c |  123 +++++++++++++---------------------
> >   1 file changed, 49 insertions(+), 74 deletions(-)
> >
>
> [snip]
>
> > @@ -636,7 +619,6 @@ static int power_allocator_bind(struct t
> >   {
> >       int ret;
> >       struct power_allocator_params *params;
> > -     struct thermal_trip trip;
> >
> >       ret = check_power_actors(tz);
> >       if (ret)
> > @@ -662,12 +644,13 @@ static int power_allocator_bind(struct t
> >       get_governor_trips(tz, params);
> >
> >       if (tz->num_trips > 0) {
> > -             ret = __thermal_zone_get_trip(tz, params->trip_max_desired_temperature,
> > -                                           &trip);
> > -             if (!ret)
> > +             const struct thermal_trip *trip;
> > +
> > +             trip = params->trip_max_desired_temperature;
> > +             if (trip)
> >                       estimate_pid_constants(tz, tz->tzp->sustainable_power,
> >                                              params->trip_switch_on,
> > -                                            trip.temperature);
> > +                                            trip->temperature);
> >       }
>
> The code check for the populated pointer (by earlier new
> get_governor_trips(tz, params)) :
>
> if (params->trip_max_desired_temperature) {
>         int temp = params->trip_max_desired_temperature->temperature;
>
>         estimate_pid_constants(...)
> }
>
> looks better (what you have figured out already).
>
> Other than that the patch LGTM:
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
> Tested-by: Lukasz Luba <lukasz.luba@arm.com>

Thanks!

> I have also tested this is a few ways and it still works as expected:
> 1. Power allocation in different conditions
> 2. Trip points properly recognized with this nice get_governor_trips(),
>    here is the test output, with many tricky trip point setups:

Much appreciated!

> use case A:
> 5 trip points:
> - 2 passive for 50, 60 degC
> - 1 active 70degC
> - 2 passive 70, 85 degC
> - 1 critical 120 degC
>
> expected IPA trip points: 50, 85 deg => 35 degC IPA operating range
>
> [   24.578806] Power allocator: IPA: trip->temperature=50000
> [   24.578824] Power allocator: IPA: passive trip->temperature=50000
> [   24.578838] Power allocator: IPA: fist passive trip->temperature=50000
> [   24.578851] Power allocator: IPA: trip->temperature=60000
> [   24.578863] Power allocator: IPA: passive trip->temperature=60000
> [   24.578875] Power allocator: IPA: trip->temperature=70000
> [   24.578888] Power allocator: IPA: active trip->temperature=70000
> [   24.578900] Power allocator: IPA: trip->temperature=120000
> [   24.578912] Power allocator: IPA: trip->temperature=70000
> [   24.578925] Power allocator: IPA: passive trip->temperature=70000
> [   24.578937] Power allocator: IPA: trip->temperature=85000
> [   24.578950] Power allocator: IPA: passive trip->temperature=85000
> [   24.578964] Power allocator: IPA: trip_switch_on->temperature=50000
> control_temp=85000
> [   24.578978] Power allocator: IPA: temperature_threshold=35000
>
> -------------------------------------------------------------
> use case B:
> 5 trip points:
> - 2 active for 50, 60 degC
> - 1 active 70degC
> - 2 passive 70, 85 degC
> - 1 critical 120 degC
>
> expected IPA trip points: 70, 85 deg => 15 degC IPA operating range
>
> [   27.402474] Power allocator: IPA: trip->temperature=50000
> [   27.402492] Power allocator: IPA: active trip->temperature=50000
> [   27.402505] Power allocator: IPA: trip->temperature=60000
> [   27.402518] Power allocator: IPA: active trip->temperature=60000
> [   27.402531] Power allocator: IPA: trip->temperature=70000
> [   27.402544] Power allocator: IPA: active trip->temperature=70000
> [   27.402557] Power allocator: IPA: trip->temperature=120000
> [   27.402570] Power allocator: IPA: trip->temperature=70000
> [   27.402582] Power allocator: IPA: passive trip->temperature=70000
> [   27.402596] Power allocator: IPA: fist passive trip->temperature=70000
> [   27.402608] Power allocator: IPA: trip->temperature=85000
> [   27.402622] Power allocator: IPA: passive trip->temperature=85000
> [   27.402635] Power allocator: IPA: trip_switch_on->temperature=70000
> control_temp=85000
> [   27.402649] Power allocator: IPA: temperature_threshold=15000
>
> --------------------------------------------------------
> use case C:
> 6 trip points:
> - 2 active for 50, 60 degC
> - 1 active 70degC
> - 3 passive 70, 85, 90 degC
> - 1 critical 120 degC
>
> expected IPA trip points: 50, 85 deg => 20 degC IPA operating range
>
> [   36.998907] Power allocator: IPA: trip->temperature=50000
> [   36.998921] Power allocator: IPA: active trip->temperature=50000
> [   36.998935] Power allocator: IPA: trip->temperature=60000
> [   36.998948] Power allocator: IPA: active trip->temperature=60000
> [   36.998960] Power allocator: IPA: trip->temperature=70000
> [   36.998973] Power allocator: IPA: active trip->temperature=70000
> [   36.998985] Power allocator: IPA: trip->temperature=120000
> [   36.998999] Power allocator: IPA: trip->temperature=70000
> [   36.999011] Power allocator: IPA: passive trip->temperature=70000
> [   36.999024] Power allocator: IPA: fist passive trip->temperature=70000
> [   36.999037] Power allocator: IPA: trip->temperature=85000
> [   36.999049] Power allocator: IPA: passive trip->temperature=85000
> [   36.999062] Power allocator: IPA: trip->temperature=90000
> [   36.999074] Power allocator: IPA: passive trip->temperature=90000
> [   36.999087] Power allocator: IPA: trip_switch_on->temperature=70000
> control_temp=90000
> [   36.999101] Power allocator: IPA: temperature_threshold=20000
>