From d1ada44ddb39b1c69e35e35965acaaa88bb45356 Mon Sep 17 00:00:00 2001 From: chaviw Date: Thu, 2 Jun 2022 11:33:17 -0500 Subject: [PATCH] Prevent windows from drawing if they're in an active sync set If a window is in a sync, but it already drew it's sync buffer, it normally would be allowed to continue processing draws and just block when it runs out of buffers. It would unblock once the sync is complete at the sync buffer was sent to SF. However, this has a few issues 1. In some cases, WMS wants to retry the draw again due to configuration changes. This could cause deadlocks because the client may already be blocked since there are no more buffers. Instead, the client will block and WMS would be responsible for dropping the last buffer and notifying the client that it can draw for a sync again 2. If there are multiple UI threads, each can block each other due to the shared Render Thread. If one of the windows in the sync continues to draw and then blocks RT due to no more buffers, all other UI threads are unable to draw. If the behavior in 1 is desired, we'd need to make sure no one in the same sync continues to draw until everyone is complete. This is to ensure dropping will actually allow another render to occur. If other windows are blocking the RT, then even with dropping, the requested sync window cannot get another buffer and will cause a deadlock. Test: Current sync requests work Bug: 233625646 Change-Id: I3a0a4f36bf61a6677507672578e447060b806651 --- core/java/android/view/IWindowSession.aidl | 5 +++ core/java/android/view/ViewRootImpl.java | 32 ++++++++++++++++++- .../android/view/WindowManagerGlobal.java | 5 +++ .../android/view/WindowlessWindowManager.java | 5 +++ .../java/com/android/server/wm/Session.java | 5 +++ .../server/wm/WindowManagerService.java | 21 ++++++++++++ .../com/android/server/wm/WindowState.java | 8 +++++ 7 files changed, 80 insertions(+), 1 deletion(-) diff --git a/core/java/android/view/IWindowSession.aidl b/core/java/android/view/IWindowSession.aidl index 649accd1126dd..8954a1eaf1469 100644 --- a/core/java/android/view/IWindowSession.aidl +++ b/core/java/android/view/IWindowSession.aidl @@ -384,4 +384,9 @@ interface IWindowSession { * Clears a touchable region set by {@link #setInsets}. */ void clearTouchableRegion(IWindow window); + + /** + * Returns whether this window needs to cancel draw and retry later. + */ + boolean cancelDraw(IWindow window); } diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index bf00b16cb61b1..25d6a8814f8af 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -84,6 +84,7 @@ import static android.view.WindowManager.LayoutParams.TYPE_STATUS_BAR_ADDITIONAL import static android.view.WindowManager.LayoutParams.TYPE_SYSTEM_ALERT; import static android.view.WindowManager.LayoutParams.TYPE_TOAST; import static android.view.WindowManager.LayoutParams.TYPE_VOLUME_OVERLAY; +import static android.view.WindowManagerGlobal.RELAYOUT_RES_CANCEL_AND_REDRAW; import static android.view.WindowManagerGlobal.RELAYOUT_RES_CONSUME_ALWAYS_SYSTEM_BARS; import static android.view.WindowManagerGlobal.RELAYOUT_RES_SURFACE_CHANGED; import static android.view.inputmethod.InputMethodEditorTraceProto.InputMethodClientsTraceProto.ClientSideProto.IME_FOCUS_CONTROLLER; @@ -601,6 +602,14 @@ public final class ViewRootImpl implements ViewParent, */ private boolean mSyncBuffer = false; + /** + * Flag to determine whether the client needs to check with WMS if it can draw. WMS will notify + * the client that it can't draw if we're still in the middle of a sync set that includes this + * window. Once the sync is complete, the window can resume drawing. This is to ensure we don't + * deadlock the client by trying to request draws when there may not be any buffers available. + */ + private boolean mCheckIfCanDraw = false; + int mSyncSeqId = 0; int mLastSyncSeqId = 0; @@ -2694,6 +2703,9 @@ public final class ViewRootImpl implements ViewParent, mIsInTraversal = true; mWillDrawSoon = true; + boolean cancelDraw = false; + boolean isSyncRequest = false; + boolean windowSizeMayChange = false; WindowManager.LayoutParams lp = mWindowAttributes; @@ -2963,6 +2975,8 @@ public final class ViewRootImpl implements ViewParent, mViewFrameInfo.flags |= FrameInfo.FLAG_WINDOW_VISIBILITY_CHANGED; } relayoutResult = relayoutWindow(params, viewVisibility, insetsPending); + cancelDraw = (relayoutResult & RELAYOUT_RES_CANCEL_AND_REDRAW) + == RELAYOUT_RES_CANCEL_AND_REDRAW; final boolean dragResizing = mPendingDragResizing; if (mSyncSeqId > mLastSyncSeqId) { mLastSyncSeqId = mSyncSeqId; @@ -2971,6 +2985,7 @@ public final class ViewRootImpl implements ViewParent, } reportNextDraw(); mSyncBuffer = true; + isSyncRequest = true; } final boolean surfaceControlChanged = @@ -3259,6 +3274,19 @@ public final class ViewRootImpl implements ViewParent, } } } else { + // If a relayout isn't going to happen, we still need to check if this window can draw + // when mCheckIfCanDraw is set. This is because it means we had a sync in the past, but + // have not been told by WMS that the sync is complete and that we can continue to draw + if (mCheckIfCanDraw) { + try { + cancelDraw = mWindowSession.cancelDraw(mWindow); + if (DEBUG_BLAST) { + Log.d(mTag, "cancelDraw returned " + cancelDraw); + } + } catch (RemoteException e) { + } + } + // Not the first pass and no window/insets/visibility change but the window // may have moved and we need check that and if so to update the left and right // in the attach info. We translate only the window frame since on window move @@ -3477,7 +3505,9 @@ public final class ViewRootImpl implements ViewParent, reportNextDraw(); } - boolean cancelAndRedraw = mAttachInfo.mTreeObserver.dispatchOnPreDraw(); + mCheckIfCanDraw = isSyncRequest || cancelDraw; + + boolean cancelAndRedraw = mAttachInfo.mTreeObserver.dispatchOnPreDraw() || cancelDraw; if (!cancelAndRedraw) { createSyncIfNeeded(); } diff --git a/core/java/android/view/WindowManagerGlobal.java b/core/java/android/view/WindowManagerGlobal.java index 85a5762f7f3d0..25445abefca20 100644 --- a/core/java/android/view/WindowManagerGlobal.java +++ b/core/java/android/view/WindowManagerGlobal.java @@ -82,6 +82,11 @@ public final class WindowManagerGlobal { */ public static final int RELAYOUT_RES_CONSUME_ALWAYS_SYSTEM_BARS = 1 << 3; + /** + * The window manager has told the window it cannot draw this frame and should retry again. + */ + public static final int RELAYOUT_RES_CANCEL_AND_REDRAW = 1 << 4; + /** * Flag for relayout: the client will be later giving * internal insets; as a result, the window will not impact other window diff --git a/core/java/android/view/WindowlessWindowManager.java b/core/java/android/view/WindowlessWindowManager.java index a212348a2315a..5832347c1947c 100644 --- a/core/java/android/view/WindowlessWindowManager.java +++ b/core/java/android/view/WindowlessWindowManager.java @@ -552,4 +552,9 @@ public class WindowlessWindowManager implements IWindowSession { } } } + + @Override + public boolean cancelDraw(IWindow window) { + return false; + } } diff --git a/services/core/java/com/android/server/wm/Session.java b/services/core/java/com/android/server/wm/Session.java index 30b50839cd35d..8d03e2e9632ab 100644 --- a/services/core/java/com/android/server/wm/Session.java +++ b/services/core/java/com/android/server/wm/Session.java @@ -249,6 +249,11 @@ class Session extends IWindowSession.Stub implements IBinder.DeathRecipient { mService.setWillReplaceWindows(appToken, childrenOnly); } + @Override + public boolean cancelDraw(IWindow window) { + return mService.cancelDraw(this, window); + } + @Override public int relayout(IWindow window, WindowManager.LayoutParams attrs, int requestedWidth, int requestedHeight, int viewFlags, int flags, diff --git a/services/core/java/com/android/server/wm/WindowManagerService.java b/services/core/java/com/android/server/wm/WindowManagerService.java index a48140eb0e046..236befc661ce3 100644 --- a/services/core/java/com/android/server/wm/WindowManagerService.java +++ b/services/core/java/com/android/server/wm/WindowManagerService.java @@ -89,6 +89,7 @@ import static android.view.WindowManager.REMOVE_CONTENT_MODE_UNDEFINED; import static android.view.WindowManager.TRANSIT_NONE; import static android.view.WindowManager.TRANSIT_RELAUNCH; import static android.view.WindowManagerGlobal.ADD_OKAY; +import static android.view.WindowManagerGlobal.RELAYOUT_RES_CANCEL_AND_REDRAW; import static android.view.WindowManagerGlobal.RELAYOUT_RES_SURFACE_CHANGED; import static android.view.WindowManagerPolicyConstants.NAV_BAR_INVALID; import static android.view.WindowManagerPolicyConstants.TYPE_LAYER_MULTIPLIER; @@ -2212,6 +2213,20 @@ public class WindowManagerService extends IWindowManager.Stub == PackageManager.PERMISSION_GRANTED; } + /** + * Returns whether this window can proceed with drawing or needs to retry later. + */ + public boolean cancelDraw(Session session, IWindow client) { + synchronized (mGlobalLock) { + final WindowState win = windowForClientLocked(session, client, false); + if (win == null) { + return false; + } + + return win.cancelAndRedraw(); + } + } + public int relayoutWindow(Session session, IWindow client, LayoutParams attrs, int requestedWidth, int requestedHeight, int viewVisibility, int flags, ClientWindowFrames outFrames, MergedConfiguration mergedConfiguration, @@ -2228,6 +2243,11 @@ public class WindowManagerService extends IWindowManager.Stub if (win == null) { return 0; } + + if (win.cancelAndRedraw()) { + result |= RELAYOUT_RES_CANCEL_AND_REDRAW; + } + final DisplayContent displayContent = win.getDisplayContent(); final DisplayPolicy displayPolicy = displayContent.getDisplayPolicy(); @@ -2530,6 +2550,7 @@ public class WindowManagerService extends IWindowManager.Stub win.mLastSeqIdSentToRelayout = win.mSyncSeqId; outSyncIdBundle.putInt("seqid", win.mSyncSeqId); + win.mAlreadyRequestedSync = true; } else { outSyncIdBundle.putInt("seqid", -1); } diff --git a/services/core/java/com/android/server/wm/WindowState.java b/services/core/java/com/android/server/wm/WindowState.java index 70499603801cc..132a0a4c596d1 100644 --- a/services/core/java/com/android/server/wm/WindowState.java +++ b/services/core/java/com/android/server/wm/WindowState.java @@ -391,6 +391,7 @@ class WindowState extends WindowContainer implements WindowManagerP */ int mSyncSeqId = 0; int mLastSeqIdSentToRelayout = 0; + boolean mAlreadyRequestedSync; /** * {@code true} when the client was still drawing for sync when the sync-set was finished or @@ -4412,6 +4413,8 @@ class WindowState extends WindowContainer implements WindowManagerP pw.println(prefix + "Requested visibilities: " + visibilityString); } } + + pw.println(prefix + "mAlreadyRequestedSync=" + mAlreadyRequestedSync); } @Override @@ -5951,6 +5954,7 @@ class WindowState extends WindowContainer implements WindowManagerP if (mSyncState == SYNC_STATE_WAITING_FOR_DRAW && mRedrawForSyncReported) { mClientWasDrawingForSync = true; } + mAlreadyRequestedSync = false; super.finishSync(outMergedTransaction, cancel); } @@ -6217,4 +6221,8 @@ class WindowState extends WindowContainer implements WindowManagerP @WindowTraceLogLevel int logLevel) { dumpDebug(proto, fieldId, logLevel); } + + public boolean cancelAndRedraw() { + return mSyncState != SYNC_STATE_NONE && mAlreadyRequestedSync; + } }