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

Commit 1e06f2da authored by Nolan Scobie's avatar Nolan Scobie
Browse files

Move Ganesh flush calls from drawLayersInternal to flushAndSubmit 

While calling skgpu::ganesh::Flush won't cause any harm in Graphite, it
doesn't make sense to call it from the shared SkiaRenderEngine.
Additionally, making flushAndSubmit accept a destination SkSurface to
support this move will also be required for GraphiteVkRenderEngine.

GaneshVkRenderEngine could probably get away with just one flush call,
but this approach keeps functional parity (and keeps traces equivalent
FWIW).

Unfortunately, drawLayersInternal still needs to call
skgpu::ganesh::Flush when kGaneshFlushAfterEveryLayer is locally
enabled. This could be moved to some abstraction on either
SkiaGpuContext, or the child implementation of SkiaRenderEngine.
However, I'd like to minimize the number of abstracted functions that
*look* load bearing, but actually no-op in Graphite. Since I believe
this is only used for locally investigating shader priming stuff, I
would rather just leave it in a weird (but documented) state until it's
removed.

While moving the call to flushAndSubmit, I also changed it to the
equivalent non-static variant that is called directly on a context,
instead of finding the context from the surface. I feel this might help
differentiate the intent of "maybe flush" in drawLayersInternal vs.
"definitely flush on this context" in flushAndSubmit.

Test: manual validation (GL+VK) & existing tests (refactor)
Bug: b/293371537
Change-Id: Ia78cd8457aa47d6bb80ad5e81eb9d79b7eed5e79
parent 02c160c8

libs/renderengine/skia/GaneshVkRenderEngine.cpp

+10 −2
Original line number Diff line number Diff line
@@ -23,6 +23,7 @@ 


#include <log/log_main.h>

#include <sync/sync.h>

#include <utils/Trace.h>



namespace android::renderengine::skia {
@@ -51,11 +52,18 @@ void GaneshVkRenderEngine::waitFence(SkiaGpuContext* context, base::borrowed_fd 
    context->grDirectContext()->wait(1, &beSemaphore, kDeleteAfterWait);

}



base::unique_fd GaneshVkRenderEngine::flushAndSubmit(SkiaGpuContext* context) {

base::unique_fd GaneshVkRenderEngine::flushAndSubmit(SkiaGpuContext* context,

                                                     sk_sp<SkSurface> dstSurface) {

    sk_sp<GrDirectContext> grContext = context->grDirectContext();

    {

        ATRACE_NAME("flush surface");

        // TODO: Investigate feasibility of combining this "surface flush" into the "context flush"

        // below.

        context->grDirectContext()->flush(dstSurface.get());

    }



    VulkanInterface& vi = getVulkanInterface(isProtected());

    VkSemaphore semaphore = vi.createExportableSemaphore();



    GrBackendSemaphore backendSemaphore = GrBackendSemaphores::MakeVk(semaphore);



    GrFlushInfo flushInfo;

libs/renderengine/skia/GaneshVkRenderEngine.h

+1 −1
Original line number Diff line number Diff line
@@ -28,7 +28,7 @@ protected: 
    GaneshVkRenderEngine(const RenderEngineCreationArgs& args) : SkiaVkRenderEngine(args) {}



    void waitFence(SkiaGpuContext* context, base::borrowed_fd fenceFd) override;

    base::unique_fd flushAndSubmit(SkiaGpuContext* context) override;

    base::unique_fd flushAndSubmit(SkiaGpuContext* context, sk_sp<SkSurface> dstSurface) override;

};



} // namespace android::renderengine::skia

libs/renderengine/skia/SkiaGLRenderEngine.cpp

+10 −5
Original line number Diff line number Diff line
@@ -337,8 +337,14 @@ void SkiaGLRenderEngine::waitFence(SkiaGpuContext*, base::borrowed_fd fenceFd) { 
    }

}



base::unique_fd SkiaGLRenderEngine::flushAndSubmit(SkiaGpuContext* context) {

    base::unique_fd drawFence = flush();

base::unique_fd SkiaGLRenderEngine::flushAndSubmit(SkiaGpuContext* context,

                                                   sk_sp<SkSurface> dstSurface) {

    sk_sp<GrDirectContext> grContext = context->grDirectContext();

    {

        ATRACE_NAME("flush surface");

        grContext->flush(dstSurface.get());

    }

    base::unique_fd drawFence = flushGL();



    bool requireSync = drawFence.get() < 0;

    if (requireSync) {
@@ -346,8 +352,7 @@ base::unique_fd SkiaGLRenderEngine::flushAndSubmit(SkiaGpuContext* context) { 
    } else {

        ATRACE_BEGIN("Submit(sync=false)");

    }

    bool success =

            context->grDirectContext()->submit(requireSync ? GrSyncCpu::kYes : GrSyncCpu::kNo);

    bool success = grContext->submit(requireSync ? GrSyncCpu::kYes : GrSyncCpu::kNo);

    ATRACE_END();

    if (!success) {

        ALOGE("Failed to flush RenderEngine commands");
@@ -394,7 +399,7 @@ bool SkiaGLRenderEngine::waitGpuFence(base::borrowed_fd fenceFd) { 
    return true;

}



base::unique_fd SkiaGLRenderEngine::flush() {

base::unique_fd SkiaGLRenderEngine::flushGL() {

    ATRACE_CALL();

    if (!GLExtensions::getInstance().hasNativeFenceSync()) {

        return base::unique_fd();

libs/renderengine/skia/SkiaGLRenderEngine.h

+2 −2
Original line number Diff line number Diff line
@@ -63,7 +63,7 @@ protected: 
    bool supportsProtectedContentImpl() const override;

    bool useProtectedContextImpl(GrProtected isProtected) override;

    void waitFence(SkiaGpuContext* context, base::borrowed_fd fenceFd) override;

    base::unique_fd flushAndSubmit(SkiaGpuContext* context) override;

    base::unique_fd flushAndSubmit(SkiaGpuContext* context, sk_sp<SkSurface> dstSurface) override;

    void appendBackendSpecificInfoToDump(std::string& result) override;



private:
@@ -71,7 +71,7 @@ private: 
                       EGLSurface placeholder, EGLContext protectedContext,

                       EGLSurface protectedPlaceholder);

    bool waitGpuFence(base::borrowed_fd fenceFd);

    base::unique_fd flush();

    base::unique_fd flushGL();

    static EGLConfig chooseEglConfig(EGLDisplay display, int format, bool logConfig);

    static EGLContext createEglContext(EGLDisplay display, EGLConfig config,

                                       EGLContext shareContext,

libs/renderengine/skia/SkiaRenderEngine.cpp

+8 −8
Original line number Diff line number Diff line
@@ -90,7 +90,7 @@ namespace { 


// Debugging settings

static const bool kPrintLayerSettings = false;

static const bool kFlushAfterEveryLayer = kPrintLayerSettings;

static const bool kGaneshFlushAfterEveryLayer = kPrintLayerSettings;

static constexpr bool kEnableLayerBrightening = true;



} // namespace
@@ -1122,8 +1122,12 @@ void SkiaRenderEngine::drawLayersInternal( 
        } else {

            canvas->drawRect(bounds.rect(), paint);

        }

        if (kFlushAfterEveryLayer) {

        if (kGaneshFlushAfterEveryLayer) {

            ATRACE_NAME("flush surface");

            // No-op in Graphite. If "flushing" Skia's drawing commands after each layer is desired

            // in Graphite, then a graphite::Recording would need to be snapped and tracked for each

            // layer, which is likely possible but adds non-trivial complexity (in both bookkeeping

            // and refactoring).

            skgpu::ganesh::Flush(activeSurface);

        }

    }
@@ -1149,13 +1153,9 @@ void SkiaRenderEngine::drawLayersInternal( 


    surfaceAutoSaveRestore.restore();

    mCapture->endCapture();

    {

        ATRACE_NAME("flush surface");

        LOG_ALWAYS_FATAL_IF(activeSurface != dstSurface);

        skgpu::ganesh::Flush(activeSurface);

    }



    auto drawFence = sp<Fence>::make(flushAndSubmit(context));

    LOG_ALWAYS_FATAL_IF(activeSurface != dstSurface);

    auto drawFence = sp<Fence>::make(flushAndSubmit(context, dstSurface));



    if (ATRACE_ENABLED()) {

        static gui::FenceMonitor sMonitor("RE Completion");