vkd3d: Fix a number of minor issues happening on MoltenVK.
Some are test failures, some are validation errors.
Merge request reports
Activity
+ /* This and similar tests later currently fail on MoltenVK with E_FAIL + * because VK_EXT_descriptor_indexing is not available. This check happens + * before detecting the overlap that would trigger E_INVALIDARG. We still + * check that we're seeing a failure. */
Using "todo" for this isn't necessarily wrong, but it's also not specific to MoltenVK; you'd see this anywhere VK_EXT_descriptor_indexing isn't supported. If we're going to add a todo, it should probably either check "binding_tier", or availability of the actual extension.
But it seems like it would be easier to just fix it? (I.e., instead of checking for "unbounded && !use_array" in d3d12_root_signature_info_count_descriptors() we could just check cbv/srv/uav/sampler_unbounded_range_count afterwards in d3d12_root_signature_info_from_desc(), or even store "unbounded" in struct d3d12_root_signature_info.)
Subject: [PATCH 2/5] vkd3d: Do not request VK_EXT_debug_marker if debug is disable.
"disabled"
+/* For VK_KHR_portability_subset. */ +#define VK_ENABLE_BETA_EXTENSIONS
We've so far stayed away from beta extensions, for I think self-evident reasons. Enabling VK_KHR_portability_subset when available seems innocuous enough, but VK_ENABLE_BETA_EXTENSIONS effectively opens the gate for other beta extensions as well.
+ vulkan_info->geometry_shaders = physical_device_info->features2.features.geometryShader;
Arguably we shouldn't allow creating a d3d12 device without geometry shaders. We already have a CHECK_FEATURE for that in vkd3d_init_feature_level(), but don't currently fail on those. In any case, I was under the impression that there's still hope for geometry shader support in MoltenVK; is this something that needs addressing in vkd3d?
Subject: [PATCH 5/5] tests: Clear UAV with the proper type.
I suppose that's fine, but the commit message seems misleading; as far as I'm aware it's unconventional but valid to clear integer format UAVs with ID3D12GraphicsCommandList_ClearUnorderedAccessViewFloat(). Why does MoltenVK care?
Using "todo" for this isn't necessarily wrong, but it's also not specific to MoltenVK; you'd see this anywhere VK_EXT_descriptor_indexing isn't supported. If we're going to add a todo, it should probably either check "binding_tier", or availability of the actual extension.
Ok, that makes sense.
But it seems like it would be easier to just fix it? (I.e., instead of checking for "unbounded && !use_array" in d3d12_root_signature_info_count_descriptors() we could just check cbv/srv/uav/sampler_unbounded_range_count afterwards in d3d12_root_signature_info_from_desc(), or even store "unbounded" in struct d3d12_root_signature_info.)
Mmh, it's not that easy, to me at least. Validating that the requested ranges are invalid doesn't happen until much later (in
validate_descriptor_register_ranges()
), and in the meantime the code seems to assume that descriptor indexing is available (if there are unbounded ranges) because if I just disable theunbounded && !use_array
check it segfaults. I don't know that piece of code very well yet (it looks quite convoluted), and my intention for this MR was to be a relatively quick thing.We've so far stayed away from beta extensions, for I think self-evident reasons. Enabling VK_KHR_portability_subset when available seems innocuous enough, but VK_ENABLE_BETA_EXTENSIONS effectively opens the gate for other beta extensions as well.
The only thing I need from that extension is its name. Would it be better for you if I just defined it in our source code, so we don't have to open the gate?
BTW, my goal here (and for the following patches) is to enhance the validation layer signal-to-noise ratio by avoiding errors that are easy to avoid. In principle this can also be handled otherwise (I suppose I can patch my own MoltenVK build to not even expose the extension), but I guess that having this patch might be useful for other developers too (and require fewer moving parts on my side).
Arguably we shouldn't allow creating a d3d12 device without geometry shaders. We already have a CHECK_FEATURE for that in vkd3d_init_feature_level(), but don't currently fail on those. In any case, I was under the impression that there's still hope for geometry shader support in MoltenVK; is this something that needs addressing in vkd3d?
Since we currently don't fail when geometry shaders are not available the current code is triggering a Vulkan validation error:
VUID-vkCmdPipelineBarrier-dstStageMask-04090(ERROR / SPEC): msgNum: 1131989869 - Validation Error: [ VUID-vkCmdPipelineBarrier-dstStageMask-04090 ] Object 0: handle = 0x14a73e288, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x4378cb6d | vkCmdPipelineBarrier(): dstStageMask includes VK_PIPELINE_STAGE_GEOMETRY_SHADER_BIT when the device does not have geometryShader feature enabled. The Vulkan spec states: If the geometryShader feature is not enabled, dstStageMask must not contain VK_PIPELINE_STAGE_GEOMETRY_SHADER_BIT (https://vulkan.lunarg.com/doc/view/1.3.268.1/mac/1.3-extensions/vkspec.html#VUID-vkCmdPipelineBarrier-dstStageMask-04090) Objects: 1 [0] 0x14a73e288, type: 6, name: NULL
This is not specific to MoltenVK, but MoltenVK makes it evident.
I suppose that's fine, but the commit message seems misleading; as far as I'm aware it's unconventional but valid to clear integer format UAVs with ID3D12GraphicsCommandList_ClearUnorderedAccessViewFloat(). Why does MoltenVK care?
This is another problem at the Vulkan level, it triggers this validation error:
VUID-vkCmdDispatch-format-07753(ERROR / SPEC): msgNum: 1729412364 - Validation Error: [ VUID-vkCmdDispatch-format-07753 ] Object 0: handle = 0x130160000000005c, type = VK_OBJECT_TYPE_DESCRIPTOR_SET; Object 1: handle = 0x4868e6000000005a, type = VK_OBJECT_TYPE_IMAGE_VIEW; | MessageID = 0x6714bd0c | vkCmdDispatch(): the descriptor (VkDescriptorSet 0x130160000000005c[], binding 0, index 0) requires FLOAT component type, but bound descriptor format is VK_FORMAT_R32_SINT. The Vulkan spec states: If a VkImageView is accessed as a result of this command, then the numeric type of the image view's format and the Sampled Type operand of the OpTypeImage must match (https://vulkan.lunarg.com/doc/view/1.3.268.1/mac/1.3-extensions/vkspec.html#VUID-vkCmdDispatch-format-07753) Objects: 2 [0] 0x130160000000005c, type: 23, name: NULL [1] 0x4868e6000000005a, type: 14, name: NULL
(and immediately after a similar Metal validation error)
If D3D12 allows clearing UAVs with uint instead of float, then we have a bug in vkd3d.
added 5 commits
- f7a95e2c - tests: Mark some root signature unbounded range failures as todo on MoltenVK.
- 0b299ee7 - vkd3d: Do not request VK_EXT_debug_marker if debug is disabled.
- 740fd44b - vkd3d: Enable VK_KHR_portability_subset if available.
- f5500116 - vkd3d: Do not synchronize with the geometry shader stage if it's not enabled.
- eb4f5355 - tests: Clear UAV with the proper type.
Toggle commit listMmh, it's not that easy, to me at least. Validating that the requested ranges are invalid doesn't happen until much later (in
validate_descriptor_register_ranges()
), and in the meantime the code seems to assume that descriptor indexing is available (if there are unbounded ranges) because if I just disable theunbounded && !use_array
check it segfaults. I don't know that piece of code very well yet (it looks quite convoluted), and my intention for this MR was to be a relatively quick thing.Oh, that's unfortunate, yes.
The only thing I need from that extension is its name. Would it be better for you if I just defined it in our source code, so we don't have to open the gate?
Yes, I think that's better. Still irritating that after all this time VK_KHR_portability_subset is still effectively unusable in a reasonable way. Should we guard that #define with a #ifndef VK_KHR_PORTABILITY_SUBSET_EXTENSION_NAME?
Since we currently don't fail when geometry shaders are not available the current code is triggering a Vulkan validation error:
Right. In theory once upstream MoltenVK supports geometry shaders we could just require them though, and then this issue would go away by definition. One of the considerations is that in practice geometry shader support is fairly essential; the number of applications that can run without geometry shader support isn't zero, but it's not especially large either.
If D3D12 allows clearing UAVs with uint instead of float, then we have a bug in vkd3d.
I think it looks like we do, although it's perhaps a little subtle. We have two variants of the UAV clear shaders for each view type: "pipelines_uint" and "pipelines_float". When we go through d3d12_command_list_ClearUnorderedAccessViewUint(), we always want to use "pipelines_uint" in order to clear with a specific bit pattern, so we create a temporary UINT view if the original view format type isn't VKD3D_FORMAT_TYPE_UINT. We don't care as much for d3d12_command_list_ClearUnorderedAccessViewFloat(), and use "pipelines_uint" for VKD3D_FORMAT_TYPE_UINT, and "pipelines_float" otherwise. The view in this test uses DXGI_FORMAT_R32_SINT though, which would be VKD3D_FORMAT_TYPE_SINT, and we end up with "pipelines_float". Note that "pipelines_uint" wouldn't be quite correct for VKD3D_FORMAT_TYPE_SINT either though.
Yes, I think that's better. Still irritating that after all this time VK_KHR_portability_subset is still effectively unusable in a reasonable way. Should we guard that #define with a #ifndef VK_KHR_PORTABILITY_SUBSET_EXTENSION_NAME?
Right, it might get out of beta eventually.
Right. In theory once upstream MoltenVK supports geometry shaders we could just require them though, and then this issue would go away by definition.
Agreed.
One of the considerations is that in practice geometry shader support is fairly essential; the number of applications that can run without geometry shader support isn't zero, but it's not especially large either.
I agree that in principle we should fail if the Vulkan implementation doesn't have geometry shaders, but possibly retain some configuration options to still be able to force success.
I think it looks like we do, although it's perhaps a little subtle. We have two variants of the UAV clear shaders for each view type: "pipelines_uint" and "pipelines_float". When we go through d3d12_command_list_ClearUnorderedAccessViewUint(), we always want to use "pipelines_uint" in order to clear with a specific bit pattern, so we create a temporary UINT view if the original view format type isn't VKD3D_FORMAT_TYPE_UINT. We don't care as much for d3d12_command_list_ClearUnorderedAccessViewFloat(), and use "pipelines_uint" for VKD3D_FORMAT_TYPE_UINT, and "pipelines_float" otherwise. The view in this test uses DXGI_FORMAT_R32_SINT though, which would be VKD3D_FORMAT_TYPE_SINT, and we end up with "pipelines_float". Note that "pipelines_uint" wouldn't be quite correct for VKD3D_FORMAT_TYPE_SINT either though.
Ok, thanks for the explanation.
added 21 commits
-
eb4f5355...cefd6f9d - 16 commits from branch
wine:master
- 5dd7be9d - tests: Mark some root signature unbounded range failures as todo on MoltenVK.
- 35e58df2 - vkd3d: Do not request VK_EXT_debug_marker if debug is disabled.
- 39e2f260 - vkd3d: Enable VK_KHR_portability_subset if available.
- e7b152bb - vkd3d: Do not synchronize with the geometry shader stage if it's not enabled.
- 384615ea - tests: Clear UAV with the proper type.
Toggle commit list-
eb4f5355...cefd6f9d - 16 commits from branch
Ok, thanks for the explanation.
To be clear, I think we'd just need something like the following to fix it?
diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index 95366d344..7ecafe3ab 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -5434,6 +5434,52 @@ static const struct vkd3d_format *vkd3d_fixup_clear_uav_uint_colour(struct d3d12 } } +static struct vkd3d_view *create_uint_view(struct d3d12_device *device, const struct vkd3d_resource_view *view, + struct d3d12_resource *resource, VkClearColorValue *colour) +{ + struct vkd3d_texture_view_desc view_desc; + const struct vkd3d_format *uint_format; + struct vkd3d_view *uint_view; + + if (!(uint_format = vkd3d_find_uint_format(device, view->format->dxgi_format)) + && !(uint_format = vkd3d_fixup_clear_uav_uint_colour(device, view->format->dxgi_format, colour))) + { + ERR("Unhandled format %#x.\n", view->format->dxgi_format); + return NULL; + } + + if (d3d12_resource_is_buffer(resource)) + { + if (!vkd3d_create_buffer_view(device, VKD3D_DESCRIPTOR_MAGIC_UAV, resource->u.vk_buffer, + uint_format, view->info.buffer.offset, view->info.buffer.size, &uint_view)) + { + ERR("Failed to create buffer view.\n"); + return NULL; + } + + return uint_view; + } + + memset(&view_desc, 0, sizeof(view_desc)); + view_desc.view_type = view->info.texture.vk_view_type; + view_desc.format = uint_format; + view_desc.miplevel_idx = view->info.texture.miplevel_idx; + view_desc.miplevel_count = 1; + view_desc.layer_idx = view->info.texture.layer_idx; + view_desc.layer_count = view->info.texture.layer_count; + view_desc.vk_image_aspect = VK_IMAGE_ASPECT_COLOR_BIT; + view_desc.usage = VK_IMAGE_USAGE_STORAGE_BIT; + + if (!vkd3d_create_texture_view(device, VKD3D_DESCRIPTOR_MAGIC_UAV, + resource->u.vk_image, &view_desc, &uint_view)) + { + ERR("Failed to create image view.\n"); + return NULL; + } + + return uint_view; +} + static void STDMETHODCALLTYPE d3d12_command_list_ClearUnorderedAccessViewUint(ID3D12GraphicsCommandList5 *iface, D3D12_GPU_DESCRIPTOR_HANDLE gpu_handle, D3D12_CPU_DESCRIPTOR_HANDLE cpu_handle, ID3D12Resource *resource, const UINT values[4], UINT rect_count, const D3D12_RECT *rects) @@ -5441,8 +5487,6 @@ static void STDMETHODCALLTYPE d3d12_command_list_ClearUnorderedAccessViewUint(ID struct d3d12_command_list *list = impl_from_ID3D12GraphicsCommandList5(iface); struct vkd3d_view *descriptor, *uint_view = NULL; struct d3d12_device *device = list->device; - struct vkd3d_texture_view_desc view_desc; - const struct vkd3d_format *uint_format; const struct vkd3d_resource_view *view; struct d3d12_resource *resource_impl; VkClearColorValue colour; @@ -5456,44 +5500,11 @@ static void STDMETHODCALLTYPE d3d12_command_list_ClearUnorderedAccessViewUint(ID view = &descriptor->v; memcpy(colour.uint32, values, sizeof(colour.uint32)); - if (view->format->type != VKD3D_FORMAT_TYPE_UINT) + if (view->format->type != VKD3D_FORMAT_TYPE_UINT + && !(descriptor = uint_view = create_uint_view(device, view, resource_impl, &colour))) { - if (!(uint_format = vkd3d_find_uint_format(device, view->format->dxgi_format)) - && !(uint_format = vkd3d_fixup_clear_uav_uint_colour(device, view->format->dxgi_format, &colour))) - { - ERR("Unhandled format %#x.\n", view->format->dxgi_format); - return; - } - - if (d3d12_resource_is_buffer(resource_impl)) - { - if (!vkd3d_create_buffer_view(device, VKD3D_DESCRIPTOR_MAGIC_UAV, resource_impl->u.vk_buffer, - uint_format, view->info.buffer.offset, view->info.buffer.size, &uint_view)) - { - ERR("Failed to create buffer view.\n"); - return; - } - } - else - { - memset(&view_desc, 0, sizeof(view_desc)); - view_desc.view_type = view->info.texture.vk_view_type; - view_desc.format = uint_format; - view_desc.miplevel_idx = view->info.texture.miplevel_idx; - view_desc.miplevel_count = 1; - view_desc.layer_idx = view->info.texture.layer_idx; - view_desc.layer_count = view->info.texture.layer_count; - view_desc.vk_image_aspect = VK_IMAGE_ASPECT_COLOR_BIT; - view_desc.usage = VK_IMAGE_USAGE_STORAGE_BIT; - - if (!vkd3d_create_texture_view(device, VKD3D_DESCRIPTOR_MAGIC_UAV, resource_impl->u.vk_image, &view_desc, - &uint_view)) - { - ERR("Failed to create image view.\n"); - return; - } - } - descriptor = uint_view; + ERR("Failed to create UINT view.\n"); + return; } d3d12_command_list_clear_uav(list, resource_impl, descriptor, &colour, rect_count, rects); @@ -5507,19 +5518,32 @@ static void STDMETHODCALLTYPE d3d12_command_list_ClearUnorderedAccessViewFloat(I const float values[4], UINT rect_count, const D3D12_RECT *rects) { struct d3d12_command_list *list = impl_from_ID3D12GraphicsCommandList5(iface); + struct vkd3d_view *descriptor, *uint_view = NULL; + struct d3d12_device *device = list->device; + const struct vkd3d_resource_view *view; struct d3d12_resource *resource_impl; VkClearColorValue colour; - struct vkd3d_view *view; TRACE("iface %p, gpu_handle %s, cpu_handle %s, resource %p, values %p, rect_count %u, rects %p.\n", iface, debug_gpu_handle(gpu_handle), debug_cpu_handle(cpu_handle), resource, values, rect_count, rects); resource_impl = unsafe_impl_from_ID3D12Resource(resource); - if (!(view = d3d12_desc_from_cpu_handle(cpu_handle)->s.u.view)) + if (!(descriptor = d3d12_desc_from_cpu_handle(cpu_handle)->s.u.view)) return; + view = &descriptor->v; memcpy(colour.float32, values, sizeof(colour.float32)); - d3d12_command_list_clear_uav(list, resource_impl, view, &colour, rect_count, rects); + if (view->format->type == VKD3D_FORMAT_TYPE_SINT + && !(descriptor = uint_view = create_uint_view(device, view, resource_impl, &colour))) + { + ERR("Failed to create UINT view.\n"); + return; + } + + d3d12_command_list_clear_uav(list, resource_impl, descriptor, &colour, rect_count, rects); + + if (uint_view) + vkd3d_view_decref(uint_view, device); } static void STDMETHODCALLTYPE d3d12_command_list_DiscardResource(ID3D12GraphicsCommandList5 *iface,
added 106 commits
-
6476fffd...36c123c0 - 101 commits from branch
wine:master
- d1713d15 - tests: Mark some root signature unbounded range failures as todo on MoltenVK.
- 22282bc7 - vkd3d: Do not request VK_EXT_debug_marker if debug is disabled.
- d57b4a09 - vkd3d: Enable VK_KHR_portability_subset if available.
- 97f93002 - vkd3d: Do not synchronize with the geometry shader stage if it's not enabled.
- 2b5d5fb1 - tests: Clear UAV with the proper type.
Toggle commit list-
6476fffd...36c123c0 - 101 commits from branch
added 1 commit
- 040e7701 - vkd3d: Always se a UINT view to clear UAVs with integer formats.
added 42 commits
-
040e7701...da1d96f7 - 37 commits from branch
wine:master
- e2a3eb74 - tests: Mark some root signature unbounded range failures as todo on MoltenVK.
- 81fd8bbd - vkd3d: Do not request VK_EXT_debug_marker if debug is disabled.
- ce810b1e - vkd3d: Enable VK_KHR_portability_subset if available.
- d36a9939 - vkd3d: Do not synchronize with the geometry shader stage if it's not enabled.
- f7c1fe5e - vkd3d: Always se a UINT view to clear UAVs with integer formats.
Toggle commit list-
040e7701...da1d96f7 - 37 commits from branch
added 8 commits
-
f7c1fe5e...8a459f59 - 3 commits from branch
wine:master
- 3ad4d984 - tests: Mark some root signature unbounded range failures as todo on MoltenVK.
- 3254eff3 - vkd3d: Do not request VK_EXT_debug_marker if debug is disabled.
- a1a07de8 - vkd3d: Enable VK_KHR_portability_subset if available.
- a54187f3 - vkd3d: Do not synchronize with the geometry shader stage if it's not enabled.
- 90f26759 - vkd3d: Always use UINT views to clear UAVs with integer formats.
Toggle commit list-
f7c1fe5e...8a459f59 - 3 commits from branch