Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Viresh Kumar <viresh.kumar@linaro.org>
To: swboyd@chromium.org, Rajendra Nayak <rnayak@codeaurora.org>,
	vincent.guittot@linaro.org, mturquette@baylibre.com
Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-spi@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-scsi@vger.kernel.org, ulf.hansson@linaro.org,
	dianders@chromium.org, rafael@kernel.org
Subject: Re: [RFC v2 01/11] OPP: Don't overwrite rounded clk rate
Date: Fri, 14 Jun 2019 10:57:32 +0530
Message-ID: <20190614052732.4w6vvwwich2h4cgu@vireshk-i7> (raw)
In-Reply-To: <20190613095419.lfjeko7nmxtix2n4@vireshk-i7>

On 13-06-19, 15:24, Viresh Kumar wrote:
> I am confused as hell on what we should be doing and what we are doing
> right now. And if we should do better.
> 
> Let me explain with an example.
> 
> - The clock provider supports following frequencies: 500, 600, 700,
>   800, 900, 1000 MHz.
> 
> - The OPP table contains/supports only a subset: 500, 700, 1000 MHz.
> 
> Now, the request to change the frequency starts from cpufreq
> governors, like schedutil when they calls:
> 
> __cpufreq_driver_target(policy, 599 MHz, CPUFREQ_RELATION_L);
> 
> CPUFREQ_RELATION_L means: lowest frequency at or above target. And so
> I would expect the frequency to get set to 600MHz (if we look at clock
> driver) or 700MHz (if we look at OPP table). I think we should decide
> this thing from the OPP table only as that's what the platform guys
> want us to use. So, we should end up with 700 MHz.
> 
> Then we land into dev_pm_opp_set_rate(), which does this (which is
> code copied from earlier version of cpufreq-dt driver):
> 
> - clk_round_rate(clk, 599 MHz).
> 
>   clk_round_rate() returns the highest frequency lower than target. So
>   it must return 500 MHz (I haven't tested this yet, all theoretical).
> 
> - _find_freq_ceil(opp_table, 500 MHz).
> 
>   This works like CPUFREQ_RELATION_L, so we find lowest frequency >=
>   target freq. And so we should get: 500 MHz itself as OPP table has
>   it.
> 
> - clk_set_rate(clk, 500 MHz).
> 
>   This must be doing round-rate again, but I think we will settle with
>   500 MHz eventually.
> 
> 
> Now the questionnaire:
> 
> - Is this whole exercise correct ?

No, I missed the call to cpufreq_frequency_table_target() in
__cpufreq_driver_target() which finds the exact frequency from cpufreq table
(which was created using opp table) and so we never screw up here. Sorry for
confusing everyone on this :(

> Now lets move to this patch, which makes it more confusing.
> 
> The OPP tables for CPUs and GPUs should already be somewhat like fmax
> tables for particular voltage values and that's why both cpufreq and
> OPP core try to find a frequency higher than target so we choose the
> most optimum one power-efficiency wise.
> 
> For cases where the OPP table is only a subset of the clk-providers
> table (almost always), if we let the clock provider to find the
> nearest frequency (which is lower) we will run the CPU/GPU at a
> not-so-optimal frequency. i.e. if 500, 600, 700 MHz all need voltage
> to be 1.2 V, we should be running at 700 always, while we may end up
> running at 500 MHz.

This won't happen for CPUs because of the reason I explained earlier. cpufreq
core does the rounding before calling dev_pm_opp_set_rate(). And no other
devices use dev_pm_opp_set_rate() right now apart from CPUs, so we are not going
to break anything.

> This kind of behavior (introduced by this patch) is important for
> other devices which want to run at the nearest frequency to target
> one, but not for CPUs/GPUs. So, we need to tag these IO devices
> separately, maybe from DT ? So we select the closest match instead of
> most optimal one.

Hmm, so this patch won't break anything and I am inclined to apply it again :)

Does anyone see any other issues with it, which I might be missing ?

-- 
viresh

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-20  9:49 [RFC v2 00/11] DVFS in the OPP core Rajendra Nayak
2019-03-20  9:49 ` [RFC v2 01/11] OPP: Don't overwrite rounded clk rate Rajendra Nayak
2019-06-11 10:54   ` Viresh Kumar
2019-06-12  7:42     ` Rajendra Nayak
2019-06-12  8:25       ` Viresh Kumar
2019-06-13  9:54         ` Viresh Kumar
2019-06-14  5:27           ` Viresh Kumar [this message]
2019-06-17  3:50             ` Viresh Kumar
2019-06-17  4:07               ` Rajendra Nayak
2019-06-17  4:17                 ` Viresh Kumar
2019-06-17  4:25                   ` Rajendra Nayak
2019-06-14  5:54           ` Rajendra Nayak
2019-03-20  9:49 ` [RFC v2 02/11] OPP: Make dev_pm_opp_set_rate() with freq=0 as valid Rajendra Nayak
2019-06-14  6:32   ` Viresh Kumar
2019-06-17  4:04     ` Rajendra Nayak
2019-03-20  9:49 ` [RFC v2 03/11] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state Rajendra Nayak
2019-03-20  9:49 ` [RFC v2 04/11] spi: spi-geni-qcom: " Rajendra Nayak
2019-03-20  9:49 ` [RFC v2 05/11] arm64: dts: sdm845: Add OPP table for all qup devices Rajendra Nayak
2019-03-20  9:49 ` [RFC v2 06/11] scsi: ufs: Add support to manage multiple power domains in ufshcd-pltfrm Rajendra Nayak
2019-03-20  9:49 ` [RFC v2 07/11] scsi: ufs: Add support for specifying OPP tables in DT Rajendra Nayak
2019-03-20  9:49 ` [RFC v2 08/11] arm64: dts: sdm845: Add ufs opps and power-domains Rajendra Nayak
2019-05-14  7:53   ` Ulf Hansson
2019-03-20  9:49 ` [RFC v2 09/11] drm/msm/dpu: Use OPP API to set clk/perf state Rajendra Nayak
2019-04-10  3:49   ` Viresh Kumar
2019-04-10  3:49     ` Viresh Kumar
2019-03-20  9:49 ` [RFC v2 10/11] drm/msm: dsi: " Rajendra Nayak
2019-03-20  9:49 ` [RFC v2 11/11] arm64: dts: sdm845: Add DSI and MDP OPP tables and power-domains Rajendra Nayak
2019-04-10  3:51 ` [RFC v2 00/11] DVFS in the OPP core Viresh Kumar
2019-04-10  3:51   ` Viresh Kumar
2019-05-21  6:22 ` Viresh Kumar
2019-05-24  6:03   ` Rajendra Nayak
2019-06-17  4:26 ` Viresh Kumar

Reply instructions:

You may reply publically 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=20190614052732.4w6vvwwich2h4cgu@vireshk-i7 \
    --to=viresh.kumar@linaro.org \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=rafael@kernel.org \
    --cc=rnayak@codeaurora.org \
    --cc=swboyd@chromium.org \
    --cc=ulf.hansson@linaro.org \
    --cc=vincent.guittot@linaro.org \
    /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

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox