On 4/12/22 06:16, Henri Verbeet wrote:
I only did a cursory review, with the idea that for something like this, unless there's something especially egregious, it's fine to fix things up after the fact. A couple of things did stand out though:
On Mon, 11 Apr 2022 at 19:35, Zebediah Figura zfigura@codeweavers.com wrote:
diff --git a/tests/shader_runner.c b/tests/shader_runner.c index bf3bc0d85..54b667fc7 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -856,6 +856,8 @@ START_TEST(shader_runner) #ifdef _WIN32 run_shader_tests_d3d9(argc, argv); run_shader_tests_d3d11(argc, argv); +#else
- run_shader_tests_vulkan(argc, argv); #endif run_shader_tests_d3d12(argc, argv); }
I theory, Windows has Vulkan too.
Indeed. The intention here is to avoid building crosstests, but that's the wrong way to check for it...
Now that we have multiple shader runners, the logs can become less than clear; vkd3d_test_check_ok() uses "vkd3d_test_name" as prefix, but that's "shader_runner" for all of these. We may want to consider setting that to e.g. "shader_runner_d3d12" for the d3d12 runner, and so on.
Modifying vkd3d_test_name sounds like a good idea.
+static void transition_image_layout(struct vulkan_shader_runner *runner,
VkImage image, VkImageLayout src_layout, VkImageLayout dst_layout)+{
- VkImageMemoryBarrier barrier = {.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER};
- barrier.srcAccessMask = VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_MEMORY_WRITE_BIT;
- barrier.dstAccessMask = VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_MEMORY_WRITE_BIT;
- barrier.oldLayout = src_layout;
- barrier.newLayout = dst_layout;
- barrier.srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
- barrier.dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED;
- barrier.image = image;
- barrier.subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT;
- barrier.subresourceRange.baseMipLevel = 0;
- barrier.subresourceRange.levelCount = 1;
- barrier.subresourceRange.baseArrayLayer = 0;
- barrier.subresourceRange.layerCount = 1;
- VK_CALL(vkCmdPipelineBarrier(runner->cmd_buffer, VK_PIPELINE_STAGE_ALL_COMMANDS_BIT,
VK_PIPELINE_STAGE_ALL_COMMANDS_BIT, 0, 0, NULL, 0, NULL, 1, &barrier));+}
That function is a fair bit less generic than its name might suggest.
I suppose so, although in what respect do you mean specifically?
+static bool compile_shader(const struct vulkan_shader_runner *runner, const char *source, const char *type,
struct vkd3d_shader_code *dxbc, struct vkd3d_shader_code *spirv)+{
[...]
- info.next = &hlsl_info;
- info.source.code = source;
- info.source.size = strlen(source);
- info.source_type = VKD3D_SHADER_SOURCE_HLSL;
- info.target_type = VKD3D_SHADER_TARGET_DXBC_TPF;
- info.log_level = VKD3D_SHADER_LOG_WARNING;
[...]
- info.next = &spirv_info;
- info.source = *dxbc;
- info.source_type = VKD3D_SHADER_SOURCE_DXBC_TPF;
- info.target_type = VKD3D_SHADER_TARGET_SPIRV_BINARY;
It should be relatively straightforward for vkd3d-shader to support compiling HLSL to SPIR-V. There may be a case for a more direct path, but for a start, compile_hlsl() could essentially just do what we're doing here.
Yes, although in this case we need to reflect the DXBC anyway, in order to retrieve vertex attribute bindings.
+static VkDescriptorSetLayout create_descriptor_set_layout(struct vulkan_shader_runner *runner) +{
[...]
- for (i = 0; i < runner->r.sampler_count; ++i)
- {
VkSamplerCreateInfo sampler_desc = {.sType = VK_STRUCTURE_TYPE_SAMPLER_CREATE_INFO};struct vulkan_sampler *vulkan_sampler = &runner->samplers[i];const struct sampler *sampler = &runner->r.samplers[i];sampler_desc.magFilter = (sampler->filter & 0x4) ? VK_FILTER_LINEAR : VK_FILTER_NEAREST;sampler_desc.minFilter = (sampler->filter & 0x1) ? VK_FILTER_LINEAR : VK_FILTER_NEAREST;sampler_desc.mipmapMode = (sampler->filter & 0x10) ? VK_SAMPLER_MIPMAP_MODE_LINEAR : VK_SAMPLER_MIPMAP_MODE_NEAREST;sampler_desc.addressModeU = vk_address_mode_from_d3d12(sampler->u_address);sampler_desc.addressModeV = vk_address_mode_from_d3d12(sampler->v_address);sampler_desc.addressModeW = vk_address_mode_from_d3d12(sampler->w_address);We may want to consider storing the filter modes separately to begin with, but if we're going to store these as D3D12_FILTER values, it would make sense to use the appropriate macros like D3D12_DECODE_MAG_FILTER to decode them as well.
Sure, that makes sense.
+static void bind_resources(struct vulkan_shader_runner *runner, VkPipelineBindPoint bind_point,
VkDescriptorSetLayout set_layout, VkPipelineLayout pipeline_layout)+{
[...]
- if (runner->r.uniform_count)
VK_CALL(vkCmdPushConstants(cmd_buffer, pipeline_layout, VK_SHADER_STAGE_ALL, 0,runner->r.uniform_count * sizeof(*runner->r.uniforms), runner->r.uniforms));Note that the number of available push constants is typically somewhat limited. A value of 128 (= 32 * float = 8 * vec4) for maxPushConstantsSize is not uncommon. That's probably sufficient for our purposes, but nevertheless something to be aware of. It's certainly a lot less than e.g. d3d9's 256 vec4 constants.
I'll add a check to make sure we don't exceed the limits.