Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Geis <pgwipeout@gmail.com>
To: Dmitry Osipenko <digetx@gmail.com>
Cc: Chanwoo Choi <cw00.choi@samsung.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>,
	linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 00/19] More improvements for Tegra30 devfreq driver
Date: Sat, 5 Oct 2019 12:29:40 -0400
Message-ID: <CAMdYzYrgvRK49ZhXfy4T6EsRmWOmB+uNK9+rumL1mX032=CdWA@mail.gmail.com> (raw)
In-Reply-To: <6967777e-b54f-8021-aa6d-8c245e529e10@gmail.com>

Tested on the Ouya (tegra30).

Tested-by: Peter Geis <pgwipeout@gmail.com>

On Wed, Oct 2, 2019 at 9:56 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 02.10.2019 03:25, Chanwoo Choi пишет:
> > Hello Dmitry and Thierry,
> >
> > On 19. 10. 2. 오전 6:15, Dmitry Osipenko wrote:
> >> 12.08.2019 00:22, Dmitry Osipenko пишет:
> >>> Hello,
> >>>
> >>> This series addresses some additional review comments that were made by
> >>> Thierry Reding to [1], makes several important changes to the driver,
> >>> fixing excessive interrupts activity, and adds new features. In the end
> >>> I'm proposing myself as a maintainer for the Tegra devfreq drivers.
> >>>
> >>> [1] https://lore.kernel.org/lkml/0fb50eb1-a173-1756-6889-2526a10ac707@gmail.com/T/
> >>>
> >>> Changelog:
> >>>
> >>> v6:  Addressed review comment that was made by Chanwoo Choi to v5 by
> >>>      squashing "Define ACTMON_DEV_CTRL_STOP" patch into the "Use CPUFreq
> >>>      notifier" patch.
> >>>
> >>> v5:  Addressed review comments that were made by Chanwoo Choi to v4 by
> >>>      squashing few patches, dropping some questionable patches, rewording
> >>>      comments to the code, restructuring the code and etc.
> >>>
> >>>      These patches are now dropped from the series:
> >>>
> >>>        PM / devfreq: tegra30: Use tracepoints for debugging
> >>>        PM / devfreq: tegra30: Inline all one-line functions
> >>>
> >>>      The interrupt-optimization patches are squashed into a single patch:
> >>>
> >>>        PM / devfreq: tegra30: Reduce unnecessary interrupts activity
> >>>
> >>>      because it's better to keep the optimizations as a separate change and
> >>>      this also helps to reduce code churning, since the code changes depend
> >>>      on a previous patch in order to stay cleaner.
> >>>
> >>>      Fixed a lockup bug that I spotted recently, which is caused by a
> >>>      clk-notifier->cpufreq_get()->clk_set_rate() sequence. Now a non-blocking
> >>>      variant of CPU's frequency retrieving is used, i.e. cpufreq_quick_get().
> >>>
> >>>      Further optimized the CPUFreq notifier by postponing the delayed
> >>>      updating in accordance to the polling interval, this actually uncovered
> >>>      the above lockup bug.
> >>>
> >>>      Implemented new minor driver feature in the new patch:
> >>>
> >>>        PM / devfreq: tegra30: Support variable polling interval
> >>>
> >>> v4:  Added two new patches to the series:
> >>>
> >>>        PM / devfreq: tegra30: Synchronize average count on target's update
> >>>        PM / devfreq: tegra30: Increase sampling period to 16ms
> >>>
> >>>      The first patch addresses problem where governor could get stuck due
> >>>      to outdated "average count" value which is snapshoted by ISR and there
> >>>      are cases where manual update of the value is required.
> >>>
> >>>      The second patch is just a minor optimization.
> >>>
> >>> v3:  Added support for tracepoints, replacing the debug messages.
> >>>      Fixed few more bugs with the help of tracepoints.
> >>>
> >>>      New patches in this version:
> >>>
> >>>        PM / devfreq: tegra30: Use tracepoints for debugging
> >>>        PM / devfreq: tegra30: Optimize CPUFreq notifier
> >>>        PM / devfreq: tegra30: Optimize upper consecutive watermark selection
> >>>        PM / devfreq: tegra30: Optimize upper average watermark selection
> >>>        PM / devfreq: tegra30: Include appropriate header
> >>>
> >>>      Some of older patches of this series also got some extra minor polish.
> >>>
> >>> v2:  Added more patches that are cleaning driver's code further and
> >>>      squashing another kHz conversion bug.
> >>>
> >>>      The patch "Rework frequency management logic" of the v1 series is now
> >>>      converted to "Set up watermarks properly" because I found some problems
> >>>      in the original patch and then realized that there is no need to change
> >>>      the logic much. So the logic mostly preserved and only got improvements.
> >>>
> >>>      The series is based on the today's linux-next (25 Jun) and takes into
> >>>      account minor changes that MyungJoo Ham made to the already queued
> >>>      patches from the first batch [1].
> >>>
> >>> Dmitry Osipenko (19):
> >>>   PM / devfreq: tegra30: Change irq type to unsigned int
> >>>   PM / devfreq: tegra30: Keep interrupt disabled while governor is
> >>>     stopped
> >>>   PM / devfreq: tegra30: Handle possible round-rate error
> >>>   PM / devfreq: tegra30: Drop write-barrier
> >>>   PM / devfreq: tegra30: Set up watermarks properly
> >>>   PM / devfreq: tegra30: Tune up boosting thresholds
> >>>   PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
> >>>   PM / devfreq: tegra30: Ensure that target freq won't overflow
> >>>   PM / devfreq: tegra30: Use kHz units uniformly in the code
> >>>   PM / devfreq: tegra30: Reduce unnecessary interrupts activity
> >>>   PM / devfreq: tegra30: Use CPUFreq notifier
> >>>   PM / devfreq: tegra30: Move clk-notifier's registration to governor's
> >>>     start
> >>>   PM / devfreq: tegra30: Reset boosting on startup
> >>>   PM / devfreq: tegra30: Don't enable consecutive-down interrupt on
> >>>     startup
> >>>   PM / devfreq: tegra30: Constify structs
> >>>   PM / devfreq: tegra30: Include appropriate header
> >>>   PM / devfreq: tegra30: Increase sampling period to 16ms
> >>>   PM / devfreq: tegra30: Support variable polling interval
> >>>   PM / devfreq: tegra20/30: Add Dmitry as a maintainer
> >>>
> >>>  MAINTAINERS                       |   9 +
> >>>  drivers/devfreq/tegra30-devfreq.c | 706 +++++++++++++++++++++++-------
> >>>  2 files changed, 555 insertions(+), 160 deletions(-)
> >>>
> >>
> >> Hello Chanwoo,
> >>
> >> I don't have any more updates in regards to this series, everything is
> >> working flawlessly for now. Will be awesome if we could continue the
> >> reviewing and then get the patches into linux-next to get some more testing.
> >>
> >>
> >
> > Hello Dmitry,
> >
> > I'm sorry for late reply. Except for patch5, I reviewed the patches.
> > Please check my comment. Actually, It is difficult to review the patch5
> > without any testing environment and detailed knowledge of watermark of tegra.
> > It is not familiar with me.
>
> Thank you very much! I'll go through yours comments and reply to them.
>
> I understand that it's not easy for you to review patch5, but probably
> you don't need to go into details and a brief-generic review of the code
> will be enough in that case.
>
> The hardware is actually very simple, there are watermarks that
> correspond to a memory activity that hardware accounts over a given
> period of time. Once watermark is reached, hardware generates interrupt.
> There are two types of watermarks: average and consecutive. In case of
> the average, the memory activity is collected over a larger window of
> time. For the consecutive case, the memory activity is collected over
> each period (16ms by default in the driver). Memory client may breach
> average watermark very frequently, although that may not affect much the
> average value and for some memory clients (like CPU) it is more
> preferred to not completely ignore those short bursts of memory
> activity. The consecutive watermarks are used in order to detect those
> short bursts, which we account in the driver in a form of boosting. You
> may notice that boost_up_coeff for the CPU's memory client is set to a
> higher value in the driver.
>
> > Hello Thierry,
> > If possible, Could you review the patch5 related to setting up the watermark
> > and other patches?
> >
>
> Indeed, will be very nice if Thierry could also take a look at this
> series. Although.. I could be wrong here, but it looks to me that
> Thierry also isn't closely familiar with this driver and the hardware.
>
> Thierry, at least please let us know if you're interested in taking a
> look at the patches, I'm pretty sure that you're quite busy with other
> things ;)

      reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-11 21:22 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
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 [this message]

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='CAMdYzYrgvRK49ZhXfy4T6EsRmWOmB+uNK9+rumL1mX032=CdWA@mail.gmail.com' \
    --to=pgwipeout@gmail.com \
    --cc=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