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
diff --git a/libs/renderengine/skia/compat/GaneshGpuContext.cpp b/libs/renderengine/skia/compat/GaneshGpuContext.cpp
index e3fec19..b2eae00 100644
--- a/libs/renderengine/skia/compat/GaneshGpuContext.cpp
+++ b/libs/renderengine/skia/compat/GaneshGpuContext.cpp
@@ -38,7 +38,6 @@
namespace android::renderengine::skia {
namespace {
-// TODO: b/293371537 - Graphite variant.
static GrContextOptions ganeshOptions(GrContextOptions::PersistentCache& skSLCacheMonitor) {
GrContextOptions options;
options.fDisableDriverCorrectnessWorkarounds = true;
@@ -67,6 +66,11 @@
LOG_ALWAYS_FATAL_IF(mGrContext.get() == nullptr, "GrDirectContext creation failed");
}
+GaneshGpuContext::~GaneshGpuContext() {
+ mGrContext->flushAndSubmit(GrSyncCpu::kYes);
+ mGrContext->abandonContext();
+};
+
sk_sp<GrDirectContext> GaneshGpuContext::grDirectContext() {
return mGrContext;
}
@@ -101,11 +105,6 @@
mGrContext->setResourceCacheLimit(maxResourceBytes);
}
-void GaneshGpuContext::finishRenderingAndAbandonContext() {
- mGrContext->flushAndSubmit(GrSyncCpu::kYes);
- mGrContext->abandonContext();
-};
-
void GaneshGpuContext::purgeUnlockedScratchResources() {
mGrContext->purgeUnlockedResources(GrPurgeResourceOptions::kScratchResourcesOnly);
}
diff --git a/libs/renderengine/skia/compat/GaneshGpuContext.h b/libs/renderengine/skia/compat/GaneshGpuContext.h
index d815d15..aeb1a82 100644
--- a/libs/renderengine/skia/compat/GaneshGpuContext.h
+++ b/libs/renderengine/skia/compat/GaneshGpuContext.h
@@ -25,7 +25,7 @@
class GaneshGpuContext : public SkiaGpuContext {
public:
GaneshGpuContext(sk_sp<GrDirectContext> grContext);
- ~GaneshGpuContext() override = default;
+ ~GaneshGpuContext() override;
sk_sp<GrDirectContext> grDirectContext() override;
@@ -39,7 +39,6 @@
bool isAbandonedOrDeviceLost() override;
void setResourceCacheLimit(size_t maxResourceBytes) override;
- void finishRenderingAndAbandonContext() override;
void purgeUnlockedScratchResources() override;
void resetContextIfApplicable() override;
diff --git a/libs/renderengine/skia/compat/GraphiteGpuContext.cpp b/libs/renderengine/skia/compat/GraphiteGpuContext.cpp
index e19d66f..69f5832 100644
--- a/libs/renderengine/skia/compat/GraphiteGpuContext.cpp
+++ b/libs/renderengine/skia/compat/GraphiteGpuContext.cpp
@@ -62,6 +62,22 @@
LOG_ALWAYS_FATAL_IF(mRecorder.get() == nullptr, "graphite::Recorder creation failed");
}
+GraphiteGpuContext::~GraphiteGpuContext() {
+ // The equivalent operation would occur when destroying the graphite::Context, but calling this
+ // explicitly allows any outstanding GraphiteBackendTextures to be released, thus allowing us to
+ // assert that this GraphiteGpuContext holds the last ref to the underlying graphite::Recorder.
+ mContext->submit(skgpu::graphite::SyncToCpu::kYes);
+ // We must call the Context's and Recorder's dtors before exiting this function, so all other
+ // refs must be released by now. Note: these assertions may be unreliable in a hypothetical
+ // future world where we take advantage of Graphite's multi-threading capabilities!
+ LOG_ALWAYS_FATAL_IF(mRecorder.use_count() > 1,
+ "Something other than GraphiteGpuContext holds a ref to the underlying "
+ "graphite::Recorder");
+ LOG_ALWAYS_FATAL_IF(mContext.use_count() > 1,
+ "Something other than GraphiteGpuContext holds a ref to the underlying "
+ "graphite::Context");
+};
+
std::shared_ptr<skgpu::graphite::Context> GraphiteGpuContext::graphiteContext() {
return mContext;
}
@@ -94,11 +110,6 @@
return mContext->isDeviceLost();
}
-void GraphiteGpuContext::finishRenderingAndAbandonContext() {
- // TODO: b/293371537 - Validate that nothing else needs to be explicitly abandoned.
- mContext->submit(skgpu::graphite::SyncToCpu::kYes);
-};
-
void GraphiteGpuContext::dumpMemoryStatistics(SkTraceMemoryDump* traceMemoryDump) const {
mContext->dumpMemoryStatistics(traceMemoryDump);
}
diff --git a/libs/renderengine/skia/compat/GraphiteGpuContext.h b/libs/renderengine/skia/compat/GraphiteGpuContext.h
index 685f899..413817f 100644
--- a/libs/renderengine/skia/compat/GraphiteGpuContext.h
+++ b/libs/renderengine/skia/compat/GraphiteGpuContext.h
@@ -26,7 +26,7 @@
class GraphiteGpuContext : public SkiaGpuContext {
public:
GraphiteGpuContext(std::unique_ptr<skgpu::graphite::Context> context);
- ~GraphiteGpuContext() override = default;
+ ~GraphiteGpuContext() override;
std::shared_ptr<skgpu::graphite::Context> graphiteContext() override;
std::shared_ptr<skgpu::graphite::Recorder> graphiteRecorder() override;
@@ -45,7 +45,6 @@
// functions yet, as its design may evolve.)
void setResourceCacheLimit(size_t maxResourceBytes) override{};
- void finishRenderingAndAbandonContext() override;
// TODO: b/293371537 - Triple-check and validate that no cleanup is necessary when switching
// contexts.
// No-op (unnecessary during context switch for Graphite's client-budgeted memory model).
@@ -58,7 +57,7 @@
private:
DISALLOW_COPY_AND_ASSIGN(GraphiteGpuContext);
- const std::shared_ptr<skgpu::graphite::Context> mContext;
+ std::shared_ptr<skgpu::graphite::Context> mContext;
std::shared_ptr<skgpu::graphite::Recorder> mRecorder;
};
diff --git a/libs/renderengine/skia/compat/SkiaGpuContext.h b/libs/renderengine/skia/compat/SkiaGpuContext.h
index a2457e5..282dfe7 100644
--- a/libs/renderengine/skia/compat/SkiaGpuContext.h
+++ b/libs/renderengine/skia/compat/SkiaGpuContext.h
@@ -36,18 +36,32 @@
/**
* Abstraction over Ganesh and Graphite's underlying context-like objects.
+ *
+ * On destruction, subclasses will submit any pending work before destroying their internal Skia
+ * context(s). Any unused cached SkiaBackendTextures created from a SkiaGpuContext that are awaiting
+ * cleanup must be deleted before destroying that SkiaGpuContext, and any textures that are released
+ * during ~SkiaGpuContext must be configured to be deleted immediately.
*/
class SkiaGpuContext {
public:
+ /**
+ * glInterface must remain valid until after SkiaGpuContext is destroyed.
+ */
static std::unique_ptr<SkiaGpuContext> MakeGL_Ganesh(
sk_sp<const GrGLInterface> glInterface,
GrContextOptions::PersistentCache& skSLCacheMonitor);
+ /**
+ * grVkBackendContext must remain valid until after SkiaGpuContext is destroyed.
+ */
static std::unique_ptr<SkiaGpuContext> MakeVulkan_Ganesh(
const GrVkBackendContext& grVkBackendContext,
GrContextOptions::PersistentCache& skSLCacheMonitor);
// TODO: b/293371537 - Need shader / pipeline monitoring support in Graphite.
+ /**
+ * vulkanBackendContext must remain valid until after SkiaGpuContext is destroyed.
+ */
static std::unique_ptr<SkiaGpuContext> MakeVulkan_Graphite(
const skgpu::VulkanBackendContext& vulkanBackendContext);
@@ -91,7 +105,6 @@
virtual size_t getMaxTextureSize() const = 0;
virtual void setResourceCacheLimit(size_t maxResourceBytes) = 0;
- virtual void finishRenderingAndAbandonContext() = 0;
virtual void purgeUnlockedScratchResources() = 0;
virtual void resetContextIfApplicable() = 0; // No-op outside of GL (&& Ganesh at this point.)