Merge the current vulkan-dev to the master #119

kkristof · 2018-10-01T17:00:54Z

There has been a while since the last merge, sorry about that :)
New things:

SPIR-V generator, which creates a nice header from the shader sources
Basic image implementation
Image support for the vulkan backend
- Every image related canvas function is supported
New utility functions for the vulkan backend, these should streamline the development
Compile time SPIR-V generation

I know this is a bit big chunk of code to review, so please take your time.
Cheers!

Signed-off-by: Kristof Kosztyo <[email protected]>

In some cases the default 16 ms timeout value wasn't enough. Signed-off-by: Kristof Kosztyo <[email protected]>

Signed-off-by: Kristof Kosztyo <[email protected]>

Updated the shader generator. Implemented simple shader for images. Applied minor refactoring to the vulkan backend. Signed-off-by: Kristof Kosztyo <[email protected]>

Signed-off-by: Kristof Kosztyo <[email protected]>

dbatyai · 2018-10-02T12:46:36Z

cmake/Dependencies.cmake

+  add_custom_command(OUTPUT ${PROJECT_SOURCE_DIR}/src/engines/vulkan/shaders/gepard-vulkan-spirv-binaries.h
+                            ${PROJECT_SOURCE_DIR}/src/engines/vulkan/shaders/gepard-vulkan-spirv-binaries.inc.h
+                     COMMAND python ${PROJECT_SOURCE_DIR}/tools/build-vulkan-shaders.py
+                     DEPENDS ${PROJECT_SOURCE_DIR}/src/engines/vulkan/shaders/*.vert


I don't think these dependencies are needed. The shaders are part of the source tree, they are not generated.

They are needed to trigger the custom command, otherwise once the generated files are created it won't run again if the shaders are changed or new shader is created.

dbatyai · 2018-10-02T13:04:14Z

cmake/Dependencies.cmake

@@ -27,6 +27,15 @@ elseif (BACKEND STREQUAL "VULKAN")

    set(VULKAN_INCLUDE_DIR ${PROJECT_BINARY_DIR}/thirdparty/include)
  endif()
+  add_custom_command(OUTPUT ${PROJECT_SOURCE_DIR}/src/engines/vulkan/shaders/gepard-vulkan-spirv-binaries.h
+                            ${PROJECT_SOURCE_DIR}/src/engines/vulkan/shaders/gepard-vulkan-spirv-binaries.inc.h


These generated files should be added to gitignore to prevent them from accidentally getting committed. As a matter of fact, it would be best if these would be in the build directory, so they don't mess up the source tree.

I agree on adding these to gitignore, but I'm not sure how to make the IDE happy and have working code completion for the shader variables.

apps/examples/image/image.cpp

szledan · 2018-10-02T18:47:28Z

src/engines/vulkan/gepard-vulkan.cpp

    _vk.vkCreateFence(_device, &fenceInfo, _allocator, &fence);
    _vk.vkQueueSubmit(queue, 1, &submitInfo, fence);
-    _vk.vkWaitForFences(_device, 1, &fence, VK_TRUE, timeout);
+    vkResult = _vk.vkWaitForFences(_device, 1, &fence, VK_TRUE, timeout);
+    if (vkResult == VK_TIMEOUT)


Please use braces here.

szledan · 2018-10-02T19:08:58Z

src/engines/vulkan/gepard-vulkan.cpp

+    GD_ASSERT(vkResult == VK_SUCCESS && "Queue submit failed!");
+
+    vkResult = _vk.vkWaitForFences(_device, 1, &fence, VK_TRUE, timeout);
+    if (vkResult == VK_TIMEOUT)


Please use braces here.

szledan · 2018-10-02T19:12:05Z

src/engines/vulkan/gepard-vulkan.cpp

@@ -877,39 +1134,17 @@ void GepardVulkan::createSurfaceImage()
    VkFence fence;
    _vk.vkCreateFence(_device, &fenceInfo, _allocator, &fence);
    _vk.vkQueueSubmit(queue, 1, &submitInfo, fence);
-    _vk.vkWaitForFences(_device, 1, &fence, VK_TRUE, timeout);
+    vkResult = _vk.vkWaitForFences(_device, 1, &fence, VK_TRUE, timeout);
+    if (vkResult == VK_TIMEOUT)


Please use braces here.

szledan · 2018-10-02T19:13:16Z

src/engines/vulkan/gepard-vulkan.cpp

    _vk.vkQueueSubmit(queue, 1, &submitInfo, fence);
-    _vk.vkWaitForFences(_device, 1, &fence, VK_TRUE, timeout);
+    vkResult = _vk.vkWaitForFences(_device, 1, &fence, VK_TRUE, timeout);
+    if (vkResult == VK_TIMEOUT)


szledan · 2018-10-02T19:17:24Z

src/engines/vulkan/gepard-vulkan.cpp

+        &_queueFamilyIndex,                     // const uint32_t*        pQueueFamilyIndices;
+    };
+
+    vkResult = _vk.vkCreateBuffer(_device, &bufferInfo, _allocator, &buffer);


VkResult vkResult = ... :)

szledan · 2018-10-02T19:20:41Z

src/engines/vulkan/gepard-vulkan.cpp

+        },                                          // VkImageSubresourceRange    subresourceRange;
+    };
+
+    vkResult = _vk.vkCreateImageView(_device, &imageViewCreateInfo, _allocator, &imageView);


VkResult...

szledan · 2018-10-02T19:23:57Z

src/engines/vulkan/gepard-vulkan.cpp

+    _vk.vkCreateFence(_device, &fenceInfo, _allocator, &fence);
+    _vk.vkQueueSubmit(queue, 1, &submitInfo, fence);
+    vkResult = _vk.vkWaitForFences(_device, 1, &fence, VK_TRUE, timeout);
+    if (vkResult == VK_TIMEOUT)


szledan · 2018-10-02T19:28:03Z

src/engines/vulkan/gepard-vulkan.h

+    void createShaderModule(VkShaderModule& shader, const uint32_t* code, const size_t codeSize);
+
+    void uploadToDeviceMemory(VkDeviceMemory buffer, const void* data, VkDeviceSize size, VkDeviceSize offset = 0);
+    void createSimplePipeline(VkPipeline& pipeline, VkPipelineLayout& layout,


Maybe if each parameter is in a separate line?

szledan · 2018-10-02T19:31:27Z

src/gepard-image.cpp

+
+#include "gepard-image.h"
+
+#include <cstring>


Maybe it is unnecessary.

elecro · 2018-10-03T15:42:10Z

src/engines/vulkan/gepard-vulkan.cpp


 GepardVulkan::GepardVulkan(GepardContext& context)
    : _context(context)
-    , _vk("libvulkan.so")
+    , _vk("libvulkan.so.1")


Why do we need the .1?

Well, it was needed before, when the libvulkan was built from source, and at some point it didn't produced the libvulkan.so, it might was a bug. I tried it locally, and it worked fine, thanks.

elecro · 2018-10-03T15:43:03Z

src/engines/vulkan/gepard-vulkan.cpp

-static const uint64_t timeout = (uint64_t)16 * oneMiliSec; // 16 ms
+static const uint64_t timeout = (uint64_t)32 * oneMiliSec; // 32 ms
+#ifdef VK_USE_PLATFORM_XCB_KHR
+static xcb_connection_t* xcbConnection;


This should be placed inside the GepardVulkan class imho. As if you are creating multiple contexts they could overwrite each other.

elecro · 2018-10-03T15:45:50Z

src/engines/vulkan/gepard-vulkan.cpp

-        _vk.vkFlushMappedMemoryRanges(_device, 1, &range);
-        _vk.vkUnmapMemory(_device, vertexBufferMemory);
-    }
+    createBuffer(vertexBuffer, vertexBufferMemory, vertexMemoryRequirements, (VkDeviceSize)sizeof(vertexData), VK_BUFFER_USAGE_VERTEX_BUFFER_BIT);


As there will be multiple buffers in the long run, maybe it would be good idea to create a class which represents a buffer object with all of it's memory requirement, vkBuffer, and everything. However it should not be done in this PR afaik.

I agree, I've created issue #134 to cover this, as there are other resources which would benefit from this. This will be resolved in a future PR.

elecro · 2018-10-03T15:50:57Z

src/engines/vulkan/gepard-vulkan.cpp

+    GD_ASSERT(vkResult == VK_SUCCESS && "Memory bind failed!");
+}
+
+void GepardVulkan::createImageView(VkImageView &imageView, VkImage image)


Would it be possible to return the imageView instead of requesting it as a reference?
That would make it possible to directly use the result in structures (if needed) or something like this:

VkImageView view = createImageView(image);

Okay, I have tried to do this for all the resources which are created this way, but with the image I got segmentation fault, which is strange. I've created issue #137 to resolve this.

elecro · 2018-10-03T15:51:37Z

src/engines/vulkan/gepard-vulkan.cpp

+        0,                                          // VkImageViewCreateFlags     flags;
+        image,                                      // VkImage                    image;
+        VK_IMAGE_VIEW_TYPE_2D,                      // VkImageViewType            viewType;
+        _imageFormat,                               // VkFormat                   format;


Is the _imageFormat global for all images?

Yes it is, at least for now.

elecro · 2018-10-03T16:46:08Z

src/engines/vulkan/gepard-vulkan.cpp

+    const VkMemoryAllocateInfo imageAllocateInfo = {
+        VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO,                                             // VkStructureType    sType;
+        nullptr,                                                                            // const void*        pNext;
+        memReq.size,                                                       // VkDeviceSize       allocationSize;


Incorrect indentation of the comments.

elecro · 2018-10-03T16:46:23Z

src/engines/vulkan/gepard-vulkan.cpp

+    const VkMappedMemoryRange range = {
+        VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE,  // VkStructureType    sType;
+        nullptr,                                // const void*        pNext;
+        buffer,                                 // VkDeviceMemory     memory;


Incorrect indentation of the comments.

elecro · 2018-10-03T16:47:31Z

src/engines/vulkan/gepard-vulkan.cpp

+            0.0f,   // float A
+        },                                                          // float                                         blendConstants[4];
+    };
+    _vk.vkCreatePipelineLayout(_device, &layoutInfo, _allocator, &layout);


Should the result be checked?

elecro · 2018-10-03T16:48:31Z

src/engines/vulkan/gepard-vulkan.cpp

+
+VkViewport GepardVulkan::getDefaultViewPort()
+{
+    VkViewport viewPort =         {


The opening brace has incorrect indentation.

elecro · 2018-10-03T16:50:51Z

src/engines/vulkan/gepard-vulkan.cpp

+    _vk.vkDestroyFence(_device, fence, _allocator);
+}
+
+void GepardVulkan::updateSurface()


hmmm this is only used at one place, however the internal parts of the method is used at least two other places.

Signed-off-by: Kristof Kosztyo <[email protected]>

dbatyai · 2018-10-03T20:44:57Z

src/CMakeLists.txt

@@ -71,17 +73,19 @@ if (BACKEND STREQUAL "VULKAN" AND NOT VULKAN_FOUND)
 endif()

 if (BACKEND STREQUAL "VULKAN")
+  include(engines/vulkan/shaders/VulkanShaders.cmake)


I think this file should go into the cmake/ directory, with the other cmake modules. Then you can simply use include(VulkanShaders).

dbatyai · 2018-10-03T20:48:27Z

thirdparty/CMakeLists.txt

@@ -22,7 +22,8 @@ set(INCLUDE_OUTPUT_DIR ${PROJECT_SOURCE_DIR}/include)
 # - A subdirectory in PROJECT_BINARY_DIR can be used as a build directory.

 add_custom_target(common ALL
-                  DEPENDS gtest)
+                  DEPENDS gtest
+                          glslang)


Shouldn't only the vulkan backend depend on this? Please move this to the vulkan target below this one.

Okay, I wasn't sure if I first build the gles2 backend then I build the vulkan it would get the glslang and I thought it doesn't have a big toll to build the glslang for every traget. Now I've tried what happend in this case and it works flawless, thanks.

kkristof · 2018-10-03T21:46:32Z

For all the vkResult and structure comment related issues I have created issue #135 and #136.

Signed-off-by: Kristof Kosztyo <[email protected]>

szledan

informal lgtm, thanks

kkristof · 2018-10-04T20:39:42Z

Hi, I've updated the PR and add several issues which can be resolved later. Please let me know if I missed something. Thanks.

because it was not a real approve

elecro

As there are issues to track the required improvements and some of them are already fixed I'm ok to land this. LGTM

szledan · 2018-10-05T14:04:07Z

🎉🍾🖖

kkristof added 19 commits November 6, 2016 21:33

Add script for SPIR-V compiling

731bae1

Signed-off-by: Kristof Kosztyo <[email protected]>

Store the SPIR-V binaries in auto-generated source files

a05f278

Signed-off-by: Kristof Kosztyo <[email protected]>

WIP: Add intel vulkan support

5d20ddf

Signed-off-by: Kristof Kosztyo <[email protected]>

Change the default timeout to 32 ms

39db3ce

In some cases the default 16 ms timeout value wasn't enough. Signed-off-by: Kristof Kosztyo <[email protected]>

Merge branch 'master' into vulkan-dev

3439dfa

Push the X11-xcb dependency to the Vulkan port

3f5511d

Signed-off-by: Kristof Kosztyo <[email protected]>

Merge branch 'master' into vulkan-dev

b6e1a1a

Add basic image class

e9bd0ce

Implement putImage function for the vulkan backend

7db4655

Signed-off-by: Kristof Kosztyo <[email protected]>

Implement getImage function for the vulkan backend

c06b917

Signed-off-by: Kristof Kosztyo <[email protected]>

Add utility functions for the vulkan backend

018beb8

Signed-off-by: Kristof Kosztyo <[email protected]>

Implement drawimage for the vulkan backend

2effe91

Updated the shader generator. Implemented simple shader for images. Applied minor refactoring to the vulkan backend. Signed-off-by: Kristof Kosztyo <[email protected]>

Add the shaders for the drawimage function

957bd74

Signed-off-by: Kristof Kosztyo <[email protected]>

Add some TODO based on the GeCo18

36c6eb5

Merge branch 'master' into vulkan-dev

46ac6ae

Adapt the image example to the API change

d0f22a6

Signed-off-by: Kristof Kosztyo <[email protected]>

Compile the vulkan shaders at build-time

661c4c1

Signed-off-by: Kristof Kosztyo <[email protected]>

Use hexa output in the generated SPIR-V header

c02ba4e

Signed-off-by: Kristof Kosztyo <[email protected]>

Use image references

420decb

Signed-off-by: Kristof Kosztyo <[email protected]>

kkristof added feature infrastructure backend important labels Oct 1, 2018

kkristof requested review from elecro, szledan and dbatyai October 1, 2018 17:00

kkristof mentioned this pull request Oct 1, 2018

[vulkan] Compile the shaders at build time #89

Closed

3 tasks

dbatyai requested changes Oct 2, 2018

View reviewed changes

szledan reviewed Oct 2, 2018

View reviewed changes

elecro reviewed Oct 3, 2018

View reviewed changes

kkristof added 3 commits October 3, 2018 20:11

Merge branch 'master' into vulkan-dev

e324bdc

Signed-off-by: Kristof Kosztyo <[email protected]>

Add glslang to the thirdparties

c6a567a

Signed-off-by: Kristof Kosztyo <[email protected]>

Rework the SPIR-V handling for the vulkan backend

a8e85ca

Signed-off-by: Kristof Kosztyo <[email protected]>

dbatyai reviewed Oct 3, 2018

View reviewed changes

kkristof added 2 commits October 3, 2018 23:50

Review fixes

9ff8d42

Signed-off-by: Kristof Kosztyo <[email protected]>

Review fixes for the build system

f0f5bf9

dbatyai approved these changes Oct 4, 2018

View reviewed changes

szledan previously approved these changes Oct 4, 2018

View reviewed changes

szledan assigned elecro Oct 5, 2018

elecro approved these changes Oct 5, 2018

View reviewed changes

kkristof merged commit b2aa74d into GepardGraphics:master Oct 5, 2018


		#include "gepard-image.h"

		#include <cstring>

Merge the current vulkan-dev to the master #119

Merge the current vulkan-dev to the master #119

Conversation

kkristof commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkristof Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkristof commented Oct 3, 2018

szledan left a comment

Choose a reason for hiding this comment

kkristof commented Oct 4, 2018

elecro left a comment

Choose a reason for hiding this comment

szledan commented Oct 5, 2018

kkristof Oct 3, 2018 •

edited

Loading