Correctly discard or load RT when doing copies as draws in Vulkan
This fixes all the copy as draw issues we've had with certain devices and
the cap is no longer needed.
Bug: skia:
Change-Id: Id0b750849c4c920beae2d8cb3eda5f402018f194
Reviewed-On: https://skia-review.googlesource.com/114860
Commit-Queue: Greg Daniel <egdaniel@google.com>
Reviewed-By: Brian Salomon <bsalomon@google.com>
Reviewed-on: https://skia-review.googlesource.com/115081
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 0e8b60f..368d83f 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -842,7 +842,12 @@
return nullptr;
}
- return this->makeWrappedSurfaceContext(std::move(proxy));
+ sk_sp<GrSurfaceContext> sContext = this->makeWrappedSurfaceContext(std::move(proxy));
+ if (sContext && sContext->asRenderTargetContext()) {
+ sContext->asRenderTargetContext()->discard();
+ }
+
+ return sContext;
}
sk_sp<GrTextureContext> GrContextPriv::makeBackendTextureContext(const GrBackendTexture& tex,
diff --git a/src/gpu/GrGpu.cpp b/src/gpu/GrGpu.cpp
index bea6a60..f2bbda1 100644
--- a/src/gpu/GrGpu.cpp
+++ b/src/gpu/GrGpu.cpp
@@ -241,7 +241,8 @@
bool GrGpu::copySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect, const SkIPoint& dstPoint) {
+ const SkIRect& srcRect, const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect) {
GR_CREATE_TRACE_MARKER_CONTEXT("GrGpu", "copySurface", fContext);
SkASSERT(dst && src);
this->handleDirtyContext();
@@ -249,7 +250,8 @@
if (GrPixelConfigIsSint(dst->config()) != GrPixelConfigIsSint(src->config())) {
return false;
}
- return this->onCopySurface(dst, dstOrigin, src, srcOrigin, srcRect, dstPoint);
+ return this->onCopySurface(dst, dstOrigin, src, srcOrigin, srcRect, dstPoint,
+ canDiscardOutsideDstRect);
}
bool GrGpu::getReadPixelsInfo(GrSurface* srcSurface, GrSurfaceOrigin srcOrigin,
diff --git a/src/gpu/GrGpu.h b/src/gpu/GrGpu.h
index 2f12073..30e565f 100644
--- a/src/gpu/GrGpu.h
+++ b/src/gpu/GrGpu.h
@@ -322,11 +322,13 @@
// Called to perform a surface to surface copy. Fallbacks to issuing a draw from the src to dst
// take place at the GrOpList level and this function implement faster copy paths. The rect
// and point are pre-clipped. The src rect and implied dst rect are guaranteed to be within the
- // src/dst bounds and non-empty.
+ // src/dst bounds and non-empty. If canDiscardOutsideDstRect is set to true then we don't need
+ // to preserve any data on the dst surface outside of the copy.
bool copySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
const SkIRect& srcRect,
- const SkIPoint& dstPoint);
+ const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect = false);
struct MultisampleSpecs {
MultisampleSpecs(uint8_t uniqueID, int effectiveSampleCnt, const SkPoint* locations)
@@ -597,7 +599,8 @@
// overridden by backend specific derived class to perform the copy surface
virtual bool onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect, const SkIPoint& dstPoint) = 0;
+ const SkIRect& srcRect, const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect) = 0;
// overridden by backend specific derived class to perform the multisample queries
virtual void onQueryMultisampleSpecs(GrRenderTarget*, GrSurfaceOrigin rtOrigin,
diff --git a/src/gpu/ddl/GrDDLGpu.h b/src/gpu/ddl/GrDDLGpu.h
index e1f463f..0508a2e 100644
--- a/src/gpu/ddl/GrDDLGpu.h
+++ b/src/gpu/ddl/GrDDLGpu.h
@@ -38,7 +38,8 @@
bool onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect, const SkIPoint& dstPoint) override {
+ const SkIRect& srcRect, const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect) override {
SkASSERT(0);
return true;
}
diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index e117488..fb0622b 100644
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -3476,7 +3476,8 @@
bool GrGLGpu::onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect, const SkIPoint& dstPoint) {
+ const SkIRect& srcRect, const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect) {
// None of our copy methods can handle a swizzle. TODO: Make copySurfaceAsDraw handle the
// swizzle.
if (this->caps()->shaderCaps()->configOutputSwizzle(src->config()) !=
diff --git a/src/gpu/gl/GrGLGpu.h b/src/gpu/gl/GrGLGpu.h
index 9ac86ba..0d55e50 100644
--- a/src/gpu/gl/GrGLGpu.h
+++ b/src/gpu/gl/GrGLGpu.h
@@ -265,7 +265,8 @@
bool onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect, const SkIPoint& dstPoint) override;
+ const SkIRect& srcRect, const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect) override;
void onQueryMultisampleSpecs(GrRenderTarget*, GrSurfaceOrigin, const GrStencilSettings&,
int* effectiveSampleCnt, SamplePattern*) override;
diff --git a/src/gpu/mock/GrMockGpu.h b/src/gpu/mock/GrMockGpu.h
index 5185ce2..b99857f 100644
--- a/src/gpu/mock/GrMockGpu.h
+++ b/src/gpu/mock/GrMockGpu.h
@@ -35,10 +35,6 @@
GrPixelConfig srcConfig, DrawPreference*,
WritePixelTempDrawInfo*) override { return true; }
- bool onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
- GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect, const SkIPoint& dstPoint) override { return true; }
-
void onQueryMultisampleSpecs(GrRenderTarget* rt, GrSurfaceOrigin, const GrStencilSettings&,
int* effectiveSampleCnt, SamplePattern*) override {
*effectiveSampleCnt = rt->numStencilSamples();
@@ -123,6 +119,12 @@
void onResolveRenderTarget(GrRenderTarget* target, GrSurfaceOrigin) override { return; }
+ bool onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin, GrSurface* src,
+ GrSurfaceOrigin srcOrigin, const SkIRect& srcRect,
+ const SkIPoint& dstPoint, bool canDiscardOutsideDstRect) override {
+ return true;
+ }
+
void onFinishFlush(bool insertedSemaphores) override {}
GrStencilAttachment* createStencilAttachmentForRenderTarget(const GrRenderTarget*,
diff --git a/src/gpu/mtl/GrMtlGpu.h b/src/gpu/mtl/GrMtlGpu.h
index 5a5289b..9199f03 100644
--- a/src/gpu/mtl/GrMtlGpu.h
+++ b/src/gpu/mtl/GrMtlGpu.h
@@ -44,7 +44,8 @@
bool onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
const SkIRect& srcRect,
- const SkIPoint& dstPoint) override { return false; }
+ const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect) override { return false; }
void onQueryMultisampleSpecs(GrRenderTarget*, GrSurfaceOrigin, const GrStencilSettings&,
int* effectiveSampleCnt, SamplePattern*) override {}
diff --git a/src/gpu/vk/GrVkCaps.cpp b/src/gpu/vk/GrVkCaps.cpp
index e56d0e8..f25c8d7 100644
--- a/src/gpu/vk/GrVkCaps.cpp
+++ b/src/gpu/vk/GrVkCaps.cpp
@@ -19,7 +19,6 @@
: INHERITED(contextOptions) {
fCanUseGLSLForShaderModule = false;
fMustDoCopiesFromOrigin = false;
- fSupportsCopiesAsDraws = true;
fMustSubmitCommandsBeforeCopyOp = false;
fMustSleepOnTearDown = false;
fNewCBOnPipelineChange = false;
@@ -64,7 +63,7 @@
// render target as well.
desc->fOrigin = src->origin();
desc->fConfig = src->config();
- if (src->numColorSamples() > 1 || (src->asTextureProxy() && this->supportsCopiesAsDraws())) {
+ if (src->numColorSamples() > 1 || src->asTextureProxy()) {
desc->fFlags = kRenderTarget_GrSurfaceFlag;
} else {
// Just going to use CopyImage here
@@ -115,13 +114,6 @@
fMustSubmitCommandsBeforeCopyOp = true;
}
- if (kQualcomm_VkVendor == properties.vendorID ||
- kARM_VkVendor == properties.vendorID) {
- fSupportsCopiesAsDraws = false;
- // We require copies as draws to support cross context textures.
- fCrossContextTextureSupport = false;
- }
-
#if defined(SK_BUILD_FOR_WIN)
if (kNvidia_VkVendor == properties.vendorID) {
fMustSleepOnTearDown = true;
diff --git a/src/gpu/vk/GrVkCaps.h b/src/gpu/vk/GrVkCaps.h
index 90f488e..75ef935 100644
--- a/src/gpu/vk/GrVkCaps.h
+++ b/src/gpu/vk/GrVkCaps.h
@@ -73,11 +73,6 @@
return fMustDoCopiesFromOrigin;
}
- // Check whether we support using draws for copies.
- bool supportsCopiesAsDraws() const {
- return fSupportsCopiesAsDraws;
- }
-
// On Nvidia there is a current bug where we must the current command buffer before copy
// operations or else the copy will not happen. This includes copies, blits, resolves, and copy
// as draws.
@@ -172,8 +167,6 @@
bool fMustDoCopiesFromOrigin;
- bool fSupportsCopiesAsDraws;
-
bool fMustSubmitCommandsBeforeCopyOp;
bool fMustSleepOnTearDown;
diff --git a/src/gpu/vk/GrVkCopyManager.cpp b/src/gpu/vk/GrVkCopyManager.cpp
index ba62980..e3e0e09 100644
--- a/src/gpu/vk/GrVkCopyManager.cpp
+++ b/src/gpu/vk/GrVkCopyManager.cpp
@@ -143,7 +143,8 @@
bool GrVkCopyManager::copySurfaceAsDraw(GrVkGpu* gpu,
GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect, const SkIPoint& dstPoint) {
+ const SkIRect& srcRect, const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect) {
// None of our copy methods can handle a swizzle. TODO: Make copySurfaceAsDraw handle the
// swizzle.
if (gpu->caps()->shaderCaps()->configOutputSwizzle(src->config()) !=
@@ -151,10 +152,6 @@
return false;
}
- if (!gpu->vkCaps().supportsCopiesAsDraws()) {
- return false;
- }
-
if (gpu->vkCaps().newCBOnPipelineChange()) {
// We bind a new pipeline here for the copy so we must start a new command buffer.
gpu->finishFlush(0, nullptr);
@@ -324,13 +321,13 @@
false);
}
- GrVkRenderPass::LoadStoreOps vkColorOps(VK_ATTACHMENT_LOAD_OP_DONT_CARE,
- VK_ATTACHMENT_STORE_OP_STORE);
+ VkAttachmentLoadOp loadOp = canDiscardOutsideDstRect ? VK_ATTACHMENT_LOAD_OP_DONT_CARE
+ : VK_ATTACHMENT_LOAD_OP_LOAD;
+ GrVkRenderPass::LoadStoreOps vkColorOps(loadOp, VK_ATTACHMENT_STORE_OP_STORE);
GrVkRenderPass::LoadStoreOps vkStencilOps(VK_ATTACHMENT_LOAD_OP_LOAD,
VK_ATTACHMENT_STORE_OP_STORE);
const GrVkRenderPass* renderPass;
- const GrVkResourceProvider::CompatibleRPHandle& rpHandle =
- rt->compatibleRenderPassHandle();
+ const GrVkResourceProvider::CompatibleRPHandle& rpHandle = rt->compatibleRenderPassHandle();
if (rpHandle.isValid()) {
renderPass = gpu->resourceProvider().findRenderPass(rpHandle,
vkColorOps,
diff --git a/src/gpu/vk/GrVkCopyManager.h b/src/gpu/vk/GrVkCopyManager.h
index 726faf3..f36cc30 100644
--- a/src/gpu/vk/GrVkCopyManager.h
+++ b/src/gpu/vk/GrVkCopyManager.h
@@ -30,7 +30,8 @@
bool copySurfaceAsDraw(GrVkGpu* gpu,
GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect, const SkIPoint& dstPoint);
+ const SkIRect& srcRect, const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect);
void destroyResources(GrVkGpu* gpu);
void abandonResources();
diff --git a/src/gpu/vk/GrVkGpu.cpp b/src/gpu/vk/GrVkGpu.cpp
index 9428e13..1269eaa 100644
--- a/src/gpu/vk/GrVkGpu.cpp
+++ b/src/gpu/vk/GrVkGpu.cpp
@@ -1861,8 +1861,8 @@
bool GrVkGpu::onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect,
- const SkIPoint& dstPoint) {
+ const SkIRect& srcRect, const SkIPoint& dstPoint,
+ bool canDiscardOutsideDstRect) {
if (can_copy_as_resolve(dst, dstOrigin, src, srcOrigin, this)) {
this->copySurfaceAsResolve(dst, dstOrigin, src, srcOrigin, srcRect, dstPoint);
return true;
@@ -1872,7 +1872,10 @@
this->submitCommandBuffer(GrVkGpu::kSkip_SyncQueue);
}
- if (fCopyManager.copySurfaceAsDraw(this, dst, dstOrigin, src, srcOrigin, srcRect, dstPoint)) {
+ if (fCopyManager.copySurfaceAsDraw(this, dst, dstOrigin, src, srcOrigin, srcRect, dstPoint,
+ canDiscardOutsideDstRect)) {
+ auto dstRect = srcRect.makeOffset(dstPoint.fX, dstPoint.fY);
+ this->didWriteToSurface(dst, &dstRect);
return true;
}
diff --git a/src/gpu/vk/GrVkGpu.h b/src/gpu/vk/GrVkGpu.h
index 114d97c..85fb213 100644
--- a/src/gpu/vk/GrVkGpu.h
+++ b/src/gpu/vk/GrVkGpu.h
@@ -77,10 +77,6 @@
GrPixelConfig srcConfig, DrawPreference*,
WritePixelTempDrawInfo*) override;
- bool onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin,
- GrSurface* src, GrSurfaceOrigin srcOrigin,
- const SkIRect& srcRect, const SkIPoint& dstPoint) override;
-
void onQueryMultisampleSpecs(GrRenderTarget*, GrSurfaceOrigin, const GrStencilSettings&,
int* effectiveSampleCnt, SamplePattern*) override;
@@ -211,6 +207,10 @@
GrPixelConfig config, GrBuffer* transferBuffer,
size_t offset, size_t rowBytes) override;
+ bool onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin, GrSurface* src,
+ GrSurfaceOrigin srcOrigin, const SkIRect& srcRect,
+ const SkIPoint& dstPoint, bool canDiscardOutsideDstRect) override;
+
void onFinishFlush(bool insertedSemaphores) override;
// Ends and submits the current command buffer to the queue and then creates a new command
diff --git a/src/gpu/vk/GrVkGpuCommandBuffer.cpp b/src/gpu/vk/GrVkGpuCommandBuffer.cpp
index dcc5e65..7bf07bc 100644
--- a/src/gpu/vk/GrVkGpuCommandBuffer.cpp
+++ b/src/gpu/vk/GrVkGpuCommandBuffer.cpp
@@ -174,7 +174,7 @@
for (int j = 0; j < cbInfo.fPreCopies.count(); ++j) {
CopyInfo& copyInfo = cbInfo.fPreCopies[j];
fGpu->copySurface(fRenderTarget, fOrigin, copyInfo.fSrc, copyInfo.fSrcOrigin,
- copyInfo.fSrcRect, copyInfo.fDstPoint);
+ copyInfo.fSrcRect, copyInfo.fDstPoint, copyInfo.fShouldDiscardDst);
}
// Make sure we do the following layout changes after all copies, uploads, or any other pre
@@ -476,6 +476,10 @@
this->addAdditionalRenderPass();
}
+ fCommandBufferInfos[fCurrentCmdInfo].fPreCopies.emplace_back(
+ src, srcOrigin, srcRect, dstPoint,
+ LoadStoreState::kStartsWithDiscard == cbInfo.fLoadStoreState);
+
if (LoadStoreState::kLoadAndStore != cbInfo.fLoadStoreState) {
// Change the render pass to do a load and store so we don't lose the results of our copy
GrVkRenderPass::LoadStoreOps vkColorOps(VK_ATTACHMENT_LOAD_OP_LOAD,
@@ -503,7 +507,6 @@
cbInfo.fLoadStoreState = LoadStoreState::kLoadAndStore;
}
- fCommandBufferInfos[fCurrentCmdInfo].fPreCopies.emplace_back(src, srcOrigin, srcRect, dstPoint);
}
////////////////////////////////////////////////////////////////////////////////
diff --git a/src/gpu/vk/GrVkGpuCommandBuffer.h b/src/gpu/vk/GrVkGpuCommandBuffer.h
index 2e9ac12..f2d48c7 100644
--- a/src/gpu/vk/GrVkGpuCommandBuffer.h
+++ b/src/gpu/vk/GrVkGpuCommandBuffer.h
@@ -143,13 +143,18 @@
struct CopyInfo {
CopyInfo(GrSurface* src, GrSurfaceOrigin srcOrigin, const SkIRect& srcRect,
- const SkIPoint& dstPoint)
- : fSrc(src), fSrcOrigin(srcOrigin), fSrcRect(srcRect), fDstPoint(dstPoint) {}
+ const SkIPoint& dstPoint, bool shouldDiscardDst)
+ : fSrc(src)
+ , fSrcOrigin(srcOrigin)
+ , fSrcRect(srcRect)
+ , fDstPoint(dstPoint)
+ , fShouldDiscardDst(shouldDiscardDst) {}
GrSurface* fSrc;
GrSurfaceOrigin fSrcOrigin;
SkIRect fSrcRect;
SkIPoint fDstPoint;
+ bool fShouldDiscardDst;
};
enum class LoadStoreState {
diff --git a/tests/VkMakeCopyPipelineTest.cpp b/tests/VkMakeCopyPipelineTest.cpp
index 2c82c03..81f3cdf 100644
--- a/tests/VkMakeCopyPipelineTest.cpp
+++ b/tests/VkMakeCopyPipelineTest.cpp
@@ -179,10 +179,6 @@
GrContext* context = ctxInfo.grContext();
GrVkGpu* gpu = static_cast<GrVkGpu*>(context->contextPriv().getGpu());
- if (!gpu->vkCaps().supportsCopiesAsDraws()) {
- return;
- }
-
TestVkCopyProgram copyProgram;
copyProgram.test(gpu, reporter);
}
