Tobski/simple_vulkan_synchronization

Wrong dstStage

Opened this issue · 7 comments

jkoenen commented

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

Tobski commented

Hi @jkoenen, while a layout transition might actually perform a write in reality, from the spec POV it does not. If an implementation does access an image as part of a layout transition, it is responsible for doing any required cache flushes - the user does not have to do anything, which is why no access masks are specified. There's no issue with the code here.

The example you've quoted does the same as this test is expecting.

jkoenen commented

I'm not sure I follow - I did get a validation error with your code and in the example the dstAccessMask is VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT instead of 0)

Tobski commented

Oh my apologies, I misread the example - been too long since I've looked at this clearly 😬

Let me take another look at this..

Tobski commented

Okay now I'm very confused. I disagree with the comments I made in the examples (I wrote those too!).

So two things - firstly, as a rule of thumb, you only need write access flags in the source and read access flags in the destination - write access in the destination is more or less meaningless, so I'm kind of confused as to why I suggested otherwise.
Secondly, layout transitions do behave as I stated in my first comment:

Available memory is automatically made visible to a layout transition, and writes performed by a layout transition are automatically made available.

(From the spec under "Image Layout Transitions").

So as far as I can tell, the example and the validation layer are both wrong here. The 2:1 ratio would make me doubt that usually, but given validation used the examples as source documentation I'm not putting too much stock in it 🙃

Could you raise an issue against the validation layers with details of what you're doing? I'll leave this open until we resolve it...

I'll also sort out the sync examples once we figure this out, so no need to raise a separate issue for that.

Tobski commented

Ack, hold up, this problem might run deeper than I thought, don't raise a validation issue yet (If you have nearly written it out feel free to post it though) - but I may need to go get some clarification on the spec here...

jkoenen commented

This section in the spec is probably relevant:

Applications must ensure that layout transitions happen-after all operations accessing the image with the old layout, and happen-before any operations that will access the image with the new layout. Layout transitions are potentially read/write operations, so not defining appropriate memory dependencies to guarantee this will result in a data race.

I think the issue is that the layout transition only makes the memory available and not visible?

Tobski commented

I think the issue is that the layout transition only makes the memory available and not visible?

Yes, the issue is that writes are only supposed to need previous writes available to avoid a data race. However, it seems the spec has been a bit loose on this, and VVL (and my example code) disagrees with what we'd intended. I've raised a spec bug against this internally and we'll see where we land with it... going to have to put a pin in this until that is sorted, because I don't want to change it and then revert it.

(Btw thank you for raising this, it's something I wish we had caught sooner!)

  • 👍1