Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit aadc62d7 authored by Leon Scroggins III's avatar Leon Scroggins III
Browse files

Fix subtle bug in disableHardwareVsync 

I54a1304a3428968134cc707b24d5b325927c31df introduced a bug in
disableHardwareVsync. If you pass false for `disallow`, it can switch
from the `Disallowed` state to the `Disabled` state, which is not
intended. Correct this by leaving the state unchanged if it's already
`Disallowed`.

Add tests. DisableDoesNotMakeAllowed catches the failure. While we're at
it, add a test that verifies that it disallows when it's intended to,
versions of the existing tests calling disableHardwareVsync that
pass true for `disallow`, and a test verifying that disable leaves it
allowed when passing `false`.

Bug: 241286146
Test: VsyncScheduleTest
Change-Id: I0036ba97b28bef64f9bae7c1c93f3c31e8733f48
parent 1af0fb6e

services/surfaceflinger/Scheduler/VsyncSchedule.cpp

+9 −3
Original line number Diff line number Diff line
@@ -176,10 +176,16 @@ void VsyncSchedule::enableHardwareVsyncLocked(ISchedulerCallback& callback) { 


void VsyncSchedule::disableHardwareVsync(ISchedulerCallback& callback, bool disallow) {

    std::lock_guard<std::mutex> lock(mHwVsyncLock);

    if (mHwVsyncState == HwVsyncState::Enabled) {

    switch (mHwVsyncState) {

        case HwVsyncState::Enabled:

            callback.setVsyncEnabled(mId, false);

    }

            [[fallthrough]];

        case HwVsyncState::Disabled:

            mHwVsyncState = disallow ? HwVsyncState::Disallowed : HwVsyncState::Disabled;

            break;

        case HwVsyncState::Disallowed:

            break;

    }

}



bool VsyncSchedule::isHardwareVsyncAllowed(bool makeAllowed) {

services/surfaceflinger/tests/unittests/VsyncScheduleTest.cpp

+41 −0
Original line number Diff line number Diff line
@@ -86,6 +86,12 @@ TEST_F(VsyncScheduleTest, DisableDoesNothingWhenDisallowed) { 
    mVsyncSchedule->disableHardwareVsync(mCallback, false /* disallow */);

}



TEST_F(VsyncScheduleTest, DisableDoesNothingWhenDisallowed2) {

    EXPECT_CALL(mCallback, setVsyncEnabled(_, _)).Times(0);



    mVsyncSchedule->disableHardwareVsync(mCallback, true /* disallow */);

}



TEST_F(VsyncScheduleTest, MakeAllowed) {

    ASSERT_TRUE(mVsyncSchedule->isHardwareVsyncAllowed(true /* makeAllowed */));

}
@@ -97,6 +103,13 @@ TEST_F(VsyncScheduleTest, DisableDoesNothingWhenDisabled) { 
    mVsyncSchedule->disableHardwareVsync(mCallback, false /* disallow */);

}



TEST_F(VsyncScheduleTest, DisableDoesNothingWhenDisabled2) {

    ASSERT_TRUE(mVsyncSchedule->isHardwareVsyncAllowed(true /* makeAllowed */));

    EXPECT_CALL(mCallback, setVsyncEnabled(_, _)).Times(0);



    mVsyncSchedule->disableHardwareVsync(mCallback, true /* disallow */);

}



TEST_F(VsyncScheduleTest, EnableWorksWhenDisabled) {

    ASSERT_TRUE(mVsyncSchedule->isHardwareVsyncAllowed(true /* makeAllowed */));

    EXPECT_CALL(mCallback, setVsyncEnabled(DEFAULT_DISPLAY_ID, true));
@@ -129,6 +142,16 @@ TEST_F(VsyncScheduleTest, EnableDisable) { 
    mVsyncSchedule->disableHardwareVsync(mCallback, false /* disallow */);

}



TEST_F(VsyncScheduleTest, EnableDisable2) {

    ASSERT_TRUE(mVsyncSchedule->isHardwareVsyncAllowed(true /* makeAllowed */));

    EXPECT_CALL(mCallback, setVsyncEnabled(DEFAULT_DISPLAY_ID, true));



    mVsyncSchedule->enableHardwareVsync(mCallback);



    EXPECT_CALL(mCallback, setVsyncEnabled(DEFAULT_DISPLAY_ID, false));

    mVsyncSchedule->disableHardwareVsync(mCallback, true /* disallow */);

}



TEST_F(VsyncScheduleTest, StartPeriodTransition) {

    // Note: startPeriodTransition is only called when hardware vsyncs are

    // allowed.
@@ -225,5 +248,23 @@ TEST_F(VsyncScheduleTest, PendingState) FTL_FAKE_GUARD(kMainThreadContext) { 
    ASSERT_FALSE(mVsyncSchedule->getPendingHardwareVsyncState());

}



TEST_F(VsyncScheduleTest, DisableDoesNotMakeAllowed) {

    ASSERT_FALSE(mVsyncSchedule->isHardwareVsyncAllowed(false /* makeAllowed */));

    mVsyncSchedule->disableHardwareVsync(mCallback, false /* disallow */);

    ASSERT_FALSE(mVsyncSchedule->isHardwareVsyncAllowed(false /* makeAllowed */));

}



TEST_F(VsyncScheduleTest, DisallowMakesNotAllowed) {

    ASSERT_TRUE(mVsyncSchedule->isHardwareVsyncAllowed(true /* makeAllowed */));

    mVsyncSchedule->disableHardwareVsync(mCallback, true /* disallow */);

    ASSERT_FALSE(mVsyncSchedule->isHardwareVsyncAllowed(false /* makeAllowed */));

}



TEST_F(VsyncScheduleTest, StillAllowedAfterDisable) {

    ASSERT_TRUE(mVsyncSchedule->isHardwareVsyncAllowed(true /* makeAllowed */));

    mVsyncSchedule->disableHardwareVsync(mCallback, false /* disallow */);

    ASSERT_TRUE(mVsyncSchedule->isHardwareVsyncAllowed(false /* makeAllowed */));

}



} // namespace

} // namespace android