From 8ffc93d01ad53fb6e13c9d3c20bd342907e8801d Mon Sep 17 00:00:00 2001 From: Oleg Petsjonkin Date: Thu, 11 May 2023 20:05:22 +0000 Subject: [PATCH] LogicalDisplay.updateDisplayGroupIdLocked should not update DisplayInfo directly Method call ordered map (before changes) is here: https://screenshot.googleplex.com/3a9qwwQ2ppdzsrV, critical part of the change is that now _updateDisplayGroupIdLocked_ is called before _updateLocked_. Followup created b/282150780 Bug: b/267814369 Test: atest LogicalDisplayTest, atest LogicalDisplayMapperTest, atest DisplayManagerServiceTest Change-Id: I76bfb2b7ee537ba4d29249de02170a693e2deadb --- .../server/display/LogicalDisplay.java | 20 ++---- .../server/display/LogicalDisplayMapper.java | 33 +++++++--- .../server/display/LogicalDisplayTest.java | 64 +++++++++++++++++++ 3 files changed, 93 insertions(+), 24 deletions(-) diff --git a/services/core/java/com/android/server/display/LogicalDisplay.java b/services/core/java/com/android/server/display/LogicalDisplay.java index e5c50e61bf259..4edc8bc3eceb2 100644 --- a/services/core/java/com/android/server/display/LogicalDisplay.java +++ b/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; - mInfo.set(null); + mDirty = true; } } @@ -932,18 +934,6 @@ final class LogicalDisplay { return mDisplayGroupName; } - /** - * Returns whether a display group other than the default display group needs to be assigned. - * - *

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); diff --git a/services/core/java/com/android/server/display/LogicalDisplayMapper.java b/services/core/java/com/android/server/display/LogicalDisplayMapper.java index 254441c2aa130..d01b03f836a50 100644 --- a/services/core/java/com/android/server/display/LogicalDisplayMapper.java +++ b/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)) { - // FLAG_OWN_DISPLAY_GROUP could have changed, recalculate just in case - assignDisplayGroupLocked(display); + } else if (wasDirty || !mTempDisplayInfo.equals(newDisplayInfo)) { // 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 - final boolean needsOwnDisplayGroup = display.needsOwnDisplayGroupLocked(); + // groupName directly from LogicalDisplay (not from DisplayInfo) + 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; diff --git a/services/tests/servicestests/src/com/android/server/display/LogicalDisplayTest.java b/services/tests/servicestests/src/com/android/server/display/LogicalDisplayTest.java index f6cf57161f594..30ff8ba6e331e 100644 --- a/services/tests/servicestests/src/com/android/server/display/LogicalDisplayTest.java +++ b/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 refreshRanges = new SparseArray<>(); @@ -206,6 +220,45 @@ public class LogicalDisplayTest { assertTrue(refreshRanges.contentEquals(info3.thermalRefreshRateThrottling)); } + @Test + public void testUpdateRefreshRateThermalThrottling_setsDirtyFlag() { + SparseArray 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()); + } }