tobski / simple_vulkan_synchronization Goto Github PK

I find the ThsvsAccessType is absolutely a great way to encapsulate synchronizations, but on top of the current design, sometimes we would need a little more fine-grained control, e.g. translate a set of access types to essentially, thsvsVkAccessInfo.

For instance, the current interface becomes inadequate when we try to incorporate subpass dependencies into the system. Say each render pass attachment is given two sets of access types (begin/end), and we'd want to map these to both initial/final layouts and external subpass dependencies.

With something like thsvsGetAccessInfo we can get all the flexibility we'd need for any other synchronization primitives that are not yet fully covered here in the library.

Hey,

To be more efficient, you need to change this:

typedef struct ThsvsVkAccessInfo {
    VkPipelineStageFlags    stageMask;
    VkAccessFlags           accessMask;
    VkImageLayout           imageLayout;
} ThsvsVkAccessInfo;

into something like this:

typedef struct ThsvsVkAccessInfo {
    VkPipelineStageFlags    stageMask;
    VkAccessFlags           dstAccessMask;
    VkAccessFlags           srcAccessMask;
    VkImageLayout           imageLayout;
} ThsvsVkAccessInfo;

For example, for THSVS_ACCESS_COLOR_ATTACHMENT_READ_WRITE, you need to use VK_ACCESS_COLOR_ATTACHMENT_READ_BIT | VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT as the dstAccessMask when transitioning into the state, but when transitioning out, you just need VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT for srcAccessMask, as WAR hazards are satisfied with execution barriers alone.

(In short, srcAccessMask never needs a read access).

While porting this library to another language: https://github.com/jayrulez/Bulkan/blob/master/Bulkan.Utilities/src/SimpleVulkanSynchronization.bf

I came across 3 enum names that can be updated to the standard KHR variants:

VK_PIPELINE_STAGE_SHADING_RATE_IMAGE_BIT_NV => VK_PIPELINE_STAGE_FRAGMENT_SHADING_RATE_ATTACHMENT_BIT_KHR
VK_ACCESS_SHADING_RATE_IMAGE_READ_BIT_NV =>	VK_ACCESS_FRAGMENT_SHADING_RATE_ATTACHMENT_READ_BIT_KHR
VK_IMAGE_LAYOUT_SHADING_RATE_OPTIMAL_NV => VK_IMAGE_LAYOUT_FRAGMENT_SHADING_RATE_ATTACHMENT_OPTIMAL_KHR

The following test seems to be wrong because the image layout transition is counted as a write operation and therefore it's required to specify VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT in the dstAccessMask.

    image_barrier_test("Graphics fragment read from sampled image, Graphics write to color attachment",
                        THSVS_ACCESS_FRAGMENT_SHADER_READ_SAMPLED_IMAGE_OR_UNIFORM_TEXEL_BUFFER,                        
                        THSVS_ACCESS_COLOR_ATTACHMENT_WRITE,
                        VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT,
                        VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT,
                        0,
                        0,
                        VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL,
                        VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL);

The problem might be in thsvsGetVulkanImageMemoryBarrier in this block:

        if (pVkBarrier->srcAccessMask != 0)
            pVkBarrier->dstAccessMask |= pNextAccessInfo->accessMask;

I think this code should run after determining the layout to see if a layout transition is specified and then set the dstaccessMask even when the srcAccessMask is 0.

For reference here is this exact synchronization example:

https://github.com/KhronosGroup/Vulkan-Docs/wiki/Synchronization-Examples-(Legacy-synchronization-APIs)#first-draw-samples-a-texture-in-the-fragment-shader-second-draw-writes-to-that-texture-as-a-color-attachment

VK_PIPELINE_STAGE_COMMAND_PROCESS_BIT_NVX, VK_ACCESS_COMMAND_PROCESS_READ_BIT_NVX, and VK_ACCESS_COMMAND_PROCESS_WRITE_BIT_NVX are no longer defined in this week's Vulkan SDK. I naively assume they've morphed into *_PREPROCESS_*_NV but what do I know, I'm way out of the loop these days :)

Currently we assert that pPrevAccesses and pNextAccesses can have at most one write access for each global barrier. Imagine a scenario like this:

1. RayTracing shader writes to buffer A
2. Compute shader writes to buffer B
3. Pipeline barrier
4. Compute shader reads from buffer A
5. RayTracing shader reads from buffer B

This should be a valid case, but the assertion is asking us to make it "appear on its own."
However, we only take one global memory barrier parameter in cmd_pipeline_barrier:

@Tobski Can you clarify what we could do in this case? Thanks.