JBR-6543 Vulkan: migrate current code to pure c #267

avu · 2024-01-11T20:03:51Z

No description provided.

src/java.desktop/share/native/common/java2d/cvulkan/VKBase.c

src/java.desktop/share/native/common/java2d/cvulkan/VKRenderQueue.h

src/java.desktop/share/native/common/java2d/cvulkan/VKSurfaceData.h

src/java.desktop/share/native/common/java2d/cvulkan/VKBase.c

src/java.desktop/share/native/common/java2d/cvulkan/VKSurfaceData.c

src/java.desktop/share/native/common/java2d/cvulkan/VKBase.c

YaaZ

Overall it's much cleaner now, C API is not THAT bad after all :)

make/modules/java.desktop/lib/Awt2dLibraries.gmk

YaaZ · 2024-04-20T14:33:05Z

src/java.desktop/share/native/common/java2d/cvulkan/CArrayUtil.h

+
+#define ARRAY_CAPACITY_MULT 2
+typedef struct {
+    size_t elem_size;


I don't think we need this field. It takes extra space, but there's no real value in having it.

It's used for reallocation of the array to provide more space (see CArrayUtil.c CARR_array_realloc)

Sure, it is only used in reallocation, but this is such a compile time constant which is not usually stored explicitly. When adding an element and trying to reallocate an array we could probably get it from the parameter via sizeof

YaaZ · 2024-04-20T14:54:32Z

src/java.desktop/share/native/common/java2d/cvulkan/VKBase.c

+        char* enabledLayers[MAX_ENABLED_LAYERS];
+        uint32_t enabledLayersCount = 0;
+        char* enabledExtensions[MAX_ENABLED_EXTENSIONS];


Maybe use new dynamic arrays here?

YaaZ · 2024-04-20T15:00:46Z

src/java.desktop/share/native/common/java2d/cvulkan/VKBase.c

+            return NULL;
+        } else {
+            J2dRlsTrace(J2D_TRACE_INFO, "Vulkan: Instance Created\n")
+        }


This is still actual (We need to set up a debug logger after creating an instance)

YaaZ · 2024-04-20T15:16:39Z

src/java.desktop/share/native/common/java2d/cvulkan/VKBase.c

+        geInstance->devices[i].name = strdup(deviceProperties2.properties.deviceName);
+        if (geInstance->devices[i].name == NULL) {


devices[i] is wrong, we need devices[ARRAY_SIZE(devices)] instead

YaaZ · 2024-04-20T15:55:34Z

src/java.desktop/share/native/common/java2d/cvulkan/VKRenderQueue.c

+        VkPresentInfoKHR presentInfo = {};
+        presentInfo.sType = VK_STRUCTURE_TYPE_PRESENT_INFO_KHR;
+
+        presentInfo.waitSemaphoreCount = 0;


YaaZ · 2024-04-20T16:02:47Z

src/java.desktop/share/native/common/java2d/cvulkan/VKSurfaceData.c

+                (uint32_t) (vkwinsdo->vksdOps.height)
+        };
+
+        uint32_t imageCount = vkwinsdo->capabilitiesKhr.minImageCount + 1;


What is the reason behind this minImageCount + 1? I think we should decide on the number of images in conjunction with the presentMode

I didn't want to allocate too many images

I mean, usual logic for choosing number of images is not to use minImageCount as a basis, but rather as a lower bound. presentMode should be a basis instead, for example in IMMEDIATE mode there's no reason to have more than one image, or there is no reason to have MAILBOX with two images, as then it will be identical to FIFO

YaaZ · 2024-04-20T16:05:55Z

src/java.desktop/share/native/common/java2d/cvulkan/VKSurfaceData.c

+        createInfoKhr.imageFormat = vkwinsdo->formatsKhr[0].format;
+        createInfoKhr.imageColorSpace = vkwinsdo->formatsKhr[0].colorSpace;


TODO: the first reported format is not always the easiest "default" format, so we will need some more logic here.
We also need to use this format when creating the render pass, currently we are using hardcoded VK_FORMAT_B8G8R8A8_UNORM there

Also, VK_FORMAT_B8G8R8A8_UNORM may be wrong too. If we want to work in sRGB color space (do we?), then we can get gamma correction done by the hardware with _SRGB formats.

YaaZ · 2024-04-20T16:08:32Z

src/java.desktop/share/native/common/java2d/cvulkan/VKSurfaceData.c

+
+        createInfoKhr.preTransform = vkwinsdo->capabilitiesKhr.currentTransform;
+        createInfoKhr.compositeAlpha = VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR;
+        createInfoKhr.presentMode = VK_PRESENT_MODE_FIFO_KHR;


TODO: inspect available modes and choose something better?

src/java.desktop/share/native/common/java2d/cvulkan/VKSurfaceData.c

tsarn · 2024-04-26T23:06:48Z

src/java.desktop/share/native/common/java2d/cvulkan/VKRenderQueue.c

+        ge->vkResetFences(logicalDevice->device, 1, &logicalDevice->inFlightFence);
+
+        uint32_t imageIndex;
+        ge->vkAcquireNextImageKHR(logicalDevice->device, winDstOps->swapchainKhr, UINT64_MAX,


I would add a TODO: handle swapchain lost/not optimal errors

True, this will fire on window resize

YaaZ · 2024-05-15T09:28:40Z

src/java.desktop/share/native/common/java2d/cvulkan/VKBuffer.c

+    return buffer;
+}
+
+VKBuffer* VKBuffer_CreateFromData(void* vertices, VkDeviceSize bufferSize)


TODO: we would need to implement some memory management strategy in order to not allocate memory once per buffer. We also don't want to create a new buffer each time we want to draw a bunch of vertices.

YaaZ · 2024-05-15T09:32:45Z

src/java.desktop/share/native/common/java2d/cvulkan/VKBuffer.c

+    VKLogicalDevice* logicalDevice = &ge->devices[ge->enabledDeviceNum];
+
+    VKBuffer* buffer = VKBuffer_Create(bufferSize,
+                                       VK_BUFFER_USAGE_TRANSFER_DST_BIT | VK_BUFFER_USAGE_VERTEX_BUFFER_BIT,


Given that we write to this buffer directly, VK_BUFFER_USAGE_TRANSFER_DST_BIT is not needed there. Also, memory which is both DEVICE_LOCAL and HOST_VISIBLE is usually very limited, so we need to use it very wisely and be ready to fall back to some other memory type.

YaaZ · 2024-05-15T09:37:36Z

src/java.desktop/share/native/common/java2d/cvulkan/VKBuffer.c

+                          VkMemoryPropertyFlags properties)
+{
+    VKGraphicsEnvironment* ge = VKGE_graphics_environment();
+    VKLogicalDevice* logicalDevice = &ge->devices[ge->enabledDeviceNum];


TODO: carry necessary device-related context. This mutable global state will give us a lot of problems when we try to switch devices on the fly.

YaaZ · 2024-05-15T09:46:12Z

src/java.desktop/share/native/common/java2d/cvulkan/VKImage.c

+            return NULL;
+        }
+
+        if (!VKImage_CreateFramebuffer(&ARRAY_LAST(images), renderPass)) {


TODO: consider using single imageless framebuffer instead?

YaaZ · 2024-05-15T09:58:54Z

src/java.desktop/share/native/common/java2d/cvulkan/VKSurfaceData.c

-    if (VK_CreateImage(vksdo->width, vksdo->height, VK_FORMAT_R8G8B8A8_UNORM, VK_IMAGE_TILING_LINEAR,
-                VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_SAMPLED_BIT, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT,
-                &vksdo->image, &vksdo->imageMemory) != VK_SUCCESS)
+    vksdo->image = VKImage_Create(vksdo->width, vksdo->height, VK_FORMAT_R8G8B8A8_UNORM, VK_IMAGE_TILING_LINEAR,


VK_IMAGE_TILING_LINEAR is only for cases when we need to directly write into image memory (e.g. from host, not by blit command). Here I believe it's better to use VK_IMAGE_TILING_OPTIMAL

YaaZ · 2024-05-15T10:11:50Z

src/java.desktop/share/native/common/java2d/cvulkan/VKVertex.h

@@ -28,15 +28,37 @@
 #define VKVertex_h_Included

 #include <vulkan/vulkan.h>
+#include "VKBuffer.h"
+
+#define RGBA_TO_L4(c)              \


I believe this is wrong. Our integer-encoded colors are in sRGB color space. Our rendering pipelines expect linear color space. Just normalizing to [0, 1] range is not enough.

I'll check but looks like we have this problem for metal and opengl.

Replaced C++ vulkan rendering with C one

YaaZ · 2024-05-27T14:48:17Z

src/java.desktop/share/glsl/vulkan/color.frag

@@ -1,8 +1,8 @@
 #version 450

-layout(location = 0) in vec4 inColor;
+layout(location = 0) in vec4 fragColor;


Do we consider gradient, or we can add flat here?

src/java.desktop/share/native/common/java2d/vulkan/VKBase.c

YaaZ · 2024-05-27T15:09:56Z

src/java.desktop/share/native/common/java2d/vulkan/VKBase.c

+    VKTxVertex* vertices = ARRAY_ALLOC(VKTxVertex, 4);
+    ARRAY_PUSH_BACK(&vertices, ((VKTxVertex){-1.0f, -1.0f, 0.0f, 0.0f}));
+    ARRAY_PUSH_BACK(&vertices, ((VKTxVertex){1.0f, -1.0f, 1.0f, 0.0f}));
+    ARRAY_PUSH_BACK(&vertices, ((VKTxVertex){-1.0f, 1.0f, 0.0f, 1.0f}));
+    ARRAY_PUSH_BACK(&vertices, ((VKTxVertex){1.0f, 1.0f, 1.0f, 1.0f}));
+    logicalDevice->blitVertexBuffer = ARRAY_TO_VERTEX_BUF(vertices);
+    if (!logicalDevice->blitVertexBuffer) {
+        J2dRlsTrace(J2D_TRACE_ERROR, "Cannot create vertex buffer\n")
+        return JNI_FALSE;
+    }
+    ARRAY_FREE(vertices);


Why do we need this? We can render a full-screen quad with a simple vertex shader and without any vertex input at all. That would simplify the pipeline creation and we wouldn't need to create and bind the buffer.

Yes, we can add a separate shader with embedded coordinates. Here I've reused the shader that will be used for blitting arbitrary areas

Even if we are blitting arbitrary area, if we are doing one draw call per rectangle, it will be much easier to go with the same vertex shader and specify position/size via push constants which will be recorded directly into the command buffer. If we are going to do batch blit with a single draw call, then sure we would better go with ordinary vertex buffer, but then we would need to support growing vertex buffer, maintain a pool or something

I suppose we'll have both. For scan-converted shapes there will be bulk rendering, for rectangular blits there will be one draw call.

src/java.desktop/share/native/common/java2d/vulkan/VKSurfaceData.c

Resolved build failure for build env without vulkan headers

Resolved review comments

Review comments: moved VKImage_CreateFramebuffer to VKSD_InitImageSurface

mkurz · 2024-05-29T18:43:58Z

How ready is Vulkan in the JBR now? Usable?

avu · 2024-05-30T14:53:10Z

@mkurz Not yet it's in the early stage hopefully we'll have something relatively feature-complete in several months.

avu marked this pull request as draft January 11, 2024 20:04

avu self-assigned this Jan 12, 2024

avu requested a review from YaaZ January 12, 2024 10:20

mkartashev requested changes Jan 16, 2024

View reviewed changes

mkartashev approved these changes Jan 17, 2024

View reviewed changes

src/java.desktop/share/native/common/java2d/cvulkan/VKRenderQueue.h Outdated Show resolved Hide resolved

src/java.desktop/share/native/common/java2d/cvulkan/VKSurfaceData.h Outdated Show resolved Hide resolved

YaaZ requested changes Jan 18, 2024

View reviewed changes

YaaZ reviewed Mar 20, 2024

View reviewed changes

avu force-pushed the avu/CVulkan.JBR-6543 branch from 171e76e to d7c2334 Compare April 19, 2024 12:41

YaaZ requested changes Apr 20, 2024

View reviewed changes

tsarn reviewed Apr 26, 2024

View reviewed changes

YaaZ reviewed May 15, 2024

View reviewed changes

JBR-6543 Vulkan: migrate current code to pure c

43a30c8

Replaced C++ vulkan rendering with C one

avu force-pushed the avu/CVulkan.JBR-6543 branch from aaff23e to 43a30c8 Compare May 24, 2024 19:17

avu marked this pull request as ready for review May 24, 2024 19:26

YaaZ approved these changes May 27, 2024

View reviewed changes

avu added 3 commits May 27, 2024 21:39

JBR-6543 Vulkan: migrate current code to pure c

85ab567

Resolved build failure for build env without vulkan headers

JBR-6543 Vulkan: migrate current code to pure c

d179bbf

Resolved review comments

JBR-6543 Vulkan: migrate current code to pure c

9b2ac6a

Review comments: moved VKImage_CreateFramebuffer to VKSD_InitImageSurface

avu merged commit 3f05010 into jbr21 May 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JBR-6543 Vulkan: migrate current code to pure c #267

JBR-6543 Vulkan: migrate current code to pure c #267

avu commented Jan 11, 2024

YaaZ left a comment

YaaZ Apr 20, 2024

avu Apr 26, 2024

YaaZ Apr 26, 2024

YaaZ Apr 20, 2024

YaaZ Apr 20, 2024 •

edited

YaaZ Apr 20, 2024

YaaZ Apr 20, 2024

YaaZ Apr 20, 2024

avu Apr 26, 2024

YaaZ Apr 26, 2024

YaaZ Apr 20, 2024

YaaZ Apr 24, 2024

YaaZ Apr 20, 2024

tsarn Apr 26, 2024

YaaZ Apr 26, 2024

YaaZ May 15, 2024

YaaZ May 15, 2024

YaaZ May 15, 2024

YaaZ May 15, 2024

YaaZ May 15, 2024

YaaZ May 15, 2024

avu May 28, 2024

YaaZ May 27, 2024

YaaZ May 27, 2024

avu May 28, 2024

YaaZ May 28, 2024

avu May 28, 2024

mkurz commented May 29, 2024

avu commented May 30, 2024

		geInstance->devices[i].name = strdup(deviceProperties2.properties.deviceName);
		if (geInstance->devices[i].name == NULL) {

		createInfoKhr.imageFormat = vkwinsdo->formatsKhr[0].format;
		createInfoKhr.imageColorSpace = vkwinsdo->formatsKhr[0].colorSpace;

JBR-6543 Vulkan: migrate current code to pure c #267

JBR-6543 Vulkan: migrate current code to pure c #267

Conversation

avu commented Jan 11, 2024

YaaZ left a 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

YaaZ Apr 20, 2024 • edited

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

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

mkurz commented May 29, 2024

avu commented May 30, 2024

YaaZ Apr 20, 2024 •

edited