From e75926d6dc49878682631508c996e472d79a8d41 Mon Sep 17 00:00:00 2001 From: Jeff Brown Date: Thu, 18 Sep 2014 15:24:49 -0700 Subject: [PATCH] Move setting the display state out of the critical section. Setting the display power state currently happens while holding the display manager lock. This change moves it out of the lock to ensure other services don't get stuck for several hundred milliseconds. Unfortunately, surface flinger ends up stalled a little later so this only solves part of the problem. Bug: 17623774 Change-Id: I201137c5e7f82c776f28a436845fcf3191fd0ca5 --- .../android/server/display/DisplayDevice.java | 6 ++- .../server/display/DisplayManagerService.java | 43 +++++++++++++++---- .../server/display/LocalDisplayAdapter.java | 28 ++++++++---- .../server/display/VirtualDisplayAdapter.java | 3 +- 4 files changed, 61 insertions(+), 19 deletions(-) diff --git a/services/core/java/com/android/server/display/DisplayDevice.java b/services/core/java/com/android/server/display/DisplayDevice.java index 16554d34982b3..683212ada6025 100644 --- a/services/core/java/com/android/server/display/DisplayDevice.java +++ b/services/core/java/com/android/server/display/DisplayDevice.java @@ -108,8 +108,12 @@ abstract class DisplayDevice { /** * Sets the display state, if supported. + * + * @return A runnable containing work to be deferred until after we have + * exited the critical section, or null if none. */ - public void requestDisplayStateLocked(int state) { + public Runnable requestDisplayStateLocked(int state) { + return null; } /** diff --git a/services/core/java/com/android/server/display/DisplayManagerService.java b/services/core/java/com/android/server/display/DisplayManagerService.java index e60970177c06c..43c01cdcc621c 100644 --- a/services/core/java/com/android/server/display/DisplayManagerService.java +++ b/services/core/java/com/android/server/display/DisplayManagerService.java @@ -63,6 +63,7 @@ import java.io.FileDescriptor; import java.io.PrintWriter; import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; /** @@ -214,6 +215,11 @@ public final class DisplayManagerService extends SystemService { private final DisplayViewport mTempDefaultViewport = new DisplayViewport(); private final DisplayViewport mTempExternalTouchViewport = new DisplayViewport(); + // Temporary list of deferred work to perform when setting the display state. + // Only used by requestDisplayState. The field is self-synchronized and only + // intended for use inside of the requestGlobalDisplayStateInternal function. + private final ArrayList mTempDisplayStateWorkQueue = new ArrayList(); + public DisplayManagerService(Context context) { super(context); mContext = context; @@ -318,11 +324,26 @@ public final class DisplayManagerService extends SystemService { } private void requestGlobalDisplayStateInternal(int state) { - synchronized (mSyncRoot) { - if (mGlobalDisplayState != state) { - mGlobalDisplayState = state; - updateGlobalDisplayStateLocked(); - scheduleTraversalLocked(false); + synchronized (mTempDisplayStateWorkQueue) { + try { + // Update the display state within the lock. + synchronized (mSyncRoot) { + if (mGlobalDisplayState != state) { + mGlobalDisplayState = state; + updateGlobalDisplayStateLocked(mTempDisplayStateWorkQueue); + scheduleTraversalLocked(false); + } + } + + // Setting the display power state can take hundreds of milliseconds + // to complete so we defer the most expensive part of the work until + // after we have exited the critical section to avoid blocking other + // threads for a long time. + for (int i = 0; i < mTempDisplayStateWorkQueue.size(); i++) { + mTempDisplayStateWorkQueue.get(i).run(); + } + } finally { + mTempDisplayStateWorkQueue.clear(); } } } @@ -669,21 +690,25 @@ public final class DisplayManagerService extends SystemService { scheduleTraversalLocked(false); } - private void updateGlobalDisplayStateLocked() { + private void updateGlobalDisplayStateLocked(List workQueue) { final int count = mDisplayDevices.size(); for (int i = 0; i < count; i++) { DisplayDevice device = mDisplayDevices.get(i); - updateDisplayStateLocked(device); + Runnable runnable = updateDisplayStateLocked(device); + if (runnable != null) { + workQueue.add(runnable); + } } } - private void updateDisplayStateLocked(DisplayDevice device) { + private Runnable updateDisplayStateLocked(DisplayDevice device) { // Blank or unblank the display immediately to match the state requested // by the display power controller (if known). DisplayDeviceInfo info = device.getDisplayDeviceInfoLocked(); if ((info.flags & DisplayDeviceInfo.FLAG_NEVER_BLANK) == 0) { - device.requestDisplayStateLocked(mGlobalDisplayState); + return device.requestDisplayStateLocked(mGlobalDisplayState); } + return null; } // Adds a new logical display based on the given display device. diff --git a/services/core/java/com/android/server/display/LocalDisplayAdapter.java b/services/core/java/com/android/server/display/LocalDisplayAdapter.java index 9c91ab505f2f8..24cf4231cdeb7 100644 --- a/services/core/java/com/android/server/display/LocalDisplayAdapter.java +++ b/services/core/java/com/android/server/display/LocalDisplayAdapter.java @@ -222,19 +222,31 @@ final class LocalDisplayAdapter extends DisplayAdapter { } @Override - public void requestDisplayStateLocked(int state) { + public Runnable requestDisplayStateLocked(final int state) { if (mState != state) { + final int displayId = mBuiltInDisplayId; + final IBinder token = getDisplayTokenLocked(); final int mode = getPowerModeForState(state); - Trace.traceBegin(Trace.TRACE_TAG_POWER, "requestDisplayState(" - + Display.stateToString(state) + ", id=" + mBuiltInDisplayId + ")"); - try { - SurfaceControl.setDisplayPowerMode(getDisplayTokenLocked(), mode); - } finally { - Trace.traceEnd(Trace.TRACE_TAG_POWER); - } mState = state; updateDeviceInfoLocked(); + + // Defer actually setting the display power mode until we have exited + // the critical section since it can take hundreds of milliseconds + // to complete. + return new Runnable() { + @Override + public void run() { + Trace.traceBegin(Trace.TRACE_TAG_POWER, "requestDisplayState(" + + Display.stateToString(state) + ", id=" + displayId + ")"); + try { + SurfaceControl.setDisplayPowerMode(token, mode); + } finally { + Trace.traceEnd(Trace.TRACE_TAG_POWER); + } + } + }; } + return null; } @Override diff --git a/services/core/java/com/android/server/display/VirtualDisplayAdapter.java b/services/core/java/com/android/server/display/VirtualDisplayAdapter.java index a6a9a8972f5d0..c2b4478e5f3c8 100644 --- a/services/core/java/com/android/server/display/VirtualDisplayAdapter.java +++ b/services/core/java/com/android/server/display/VirtualDisplayAdapter.java @@ -188,7 +188,7 @@ final class VirtualDisplayAdapter extends DisplayAdapter { } @Override - public void requestDisplayStateLocked(int state) { + public Runnable requestDisplayStateLocked(int state) { if (state != mDisplayState) { mDisplayState = state; if (state == Display.STATE_OFF) { @@ -197,6 +197,7 @@ final class VirtualDisplayAdapter extends DisplayAdapter { mCallback.dispatchDisplayResumed(); } } + return null; } @Override