Surfaceflinger wastes cpu polling for early suspend when its not present

Bug #1023985 reported by Zach Pfeffer
16
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Linaro Android
Fix Released
Critical
John Stultz

Bug Description

Surface flinger has a thread which it uses to track if the screen is on or off that watches the earlysuspend interfaces found in /sys/power/wait_for_fb_wake and /sys/power/wait_for_fb_sleep, basically in a loop reading from each one.

With 3.3+ kernels, the Android patch set dropped earlysuspend, so these files don't exist. I assume they also have changed their userland, but we'll not see that till Oct/Nov.

Since these interface files don't exist w/ 3.3+ kernels, surface flinger spends a decent amount of time trying to open the two files, printing out an error and trying again.

To try to address this, I found the patch that added the interfaces and stubbed out its dependency on the earlysuspend core, so the files exist and as far as surface flinger is concerned the display is never turned off. This avoids the spinning in userland.

Revision history for this message
Zach Pfeffer (pfefferz) wrote :

Thread:

John Stultz ✆
15:01 (20 hours ago)

to me, Deepak, Jon, Thomas, Amit, Annamalai, rosenkranzer, Kejun, Patrik, Vishal, yong, Zygmunt
On 07/11/2012 12:46 PM, Zach Pfeffer wrote:
On 11 July 2012 14:45, John Stultz <email address hidden> wrote:
On 07/11/2012 12:40 PM, Deepak Saxena wrote:
On 11 July 2012 12:37, Zach Pfeffer <email address hidden> wrote:
Deepak/John

What userspace changes are you looking for?
Basically we want to remove the polling calls from userspace into the
earlysuspend APIs. This is the best long-term solution and we could
maybe just give it the code to Google to integrate into the next
release.
Well, that might be optimistic, as I suspect they already have something in
place.

But basically while I'm working on the kernel shim side, it may be good to
work this from both ends.
What device does it poll?

Looking at Tixy's change to remove the warning messages:
http://review.android.git.linaro.org/#change,1745

See boolDisplayHardwareBase::DisplayEventThread::threadLoop()

http://source-android.frandroid.com/frameworks/base/services/surfaceflinger/DisplayHardware/DisplayHardwareBase.cpp

I'm guessing if we modified Tixy's change to not just return but to sleep for 10 seconds before doing so, it likely will provide similar behavior to what I'm planning.

Changed in linaro-android:
status: New → Fix Committed
importance: Undecided → Critical
assignee: nobody → John Stultz (jstultz)
milestone: none → 12.07
Revision history for this message
Zach Pfeffer (pfefferz) wrote :

Jon Medhurst (Tixy) ✆
01:52 (9 hours ago)

to John, me, Deepak, Thomas, Amit, Annamalai, rosenkranzer, Kejun, Patrik, Vishal, yong, Zygmunt

I'm about to jump into car to drive to ARM's offices for day, but
managed to give it a quick test and I must say the patch seems to work a
treat. :-)

Before patch, surfaceflinger always used at least 20% CPU (i.e. one of
the 5 cores). Now, during the boot animation it uses 4% and when in an
app with unchanging screen contents it uses 0% :-)

A big thanks to everyone involved. Great Work!

Revision history for this message
Zach Pfeffer (pfefferz) wrote :

Origen, Snowball and TI need to pick this up.

John Stultz (jstultz)
summary: - Surfaceflinger times out on early suspend poll causing wasted compute
+ Surfaceflinger wastes cpu polling for early suspend when its not present
Revision history for this message
John Stultz (jstultz) wrote :

Here's the patch I used. It applies on top of my 3.5-rc rebase tree. Should backport without too much trouble (only makefile and Kconfig collisions).

You'll also need to set CONFIG_FB_EARLYSUSPEND on for this patch to work.

You only need this if you're running on 3.3, 3.4 or 3.5 based trees.

Some 3.4 trees may already contain the -compat layer the Google folks provided, however that branch is a little out of date since the current 3.4 tree has included the upstream wakelock implementation for 3.5 (which collides with the -compat earlysuspend).

Revision history for this message
Yann BENIGOT (yann-benigot-w) wrote :

With https://bugs.launchpad.net/linaro-android/+bug/1006751 I used a quick way to fix this without patching the kernel by simply adding a usleep() call in the DisplayEventThread::threadLoop() call, so that the function is only called six times per second. Perhaps the thread should juste be interrupted when FB_EARLYSUSPEND is absent since it does nothing but loop.

It is a dirty fix, but it does the job without needing FB_EARLYSUSPEND.

Revision history for this message
Zach Pfeffer (pfefferz) wrote :

Thanks Yann. I put together a commit http://review.android.git.linaro.org/#change,2431 based on your patch. Its been merged and test builds are building.

vishal (vishalbhoj)
Changed in linaro-android:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.