exynos/gsc: implement dynamic clock gating [chromiumos/third_party/kernel : chromeos-3.4]

37 views
Skip to first unread message

Prathyush Kalashwaram (Code Review)

unread,
Oct 11, 2012, 5:38:30 AM10/11/12
to Alim Akhtar, Olof Johansson, Mandeep Singh Baines
Hello Alim Akhtar,

I'd like you to do a code review. Please visit

https://gerrit.chromium.org/gerrit/35262

to review the following change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................

exynos/gsc: implement dynamic clock gating

This patch adds aggressive clock gating support to gscalar driver.
The gscalar clock is turned off whenever not in use (in between
video decoding of frames) thus ensuring maximum power saving. The
sysmmu of gscalar is also turned off and on along with the parent
device.

BUG=chrome-os-partner:15149
TEST=Booted on snow and played video. Tested with multiple S2Rs. Checked
clock gating registers when video is paused.
Before: (gsc) armio 0x10020920 -> fffff8f1, (mfc) armio 0x1002092c -> ffffffff
After: (gsc) armio 0x10020920 -> fffff870, (mfc) armio 0x1002092c -> fffffff8

Change-Id: I76df07c326964d670630e3a434daba6f209db964
Signed-off-by: Prathyush K <prath...@samsung.com>
Signed-off-by: Alim Akhtar <alim....@samsung.com>
---
M drivers/media/video/exynos/gsc/gsc-core.c
M drivers/media/video/exynos/gsc/gsc-core.h
M drivers/media/video/exynos/gsc/gsc-m2m.c
3 files changed, 33 insertions(+), 14 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/kernel refs/changes/62/35262/1
--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>

Prathyush Kalashwaram (Code Review)

unread,
Oct 11, 2012, 5:45:36 AM10/11/12
to Doug Anderson, Grant Grundler, Pawel Osciak, Prashanth Godrehal, Alim Akhtar, Naveen Krishna, Shirish S
Prathyush Kalashwaram has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 1: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Grant Grundler <grun...@google.com>
Gerrit-Reviewer: Naveen Krishna <naveenkr...@gmail.com>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Shirish S <shi...@chromium.org>

Prashanth Godrehal (Code Review)

unread,
Oct 11, 2012, 6:21:13 AM10/11/12
to Prathyush Kalashwaram, Doug Anderson, Grant Grundler, Pawel Osciak, Alim Akhtar, Naveen Krishna, Shirish S
Prashanth Godrehal has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 1:

In the test field update also the power saving with these patches when we run the full HD video? Because that is the main requirement for these patches

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>

Alim Akhtar (Code Review)

unread,
Oct 11, 2012, 9:04:30 AM10/11/12
to Prathyush Kalashwaram, Doug Anderson, Grant Grundler, Pawel Osciak, Prashanth Godrehal, Naveen Krishna, Shirish S
Alim Akhtar has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 1: Looks good to me, but someone else must approve

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>

Todd Broch (Code Review)

unread,
Oct 19, 2012, 6:36:19 PM10/19/12
to Prathyush Kalashwaram, Grant Grundler, Bryan Freed, Doug Anderson, Pawel Osciak, Prashanth Godrehal, Alim Akhtar, Naveen Krishna, Shirish S
Todd Broch has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 1: Verified; Looks good to me, but someone else must approve

(1 inline comment)

Optional nits.

I've verified the set of 4 patches to enable power gating. Getting similar power-savings as Prathyush (~130mW for 1080p full-screen). See tracker for details.

+1 for now ... will +2 early next week to give other reviewers a little time to visit as well.

....................................................
File drivers/media/video/exynos/gsc/gsc-core.c
Line 1094: void gsc_clock_gating(struct gsc_dev *gsc, enum gsc_clk_status status)
Nit, declare above first use

Nit, find the naming counter-intuitive. Gating means clock should be off but status of GSC_CLK_ON turns clock on. Why not just gsc_clock

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>
Gerrit-Reviewer: Bryan Freed <bfr...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Grant Grundler <grun...@chromium.org>
Gerrit-Reviewer: Naveen Krishna <naveenkr...@gmail.com>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Shirish S <shi...@chromium.org>
Gerrit-Reviewer: Todd Broch <tbr...@chromium.org>

Bryan Freed (Code Review)

unread,
Oct 26, 2012, 7:27:10 PM10/26/12
to Prathyush Kalashwaram, Grant Grundler, Doug Anderson, Todd Broch, Pawel Osciak, Prashanth Godrehal, Alim Akhtar, Naveen Krishna, Shirish S
Bryan Freed has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 1: Do not submit

(1 inline comment)

....................................................
File drivers/media/video/exynos/gsc/gsc-core.c
Line 1094: void gsc_clock_gating(struct gsc_dev *gsc, enum gsc_clk_status status)
This function does not look right to me.
It looks like it tries to support multiple GSC_CLK_ON calls followed by an equal number of GSC_CLK_OFF calls, and that the first _ON turns on the clock while the last off turns off the clock.
But this is not the case. It actually only hits atomic_inc() the first time, so the first _OFF will turn off the clock.
Also, the use of local variable clk_cnt makes decisions based on the clk_cnt field non-atomic. That is, different threads hitting this function at the same time can lead to problems.
I recommend we split this function in two: gsc_clock_enable() and gsc_clock_disable() and use the atomic_{inc|dec}_and_test() macros to protect our decisions.
Even that may not be enough. Is the case of _OFF and _ON at the same time possible? If so, and clk_cnt==1, it is a race to see which thread sets the state.
Bottom line, we need to try again on this.

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>

Prathyush Kalashwaram (Code Review)

unread,
Oct 31, 2012, 9:32:37 AM10/31/12
to Grant Grundler, Bryan Freed, Doug Anderson, Todd Broch, Pawel Osciak, Prashanth Godrehal, Alim Akhtar, Naveen Krishna, Shirish S, Gerrit
Prathyush Kalashwaram has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 2: Verified

Reworked the clock gating method used in the previous patch. Clock on/off calls are done only during gsc_m2m_run and irq_handler and not in any of the pm calls. These two functions cannot be called together so no locking is necessary. (Updated the commit message)
This is working correctly as expected.

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>
Gerrit-Reviewer: Bryan Freed <bfr...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>

Prathyush Kalashwaram (Code Review)

unread,
Nov 2, 2012, 8:07:09 AM11/2/12
to Grant Grundler, Bryan Freed, Doug Anderson, Todd Broch, Pawel Osciak, Prashanth Godrehal, Alim Akhtar, Naveen Krishna, Shirish S, Gerrit
Prathyush Kalashwaram has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 3: Verified

Reordered patches as suggested by Doug.

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>

Alim Akhtar (Code Review)

unread,
Nov 3, 2012, 2:13:01 AM11/3/12
to Prathyush Kalashwaram, Grant Grundler, Bryan Freed, Doug Anderson, Todd Broch, Pawel Osciak, Prashanth Godrehal, Naveen Krishna, Shirish S, Gerrit, Sean Paul, Stéphane Marchesin
Alim Akhtar has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 3: Looks good to me, but someone else must approve

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>
Gerrit-Reviewer: Bryan Freed <bfr...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Grant Grundler <grun...@chromium.org>
Gerrit-Reviewer: Naveen Krishna <naveenkr...@gmail.com>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Sean Paul <sean...@chromium.org>
Gerrit-Reviewer: Shirish S <shi...@chromium.org>
Gerrit-Reviewer: Stéphane Marchesin <mar...@chromium.org>
Gerrit-Reviewer: Todd Broch <tbr...@chromium.org>

Pawel Osciak (Code Review)

unread,
Nov 5, 2012, 2:18:49 PM11/5/12
to Prathyush Kalashwaram, Grant Grundler, Bryan Freed, Doug Anderson, Todd Broch, Pawel Osciak, Prashanth Godrehal, Alim Akhtar, Naveen Krishna, Shirish S, Gerrit, Sean Paul, Stéphane Marchesin
Pawel Osciak has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 3: Do not submit

This patch turns sysmmu off and on every video frame. What is the performance impact of this?

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>
Gerrit-Reviewer: Bryan Freed <bfr...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Grant Grundler <grun...@chromium.org>
Gerrit-Reviewer: Naveen Krishna <naveenkr...@gmail.com>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@google.com>

Prathyush Kalashwaram (Code Review)

unread,
Nov 6, 2012, 7:09:31 AM11/6/12
to Grant Grundler, Bryan Freed, Doug Anderson, Todd Broch, Pawel Osciak, Prashanth Godrehal, Alim Akhtar, Naveen Krishna, Shirish S, Gerrit, Sean Paul, Stéphane Marchesin, Pawel Osciak, kiran
Prathyush Kalashwaram has posted comments on this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 3:

@Pawel, I measured the FPS of video playback (1080p) - both single and triple instance and I did not see any decrease in FPS with and without this patch. Infact, the performance seems to improve a bit for single instance.
Before patches:
single instance: 48-50 FPS
triple instance: 33-34 FPS
After patches:
single instance: 50-52 FPS
triple instance: 33-34 FPS

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Alim Akhtar <alim....@samsung.com>
Gerrit-Reviewer: Bryan Freed <bfr...@chromium.org>
Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Grant Grundler <grun...@chromium.org>
Gerrit-Reviewer: Naveen Krishna <naveenkr...@gmail.com>
Gerrit-Reviewer: Pawel Osciak <pos...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@google.com>
Gerrit-Reviewer: Prashanth Godrehal <prash...@samsung.com>
Gerrit-Reviewer: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Sean Paul <sean...@chromium.org>
Gerrit-Reviewer: Shirish S <shi...@chromium.org>
Gerrit-Reviewer: Stéphane Marchesin <mar...@chromium.org>
Gerrit-Reviewer: Todd Broch <tbr...@chromium.org>
Gerrit-Reviewer: kiran <ki...@chromium.org>

Prathyush Kalashwaram (Code Review)

unread,
Dec 23, 2012, 10:41:58 PM12/23/12
to Bryan Freed, Pawel Osciak
Prathyush Kalashwaram has abandoned this change.

Change subject: exynos/gsc: implement dynamic clock gating
......................................................................


Patch Set 3: Abandoned

Abandoning for now.

--
To view, visit https://gerrit.chromium.org/gerrit/35262
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I76df07c326964d670630e3a434daba6f209db964
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Prathyush Kalashwaram <prat...@chromium.org>
Gerrit-Reviewer: Bryan Freed <bfr...@chromium.org>
Gerrit-Reviewer: Pawel Osciak <pos...@google.com>
Reply all
Reply to author
Forward
0 new messages