KhronosGroup / Vulkan-LoaderAndValidationLayers

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

Incorrect image layer validation with secondary command buffers #2653

Open fduranleau-gl opened 6 years ago

fduranleau-gl commented 6 years ago

We had this situation where we used an image in a render pass both as a depth attachment and as shader resource. The render pass set the image's layout to VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL, but the same image was used in a descriptor with layout VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL. So obviously, we had image layout conflicts. However, commands for the render pass were recorded into secondary command buffers, and we had no errors being reported by core validation layer. As soon as we did not use secondary command buffers, we got the errors being reported.

There is no need record those secondary command buffes in parallel in other threads to get this error. Simply doing this sequence of commands will show the problem:

  1. begin the render pass, with flag to use secondary command buffers
  2. begin a secondary command buffer, with inheritance using render pass from 1.
  3. issue a draw command using one of the render pass' attachments in a descriptor, but with a different layout
  4. end command buffer
  5. execute command buffer
  6. end render pass

Doing the above, validation reports no errors. But doing this:

  1. begin the render pass
  2. issue a draw command using one of the render pass' attachments in a descriptor, but with a different layout
  3. end render pass

triggers the expected error.

For the record, if it changes something to analyse this problem, the whole render pass in our use case is in its own primary command buffer. The rest is in other primary command buffers.

tobine commented 6 years ago

@jzulauf-lunarg this replaces #2640

fduranleau-gl commented 6 years ago

Indeed, I forgot to mention that.

chrisforbes commented 6 years ago

Even without the layout conflict, this seems likely to not be valid; see this bit of 7.3:

Applications must ensure that all accesses to memory that backs image subresources used as
attachments in a given renderpass instance either happen-before the load operations for those
attachments, or happen-after the store operations for those attachments.

For depth/stencil attachments, each aspect can be used separately as attachments and nonattachments
as long as the non-attachment accesses are also via an image subresource in either the
VK_IMAGE_LAYOUT_DEPTH_READ_ONLY_STENCIL_ATTACHMENT_OPTIMAL layout or the
VK_IMAGE_LAYOUT_DEPTH_ATTACHMENT_STENCIL_READ_ONLY_OPTIMAL layout, and the attachment resource
uses whichever of those two layouts the image accesses do not. Use of non-attachment aspects in
this case is only well defined if the attachment is used in the subpass where the non-attachment
access is being made, or the layout of the image subresource is constant throughout the entire
render pass instance, including the initialLayout and finalLayout.

Note
These restrictions mean that the render pass has full knowledge of all uses of all of
the attachments, so that the implementation is able to make correct decisions
about when and how to perform layout transitions, when to overlap execution of
subpasses, etc.
fduranleau-gl commented 6 years ago

Right, I did miss that part of the spec. The situation comes from a sample using our engine that worked fine with GL and Metal. We hit this while porting to Vulkan (we would need to use an input attachment and multiple subpasses). That being said, the problem remains. I still expect to get an error to be reported when I use a secondary command buffer, whether that error is a conflict of layout or a non-attachment access during a render pass.