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

Commit b42cdc17 authored by Ady Abraham's avatar Ady Abraham
Browse files

SF: do not skip validate if it is called more than a vsync earlier 

To prevent an early present, we don't want to skip validate and
present immediately if we started the frame to early. Instead we will
do the validate and wait before calling to present. The reason why we
can't wait before validate, is that we might need to do client
composition after validate (based on hwc composition strategy)
and we might miss a frame if we waited before validate.


Bug: 187871031
Test: observe systraces
Change-Id: I9b4b0d4c9c58ddc703bcddadd44dfb0e7eff1cbd
parent 562c2718

services/surfaceflinger/CompositionEngine/src/Display.cpp

+2 −7
Original line number Diff line number Diff line
@@ -267,7 +267,7 @@ void Display::chooseCompositionStrategy() { 
    auto& hwc = getCompositionEngine().getHwComposer();

    if (status_t result =

                hwc.getDeviceCompositionChanges(*halDisplayId, anyLayersRequireClientComposition(),

                                                &changes);

                                                getState().earliestPresentTime, &changes);

        result != NO_ERROR) {

        ALOGE("chooseCompositionStrategy failed for %s: %d (%s)", getName().c_str(), result,

              strerror(-result));
@@ -367,13 +367,8 @@ compositionengine::Output::FrameFences Display::presentAndGetFrameFences() { 
        return fences;

    }



    {

        ATRACE_NAME("wait for earliest present time");

        std::this_thread::sleep_until(getState().earliestPresentTime);

    }



    auto& hwc = getCompositionEngine().getHwComposer();

    hwc.presentAndGetReleaseFences(*halDisplayIdOpt);

    hwc.presentAndGetReleaseFences(*halDisplayIdOpt, getState().earliestPresentTime);



    fences.presentFence = hwc.getPresentFence(*halDisplayIdOpt);

services/surfaceflinger/CompositionEngine/tests/DisplayTest.cpp

+9 −6
Original line number Diff line number Diff line
@@ -627,7 +627,7 @@ TEST_F(DisplayChooseCompositionStrategyTest, takesEarlyOutIfNotAHwcDisplay) { 
TEST_F(DisplayChooseCompositionStrategyTest, takesEarlyOutOnHwcError) {

    EXPECT_CALL(*mDisplay, anyLayersRequireClientComposition()).WillOnce(Return(false));

    EXPECT_CALL(mHwComposer,

                getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), false, _))

                getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), false, _, _))

            .WillOnce(Return(INVALID_OPERATION));



    mDisplay->chooseCompositionStrategy();
@@ -649,7 +649,8 @@ TEST_F(DisplayChooseCompositionStrategyTest, normalOperation) { 
            .InSequence(s)

            .WillOnce(Return(false));



    EXPECT_CALL(mHwComposer, getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), true, _))

    EXPECT_CALL(mHwComposer,

                getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), true, _, _))

            .WillOnce(Return(NO_ERROR));

    EXPECT_CALL(*mDisplay, allLayersRequireClientComposition()).WillOnce(Return(false));
@@ -679,8 +680,9 @@ TEST_F(DisplayChooseCompositionStrategyTest, normalOperationWithChanges) { 
            .InSequence(s)

            .WillOnce(Return(false));



    EXPECT_CALL(mHwComposer, getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), true, _))

            .WillOnce(DoAll(SetArgPointee<2>(changes), Return(NO_ERROR)));

    EXPECT_CALL(mHwComposer,

                getDeviceCompositionChanges(HalDisplayId(DEFAULT_DISPLAY_ID), true, _, _))

            .WillOnce(DoAll(SetArgPointee<3>(changes), Return(NO_ERROR)));

    EXPECT_CALL(*mDisplay, applyChangedTypesToLayers(changes.changedTypes)).Times(1);

    EXPECT_CALL(*mDisplay, applyDisplayRequests(changes.displayRequests)).Times(1);

    EXPECT_CALL(*mDisplay, applyLayerRequestsToLayers(changes.layerRequests)).Times(1);
@@ -867,7 +869,8 @@ TEST_F(DisplayPresentAndGetFrameFencesTest, returnsPresentAndLayerFences) { 
    sp<Fence> layer1Fence = new Fence();

    sp<Fence> layer2Fence = new Fence();



    EXPECT_CALL(mHwComposer, presentAndGetReleaseFences(HalDisplayId(DEFAULT_DISPLAY_ID))).Times(1);

    EXPECT_CALL(mHwComposer, presentAndGetReleaseFences(HalDisplayId(DEFAULT_DISPLAY_ID), _))

            .Times(1);

    EXPECT_CALL(mHwComposer, getPresentFence(HalDisplayId(DEFAULT_DISPLAY_ID)))

            .WillOnce(Return(presentFence));

    EXPECT_CALL(mHwComposer,
@@ -1039,7 +1042,7 @@ TEST_F(DisplayFunctionalTest, postFramebufferCriticalCallsAreOrdered) { 


    mDisplay->editState().isEnabled = true;



    EXPECT_CALL(mHwComposer, presentAndGetReleaseFences(_));

    EXPECT_CALL(mHwComposer, presentAndGetReleaseFences(_, _));

    EXPECT_CALL(*mDisplaySurface, onFrameCommitted());



    mDisplay->postFramebuffer();

services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h

+4 −3
Original line number Diff line number Diff line
@@ -50,13 +50,14 @@ public: 
    MOCK_METHOD2(allocatePhysicalDisplay, void(hal::HWDisplayId, PhysicalDisplayId));

    MOCK_METHOD1(createLayer, HWC2::Layer*(HalDisplayId));

    MOCK_METHOD2(destroyLayer, void(HalDisplayId, HWC2::Layer*));

    MOCK_METHOD3(getDeviceCompositionChanges,

                 status_t(HalDisplayId, bool,

    MOCK_METHOD4(getDeviceCompositionChanges,

                 status_t(HalDisplayId, bool, std::chrono::steady_clock::time_point,

                          std::optional<android::HWComposer::DeviceRequestedChanges>*));

    MOCK_METHOD5(setClientTarget,

                 status_t(HalDisplayId, uint32_t, const sp<Fence>&, const sp<GraphicBuffer>&,

                          ui::Dataspace));

    MOCK_METHOD1(presentAndGetReleaseFences, status_t(HalDisplayId));

    MOCK_METHOD2(presentAndGetReleaseFences,

                 status_t(HalDisplayId, std::chrono::steady_clock::time_point));

    MOCK_METHOD2(setPowerMode, status_t(PhysicalDisplayId, hal::PowerMode));

    MOCK_METHOD2(setActiveConfig, status_t(HalDisplayId, size_t));

    MOCK_METHOD2(setColorTransform, status_t(HalDisplayId, const mat4&));

services/surfaceflinger/DisplayHardware/HWComposer.cpp

+15 −6
Original line number Diff line number Diff line
@@ -471,6 +471,7 @@ status_t HWComposer::setClientTarget(HalDisplayId displayId, uint32_t slot, 


status_t HWComposer::getDeviceCompositionChanges(

        HalDisplayId displayId, bool frameUsesClientComposition,

        std::chrono::steady_clock::time_point earliestPresentTime,

        std::optional<android::HWComposer::DeviceRequestedChanges>* outChanges) {

    ATRACE_CALL();
@@ -487,12 +488,14 @@ status_t HWComposer::getDeviceCompositionChanges( 


    hal::Error error = hal::Error::NONE;



    // First try to skip validate altogether when there is no client

    // composition.  When there is client composition, since we haven't

    // rendered to the client target yet, we should not attempt to skip

    // validate.

    // First try to skip validate altogether when we passed the earliest time

    // to present and there is no client. Otherwise, we may present a frame too

    // early or in case of client composition we first need to render the

    // client target buffer.

    const bool canSkipValidate =

            std::chrono::steady_clock::now() >= earliestPresentTime && !frameUsesClientComposition;

    displayData.validateWasSkipped = false;

    if (!frameUsesClientComposition) {

    if (canSkipValidate) {

        sp<Fence> outPresentFence;

        uint32_t state = UINT32_MAX;

        error = hwcDisplay->presentOrValidate(&numTypes, &numRequests, &outPresentFence , &state);
@@ -556,7 +559,8 @@ sp<Fence> HWComposer::getLayerReleaseFence(HalDisplayId displayId, HWC2::Layer* 
    return fence->second;

}



status_t HWComposer::presentAndGetReleaseFences(HalDisplayId displayId) {

status_t HWComposer::presentAndGetReleaseFences(

        HalDisplayId displayId, std::chrono::steady_clock::time_point earliestPresentTime) {

    ATRACE_CALL();



    RETURN_IF_INVALID_DISPLAY(displayId, BAD_INDEX);
@@ -572,6 +576,11 @@ status_t HWComposer::presentAndGetReleaseFences(HalDisplayId displayId) { 
        return NO_ERROR;

    }



    {

        ATRACE_NAME("wait for earliest present time");

        std::this_thread::sleep_until(earliestPresentTime);

    }



    auto error = hwcDisplay->present(&displayData.lastPresentFence);

    RETURN_IF_HWC_ERROR_FOR("present", error, displayId, UNKNOWN_ERROR);

services/surfaceflinger/DisplayHardware/HWComposer.h

+6 −2
Original line number Diff line number Diff line
@@ -129,13 +129,15 @@ public: 
    // expected.

    virtual status_t getDeviceCompositionChanges(

            HalDisplayId, bool frameUsesClientComposition,

            std::chrono::steady_clock::time_point earliestPresentTime,

            std::optional<DeviceRequestedChanges>* outChanges) = 0;



    virtual status_t setClientTarget(HalDisplayId, uint32_t slot, const sp<Fence>& acquireFence,

                                     const sp<GraphicBuffer>& target, ui::Dataspace) = 0;



    // Present layers to the display and read releaseFences.

    virtual status_t presentAndGetReleaseFences(HalDisplayId) = 0;

    virtual status_t presentAndGetReleaseFences(

            HalDisplayId, std::chrono::steady_clock::time_point earliestPresentTime) = 0;



    // set power mode

    virtual status_t setPowerMode(PhysicalDisplayId, hal::PowerMode) = 0;
@@ -268,13 +270,15 @@ public: 


    status_t getDeviceCompositionChanges(

            HalDisplayId, bool frameUsesClientComposition,

            std::chrono::steady_clock::time_point earliestPresentTime,

            std::optional<DeviceRequestedChanges>* outChanges) override;



    status_t setClientTarget(HalDisplayId, uint32_t slot, const sp<Fence>& acquireFence,

                             const sp<GraphicBuffer>& target, ui::Dataspace) override;



    // Present layers to the display and read releaseFences.

    status_t presentAndGetReleaseFences(HalDisplayId) override;

    status_t presentAndGetReleaseFences(

            HalDisplayId, std::chrono::steady_clock::time_point earliestPresentTime) override;



    // set power mode

    status_t setPowerMode(PhysicalDisplayId, hal::PowerMode mode) override;