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

Commit dba591c6 authored by arthurhung's avatar arthurhung Committed by Arthur Hung
Browse files

SF: non blocking createSurface 

The createSurface would add the created layer into current state, which
would hold the state lock and may block the binder thread while the main
thread is holding the same lock.

This CL uses a transaction to perform the behaviors of adding the
created layer into current state, so we could return the created layer
without holding the lock and do other stuff in the main thread.

Also make some test cases to perform a sync transaction after calling
createSurface, that could make sure we have done the layer creation.

Bug: 179647628
Test: atest libsurfaceflinger_unittest
Test: atest SurfaceFlinger_test
Test: libgui_test
Test: SurfaceControlTest

Change-Id: I394b74e9c1cc675df4cacd38ab5da24f0492289d
parent 7d54b9ca

libs/gui/include/gui/LayerState.h

+1 −0
Original line number Diff line number Diff line
@@ -84,6 +84,7 @@ struct layer_state_t { 
        eLayerStackChanged = 0x00000080,

        eReleaseBufferListenerChanged = 0x00000400,

        eShadowRadiusChanged = 0x00000800,

        eLayerCreated = 0x00001000,

        /* was eDetachChildren, now available 0x00002000, */

        eRelativeLayerChanged = 0x00004000,

        eReparent = 0x00008000,

libs/gui/tests/Surface_test.cpp

+2 −3
Original line number Diff line number Diff line
@@ -102,14 +102,13 @@ protected: 
        //   test flakiness.

        mSurfaceControl = mComposerClient->createSurface(

                String8("Test Surface"), 32, 32, PIXEL_FORMAT_RGBA_8888, 0);

        SurfaceComposerClient::Transaction().apply(true);



        ASSERT_TRUE(mSurfaceControl != nullptr);

        ASSERT_TRUE(mSurfaceControl->isValid());



        Transaction t;

        ASSERT_EQ(NO_ERROR, t.setLayer(mSurfaceControl, 0x7fffffff)

                .show(mSurfaceControl)

                .apply());

        ASSERT_EQ(NO_ERROR, t.setLayer(mSurfaceControl, 0x7fffffff).show(mSurfaceControl).apply());



        mSurface = mSurfaceControl->getSurface();

        ASSERT_TRUE(mSurface != nullptr);

services/surfaceflinger/SurfaceFlinger.cpp

+121 −70
Original line number Diff line number Diff line
@@ -2750,6 +2750,7 @@ void SurfaceFlinger::processDisplayChanged(const wp<IBinder>& displayToken, 
            (currentState.orientedDisplaySpaceRect != drawingState.orientedDisplaySpaceRect)) {

            display->setProjection(currentState.orientation, currentState.layerStackSpaceRect,

                                   currentState.orientedDisplaySpaceRect);

            mDefaultDisplayTransformHint = display->getTransformHint();

        }

        if (currentState.width != drawingState.width ||

            currentState.height != drawingState.height) {
@@ -3256,70 +3257,38 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client, const sp<IBind 
                                        const sp<IBinder>& parentHandle,

                                        const sp<Layer>& parentLayer, bool addToCurrentState,

                                        uint32_t* outTransformHint) {

    // add this layer to the current state list

    {

        Mutex::Autolock _l(mStateLock);

        sp<Layer> parent;

        if (parentHandle != nullptr) {

            parent = fromHandleLocked(parentHandle).promote();

            if (parent == nullptr) {

                return NAME_NOT_FOUND;

            }

        } else {

            parent = parentLayer;

        }



    if (mNumLayers >= ISurfaceComposer::MAX_LAYERS) {

        ALOGE("AddClientLayer failed, mNumLayers (%zu) >= MAX_LAYERS (%zu)", mNumLayers.load(),

              ISurfaceComposer::MAX_LAYERS);

        return NO_MEMORY;

    }



        mLayersByLocalBinderToken.emplace(handle->localBinder(), lbc);



        if (parent == nullptr && addToCurrentState) {

            mCurrentState.layersSortedByZ.add(lbc);

        } else if (parent == nullptr) {

            lbc->onRemovedFromCurrentState();

        } else if (parent->isRemovedFromCurrentState()) {

            parent->addChild(lbc);

            lbc->onRemovedFromCurrentState();

        } else {

            parent->addChild(lbc);

        }



    wp<IBinder> initialProducer;

    if (gbc != nullptr) {

            mGraphicBufferProducerList.insert(IInterface::asBinder(gbc).get());

            LOG_ALWAYS_FATAL_IF(mGraphicBufferProducerList.size() >

                                        mMaxGraphicBufferProducerListSize,

                                "Suspected IGBP leak: %zu IGBPs (%zu max), %zu Layers",

                                mGraphicBufferProducerList.size(),

                                mMaxGraphicBufferProducerListSize, mNumLayers.load());

            if (mGraphicBufferProducerList.size() > mGraphicBufferProducerListSizeLogThreshold) {

                ALOGW("Suspected IGBP leak: %zu IGBPs (%zu max), %zu Layers",

                      mGraphicBufferProducerList.size(), mMaxGraphicBufferProducerListSize,

                      mNumLayers.load());

            }

        initialProducer = IInterface::asBinder(gbc);

    }

    setLayerCreatedState(handle, lbc, parentHandle, parentLayer, initialProducer);



        if (const auto token = getInternalDisplayTokenLocked()) {

            const ssize_t index = mCurrentState.displays.indexOfKey(token);

            if (index >= 0) {

                const DisplayDeviceState& state = mCurrentState.displays.valueAt(index);

                lbc->updateTransformHint(ui::Transform::toRotationFlags(state.orientation));

            }

        }

        if (outTransformHint) {

            *outTransformHint = lbc->getTransformHint();

        }

    // Create a transaction includes the initial parent and producer.

    Vector<ComposerState> states;

    Vector<DisplayState> displays;



        mLayersAdded = true;

    }

    ComposerState composerState;

    composerState.state.what = layer_state_t::eLayerCreated;

    composerState.state.surface = handle;

    states.add(composerState);



    lbc->updateTransformHint(mDefaultDisplayTransformHint);

    if (outTransformHint) {

        *outTransformHint = mDefaultDisplayTransformHint;

    }

    // attach this layer to the client

    client->attachLayer(handle, lbc);



    return NO_ERROR;

    return setTransactionState(FrameTimelineInfo{}, states, displays, 0 /* flags */, nullptr,

                               InputWindowCommands{}, -1 /* desiredPresentTime */,

                               true /* isAutoTimestamp */, {}, false /* hasListenerCallbacks */, {},

                               0 /* Undefined transactionId */);

}



void SurfaceFlinger::removeGraphicBufferProducerAsync(const wp<IBinder>& binder) {
@@ -3799,9 +3768,21 @@ uint32_t SurfaceFlinger::setClientStateLocked( 
        listenerCallbacks.insert(listener);

    }



    const uint64_t what = s.what;

    uint32_t flags = 0;

    sp<Layer> layer = nullptr;

    if (s.surface) {

        if (what & layer_state_t::eLayerCreated) {

            layer = handleLayerCreatedLocked(s.surface, privileged);

            if (layer) {

                // put the created layer into mLayersByLocalBinderToken.

                mLayersByLocalBinderToken.emplace(s.surface->localBinder(), layer);

                flags |= eTransactionNeeded | eTraversalNeeded;

                mLayersAdded = true;

            }

        } else {

            layer = fromHandleLocked(s.surface).promote();

        }

    } else {

        // The client may provide us a null handle. Treat it as if the layer was removed.

        ALOGW("Attempt to set client state with a null layer handle");
@@ -3814,10 +3795,6 @@ uint32_t SurfaceFlinger::setClientStateLocked( 
        return 0;

    }



    uint32_t flags = 0;



    const uint64_t what = s.what;



    // Only set by BLAST adapter layers

    if (what & layer_state_t::eProducerDisconnect) {

        layer->onDisconnect();
@@ -4278,14 +4255,7 @@ status_t SurfaceFlinger::createBufferStateLayer(const sp<Client>& client, std::s 
                                                sp<Layer>* outLayer) {

    LayerCreationArgs args(this, client, std::move(name), w, h, flags, std::move(metadata));

    args.textureName = getNewTexture();

    sp<BufferStateLayer> layer;

    {

        // TODO (b/173538294): Investigate why we need mStateLock here and above in

        // createBufferQueue layer. Is it the renderengine::Image?

        Mutex::Autolock lock(mStateLock);

        layer = getFactory().createBufferStateLayer(args);



    }

    sp<BufferStateLayer> layer = getFactory().createBufferStateLayer(args);

    *handle = layer->getHandle();

    *outLayer = layer;
@@ -4373,7 +4343,7 @@ void SurfaceFlinger::onInitializeDisplays() { 
    setPowerModeInternal(display, hal::PowerMode::ON);

    const nsecs_t vsyncPeriod = mRefreshRateConfigs->getCurrentRefreshRate().getVsyncPeriod();

    mAnimFrameTracker.setDisplayRefreshPeriod(vsyncPeriod);



    mDefaultDisplayTransformHint = display->getTransformHint();

    // Use phase of 0 since phase is not known.

    // Use latency of 0, which will snap to the ideal latency.

    DisplayStatInfo stats{0 /* vsyncTime */, vsyncPeriod};
@@ -6643,6 +6613,87 @@ void SurfaceFlinger::TransactionState::traverseStatesWithBuffers( 
    }

}



void SurfaceFlinger::setLayerCreatedState(const sp<IBinder>& handle, const wp<Layer>& layer,

                                          const wp<IBinder>& parent, const wp<Layer> parentLayer,

                                          const wp<IBinder>& producer) {

    Mutex::Autolock lock(mCreatedLayersLock);

    mCreatedLayers[handle->localBinder()] =

            std::make_unique<LayerCreatedState>(layer, parent, parentLayer, producer);

}



auto SurfaceFlinger::getLayerCreatedState(const sp<IBinder>& handle) {

    Mutex::Autolock lock(mCreatedLayersLock);

    BBinder* b = nullptr;

    if (handle) {

        b = handle->localBinder();

    }



    if (b == nullptr) {

        return std::unique_ptr<LayerCreatedState>(nullptr);

    }



    auto it = mCreatedLayers.find(b);

    if (it == mCreatedLayers.end()) {

        ALOGE("Can't find layer from handle %p", handle.get());

        return std::unique_ptr<LayerCreatedState>(nullptr);

    }



    auto state = std::move(it->second);

    mCreatedLayers.erase(it);

    return state;

}



sp<Layer> SurfaceFlinger::handleLayerCreatedLocked(const sp<IBinder>& handle, bool privileged) {

    const auto& state = getLayerCreatedState(handle);

    if (!state) {

        return nullptr;

    }



    sp<Layer> layer = state->layer.promote();

    if (!layer) {

        ALOGE("Invalid layer %p", state->layer.unsafe_get());

        return nullptr;

    }



    sp<Layer> parent;

    bool allowAddRoot = privileged;

    if (state->initialParent != nullptr) {

        parent = fromHandleLocked(state->initialParent.promote()).promote();

        if (parent == nullptr) {

            ALOGE("Invalid parent %p", state->initialParent.unsafe_get());

            allowAddRoot = false;

        }

    } else if (state->initialParentLayer != nullptr) {

        parent = state->initialParentLayer.promote();

        allowAddRoot = false;

    }



    if (parent == nullptr && allowAddRoot) {

        mCurrentState.layersSortedByZ.add(layer);

    } else if (parent == nullptr) {

        layer->onRemovedFromCurrentState();

    } else if (parent->isRemovedFromCurrentState()) {

        parent->addChild(layer);

        layer->onRemovedFromCurrentState();

    } else {

        parent->addChild(layer);

    }



    if (state->initialProducer != nullptr) {

        mGraphicBufferProducerList.insert(state->initialProducer);

        LOG_ALWAYS_FATAL_IF(mGraphicBufferProducerList.size() > mMaxGraphicBufferProducerListSize,

                            "Suspected IGBP leak: %zu IGBPs (%zu max), %zu Layers",

                            mGraphicBufferProducerList.size(), mMaxGraphicBufferProducerListSize,

                            mNumLayers.load());

        if (mGraphicBufferProducerList.size() > mGraphicBufferProducerListSizeLogThreshold) {

            ALOGW("Suspected IGBP leak: %zu IGBPs (%zu max), %zu Layers",

                  mGraphicBufferProducerList.size(), mMaxGraphicBufferProducerListSize,

                  mNumLayers.load());

        }

    }



    return layer;

}

} // namespace android



#if defined(__gl_h_)

services/surfaceflinger/SurfaceFlinger.h

+29 −0
Original line number Diff line number Diff line
@@ -1393,6 +1393,35 @@ private: 


    std::unordered_map<DisplayId, sp<HdrLayerInfoReporter>> mHdrLayerInfoListeners

            GUARDED_BY(mStateLock);

    mutable Mutex mCreatedLayersLock;

    struct LayerCreatedState {

        LayerCreatedState(const wp<Layer>& layer, const wp<IBinder>& parent,

                          const wp<Layer> parentLayer, const wp<IBinder>& producer)

              : layer(layer),

                initialParent(parent),

                initialParentLayer(parentLayer),

                initialProducer(producer) {}

        wp<Layer> layer;

        // Indicates the initial parent of the created layer, only used for creating layer in

        // SurfaceFlinger. If nullptr, it may add the created layer into the current root layers.

        wp<IBinder> initialParent;

        wp<Layer> initialParentLayer;

        // Indicates the initial graphic buffer producer of the created layer, only used for

        // creating layer in SurfaceFlinger.

        wp<IBinder> initialProducer;

    };



    // A temporay pool that store the created layers and will be added to current state in main

    // thread.

    std::unordered_map<BBinder*, std::unique_ptr<LayerCreatedState>> mCreatedLayers;

    void setLayerCreatedState(const sp<IBinder>& handle, const wp<Layer>& layer,

                              const wp<IBinder>& parent, const wp<Layer> parentLayer,

                              const wp<IBinder>& producer);

    auto getLayerCreatedState(const sp<IBinder>& handle);

    sp<Layer> handleLayerCreatedLocked(const sp<IBinder>& handle, bool privileged)

            REQUIRES(mStateLock);



    std::atomic<ui::Transform::RotationFlags> mDefaultDisplayTransformHint;

};



} // namespace android

services/surfaceflinger/tests/InvalidHandles_test.cpp

+9 −6
Original line number Diff line number Diff line
@@ -52,12 +52,15 @@ protected: 
    }

};



TEST_F(InvalidHandleTest, createSurfaceInvalidHandle) {

    auto notSc = makeNotSurfaceControl();

    ASSERT_EQ(nullptr,

              mScc->createSurface(String8("lolcats"), 19, 47, PIXEL_FORMAT_RGBA_8888, 0,

                                  notSc->getHandle())

                      .get());

TEST_F(InvalidHandleTest, createSurfaceInvalidParentHandle) {

    // The createSurface is scheduled now, we could still get a created surface from createSurface.

    // Should verify if it actually added into current state by checking the screenshot.

    auto notSc = mScc->createSurface(String8("lolcats"), 19, 47, PIXEL_FORMAT_RGBA_8888, 0,

                                     mNotSc->getHandle());

    LayerCaptureArgs args;

    args.layerHandle = notSc->getHandle();

    ScreenCaptureResults captureResults;

    ASSERT_EQ(NAME_NOT_FOUND, ScreenCapture::captureLayers(args, captureResults));

}



TEST_F(InvalidHandleTest, captureLayersInvalidHandle) {