-
Notifications
You must be signed in to change notification settings - Fork 410
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
layers: Fix check for VUID-07908 #6931
base: main
Are you sure you want to change the base?
Conversation
CI Vulkan-ValidationLayers build queued with queue ID 80388. |
CI Vulkan-ValidationLayers build # 14455 running. |
CI Vulkan-ValidationLayers build # 14455 passed. |
04f3145
to
f8ebd1f
Compare
CI Vulkan-ValidationLayers build queued with queue ID 82144. |
f8ebd1f
to
327fedb
Compare
CI Vulkan-ValidationLayers build queued with queue ID 82146. |
327fedb
to
dfa0c29
Compare
CI Vulkan-ValidationLayers build queued with queue ID 82174. |
CI Vulkan-ValidationLayers build # 14494 running. |
CI Vulkan-ValidationLayers build # 14494 passed. |
tests/unit/dynamic_rendering.cpp
Outdated
@@ -3680,8 +3679,11 @@ TEST_F(NegativeDynamicRendering, FragmentDensityMapAttachmentLayerCount) { | |||
VK_FORMAT_FEATURE_FRAGMENT_DENSITY_MAP_BIT_EXT)) { | |||
GTEST_SKIP() << "VK_FORMAT_FEATURE_FRAGMENT_DENSITY_MAP_BIT_EXT not supported"; | |||
} | |||
if (!multiview_features.multiview) { | |||
GTEST_SKIP() << "Test requires (unsupported) multiview"; | |||
if (multiview_features.multiview || physDevProps().apiVersion >= VK_API_VERSION_1_1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so for these we have done
SetTargetApiVersion(VK_API_VERSION_1_0);
AddRequiredExtensions(VK_EXT_FRAGMENT_DENSITY_MAP_EXTENSION_NAME);
RETURN_IF_SKIP(InitFramework())
if (DeviceValidationVersion() != VK_API_VERSION_1_0) {
GTEST_SKIP() << "Tests for 1.0 only";
}
there is no need to query VkPhysicalDeviceMultiviewFeatures
if we are not enabling the extension so that line can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied the changed.
There's one minor comment. I believe this VUID is either malformed or adds nothing. Mainly because the VUID is only present with dynamic rendering which requires multiview, so multiview extension will always be enabled. Shouldn't the VUID check for the feature support instead of extension being enabled? The VUID states:
If VK_KHR_multiview is not enabled
, but shouldn't it be If VkPhysicalDeviceMultiviewFeatures.multiview is VK_FALSE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a good chance it adds no value, we have found a few VUs that are really just caught elsewhere, but were hard to tell with the old ifdef
hell in the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://gitlab.khronos.org/vulkan/vulkan/-/issues/3714
I'll wait for the resolution of that issue before we move in any direction in VVL
dfa0c29
to
f9918a1
Compare
CI Vulkan-ValidationLayers build queued with queue ID 87124. |
CI Vulkan-ValidationLayers build # 14608 running. |
Extension and api version was missing from the check
Remove VUID-07908 from NegativeDynamicRendering.RenderingFragmentDensityMapAttachment Account for VUID-07908 changes in NegativeDynamicRendering.FragmentDensityMapAttachmentLayerCount
f9918a1
to
9ead60a
Compare
CI Vulkan-ValidationLayers build queued with queue ID 87137. |
CI Vulkan-ValidationLayers build # 14609 running. |
CI Vulkan-ValidationLayers build # 14609 passed. |
Extension and api version was missing from the check