Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Chanwoo Choi <cw00.choi@samsung.com>
To: Dmitry Osipenko <digetx@gmail.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	MyungJoo Ham <myungjoo.ham@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 10/19] PM / devfreq: tegra30: Reduce unnecessary interrupts activity
Date: Wed, 2 Oct 2019 08:35:13 +0900
Message-ID: <b67a7878-fa74-df89-9a62-556b9300b5a5@samsung.com> (raw)
In-Reply-To: <20190811212315.12689-11-digetx@gmail.com>

Hi,

On 19. 8. 12. 오전 6:23, Dmitry Osipenko wrote:
> There are cases where unnecessary ACTMON interrupts could be avoided,
> like when one memory client device requests higher clock rate than the
> other or when clock rate is manually limited using sysfs devfreq
> parameters. These cases could be avoided by tuning upper watermark or
> disabling hardware events when min/max boosting thresholds are reached.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra30-devfreq.c | 87 ++++++++++++++++++++++++++++---
>  1 file changed, 80 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index 43d50b4366dd..a2623de56d20 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -312,7 +312,8 @@ static void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra,
>  }
>  
>  static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
> -					   struct tegra_devfreq_device *dev)
> +					   struct tegra_devfreq_device *dev,
> +					   unsigned long freq)
>  {
>  	unsigned long avg_dependency_freq, lower, upper;
>  
> @@ -320,6 +321,22 @@ static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>  
>  	avg_dependency_freq = tegra_actmon_dev_avg_dependency_freq(tegra, dev);
>  
> +	/*
> +	 * If cumulative EMC frequency selection (MCALL / min_freq) is
> +	 * higher than the device's, then there is no need to set upper
> +	 * watermark to a lower value because it will result in unnecessary
> +	 * upper interrupts.
> +	 *
> +	 * Note that average watermarks are also updated after EMC
> +	 * clock rate change, hence if clock rate goes down, then the
> +	 * watermarks will be set in accordance to the new rate after
> +	 * changing the rate. There are other ways to achieve the same
> +	 * result, but this one is probably the least churning, although
> +	 * it may look a bit convoluted.
> +	 */
> +	if (freq * ACTMON_SAMPLING_PERIOD > upper)
> +		upper = freq * ACTMON_SAMPLING_PERIOD;
> +
>  	/*
>  	 * We want to get interrupts when MCCPU client crosses the
>  	 * dependency threshold in order to take into / out of account
> @@ -361,7 +378,18 @@ static void tegra_devfreq_update_wmark(struct tegra_devfreq *tegra,
>  	tegra_actmon_get_lower_upper(tegra, dev, freq - 1, &lower, &upper);
>  
>  	delta = do_percent(upper - lower, dev->config->boost_up_threshold);
> -	device_writel(dev, lower + delta, ACTMON_DEV_UPPER_WMARK);


Also, this patch edits the added codes on front patch.
This code was added on patch5 and then delete it on this patch.
If it is not necessary, you can remove it on patch5 by refactoring.

> +
> +	/*
> +	 * The memory events count could go a bit higher than the maximum
> +	 * defined by the OPPs, hence make the upper watermark infinitely
> +	 * high to avoid unnecessary upper interrupts in that case.
> +	 */
> +	if (freq == tegra->max_freq)
> +		upper = ULONG_MAX;
> +	else
> +		upper = lower + delta;
> +
> +	device_writel(dev, upper, ACTMON_DEV_UPPER_WMARK);

I think that the changes of tegra_devfreq_update_avg_wmark() on this patch
can be merged to patch5.

>  
>  	/*
>  	 * Meanwhile the lower mark is based on the average value
> @@ -379,6 +407,7 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  			      struct tegra_devfreq_device *dev)
>  {
>  	u32 intr_status, dev_ctrl, avg_intr_mask, avg_count;
> +	unsigned long freq;
>  
>  	intr_status = device_readl(dev, ACTMON_DEV_INTR_STATUS);
>  	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
> @@ -389,8 +418,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
>  			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
>  
> -	if (intr_status & avg_intr_mask)
> -		tegra_devfreq_update_avg_wmark(tegra, dev);
> +	if (intr_status & avg_intr_mask) {
> +		freq = clk_get_rate(tegra->emc_clock) / KHZ;
> +		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
> +	}
>  
>  	if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
>  		/*
> @@ -412,6 +443,8 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  		dev->boost_freq = do_percent(dev->boost_freq,
>  					     dev->config->boost_down_coeff);
>  
> +		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +
>  		if (dev->boost_freq < (ACTMON_BOOST_FREQ_STEP >> 1))
>  			dev->boost_freq = 0;
>  	}
> @@ -427,8 +460,16 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  	}
>  
>  	/* no boosting => no need for consecutive-down interrupt */
> -	if (dev->boost_freq == 0)
> +	if (dev->boost_freq == 0) {
>  		dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +	}
> +
> +	/* boosting max-out => no need for consecutive-up interrupt */
> +	if (dev->boost_freq == tegra->max_freq) {
> +		dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +		dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +	}
>  
>  	device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
>  	device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> @@ -437,8 +478,40 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  static unsigned long actmon_update_target(struct tegra_devfreq *tegra,
>  					  struct tegra_devfreq_device *dev)
>  {
> +	u32 avg_count, avg_freq, old_upper, new_upper, dev_ctrl;
>  	unsigned long target_freq;
>  
> +	/*
> +	 * The avg_count / avg_freq is getting snapshoted on device's
> +	 * interrupt, but there are cases where actual value need to
> +	 * be utilized on target's update, like CPUFreq boosting and
> +	 * overriding the min freq via /sys/class/devfreq/devfreq0/min_freq
> +	 * because we're optimizing the upper watermark based on the
> +	 * actual EMC frequency. This means that interrupt may be
> +	 * inactive for a long time and thus making snapshoted value
> +	 * outdated.
> +	 */
> +	avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
> +	avg_freq = avg_count / ACTMON_SAMPLING_PERIOD;
> +
> +	old_upper = tegra_actmon_upper_freq(tegra, dev->avg_freq);
> +	new_upper = tegra_actmon_upper_freq(tegra, avg_freq);
> +
> +	/* similar to ISR, see comments in actmon_isr_device() */
> +	if (old_upper != new_upper) {
> +		if (dev->boost_freq == tegra->max_freq) {
> +			dev_ctrl = device_readl(dev, ACTMON_DEV_CTRL);
> +
> +			dev_ctrl &= ~ACTMON_DEV_CTRL_CONSECUTIVE_BELOW_WMARK_EN;
> +			dev_ctrl |= ACTMON_DEV_CTRL_CONSECUTIVE_ABOVE_WMARK_EN;
> +
> +			device_writel(dev, dev_ctrl, ACTMON_DEV_CTRL);
> +		}
> +
> +		dev->avg_freq = avg_freq;
> +		dev->boost_freq = 0;
> +	}
> +
>  	target_freq = dev->avg_freq + dev->boost_freq;
>  	target_freq = tegra_actmon_account_cpu_freq(tegra, dev, target_freq);
>  
> @@ -506,7 +579,7 @@ static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>  	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>  		dev = &tegra->devices[i];
>  
> -		tegra_devfreq_update_avg_wmark(tegra, dev);
> +		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
>  		tegra_devfreq_update_wmark(tegra, dev, freq);
>  	}
>  
> @@ -522,7 +595,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>  	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
>  		      ACTMON_DEV_INIT_AVG);
>  
> -	tegra_devfreq_update_avg_wmark(tegra, dev);
> +	tegra_devfreq_update_avg_wmark(tegra, dev, dev->avg_freq);
>  	tegra_devfreq_update_wmark(tegra, dev, dev->avg_freq);
>  
>  	device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-11 21:22 [PATCH v6 00/19] More improvements for Tegra30 devfreq driver Dmitry Osipenko
2019-08-11 21:22 ` [PATCH v6 01/19] PM / devfreq: tegra30: Change irq type to unsigned int Dmitry Osipenko
2019-08-11 21:22 ` [PATCH v6 02/19] PM / devfreq: tegra30: Keep interrupt disabled while governor is stopped Dmitry Osipenko
2019-08-20  0:02   ` Chanwoo Choi
2019-08-11 21:22 ` [PATCH v6 03/19] PM / devfreq: tegra30: Handle possible round-rate error Dmitry Osipenko
2019-08-20  0:06   ` Chanwoo Choi
2019-08-11 21:23 ` [PATCH v6 04/19] PM / devfreq: tegra30: Drop write-barrier Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 05/19] PM / devfreq: tegra30: Set up watermarks properly Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 06/19] PM / devfreq: tegra30: Tune up boosting thresholds Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 07/19] PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 08/19] PM / devfreq: tegra30: Ensure that target freq won't overflow Dmitry Osipenko
2019-08-20  0:23   ` Chanwoo Choi
2019-08-20 23:19     ` Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 09/19] PM / devfreq: tegra30: Use kHz units uniformly in the code Dmitry Osipenko
2019-10-01 23:29   ` Chanwoo Choi
2019-10-02 18:26     ` Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 10/19] PM / devfreq: tegra30: Reduce unnecessary interrupts activity Dmitry Osipenko
2019-10-01 23:35   ` Chanwoo Choi [this message]
2019-10-02 18:40     ` Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 11/19] PM / devfreq: tegra30: Use CPUFreq notifier Dmitry Osipenko
2019-10-02  0:02   ` Chanwoo Choi
2019-10-02 14:06     ` Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 12/19] PM / devfreq: tegra30: Move clk-notifier's registration to governor's start Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 13/19] PM / devfreq: tegra30: Reset boosting on startup Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 14/19] PM / devfreq: tegra30: Don't enable consecutive-down interrupt " Dmitry Osipenko
2019-10-02  0:04   ` Chanwoo Choi
2019-08-11 21:23 ` [PATCH v6 15/19] PM / devfreq: tegra30: Constify structs Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 16/19] PM / devfreq: tegra30: Include appropriate header Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 17/19] PM / devfreq: tegra30: Increase sampling period to 16ms Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 18/19] PM / devfreq: tegra30: Support variable polling interval Dmitry Osipenko
2019-10-02  0:18   ` Chanwoo Choi
2019-10-02 15:27     ` Dmitry Osipenko
2019-08-11 21:23 ` [PATCH v6 19/19] PM / devfreq: tegra20/30: Add Dmitry as a maintainer Dmitry Osipenko
2019-10-01 21:15 ` [PATCH v6 00/19] More improvements for Tegra30 devfreq driver Dmitry Osipenko
2019-10-02  0:25   ` Chanwoo Choi
2019-10-02 13:54     ` Dmitry Osipenko
2019-10-05 16:29       ` Peter Geis

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=b67a7878-fa74-df89-9a62-556b9300b5a5@samsung.com \
    --to=cw00.choi@samsung.com \
    --cc=digetx@gmail.com \
    --cc=jonathanh@nvidia.com \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=myungjoo.ham@samsung.com \
    --cc=thierry.reding@gmail.com \
    --cc=tomeu.vizoso@collabora.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

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/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-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

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


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