Conversation
Descriptors were using separate transfer buffers when they were using same buffer. This prevented e.g. using the same buffer in multiple storage buffer bindings. This commit moves the transfer buffer/image ownership from descriptors to pipeline to allow descriptors to share the transfer buffers. Now the descriptors can have the same buffer binding even if the descriptors are of different types e.g. storage/uniform/storage_texel_buffer etc. Fixes google#950
src/vulkan/buffer_descriptor.cc
Outdated
| return Result("Unexpected buffer type when deciding usage flags"); | ||
| auto& transfer_buffer = transfer_resources[amber_buffer]; | ||
| // Unset transfer buffer's read only property if needed. | ||
| if (!IsReadOnly() && transfer_resources[amber_buffer]->IsReadOnly()) { |
There was a problem hiding this comment.
Does the SetReadOnly do something else than just set a member variable to true or false? If not, could simply do SetReadOnly(IsReadOnly()) outside these blocks.
There was a problem hiding this comment.
I simplified this a little bit. The transfer buffer's readOnly property should never be changed from false to true, because some descriptors may need only read access, but some other may need write access too.
src/vulkan/pipeline.cc
Outdated
| } | ||
| } | ||
|
|
||
| // Initialize transfer buffers. Transfer images are already initialized in |
There was a problem hiding this comment.
Why do we need to initialize them in different places?
There was a problem hiding this comment.
Transfer buffers needed to be initialized in later phase, because the buffer usage flags could change while looping through the descriptors in the loop above. Both transfer buffers and images are now initialized here to make the code more readable.
Simplify transfer buffer's readOnly property logic.
paulthomson
left a comment
paulthomson
left a comment
There was a problem hiding this comment.
Nice! One comment. And please rebase and/or merge from main.
src/vulkan/transfer_buffer.h
Outdated
|
|
||
| Result Initialize(const VkBufferUsageFlags usage); | ||
| TransferBuffer* AsTransferBuffer() override { return this; } | ||
| void AddUsageFlags(VkBufferUsageFlags flags) { usage_flags_ |= flags; } |
There was a problem hiding this comment.
Should we assert that buffer_ is VK_NULL_HANDLE? I.e. you cannot add usage flags after Initialize?
There was a problem hiding this comment.
Good point. I'll add that
Verify that the transfer buffer is not initialized when adding buffer usage flags.
3 participants
Descriptors were using separate transfer buffers when they were
using same buffer. This prevented e.g. using the same buffer in
multiple storage buffer bindings.
This commit moves the transfer buffer/image ownership from descriptors to
pipeline to allow descriptors to share the transfer buffers. Now the
descriptors can have the same buffer binding even if the descriptors
are of different types e.g. storage/uniform/storage_texel_buffer etc.
Fixes #950