Merge "WindowManager: Gate pending-transaction behind sync" into tm-dev

services/core/java/com/android/server/wm/BLASTSync.md

@@ -106,3 +106,88 @@ to the client via ClientTransaction), we haven't even incremented the seqId yet,
 at the same time as the state? We solve this by pushing all client communication through a handler thread that has to
 acquire the lock. This ensures we uphold requirement 2.
     
+= Transaction ordering =
+
+Applying transactions from different process, and in to different server side transaction queues
+raises various questions about transaction ordering. There are two tricky questions in this
+domain that we address here:
+    1. The ordering of Transactions from a single BLASTBufferQueue wrt to eachother
+    2. The ordering of non synced WM updates to syncable state, wrt a BLASTSyncEngine
+       transaction
+       
+== Ordering of Transactions in a single BBQ ==
+
+We can see if sync is never involved, there are never any real questions about ordering.
+Even using one-way setTransactionState, the calls are from a single thread to a single
+interface on another, and will be ordered. When we hand out transactions for sync
+is where issues can start to arise. Obviously if we apply another transaction
+immediately after handing out the sync transaction it could arrive first, and this would
+cause an ordering issue. It's also possible that the sync transaction arrives before the
+transaction applied before it (since setTransactionState is one way from BBQ). Even if
+the transactions are applied in the right order, it's possible for them to be
+commited out of sync, as the BBQ and SyncConsumer may be using different apply tokens
+resulting in the transaction being placed in different server side queues.
+
+To solve these issues, we use a scheme involving "barrier transactions". We show how
+this scheme handles every permutation of (Sync, NotSync).
+
+1. NotSync, NotSync: This is the trivial case. As long as both buffers are submitted from
+   the same thread, they will arrive in SF in order, and SF will place them in the same
+   queue (since there is a shared apply token), which it will process in order.
+2. Sync, NotSync: For sync transactions we register a commit callback, and require it to
+   be fired before applying the next transaction. Since the commit callback is only
+   fired when the transaction has latched on the server side, any transaction applied
+   later, must occur later.
+3. Sync, Sync: Ordering of consecutive sync transactions is delegated to the sync
+    consumer
+4. NotSync, Sync: This is the trickiest case, as we don't want to set a commit callback
+   on not sync transactions (as this would incur an unacceptable performance overhead).
+   Further complicating matters, we want to apply them with one-way binder, since
+   this is the hot path for all graphical updates. To solve this we use a
+   "setBufferHasBarrier" system. Sync transactions (before they are handed out)
+   are tagged with a barrier, referring to the frame number of the last
+   non sync transaction. SurfaceFlinger will ensure the correct ordering
+   by stalling transactions in the queue until barriers are fulfilled. This barrier
+   primitive is dangerous, because it could result in deadlocking, but its ok in
+   this scenario since only BBQ uses its apply token.
+   
+We can see from this that all frames are in order.
+
+== Ordering of WM updates to syncable state ==
+
+A second interesting question, is about the ordering of WM updates to syncable-state. In
+the WM we frequently write code like
+    getPendingTransaction/getSyncTransaction().show(mSurfaceControl).
+In normal operation, getSyncTransaction and getPendingTransaction both refer to the same
+transaction which is the displaycontent pendingtransaction, applied by the WindowManager.
+During sync, each syncing container will instead use its mSyncTransaction, which will
+eventually be merged to be applied by the WindowManager. We can see that we have a congruent
+"problem" to BLASTBufferQueue transaction ordering. Namely, earlier state updates which were
+given to SysUI could end up being applied later than state updates which were made later but
+directly applied by the WM. We can break this down in to two cases:
+=== getPendingTransaction() and getSyncTransaction() ordering ===
+It's possible for code like this to occur:
+getSyncTransaction().OP1
+getPendingTransaction().OP2
+applyPendingTransaction
+applySyncTransaction
+
+in this case the change in OP2 was made later, but occurs first. We define this case as
+intended behavior, and say there is no ordering guarantee between the pending
+and sync transactions. This means the pending transaction is only useful in
+some marginal cases and should probably be considered a deprecated primitive.
+=== getSyncTransaction() and getSyncTransaction() ordering ===
+However, we take great care to ensure usage of getSyncTransaction() reflects
+the ordering the developer would expect. BLASTSyncEngine will register
+a commit callback on all transactions it hands out. In the interval between
+receiving this commit callback and sending out the transaction, we may have other
+data enter getSyncTransaction. If we returned the pending transaction
+(as we do in normal time), then we could create ordering issues, since the pending
+transactions ordering is undefined. Instead we continue to return the sync transaction
+during this interval. If no second sync has started by the time we receive
+the commit callback, then we directly apply this left over data in the sync transaction
+guaranteed it will be ordered correctly, and return to using the pending
+transaction. If a second sync has started, then we just allow the data
+to persist in the mSyncTransaction, potentially to be overwritten
+by the new sync. It will eventually apply with SysUI's apply token and
+ordering will be maintained.

services/core/java/com/android/server/wm/BLASTSyncEngine.java

@@ -19,6 +19,7 @@ package com.android.server.wm;
 import static android.os.Trace.TRACE_TAG_WINDOW_MANAGER;
 
 import static com.android.internal.protolog.ProtoLogGroup.WM_DEBUG_SYNC_ENGINE;
+import static com.android.server.wm.WindowManagerService.H.WINDOW_STATE_BLAST_SYNC_TIMEOUT;
 
 import android.annotation.NonNull;
 import android.os.Trace;
@@ -147,6 +148,33 @@ class BLASTSyncEngine {
             for (WindowContainer wc : mRootMembers) {
                 wc.finishSync(merged, false /* cancel */);
             }
+
+            final ArraySet<WindowContainer> wcAwaitingCommit = new ArraySet<>();
+            for (WindowContainer wc : mRootMembers) {
+                wc.waitForSyncTransactionCommit(wcAwaitingCommit);
+            }
+            final Runnable callback = new Runnable() {
+                // Can run a second time if the action completes after the timeout.
+                boolean ran = false;
+                public void run() {
+                    synchronized (mWm.mGlobalLock) {
+                        if (ran) {
+                            return;
+                        }
+                        mWm.mH.removeCallbacks(this);
+                        ran = true;
+                        SurfaceControl.Transaction t = new SurfaceControl.Transaction();
+                        for (WindowContainer wc : wcAwaitingCommit) {
+                            wc.onSyncTransactionCommitted(t);
+                        }
+                        t.apply();
+                        wcAwaitingCommit.clear();
+                    }
+                }
+            };
+            merged.addTransactionCommittedListener((r) -> { r.run(); }, callback::run);
+            mWm.mH.postDelayed(callback, WINDOW_STATE_BLAST_SYNC_TIMEOUT);
+
             Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER, "onTransactionReady");
             mListener.onTransactionReady(mSyncId, merged);
             Trace.traceEnd(TRACE_TAG_WINDOW_MANAGER);

services/core/java/com/android/server/wm/WindowContainer.java

@@ -236,6 +236,8 @@ class WindowContainer<E extends WindowContainer> extends ConfigurationContainer<
      */
     private boolean mCommittedReparentToAnimationLeash;
 
+    private int mSyncTransactionCommitCallbackDepth = 0;
+
     /** Interface for {@link #isAnimating} to check which cases for the container is animating. */
     public interface AnimationFlags {
         /**
@@ -2688,6 +2690,9 @@ class WindowContainer<E extends WindowContainer> extends ConfigurationContainer<
      * {@link #getPendingTransaction()}
      */
     public Transaction getSyncTransaction() {
+        if (mSyncTransactionCommitCallbackDepth > 0) {
+            return mSyncTransaction;
+        }
         if (mSyncState != SYNC_STATE_NONE) {
             return mSyncTransaction;
         }
@@ -3909,4 +3914,29 @@ class WindowContainer<E extends WindowContainer> extends ConfigurationContainer<
             p.updateOverlayInsetsState(originalChange);
         }
     }
+
+    void waitForSyncTransactionCommit(ArraySet<WindowContainer> wcAwaitingCommit) {
+        if (wcAwaitingCommit.contains(this)) {
+            return;
+        }
+        mSyncTransactionCommitCallbackDepth++;
+        wcAwaitingCommit.add(this);
+
+        for (int i = mChildren.size() - 1; i >= 0; --i) {
+            mChildren.get(i).waitForSyncTransactionCommit(wcAwaitingCommit);
+        }
+    }
+
+    void onSyncTransactionCommitted(SurfaceControl.Transaction t) {
+        mSyncTransactionCommitCallbackDepth--;
+        if (mSyncTransactionCommitCallbackDepth > 0) {
+            return;
+        }
+        if (mSyncState != SYNC_STATE_NONE) {
+            return;
+        }
+
+        t.merge(mSyncTransaction);
+    }
+
 }