Cuda interop #517
Cuda interop #517
Conversation
| void chainPreDestroyCleanup(std::unique_ptr<ICleanup> first) | ||
| { | ||
| ICleanup::chain(m_cachedCreationParams.preDestroyCleanup, std::move(first)); | ||
| } |
There was a problem hiding this comment.
the cleanups are not meant to be dynamic, all the cleanups you'll ever need are required to be specified in the SCreationParams
There was a problem hiding this comment.
the fact you've added this method tells me one thing, you're attempting to use the system to make the Nabla object hold onto something after it has already been created, probably the CUDA resource that you've created from it. Esp because you're hooking into the pre-destroy cleanup.
Whereas the intended use is for Nabla object to hold onto some external resoruce it has been created from.
In the case where you have a Nabla object, owned and created by Nabla, being exported, which created an object in some other API that needs to be destroyed before the Nabla object...
The correct way is for the other API object wrapper to hold a smart_Refctd_ptr to the Nabla object it was imported from.
| std::unique_ptr<ICleanup> next; | ||
|
|
||
| static void chain(std::unique_ptr<ICleanup>& first, std::unique_ptr<ICleanup>&& next) | ||
| { | ||
| if (first) | ||
| return chain(first->next, std::move(next)); | ||
| first = std::move(next); | ||
| } |
There was a problem hiding this comment.
don't like it being the default, if you want to chain up cleanups the proper way would be to either:
- have a "metacleanup" that inherits from multiple cleanups (and each of them virtually from ICleanup)
- have this same chain, but in a
CLinkedListCleanup final
Anyway this is probably not needed for anything due to what I think is the misuse of the API https://github.com/Devsh-Graphics-Programming/Nabla/pull/517/files#r1264588641
include/nbl/video/ILogicalDevice.h
Outdated
| @@ -423,6 +423,8 @@ class NBL_API2 ILogicalDevice : public core::IReferenceCounted, public IDeviceMe | |||
| // | |||
| virtual void unmapMemory(IDeviceMemoryAllocation* memory) = 0; | |||
|
|
|||
| virtual void* getExternalMemoryHandle(IDeviceMemoryBacked* obj) const = 0; | |||
There was a problem hiding this comment.
do we need it though?
We already have getExternalMemoryHandle on the IDeviceMemoryBacked itself
There was a problem hiding this comment.
IDeviceMemoryBacked calls this one and caches the returned handle. This could be removed. Also, I think these handles need to be released by platform specific functions such as CloseHandle on win32 so we might be needing to track them.
| const char* nvrtc64_versions[] = { "nvrtc64_120", "nvrtc64_111","nvrtc64_110","nvrtc64_102","nvrtc64_101","nvrtc64_100","nvrtc64_92","nvrtc64_91","nvrtc64_90","nvrtc64_80","nvrtc64_75","nvrtc64_70",nullptr }; | ||
| const char* nvrtc64_suffices[] = {"","_","_0","_1","_2",nullptr}; |
There was a problem hiding this comment.
btw CUDA 11 had 8 minor versions, and 12 has 3 minor versions, so you're probably missing a few in the version table
src/nbl/video/CVulkanLogicalDevice.h
Outdated
| VkMemoryGetWin32HandleInfoKHR getHandleInfo = { | ||
| .sType = VK_STRUCTURE_TYPE_MEMORY_GET_WIN32_HANDLE_INFO_KHR, | ||
| .memory = vkHandle, | ||
| .handleType = VkExternalMemoryHandleTypeFlagBits(obj->getCachedCreationParams().externalHandleType.value), | ||
| }; | ||
| void* handle = 0; | ||
| if(VK_SUCCESS == m_devf.vk.vkGetMemoryWin32HandleKHR(m_vkdev, &getHandleInfo, &handle)) |
There was a problem hiding this comment.
please guard Windows specific code with the appropriate #ifdef NBL_....._PLATFORM_....
| .semaphore = semaphore, | ||
| .handleType = static_cast<VkExternalSemaphoreHandleTypeFlagBits>(params.externalHandleType.value), | ||
| }; | ||
| if (VK_SUCCESS != m_devf.vk.vkGetSemaphoreWin32HandleKHR(m_vkdev, &props, ¶ms.externalHandle)) |
There was a problem hiding this comment.
I'd make sure you follow the same pattern as you do with Buffers/Images, which is not to hotpatch the creation parameters (because we can no longer tell if the thing was created from an import or created for export)
There was a problem hiding this comment.
Yeah, I'm aware this broke the pattern with the external resources. Although atm CUDA can only import semaphores so we don't need to deal with imports on nabla side.
There was a problem hiding this comment.
with refcounted osHandles we will no longer need to know/care if object was created from import or to export
| bufferMemoryReqs.prefersDedicatedAllocation = vk_dedicatedMemoryRequirements.prefersDedicatedAllocation; | ||
| bufferMemoryReqs.requiresDedicatedAllocation = vk_dedicatedMemoryRequirements.requiresDedicatedAllocation; |
There was a problem hiding this comment.
override these two with true if external or assert that they were true, unless your requirements are not true here
https://github.com/Devsh-Graphics-Programming/Nabla/pull/517/files#r1264590615
There was a problem hiding this comment.
Just to make it clear, unlike textures it is not required for external buffers to have dedicated allocations. It's just easier for us to make them dedicated.
…useful as a utility function)
Description
Testing
TODO list: