Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Best Practices Tracking Issue #24

Closed
karl-lunarg opened this issue May 14, 2018 · 21 comments
Closed

Best Practices Tracking Issue #24

karl-lunarg opened this issue May 14, 2018 · 21 comments
Assignees
Milestone

Comments

@karl-lunarg
Copy link
Contributor

karl-lunarg commented May 14, 2018

All Best Practices information is now tracked via the Vulkan-ValidationLayers project page. Please feel free to add cards to that project, or to create new issues for enhancements to Best Practices, so that we may add them to the project.

Thanks!

@karl-lunarg karl-lunarg added this to the P2 milestone May 14, 2018
@karl-lunarg karl-lunarg added the Enhancement New feature or request label May 14, 2018
@karl-lunarg
Copy link
Contributor Author

Comment by karl-lunarg (MIGRATED)
Tuesday Mar 28, 2017 at 19:27 GMT


#1628 "VK_LAYER_LUNARG_core_validation queue submissions list grows forever and causes crash" might be another candidate condition to check for in this layer.

@karl-lunarg
Copy link
Contributor Author

Comment by RippeR37 (MIGRATED)
Sunday Apr 23, 2017 at 09:20 GMT


Idea: Maybe this layer could also check if same state is set multiple times in a row which can lead to lower performance?

@karl-lunarg
Copy link
Contributor Author

Comment by mark-lunarg (MIGRATED)
Friday May 05, 2017 at 13:53 GMT


From @Tony-LunarG: Assuming that the 99% common workflow is [BeginCommandBuffer; write commands to command buffer; EndCommandBuffer], it would be nice to warn developer if the code is recording two command buffers simultaneously. Would also need to validate assumption about workflow. Note that it needs to be thread smart since we expect multiple threads to be recording multiple command buffers simultaneously

@karl-lunarg
Copy link
Contributor Author

Comment by MarkY-LunarG (MIGRATED)
Tuesday Jun 27, 2017 at 14:14 GMT


Idea: Detect case of LVL issue #1917 and warn.

@karl-lunarg
Copy link
Contributor Author

Comment by karl-lunarg (MIGRATED)
Thursday Sep 07, 2017 at 15:16 GMT


Idea: #2061 - Add a layer that writes a unique ID to a CPU-visible buffer whenever a marker is encountered. After a GPU crash, the buffer can be examined to see the last ID written.

@karl-lunarg
Copy link
Contributor Author

Comment by HansKristian-ARM (MIGRATED)
Thursday Dec 07, 2017 at 16:12 GMT


Is this supposed to be a generic layer for best practices on Vulkan which collects best practices from a spec POV, or would there be room for IHV specifics as well (which are probably where the most useful/interesting checks will be)?

@karl-lunarg
Copy link
Contributor Author

Comment by mark-lunarg (MIGRATED)
Thursday Dec 07, 2017 at 17:12 GMT


@HansKristian-ARM, the Assistant Layer effort is currently focused on the former. However, it is a simple matter to create related assistant-type layers specific to a particular IHV. The aim of the layer framework is to make the creation of these types of layers much more straightforward.

@karl-lunarg
Copy link
Contributor Author

Comment by hrydgard (MIGRATED)
Saturday Mar 17, 2018 at 12:42 GMT


Regarding transitions from VK_IMAGE_LAYOUT_UNDEFINED to anything: When initializing textures with known data, I:

  • create with VK_IMAGE_LAYOUT_UNDEFINED
  • transition to VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL (for the src stage, I use VK_PIPELINE_STAGE_ALL_COMMANDS)
  • vkCmdCopyBufferToImage to write the contents
  • transition to VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL

As this sequence will trigger a couple of the proposed checks in this issue, what's the recommended way to do this? PREINITIALIZED is no good because I don't want to create a linear image.

It seems to me that it would make sense to allow creating images directly in VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL and copy to them , I don't quite understand why that's not allowed.

@karl-lunarg
Copy link
Contributor Author

Comment by krOoze (MIGRATED)
Saturday Mar 17, 2018 at 17:45 GMT


As this sequence will trigger a couple of the proposed checks in this issue, what's the recommended way to do this?

It will?
You are making the contents defined in step 3, so the layers should not complain. Your sequence seems fine to me (except perhaps the wasteful VK_PIPELINE_STAGE_ALL_COMMANDS).

creating images directly in VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL and copy to them , I don't quite understand why that's not allowed.

I would assume the same reason why PREINITIALIZED is distinct from GENERAL. It ensures there will be at least one barrier before the first use of the Image by the device.

@karl-lunarg
Copy link
Contributor Author

Comment by hrydgard (MIGRATED)
Saturday Mar 17, 2018 at 18:11 GMT


It will?

This one: "Transitions from VK_IMAGE_LAYOUT_UNDEFINED to anything".

wasteful VK_PIPELINE_STAGE_ALL_COMMANDS

What src pipeline stage would you suggest for transitioning from UNDEFINED? Simply BOTTOM_OF_PIPE?

EDIT: Well TOP_OF_PIPE of course, thanks. Keep confusing them.

@karl-lunarg
Copy link
Contributor Author

Comment by krOoze (MIGRATED)
Saturday Mar 17, 2018 at 18:28 GMT


It will?

This one: "Transitions from VK_IMAGE_LAYOUT_UNDEFINED to anything".

You need to read the rest of that paragraph. It concerns itself with a situation where someone transitions from UNDEFINED and then reads the contents. The contents become undefined, so (depending on platform) it may contain original data or it may be trashed upon the read.

But you are writing the content, which makes it defined again. So it should not trigger any such checks.
Check implemented in a way which interferes with your case would indeed be useless. It is a very common case, and it cannot be done any differently for TILING_OPTIMAL images.

What src pipeline stage would you suggest for transitioning from UNDEFINED? Simply BOTTOM_OF_PIPE?

New Image\Memory does not have any outstanding read\write operations on it to synchronize against. So, assuming you are not merging other barriers to this, srcStage = TOP_OF_PIPE (i.e. no execution dependency) will suffice.

@TonyBarbour
Copy link
Contributor

Warn if calling vkAcquireNextImageKHR before calling vkGetSwapchainImagesKHR

@tobine
Copy link
Contributor

tobine commented Dec 20, 2018

Moving former issue #40 into this issue.
Note disturbed descriptors when unbound set used
We had a previous perf warning for disturbed descriptors, along with associate test. Developers didn't like this perf warning (See #2020 ) so we removed it. As an enhancement, track disturbed descriptors and if a disturbed descriptor is used while unbound, note that it's unbound due to previously being disturbed.

@daveh-lunarg
Copy link
Contributor

Moving issue #512 here. Likely covered by #2089 in initial post.

@asuonpaa
Copy link
Contributor

As mentioned in issue #578 validation layers was producing a false positive by checking that attachment references in subpass description don't use the same attachment ID more than once. In most cases this is not what was intended, so assistant layers could warn about this.

@MarkY-LunarG
Copy link
Contributor

Detect when the stride in VkVertexInputBindingDescription is less than the largest offset + size of the VkVertexInputAttributeDescription in the VkPipelineVertexInputStateCreateInfo structure. I ran into this where my VkVertexInputBindingDescription.stride was less and I got corruption. Validation layers don't trigger a warning/error of this case.

camden-lunarg added a commit that referenced this issue Nov 6, 2019
resolves #1307 which was tracked in #24

Change-Id: If34c89c4fd40d75803ea2d38539a2050570cf374
camden-lunarg added a commit that referenced this issue Nov 8, 2019
resolves #1307 which was tracked in #24

Change-Id: If34c89c4fd40d75803ea2d38539a2050570cf374
camden-lunarg added a commit that referenced this issue Nov 14, 2019
resolves #1307 which was tracked in #24

Change-Id: If34c89c4fd40d75803ea2d38539a2050570cf374
@mikew-lunarg
Copy link

Added specialuse checklist rollup 1938

@gfxstrand
Copy link
Contributor

It would be good to warn if the app unnecessarily creates large quantities of duplicated objects such as samplers or pipelines. Some objects such as samplers, fences, images, and buffers make sense to have lots of similar ones. However, for meta-data objects like render passes and fully encapsulated objects such as samplers and pipelines, the duplication isn't necessary. We probably don't want to throw a warning for a small quantity of duplication but if you've got 1000 identical samplers, a warning might be helpful to the developer.

It's worth noting that on most hardware implementations, I wouldn't expect de-duplication to make a significant performance improvement. There's nothing inherently wrong with having 1000 identical samplers. It's more that if you have 1000 identical samplers, you're probably also doing way too much descriptor set binding and updating and that can have a negative performance impact.

@TomOlson
Copy link

Please add #2063 to the feature request list. The WG will document what needs to be checked and drop a reference into that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests