[PATCH vkd3d 2/3] vkd3d-shader: Introduce a compile option to use Unknown format for typed UAV reads.
Specifying R32ui for UAVs created with a vector format, e.g. R32G32B32A32_FLOAT, results in only the red being loaded/stored, potentially causing images to contain only the red component.
Signed-off-by: Conor McCarthy cmccarthy@codeweavers.com --- include/vkd3d_shader.h | 18 ++++++++++++++++++ libs/vkd3d-shader/spirv.c | 16 +++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index ebddbba7..4175223f 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -99,6 +99,22 @@ enum vkd3d_shader_compile_option_buffer_uav VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_COMPILE_OPTION_BUFFER_UAV), };
+/** + * Determines how typed UAVs are declared. + */ +enum vkd3d_shader_compile_option_typed_uav +{ + /** Use R32ui format for UAVs which are read from. This is the default value. */ + VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV_READ_FORMAT_R32UI = 0x00000000, + /** + * Use Unknown format for UAVs which are read from. This should only be set if + * shaderStorageImageReadWithoutFormat is enabled in the target environment. + */ + VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV_READ_FORMAT_UNKNOWN = 0x00000001, + + VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV), +}; + enum vkd3d_shader_compile_option_formatting_flags { VKD3D_SHADER_COMPILE_OPTION_FORMATTING_NONE = 0x00000000, @@ -127,6 +143,8 @@ enum vkd3d_shader_compile_option_name VKD3D_SHADER_COMPILE_OPTION_FORMATTING = 0x00000003, /** \a value is a member of enum vkd3d_shader_api_version. \since 1.3 */ VKD3D_SHADER_COMPILE_OPTION_API_VERSION = 0x00000004, + /** \a value is a member of enum vkd3d_shader_compile_option_typed_uav. \since 1.5 */ + VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV = 0x00000005,
VKD3D_FORCE_32_BIT_ENUM(VKD3D_SHADER_COMPILE_OPTION_NAME), }; diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 776533cb..6450f818 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2228,6 +2228,7 @@ struct vkd3d_dxbc_compiler
bool strip_debug; bool ssbo_uavs; + bool uav_read_without_format;
struct rb_tree symbol_table; uint32_t temp_id; @@ -2379,6 +2380,15 @@ struct vkd3d_dxbc_compiler *vkd3d_dxbc_compiler_create(const struct vkd3d_shader
case VKD3D_SHADER_COMPILE_OPTION_API_VERSION: break; + + case VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV: + if (option->value == VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV_READ_FORMAT_R32UI) + compiler->uav_read_without_format = false; + else if (option->value == VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV_READ_FORMAT_UNKNOWN) + compiler->uav_read_without_format = true; + else + WARN("Ignoring unrecognised value %#x for option %#x.\n", option->value, option->name); + break; } }
@@ -5856,14 +5866,18 @@ static uint32_t vkd3d_dxbc_compiler_get_image_type_id(struct vkd3d_dxbc_compiler const struct vkd3d_shader_descriptor_info *d; uint32_t sampled_type_id; SpvImageFormat format; + bool uav_read;
format = SpvImageFormatUnknown; if (reg->type == VKD3DSPR_UAV) { d = vkd3d_dxbc_compiler_get_descriptor_info(compiler, VKD3D_SHADER_DESCRIPTOR_TYPE_UAV, range); - if (raw_structured || (d->flags & VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_UAV_READ)) + uav_read = !!(d->flags & VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_UAV_READ); + if (raw_structured || (uav_read && !compiler->uav_read_without_format)) format = image_format_for_image_read(data_type); + else if (uav_read) + vkd3d_spirv_enable_capability(builder, SpvCapabilityStorageImageReadWithoutFormat); }
sampled_type_id = vkd3d_spirv_get_type_id(builder, data_type, 1);
Fixes reflections in Control appearing with only the red component.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52146 Signed-off-by: Conor McCarthy cmccarthy@codeweavers.com --- libs/vkd3d/device.c | 2 ++ libs/vkd3d/state.c | 17 +++++++++++++---- libs/vkd3d/vkd3d_private.h | 2 ++ tests/d3d12.c | 2 +- 4 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/libs/vkd3d/device.c b/libs/vkd3d/device.c index eaedc444..983471b7 100644 --- a/libs/vkd3d/device.c +++ b/libs/vkd3d/device.c @@ -1310,6 +1310,7 @@ static void vkd3d_init_feature_level(struct vkd3d_vulkan_info *vk_info, CHECK_FEATURE(shaderClipDistance); CHECK_FEATURE(shaderCullDistance); CHECK_FEATURE(shaderImageGatherExtended); + CHECK_FEATURE(shaderStorageImageReadWithoutFormat); CHECK_FEATURE(shaderStorageImageWriteWithoutFormat); CHECK_FEATURE(tessellationShader);
@@ -1425,6 +1426,7 @@ static HRESULT vkd3d_init_device_caps(struct d3d12_device *device, vulkan_info->sparse_properties = physical_device_info->properties2.properties.sparseProperties; vulkan_info->rasterization_stream = physical_device_info->xfb_properties.transformFeedbackRasterizationStreamSelect; vulkan_info->transform_feedback_queries = physical_device_info->xfb_properties.transformFeedbackQueries; + vulkan_info->uav_read_without_format = features->shaderStorageImageReadWithoutFormat; vulkan_info->max_vertex_attrib_divisor = max(physical_device_info->vertex_divisor_properties.maxVertexAttribDivisor, 1);
device->feature_options.DoublePrecisionFloatShaderOps = features->shaderFloat64; diff --git a/libs/vkd3d/state.c b/libs/vkd3d/state.c index 7a29ade8..212fe579 100644 --- a/libs/vkd3d/state.c +++ b/libs/vkd3d/state.c @@ -1944,6 +1944,13 @@ struct d3d12_pipeline_state *unsafe_impl_from_ID3D12PipelineState(ID3D12Pipeline return impl_from_ID3D12PipelineState(iface); }
+static inline unsigned int typed_uav_compile_option(const struct d3d12_device *device) +{ + return device->vk_info.uav_read_without_format + ? VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV_READ_FORMAT_UNKNOWN + : VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV_READ_FORMAT_R32UI; +} + static HRESULT create_shader_stage(struct d3d12_device *device, struct VkPipelineShaderStageCreateInfo *stage_desc, enum VkShaderStageFlagBits stage, const D3D12_SHADER_BYTECODE *code, const struct vkd3d_shader_interface_info *shader_interface) @@ -1955,9 +1962,10 @@ static HRESULT create_shader_stage(struct d3d12_device *device, VkResult vr; int ret;
- static const struct vkd3d_shader_compile_option options[] = + const struct vkd3d_shader_compile_option options[] = { {VKD3D_SHADER_COMPILE_OPTION_API_VERSION, VKD3D_SHADER_API_VERSION_1_4}, + {VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV, typed_uav_compile_option(device)}, };
stage_desc->sType = VK_STRUCTURE_TYPE_PIPELINE_SHADER_STAGE_CREATE_INFO; @@ -2001,14 +2009,15 @@ static HRESULT create_shader_stage(struct d3d12_device *device, return S_OK; }
-static int vkd3d_scan_dxbc(const D3D12_SHADER_BYTECODE *code, +static int vkd3d_scan_dxbc(const struct d3d12_device *device, const D3D12_SHADER_BYTECODE *code, struct vkd3d_shader_scan_descriptor_info *descriptor_info) { struct vkd3d_shader_compile_info compile_info;
- static const struct vkd3d_shader_compile_option options[] = + const struct vkd3d_shader_compile_option options[] = { {VKD3D_SHADER_COMPILE_OPTION_API_VERSION, VKD3D_SHADER_API_VERSION_1_4}, + {VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV, typed_uav_compile_option(device)}, };
compile_info.type = VKD3D_SHADER_STRUCTURE_TYPE_COMPILE_INFO; @@ -2170,7 +2179,7 @@ static HRESULT d3d12_pipeline_state_find_and_init_uav_counters(struct d3d12_pipe
shader_info.type = VKD3D_SHADER_STRUCTURE_TYPE_SCAN_DESCRIPTOR_INFO; shader_info.next = NULL; - if ((ret = vkd3d_scan_dxbc(code, &shader_info)) < 0) + if ((ret = vkd3d_scan_dxbc(device, code, &shader_info)) < 0) { WARN("Failed to scan shader bytecode, stage %#x, vkd3d result %d.\n", stage_flags, ret); return hresult_from_vkd3d_result(ret); diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h index f00181a2..9976fe58 100644 --- a/libs/vkd3d/vkd3d_private.h +++ b/libs/vkd3d/vkd3d_private.h @@ -143,6 +143,8 @@ struct vkd3d_vulkan_info bool rasterization_stream; bool transform_feedback_queries;
+ bool uav_read_without_format; + bool vertex_attrib_zero_divisor; unsigned int max_vertex_attrib_divisor;
diff --git a/tests/d3d12.c b/tests/d3d12.c index 57e072e4..c59333f8 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -21093,7 +21093,7 @@ static void test_typed_buffer_uav(void) D3D12_RESOURCE_STATE_UNORDERED_ACCESS, D3D12_RESOURCE_STATE_COPY_SOURCE);
get_buffer_readback_with_command_list(resource, uav_desc.Format, &rb, queue, command_list); - todo check_readback_data_vec4(&rb.rb, NULL, &expected, 0); + check_readback_data_vec4(&rb.rb, NULL, &expected, 0); release_resource_readback(&rb);
ID3D12Resource_Release(resource);
On Mon, 11 Jul 2022 at 16:28, Conor McCarthy cmccarthy@codeweavers.com wrote:
@@ -1310,6 +1310,7 @@ static void vkd3d_init_feature_level(struct vkd3d_vulkan_info *vk_info, CHECK_FEATURE(shaderClipDistance); CHECK_FEATURE(shaderCullDistance); CHECK_FEATURE(shaderImageGatherExtended);
- CHECK_FEATURE(shaderStorageImageReadWithoutFormat); CHECK_FEATURE(shaderStorageImageWriteWithoutFormat); CHECK_FEATURE(tessellationShader);
I don't think that should be required for feature level 11.0?
On Thu, Jul 21, 2022 at 2:45 AM Henri Verbeet hverbeet@gmail.com wrote:
I don't think that should be required for feature level 11.0?
The Microsoft doc states, "all D3D11.3 and D3D12 hardware," and I don't think it's a reference to feature level. There's no feature level 11.3 for D3D12.
On Thu, 21 Jul 2022 at 16:10, Conor McCarthy conor.mccarthy.444@gmail.com wrote:
On Thu, Jul 21, 2022 at 2:45 AM Henri Verbeet hverbeet@gmail.com wrote:
I don't think that should be required for feature level 11.0?
The Microsoft doc states, "all D3D11.3 and D3D12 hardware," and I don't think it's a reference to feature level. There's no feature level 11.3 for D3D12.
I don't know which specific documentation you're referencing, but I'd argue there's a reason D3D12_FEATURE_DATA_D3D12_OPTIONS exists; if feature level 11_0 (i.e., the minimum for Direct3D 12) required TypedUAVLoadAdditionalFormats to be TRUE, there would be no point in having that field. Also, [1] suggests only the original 3 formats are required for 11_0 and 11_1.
[1] https://docs.microsoft.com/en-us/windows/win32/direct3d12/hardware-feature-l...
On Mon, 11 Jul 2022 at 16:28, Conor McCarthy cmccarthy@codeweavers.com wrote:
+enum vkd3d_shader_compile_option_typed_uav +{
- /** Use R32ui format for UAVs which are read from. This is the default value. */
- VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV_READ_FORMAT_R32UI = 0x00000000,
That's not quite what the code does though, is it? image_format_for_image_read() will use the declared resource type to determine the SPIR-V type. For VKD3D_SHADER_COMPONENT_UINT that's SpvImageFormatR32ui, but for e.g. VKD3D_SHADER_COMPONENT_FLOAT it would be SpvImageFormatR32f.
Where this breaks down is component counts; image_format_for_image_read() assumes single component formats, in part because that's all that was originally required. These additional multi-component formats are an optional feature, see also "TypedUAVLoadAdditionalFormats" from D3D12_FEATURE_DATA_D3D12_OPTIONS.
So what this compile option then comes down to is using either the declared UAV type, or SpvImageFormatUnknown. And when not using SpvImageFormatUnknown, there's arguably still a bug of ignoring component counts. We could opt to not fix that bug, or fail shader compilation for those formats, but it doesn't seem terribly hard to fix, and I don't think there are that many additional formats anyway.
On Thu, Jul 21, 2022 at 2:45 AM Henri Verbeet hverbeet@gmail.com wrote:
Where this breaks down is component counts; image_format_for_image_read() assumes single component formats, in part because that's all that was originally required. These additional multi-component formats are an optional feature, see also "TypedUAVLoadAdditionalFormats" from D3D12_FEATURE_DATA_D3D12_OPTIONS.
TypedUAVLoadAdditionalFormats refers to the "if any one is supported, all are supported" set in: https://docs.microsoft.com/en-us/windows/win32/direct3d12/typed-unordered-ac...
These are all vec4 formats. We currently don't set D3D12_FORMAT_SUPPORT2_UAV_TYPED_LOAD for any formats, so the formats with component count < 4 are not supported. Would that be a separate patch series?
On Thu, 21 Jul 2022 at 15:57, Conor McCarthy conor.mccarthy.444@gmail.com wrote:
These are all vec4 formats. We currently don't set D3D12_FORMAT_SUPPORT2_UAV_TYPED_LOAD for any formats, so the formats with component count < 4 are not supported. Would that be a separate patch series?
Sure. Note that we currently set "device->feature_options.TypedUAVLoadAdditionalFormats" based on "features->shaderStorageImageExtendedFormats". That wouldn't necessarily be wrong (if we generated the correct SPIR-V at least...), but it's a somewhat stricter requirement than necessary.
On Thu, Jul 21, 2022 at 2:45 AM Henri Verbeet hverbeet@gmail.com wrote:
So what this compile option then comes down to is using either the declared UAV type, or SpvImageFormatUnknown. And when not using SpvImageFormatUnknown, there's arguably still a bug of ignoring component counts. We could opt to not fix that bug, or fail shader compilation for those formats, but it doesn't seem terribly hard to fix, and I don't think there are that many additional formats anyway.
What do you propose to do for component counts? UAVs with < 4 components are still declared in DXBC as vec4, and loads declare e.g. u1.xyzw as the source. Information about the actual component count seems to be missing. It is stored in DXIL, so maybe it can be used there, but not for SM < 6.
On Fri, 22 Jul 2022 at 11:58, Conor McCarthy conor.mccarthy.444@gmail.com wrote:
On Thu, Jul 21, 2022 at 2:45 AM Henri Verbeet hverbeet@gmail.com wrote:
So what this compile option then comes down to is using either the declared UAV type, or SpvImageFormatUnknown. And when not using SpvImageFormatUnknown, there's arguably still a bug of ignoring component counts. We could opt to not fix that bug, or fail shader compilation for those formats, but it doesn't seem terribly hard to fix, and I don't think there are that many additional formats anyway.
What do you propose to do for component counts? UAVs with < 4 components are still declared in DXBC as vec4, and loads declare e.g. u1.xyzw as the source. Information about the actual component count seems to be missing. It is stored in DXIL, so maybe it can be used there, but not for SM < 6.
I'm not entirely sure we're decoding these correctly. The format is certainly specified in the HLSL; it would seem surprising (though not inconceivable) that the information just gets dropped when generating the bytecode.
On Fri, Jul 22, 2022 at 8:28 PM Henri Verbeet hverbeet@gmail.com wrote:
I'm not entirely sure we're decoding these correctly. The format is certainly specified in the HLSL; it would seem surprising (though not inconceivable) that the information just gets dropped when generating the bytecode.
It appears from the spec we are not missing anything. The obvious way to encode a format component count is in the dword which specifies the type of each component. Those beyond the last component could be D3D11_SB_RETURN_TYPE_UNUSED, but instead they all have the same type. There's no other place to store it. The output from /dumpbin has four components for the declaration too, and the load instruction specifies a vec4 load. Looks like the actual format component count is left out.
-
Conor McCarthy -
Conor McCarthy -
Henri Verbeet