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>
diff --git a/src/gpu/vk/GrVkCaps.cpp b/src/gpu/vk/GrVkCaps.cpp
index 7e22135..0d75cbc 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;
@@ -65,7 +64,7 @@
// render target as well.
*origin = 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
@@ -116,13 +115,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 d8d1d2e..63bd4e9 100644
--- a/src/gpu/vk/GrVkCaps.h
+++ b/src/gpu/vk/GrVkCaps.h
@@ -74,11 +74,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.
@@ -176,8 +171,6 @@
bool fMustDoCopiesFromOrigin;
- bool fSupportsCopiesAsDraws;
-
bool fMustSubmitCommandsBeforeCopyOp;
bool fMustSleepOnTearDown;
diff --git a/src/gpu/vk/GrVkCopyManager.cpp b/src/gpu/vk/GrVkCopyManager.cpp
index f22d99b..8c54fc5 100644
--- a/src/gpu/vk/GrVkCopyManager.cpp
+++ b/src/gpu/vk/GrVkCopyManager.cpp
@@ -142,7 +142,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()) !=
@@ -150,10 +151,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);
@@ -312,13 +309,13 @@
VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT,
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 4bd3179..b59d85e 100644
--- a/src/gpu/vk/GrVkGpu.cpp
+++ b/src/gpu/vk/GrVkGpu.cpp
@@ -1854,8 +1854,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;
@@ -1865,7 +1865,8 @@
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, dstOrigin, &dstRect);
return true;
diff --git a/src/gpu/vk/GrVkGpu.h b/src/gpu/vk/GrVkGpu.h
index 6eac5c8..20df339 100644
--- a/src/gpu/vk/GrVkGpu.h
+++ b/src/gpu/vk/GrVkGpu.h
@@ -204,7 +204,7 @@
bool onCopySurface(GrSurface* dst, GrSurfaceOrigin dstOrigin, GrSurface* src,
GrSurfaceOrigin srcOrigin, const SkIRect& srcRect,
- const SkIPoint& dstPoint) override;
+ const SkIPoint& dstPoint, bool canDiscardOutsideDstRect) override;
void onFinishFlush(bool insertedSemaphores) override;
diff --git a/src/gpu/vk/GrVkGpuCommandBuffer.cpp b/src/gpu/vk/GrVkGpuCommandBuffer.cpp
index 4fb6e2e..c05cb61 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 {