KhronosGroup / Vulkan-LoaderAndValidationLayers

**Deprecated repository** for Vulkan loader and validation layers
Apache License 2.0
414 stars 172 forks source link

Initial image layout not correctly validated #2652

Closed tobine closed 6 years ago

tobine commented 6 years ago

In analyzing InvalidImageLayout test with @Whenning42 he highlighted what appears to be a fundamental issue in how initial image layouts are treated in validation.

First some spec background. According to section 11.4 Image Layouts:

Upon creation, all image subresources of an image are initially in the same layout, where that layout is selected by the VkImageCreateInfo::initialLayout member. The initialLayout must be either VK_IMAGE_LAYOUT_UNDEFINED or VK_IMAGE_LAYOUT_PREINITIALIZED. For either of these initial layouts, any image subresources must be transitioned to another layout before they are accessed by the device.

Just above that text, in the same section, two conditions of when an image layout transition are defined as:

Transitions can happen with an image memory barrier, included as part of a vkCmdPipelineBarrier or a vkCmdWaitEvents command buffer command (see Image Memory Barriers), or as part of a subpass dependency within a render pass (see VkSubpassDependency).

However, the way validation currently behaves, in the InvalidImageLayout test there is an implicit transition that occurs from the vkCreateImage() initialLayout of VK_IMAGE_LAYOUT_UNDEFINED to VK_IMAGE_LAYOUT_GENERAL. This occurs in the first vkCmdCopyImage() call, which is the first time that src_image is used. The PreCallValidateCmdCopyImage() calls down to VerifyImageLayout() which then makes this call to FindCmdBufLayout(). That fails to find the layout in the cmdBuffer, as this is the first use of the Image, but validation goes along and in the vkCmdCopyImage() case it assumes that the explicit layout passed in is the correct current layout. This results in an implicit assumption that the layout was transition toe the explicit value, VK_IMAGE_LAYOUT_GENERAL in this case. This implicit transition is made explicit here in PreCallCmdRecordCmdCopyImage().

So, the fundamental image layout transition bug here seems to be that when the FindCmdBufLayout() fails we shouldn't just proceed, but should check the Image's creation initialLayout against the current explicit layout.

There also seems to be a bug in the fact that vkCmdCopyImage() is setting the layout. This should be unnecessary if the checking is correct, and it's a bug in that vkCmdCopyImage() is not a transition command.

This behavior may be related to #2640 although I don't know that it's exactly the same issue as that issue also deals with inter-cmd-buffer layout issues.

tobine commented 6 years ago

Alright, @chrisforbes set me straight and I think this bug is bogus. We can't validate the initial layout of an image at record time into a cmdBuffer. Can only verify that at submit time, so it's invalid to do any checking of initialLayout from image creation at cmd record time. Also, we need to record the explicit layout for vkCmdCopyImage() at record time (the SetLayout() call in PreCallCmdRecordCmdCopyImage()) because that will be the layout at that point in the cmdBuffer going fwd. This only comes into play when the vkCmdCopyImage() is the first cmd of the cmd buffer. What I was mistaking for an "implicit layout transition" is really just recording reality for subsequent commands in a cmd buffer that can be checked for layout at cmd record time.

Going to close this issue but will revisit the code an annotate with comments for now to clarify a couple of sections. Ultimately would be nice to overhaul the layout validation system but comments will have to do for now.