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.

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

Added new sample to demonstrate OpenCL-Vulkan interop with ocean surface simulation #110

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

shajder
Copy link

@shajder shajder commented Jul 27, 2024

The sample follow general steps (with multiple optimizations) described in the publication: Realtime GPGPU FFT ocean water simulation

Main focus of the sample is to demonstrate how to share compute/render resources between OpenCL and Vulkan to simulate an ocean surface.

ocean

main subject of this sample is ocean surface simulation
@CLAassistant
Copy link

CLAassistant commented Jul 27, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

This is awesome! I love this! Nice work!

I'll do a more thorough review a little later, but here are a few minor issues I found running this on our implementation, and a few high-level questions:

  1. This sample uses glfw for its windowing library. I'm OK with this personally, and it's used for other Vulkan samples, but the OpenGL samples in this SDK currently use SFML instead. If we're going to use glfw for the Vulkan samples, should we consider adding some CMake smarts for it? Getting glfw working on Windows especially can be challenging.

  2. Would it be possible to add a framerate counter or similar, to quantify the benefit of using external memory vs. copying via the host?

Thanks!

float4 data = read_imagef(twiddle, sampler, data_coords);


work_group_barrier(CLK_IMAGE_MEM_FENCE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this work_group_barrier here, or can it be removed?

If we need it for some reason, then we'll need to compile this kernel for -cl-std=CL2.0 or newer, since neither work_group_barrier nor CLK_IMAGE_MEM_FENCE are in OpenCL C 1.2.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, corrected

Comment on lines 46 to 50
int2 pp_coords0 = (int2)(data.z, uv.y) * (1-mode.x) + (int2)(uv.x, data.z) * mode.x;
float2 p = read_imagef(src, sampler, pp_coords0).rg;

int2 pp_coords1 = (int2)(data.w, uv.y) * (1-mode.x) + (int2)(uv.x, data.w) * mode.x;
float2 q = read_imagef(src, sampler, pp_coords1).rg;
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, to use these .rg swizzles we'll need to compile this kernel for -cl-std=CL3.0. It might be easiest to just use the .xy swizzles, instead.

Suggested change
int2 pp_coords0 = (int2)(data.z, uv.y) * (1-mode.x) + (int2)(uv.x, data.z) * mode.x;
float2 p = read_imagef(src, sampler, pp_coords0).rg;
int2 pp_coords1 = (int2)(data.w, uv.y) * (1-mode.x) + (int2)(uv.x, data.w) * mode.x;
float2 q = read_imagef(src, sampler, pp_coords1).rg;
int2 pp_coords0 = (int2)(data.z, uv.y) * (1-mode.x) + (int2)(uv.x, data.z) * mode.x;
float2 p = read_imagef(src, sampler, pp_coords0).xy;
int2 pp_coords1 = (int2)(data.w, uv.y) * (1-mode.x) + (int2)(uv.x, data.w) * mode.x;
float2 q = read_imagef(src, sampler, pp_coords1).xy;

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines 202 to 205
clEnqueueAcquireExternalMemObjectsKHR_fn
clEnqueueAcquireExternalMemObjectsKHR = NULL;
clEnqueueReleaseExternalMemObjectsKHR_fn
clEnqueueReleaseExternalMemObjectsKHR = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Having these function pointers won't hurt, but this sample is currently calling into the C++ bindings (OpenCL-CLHPP) to acquire and release the external memory objects. The C++ bindings query their own function pointers, so I'm pretty sure these function pointers are never used and can be removed.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, thanks

@shajder
Copy link
Author

shajder commented Aug 3, 2024

This is awesome! I love this! Nice work!

Thanks :)

  1. This sample uses glfw for its windowing library. I'm OK with this personally, and it's used for other Vulkan samples, but the OpenGL samples in this SDK currently use SFML instead. If we're going to use glfw for the Vulkan samples, should we consider adding some CMake smarts for it? Getting glfw working on Windows especially can be challenging.

I could use SFML but in order to support vulkan window that would require newer version than requested 2.5.1.

Update: I just added GLFW related corrections to cmake files but I am not sure if that satisfies all project requirement, it builds at my machine, I will ask mobica fellows for verification as well.

  1. Would it be possible to add a framerate counter or similar, to quantify the benefit of using external memory vs. copying via the host?

Indeed that would be useful, I will check it

Update: added tracking of FPS in a title bar of main window which could be toggled with 'e' key

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants