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

Commit 2526b2f2 authored by Nolan Scobie's avatar Nolan Scobie
Browse files

Fix RE's VulkanInterface destruction & unnecessary initialization 

SurfaceFlinger's initialization of RE now:
- Only attempts to check for VK support if either the GaneshVk or
  GraphiteVk flag is set.
- Caches the VulkanInterface created to check for VK support until a VK
  instance of RE is created.
- Tears down a partially initialized VulkanInterface if some required
  feature is unsupported.

Additionally, SkiaVkRenderEngine's destructor now tears down the static
VulkanInterfaces it uses, which necessitates:
- Caching whether VK is supported.
- Ensuring all Skia resources are destroyed *before* the VK resources
  managed by VulkanInterface that they rely on are torn down. This
  involves ensuring all textures are destroyed, and all Skia
  context-like objects are destroyed.

This latter change means that tests in librenderengine_test that are
parameterized by Skia backend must now recreate RE's VulkanInterfaces
twice for each test case (once for GaneshVk, and again for GraphiteVk),
which results in a minor regression in test duration. However, this is
necessary because holding on to a VulkanInterface while attempting to
set up a test for GaneshGL will cause contention over the real-time
GPU context priority, which can only be held exclusively by either a GL
context OR a VK context on some hardware.

Many thanks to joseph.cheng@imgtec.com for raising the issue of SF's
init of RE not tearing down the VulkanInterface used to check for
support, and for proposing an initial patch (b/333477752) which this
change builds upon. And thanks to scroggo@google.com for proposing the
reordering of SF's flag vs. VK support checks.

Bug: b/293371537
Bug: b/333477752
Test: librenderengine_test && manual boot validation across 3 backends
Flag: com.android.graphics.surfaceflinger.flags.graphite_renderengine-READ-ONLY
Change-Id: I289c3f7699d16707d1462179f4d5e8c54e4bb049
parent 70da543d

libs/renderengine/include/renderengine/RenderEngine.h

+18 −1
Original line number Diff line number Diff line
@@ -120,7 +120,24 @@ public: 


    static std::unique_ptr<RenderEngine> create(const RenderEngineCreationArgs& args);



    static bool canSupport(GraphicsApi);

    // Check if the device supports the given GraphicsApi.

    //

    // If called for GraphicsApi::VK then underlying (unprotected) VK resources will be preserved

    // to optimize subsequent VK initialization, but teardown(GraphicsApi::VK) must be invoked if

    // the caller subsequently decides to NOT use VK.

    //

    // The first call may require significant resource initialization, but subsequent checks are

    // cached internally.

    static bool canSupport(GraphicsApi graphicsApi);



    // Teardown any GPU API resources that were previously initialized but are no longer needed.

    //

    // Must be called with GraphicsApi::VK if canSupport(GraphicsApi::VK) was previously invoked but

    // the caller subsequently decided to not use VK.

    //

    // This is safe to call if there is nothing to teardown, but NOT safe to call if a RenderEngine

    // instance exists. The RenderEngine destructor will handle its own teardown logic.

    static void teardown(GraphicsApi graphicsApi);



    virtual ~RenderEngine() = 0;

libs/renderengine/skia/SkiaGLRenderEngine.cpp

+1 −1
Original line number Diff line number Diff line
@@ -279,7 +279,7 @@ SkiaGLRenderEngine::SkiaGLRenderEngine(const RenderEngineCreationArgs& args, EGL 
        mProtectedPlaceholderSurface(protectedPlaceholder) {}



SkiaGLRenderEngine::~SkiaGLRenderEngine() {

    finishRenderingAndAbandonContext();

    finishRenderingAndAbandonContexts();

    if (mPlaceholderSurface != EGL_NO_SURFACE) {

        eglDestroySurface(mEGLDisplay, mPlaceholderSurface);

    }

libs/renderengine/skia/SkiaRenderEngine.cpp

+10 −8
Original line number Diff line number Diff line
@@ -297,21 +297,23 @@ SkiaRenderEngine::SkiaRenderEngine(Threaded threaded, PixelFormat pixelFormat, 


SkiaRenderEngine::~SkiaRenderEngine() { }



// To be called from backend dtors.

void SkiaRenderEngine::finishRenderingAndAbandonContext() {

// To be called from backend dtors. Used to clean up Skia objects before GPU API contexts are

// destroyed by subclasses.

void SkiaRenderEngine::finishRenderingAndAbandonContexts() {

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



    if (mBlurFilter) {

        delete mBlurFilter;

    }



    if (mContext) {

        mContext->finishRenderingAndAbandonContext();

    }

    // Leftover textures may hold refs to backend-specific Skia contexts, which must be released

    // before ~SkiaGpuContext is called.

    mTextureCleanupMgr.setDeferredStatus(false);

    mTextureCleanupMgr.cleanup();



    if (mProtectedContext) {

        mProtectedContext->finishRenderingAndAbandonContext();

    }

    // ~SkiaGpuContext must be called before GPU API contexts are torn down.

    mContext.reset();

    mProtectedContext.reset();

}



void SkiaRenderEngine::useProtectedContext(bool useProtectedContext) {

libs/renderengine/skia/SkiaRenderEngine.h

+3 −3
Original line number Diff line number Diff line
@@ -79,9 +79,9 @@ public: 
    void ensureContextsCreated();



protected:

    // This is so backends can stop the generic rendering state first before

    // cleaning up backend-specific state

    void finishRenderingAndAbandonContext();

    // This is so backends can stop the generic rendering state first before cleaning up

    // backend-specific state. SkiaGpuContexts are invalid after invocation.

    void finishRenderingAndAbandonContexts();



    // Functions that a given backend (GLES, Vulkan) must implement

    using Contexts = std::pair<unique_ptr<SkiaGpuContext>, unique_ptr<SkiaGpuContext>>;

libs/renderengine/skia/SkiaVkRenderEngine.cpp

+48 −6
Original line number Diff line number Diff line
@@ -69,12 +69,38 @@ bool RenderEngine::canSupport(GraphicsApi graphicsApi) { 
        case GraphicsApi::GL:

            return true;

        case GraphicsApi::VK: {

            // Static local variables are initialized once, on first invocation of the function.

            static const bool canSupportVulkan = []() {

                if (!sVulkanInterface.isInitialized()) {

                    sVulkanInterface.init(false /* no protected content */);

                    ALOGD("%s: initialized == %s.", __func__,

                          sVulkanInterface.isInitialized() ? "true" : "false");

                    if (!sVulkanInterface.isInitialized()) {

                        sVulkanInterface.teardown();

                        return false;

                    }

                }

            return sVulkanInterface.isInitialized();

                return true;

            }();

            return canSupportVulkan;

        }

    }

}



void RenderEngine::teardown(GraphicsApi graphicsApi) {

    switch (graphicsApi) {

        case GraphicsApi::GL:

            break;

        case GraphicsApi::VK: {

            if (sVulkanInterface.isInitialized()) {

                sVulkanInterface.teardown();

                ALOGD("Tearing down the unprotected VulkanInterface.");

            }

            if (sProtectedContentVulkanInterface.isInitialized()) {

                sProtectedContentVulkanInterface.teardown();

                ALOGD("Tearing down the protected VulkanInterface.");

            }

            break;

        }

    }

}
@@ -88,11 +114,27 @@ SkiaVkRenderEngine::SkiaVkRenderEngine(const RenderEngineCreationArgs& args) 
                         args.blurAlgorithm) {}



SkiaVkRenderEngine::~SkiaVkRenderEngine() {

    finishRenderingAndAbandonContext();

    finishRenderingAndAbandonContexts();

    // Teardown VulkanInterfaces after Skia contexts have been abandoned

    teardown(GraphicsApi::VK);

}



SkiaRenderEngine::Contexts SkiaVkRenderEngine::createContexts() {

    sSetupVulkanInterface();

    // More work would need to be done in order to have multiple RenderEngine instances. In

    // particular, they would not be able to share the same VulkanInterface(s).

    LOG_ALWAYS_FATAL_IF(!sVulkanInterface.takeOwnership(),

                        "SkiaVkRenderEngine couldn't take ownership of existing unprotected "

                        "VulkanInterface! Only one SkiaVkRenderEngine instance may exist at a "

                        "time.");

    if (sProtectedContentVulkanInterface.isInitialized()) {

        // takeOwnership fails on an uninitialized VulkanInterface, but protected content support is

        // optional.

        LOG_ALWAYS_FATAL_IF(!sProtectedContentVulkanInterface.takeOwnership(),

                            "SkiaVkRenderEngine couldn't take ownership of existing protected "

                            "VulkanInterface! Only one SkiaVkRenderEngine instance may exist at a "

                            "time.");

    }



    SkiaRenderEngine::Contexts contexts;

    contexts.first = createContext(sVulkanInterface);