From 28d15e772db8748d5299bb868a1436486f02dc6b Mon Sep 17 00:00:00 2001 From: Ilkka Saarelainen Date: Thu, 16 Jul 2020 16:47:34 +0300 Subject: [PATCH 1/2] Fix vkscript buffers being added twice Some of the buffers were added twice if vkscript format was used. This patch removes unnecessary calls to Pipeline::AddBufferDescriptor() function and therefore prevents buffers to be added more than once. --- src/CMakeLists.txt | 4 +- src/vulkan/engine_vulkan.cc | 8 ++- src/vulkan/pipeline.cc | 8 ++- src/vulkan/pipeline_test.cc | 56 ++++++++++++++++++++ tests/cases/graphics_uniform_buffer.vkscript | 35 ++++++++++++ tests/run_tests.py | 2 +- 6 files changed, 107 insertions(+), 6 deletions(-) create mode 100644 src/vulkan/pipeline_test.cc create mode 100644 tests/cases/graphics_uniform_buffer.vkscript diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 5618bffba..7dc0e890e 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -180,7 +180,9 @@ if (${AMBER_ENABLE_TESTS}) ) if (${Vulkan_FOUND}) - list(APPEND TEST_SRCS vulkan/vertex_buffer_test.cc) + list(APPEND TEST_SRCS + vulkan/vertex_buffer_test.cc + vulkan/pipeline_test.cc) endif() if (${Dawn_FOUND}) diff --git a/src/vulkan/engine_vulkan.cc b/src/vulkan/engine_vulkan.cc index 97672cba2..4d973f53b 100644 --- a/src/vulkan/engine_vulkan.cc +++ b/src/vulkan/engine_vulkan.cc @@ -652,13 +652,17 @@ Result EngineVulkan::DoBuffer(const BufferCommand* cmd) { "Vulkan::DoBuffer exceed maxBoundDescriptorSets limit of physical " "device"); } - auto& info = pipeline_map_[cmd->GetPipeline()]; if (cmd->GetValues().empty()) { cmd->GetBuffer()->SetSizeInElements(cmd->GetBuffer()->ElementCount()); } else { cmd->GetBuffer()->SetDataWithOffset(cmd->GetValues(), cmd->GetOffset()); } - return info.vk_pipeline->AddBufferDescriptor(cmd); + if (cmd->IsPushConstant()) { + auto& info = pipeline_map_[cmd->GetPipeline()]; + return info.vk_pipeline->AddPushConstantBuffer(cmd->GetBuffer(), + cmd->GetOffset()); + } + return {}; } } // namespace vulkan diff --git a/src/vulkan/pipeline.cc b/src/vulkan/pipeline.cc index 1dc51ed07..315866e54 100644 --- a/src/vulkan/pipeline.cc +++ b/src/vulkan/pipeline.cc @@ -276,8 +276,6 @@ Result Pipeline::GetDescriptorSlot(uint32_t desc_set, Result Pipeline::AddBufferDescriptor(const BufferCommand* cmd) { if (cmd == nullptr) return Result("Pipeline::AddBufferDescriptor BufferCommand is nullptr"); - if (cmd->IsPushConstant()) - return AddPushConstantBuffer(cmd->GetBuffer(), cmd->GetOffset()); if (!cmd->IsSSBO() && !cmd->IsUniform() && !cmd->IsStorageImage() && !cmd->IsSampledImage() && !cmd->IsCombinedImageSampler() && !cmd->IsUniformTexelBuffer() && !cmd->IsStorageTexelBuffer() && @@ -338,6 +336,12 @@ Result Pipeline::AddBufferDescriptor(const BufferCommand* cmd) { "Descriptors bound to the same binding needs to have matching " "descriptor types"); } + // Check that the buffer is not added already. + const auto& buffers = desc->AsBufferBackedDescriptor()->GetAmberBuffers(); + if (std::find(buffers.begin(), buffers.end(), cmd->GetBuffer()) != + buffers.end()) { + return Result("Buffer has been added already"); + } desc->AsBufferBackedDescriptor()->AddAmberBuffer(cmd->GetBuffer()); } diff --git a/src/vulkan/pipeline_test.cc b/src/vulkan/pipeline_test.cc new file mode 100644 index 000000000..a6e725f95 --- /dev/null +++ b/src/vulkan/pipeline_test.cc @@ -0,0 +1,56 @@ +// Copyright 2020 The Amber Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "src/vulkan/pipeline.h" + +#include "gtest/gtest.h" +#include "src/pipeline.h" +#include "src/vulkan/compute_pipeline.h" + +namespace amber { +namespace vulkan { + +using VulkanPipelineTest = testing::Test; + +TEST_F(VulkanPipelineTest, AddBufferDescriptorAddPushConstant) { + amber::Pipeline amber_pipeline(PipelineType::kCompute); + std::vector create_infos; + ComputePipeline pipeline(nullptr, 0, create_infos); + + auto cmd = MakeUnique(BufferCommand::BufferType::kPushConstant, + &amber_pipeline); + // Push constant buffers should not be passed to AddBufferDescriptor(). + Result r = pipeline.AddBufferDescriptor(cmd.get()); + ASSERT_FALSE(r.IsSuccess()); +} + +TEST_F(VulkanPipelineTest, AddBufferDescriptorAddBufferTwice) { + amber::Pipeline amber_pipeline(PipelineType::kCompute); + std::vector create_infos; + ComputePipeline pipeline(nullptr, 0, create_infos); + + auto cmd = MakeUnique(BufferCommand::BufferType::kPushConstant, + &amber_pipeline); + + cmd = MakeUnique(BufferCommand::BufferType::kUniform, + &amber_pipeline); + Result r = pipeline.AddBufferDescriptor(cmd.get()); + ASSERT_TRUE(r.IsSuccess()) << r.Error(); + // Adding same buffer again should fail. + r = pipeline.AddBufferDescriptor(cmd.get()); + ASSERT_FALSE(r.IsSuccess()); +} + +} // namespace vulkan +} // namespace amber diff --git a/tests/cases/graphics_uniform_buffer.vkscript b/tests/cases/graphics_uniform_buffer.vkscript new file mode 100644 index 000000000..00a47d406 --- /dev/null +++ b/tests/cases/graphics_uniform_buffer.vkscript @@ -0,0 +1,35 @@ +# Copyright 2019 The Amber Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +[vertex shader passthrough] +[fragment shader] +#version 430 + +layout(set = 0, binding = 0) uniform Uniform { + float uniform_value; +}; + +layout(location = 0) out vec4 outColor; + +void main() { + outColor = vec4(uniform_value, 0.0, 0.0, uniform_value); +} + +[test] +uniform ubo 0:0 float 0 1.0 + +clear +draw rect -1 -1 2 2 + +probe rect rgba (0, 0, 250, 250) (1.0 0.0 0.0 1.0) diff --git a/tests/run_tests.py b/tests/run_tests.py index 4a6091557..def893808 100755 --- a/tests/run_tests.py +++ b/tests/run_tests.py @@ -333,7 +333,7 @@ def Run(self): print("--test-prog-path must point to an executable") return 1 - input_file_re = re.compile('^.+[\.]amber|vkscript') + input_file_re = re.compile('^.+[\.](amber|vkscript)') self.test_cases = [] if self.args: From cb079c42490631c403931ad121ad005d59a0cdb0 Mon Sep 17 00:00:00 2001 From: Paul Thomson Date: Fri, 17 Jul 2020 15:57:24 +0100 Subject: [PATCH 2/2] Remove redundant line in test --- src/vulkan/pipeline_test.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/vulkan/pipeline_test.cc b/src/vulkan/pipeline_test.cc index a6e725f95..53eca444b 100644 --- a/src/vulkan/pipeline_test.cc +++ b/src/vulkan/pipeline_test.cc @@ -40,11 +40,8 @@ TEST_F(VulkanPipelineTest, AddBufferDescriptorAddBufferTwice) { std::vector create_infos; ComputePipeline pipeline(nullptr, 0, create_infos); - auto cmd = MakeUnique(BufferCommand::BufferType::kPushConstant, + auto cmd = MakeUnique(BufferCommand::BufferType::kUniform, &amber_pipeline); - - cmd = MakeUnique(BufferCommand::BufferType::kUniform, - &amber_pipeline); Result r = pipeline.AddBufferDescriptor(cmd.get()); ASSERT_TRUE(r.IsSuccess()) << r.Error(); // Adding same buffer again should fail.