-
Notifications
You must be signed in to change notification settings - Fork 119
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
Implemented external memory sample #90
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Large change to review meaningfully:)
Approving based on our successful testing on NVIDIA implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// 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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this still needs to be fixed. Have these samples been tested on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I've left comments on the C++ sample, but many of the issues occur in the C sample also. Thanks!
bf01f67
to
aae074f
Compare
There is a merge conflict here that needs to be resolved, also. |
3f900c6
to
5dbeb7b
Compare
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C sample is fixed and doesn't crash for me anymore, thanks!
I think the Windows sample is still broken, though.
printf("Kernel execution time as measured by device:\n"); | ||
printf("\t%llu us.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor: the CPP sample doesn't include an extra newline here, and prints the device time on the same line, similar to the host time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed as requested!
90df63b
to
f220052
Compare
f220052
to
7cff095
Compare
7cff095
to
4bf7ad4
Compare
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.