Resource::MemoryBarrier() does not seem to provide the assumed host synchronization
- Dominant language
- C++
- Stars
- 228
- Forks
- 77
- PR merge metrics
- No merged PRs in 30d
Description
Amber appears to rely on [Resource::MemoryBarrier()](https://github.com/google/amber/blob/a76118c1c79ae5615e6684bc3fa267e20c14baf2/src/vulkan/resource.cc#L194) to make device writes visible before reading mapped memory on the host.
For example [TransferImage::CopyToHost()](https://github.com/google/amber/blob/a76118c1c79ae5615e6684bc3fa267e20c14baf2/src/vulkan/transfer_image.cc#L285) calls it after vkCmdCopyImageToBuffer(), before the copied data is read through the mapped staging buffer.
Two recent PRs have made changes to MemoryBarrier: PR #1095 and PR #1097.
Before PR #1095, [kMemoryBarrierForAll](https://github.com/google/amber/blob/a76118c1c79ae5615e6684bc3fa267e20c14baf2/src/vulkan/resource.cc#L28) included VK_ACCESS_HOST_READ_BIT and VK_ACCESS_HOST_WRITE_BIT, but Resource::MemoryBarrier() did not specify VK_PIPELINE_STAGE_HOST_BIT.
This likely produced the VVL errors cited by PR #1097:
[VUID-vkCmdPipelineBarrier-srcAccessMask-02815](https://docs.vulkan.org/refpages/latest/refpages/source/vkCmdPipelineBarrier.html#VUID-vkCmdPipelineBarrier-srcAccessMask-02815)
[VUID-vkCmdPipelineBarrier-dstAccessMask-02816](https://docs.vulkan.org/refpages/latest/refpages/source/vkCmdPipelineBarrier.html#VUID-vkCmdPipelineBarrier-dstAccessMask-02816)
1. PR #1095 addressed the VVL errors by adding VK_PIPELINE_STAGE_HOST_BIT, which achieves the assumed host synchronization of MemoryBarrier().
2. PR #1097 addressed the VVL errors by removing VK_ACCESS_HOST_[READ|WRITE]_BIT, which does not achieve the assumed host synchronization.
For some reason the change from PR #1095 does not seem to have been present at the time of PR #1097.
I believe PR #1095 resolved the VVL errors in the desired way, since it seems users of MemoryBarrier assume the mentioned host synchronization.
Contributor guide
Research direction
Start by reading src/vulkan/resource.cc, especially Resource::MemoryBarrier() and kMemoryBarrierForAll, then inspect src/vulkan/transfer_image.cc at TransferImage::CopyToHost(). Compare the changes in PR #1095 and PR #1097 and review the cited Vulkan validation errors. Done means the chosen barrier behavior provides the host synchronization callers assume without producing those validation errors.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100