diff options
34 files changed, 540 insertions, 276 deletions
diff --git a/include/input/InputTransport.h b/include/input/InputTransport.h index 0cd87201fb..279a4ae35c 100644 --- a/include/input/InputTransport.h +++ b/include/input/InputTransport.h @@ -454,4 +454,6 @@ private: InputVerifier mInputVerifier; }; +std::ostream& operator<<(std::ostream& out, const InputMessage& msg); + } // namespace android diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 777c22a63e..2c37624304 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -542,7 +542,7 @@ status_t Parcel::appendFrom(const Parcel* parcel, size_t offset, size_t len) { return BAD_VALUE; } - if ((mDataSize+len) > mDataCapacity) { + if ((mDataPos + len) > mDataCapacity) { // grow data err = growData(len); if (err != NO_ERROR) { diff --git a/libs/binder/tests/binderParcelUnitTest.cpp b/libs/binder/tests/binderParcelUnitTest.cpp index 6259d9d2d2..a71da3f384 100644 --- a/libs/binder/tests/binderParcelUnitTest.cpp +++ b/libs/binder/tests/binderParcelUnitTest.cpp @@ -197,6 +197,17 @@ TEST(Parcel, AppendPlainDataPartial) { ASSERT_EQ(2, p2.readInt32()); } +TEST(Parcel, AppendWithBadDataPos) { + Parcel p1; + p1.writeInt32(1); + p1.writeInt32(1); + Parcel p2; + p2.setDataCapacity(8); + p2.setDataPosition(10000); + + EXPECT_EQ(android::BAD_VALUE, p2.appendFrom(&p1, 0, 8)); +} + TEST(Parcel, HasBinders) { sp<IBinder> b1 = sp<BBinder>::make(); diff --git a/libs/gui/include/gui/BufferQueueProducer.h b/libs/gui/include/gui/BufferQueueProducer.h index 50abadb922..6a1e9f6f03 100644 --- a/libs/gui/include/gui/BufferQueueProducer.h +++ b/libs/gui/include/gui/BufferQueueProducer.h @@ -36,8 +36,6 @@ class BufferQueueProducer : public BnGraphicBufferProducer { public: friend class BufferQueue; // Needed to access binderDied - explicit BufferQueueProducer(const sp<BufferQueueCore>& core, - bool consumerIsSurfaceFlinger = false); ~BufferQueueProducer() override; // requestBuffer returns the GraphicBuffer for slot N. @@ -219,6 +217,9 @@ public: #endif protected: + explicit BufferQueueProducer(const sp<BufferQueueCore>& core, + bool consumerIsSurfaceFlinger = false); + friend class sp<BufferQueueProducer>; // see IGraphicsBufferProducer::setMaxDequeuedBufferCount, but with the ability to retrieve the // total maximum buffer count for the buffer queue (dequeued AND acquired) status_t setMaxDequeuedBufferCount(int maxDequeuedBuffers, int* maxBufferCount); diff --git a/libs/gui/include/gui/Choreographer.h b/libs/gui/include/gui/Choreographer.h index 5862967d4a..b436af1eb3 100644 --- a/libs/gui/include/gui/Choreographer.h +++ b/libs/gui/include/gui/Choreographer.h @@ -79,8 +79,6 @@ public: }; static Context gChoreographers; - explicit Choreographer(const sp<Looper>& looper, const sp<IBinder>& layerHandle = nullptr) - EXCLUDES(gChoreographers.lock); void postFrameCallbackDelayed(AChoreographer_frameCallback cb, AChoreographer_frameCallback64 cb64, AChoreographer_vsyncCallback vsyncCallback, void* data, @@ -113,6 +111,10 @@ public: private: Choreographer(const Choreographer&) = delete; + explicit Choreographer(const sp<Looper>& looper, const sp<IBinder>& layerHandle = nullptr) + EXCLUDES(gChoreographers.lock); + friend class sp<Choreographer>; + friend AChoreographer* AChoreographer_create(); void dispatchVsync(nsecs_t timestamp, PhysicalDisplayId displayId, uint32_t count, VsyncEventData vsyncEventData) override; diff --git a/libs/gui/include/gui/DisplayEventDispatcher.h b/libs/gui/include/gui/DisplayEventDispatcher.h index b06ad077f4..cdf216c945 100644 --- a/libs/gui/include/gui/DisplayEventDispatcher.h +++ b/libs/gui/include/gui/DisplayEventDispatcher.h @@ -23,12 +23,6 @@ using FrameRateOverride = DisplayEventReceiver::Event::FrameRateOverride; class DisplayEventDispatcher : public LooperCallback { public: - explicit DisplayEventDispatcher(const sp<Looper>& looper, - gui::ISurfaceComposer::VsyncSource vsyncSource = - gui::ISurfaceComposer::VsyncSource::eVsyncSourceApp, - EventRegistrationFlags eventRegistration = {}, - const sp<IBinder>& layerHandle = nullptr); - status_t initialize(); void dispose(); status_t scheduleVsync(); @@ -38,6 +32,14 @@ public: status_t getLatestVsyncEventData(ParcelableVsyncEventData* outVsyncEventData) const; protected: + explicit DisplayEventDispatcher(const sp<Looper>& looper, + gui::ISurfaceComposer::VsyncSource vsyncSource = + gui::ISurfaceComposer::VsyncSource::eVsyncSourceApp, + EventRegistrationFlags eventRegistration = {}, + const sp<IBinder>& layerHandle = nullptr); + + friend class sp<DisplayEventDispatcher>; + virtual ~DisplayEventDispatcher() = default; private: diff --git a/libs/gui/include/gui/WindowInfosListenerReporter.h b/libs/gui/include/gui/WindowInfosListenerReporter.h index 684e21ad96..f9a3acee41 100644 --- a/libs/gui/include/gui/WindowInfosListenerReporter.h +++ b/libs/gui/include/gui/WindowInfosListenerReporter.h @@ -40,6 +40,9 @@ public: void reconnect(const sp<gui::ISurfaceComposer>&); private: + WindowInfosListenerReporter() = default; + friend class sp<WindowInfosListenerReporter>; + std::mutex mListenersMutex; std::unordered_set<sp<gui::WindowInfosListener>, gui::SpHash<gui::WindowInfosListener>> mWindowInfosListeners GUARDED_BY(mListenersMutex); diff --git a/libs/input/InputConsumerNoResampling.cpp b/libs/input/InputConsumerNoResampling.cpp index 6087461f6c..9578639e2b 100644 --- a/libs/input/InputConsumerNoResampling.cpp +++ b/libs/input/InputConsumerNoResampling.cpp @@ -171,11 +171,6 @@ InputMessage createTimelineMessage(int32_t inputEventId, nsecs_t gpuCompletedTim return msg; } -std::ostream& operator<<(std::ostream& out, const InputMessage& msg) { - out << ftl::enum_string(msg.header.type); - return out; -} - } // namespace // --- InputConsumerNoResampling --- diff --git a/libs/input/InputTransport.cpp b/libs/input/InputTransport.cpp index d388d48e8d..cb6f7f0707 100644 --- a/libs/input/InputTransport.cpp +++ b/libs/input/InputTransport.cpp @@ -436,16 +436,29 @@ android::base::Result<InputMessage> InputChannel::receiveMessage() { if (error == EAGAIN || error == EWOULDBLOCK) { return android::base::Error(WOULD_BLOCK); } - if (error == EPIPE || error == ENOTCONN || error == ECONNREFUSED) { - return android::base::Error(DEAD_OBJECT); + if (error == EPIPE) { + return android::base::ResultError("Got EPIPE", DEAD_OBJECT); + } + if (error == ENOTCONN) { + return android::base::ResultError("Got ENOTCONN", DEAD_OBJECT); + } + if (error == ECONNREFUSED) { + return android::base::ResultError("Got ECONNREFUSED", DEAD_OBJECT); + } + if (error == ECONNRESET) { + // This means that the client has closed the channel while there was + // still some data in the buffer. In most cases, subsequent reads + // would result in more data. However, that is not guaranteed, so we + // should not return WOULD_BLOCK here to try again. + return android::base::ResultError("Got ECONNRESET", DEAD_OBJECT); } return android::base::Error(-error); } if (nRead == 0) { // check for EOF - ALOGD_IF(DEBUG_CHANNEL_MESSAGES, - "channel '%s' ~ receive message failed because peer was closed", name.c_str()); - return android::base::Error(DEAD_OBJECT); + LOG_IF(INFO, DEBUG_CHANNEL_MESSAGES) + << "channel '" << name << "' ~ receive message failed because peer was closed"; + return android::base::ResultError("::recv returned 0", DEAD_OBJECT); } if (!msg.isValid(nRead)) { @@ -766,4 +779,9 @@ android::base::Result<InputPublisher::ConsumerResponse> InputPublisher::receiveC return android::base::Error(UNKNOWN_ERROR); } +std::ostream& operator<<(std::ostream& out, const InputMessage& msg) { + out << ftl::enum_string(msg.header.type); + return out; +} + } // namespace android diff --git a/libs/input/tests/InputChannel_test.cpp b/libs/input/tests/InputChannel_test.cpp index 25356cfcf0..9b582d99dc 100644 --- a/libs/input/tests/InputChannel_test.cpp +++ b/libs/input/tests/InputChannel_test.cpp @@ -20,6 +20,7 @@ #include <time.h> #include <errno.h> +#include <android-base/logging.h> #include <binder/Binder.h> #include <binder/Parcel.h> #include <gtest/gtest.h> @@ -43,6 +44,39 @@ bool operator==(const InputChannel& left, const InputChannel& right) { return left.getName() == right.getName() && left.getConnectionToken() == right.getConnectionToken() && lhs.st_ino == rhs.st_ino; } + +/** + * Read a message from the provided channel. Read will continue until there's data, so only call + * this if there's data in the channel, or it's closed. If there's no data, this will loop forever. + */ +android::base::Result<InputMessage> readMessage(InputChannel& channel) { + while (true) { + // Keep reading until we get something other than 'WOULD_BLOCK' + android::base::Result<InputMessage> result = channel.receiveMessage(); + if (!result.ok() && result.error().code() == WOULD_BLOCK) { + // The data is not available yet. + continue; // try again + } + return result; + } +} + +InputMessage createFinishedMessage(uint32_t seq) { + InputMessage finish{}; + finish.header.type = InputMessage::Type::FINISHED; + finish.header.seq = seq; + finish.body.finished.handled = true; + return finish; +} + +InputMessage createKeyMessage(uint32_t seq) { + InputMessage key{}; + key.header.type = InputMessage::Type::KEY; + key.header.seq = seq; + key.body.key.action = AKEY_EVENT_ACTION_DOWN; + return key; +} + } // namespace class InputChannelTest : public testing::Test { @@ -227,6 +261,120 @@ TEST_F(InputChannelTest, SendAndReceive_MotionClassification) { } } +/** + * In this test, server writes 3 key events to the client. The client, upon receiving the first key, + * sends a "finished" signal back to server, and then closes the fd. + * + * Next, we check what the server receives. + * + * In most cases, the server will receive the finish event, and then an 'fd closed' event. + * + * However, sometimes, the 'finish' event will not be delivered to the server. This is communicated + * to the server via 'ECONNRESET', which the InputChannel converts into DEAD_OBJECT. + * + * The server needs to be aware of this behaviour and correctly clean up any state associated with + * the client, even if the client did not end up finishing some of the messages. + * + * This test is written to expose a behaviour on the linux side - occasionally, the + * last events written to the fd by the consumer are not delivered to the server. + * + * When tested on 2025 hardware, ECONNRESET was received approximately 1 out of 40 tries. + * In vast majority (~ 29999 / 30000) of cases, after receiving ECONNRESET, the server could still + * read the client data after receiving ECONNRESET. + */ +TEST_F(InputChannelTest, ReceiveAfterCloseMultiThreaded) { + std::unique_ptr<InputChannel> serverChannel, clientChannel; + status_t result = + InputChannel::openInputChannelPair("channel name", serverChannel, clientChannel); + ASSERT_EQ(OK, result) << "should have successfully opened a channel pair"; + + // Sender / publisher: publish 3 keys + InputMessage key1 = createKeyMessage(/*seq=*/1); + serverChannel->sendMessage(&key1); + // The client should close the fd after it reads this one, but we will send 2 more here. + InputMessage key2 = createKeyMessage(/*seq=*/2); + serverChannel->sendMessage(&key2); + InputMessage key3 = createKeyMessage(/*seq=*/3); + serverChannel->sendMessage(&key3); + + std::thread consumer = std::thread([clientChannel = std::move(clientChannel)]() mutable { + // Read the first key + android::base::Result<InputMessage> firstKey = readMessage(*clientChannel); + if (!firstKey.ok()) { + FAIL() << "Did not receive the first key"; + } + + // Send finish + const InputMessage finish = createFinishedMessage(firstKey->header.seq); + clientChannel->sendMessage(&finish); + // Now close the fd + clientChannel.reset(); + }); + + // Now try to read the finish message, even though client closed the fd + android::base::Result<InputMessage> response = readMessage(*serverChannel); + consumer.join(); + if (response.ok()) { + ASSERT_EQ(response->header.type, InputMessage::Type::FINISHED); + } else { + // It's possible that after the client closes the fd, server will receive ECONNRESET. + // In those situations, this error code will be translated into DEAD_OBJECT by the + // InputChannel. + ASSERT_EQ(response.error().code(), DEAD_OBJECT); + // In most cases, subsequent attempts to read the client channel at this + // point would succeed. However, for simplicity, we exit here (since + // it's not guaranteed). + return; + } + + // There should not be any more events from the client, since the client closed fd after the + // first key. + android::base::Result<InputMessage> noEvent = serverChannel->receiveMessage(); + ASSERT_FALSE(noEvent.ok()) << "Got event " << *noEvent; +} + +/** + * Similar test as above, but single-threaded. + */ +TEST_F(InputChannelTest, ReceiveAfterCloseSingleThreaded) { + std::unique_ptr<InputChannel> serverChannel, clientChannel; + status_t result = + InputChannel::openInputChannelPair("channel name", serverChannel, clientChannel); + ASSERT_EQ(OK, result) << "should have successfully opened a channel pair"; + + // Sender / publisher: publish 3 keys + InputMessage key1 = createKeyMessage(/*seq=*/1); + serverChannel->sendMessage(&key1); + // The client should close the fd after it reads this one, but we will send 2 more here. + InputMessage key2 = createKeyMessage(/*seq=*/2); + serverChannel->sendMessage(&key2); + InputMessage key3 = createKeyMessage(/*seq=*/3); + serverChannel->sendMessage(&key3); + + // Read the first key + android::base::Result<InputMessage> firstKey = readMessage(*clientChannel); + if (!firstKey.ok()) { + FAIL() << "Did not receive the first key"; + } + + // Send finish + const InputMessage finish = createFinishedMessage(firstKey->header.seq); + clientChannel->sendMessage(&finish); + // Now close the fd + clientChannel.reset(); + + // Now try to read the finish message, even though client closed the fd + android::base::Result<InputMessage> response = readMessage(*serverChannel); + ASSERT_FALSE(response.ok()); + ASSERT_EQ(response.error().code(), DEAD_OBJECT); + + // We can still read the finish event (but in practice, the expectation is that the server will + // not be doing this after getting DEAD_OBJECT). + android::base::Result<InputMessage> finishEvent = serverChannel->receiveMessage(); + ASSERT_TRUE(finishEvent.ok()); + ASSERT_EQ(finishEvent->header.type, InputMessage::Type::FINISHED); +} + TEST_F(InputChannelTest, DuplicateChannelAndAssertEqual) { std::unique_ptr<InputChannel> serverChannel, clientChannel; diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h index 608bec4a0c..c8432005a4 100644 --- a/services/inputflinger/include/InputReaderBase.h +++ b/services/inputflinger/include/InputReaderBase.h @@ -443,6 +443,9 @@ public: /* Get the Bluetooth address of an input device, if known. */ virtual std::optional<std::string> getBluetoothAddress(int32_t deviceId) const = 0; + /* Gets the sysfs root path for this device. Returns an empty path if there is none. */ + virtual std::filesystem::path getSysfsRootPath(int32_t deviceId) const = 0; + /* Sysfs node change reported. Recreate device if required to incorporate the new sysfs nodes */ virtual void sysfsNodeChanged(const std::string& sysfsNodePath) = 0; diff --git a/services/inputflinger/reader/EventHub.cpp b/services/inputflinger/reader/EventHub.cpp index 2fcb5d831f..559bc0aa7a 100644 --- a/services/inputflinger/reader/EventHub.cpp +++ b/services/inputflinger/reader/EventHub.cpp @@ -246,7 +246,7 @@ static nsecs_t processEventTimestamp(const struct input_event& event) { /** * Returns the sysfs root path of the input device. */ -static std::optional<std::filesystem::path> getSysfsRootPath(const char* devicePath) { +static std::optional<std::filesystem::path> getSysfsRootForEvdevDevicePath(const char* devicePath) { std::error_code errorCode; // Stat the device path to get the major and minor number of the character file @@ -1619,7 +1619,7 @@ void EventHub::assignDescriptorLocked(InputDeviceIdentifier& identifier) { std::shared_ptr<const EventHub::AssociatedDevice> EventHub::obtainAssociatedDeviceLocked( const std::filesystem::path& devicePath, const std::shared_ptr<PropertyMap>& config) const { const std::optional<std::filesystem::path> sysfsRootPathOpt = - getSysfsRootPath(devicePath.c_str()); + getSysfsRootForEvdevDevicePath(devicePath.c_str()); if (!sysfsRootPathOpt) { return nullptr; } @@ -1897,58 +1897,89 @@ std::vector<RawEvent> EventHub::getEvents(int timeoutMillis) { break; // return to the caller before we actually rescan } - // Report any devices that had last been added/removed. - for (auto it = mClosingDevices.begin(); it != mClosingDevices.end();) { - std::unique_ptr<Device> device = std::move(*it); - ALOGV("Reporting device closed: id=%d, name=%s\n", device->id, device->path.c_str()); - const int32_t deviceId = (device->id == mBuiltInKeyboardId) - ? ReservedInputDeviceId::BUILT_IN_KEYBOARD_ID - : device->id; - events.push_back({ - .when = now, - .deviceId = deviceId, - .type = DEVICE_REMOVED, - }); - it = mClosingDevices.erase(it); - if (events.size() == EVENT_BUFFER_SIZE) { - break; + handleSysfsNodeChangeNotificationsLocked(); + + // Use a do-while loop to ensure that we drain the closing and opening devices loop + // at least once, even if there are no devices to re-open. + do { + if (!mDeviceIdsToReopen.empty()) { + // If there are devices that need to be re-opened, ensure that we re-open them + // one at a time to send the DEVICE_REMOVED and DEVICE_ADDED notifications for + // each before moving on to the next. This is to avoid notifying all device + // removals and additions in one batch, which could cause additional unnecessary + // device added/removed notifications for merged InputDevices from InputReader. + const int32_t deviceId = mDeviceIdsToReopen.back(); + mDeviceIdsToReopen.erase(mDeviceIdsToReopen.end() - 1); + if (auto it = mDevices.find(deviceId); it != mDevices.end()) { + ALOGI("Reopening input device: id=%d, name=%s", it->second->id, + it->second->identifier.name.c_str()); + const auto path = it->second->path; + closeDeviceLocked(*it->second); + openDeviceLocked(path); + } } - } - if (mNeedToScanDevices) { - mNeedToScanDevices = false; - scanDevicesLocked(); - } - - while (!mOpeningDevices.empty()) { - std::unique_ptr<Device> device = std::move(*mOpeningDevices.rbegin()); - mOpeningDevices.pop_back(); - ALOGV("Reporting device opened: id=%d, name=%s\n", device->id, device->path.c_str()); - const int32_t deviceId = device->id == mBuiltInKeyboardId ? 0 : device->id; - events.push_back({ - .when = now, - .deviceId = deviceId, - .type = DEVICE_ADDED, - }); - - // Try to find a matching video device by comparing device names - for (auto it = mUnattachedVideoDevices.begin(); it != mUnattachedVideoDevices.end(); - it++) { - std::unique_ptr<TouchVideoDevice>& videoDevice = *it; - if (tryAddVideoDeviceLocked(*device, videoDevice)) { - // videoDevice was transferred to 'device' - it = mUnattachedVideoDevices.erase(it); + // Report any devices that had last been added/removed. + for (auto it = mClosingDevices.begin(); it != mClosingDevices.end();) { + std::unique_ptr<Device> device = std::move(*it); + ALOGV("Reporting device closed: id=%d, name=%s\n", device->id, + device->path.c_str()); + const int32_t deviceId = (device->id == mBuiltInKeyboardId) + ? ReservedInputDeviceId::BUILT_IN_KEYBOARD_ID + : device->id; + events.push_back({ + .when = now, + .deviceId = deviceId, + .type = DEVICE_REMOVED, + }); + it = mClosingDevices.erase(it); + if (events.size() == EVENT_BUFFER_SIZE) { break; } } - auto [dev_it, inserted] = mDevices.insert_or_assign(device->id, std::move(device)); - if (!inserted) { - ALOGW("Device id %d exists, replaced.", device->id); + if (mNeedToScanDevices) { + mNeedToScanDevices = false; + scanDevicesLocked(); } - if (events.size() == EVENT_BUFFER_SIZE) { - break; + + while (!mOpeningDevices.empty()) { + std::unique_ptr<Device> device = std::move(*mOpeningDevices.rbegin()); + mOpeningDevices.pop_back(); + ALOGV("Reporting device opened: id=%d, name=%s\n", device->id, + device->path.c_str()); + const int32_t deviceId = device->id == mBuiltInKeyboardId ? 0 : device->id; + events.push_back({ + .when = now, + .deviceId = deviceId, + .type = DEVICE_ADDED, + }); + + // Try to find a matching video device by comparing device names + for (auto it = mUnattachedVideoDevices.begin(); it != mUnattachedVideoDevices.end(); + it++) { + std::unique_ptr<TouchVideoDevice>& videoDevice = *it; + if (tryAddVideoDeviceLocked(*device, videoDevice)) { + // videoDevice was transferred to 'device' + it = mUnattachedVideoDevices.erase(it); + break; + } + } + + auto [dev_it, inserted] = mDevices.insert_or_assign(device->id, std::move(device)); + if (!inserted) { + ALOGW("Device id %d exists, replaced.", device->id); + } + if (events.size() == EVENT_BUFFER_SIZE) { + break; + } } + + // Perform this loop of re-opening devices so that we re-open one device at a time. + } while (!mDeviceIdsToReopen.empty()); + + if (events.size() == EVENT_BUFFER_SIZE) { + break; } // Grab the next input event. @@ -2664,17 +2695,44 @@ status_t EventHub::disableDevice(int32_t deviceId) { return device->disable(); } +std::filesystem::path EventHub::getSysfsRootPath(int32_t deviceId) const { + std::scoped_lock _l(mLock); + Device* device = getDeviceLocked(deviceId); + if (device == nullptr) { + ALOGE("Invalid device id=%" PRId32 " provided to %s", deviceId, __func__); + return {}; + } + + return device->associatedDevice ? device->associatedDevice->sysfsRootPath + : std::filesystem::path{}; +} + // TODO(b/274755573): Shift to uevent handling on native side and remove this method // Currently using Java UEventObserver to trigger this which uses UEvent infrastructure that uses a // NETLINK socket to observe UEvents. We can create similar infrastructure on Eventhub side to // directly observe UEvents instead of triggering from Java side. void EventHub::sysfsNodeChanged(const std::string& sysfsNodePath) { - std::scoped_lock _l(mLock); + mChangedSysfsNodeNotifications.emplace(sysfsNodePath); +} + +void EventHub::handleSysfsNodeChangeNotificationsLocked() { + // Use a set to de-dup any repeated notifications. + std::set<std::string> changedNodes; + while (true) { + auto node = mChangedSysfsNodeNotifications.popWithTimeout(std::chrono::nanoseconds(0)); + if (!node.has_value()) break; + changedNodes.emplace(*node); + } + if (changedNodes.empty()) { + return; + } // Testing whether a sysfs node changed involves several syscalls, so use a cache to avoid // testing the same node multiple times. + // TODO(b/281822656): Notify InputReader separately when an AssociatedDevice changes, + // instead of needing to re-open all of Devices that are associated with it. std::map<std::shared_ptr<const AssociatedDevice>, bool /*changed*/> testedDevices; - auto isAssociatedDeviceChanged = [&testedDevices, &sysfsNodePath](const Device& dev) { + auto shouldReopenDevice = [&testedDevices, &changedNodes](const Device& dev) { if (!dev.associatedDevice) { return false; } @@ -2683,45 +2741,45 @@ void EventHub::sysfsNodeChanged(const std::string& sysfsNodePath) { return testedIt->second; } // Cache miss - if (sysfsNodePath.find(dev.associatedDevice->sysfsRootPath.string()) == std::string::npos) { + const bool anyNodesChanged = + std::any_of(changedNodes.begin(), changedNodes.end(), [&](const std::string& node) { + return node.find(dev.associatedDevice->sysfsRootPath.string()) != + std::string::npos; + }); + if (!anyNodesChanged) { testedDevices.emplace(dev.associatedDevice, false); return false; } auto reloadedDevice = AssociatedDevice(dev.associatedDevice->sysfsRootPath, dev.associatedDevice->baseDevConfig); const bool changed = *dev.associatedDevice != reloadedDevice; + if (changed) { + ALOGI("sysfsNodeChanged: Identified change in sysfs nodes for device: %s", + dev.identifier.name.c_str()); + } testedDevices.emplace(dev.associatedDevice, changed); return changed; }; - std::set<Device*> devicesToClose; - std::set<std::string /*path*/> devicesToOpen; - - // Check in opening devices. If its associated device changed, - // the device should be removed from mOpeningDevices and needs to be opened again. - std::erase_if(mOpeningDevices, [&](const auto& dev) { - if (isAssociatedDeviceChanged(*dev)) { - devicesToOpen.emplace(dev->path); - return true; + // Check in opening devices. These can be re-opened directly because we have not yet notified + // the Reader about these devices. + for (const auto& dev : mOpeningDevices) { + if (shouldReopenDevice(*dev)) { + ALOGI("Reopening input device from mOpeningDevices: id=%d, name=%s", dev->id, + dev->identifier.name.c_str()); + const auto path = dev->path; + closeDeviceLocked(*dev); // The Device object is deleted by this function. + openDeviceLocked(path); } - return false; - }); + } - // Check in already added device. If its associated device changed, - // the device needs to be re-opened. + // Check in already added devices. Add them to the re-opening list so they can be + // re-opened serially. for (const auto& [id, dev] : mDevices) { - if (isAssociatedDeviceChanged(*dev)) { - devicesToOpen.emplace(dev->path); - devicesToClose.emplace(dev.get()); + if (shouldReopenDevice(*dev)) { + mDeviceIdsToReopen.emplace_back(dev->id); } } - - for (auto* device : devicesToClose) { - closeDeviceLocked(*device); - } - for (const auto& path : devicesToOpen) { - openDeviceLocked(path); - } } void EventHub::createVirtualKeyboardLocked() { @@ -2817,9 +2875,23 @@ void EventHub::closeDeviceLocked(Device& device) { releaseControllerNumberLocked(device.controllerNumber); device.controllerNumber = 0; device.close(); - mClosingDevices.push_back(std::move(mDevices[device.id])); - mDevices.erase(device.id); + // Try to remove this device from mDevices. + if (auto it = mDevices.find(device.id); it != mDevices.end()) { + mClosingDevices.push_back(std::move(mDevices[device.id])); + mDevices.erase(device.id); + return; + } + + // Try to remove this device from mOpeningDevices. + if (auto it = std::find_if(mOpeningDevices.begin(), mOpeningDevices.end(), + [&device](auto& d) { return d->id == device.id; }); + it != mOpeningDevices.end()) { + mOpeningDevices.erase(it); + return; + } + + LOG_ALWAYS_FATAL("%s: Device with id %d was not found!", __func__, device.id); } base::Result<void> EventHub::readNotifyLocked() { diff --git a/services/inputflinger/reader/InputDevice.cpp b/services/inputflinger/reader/InputDevice.cpp index 5e42d57f06..594dcba144 100644 --- a/services/inputflinger/reader/InputDevice.cpp +++ b/services/inputflinger/reader/InputDevice.cpp @@ -136,6 +136,8 @@ void InputDevice::dump(std::string& dump, const std::string& eventHubDevStr) { } else { dump += "<none>\n"; } + dump += StringPrintf(INDENT2 "SysfsRootPath: %s\n", + mSysfsRootPath.empty() ? "<none>" : mSysfsRootPath.c_str()); dump += StringPrintf(INDENT2 "HasMic: %s\n", toString(mHasMic)); dump += StringPrintf(INDENT2 "Sources: %s\n", inputEventSourceToString(deviceInfo.getSources()).c_str()); @@ -195,6 +197,10 @@ void InputDevice::addEmptyEventHubDevice(int32_t eventHubId) { DevicePair& devicePair = mDevices[eventHubId]; devicePair.second = createMappers(*devicePair.first, readerConfig); + if (mSysfsRootPath.empty()) { + mSysfsRootPath = devicePair.first->getSysfsRootPath(); + } + // Must change generation to flag this device as changed bumpGeneration(); return out; diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp index 58df692b3e..74ef972848 100644 --- a/services/inputflinger/reader/InputReader.cpp +++ b/services/inputflinger/reader/InputReader.cpp @@ -917,8 +917,19 @@ bool InputReader::canDispatchToDisplay(int32_t deviceId, ui::LogicalDisplayId di return *associatedDisplayId == displayId; } +std::filesystem::path InputReader::getSysfsRootPath(int32_t deviceId) const { + std::scoped_lock _l(mLock); + + const InputDevice* device = findInputDeviceLocked(deviceId); + if (!device) { + return {}; + } + return device->getSysfsRootPath(); +} + void InputReader::sysfsNodeChanged(const std::string& sysfsNodePath) { mEventHub->sysfsNodeChanged(sysfsNodePath); + mEventHub->wake(); } DeviceId InputReader::getLastUsedInputDeviceId() { diff --git a/services/inputflinger/reader/include/EventHub.h b/services/inputflinger/reader/include/EventHub.h index adbfdebfb0..9f3a57c265 100644 --- a/services/inputflinger/reader/include/EventHub.h +++ b/services/inputflinger/reader/include/EventHub.h @@ -31,6 +31,7 @@ #include <batteryservice/BatteryService.h> #include <ftl/flags.h> +#include <input/BlockingQueue.h> #include <input/Input.h> #include <input/InputDevice.h> #include <input/KeyCharacterMap.h> @@ -398,6 +399,9 @@ public: /* Disable an input device. Closes file descriptor to that device. */ virtual status_t disableDevice(int32_t deviceId) = 0; + /* Gets the sysfs root path for this device. Returns an empty path if there is none. */ + virtual std::filesystem::path getSysfsRootPath(int32_t deviceId) const = 0; + /* Sysfs node changed. Reopen the Eventhub device if any new Peripheral like Light, Battery, * etc. is detected. */ virtual void sysfsNodeChanged(const std::string& sysfsNodePath) = 0; @@ -613,6 +617,8 @@ public: status_t disableDevice(int32_t deviceId) override final; + std::filesystem::path getSysfsRootPath(int32_t deviceId) const override final; + void sysfsNodeChanged(const std::string& sysfsNodePath) override final; bool setKernelWakeEnabled(int32_t deviceId, bool enabled) override final; @@ -775,6 +781,8 @@ private: void addDeviceInputInotify(); void addDeviceInotify(); + void handleSysfsNodeChangeNotificationsLocked() REQUIRES(mLock); + // Protect all internal state. mutable std::mutex mLock; @@ -807,6 +815,7 @@ private: bool mNeedToReopenDevices; bool mNeedToScanDevices; std::vector<std::string> mExcludedDevices; + std::vector<int32_t> mDeviceIdsToReopen; int mEpollFd; int mINotifyFd; @@ -824,6 +833,10 @@ private: size_t mPendingEventCount; size_t mPendingEventIndex; bool mPendingINotify; + + // The sysfs node change notifications that have been sent to EventHub. + // Enqueuing notifications does not require the lock to be held. + BlockingQueue<std::string> mChangedSysfsNodeNotifications; }; } // namespace android diff --git a/services/inputflinger/reader/include/InputDevice.h b/services/inputflinger/reader/include/InputDevice.h index 4744dd0e4e..a1a8891940 100644 --- a/services/inputflinger/reader/include/InputDevice.h +++ b/services/inputflinger/reader/include/InputDevice.h @@ -81,6 +81,8 @@ public: inline virtual KeyboardType getKeyboardType() const { return mKeyboardType; } + inline std::filesystem::path getSysfsRootPath() const { return mSysfsRootPath; } + bool isEnabled(); void dump(std::string& dump, const std::string& eventHubDevStr); @@ -209,6 +211,7 @@ private: bool mHasMic; bool mDropUntilNextSync; std::optional<bool> mShouldSmoothScroll; + std::filesystem::path mSysfsRootPath; typedef int32_t (InputMapper::*GetStateFunc)(uint32_t sourceMask, int32_t code); int32_t getState(uint32_t sourceMask, int32_t code, GetStateFunc getStateFunc); @@ -471,6 +474,9 @@ public: inline void setKeyboardType(KeyboardType keyboardType) { return mDevice.setKeyboardType(keyboardType); } + inline std::filesystem::path getSysfsRootPath() const { + return mEventHub->getSysfsRootPath(mId); + } inline bool setKernelWakeEnabled(bool enabled) { return mEventHub->setKernelWakeEnabled(mId, enabled); } diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h index 6a259373df..9212d37966 100644 --- a/services/inputflinger/reader/include/InputReader.h +++ b/services/inputflinger/reader/include/InputReader.h @@ -118,6 +118,8 @@ public: std::optional<std::string> getBluetoothAddress(int32_t deviceId) const override; + std::filesystem::path getSysfsRootPath(int32_t deviceId) const override; + void sysfsNodeChanged(const std::string& sysfsNodePath) override; DeviceId getLastUsedInputDeviceId() override; diff --git a/services/inputflinger/tests/FakeEventHub.cpp b/services/inputflinger/tests/FakeEventHub.cpp index e72c440480..8987b99525 100644 --- a/services/inputflinger/tests/FakeEventHub.cpp +++ b/services/inputflinger/tests/FakeEventHub.cpp @@ -627,6 +627,14 @@ void FakeEventHub::setSysfsRootPath(int32_t deviceId, std::string sysfsRootPath) device->sysfsRootPath = sysfsRootPath; } +std::filesystem::path FakeEventHub::getSysfsRootPath(int32_t deviceId) const { + Device* device = getDevice(deviceId); + if (device == nullptr) { + return {}; + } + return device->sysfsRootPath; +} + void FakeEventHub::sysfsNodeChanged(const std::string& sysfsNodePath) { int32_t foundDeviceId = -1; Device* foundDevice = nullptr; diff --git a/services/inputflinger/tests/FakeEventHub.h b/services/inputflinger/tests/FakeEventHub.h index 143b93b245..1cd33c1c98 100644 --- a/services/inputflinger/tests/FakeEventHub.h +++ b/services/inputflinger/tests/FakeEventHub.h @@ -222,6 +222,7 @@ private: std::optional<int32_t> getLightBrightness(int32_t deviceId, int32_t lightId) const override; std::optional<std::unordered_map<LightColor, int32_t>> getLightIntensities( int32_t deviceId, int32_t lightId) const override; + std::filesystem::path getSysfsRootPath(int32_t deviceId) const override; void sysfsNodeChanged(const std::string& sysfsNodePath) override; void dump(std::string&) const override {} void monitor() const override {} diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index 43d2378f61..d1d8192395 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -611,8 +611,10 @@ protected: } void addDevice(int32_t eventHubId, const std::string& name, - ftl::Flags<InputDeviceClass> classes, const PropertyMap* configuration) { + ftl::Flags<InputDeviceClass> classes, const PropertyMap* configuration, + std::string sysfsRootPath = "") { mFakeEventHub->addDevice(eventHubId, name, classes); + mFakeEventHub->setSysfsRootPath(eventHubId, sysfsRootPath); if (configuration) { mFakeEventHub->addConfigurationMap(eventHubId, configuration); @@ -664,6 +666,18 @@ TEST_F(InputReaderTest, PolicyGetInputDevices) { ASSERT_EQ(0U, inputDevices[0].getMotionRanges().size()); } +TEST_F(InputReaderTest, GetSysfsRootPath) { + constexpr std::string SYSFS_ROOT = "xyz"; + ASSERT_NO_FATAL_FAILURE( + addDevice(1, "keyboard", InputDeviceClass::KEYBOARD, nullptr, SYSFS_ROOT)); + + // Should also have received a notification describing the new input device. + ASSERT_EQ(1U, mFakePolicy->getInputDevices().size()); + InputDeviceInfo inputDevice = mFakePolicy->getInputDevices()[0]; + + ASSERT_EQ(SYSFS_ROOT, mReader->getSysfsRootPath(inputDevice.getId()).string()); +} + TEST_F(InputReaderTest, InputDeviceRecreatedOnSysfsNodeChanged) { ASSERT_NO_FATAL_FAILURE(addDevice(1, "keyboard", InputDeviceClass::KEYBOARD, nullptr)); mFakeEventHub->setSysfsRootPath(1, "xyz"); diff --git a/services/inputflinger/tests/InterfaceMocks.h b/services/inputflinger/tests/InterfaceMocks.h index d4e4bb00f7..8eded2e279 100644 --- a/services/inputflinger/tests/InterfaceMocks.h +++ b/services/inputflinger/tests/InterfaceMocks.h @@ -181,6 +181,7 @@ public: MOCK_METHOD(bool, isDeviceEnabled, (int32_t deviceId), (const, override)); MOCK_METHOD(status_t, enableDevice, (int32_t deviceId), (override)); MOCK_METHOD(status_t, disableDevice, (int32_t deviceId), (override)); + MOCK_METHOD(std::filesystem::path, getSysfsRootPath, (int32_t deviceId), (const, override)); MOCK_METHOD(void, sysfsNodeChanged, (const std::string& sysfsNodePath), (override)); MOCK_METHOD(bool, setKernelWakeEnabled, (int32_t deviceId, bool enabled), (override)); }; diff --git a/services/inputflinger/tests/fuzzers/InputReaderFuzzer.cpp b/services/inputflinger/tests/fuzzers/InputReaderFuzzer.cpp index 6be922dfdb..0c8ba50776 100644 --- a/services/inputflinger/tests/fuzzers/InputReaderFuzzer.cpp +++ b/services/inputflinger/tests/fuzzers/InputReaderFuzzer.cpp @@ -165,6 +165,10 @@ public: return reader->getBluetoothAddress(deviceId); } + std::filesystem::path getSysfsRootPath(int32_t deviceId) const { + return reader->getSysfsRootPath(deviceId); + } + void sysfsNodeChanged(const std::string& sysfsNodePath) { reader->sysfsNodeChanged(sysfsNodePath); } @@ -297,6 +301,7 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t* data, size_t size) { std::chrono::microseconds(fdp->ConsumeIntegral<size_t>())); }, [&]() -> void { reader->getBluetoothAddress(fdp->ConsumeIntegral<int32_t>()); }, + [&]() -> void { reader->getSysfsRootPath(fdp->ConsumeIntegral<int32_t>()); }, })(); } diff --git a/services/inputflinger/tests/fuzzers/MapperHelpers.h b/services/inputflinger/tests/fuzzers/MapperHelpers.h index 9a5903981b..f619c48f3f 100644 --- a/services/inputflinger/tests/fuzzers/MapperHelpers.h +++ b/services/inputflinger/tests/fuzzers/MapperHelpers.h @@ -296,6 +296,7 @@ public: bool isDeviceEnabled(int32_t deviceId) const override { return mFdp->ConsumeBool(); } status_t enableDevice(int32_t deviceId) override { return mFdp->ConsumeIntegral<status_t>(); } status_t disableDevice(int32_t deviceId) override { return mFdp->ConsumeIntegral<status_t>(); } + std::filesystem::path getSysfsRootPath(int32_t deviceId) const override { return {}; } void sysfsNodeChanged(const std::string& sysfsNodePath) override {} bool setKernelWakeEnabled(int32_t deviceId, bool enabled) override { return mFdp->ConsumeBool(); diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp index de7d455fa4..bad5e2e3b5 100644 --- a/services/surfaceflinger/DisplayDevice.cpp +++ b/services/surfaceflinger/DisplayDevice.cpp @@ -50,6 +50,17 @@ namespace android { namespace hal = hardware::graphics::composer::hal; +namespace gui { +inline std::string_view to_string(ISurfaceComposer::OptimizationPolicy optimizationPolicy) { + switch (optimizationPolicy) { + case ISurfaceComposer::OptimizationPolicy::optimizeForPower: + return "optimizeForPower"; + case ISurfaceComposer::OptimizationPolicy::optimizeForPerformance: + return "optimizeForPerformance"; + } +} +} // namespace gui + DisplayDeviceCreationArgs::DisplayDeviceCreationArgs( const sp<SurfaceFlinger>& flinger, HWComposer& hwComposer, const wp<IBinder>& displayToken, std::shared_ptr<compositionengine::Display> compositionDisplay) diff --git a/services/surfaceflinger/DisplayDevice.h b/services/surfaceflinger/DisplayDevice.h index 1b14145147..7d7c8adb7b 100644 --- a/services/surfaceflinger/DisplayDevice.h +++ b/services/surfaceflinger/DisplayDevice.h @@ -67,17 +67,6 @@ namespace display { class DisplaySnapshot; } // namespace display -namespace gui { -inline const char* to_string(ISurfaceComposer::OptimizationPolicy optimizationPolicy) { - switch (optimizationPolicy) { - case ISurfaceComposer::OptimizationPolicy::optimizeForPower: - return "optimizeForPower"; - case ISurfaceComposer::OptimizationPolicy::optimizeForPerformance: - return "optimizeForPerformance"; - } -} -} // namespace gui - class DisplayDevice : public RefBase { public: constexpr static float sDefaultMinLumiance = 0.0; diff --git a/services/surfaceflinger/FrontEnd/LayerHierarchy.cpp b/services/surfaceflinger/FrontEnd/LayerHierarchy.cpp index da536b6660..00ec863bd9 100644 --- a/services/surfaceflinger/FrontEnd/LayerHierarchy.cpp +++ b/services/surfaceflinger/FrontEnd/LayerHierarchy.cpp @@ -54,7 +54,8 @@ LayerHierarchy::LayerHierarchy(const LayerHierarchy& hierarchy, bool childrenOnl mChildren = hierarchy.mChildren; } -void LayerHierarchy::traverse(const Visitor& visitor, LayerHierarchy::TraversalPath& traversalPath, +void LayerHierarchy::traverse(const Visitor& visitor, + const LayerHierarchy::TraversalPath& traversalPath, uint32_t depth) const { LLOG_ALWAYS_FATAL_WITH_TRACE_IF(depth > 50, "Cycle detected in LayerHierarchy::traverse. See " @@ -70,14 +71,13 @@ void LayerHierarchy::traverse(const Visitor& visitor, LayerHierarchy::TraversalP LLOG_ALWAYS_FATAL_WITH_TRACE_IF(traversalPath.hasRelZLoop(), "Found relative z loop layerId:%d", traversalPath.invalidRelativeRootId); for (auto& [child, childVariant] : mChildren) { - ScopedAddToTraversalPath addChildToTraversalPath(traversalPath, child->mLayer->id, - childVariant); - child->traverse(visitor, traversalPath, depth + 1); + child->traverse(visitor, traversalPath.makeChild(child->mLayer->id, childVariant), + depth + 1); } } void LayerHierarchy::traverseInZOrder(const Visitor& visitor, - LayerHierarchy::TraversalPath& traversalPath) const { + const LayerHierarchy::TraversalPath& traversalPath) const { bool traverseThisLayer = (mLayer != nullptr); for (auto it = mChildren.begin(); it < mChildren.end(); it++) { auto& [child, childVariant] = *it; @@ -91,9 +91,7 @@ void LayerHierarchy::traverseInZOrder(const Visitor& visitor, if (childVariant == LayerHierarchy::Variant::Detached) { continue; } - ScopedAddToTraversalPath addChildToTraversalPath(traversalPath, child->mLayer->id, - childVariant); - child->traverseInZOrder(visitor, traversalPath); + child->traverseInZOrder(visitor, traversalPath.makeChild(child->mLayer->id, childVariant)); } if (traverseThisLayer) { @@ -568,42 +566,23 @@ std::string LayerHierarchy::TraversalPath::toString() const { return ss.str(); } -// Helper class to update a passed in TraversalPath when visiting a child. When the object goes out -// of scope the TraversalPath is reset to its original state. -LayerHierarchy::ScopedAddToTraversalPath::ScopedAddToTraversalPath(TraversalPath& traversalPath, - uint32_t layerId, - LayerHierarchy::Variant variant) - : mTraversalPath(traversalPath), mParentPath(traversalPath) { - // Update the traversal id with the child layer id and variant. Parent id and variant are - // stored to reset the id upon destruction. - traversalPath.id = layerId; - traversalPath.variant = variant; +LayerHierarchy::TraversalPath LayerHierarchy::TraversalPath::makeChild( + uint32_t layerId, LayerHierarchy::Variant variant) const { + TraversalPath child{*this}; + child.id = layerId; + child.variant = variant; if (LayerHierarchy::isMirror(variant)) { - traversalPath.mirrorRootIds.emplace_back(mParentPath.id); + child.mirrorRootIds.emplace_back(id); } else if (variant == LayerHierarchy::Variant::Relative) { - if (std::find(traversalPath.relativeRootIds.begin(), traversalPath.relativeRootIds.end(), - layerId) != traversalPath.relativeRootIds.end()) { - traversalPath.invalidRelativeRootId = layerId; + if (std::find(relativeRootIds.begin(), relativeRootIds.end(), layerId) != + relativeRootIds.end()) { + child.invalidRelativeRootId = layerId; } - traversalPath.relativeRootIds.emplace_back(layerId); + child.relativeRootIds.emplace_back(layerId); } else if (variant == LayerHierarchy::Variant::Detached) { - traversalPath.detached = true; + child.detached = true; } -} -LayerHierarchy::ScopedAddToTraversalPath::~ScopedAddToTraversalPath() { - // Reset the traversal id to its original parent state using the state that was saved in - // the constructor. - if (LayerHierarchy::isMirror(mTraversalPath.variant)) { - mTraversalPath.mirrorRootIds.pop_back(); - } else if (mTraversalPath.variant == LayerHierarchy::Variant::Relative) { - mTraversalPath.relativeRootIds.pop_back(); - } - if (mTraversalPath.invalidRelativeRootId == mTraversalPath.id) { - mTraversalPath.invalidRelativeRootId = UNASSIGNED_LAYER_ID; - } - mTraversalPath.id = mParentPath.id; - mTraversalPath.variant = mParentPath.variant; - mTraversalPath.detached = mParentPath.detached; + return child; } } // namespace android::surfaceflinger::frontend diff --git a/services/surfaceflinger/FrontEnd/LayerHierarchy.h b/services/surfaceflinger/FrontEnd/LayerHierarchy.h index 4fdbae1831..c8c6b4df15 100644 --- a/services/surfaceflinger/FrontEnd/LayerHierarchy.h +++ b/services/surfaceflinger/FrontEnd/LayerHierarchy.h @@ -104,6 +104,8 @@ public: TraversalPath getClonedFrom() const { return {.id = id, .variant = variant}; } + TraversalPath makeChild(uint32_t layerId, LayerHierarchy::Variant variant) const; + bool operator==(const TraversalPath& other) const { return id == other.id && mirrorRootIds == other.mirrorRootIds; } @@ -122,18 +124,6 @@ public: } }; - // Helper class to add nodes to an existing traversal id and removes the - // node when it goes out of scope. - class ScopedAddToTraversalPath { - public: - ScopedAddToTraversalPath(TraversalPath& traversalPath, uint32_t layerId, - LayerHierarchy::Variant variantArg); - ~ScopedAddToTraversalPath(); - - private: - TraversalPath& mTraversalPath; - TraversalPath mParentPath; - }; LayerHierarchy(RequestedLayerState* layer); // Visitor function that provides the hierarchy node and a traversal id which uniquely @@ -191,8 +181,9 @@ private: void removeChild(LayerHierarchy*); void sortChildrenByZOrder(); void updateChild(LayerHierarchy*, LayerHierarchy::Variant); - void traverseInZOrder(const Visitor& visitor, LayerHierarchy::TraversalPath& parent) const; - void traverse(const Visitor& visitor, LayerHierarchy::TraversalPath& parent, + void traverseInZOrder(const Visitor& visitor, + const LayerHierarchy::TraversalPath& parent) const; + void traverse(const Visitor& visitor, const LayerHierarchy::TraversalPath& parent, uint32_t depth = 0) const; void dump(std::ostream& out, const std::string& prefix, LayerHierarchy::Variant variant, bool isLastChild, bool includeMirroredHierarchy) const; diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp index 28a6031c97..50ed72de9e 100644 --- a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp +++ b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp @@ -447,15 +447,14 @@ void LayerSnapshotBuilder::updateSnapshots(const Args& args) { if (args.root.getLayer()) { // The hierarchy can have a root layer when used for screenshots otherwise, it will have // multiple children. - LayerHierarchy::ScopedAddToTraversalPath addChildToPath(root, args.root.getLayer()->id, - LayerHierarchy::Variant::Attached); - updateSnapshotsInHierarchy(args, args.root, root, rootSnapshot, /*depth=*/0); + LayerHierarchy::TraversalPath childPath = + root.makeChild(args.root.getLayer()->id, LayerHierarchy::Variant::Attached); + updateSnapshotsInHierarchy(args, args.root, childPath, rootSnapshot, /*depth=*/0); } else { for (auto& [childHierarchy, variant] : args.root.mChildren) { - LayerHierarchy::ScopedAddToTraversalPath addChildToPath(root, - childHierarchy->getLayer()->id, - variant); - updateSnapshotsInHierarchy(args, *childHierarchy, root, rootSnapshot, /*depth=*/0); + LayerHierarchy::TraversalPath childPath = + root.makeChild(childHierarchy->getLayer()->id, variant); + updateSnapshotsInHierarchy(args, *childHierarchy, childPath, rootSnapshot, /*depth=*/0); } } @@ -520,7 +519,7 @@ void LayerSnapshotBuilder::update(const Args& args) { const LayerSnapshot& LayerSnapshotBuilder::updateSnapshotsInHierarchy( const Args& args, const LayerHierarchy& hierarchy, - LayerHierarchy::TraversalPath& traversalPath, const LayerSnapshot& parentSnapshot, + const LayerHierarchy::TraversalPath& traversalPath, const LayerSnapshot& parentSnapshot, int depth) { LLOG_ALWAYS_FATAL_WITH_TRACE_IF(depth > 50, "Cycle detected in LayerSnapshotBuilder. See " @@ -549,12 +548,10 @@ const LayerSnapshot& LayerSnapshotBuilder::updateSnapshotsInHierarchy( bool childHasValidFrameRate = false; for (auto& [childHierarchy, variant] : hierarchy.mChildren) { - LayerHierarchy::ScopedAddToTraversalPath addChildToPath(traversalPath, - childHierarchy->getLayer()->id, - variant); + LayerHierarchy::TraversalPath childPath = + traversalPath.makeChild(childHierarchy->getLayer()->id, variant); const LayerSnapshot& childSnapshot = - updateSnapshotsInHierarchy(args, *childHierarchy, traversalPath, *snapshot, - depth + 1); + updateSnapshotsInHierarchy(args, *childHierarchy, childPath, *snapshot, depth + 1); updateFrameRateFromChildSnapshot(*snapshot, childSnapshot, *childHierarchy->getLayer(), args, &childHasValidFrameRate); } diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h index 486cb33959..94b7e5fa5a 100644 --- a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h +++ b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h @@ -106,9 +106,10 @@ private: void updateSnapshots(const Args& args); - const LayerSnapshot& updateSnapshotsInHierarchy(const Args&, const LayerHierarchy& hierarchy, - LayerHierarchy::TraversalPath& traversalPath, - const LayerSnapshot& parentSnapshot, int depth); + const LayerSnapshot& updateSnapshotsInHierarchy( + const Args&, const LayerHierarchy& hierarchy, + const LayerHierarchy::TraversalPath& traversalPath, const LayerSnapshot& parentSnapshot, + int depth); void updateSnapshot(LayerSnapshot&, const Args&, const RequestedLayerState&, const LayerSnapshot& parentSnapshot, const LayerHierarchy::TraversalPath&); static void updateRelativeState(LayerSnapshot& snapshot, const LayerSnapshot& parentSnapshot, diff --git a/services/surfaceflinger/LayerProtoHelper.cpp b/services/surfaceflinger/LayerProtoHelper.cpp index 84b1a73e0b..280d66e12a 100644 --- a/services/surfaceflinger/LayerProtoHelper.cpp +++ b/services/surfaceflinger/LayerProtoHelper.cpp @@ -278,10 +278,9 @@ LayerProtoFromSnapshotGenerator& LayerProtoFromSnapshotGenerator::with( stackIdsToSkip.find(child->getLayer()->layerStack.id) != stackIdsToSkip.end()) { continue; } - frontend::LayerHierarchy::ScopedAddToTraversalPath addChildToPath(path, - child->getLayer()->id, - variant); - LayerProtoFromSnapshotGenerator::writeHierarchyToProto(*child, path); + LayerProtoFromSnapshotGenerator::writeHierarchyToProto(*child, + path.makeChild(child->getLayer()->id, + variant)); } // fill in relative and parent info @@ -338,7 +337,8 @@ LayerProtoFromSnapshotGenerator& LayerProtoFromSnapshotGenerator::withOffscreenL } frontend::LayerSnapshot* LayerProtoFromSnapshotGenerator::getSnapshot( - frontend::LayerHierarchy::TraversalPath& path, const frontend::RequestedLayerState& layer) { + const frontend::LayerHierarchy::TraversalPath& path, + const frontend::RequestedLayerState& layer) { frontend::LayerSnapshot* snapshot = mSnapshotBuilder.getSnapshot(path); if (snapshot) { return snapshot; @@ -349,7 +349,7 @@ frontend::LayerSnapshot* LayerProtoFromSnapshotGenerator::getSnapshot( } void LayerProtoFromSnapshotGenerator::writeHierarchyToProto( - const frontend::LayerHierarchy& root, frontend::LayerHierarchy::TraversalPath& path) { + const frontend::LayerHierarchy& root, const frontend::LayerHierarchy::TraversalPath& path) { using Variant = frontend::LayerHierarchy::Variant; perfetto::protos::LayerProto* layerProto = mLayersProto.add_layers(); const frontend::RequestedLayerState& layer = *root.getLayer(); @@ -362,10 +362,8 @@ void LayerProtoFromSnapshotGenerator::writeHierarchyToProto( LayerProtoHelper::writeSnapshotToProto(layerProto, layer, *snapshot, mTraceFlags); for (const auto& [child, variant] : root.mChildren) { - frontend::LayerHierarchy::ScopedAddToTraversalPath addChildToPath(path, - child->getLayer()->id, - variant); - frontend::LayerSnapshot* childSnapshot = getSnapshot(path, layer); + frontend::LayerSnapshot* childSnapshot = + getSnapshot(path.makeChild(child->getLayer()->id, variant), layer); if (variant == Variant::Attached || variant == Variant::Detached || frontend::LayerHierarchy::isMirror(variant)) { mChildToParent[childSnapshot->uniqueSequence] = snapshot->uniqueSequence; @@ -388,10 +386,7 @@ void LayerProtoFromSnapshotGenerator::writeHierarchyToProto( if (variant == Variant::Detached) { continue; } - frontend::LayerHierarchy::ScopedAddToTraversalPath addChildToPath(path, - child->getLayer()->id, - variant); - writeHierarchyToProto(*child, path); + writeHierarchyToProto(*child, path.makeChild(child->getLayer()->id, variant)); } } diff --git a/services/surfaceflinger/LayerProtoHelper.h b/services/surfaceflinger/LayerProtoHelper.h index 3ca553a903..28924e45be 100644 --- a/services/surfaceflinger/LayerProtoHelper.h +++ b/services/surfaceflinger/LayerProtoHelper.h @@ -98,8 +98,8 @@ public: private: void writeHierarchyToProto(const frontend::LayerHierarchy& root, - frontend::LayerHierarchy::TraversalPath& path); - frontend::LayerSnapshot* getSnapshot(frontend::LayerHierarchy::TraversalPath& path, + const frontend::LayerHierarchy::TraversalPath& path); + frontend::LayerSnapshot* getSnapshot(const frontend::LayerHierarchy::TraversalPath& path, const frontend::RequestedLayerState& layer); const frontend::LayerSnapshotBuilder& mSnapshotBuilder; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 9a0b87deca..2fb3500798 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5696,13 +5696,7 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa incRefreshableDisplays(); } - if (displayId == mActiveDisplayId && - FlagManager::getInstance().correct_virtual_display_power_state()) { - applyOptimizationPolicy(__func__); - } - const auto activeMode = display->refreshRateSelector().getActiveMode().modePtr; - using OptimizationPolicy = gui::ISurfaceComposer::OptimizationPolicy; if (currentMode == hal::PowerMode::OFF) { // Turn on the display @@ -5717,10 +5711,12 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa onActiveDisplayChangedLocked(activeDisplay.get(), *display); } - if (displayId == mActiveDisplayId && - !FlagManager::getInstance().correct_virtual_display_power_state()) { - optimizeThreadScheduling("setPhysicalDisplayPowerMode(ON/DOZE)", - OptimizationPolicy::optimizeForPerformance); + if (displayId == mActiveDisplayId) { + if (FlagManager::getInstance().correct_virtual_display_power_state()) { + applyOptimizationPolicy("setPhysicalDisplayPowerMode(ON)"); + } else { + disablePowerOptimizations("setPhysicalDisplayPowerMode(ON)"); + } } getHwComposer().setPowerMode(displayId, mode); @@ -5729,8 +5725,7 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa mScheduler->getVsyncSchedule(displayId)->getPendingHardwareVsyncState(); requestHardwareVsync(displayId, enable); - if (displayId == mActiveDisplayId && - !FlagManager::getInstance().correct_virtual_display_power_state()) { + if (displayId == mActiveDisplayId) { mScheduler->enableSyntheticVsync(false); } @@ -5747,13 +5742,13 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa if (const auto display = getActivatableDisplay()) { onActiveDisplayChangedLocked(activeDisplay.get(), *display); } else { - if (!FlagManager::getInstance().correct_virtual_display_power_state()) { - optimizeThreadScheduling("setPhysicalDisplayPowerMode(OFF)", - OptimizationPolicy::optimizeForPower); + if (FlagManager::getInstance().correct_virtual_display_power_state()) { + applyOptimizationPolicy("setPhysicalDisplayPowerMode(OFF)"); + } else { + enablePowerOptimizations("setPhysicalDisplayPowerMode(OFF)"); } - if (currentModeNotDozeSuspend && - !FlagManager::getInstance().correct_virtual_display_power_state()) { + if (currentModeNotDozeSuspend) { mScheduler->enableSyntheticVsync(); } } @@ -5781,9 +5776,7 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa ALOGI("Force repainting for DOZE_SUSPEND -> DOZE or ON."); mVisibleRegionsDirty = true; scheduleRepaint(); - if (!FlagManager::getInstance().correct_virtual_display_power_state()) { - mScheduler->enableSyntheticVsync(false); - } + mScheduler->enableSyntheticVsync(false); } constexpr bool kAllowToEnable = true; mScheduler->resyncToHardwareVsync(displayId, kAllowToEnable, activeMode.get()); @@ -5793,8 +5786,7 @@ void SurfaceFlinger::setPhysicalDisplayPowerMode(const sp<DisplayDevice>& displa constexpr bool kDisallow = true; mScheduler->disableHardwareVsync(displayId, kDisallow); - if (displayId == mActiveDisplayId && - !FlagManager::getInstance().correct_virtual_display_power_state()) { + if (displayId == mActiveDisplayId) { mScheduler->enableSyntheticVsync(); } getHwComposer().setPowerMode(displayId, mode); @@ -5833,44 +5825,43 @@ void SurfaceFlinger::setVirtualDisplayPowerMode(const sp<DisplayDevice>& display to_string(displayId).c_str()); } -void SurfaceFlinger::optimizeThreadScheduling( - const char* whence, gui::ISurfaceComposer::OptimizationPolicy optimizationPolicy) { - ALOGD("%s: Optimizing thread scheduling: %s", whence, to_string(optimizationPolicy)); +bool SurfaceFlinger::shouldOptimizeForPerformance() { + for (const auto& [_, display] : mDisplays) { + // Displays that are optimized for power are always powered on and should not influence + // whether there is an active display for the purpose of power optimization, etc. If these + // displays are being shown somewhere, a different (physical or virtual) display that is + // optimized for performance will be powered on in addition. Displays optimized for + // performance will change power mode, so if they are off then they are not active. + if (display->isPoweredOn() && + display->getOptimizationPolicy() == + gui::ISurfaceComposer::OptimizationPolicy::optimizeForPerformance) { + return true; + } + } + return false; +} + +void SurfaceFlinger::enablePowerOptimizations(const char* whence) { + ALOGD("%s: Enabling power optimizations", whence); + + setSchedAttr(false, whence); + setSchedFifo(false, whence); +} + +void SurfaceFlinger::disablePowerOptimizations(const char* whence) { + ALOGD("%s: Disabling power optimizations", whence); - const bool optimizeForPerformance = - optimizationPolicy == gui::ISurfaceComposer::OptimizationPolicy::optimizeForPerformance; // TODO: b/281692563 - Merge the syscalls. For now, keep uclamp in a separate syscall // and set it before SCHED_FIFO due to b/190237315. - setSchedAttr(optimizeForPerformance, whence); - setSchedFifo(optimizeForPerformance, whence); + setSchedAttr(true, whence); + setSchedFifo(true, whence); } void SurfaceFlinger::applyOptimizationPolicy(const char* whence) { - using OptimizationPolicy = gui::ISurfaceComposer::OptimizationPolicy; - - const bool optimizeForPerformance = - std::any_of(mDisplays.begin(), mDisplays.end(), [](const auto& pair) { - const auto& display = pair.second; - return display->isPoweredOn() && - display->getOptimizationPolicy() == - OptimizationPolicy::optimizeForPerformance; - }); - - optimizeThreadScheduling(whence, - optimizeForPerformance ? OptimizationPolicy::optimizeForPerformance - : OptimizationPolicy::optimizeForPower); - - if (mScheduler) { - const bool disableSyntheticVsync = - std::any_of(mDisplays.begin(), mDisplays.end(), [](const auto& pair) { - const auto& display = pair.second; - const hal::PowerMode powerMode = display->getPowerMode(); - return powerMode != hal::PowerMode::OFF && - powerMode != hal::PowerMode::DOZE_SUSPEND && - display->getOptimizationPolicy() == - OptimizationPolicy::optimizeForPerformance; - }); - mScheduler->enableSyntheticVsync(!disableSyntheticVsync); + if (shouldOptimizeForPerformance()) { + disablePowerOptimizations(whence); + } else { + enablePowerOptimizations(whence); } } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index c472c4c6d4..1a09269545 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -739,14 +739,19 @@ private: void setVirtualDisplayPowerMode(const sp<DisplayDevice>& display, hal::PowerMode mode) REQUIRES(mStateLock, kMainThreadContext); - // Adjusts thread scheduling according to the optimization policy - static void optimizeThreadScheduling( - const char* whence, gui::ISurfaceComposer::OptimizationPolicy optimizationPolicy); + // Returns whether to optimize globally for performance instead of power. + bool shouldOptimizeForPerformance() REQUIRES(mStateLock); + + // Turns on power optimizations, for example when there are no displays to be optimized for + // performance. + static void enablePowerOptimizations(const char* whence); + + // Turns off power optimizations. + static void disablePowerOptimizations(const char* whence); // Enables or disables power optimizations depending on whether there are displays that should // be optimized for performance. - void applyOptimizationPolicy(const char* whence) REQUIRES(kMainThreadContext) - REQUIRES(mStateLock); + void applyOptimizationPolicy(const char* whence) REQUIRES(mStateLock); // Returns the preferred mode for PhysicalDisplayId if the Scheduler has selected one for that // display. Falls back to the display's defaultModeId otherwise. diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPhysicalDisplayPowerModeTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPhysicalDisplayPowerModeTest.cpp index 2332bf62da..d5c22a9601 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPhysicalDisplayPowerModeTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPhysicalDisplayPowerModeTest.cpp @@ -17,7 +17,6 @@ #undef LOG_TAG #define LOG_TAG "LibSurfaceFlingerUnittests" -#include <android_companion_virtualdevice_flags.h> #include <com_android_graphics_surfaceflinger_flags.h> #include <common/test/FlagUtils.h> #include "DisplayTransactionTestHelpers.h" @@ -79,19 +78,11 @@ struct EventThreadBaseSupportedVariant { struct EventThreadNotSupportedVariant : public EventThreadBaseSupportedVariant { static void setupEnableVsyncCallExpectations(DisplayTransactionTest* test) { EXPECT_CALL(test->mFlinger.scheduler()->mockRequestHardwareVsync, Call(_, true)).Times(1); - setupDisableSyntheticVsyncCallExpectations(test); - } - - static void setupDisableSyntheticVsyncCallExpectations(DisplayTransactionTest* test) { EXPECT_CALL(*test->mEventThread, enableSyntheticVsync(_)).Times(0); } static void setupDisableVsyncCallExpectations(DisplayTransactionTest* test) { EXPECT_CALL(test->mFlinger.scheduler()->mockRequestHardwareVsync, Call(_, false)).Times(1); - setupEnableSyntheticVsyncCallExpectations(test); - } - - static void setupEnableSyntheticVsyncCallExpectations(DisplayTransactionTest* test) { EXPECT_CALL(*test->mEventThread, enableSyntheticVsync(_)).Times(0); } }; @@ -100,20 +91,12 @@ struct EventThreadIsSupportedVariant : public EventThreadBaseSupportedVariant { static void setupEnableVsyncCallExpectations(DisplayTransactionTest* test) { // Expect to enable hardware VSYNC and disable synthetic VSYNC. EXPECT_CALL(test->mFlinger.scheduler()->mockRequestHardwareVsync, Call(_, true)).Times(1); - setupDisableSyntheticVsyncCallExpectations(test); - } - - static void setupDisableSyntheticVsyncCallExpectations(DisplayTransactionTest* test) { EXPECT_CALL(*test->mEventThread, enableSyntheticVsync(false)).Times(1); } static void setupDisableVsyncCallExpectations(DisplayTransactionTest* test) { // Expect to disable hardware VSYNC and enable synthetic VSYNC. EXPECT_CALL(test->mFlinger.scheduler()->mockRequestHardwareVsync, Call(_, false)).Times(1); - setupEnableSyntheticVsyncCallExpectations(test); - } - - static void setupEnableSyntheticVsyncCallExpectations(DisplayTransactionTest* test) { EXPECT_CALL(*test->mEventThread, enableSyntheticVsync(true)).Times(1); } }; @@ -168,7 +151,7 @@ struct TransitionOffToDozeSuspendVariant template <typename Case> static void setupCallExpectations(DisplayTransactionTest* test) { Case::setupComposerCallExpectations(test, Case::Doze::ACTUAL_POWER_MODE_FOR_DOZE_SUSPEND); - Case::EventThread::setupEnableSyntheticVsyncCallExpectations(test); + Case::EventThread::setupVsyncNoCallExpectations(test); Case::setupRepaintEverythingCallExpectations(test); } @@ -193,7 +176,7 @@ struct TransitionDozeSuspendToOffVariant : public TransitionVariantCommon<PowerMode::DOZE_SUSPEND, PowerMode::OFF> { template <typename Case> static void setupCallExpectations(DisplayTransactionTest* test) { - Case::EventThread::setupEnableSyntheticVsyncCallExpectations(test); + Case::EventThread::setupVsyncNoCallExpectations(test); Case::setupComposerCallExpectations(test, IComposerClient::PowerMode::OFF); } @@ -205,7 +188,7 @@ struct TransitionDozeSuspendToOffVariant struct TransitionOnToDozeVariant : public TransitionVariantCommon<PowerMode::ON, PowerMode::DOZE> { template <typename Case> static void setupCallExpectations(DisplayTransactionTest* test) { - Case::EventThread::setupDisableSyntheticVsyncCallExpectations(test); + Case::EventThread::setupVsyncNoCallExpectations(test); Case::setupComposerCallExpectations(test, Case::Doze::ACTUAL_POWER_MODE_FOR_DOZE); } }; @@ -223,7 +206,7 @@ struct TransitionDozeSuspendToDozeVariant struct TransitionDozeToOnVariant : public TransitionVariantCommon<PowerMode::DOZE, PowerMode::ON> { template <typename Case> static void setupCallExpectations(DisplayTransactionTest* test) { - Case::EventThread::setupDisableSyntheticVsyncCallExpectations(test); + Case::EventThread::setupVsyncNoCallExpectations(test); Case::setupComposerCallExpectations(test, IComposerClient::PowerMode::ON); } }; @@ -251,7 +234,7 @@ struct TransitionOnToUnknownVariant : public TransitionVariantCommon<PowerMode::ON, static_cast<PowerMode>(POWER_MODE_LEET)> { template <typename Case> static void setupCallExpectations(DisplayTransactionTest* test) { - Case::EventThread::setupDisableSyntheticVsyncCallExpectations(test); + Case::EventThread::setupVsyncNoCallExpectations(test); Case::setupNoComposerPowerModeCallExpectations(test); } }; @@ -352,9 +335,6 @@ void SetPhysicalDisplayPowerModeTest::transitionDisplayCommon() { // -------------------------------------------------------------------- // Preconditions - SET_FLAG_FOR_TEST(android::companion::virtualdevice::flags::correct_virtual_display_power_state, - true); - Case::Doze::setupComposerCallExpectations(this); auto display = Case::injectDisplayWithInitialPowerMode(this, Case::Transition::INITIAL_POWER_MODE); |