Re: [PATCH v3 3/3] wined3d: Implement UAV clears on the Vulkan backend.

[Henri Verbeet]

On Thu, 23 Sept 2021 at 10:43, Jan Sikorski  wrote:
> @@ -1166,6 +1173,7 @@ static const struct wined3d_shader_backend_ops spirv_shader_backend_vk =
>      .shader_get_caps = shader_spirv_get_caps,
>      .shader_color_fixup_supported = shader_spirv_color_fixup_supported,
>      .shader_has_ffp_proj_control = shader_spirv_has_ffp_proj_control,
> - .shader_compile = wined3d_spirv_compile,
>
> };

The name wined3d_spirv_compile() looks a bit out of place, I think. I
guess that's to avoid clashing with the existing
shader_spirv_compile(), but perhaps it makes sense to rename
shader_spirv_compile() to e.g. shader_spirv_compile_spirv(),
shader_spirv_compile_shader(), or some variant.
> @@ -5115,6 +5115,9 @@ BOOL wined3d_texture_vk_prepare_texture(struct wined3d_texture_vk *texture_vk,
>      if (wined3d_format_is_typeless(&format_vk->f) || texture_vk->t.swapchain)
>          flags |= VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT;
> - if (texture_vk->t.resource.bind_flags & WINED3D_BIND_UNORDERED_ACCESS)
> -
>    flags |= VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT;
> -

This could probably be done as a separate change, and I think it
deserves a comment that this is for creating views with a different
format for UAV clears. We may as well use "else if", I guess.
> +void wined3d_device_vk_uav_clear_state_init(struct wined3d_device_vk *device_vk)
> +{
> - struct wined3d_uav_clear_state_vk *state = &device_vk->uav_clear_state;
> - struct wined3d_context_vk *context_vk = &device_vk->context_vk;
> - VkDescriptorSetLayoutBinding vk_set_bindings[2];
> -
> - vk_set_bindings[1].binding = 0;
> - vk_set_bindings[1].descriptorCount = 1;
> - vk_set_bindings[1].stageFlags = VK_SHADER_STAGE_COMPUTE_BIT;
> - vk_set_bindings[1].pImmutableSamplers = NULL;
> - vk_set_bindings[1].descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER;
> -
> - vk_set_bindings[0].binding = 1;
> - vk_set_bindings[0].descriptorCount = 1;
> - vk_set_bindings[0].stageFlags = VK_SHADER_STAGE_COMPUTE_BIT;
> - vk_set_bindings[0].pImmutableSamplers = NULL;
> -
> - vk_set_bindings[0].descriptorType = VK_DESCRIPTOR_TYPE_STORAGE_IMAGE;
> - state->image_layout = wined3d_context_vk_get_pipeline_layout(context_vk, vk_set_bindings, 2);
> -
> - vk_set_bindings[0].descriptorType = VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER;
> - state->buffer_layout = wined3d_context_vk_get_pipeline_layout(context_vk, vk_set_bindings, 2);
> -

The initialisation order of "vk_set_bindings" seems a little odd. I
suppose it's ok.
> +#define SHADER_DESC(name) (struct wined3d_shader_desc){ name, sizeof(name) }
> - state->float_pipelines.buffer = create_uav_pipeline(context_vk, state->buffer_layout,
> -
>        SHADER_DESC(cs_uav_clear_buffer_float_code), WINED3D_SHADER_RESOURCE_BUFFER);
> - state->uint_pipelines.buffer = create_uav_pipeline(context_vk, state->buffer_layout,
> -
>        SHADER_DESC(cs_uav_clear_buffer_uint_code), WINED3D_SHADER_RESOURCE_BUFFER);
> - state->float_pipelines.image_1d = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_1d_float_code), WINED3D_SHADER_RESOURCE_TEXTURE_1D);
> - state->uint_pipelines.image_1d = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_1d_uint_code), WINED3D_SHADER_RESOURCE_TEXTURE_1D);
> - state->float_pipelines.image_1d_array = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_1d_array_float_code), WINED3D_SHADER_RESOURCE_TEXTURE_1DARRAY);
> - state->uint_pipelines.image_1d_array = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_1d_array_uint_code), WINED3D_SHADER_RESOURCE_TEXTURE_1DARRAY);
> - state->float_pipelines.image_2d = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_2d_float_code), WINED3D_SHADER_RESOURCE_TEXTURE_2D);
> - state->uint_pipelines.image_2d = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_2d_uint_code), WINED3D_SHADER_RESOURCE_TEXTURE_2D);
> - state->float_pipelines.image_2d_array = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_2d_array_float_code), WINED3D_SHADER_RESOURCE_TEXTURE_2DARRAY);
> - state->uint_pipelines.image_2d_array = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_2d_array_uint_code), WINED3D_SHADER_RESOURCE_TEXTURE_2DARRAY);
> - state->float_pipelines.image_3d = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_3d_float_code), WINED3D_SHADER_RESOURCE_TEXTURE_3D);
> - state->uint_pipelines.image_3d = create_uav_pipeline(context_vk, state->image_layout,
> -
>        SHADER_DESC(cs_uav_clear_3d_uint_code), WINED3D_SHADER_RESOURCE_TEXTURE_3D);
>
> +#undef SHADER_DESC
> -
> - state->buffer_group_size         = (struct wined3d_shader_thread_group_size){128, 1, 1};
> - state->image_1d_group_size       = (struct wined3d_shader_thread_group_size){ 64, 1, 1};
> - state->image_1d_array_group_size = (struct wined3d_shader_thread_group_size){ 64, 1, 1};
> - state->image_2d_group_size       = (struct wined3d_shader_thread_group_size){  8, 8, 1};
> - state->image_2d_array_group_size = (struct wined3d_shader_thread_group_size){  8, 8, 1};
> - state->image_3d_group_size       = (struct wined3d_shader_thread_group_size){  8, 8, 1};
>
> +}
> -

I'm not sure how we feel about compound literals in Wine these days. I
guess they're convenient here, but they don't seem terribly hard to
avoid either.
> - if (!fp)
> - {
> -
>    /* Make sure values are truncated, not saturated to some maximum value. */
> -
>    constants.color.uint32[0] &= ~(~0ull << view_format->red_size);
> -
>    constants.color.uint32[1] &= ~(~0ull << view_format->green_size);
> -
>    constants.color.uint32[2] &= ~(~0ull << view_format->blue_size);
> -
>    constants.color.uint32[3] &= ~(~0ull << view_format->alpha_size);
> -

That's wined3d_mask_from_size(), essentially.