Vulkan-Samples icon indicating copy to clipboard operation
Vulkan-Samples copied to clipboard

Remove transient bit for depth image usage flags

Open asuessenbach opened this issue 2 years ago • 2 comments

Description

With a couple of samples (at least afbc, command_buffer_usage, and [hpp_]pipeline_cache), you get this validation layer warning: Validation Performance Warning: [ UNASSIGNED-BestPractices-vkCreateFramebuffer-attachment-should-not-be-transient ] | MessageID = 0xc80d9cbd | vkCreateFramebuffer(): Attachment 1 in VkFramebuffer uses loadOp/storeOps which need to access physical memory, but the image backing the image view has VK_IMAGE_USAGE_TRANSIENT_ATTACHMENT_BIT set. Physical memory will need to be backed lazily to this image, potentially causing stalls. Removing the transient bit from the depth_image resolves it. But I'm not sure, if that's the correct resolution of this warning. Maybe one should adjust the corresponding loadOp/storeOps instead.

Build tested on Win10 with VS2022. Run tested on Win10 with NVidia GPU.

General Checklist:

Please ensure the following points are checked:

  • [x] My code follows the coding style
  • [ x I have reviewed file licenses
  • [x] I have commented any added functions (in line with Doxygen)
  • [x] I have commented any code that could be hard to understand
  • [x] My changes do not add any new compiler warnings
  • [x] My changes do not add any new validation layer errors or warnings
  • [x] I have used existing framework/helper functions where possible
  • [x] My changes do not add any regressions
  • [ ] I have tested every sample to ensure everything runs correctly
  • [x] This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • [x] I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • [x] My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

  • [ ] I have tested the sample on at least one compliant Vulkan implementation
  • [ ] If the sample is vendor-specific, I have tagged it appropriately
  • [ ] I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
  • [ ] Any dependent assets have been merged and published in downstream modules
  • [ ] For new samples, I have added a paragraph with a summary to the appropriate chapter in the samples readme
  • [ ] For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
  • [ ] For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

asuessenbach avatar Jan 17 '24 12:01 asuessenbach

HI @asuessenbach , I tried the samples listed above with a similar setup, and cannot reproduce the error messages (get an assertion for command_buffer_usage though...). I'm using SDK 1.3.275.

Update: apologies, had not enabled Best Practice warnings, can see it now and will try to fix it

JoseEmilio-ARM avatar Apr 08 '24 13:04 JoseEmilio-ARM

HI @asuessenbach , what do you think of this solution?

diff --git a/framework/rendering/render_pipeline.cpp b/framework/rendering/render_pipeline.cpp
index 49085b81..3bb6842e 100644
--- a/framework/rendering/render_pipeline.cpp
+++ b/framework/rendering/render_pipeline.cpp
@@ -34,6 +34,14 @@ RenderPipeline::RenderPipeline(std::vector<std::unique_ptr<Subpass>> &&subpasses
 {
        prepare();

+       // Default load/store for swapchain
+       load_store[0].load_op  = VK_ATTACHMENT_LOAD_OP_CLEAR;
+       load_store[0].store_op = VK_ATTACHMENT_STORE_OP_STORE;
+
+       // Default load/store for depth attachment
+       load_store[1].load_op  = VK_ATTACHMENT_LOAD_OP_CLEAR;
+       load_store[1].store_op = VK_ATTACHMENT_STORE_OP_DONT_CARE;
+
        // Default clear value
        clear_value[0].color        = {0.0f, 0.0f, 0.0f, 1.0f};
        clear_value[1].depthStencil = {0.0f, ~0U};

I tested it and it fixes the validation messages on my side, without risking making the depth attachment non-transient by default.

JoseEmilio-ARM avatar Apr 08 '24 14:04 JoseEmilio-ARM

Closing as PR 1058 covers this

marty-johnson59 avatar Jun 03 '24 19:06 marty-johnson59