[PATCH vkd3d 4/4] tests: Introduce a Vulkan shader runner.

[Zebediah Figura]

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 at 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.