Received-SPF: pass (google.com: domain of linux-kernel+bounces-209584-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 11 Jun 2024 10:39:18 +0100
From: Ionela Voinescu <ionela.voinescu@arm.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "liwei (JK)" <liwei728@huawei.com>,
	Beata Michalska <beata.michalska@arm.com>,
	Vanshidhar Konda <vanshikonda@os.amperecomputing.com>,
	rafael@kernel.org, al.stone@linaro.org, ashwin.chaugule@linaro.org,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	liwei391@huawei.com, liaoyu15@huawei.com
Subject: Re: [PATCH] cpufreq/cppc: changing highest_perf to nominal_perf in
 cppc_cpufreq_cpu_init()
Message-ID: <ZmgbRh+m9MmEaopK@arm.com>
References: <20240428092852.1588188-1-liwei728@huawei.com>
 <20240429104945.esdukn6ayudgyumc@vireshk-i7>
 <ZjoBrF4bAK5ukm7H@arm.com>
 <be312b75-eede-44f5-b7f3-b50f50c6fb56@huawei.com>
 <ZmB1qKucR5fXk100@arm.com>
 <20240606072031.lxr7tykl7sdgjwva@vireshk-i7>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240606072031.lxr7tykl7sdgjwva@vireshk-i7>

Hey,

On Thursday 06 Jun 2024 at 12:50:31 (+0530), Viresh Kumar wrote:
> On 05-06-24, 15:26, Ionela Voinescu wrote:
> > > > > > 		cpufreq_start_governor() // governor: performance
> > > > > > 			new_freq = cpufreq_driver->get() // if new_freq == policy->max
> > > > > > 			if (policy->cur != new_freq)
> > > > > > 			cpufreq_out_of_sync(policy, new_freq)
> > > > > > 				...
> > > > > > 				policy->cur = new_freq
> > > > I believe the problem is here   ^^^^^^^^^^^^^^^^^^^^^^.
> > > > 
> > > > cpufreq_verify_current_freq() should not update policy->cur unless a
> > > > request to change frequency has actually reached the driver. I believe
> > > > policy->cur should always reflect the request, not the actual current
> > > > frequency of the CPU.
> 
> There are times when the core doesn't have any prior information about
> the frequency, for example at driver probe time and resume. And so
> needs to set policy->cur by reading it from the hardware.
> 

Makes sense! But maybe we should no longer update policy->cur to the
current/hardware frequency once a request comes through from a
governor, and we have a first actually requested value.

> > > > Given that new_freq is the current (hardware) frequency of the CPU,
> > > > obtained via .get(), it can be the nominal frequency, as it is in your
> > > > case, or any frequency, if there is any firmware/hardware capping in
> > > > place.
> > > > 
> > > > This causes the issue in your scenario, in which __cpufreq_driver_target()
> > > > filters the request from the governor as it finds it equal to policy->cur,
> > > > and it believes it's already set by hardware.
> 
> I am still not sure why mismatch happens at boot time here.
> 
> > > > This causes another issue in which scaling_cur_freq, which for some
> > > > systems returns policy->cur, ends up returning the hardware frequency of
> > > > the CPUs, and not the last frequency request, as it should:
> > > > 
> > > > "scaling_cur_freq
> > > > Current frequency of all of the CPUs belonging to this policy (in kHz).
> > > > 
> > > > In the majority of cases, this is the frequency of the last P-state
> > > > requested by the scaling driver from the hardware using the scaling
> > > > interface provided by it, which may or may not reflect the frequency
> > > > the CPU is actually running at (due to hardware design and other
> > > > limitations)." [1]
> 
> There is discussion going on about this in another thread [1] now.

Thanks for the contributions to that topic, by the way.

Kind regards,
Ionela.

> 
> > > > Therefore policy->cur gets polluted with the hardware frequency of the
> > > > CPU sampled at that one time, and this affects governor decisions, as
> > > > in your case, and scaling_cur_freq feedback as well. This bad value will
> > > > not change until there's another .target() or cpufreq_out_of_sync()
> > > > call, which will never happen for fixed frequency governors like the
> > > > performance governor.
> 
> --
> viresh
> 
> [1] https://lore.kernel.org/all/20240603081331.3829278-2-beata.michalska@arm.com/