google / google/amber

Resource::MemoryBarrier() does not seem to provide the assumed host synchronization

Open
#1,121 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.