Merge "LogicalDisplay.updateDisplayGroupIdLocked should not update DisplayInfo directly" into udc-dev

services/core/java/com/android/server/display/LogicalDisplay.java

@@ -23,7 +23,6 @@ import android.annotation.Nullable;
 import android.graphics.Point;
 import android.graphics.Rect;
 import android.hardware.display.DisplayManagerInternal;
-import android.text.TextUtils;
 import android.util.ArraySet;
 import android.util.SparseArray;
 import android.view.Display;
@@ -333,6 +332,10 @@ final class LogicalDisplay {
         return mPrimaryDisplayDevice != null;
     }
 
+    boolean isDirtyLocked() {
+        return mDirty;
+    }
+
     /**
      * Updates the {@link DisplayGroup} to which the logical display belongs.
      *
@@ -341,8 +344,7 @@ final class LogicalDisplay {
     public void updateDisplayGroupIdLocked(int groupId) {
         if (groupId != mDisplayGroupId) {
             mDisplayGroupId = groupId;
-            mBaseDisplayInfo.displayGroupId = groupId;
+            mDirty = true;
-            mInfo.set(null);
         }
     }
 
@@ -932,18 +934,6 @@ final class LogicalDisplay {
         return mDisplayGroupName;
     }
 
-    /**
-     * Returns whether a display group other than the default display group needs to be assigned.
-     *
-     * <p>If display group name is empty or {@code Display.FLAG_OWN_DISPLAY_GROUP} is set, the
-     * display is assigned to the default display group.
-     */
-    public boolean needsOwnDisplayGroupLocked() {
-        DisplayInfo info = getDisplayInfoLocked();
-        return (info.flags & Display.FLAG_OWN_DISPLAY_GROUP) != 0
-                || !TextUtils.isEmpty(mDisplayGroupName);
-    }
-
     public void dumpLocked(PrintWriter pw) {
         pw.println("mDisplayId=" + mDisplayId);
         pw.println("mIsEnabled=" + mIsEnabled);

services/core/java/com/android/server/display/LogicalDisplayMapper.java

@@ -679,7 +679,9 @@ class LogicalDisplayMapper implements DisplayDeviceRepository.Listener {
         for (int i = mLogicalDisplays.size() - 1; i >= 0; i--) {
             final int displayId = mLogicalDisplays.keyAt(i);
             LogicalDisplay display = mLogicalDisplays.valueAt(i);
+            assignDisplayGroupLocked(display);
 
+            boolean wasDirty = display.isDirtyLocked();
             mTempDisplayInfo.copyFrom(display.getDisplayInfoLocked());
             display.getNonOverrideDisplayInfoLocked(mTempNonOverrideDisplayInfo);
 
@@ -713,19 +715,14 @@ class LogicalDisplayMapper implements DisplayDeviceRepository.Listener {
             // The display is new.
             } else if (!wasPreviouslyUpdated) {
                 Slog.i(TAG, "Adding new display: " + displayId + ": " + newDisplayInfo);
-                assignDisplayGroupLocked(display);
                 mLogicalDisplaysToUpdate.put(displayId, LOGICAL_DISPLAY_EVENT_ADDED);
 
             // Underlying displays device has changed to a different one.
             } else if (!TextUtils.equals(mTempDisplayInfo.uniqueId, newDisplayInfo.uniqueId)) {
-                // FLAG_OWN_DISPLAY_GROUP could have changed, recalculate just in case
-                assignDisplayGroupLocked(display);
                 mLogicalDisplaysToUpdate.put(displayId, LOGICAL_DISPLAY_EVENT_SWAPPED);
 
             // Something about the display device has changed.
-            } else if (!mTempDisplayInfo.equals(newDisplayInfo)) {
+            } else if (wasDirty || !mTempDisplayInfo.equals(newDisplayInfo)) {
-                // FLAG_OWN_DISPLAY_GROUP could have changed, recalculate just in case
-                assignDisplayGroupLocked(display);
                 // If only the hdr/sdr ratio changed, then send just the event for that case
                 if ((diff == DisplayDeviceInfo.DIFF_HDR_SDR_RATIO)) {
                     mLogicalDisplaysToUpdate.put(displayId,
@@ -851,9 +848,18 @@ class LogicalDisplayMapper implements DisplayDeviceRepository.Listener {
         }
     }
 
+    /** This method should be called before LogicalDisplay.updateLocked,
+     * DisplayInfo in LogicalDisplay (display.getDisplayInfoLocked()) is not updated yet,
+     * and should not be used directly or indirectly in this method */
     private void assignDisplayGroupLocked(LogicalDisplay display) {
+        if (!display.isValidLocked()) { // null check for display.mPrimaryDisplayDevice
+            return;
+        }
+        // updated primary device directly from LogicalDisplay (not from DisplayInfo)
+        final DisplayDevice displayDevice = display.getPrimaryDisplayDeviceLocked();
+        // final in LogicalDisplay
         final int displayId = display.getDisplayIdLocked();
-        final String primaryDisplayUniqueId = display.getPrimaryDisplayDeviceLocked().getUniqueId();
+        final String primaryDisplayUniqueId = displayDevice.getUniqueId();
         final Integer linkedDeviceUniqueId =
                 mVirtualDeviceDisplayMapping.get(primaryDisplayUniqueId);
 
@@ -866,8 +872,17 @@ class LogicalDisplayMapper implements DisplayDeviceRepository.Listener {
         }
         final DisplayGroup oldGroup = getDisplayGroupLocked(groupId);
 
-        // Get the new display group if a change is needed
+        // groupName directly from LogicalDisplay (not from DisplayInfo)
-        final boolean needsOwnDisplayGroup = display.needsOwnDisplayGroupLocked();
+        final String groupName = display.getDisplayGroupNameLocked();
+        // DisplayDeviceInfo is safe to use, it is updated earlier
+        final DisplayDeviceInfo displayDeviceInfo = displayDevice.getDisplayDeviceInfoLocked();
+        // Get the new display group if a change is needed, if display group name is empty and
+        // {@code DisplayDeviceInfo.FLAG_OWN_DISPLAY_GROUP} is not set, the display is assigned
+        // to the default display group.
+        final boolean needsOwnDisplayGroup =
+                (displayDeviceInfo.flags & DisplayDeviceInfo.FLAG_OWN_DISPLAY_GROUP) != 0
+                        || !TextUtils.isEmpty(groupName);
+
         final boolean hasOwnDisplayGroup = groupId != Display.DEFAULT_DISPLAY_GROUP;
         final boolean needsDeviceDisplayGroup =
                 !needsOwnDisplayGroup && linkedDeviceUniqueId != null;

services/tests/servicestests/src/com/android/server/display/LogicalDisplayTest.java

@@ -17,6 +17,7 @@
 package com.android.server.display;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.any;
@@ -190,6 +191,19 @@ public class LogicalDisplayTest {
         assertEquals(layoutLimitedRefreshRate, info3.layoutLimitedRefreshRate);
     }
 
+    @Test
+    public void testUpdateLayoutLimitedRefreshRate_setsDirtyFlag() {
+        SurfaceControl.RefreshRateRange layoutLimitedRefreshRate =
+                new SurfaceControl.RefreshRateRange(0, 120);
+        assertFalse(mLogicalDisplay.isDirtyLocked());
+
+        mLogicalDisplay.updateLayoutLimitedRefreshRateLocked(layoutLimitedRefreshRate);
+        assertTrue(mLogicalDisplay.isDirtyLocked());
+
+        mLogicalDisplay.updateLocked(mDeviceRepo);
+        assertFalse(mLogicalDisplay.isDirtyLocked());
+    }
+
     @Test
     public void testUpdateRefreshRateThermalThrottling() {
         SparseArray<SurfaceControl.RefreshRateRange> refreshRanges = new SparseArray<>();
@@ -206,6 +220,45 @@ public class LogicalDisplayTest {
         assertTrue(refreshRanges.contentEquals(info3.thermalRefreshRateThrottling));
     }
 
+    @Test
+    public void testUpdateRefreshRateThermalThrottling_setsDirtyFlag() {
+        SparseArray<SurfaceControl.RefreshRateRange> refreshRanges = new SparseArray<>();
+        refreshRanges.put(0, new SurfaceControl.RefreshRateRange(0, 120));
+        assertFalse(mLogicalDisplay.isDirtyLocked());
+
+        mLogicalDisplay.updateThermalRefreshRateThrottling(refreshRanges);
+        assertTrue(mLogicalDisplay.isDirtyLocked());
+
+        mLogicalDisplay.updateLocked(mDeviceRepo);
+        assertFalse(mLogicalDisplay.isDirtyLocked());
+    }
+
+    @Test
+    public void testUpdateDisplayGroupIdLocked() {
+        int newId = 999;
+        DisplayInfo info1 = mLogicalDisplay.getDisplayInfoLocked();
+        mLogicalDisplay.updateDisplayGroupIdLocked(newId);
+        DisplayInfo info2 = mLogicalDisplay.getDisplayInfoLocked();
+        // Display info should only be updated when updateLocked is called
+        assertEquals(info2, info1);
+
+        mLogicalDisplay.updateLocked(mDeviceRepo);
+        DisplayInfo info3 = mLogicalDisplay.getDisplayInfoLocked();
+        assertNotEquals(info3, info2);
+        assertEquals(newId, info3.displayGroupId);
+    }
+
+    @Test
+    public void testUpdateDisplayGroupIdLocked_setsDirtyFlag() {
+        assertFalse(mLogicalDisplay.isDirtyLocked());
+
+        mLogicalDisplay.updateDisplayGroupIdLocked(99);
+        assertTrue(mLogicalDisplay.isDirtyLocked());
+
+        mLogicalDisplay.updateLocked(mDeviceRepo);
+        assertFalse(mLogicalDisplay.isDirtyLocked());
+    }
+
     @Test
     public void testSetThermalBrightnessThrottlingDataId() {
         String brightnessThrottlingDataId = "throttling_data_id";
@@ -220,4 +273,15 @@ public class LogicalDisplayTest {
         assertNotEquals(info3, info2);
         assertEquals(brightnessThrottlingDataId, info3.thermalBrightnessThrottlingDataId);
     }
+
+    @Test
+    public void testSetThermalBrightnessThrottlingDataId_setsDirtyFlag() {
+        assertFalse(mLogicalDisplay.isDirtyLocked());
+
+        mLogicalDisplay.setThermalBrightnessThrottlingDataIdLocked("99");
+        assertTrue(mLogicalDisplay.isDirtyLocked());
+
+        mLogicalDisplay.updateLocked(mDeviceRepo);
+        assertFalse(mLogicalDisplay.isDirtyLocked());
+    }
 }