Autodesk: General HgiVulkan stability #2852
Conversation
1. Fixes Color Correction 2. Provides Layout Transition infrastructure 3. Fixes most Vulkan validation errors 4. Fixes debug layer struct issue (cherry picked from commit d10ea20a202458727ae5d667dae5dd9bf9bdfbf7)
| { { -1, 3, 0, 1, 0, 2 }, | ||
| { -1, -1, 0, 1, 0, 0 }, | ||
| { 3, -1, 0, 1, 2, 0 } }; | ||
|
|
||
| // Vulkan back-end needs the UVs inverted along the y axis |
There was a problem hiding this comment.
Instead of fixing this here, it might be better to bring in the fix at the shader generation level. See #2504
There was a problem hiding this comment.
My PR had some issues with HdStorm, I have updated it to remove some shader compilation errors.
|
Filed as internal issue #USD-9060 |
| // The following cases are based on few initial assumptions to provide | ||
| // an infrastructure for access mask selection based on layouts. | ||
| // Feel free to update depending on need and use cases. | ||
| switch (GetImageLayout()) { |
There was a problem hiding this comment.
This and the following switch statement need a default case, otherwise they fail to compile for me.
| @@ -132,21 +132,19 @@ HgiVulkanGraphicsPipeline::HgiVulkanGraphicsPipeline( | |||
| vertBufs.push_back(std::move(vib)); | |||
| } | |||
|
|
|||
| VkPipelineVertexInputDivisorStateCreateInfoEXT vertexInputDivisor = | |||
There was a problem hiding this comment.
I'm guessing this work was done on an earlier version of the dev/feature branch, since I believe this change is no longer needed.
There was a problem hiding this comment.
Sure. This will be undone in the oncoming push
pxr/imaging/hgiVulkan/instance.cpp
Outdated
| @@ -67,15 +67,15 @@ HgiVulkanInstance::HgiVulkanInstance() | |||
| VK_KHR_GET_PHYSICAL_DEVICE_PROPERTIES_2_EXTENSION_NAME, | |||
| }; | |||
|
|
|||
| std::vector<const char*> instanceLayerList; | |||
There was a problem hiding this comment.
This I think is also unneeded now.
| bufDesc.usage = HgiBufferUsageUniform; | ||
| // Vulkan Validation layer is raising an error here because | ||
| // a usage flag of Uniform is applied to a buffer bound as storage | ||
| bufDesc.usage = HgiBufferUsageUniform | HgiBufferUsageStorage; |
There was a problem hiding this comment.
If you want this to run with HgiGL, too, then can you add a Vulkan-specific check here like you did for vboMemoryManger.cpp?
| bufDesc.usage = HgiBufferUsageUniform | HgiBufferUsageVertex; | ||
| // Vulkan Validation layer is raising an error here because | ||
| // a usage flag of Uniform is applied to a buffer bound as storage | ||
| bufDesc.usage = HgiBufferUsageUniform | HgiBufferUsageStorage; |
There was a problem hiding this comment.
Same here re: a Vulkan-specific check so this works for HgiGL too.
(cherry picked from commit 173d02c573ed820e4d79aa3aebe5610298963d04)
| @@ -147,8 +147,14 @@ TF_DEFINE_PRIVATE_TOKENS( | |||
| (early_fragment_tests) | |||
| ); | |||
|
|
|||
| // For now, looks like for Vulkan path, we require HGI Resource Generation path enabled | |||
There was a problem hiding this comment.
This too is already fixed in the current branch, just FYI.
1. VkPipelineVertexInputStateCreateInfo (following latest fixes proposed by Pixar) 2. Instance Layer Extensions (following latest fixes proposed by Pixar) 3. UV flipping fix in shader selection for Vulkan (following latest fixes proposed by Adobe) 4. Undoing temp code aound flipping color correction UVs flip (cherry picked from commit db0a46bede5b9504bb6954dc8ac7b1b5d6e5f364)
| @@ -253,7 +254,7 @@ HgiVulkanDevice::HgiVulkanDevice(HgiVulkanInstance* instance) | |||
|
|
|||
| VkPhysicalDeviceFeatures2 features = | |||
| {VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2}; | |||
| features.pNext = &vulkan11Features; | |||
| features.pNext = (instance->GetVulkanVersion() >= VK_API_VERSION_1_2) ? &vulkan11Features : nullptr; | |||
There was a problem hiding this comment.
Can you elaborate on the need for this change? I'm assuming this is because you're potentially targeting Vulkan versions < 1.2 for Android support. Do you have an idea of what other changes will be needed for that goal?
Right now we use vulkan11Features here to potentially enable shaderDrawParameters (you can see the associated hgi capability flag set in HgiVulkanCapabilities). If we potentially disapply vulkan11Features here, would that need a corresponding modification to HgiVulkanCapabilities?
clach
merged commit 97b5aad
into
PixarAnimationStudios:feature-hgi-vulkan
…ly expects one aspect when copying an image to buffer, so if the image is a depth-stencil image, assume we wish to copy the depth aspect. See #2852 (Internal change: 2316631)
Description of Change(s)
This PR provides the following support to the hgiVulkan backend
Fixes Issue(s)