Vulkan: change framebuffer image layout correctly (#156)

`oldLayout` of `VkImageMemoryBarrier` when calling
`vkCmdPipelineBarrier()` must match previous image layout. This
commit changes image layout of framebuffer correctly and fixes bugs
of incorrect drawing results in Intel GPU.

Fixes #155

4 files changed, 41 insertions, 25 deletions

diff --git a/src/vulkan/descriptor.h b/src/vulkan/descriptor.h
index 4bc26db..8bb09ac 100644
--- a/src/vulkan/descriptor.h
+++ b/src/vulkan/descriptor.h
@@ -60,11 +60,6 @@ class Descriptor {
 
   uint32_t GetDescriptorSet() const { return descriptor_set_; }
   uint32_t GetBinding() const { return binding_; }
-  bool operator<(const Descriptor& r) {
-    if (descriptor_set_ == r.descriptor_set_)
-      return binding_ < r.binding_;
-    return descriptor_set_ < r.descriptor_set_;
-  }
 
   DescriptorType GetType() const { return type_; }
 
diff --git a/src/vulkan/frame_buffer.cc b/src/vulkan/frame_buffer.cc
index 8177405..85f4c9b 100644
--- a/src/vulkan/frame_buffer.cc
+++ b/src/vulkan/frame_buffer.cc
@@ -89,10 +89,22 @@ Result FrameBuffer::ChangeFrameImageLayout(VkCommandBuffer command,
           "FrameBuffer::ChangeFrameImageLayout new layout cannot be kProbe "
           "from kInit");
     }
-    // Note that we set the final layout of RenderPass as
-    // VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL, thus we must not change
-    // image layout of frame buffer from color attachment to transfer
-    // source here.
+
+    if (color_image_) {
+      color_image_->ChangeLayout(command,
+                                 VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL,
+                                 VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL,
+                                 VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT,
+                                 VK_PIPELINE_STAGE_TRANSFER_BIT);
+    }
+    if (depth_image_) {
+      depth_image_->ChangeLayout(
+          command, VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL,
+          VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL,
+          VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT,
+          VK_PIPELINE_STAGE_TRANSFER_BIT);
+    }
+
     frame_image_layout_ = FrameImageState::kProbe;
     return {};
   }
@@ -113,7 +125,7 @@ Result FrameBuffer::ChangeFrameImageLayout(VkCommandBuffer command,
                                VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,
                                VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT);
   }
-  frame_image_layout_ = layout;
+  frame_image_layout_ = FrameImageState::kClearOrDraw;
   return {};
 }
 
diff --git a/src/vulkan/graphics_pipeline.cc b/src/vulkan/graphics_pipeline.cc
index 36444bf..e74e8c9 100644
--- a/src/vulkan/graphics_pipeline.cc
+++ b/src/vulkan/graphics_pipeline.cc
@@ -131,6 +131,10 @@ Result GraphicsPipeline::CreateRenderPass() {
   if (color_format_ != VK_FORMAT_UNDEFINED) {
     attachment_desc.push_back(kDefaultAttachmentDesc);
     attachment_desc.back().format = color_format_;
+    attachment_desc.back().initialLayout =
+        VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL;
+    attachment_desc.back().finalLayout =
+        VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL;
 
     color_refer.attachment = static_cast<uint32_t>(attachment_desc.size() - 1);
     color_refer.layout = VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL;
@@ -142,6 +146,10 @@ Result GraphicsPipeline::CreateRenderPass() {
   if (depth_stencil_format_ != VK_FORMAT_UNDEFINED) {
     attachment_desc.push_back(kDefaultAttachmentDesc);
     attachment_desc.back().format = depth_stencil_format_;
+    attachment_desc.back().initialLayout =
+        VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL;
+    attachment_desc.back().finalLayout =
+        VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL;
 
     depth_refer.attachment = static_cast<uint32_t>(attachment_desc.size() - 1);
     depth_refer.layout = VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL;
@@ -358,9 +366,14 @@ Result GraphicsPipeline::SendBufferDataIfNeeded() {
                                         memory_properties_);
 }
 
-void GraphicsPipeline::ActivateRenderPassIfNeeded() {
+Result GraphicsPipeline::ActivateRenderPassIfNeeded() {
   if (render_pass_state_ == RenderPassState::kActive)
-    return;
+    return {};
+
+  Result r = frame_->ChangeFrameImageLayout(command_->GetCommandBuffer(),
+                                            FrameImageState::kClearOrDraw);
+  if (!r.IsSuccess())
+    return r;
 
   VkRenderPassBeginInfo render_begin_info = {};
   render_begin_info.sType = VK_STRUCTURE_TYPE_RENDER_PASS_BEGIN_INFO;
@@ -370,6 +383,7 @@ void GraphicsPipeline::ActivateRenderPassIfNeeded() {
   vkCmdBeginRenderPass(command_->GetCommandBuffer(), &render_begin_info,
                        VK_SUBPASS_CONTENTS_INLINE);
   render_pass_state_ = RenderPassState::kActive;
+  return {};
 }
 
 void GraphicsPipeline::DeactivateRenderPassIfNeeded() {
@@ -447,15 +461,10 @@ Result GraphicsPipeline::ClearBuffer(const VkClearValue& clear_value,
   if (!r.IsSuccess())
     return r;
 
-  r = frame_->ChangeFrameImageLayout(command_->GetCommandBuffer(),
-                                     FrameImageState::kClearOrDraw);
+  r = ActivateRenderPassIfNeeded();
   if (!r.IsSuccess())
     return r;
 
-  // TODO(jaebaek): When multiple clear and draw commands exist, handle
-  //                begin/end render pass properly.
-  ActivateRenderPassIfNeeded();
-
   VkClearAttachment clear_attachment = {};
   clear_attachment.aspectMask = aspect;
   clear_attachment.colorAttachment = 0;
@@ -507,7 +516,6 @@ Result GraphicsPipeline::Draw(const DrawArraysCommand* command) {
   if (!r.IsSuccess())
     return r;
 
-  // TODO(jaebaek): Handle primitive topology.
   if (pipeline_ == VK_NULL_HANDLE) {
     r = CreateVkGraphicsPipeline(ToVkTopology(command->GetTopology()));
     if (!r.IsSuccess())
@@ -522,10 +530,9 @@ Result GraphicsPipeline::Draw(const DrawArraysCommand* command) {
   if (!r.IsSuccess())
     return r;
 
-  frame_->ChangeFrameImageLayout(command_->GetCommandBuffer(),
-                                 FrameImageState::kClearOrDraw);
-
-  ActivateRenderPassIfNeeded();
+  r = ActivateRenderPassIfNeeded();
+  if (!r.IsSuccess())
+    return r;
 
   BindVkDescriptorSets();
   BindVkPipeline();
@@ -548,7 +555,9 @@ Result GraphicsPipeline::ProcessCommands() {
   if (!r.IsSuccess())
     return r;
 
-  ActivateRenderPassIfNeeded();
+  r = ActivateRenderPassIfNeeded();
+  if (!r.IsSuccess())
+    return r;
   DeactivateRenderPassIfNeeded();
 
   r = frame_->CopyColorImageToHost(command_->GetCommandBuffer());
diff --git a/src/vulkan/graphics_pipeline.h b/src/vulkan/graphics_pipeline.h
index 4142482..5515316 100644
--- a/src/vulkan/graphics_pipeline.h
+++ b/src/vulkan/graphics_pipeline.h
@@ -85,7 +85,7 @@ class GraphicsPipeline : public Pipeline {
   Result CreateVkGraphicsPipeline(VkPrimitiveTopology topology);
 
   Result CreateRenderPass();
-  void ActivateRenderPassIfNeeded();
+  Result ActivateRenderPassIfNeeded();
   void DeactivateRenderPassIfNeeded();
 
   // Send vertex and index buffers.