Re: [PATCH v4 0/9] Inefficient OPPs

From: Viresh Kumar <viresh.kumar@linaro.org>
To: Quentin Perret <qperret@google.com>
Cc: Lukasz Luba <lukasz.luba@arm.com>,
	r@google.com, "Rafael J. Wysocki" <rafael@kernel.org>,
	Vincent Donnefort <vincent.donnefort@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	Ionela Voinescu <ionela.voinescu@arm.com>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Matthias Kaehlcke <mka@chromium.org>
Subject: Re: [PATCH v4 0/9] Inefficient OPPs
Date: Wed, 11 Aug 2021 10:33:27 +0530	[thread overview]
Message-ID: <20210811050327.3yxrk4kqxjjwaztx@vireshk-i7> (raw)
In-Reply-To: <YRKfluMP8G41/P61@google.com>

On 10-08-21, 16:47, Quentin Perret wrote:
> On Tuesday 10 Aug 2021 at 16:12:29 (+0100), Lukasz Luba wrote:
> > I've checked that. It's not the policy notifier and arch_topology which
> > cause an issue, but the cpufreq governor setup code. Anyway, we cannot
> > wait so late with the EM registration, till e.g. ::ready() callback.
> 
> Aha, yes, because by the time the arch_topology driver rebuilds the
> sched domains, the governor is not 'installed', which means the
> scheduler is not in a position to enable EAS yet. So we need to wait
> until sched_cpufreq_governor_change() is called for that. Makes sense,
> thanks for checking, and +1 to your conclusion.

What about this then ?

Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Wed Aug 11 10:24:28 2021 +0530

    cpufreq: Call ->ready() before initializing governor

    The driver may want to do stuff from the ->ready() callback, like
    registering with the EM core, after the policy is initialized, but
    before the governor is setup (since governor may end up using that
    information).

    Call the ->ready() callback before setting up the governor.

    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a060dc2aa2f2..2df41b98bbb3 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1494,6 +1494,10 @@ static int cpufreq_online(unsigned int cpu)
                write_unlock_irqrestore(&cpufreq_driver_lock, flags);
        }

+       /* Callback for handling stuff after policy is ready */
+       if (cpufreq_driver->ready)
+               cpufreq_driver->ready(policy);
+
        ret = cpufreq_init_policy(policy);
        if (ret) {
                pr_err("%s: Failed to initialize policy for cpu: %d (%d)\n",
@@ -1505,10 +1509,6 @@ static int cpufreq_online(unsigned int cpu)

        kobject_uevent(&policy->kobj, KOBJ_ADD);

-       /* Callback for handling stuff after policy is ready */
-       if (cpufreq_driver->ready)
-               cpufreq_driver->ready(policy);
-
        if (cpufreq_thermal_control_enabled(cpufreq_driver))
                policy->cdev = of_cpufreq_cooling_register(policy);

-- 
viresh
