Implemented external memory sample by mfep · Pull Request #90 · KhronosGroup/OpenCL-SDK

mfep · 2023-10-27T12:47:53Z

NOTE: this PR includes all commits from branch StreamHPC:release-cd. This is required, because the implemented sample requires additional updates to the CI scripts to pull its new dependencies. After #88 is merged, this should be trivial to rebase/merge. Only the last commit is relevant for the sample, all other commits are from the release-cd branch.

Implemented sample showcasing the interop of OpenCL and Vulkan, using external memory,
for details, please visit the newly added README file.

nikhiljnv

Large change to review meaningfully:)
Approving based on our successful testing on NVIDIA implementation.

bashbaug

Note: I only reviewed the last commit.

At some point it would be good to update this sample so it worked with other types of external memory as well, e.g. dma_buf file descriptors.

cmake/Dependencies.cmake

samples/extensions/khr/externalmemory/CMakeLists.txt

bashbaug · 2023-12-04T21:32:52Z

samples/extensions/khr/externalmemory/main.cpp

+        // We need to get the pointer to the vkGetMemoryFdKHR function because
+        // it's from extension VK_KHR_external_memory_fd.
+        PFN_vkGetMemoryFdKHR vkGetMemoryFdKHR =
+            (PFN_vkGetMemoryFdKHR)vkGetDeviceProcAddr(vk_device,
+                                                      "vkGetMemoryFdKHR");


This is the function that is needed for Linux and file descriptors, but a different function is needed for Windows and NT handles. Has this sample been tested on Windows?

Yes, this sample has been tested on Windows, but we got

Test #31: externalmemory_Release ...........Exit code 0xc0000135 ***Exception: 0.01 sec Test #32: externalmemorycpp_Release ........Exit code 0xc0000135 ***Exception: 0.01 sec

In the latest commit, I have added the logic to get the function pointer and call the correct function on Windows, but the runtime fail is still there: https://github.com/StreamHPC/OpenCL-SDK/actions/runs/7216235279/job/19662376992. This runtime error also appears on MacOS, although for now both tests are disabled on CI (macos' here and windows' here).

Given that it works fine in Linux, we assumed this failure could be due to no appropriate Vulkan device present on the failing test machines from CI. Perhaps you have more insights about this?

I think this is still failing because the handle type needs to be updated for Windows:

(cl_mem_properties)CL_EXTERNAL_MEMORY_HANDLE_OPAQUE_FD_KHR, #ifdef _WIN32 (cl_mem_properties)handle_y, #else (cl_mem_properties)fd_y, #endif

I think this still needs to be fixed. Have these samples been tested on Windows?

Forgot to answer this, sorry. The handle type is allegedly fixed, but the example still fails on Windows. Right now is disabled on CI, but I tested it on my personal fork https://github.com/Beanavil/OpenCL-SDK/actions/runs/9563524333/job/26362215011.

samples/extensions/khr/externalmemory/vulkan_utils.h

bashbaug

Note: I've left comments on the C++ sample, but many of the issues occur in the C sample also. Thanks!

samples/extensions/khr/externalmemory/main.cpp

bashbaug · 2024-05-27T16:15:39Z

There is a merge conflict here that needs to be resolved, also.

Beanavil · 2024-05-29T10:47:24Z

Addressed the comments and rebased onto StreamHPC/release-cd. There are some test failures from the other tests of the repo, but I think those are due to release-cd still being worked on to solve some new issues after rebasing. It's probably better to wait until #88 is ready to merge and rebase this one onto it.

bashbaug

The C sample is fixed and doesn't crash for me anymore, thanks!

I think the Windows sample is still broken, though.

samples/extensions/khr/externalmemory/main.c

bashbaug

FYI, here are a few more issues I found.

Note that the sample still does not execute properly on Windows.

samples/extensions/khr/externalmemory/main.cpp

samples/extensions/khr/externalmemory/vulkan_utils.h

Beanavil · 2024-10-08T10:34:52Z

@bashbaug about the Windows crash: unfortunately, I'm only able to run this sample in Windows through the CI and debugging it is rather slow. I checked the error that the sample produced, and it was the 0xc0000135, which means there was a missing DLL (the vulkan-1.dll one it seems). As expected, installing the Vulkan loader fixed that error, but then the example still fails with a 0xc0000139. I think this error is an "Entry point not found" (some DLL is being loaded but that cannot find the other DLLs it relies on), so it may be that other Vulkan-related dependencies are missing. I'll look further into this sometime this week if that's okay.

Beanavil · 2024-11-12T14:49:27Z

Rebased the branch onto main because of merge conflicts
Added the fixes discussed in fixes for Windows StreamHPC/OpenCL-SDK#8

bashbaug

I'd like to fix the first issue below if possible, but otherwise I think this is ready to go. If we find any additional issues we can fix them after merging.

bashbaug · 2025-01-27T00:06:20Z

samples/extensions/khr/externalmemory/main.c

+            cl_device, CL_DEVICE_EXTERNAL_MEMORY_IMPORT_HANDLE_TYPES_KHR,
+            supported_handle_types_count, supported_handle_types, NULL),
+        error, err);
+    for (size_t i = 0; i < supported_handle_types_count; ++i)


supported_handle_types_count is actually the size in bytes rather than a count, so this loop will read off the end of the allocation. There needs to be a divide by cl_external_memory_handle_type_khr somewhere to compute the count.

bashbaug · 2025-01-27T00:18:59Z

samples/extensions/khr/externalmemory/main.c

+        if (opencl_version_is_major(&dev_versions[i], 3))
+        {
+            strcat(compiler_options, "-cl-std=CL3.0 ");
+        }


I don't see anything in the external_saxpy.cl kernel that requires OpenCL C 3.0. Am I missing something? If not, we could remove this code block to shorten the sample, and it could run on more devices also (in theory, at least).

Note, if we remove this code block from the C sample, we should remove it from the C++ sample also.

Removed! OpenCL 3.0 is certainly only necessary for the external memory extension.

CLAassistant · 2025-03-17T20:18:25Z

All committers have signed the CLA.

…ount

nikhiljnv approved these changes Nov 29, 2023

View reviewed changes

bashbaug reviewed Dec 4, 2023

View reviewed changes

bashbaug requested changes Dec 4, 2023

View reviewed changes

samples/extensions/khr/externalmemory/main.cpp Show resolved Hide resolved

Beanavil force-pushed the external-mem-sample branch 7 times, most recently from bf01f67 to aae074f Compare December 15, 2023 09:35

Beanavil force-pushed the external-mem-sample branch 5 times, most recently from 3f900c6 to 5dbeb7b Compare May 29, 2024 09:36

bashbaug reviewed Jun 4, 2024

View reviewed changes

samples/extensions/khr/externalmemory/main.c Outdated Show resolved Hide resolved

Beanavil force-pushed the external-mem-sample branch 2 times, most recently from 90df63b to f220052 Compare June 5, 2024 08:05

Beanavil force-pushed the external-mem-sample branch from f220052 to 7cff095 Compare June 18, 2024 10:11

Beanavil force-pushed the external-mem-sample branch from 7cff095 to 4bf7ad4 Compare July 16, 2024 16:30

Beanavil force-pushed the external-mem-sample branch 5 times, most recently from fb99610 to 7101a17 Compare October 2, 2024 09:25

bashbaug requested changes Oct 2, 2024

View reviewed changes

samples/extensions/khr/externalmemory/main.cpp Outdated Show resolved Hide resolved

samples/extensions/khr/externalmemory/vulkan_utils.h Outdated Show resolved Hide resolved

Beanavil force-pushed the external-mem-sample branch from 7101a17 to 294d0bc Compare October 3, 2024 07:37

bashbaug reviewed Oct 3, 2024

View reviewed changes

samples/extensions/khr/externalmemory/vulkan_utils.h Outdated Show resolved Hide resolved

Beanavil force-pushed the external-mem-sample branch from 294d0bc to 1c8c8db Compare October 4, 2024 19:39

Beanavil force-pushed the external-mem-sample branch from 1c8c8db to 780296c Compare November 12, 2024 14:44

Beanavil force-pushed the external-mem-sample branch from c91ebd6 to 5ca6f4c Compare November 12, 2024 16:29

bashbaug approved these changes Jan 27, 2025

View reviewed changes

Beanavil force-pushed the external-mem-sample branch from 5ca6f4c to ad13557 Compare March 17, 2025 20:18

Beanavil force-pushed the external-mem-sample branch 2 times, most recently from e724350 to ad13557 Compare March 30, 2025 18:49

Beanavil and others added 13 commits April 2, 2025 09:55

External memory C/C++ example

5304c71

Fixes from review

4b430eb

Fix find_suitable_device logic to conditionally increment cl_device_c…

14ce09f

…ount

Conform to OpenCL v3.0.15 specification release

5ba0d26

Fix kernel execution time report

9717d64

Fix opencl version check

5d959f9

Fix logic for finding suitable opencl device

989bc64

Inform when kernel source is not found

8bad1b0

Set VkPhysicalDeviceExternalBufferInfo->usage

168014b

Set VkExternalBufferProperties->sType and pNext

f168b3f

fixes for Windows

b1d906a

Fix external memory handle types loop count

2c3b6dc

Remove unnecesary OpenCL kernel compilation to 3.0

5f52d0d

Beanavil force-pushed the external-mem-sample branch from ad13557 to 5f52d0d Compare April 2, 2025 12:57

bashbaug merged commit 89e0e06 into KhronosGroup:main Apr 2, 2025
82 checks passed

Conversation

mfep commented Oct 27, 2023

Uh oh!

nikhiljnv left a comment

Choose a reason for hiding this comment

Uh oh!

bashbaug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bashbaug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bashbaug commented May 27, 2024

Uh oh!

Beanavil commented May 29, 2024

Uh oh!

bashbaug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bashbaug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Beanavil commented Oct 8, 2024

Uh oh!

Beanavil commented Nov 12, 2024

Uh oh!

bashbaug left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Mar 17, 2025 •

edited

Loading