From ce3bfb1ed155880585b2d0bb0a8d3e43306e23f2 Mon Sep 17 00:00:00 2001 From: egdaniel Date: Fri, 26 Aug 2016 11:05:13 -0700 Subject: [PATCH] Don't add the resolve attachment to vulkan render passes. Also includes some other msaa bug fixes BUG=skia:5127 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2256843002 Review-Url: https://codereview.chromium.org/2256843002 --- src/gpu/vk/GrVkFramebuffer.cpp | 4 --- src/gpu/vk/GrVkFramebuffer.h | 1 - src/gpu/vk/GrVkGpu.cpp | 4 +-- src/gpu/vk/GrVkGpuCommandBuffer.cpp | 23 +++++-------- src/gpu/vk/GrVkRenderPass.cpp | 61 ++++------------------------------ src/gpu/vk/GrVkRenderPass.h | 9 +---- src/gpu/vk/GrVkRenderTarget.cpp | 15 ++------- src/gpu/vk/GrVkResourceProvider.cpp | 6 ++-- src/gpu/vk/GrVkResourceProvider.h | 1 - src/gpu/vk/GrVkTextureRenderTarget.cpp | 1 + 10 files changed, 25 insertions(+), 100 deletions(-) diff --git a/src/gpu/vk/GrVkFramebuffer.cpp b/src/gpu/vk/GrVkFramebuffer.cpp index 49c6e41..f9add63 100644 --- a/src/gpu/vk/GrVkFramebuffer.cpp +++ b/src/gpu/vk/GrVkFramebuffer.cpp @@ -15,7 +15,6 @@ GrVkFramebuffer* GrVkFramebuffer::Create(GrVkGpu* gpu, int width, int height, const GrVkRenderPass* renderPass, const GrVkImageView* colorAttachment, - const GrVkImageView* resolveAttachment, const GrVkImageView* stencilAttachment) { // At the very least we need a renderPass and a colorAttachment SkASSERT(renderPass); @@ -24,9 +23,6 @@ GrVkFramebuffer* GrVkFramebuffer::Create(GrVkGpu* gpu, VkImageView attachments[3]; attachments[0] = colorAttachment->imageView(); int numAttachments = 1; - if (resolveAttachment) { - attachments[numAttachments++] = resolveAttachment->imageView(); - } if (stencilAttachment) { attachments[numAttachments++] = stencilAttachment->imageView(); } diff --git a/src/gpu/vk/GrVkFramebuffer.h b/src/gpu/vk/GrVkFramebuffer.h index e16ac22..b0f4beb 100644 --- a/src/gpu/vk/GrVkFramebuffer.h +++ b/src/gpu/vk/GrVkFramebuffer.h @@ -24,7 +24,6 @@ public: int width, int height, const GrVkRenderPass* renderPass, const GrVkImageView* colorAttachment, - const GrVkImageView* resolveAttachment, const GrVkImageView* stencilAttachment); VkFramebuffer framebuffer() const { return fFramebuffer; } diff --git a/src/gpu/vk/GrVkGpu.cpp b/src/gpu/vk/GrVkGpu.cpp index 0a713f4..495b85e 100644 --- a/src/gpu/vk/GrVkGpu.cpp +++ b/src/gpu/vk/GrVkGpu.cpp @@ -401,7 +401,7 @@ void GrVkGpu::onResolveRenderTarget(GrRenderTarget* target) { VK_PIPELINE_STAGE_TRANSFER_BIT, false); - fCurrentCmdBuffer->resolveImage(this, *rt, *rt->msaaImage(), 1, &resolveInfo); + fCurrentCmdBuffer->resolveImage(this, *rt->msaaImage(), *rt, 1, &resolveInfo); rt->flagAsResolved(); } @@ -1800,6 +1800,6 @@ void GrVkGpu::submitSecondaryCommandBuffer(GrVkSecondaryCommandBuffer* buffer, fCurrentCmdBuffer->executeCommands(this, buffer); fCurrentCmdBuffer->endRenderPass(this); - this->didWriteToSurface(target, pBounds); + this->didWriteToSurface(target, &bounds); } diff --git a/src/gpu/vk/GrVkGpuCommandBuffer.cpp b/src/gpu/vk/GrVkGpuCommandBuffer.cpp index 77a038a..f49b180 100644 --- a/src/gpu/vk/GrVkGpuCommandBuffer.cpp +++ b/src/gpu/vk/GrVkGpuCommandBuffer.cpp @@ -100,12 +100,15 @@ void GrVkGpuCommandBuffer::end() { } void GrVkGpuCommandBuffer::onSubmit(const SkIRect& bounds) { - // Change layout of our render target so it can be used as the color attachment - fRenderTarget->setImageLayout(fGpu, - VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL, - VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT, - VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, - false); + // Change layout of our render target so it can be used as the color attachment. Currently + // we don't attach the resolve to the framebuffer so no need to change its layout. + GrVkImage* targetImage = fRenderTarget->msaaImage() ? fRenderTarget->msaaImage() + : fRenderTarget; + targetImage->setImageLayout(fGpu, + VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL, + VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT, + VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, + false); // If we are using a stencil attachment we also need to update its layout if (GrStencilAttachment* stencil = fRenderTarget->renderTargetPriv().getStencilAttachment()) { @@ -118,14 +121,6 @@ void GrVkGpuCommandBuffer::onSubmit(const SkIRect& bounds) { false); } - if (GrVkImage* msaaImage = fRenderTarget->msaaImage()) { - msaaImage->setImageLayout(fGpu, - VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL, - VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT, - VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, - false); - } - for (int i = 0; i < fSampledImages.count(); ++i) { fSampledImages[i]->setImageLayout(fGpu, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL, diff --git a/src/gpu/vk/GrVkRenderPass.cpp b/src/gpu/vk/GrVkRenderPass.cpp index ba325ef..ee2d3d9 100644 --- a/src/gpu/vk/GrVkRenderPass.cpp +++ b/src/gpu/vk/GrVkRenderPass.cpp @@ -46,12 +46,11 @@ void GrVkRenderPass::initSimple(const GrVkGpu* gpu, const GrVkRenderTarget& targ static const GrVkRenderPass::LoadStoreOps kBasicLoadStoreOps(VK_ATTACHMENT_LOAD_OP_LOAD, VK_ATTACHMENT_STORE_OP_STORE); - this->init(gpu, target, kBasicLoadStoreOps, kBasicLoadStoreOps, kBasicLoadStoreOps); + this->init(gpu, target, kBasicLoadStoreOps, kBasicLoadStoreOps); } void GrVkRenderPass::init(const GrVkGpu* gpu, const LoadStoreOps& colorOp, - const LoadStoreOps& resolveOp, const LoadStoreOps& stencilOp) { uint32_t numAttachments = fAttachmentsDescriptor.fAttachmentCount; // Attachment descriptions to be set on the render pass @@ -62,11 +61,10 @@ void GrVkRenderPass::init(const GrVkGpu* gpu, // Refs to attachments on the render pass (as described by teh VkAttachmentDescription above), // that are used by the subpass. VkAttachmentReference colorRef; - VkAttachmentReference resolveRef; VkAttachmentReference stencilRef; uint32_t currentAttachment = 0; - // Go through each of the attachment types (color, resolve, stencil) and set the necessary + // Go through each of the attachment types (color, stencil) and set the necessary // on the various Vk structs. VkSubpassDescription subpassDesc; memset(&subpassDesc, 0, sizeof(VkSubpassDescription)); @@ -74,6 +72,8 @@ void GrVkRenderPass::init(const GrVkGpu* gpu, subpassDesc.pipelineBindPoint = VK_PIPELINE_BIND_POINT_GRAPHICS; subpassDesc.inputAttachmentCount = 0; subpassDesc.pInputAttachments = nullptr; + subpassDesc.pResolveAttachments = nullptr; + if (fAttachmentFlags & kColor_AttachmentFlag) { // set up color attachment fAttachmentsDescriptor.fColor.fLoadStoreOps = colorOp; @@ -93,21 +93,6 @@ void GrVkRenderPass::init(const GrVkGpu* gpu, } subpassDesc.pColorAttachments = &colorRef; - if (fAttachmentFlags & kResolve_AttachmentFlag) { - // set up resolve attachment - fAttachmentsDescriptor.fResolve.fLoadStoreOps = resolveOp; - setup_vk_attachment_description(&attachments[currentAttachment], - fAttachmentsDescriptor.fResolve, - VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL); - // setup subpass use of attachment - resolveRef.attachment = currentAttachment++; - // I'm really not sure what the layout should be for the resolve textures. - resolveRef.layout = VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL; - subpassDesc.pResolveAttachments = &resolveRef; - } else { - subpassDesc.pResolveAttachments = nullptr; - } - if (fAttachmentFlags & kStencil_AttachmentFlag) { // set up stencil attachment fAttachmentsDescriptor.fStencil.fLoadStoreOps = stencilOp; @@ -155,22 +140,20 @@ void GrVkRenderPass::init(const GrVkGpu* gpu, void GrVkRenderPass::init(const GrVkGpu* gpu, const GrVkRenderPass& compatibleRenderPass, const LoadStoreOps& colorOp, - const LoadStoreOps& resolveOp, const LoadStoreOps& stencilOp) { fAttachmentFlags = compatibleRenderPass.fAttachmentFlags; fAttachmentsDescriptor = compatibleRenderPass.fAttachmentsDescriptor; - this->init(gpu, colorOp, resolveOp, stencilOp); + this->init(gpu, colorOp, stencilOp); } void GrVkRenderPass::init(const GrVkGpu* gpu, const GrVkRenderTarget& target, const LoadStoreOps& colorOp, - const LoadStoreOps& resolveOp, const LoadStoreOps& stencilOp) { // Get attachment information from render target. This includes which attachments the render - // target has (color, resolve, stencil) and the attachments format and sample count. + // target has (color, stencil) and the attachments format and sample count. target.getAttachmentsDescriptor(&fAttachmentsDescriptor, &fAttachmentFlags); - this->init(gpu, colorOp, resolveOp, stencilOp); + this->init(gpu, colorOp, stencilOp); } void GrVkRenderPass::freeGPUData(const GrVkGpu* gpu) const { @@ -187,18 +170,6 @@ bool GrVkRenderPass::colorAttachmentIndex(uint32_t* index) const { return false; } -// Works under the assumption that resolve attachment will always be after the color attachment. -bool GrVkRenderPass::resolveAttachmentIndex(uint32_t* index) const { - *index = 0; - if (fAttachmentFlags & kColor_AttachmentFlag) { - ++(*index); - } - if (fAttachmentFlags & kResolve_AttachmentFlag) { - return true; - } - return false; -} - // Works under the assumption that stencil attachment will always be after the color and resolve // attachment. bool GrVkRenderPass::stencilAttachmentIndex(uint32_t* index) const { @@ -206,9 +177,6 @@ bool GrVkRenderPass::stencilAttachmentIndex(uint32_t* index) const { if (fAttachmentFlags & kColor_AttachmentFlag) { ++(*index); } - if (fAttachmentFlags & kResolve_AttachmentFlag) { - ++(*index); - } if (fAttachmentFlags & kStencil_AttachmentFlag) { return true; } @@ -249,11 +217,6 @@ bool GrVkRenderPass::isCompatible(const AttachmentsDescriptor& desc, return false; } } - if (fAttachmentFlags & kResolve_AttachmentFlag) { - if (!fAttachmentsDescriptor.fResolve.isCompatible(desc.fResolve)) { - return false; - } - } if (fAttachmentFlags & kStencil_AttachmentFlag) { if (!fAttachmentsDescriptor.fStencil.isCompatible(desc.fStencil)) { return false; @@ -276,18 +239,12 @@ bool GrVkRenderPass::isCompatible(const GrVkRenderPass& renderPass) const { } bool GrVkRenderPass::equalLoadStoreOps(const LoadStoreOps& colorOps, - const LoadStoreOps& resolveOps, const LoadStoreOps& stencilOps) const { if (fAttachmentFlags & kColor_AttachmentFlag) { if (fAttachmentsDescriptor.fColor.fLoadStoreOps != colorOps) { return false; } } - if (fAttachmentFlags & kResolve_AttachmentFlag) { - if (fAttachmentsDescriptor.fResolve.fLoadStoreOps != resolveOps) { - return false; - } - } if (fAttachmentFlags & kStencil_AttachmentFlag) { if (fAttachmentsDescriptor.fStencil.fLoadStoreOps != stencilOps) { return false; @@ -302,10 +259,6 @@ void GrVkRenderPass::genKey(GrProcessorKeyBuilder* b) const { b->add32(fAttachmentsDescriptor.fColor.fFormat); b->add32(fAttachmentsDescriptor.fColor.fSamples); } - if (fAttachmentFlags & kResolve_AttachmentFlag) { - b->add32(fAttachmentsDescriptor.fResolve.fFormat); - b->add32(fAttachmentsDescriptor.fResolve.fSamples); - } if (fAttachmentFlags & kStencil_AttachmentFlag) { b->add32(fAttachmentsDescriptor.fStencil.fFormat); b->add32(fAttachmentsDescriptor.fStencil.fSamples); diff --git a/src/gpu/vk/GrVkRenderPass.h b/src/gpu/vk/GrVkRenderPass.h index 21146ab..d59b5fa 100644 --- a/src/gpu/vk/GrVkRenderPass.h +++ b/src/gpu/vk/GrVkRenderPass.h @@ -43,13 +43,11 @@ public: void init(const GrVkGpu* gpu, const GrVkRenderTarget& target, const LoadStoreOps& colorOp, - const LoadStoreOps& resolveOp, const LoadStoreOps& stencilOp); void init(const GrVkGpu* gpu, const GrVkRenderPass& compatibleRenderPass, const LoadStoreOps& colorOp, - const LoadStoreOps& resolveOp, const LoadStoreOps& stencilOp); struct AttachmentsDescriptor { @@ -75,15 +73,13 @@ public: } }; AttachmentDesc fColor; - AttachmentDesc fResolve; AttachmentDesc fStencil; uint32_t fAttachmentCount; }; enum AttachmentFlags { kColor_AttachmentFlag = 0x1, - kResolve_AttachmentFlag = 0x2, - kStencil_AttachmentFlag = 0x4, + kStencil_AttachmentFlag = 0x2, }; GR_DECL_BITFIELD_OPS_FRIENDS(AttachmentFlags); @@ -91,7 +87,6 @@ public: // If the render pass does not have the given attachment it will return false and not set the // index value. bool colorAttachmentIndex(uint32_t* index) const; - bool resolveAttachmentIndex(uint32_t* index) const; bool stencilAttachmentIndex(uint32_t* index) const; // Sets the VkRenderPassBeginInfo and VkRenderPassContents need to begin a render pass. @@ -113,7 +108,6 @@ public: bool isCompatible(const GrVkRenderPass& renderPass) const; bool equalLoadStoreOps(const LoadStoreOps& colorOps, - const LoadStoreOps& resolveOps, const LoadStoreOps& stencilOps) const; VkRenderPass vkRenderPass() const { return fRenderPass; } @@ -133,7 +127,6 @@ private: void init(const GrVkGpu* gpu, const LoadStoreOps& colorOps, - const LoadStoreOps& resolveOps, const LoadStoreOps& stencilOps); bool isCompatible(const AttachmentsDescriptor&, const AttachmentFlags&) const; diff --git a/src/gpu/vk/GrVkRenderTarget.cpp b/src/gpu/vk/GrVkRenderTarget.cpp index dc966ca..59fc0b9 100644 --- a/src/gpu/vk/GrVkRenderTarget.cpp +++ b/src/gpu/vk/GrVkRenderTarget.cpp @@ -239,7 +239,7 @@ void GrVkRenderTarget::createFramebuffer(GrVkGpu* gpu) { const GrVkImageView* stencilView = this->stencilAttachmentView(); fFramebuffer = GrVkFramebuffer::Create(gpu, this->width(), this->height(), fCachedSimpleRenderPass, fColorAttachmentView, - fResolveAttachmentView, stencilView); + stencilView); SkASSERT(fFramebuffer); } @@ -253,12 +253,6 @@ void GrVkRenderTarget::getAttachmentsDescriptor( desc->fColor.fSamples = colorSamples ? colorSamples : 1; *attachmentFlags = GrVkRenderPass::kColor_AttachmentFlag; uint32_t attachmentCount = 1; - if (colorSamples > 0) { - desc->fResolve.fFormat = colorFormat; - desc->fResolve.fSamples = 1; - *attachmentFlags |= GrVkRenderPass::kResolve_AttachmentFlag; - ++attachmentCount; - } const GrStencilAttachment* stencil = this->renderTargetPriv().getStencilAttachment(); if (stencil) { @@ -284,12 +278,9 @@ GrVkRenderTarget::~GrVkRenderTarget() { void GrVkRenderTarget::addResources(GrVkCommandBuffer& commandBuffer) const { commandBuffer.addResource(this->framebuffer()); - commandBuffer.addResource(this->resource()); commandBuffer.addResource(this->colorAttachmentView()); - if (this->msaaImageResource()) { - commandBuffer.addResource(this->msaaImageResource()); - commandBuffer.addResource(this->resolveAttachmentView()); - } + commandBuffer.addResource(this->msaaImageResource() ? this->msaaImageResource() + : this->resource()); if (this->stencilImageResource()) { commandBuffer.addResource(this->stencilImageResource()); commandBuffer.addResource(this->stencilAttachmentView()); diff --git a/src/gpu/vk/GrVkResourceProvider.cpp b/src/gpu/vk/GrVkResourceProvider.cpp index e581384..8ae5162 100644 --- a/src/gpu/vk/GrVkResourceProvider.cpp +++ b/src/gpu/vk/GrVkResourceProvider.cpp @@ -130,7 +130,6 @@ GrVkResourceProvider::findRenderPass(const CompatibleRPHandle& compatibleHandle, CompatibleRenderPassSet& compatibleSet = fRenderPassArray[compatibleHandle.toIndex()]; const GrVkRenderPass* renderPass = compatibleSet.getRenderPass(fGpu, colorOps, - resolveOps, stencilOps); renderPass->ref(); return renderPass; @@ -417,17 +416,16 @@ bool GrVkResourceProvider::CompatibleRenderPassSet::isCompatible( GrVkRenderPass* GrVkResourceProvider::CompatibleRenderPassSet::getRenderPass( const GrVkGpu* gpu, const GrVkRenderPass::LoadStoreOps& colorOps, - const GrVkRenderPass::LoadStoreOps& resolveOps, const GrVkRenderPass::LoadStoreOps& stencilOps) { for (int i = 0; i < fRenderPasses.count(); ++i) { int idx = (i + fLastReturnedIndex) % fRenderPasses.count(); - if (fRenderPasses[idx]->equalLoadStoreOps(colorOps, resolveOps, stencilOps)) { + if (fRenderPasses[idx]->equalLoadStoreOps(colorOps, stencilOps)) { fLastReturnedIndex = idx; return fRenderPasses[idx]; } } GrVkRenderPass* renderPass = fRenderPasses.emplace_back(new GrVkRenderPass()); - renderPass->init(gpu, *this->getCompatibleRenderPass(), colorOps, resolveOps, stencilOps); + renderPass->init(gpu, *this->getCompatibleRenderPass(), colorOps, stencilOps); fLastReturnedIndex = fRenderPasses.count() - 1; return renderPass; } diff --git a/src/gpu/vk/GrVkResourceProvider.h b/src/gpu/vk/GrVkResourceProvider.h index 7f6ce6d..19d493a 100644 --- a/src/gpu/vk/GrVkResourceProvider.h +++ b/src/gpu/vk/GrVkResourceProvider.h @@ -209,7 +209,6 @@ private: GrVkRenderPass* getRenderPass(const GrVkGpu* gpu, const GrVkRenderPass::LoadStoreOps& colorOps, - const GrVkRenderPass::LoadStoreOps& resolveOps, const GrVkRenderPass::LoadStoreOps& stencilOps); void releaseResources(const GrVkGpu* gpu); diff --git a/src/gpu/vk/GrVkTextureRenderTarget.cpp b/src/gpu/vk/GrVkTextureRenderTarget.cpp index bee6a04..532d0a2 100644 --- a/src/gpu/vk/GrVkTextureRenderTarget.cpp +++ b/src/gpu/vk/GrVkTextureRenderTarget.cpp @@ -53,6 +53,7 @@ GrVkTextureRenderTarget* GrVkTextureRenderTarget::Create(GrVkGpu* gpu, msImageDesc.fMemProps = VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; if (!GrVkImage::InitImageInfo(gpu, msImageDesc, &msInfo)) { + imageView->unref(gpu); return nullptr; } -- 2.7.4