Skip to content

Commit

Permalink
Use shared transfer buffers (#951)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@ilkkasaa
ilkkasaa authored Jun 18, 2021
1 parent aaac58d commit 8c3bfef
Show file tree
Hide file tree
Showing 18 changed files with 370 additions and 265 deletions.
92 changes: 39 additions & 53 deletions src/vulkan/buffer_backed_descriptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,79 +26,65 @@ BufferBackedDescriptor::BufferBackedDescriptor(Buffer* buffer,
DescriptorType type,
Device* device,
uint32_t desc_set,
uint32_t binding)
: Descriptor(type, device, desc_set, binding) {
uint32_t binding,
Pipeline* pipeline)
: Descriptor(type, device, desc_set, binding), pipeline_(pipeline) {
AddAmberBuffer(buffer);
}

BufferBackedDescriptor::~BufferBackedDescriptor() = default;

Result BufferBackedDescriptor::RecordCopyDataToResourceIfNeeded(
CommandBuffer* command) {
for (const auto& resource : GetResources()) {
if (!resource.first->ValuePtr()->empty()) {
resource.second->UpdateMemoryWithRawData(*resource.first->ValuePtr());
// If the resource is read-only, keep the buffer data; Amber won't copy
// read-only resources back into the host buffers, so it makes sense to
// leave the buffer intact.
if (!IsReadOnly())
resource.first->ValuePtr()->clear();
}

resource.second->CopyToDevice(command);
}
Result BufferBackedDescriptor::RecordCopyBufferDataToTransferResourceIfNeeded(
CommandBuffer* command_buffer,
Buffer* buffer,
Resource* transfer_resource) {
transfer_resource->UpdateMemoryWithRawData(*buffer->ValuePtr());
// If the resource is read-only, keep the buffer data; Amber won't copy
// read-only resources back into the host buffers, so it makes sense to
// leave the buffer intact.
if (!transfer_resource->IsReadOnly())
buffer->ValuePtr()->clear();

transfer_resource->CopyToDevice(command_buffer);
return {};
}

Result BufferBackedDescriptor::RecordCopyDataToHost(CommandBuffer* command) {
if (!IsReadOnly()) {
if (GetResources().empty()) {
return Result(
"Vulkan: BufferBackedDescriptor::RecordCopyDataToHost() no transfer "
"resources");
}

for (const auto& r : GetResources())
r.second->CopyToHost(command);
Result BufferBackedDescriptor::RecordCopyTransferResourceToHost(
CommandBuffer* command_buffer,
Resource* transfer_resource) {
if (!transfer_resource->IsReadOnly()) {
transfer_resource->CopyToHost(command_buffer);
}

return {};
}

Result BufferBackedDescriptor::MoveResourceToBufferOutput() {
// No need to copy results of read only resources.
if (IsReadOnly())
Result BufferBackedDescriptor::MoveTransferResourceToBufferOutput(
Resource* transfer_resource,
Buffer* buffer) {
// No need to move read only resources to an output buffer.
if (transfer_resource->IsReadOnly()) {
return {};
}

auto resources = GetResources();

if (resources.empty()) {
void* resource_memory_ptr = transfer_resource->HostAccessibleMemoryPtr();
if (!resource_memory_ptr) {
return Result(
"Vulkan: BufferBackedDescriptor::MoveResourceToBufferOutput() no "
"transfer resource");
"Vulkan: BufferBackedDescriptor::MoveTransferResourceToBufferOutput() "
"no host accessible memory pointer");
}

for (const auto& resource : resources) {
void* resource_memory_ptr = resource.second->HostAccessibleMemoryPtr();
auto* buffer = resource.first;
if (!resource_memory_ptr) {
return Result(
"Vulkan: BufferBackedDescriptor::MoveResourceToBufferOutput() "
"no host accessible memory pointer");
}

if (!buffer->ValuePtr()->empty()) {
return Result(
"Vulkan: BufferBackedDescriptor::MoveResourceToBufferOutput() "
"output buffer is not empty");
}

auto size_in_bytes = resource.second->GetSizeInBytes();
buffer->SetElementCount(size_in_bytes / buffer->GetFormat()->SizeInBytes());
buffer->ValuePtr()->resize(size_in_bytes);
std::memcpy(buffer->ValuePtr()->data(), resource_memory_ptr, size_in_bytes);
if (!buffer->ValuePtr()->empty()) {
return Result(
"Vulkan: BufferBackedDescriptor::MoveTransferResourceToBufferOutput() "
"output buffer is not empty");
}

auto size_in_bytes = transfer_resource->GetSizeInBytes();
buffer->SetElementCount(size_in_bytes / buffer->GetFormat()->SizeInBytes());
buffer->ValuePtr()->resize(size_in_bytes);
std::memcpy(buffer->ValuePtr()->data(), resource_memory_ptr, size_in_bytes);

return {};
}

Expand Down
21 changes: 13 additions & 8 deletions src/vulkan/buffer_backed_descriptor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "src/buffer.h"
#include "src/engine.h"
#include "src/vulkan/descriptor.h"
#include "src/vulkan/pipeline.h"
#include "src/vulkan/resource.h"

namespace amber {
Expand All @@ -36,13 +37,19 @@ class BufferBackedDescriptor : public Descriptor {
DescriptorType type,
Device* device,
uint32_t desc_set,
uint32_t binding);
uint32_t binding,
Pipeline* pipeline);
~BufferBackedDescriptor() override;

Result CreateResourceIfNeeded() override { return {}; }
Result RecordCopyDataToResourceIfNeeded(CommandBuffer* command) override;
Result RecordCopyDataToHost(CommandBuffer* command) override;
Result MoveResourceToBufferOutput() override;
static Result RecordCopyBufferDataToTransferResourceIfNeeded(
CommandBuffer* command_buffer,
Buffer* buffer,
Resource* transfer_resource);
static Result RecordCopyTransferResourceToHost(CommandBuffer* command_buffer,
Resource* transfer_resource);
static Result MoveTransferResourceToBufferOutput(Resource* transfer_resource,
Buffer* buffer);
uint32_t GetDescriptorCount() override {
return static_cast<uint32_t>(amber_buffers_.size());
}
Expand All @@ -52,10 +59,8 @@ class BufferBackedDescriptor : public Descriptor {
bool IsReadOnly() const;

protected:
/// Returns a list of unique transfer buffer resources. Note that this list
/// may contain less items than the |amber_buffers| vector contains if two or
/// more amber buffers use same Vulkan buffer.
virtual std::vector<std::pair<Buffer*, Resource*>> GetResources() = 0;
// Pipeline where this descriptor is attached to.
Pipeline* pipeline_;

private:
std::vector<Buffer*> amber_buffers_;
Expand Down
106 changes: 44 additions & 62 deletions src/vulkan/buffer_descriptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,65 +29,63 @@ BufferDescriptor::BufferDescriptor(Buffer* buffer,
DescriptorType type,
Device* device,
uint32_t desc_set,
uint32_t binding)
: BufferBackedDescriptor(buffer, type, device, desc_set, binding) {}
uint32_t binding,
Pipeline* pipeline)
: BufferBackedDescriptor(buffer,
type,
device,
desc_set,
binding,
pipeline) {}

BufferDescriptor::~BufferDescriptor() = default;

Result BufferDescriptor::CreateResourceIfNeeded() {
if (!transfer_buffers_.empty()) {
return Result(
"Vulkan: BufferDescriptor::CreateResourceIfNeeded() must be called "
"only when |transfer_buffers| is empty");
auto& transfer_resources = pipeline_->GetDescriptorTransferResources();

VkBufferUsageFlags flags =
VK_BUFFER_USAGE_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT;
if (IsUniformBuffer() || IsUniformBufferDynamic()) {
flags |= VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT;
} else if (IsStorageBuffer() || IsStorageBufferDynamic()) {
flags |= VK_BUFFER_USAGE_STORAGE_BUFFER_BIT;
} else if (IsUniformTexelBuffer()) {
flags |= VK_BUFFER_USAGE_UNIFORM_TEXEL_BUFFER_BIT;
} else if (IsStorageTexelBuffer()) {
flags |= VK_BUFFER_USAGE_STORAGE_TEXEL_BUFFER_BIT;
} else {
return Result("Unexpected buffer type when deciding usage flags");
}

descriptor_offsets_.reserve(GetAmberBuffers().size());
descriptor_ranges_.reserve(GetAmberBuffers().size());

for (const auto& amber_buffer : GetAmberBuffers()) {
if (amber_buffer->ValuePtr()->empty())
continue;

// Check if the transfer buffer is already created.
if (transfer_buffers_.count(amber_buffer) > 0)
continue;

auto size_in_bytes =
static_cast<uint32_t>(amber_buffer->ValuePtr()->size());

auto transfer_buffer = MakeUnique<TransferBuffer>(
device_, size_in_bytes, amber_buffer->GetFormat());

VkBufferUsageFlags flags =
VK_BUFFER_USAGE_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT;
if (IsUniformBuffer() || IsUniformBufferDynamic()) {
flags |= VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT;
} else if (IsStorageBuffer() || IsStorageBufferDynamic()) {
flags |= VK_BUFFER_USAGE_STORAGE_BUFFER_BIT;
} else if (IsUniformTexelBuffer()) {
flags |= VK_BUFFER_USAGE_UNIFORM_TEXEL_BUFFER_BIT;
} else if (IsStorageTexelBuffer()) {
flags |= VK_BUFFER_USAGE_STORAGE_TEXEL_BUFFER_BIT;
// Create (but don't initialize) the transfer buffer if not already created.
if (transfer_resources.count(amber_buffer) == 0) {
auto size_in_bytes =
static_cast<uint32_t>(amber_buffer->ValuePtr()->size());
auto transfer_buffer = MakeUnique<TransferBuffer>(
device_, size_in_bytes, amber_buffer->GetFormat());
transfer_buffer->SetReadOnly(IsReadOnly());
transfer_resources[amber_buffer] = std::move(transfer_buffer);
} else {
return Result("Unexpected buffer type when deciding usage flags");
// Unset transfer buffer's read only property if needed.
if (!IsReadOnly()) {
transfer_resources[amber_buffer]->SetReadOnly(false);
}
}

Result r = transfer_buffer->Initialize(flags);
// Update the buffer create flags.
Result r =
transfer_resources[amber_buffer]->AsTransferBuffer()->AddUsageFlags(
flags);
if (!r.IsSuccess())
return r;
transfer_buffers_[amber_buffer] = std::move(transfer_buffer);
}

is_descriptor_set_update_needed_ = true;
return {};
}

Result BufferDescriptor::MoveResourceToBufferOutput() {
Result r = BufferBackedDescriptor::MoveResourceToBufferOutput();

transfer_buffers_.clear();
descriptor_offsets_.reserve(GetAmberBuffers().size());
descriptor_ranges_.reserve(GetAmberBuffers().size());

return r;
return {};
}

void BufferDescriptor::UpdateDescriptorSetIfNeeded(
Expand All @@ -100,7 +98,9 @@ void BufferDescriptor::UpdateDescriptorSetIfNeeded(

// Create VkDescriptorBufferInfo for every descriptor buffer.
for (uint32_t i = 0; i < GetAmberBuffers().size(); i++) {
const auto& buffer = transfer_buffers_[GetAmberBuffers()[i]];
const auto& buffer =
pipeline_->GetDescriptorTransferResources()[GetAmberBuffers()[i]]
->AsTransferBuffer();
assert(buffer->GetVkBuffer() && "Unexpected descriptor type");
// Add buffer infos for uniform and storage buffers.
if (IsUniformBuffer() || IsUniformBufferDynamic() || IsStorageBuffer() ||
Expand Down Expand Up @@ -146,23 +146,5 @@ void BufferDescriptor::UpdateDescriptorSetIfNeeded(
is_descriptor_set_update_needed_ = false;
}

std::vector<std::pair<Buffer*, Resource*>> BufferDescriptor::GetResources() {
std::vector<std::pair<Buffer*, Resource*>> ret;
// Add unique amber buffers and related transfer buffers to the vector.
for (const auto& amber_buffer : GetAmberBuffers()) {
// Skip duplicate values.
const auto& image =
std::find_if(ret.begin(), ret.end(),
[&](const std::pair<Buffer*, Resource*>& buffer) {
return buffer.first == amber_buffer;
});
if (image != ret.end())
continue;

ret.emplace_back(amber_buffer, transfer_buffers_[amber_buffer].get());
}
return ret;
}

} // namespace vulkan
} // namespace amber
10 changes: 3 additions & 7 deletions src/vulkan/buffer_descriptor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "src/buffer.h"
#include "src/engine.h"
#include "src/vulkan/buffer_backed_descriptor.h"
#include "src/vulkan/pipeline.h"
#include "src/vulkan/transfer_buffer.h"

namespace amber {
Expand All @@ -42,12 +43,12 @@ class BufferDescriptor : public BufferBackedDescriptor {
DescriptorType type,
Device* device,
uint32_t desc_set,
uint32_t binding);
uint32_t binding,
vulkan::Pipeline* pipeline);
~BufferDescriptor() override;

void UpdateDescriptorSetIfNeeded(VkDescriptorSet descriptor_set) override;
Result CreateResourceIfNeeded() override;
Result MoveResourceToBufferOutput() override;
std::vector<uint32_t> GetDynamicOffsets() override {
return dynamic_offsets_;
}
Expand All @@ -67,12 +68,7 @@ class BufferDescriptor : public BufferBackedDescriptor {

BufferDescriptor* AsBufferDescriptor() override { return this; }

protected:
std::vector<std::pair<Buffer*, Resource*>> GetResources() override;

private:
std::unordered_map<Buffer*, std::unique_ptr<TransferBuffer>>
transfer_buffers_;
std::vector<uint32_t> dynamic_offsets_;
std::vector<VkDeviceSize> descriptor_offsets_;
std::vector<VkDeviceSize> descriptor_ranges_;
Expand Down
7 changes: 2 additions & 5 deletions src/vulkan/descriptor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ namespace vulkan {
class CommandBuffer;
class Device;
class BufferDescriptor;
class ImageDescriptor;
class BufferBackedDescriptor;
class SamplerDescriptor;

Expand All @@ -57,16 +58,12 @@ class Descriptor {

virtual void UpdateDescriptorSetIfNeeded(VkDescriptorSet descriptor_set) = 0;
virtual Result CreateResourceIfNeeded() = 0;
virtual Result RecordCopyDataToResourceIfNeeded(CommandBuffer*) { return {}; }
virtual Result RecordCopyDataToHost(CommandBuffer*) { return {}; }
virtual Result MoveResourceToBufferOutput() { return {}; }
virtual Result SetSizeInElements(uint32_t) { return {}; }
virtual Result AddToBuffer(const std::vector<Value>&, uint32_t) { return {}; }
virtual uint32_t GetDescriptorCount() { return 1; }
virtual std::vector<uint32_t> GetDynamicOffsets() { return {}; }
virtual std::vector<VkDeviceSize> GetDescriptorOffsets() { return {}; }
virtual std::vector<VkDeviceSize> GetDescriptorRanges() { return {}; }
virtual BufferDescriptor* AsBufferDescriptor() { return nullptr; }
virtual ImageDescriptor* AsImageDescriptor() { return nullptr; }
virtual BufferBackedDescriptor* AsBufferBackedDescriptor() { return nullptr; }
virtual SamplerDescriptor* AsSamplerDescriptor() { return nullptr; }
uint32_t GetDescriptorSet() const { return descriptor_set_; }
Expand Down
Loading

0 comments on commit 8c3bfef

Please sign in to comment.