Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Dawn] Implement DoCompute support in dawn backend and fix sparse descriptor set support #585

Merged
merged 21 commits into from
Jul 31, 2019
Merged

[Dawn] Implement DoCompute support in dawn backend and fix sparse descriptor set support #585

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
765854d
initial commit
sarahM0 Jul 19, 2019
0d249e9
Merge branch 'master' into compute
sarahM0 Jul 19, 2019
ca8f887
map results back
sarahM0 Jul 22, 2019
4044fbb
Merge branch 'compute' of github.com:sarahM0/amber into compute
sarahM0 Jul 22, 2019
974630c
Merge branch 'master' into compute
sarahM0 Jul 22, 2019
8285ec5
fix read back
sarahM0 Jul 22, 2019
ab7f0f7
Merge branch 'compute' of github.com:sarahM0/amber into compute
sarahM0 Jul 22, 2019
f1ff19d
update test scripte
sarahM0 Jul 22, 2019
68eee4e
fix broken unit test (temporaray solution - open to suggestion)
sarahM0 Jul 22, 2019
54ebfce
review comments
sarahM0 Jul 24, 2019
5abf09e
Merge branch 'master' of github.com:google/amber into compute
sarahM0 Jul 24, 2019
7c8448d
review comments v2
sarahM0 Jul 24, 2019
94e9bc6
Merge branch 'master' into compute
sarahM0 Jul 25, 2019
9390fd2
fix sparse descriptor set in compute
sarahM0 Jul 25, 2019
d5e44d6
fix sparse descriptor set in graphics
sarahM0 Jul 25, 2019
5ac5121
Merge branch 'compute' of github.com:sarahM0/amber into compute
sarahM0 Jul 25, 2019
9f05251
set/get max_size_in_bytes_
sarahM0 Jul 26, 2019
ad578e6
Merge branch 'master' into compute
sarahM0 Jul 26, 2019
fcb005f
fix wrong merge
sarahM0 Jul 26, 2019
bcbccae
RecalculateMaxSizeInBytes
sarahM0 Jul 26, 2019
2a38abe
Merge branch 'master' into compute
dneto0 Jul 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,17 @@ void Buffer::SetSizeInBytes(uint32_t size_in_bytes) {
bytes_.resize(size_in_bytes);
}

void Buffer::SetMaxSizeInBytes(uint32_t max_size_in_bytes) {
max_size_in_bytes_ = max_size_in_bytes;
}

uint32_t Buffer::GetMaxSizeInBytes() const {
if (max_size_in_bytes_ != 0)
return max_size_in_bytes_;
else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Else not need since about always returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set max_size_in_bytes_ in processSSBO/UBO while parsing, which means it stays at 0 for amber buffers. That why I return GetSizeInBytes() if it was zero (the comment in the header tries to explain this).

return GetSizeInBytes();
}

Result Buffer::SetDataFromBuffer(const Buffer* src, uint32_t offset) {
if (bytes_.size() < offset + src->bytes_.size())
bytes_.resize(offset + src->bytes_.size());
Expand Down
10 changes: 10 additions & 0 deletions src/buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,15 @@ class Buffer {
/// |element_count| elements
void SetSizeInBytes(uint32_t size_in_bytes);

/// max_size_in_bytes_ is the total size in bytes needed to hold the buffer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a definition of the max_size_in_bytes_ field, which should go just before the member declaration at line 200.

Separately, there's a fragile disconnect between setting max_size_in_bytes_, element_count_ and resizing the bytes_ vector. No single one of them is the source of truth.
You'll need to write comments at their member declarations to define exactly what they mean, and therefore determine the single source of truth. To be clear, this is a continuation of a previously existing problem with this class definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can predict this can get complicated so I filed issue #599

/// over all ssbo size and ssbo subdata size calls.
/// Sets the max_size_in_bytes_ to |max_size_in_bytes| bytes
void SetMaxSizeInBytes(uint32_t max_size_in_bytes);
/// Returns max_size_in_bytes_ if it is not zero. Otherwise it means this
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good comment.

/// buffer is an amber buffer which has a fix size and returns
/// GetSizeInBytes()
uint32_t GetMaxSizeInBytes() const;

/// Write |data| into the buffer |offset| bytes from the start. Write
/// |size_in_bytes| of data.
Result SetDataWithOffset(const std::vector<Value>& data, uint32_t offset);
Expand Down Expand Up @@ -188,6 +197,7 @@ class Buffer {

BufferType buffer_type_ = BufferType::kUnknown;
std::string name_;
uint32_t max_size_in_bytes_ = 0;
uint32_t element_count_ = 0;
uint32_t width_ = 0;
uint32_t height_ = 0;
Expand Down
10 changes: 5 additions & 5 deletions src/dawn/engine_dawn.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1293,7 +1293,7 @@ Result EngineDawn::DoBuffer(const BufferCommand* command) {
if (amber_buffer) {
amber_buffer->SetDataWithOffset(command->GetValues(), command->GetOffset());

dawn_buffer->SetSubData(0, amber_buffer->GetSizeInBytes(),
dawn_buffer->SetSubData(0, amber_buffer->GetMaxSizeInBytes(),
amber_buffer->ValuePtr()->data());
}

Expand Down Expand Up @@ -1441,7 +1441,7 @@ Result EngineDawn::AttachBuffersAndTextures(

render_pipeline->buffers.emplace_back(
CreateBufferFromData(*device_, buf_info.buffer->ValuePtr()->data(),
buf_info.buffer->GetSizeInBytes(),
buf_info.buffer->GetMaxSizeInBytes(),
bufferUsage | ::dawn::BufferUsageBit::CopySrc |
::dawn::BufferUsageBit::CopyDst));

Expand All @@ -1459,7 +1459,7 @@ Result EngineDawn::AttachBuffersAndTextures(

BindingInitializationHelper tempBinding = BindingInitializationHelper(
buf_info.binding, render_pipeline->buffers.back(), 0,
buf_info.buffer->GetSizeInBytes());
buf_info.buffer->GetMaxSizeInBytes());
bindingInitalizerHelper[buf_info.descriptor_set].push_back(tempBinding);
}

Expand Down Expand Up @@ -1541,7 +1541,7 @@ Result EngineDawn::AttachBuffers(ComputePipelineInfo* compute_pipeline) {

compute_pipeline->buffers.emplace_back(
CreateBufferFromData(*device_, buf_info.buffer->ValuePtr()->data(),
buf_info.buffer->GetSizeInBytes(),
buf_info.buffer->GetMaxSizeInBytes(),
bufferUsage | ::dawn::BufferUsageBit::CopySrc |
::dawn::BufferUsageBit::CopyDst));

Expand All @@ -1559,7 +1559,7 @@ Result EngineDawn::AttachBuffers(ComputePipelineInfo* compute_pipeline) {

BindingInitializationHelper tempBinding = BindingInitializationHelper(
buf_info.binding, compute_pipeline->buffers.back(), 0,
buf_info.buffer->GetSizeInBytes());
buf_info.buffer->GetMaxSizeInBytes());
bindingInitalizerHelper[buf_info.descriptor_set].push_back(tempBinding);
}

Expand Down
38 changes: 18 additions & 20 deletions src/vkscript/command_parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,15 +627,15 @@ Result CommandParser::ProcessSSBO() {
((cmd->GetOffset() / buf->GetFormat()->SizeInBytes()) *
buf->GetFormat()->InputNeededPerElement()) +
static_cast<uint32_t>(values.size());
// The buffer should only be resized to become bigger. This means that if a
// command was run to set the buffer size we'll honour that size until a
// request happens to make the buffer bigger.
if (value_count > buf->ValueCount())
buf->SetValueCount(value_count);

// Even if the value count doesn't change, the buffer is still resized
// because this maybe the first time data is set into the buffer.
buf->SetSizeInBytes(buf->GetSizeInBytes());
uint32_t element_count = value_count;
if (buf->GetFormat()->GetPackSize() == 0) {
// This divides by the needed input values, not the values per element.
// The assumption being the values coming in are read from the input,
// where components are specified. The needed values maybe less then the
// values per element.
element_count = value_count / buf->GetFormat()->InputNeededPerElement();
}
buf->SetMaxSizeInBytes(element_count * buf->GetFormat()->SizeInBytes());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new implementation allows the max-size-in-bytes to shrink.
I think that can happen if you have an ssbo subdata command with a big offset, then a later one with a small offset. That's because value_count computed a few lines above takes into account the offset of this command but not the prior size.

Copy link
Contributor Author

@sarahM0 sarahM0 Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right I forgot about this condition:
if(buf->GetMaxSizeInBytes() < element_count * buf->GetFormat()->SizeInBytes())
buf->SetMaxSizeInBytes(element_count * buf->GetFormat()->SizeInBytes());
Does this fix the issue you've mentioned?


cmd->SetValues(std::move(values));

Expand Down Expand Up @@ -789,20 +789,18 @@ Result CommandParser::ProcessUniform() {
if (!r.IsSuccess())
return r;

// Multiply by the input needed because the value count will use the needed
// input as the multiplier
uint32_t value_count = ((cmd->GetOffset() / buf->GetFormat()->SizeInBytes()) *
buf->GetFormat()->InputNeededPerElement()) +
static_cast<uint32_t>(values.size());
// The buffer should only be resized to become bigger. This means that if a
// command was run to set the buffer size we'll honour that size until a
// request happens to make the buffer bigger.
if (value_count > buf->ValueCount())
buf->SetValueCount(value_count);

// Even if the value count doesn't change, the buffer is still resized
// because this maybe the first time data is set into the buffer.
buf->SetSizeInBytes(buf->GetSizeInBytes());
uint32_t element_count = value_count;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a repeat of the code sequence at the SSBO routine. That's a code smell that there's a common functionality should be refactored. By the look of it, it seems like all this should be a method on the buffer itself.
In fact, after some digging, there is a similar calculation in Buffer::SetDataWithOffset.
(Hmmm. But that seems to not accomodate the Pack case... ?)

Suggest defining
Buffer::ResizeForDataWithOffset(const std::vector& data, uint32_t offset);
This mirrors Buffer::SetDataWithOffset.

Copy link
Contributor Author

@sarahM0 sarahM0 Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really resizing though. It is setting a fake size to be used in Dawn buffer creation.
How about RecalculateMaxSizeInBytes?

if (buf->GetFormat()->GetPackSize() == 0) {
// This divides by the needed input values, not the values per element.
// The assumption being the values coming in are read from the input,
// where components are specified. The needed values maybe less then the
// values per element.
element_count = value_count / buf->GetFormat()->InputNeededPerElement();
}
buf->SetMaxSizeInBytes(element_count * buf->GetFormat()->SizeInBytes());

if (cmd->IsPushConstant())
buf->SetData(values);
Expand Down
4 changes: 2 additions & 2 deletions src/vkscript/command_parser_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3339,7 +3339,7 @@ TEST_F(CommandParserTest, Uniform) {
const auto* buf = cmd->GetBuffer();
const auto* values = buf->GetValues<float>();
std::vector<float> results = {2.1f, 3.2f, 4.3f, 0.f};
EXPECT_TRUE(results.size() < buf->ValueCount());
EXPECT_EQ(results.size(), buf->ValueCount());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad this is sorted out.

for (size_t i = 0; i < results.size(); ++i) {
EXPECT_FLOAT_EQ(results[i], values[i]);
}
Expand Down Expand Up @@ -3381,7 +3381,7 @@ TEST_F(CommandParserTest, UniformWithContinuation) {
const auto* buf = cmd->GetBuffer();
const auto* values = buf->GetValues<float>();
std::vector<float> results = {2.1f, 3.2f, 4.3f, 0.f, 5.4f, 6.7f, 8.9f, 0.f};
EXPECT_TRUE(results.size() < buf->ValueCount());
EXPECT_EQ(results.size(), buf->ValueCount());
for (size_t i = 0; i < results.size(); ++i) {
EXPECT_FLOAT_EQ(results[i], values[i]);
}
Expand Down