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

Add VulkanDirectAllocator #1850

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

antonio-lunarg
Copy link
Contributor

Here's my attempt at simplifying the VulkanResourceAllocator interface by extracting *Direct functions into a separate class VulkanDirectAllocator.

This is one option: VulkanResourceAllocator subclasses, just need to implement GetDirectAllocator().

Currently, VulkanDirectAllocator is just a copy of VulkanDefaultAllocator. If we want to keep it decoupled by the other allocators, it can evolve separately.

Another option might be having VulkanDirectAllocator accept a pointer to the concrete allocator, and make all direct functions forward their arguments to the concrete allocator. E.g.:

class VulkanDirectAllocator {
    VulkanResourceAllocator& alloc_;

    VkResult CreateBuffer(const VkBufferCreateInfo*    create_info,
                          const VkAllocationCallbacks* allocation_callbacks,
                          VkBuffer*                    buffer,
                          ResourceData*                allocator_data)
    {
        return alloc_.CrateBuffer(create_info, allocation_callbacks, format::kNullHandleId, buffer, allocator_data);
    }
}

Closes #1824

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291193.

@CLAassistant
Copy link

CLAassistant commented Oct 30, 2024

CLA assistant check
All committers have signed the CLA.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5241 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291226.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5242 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5242 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291798.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5247 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5247 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 292040.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5251 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5251 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 292441.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5256 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5256 passed.

@antonio-lunarg
Copy link
Contributor Author

To recap: I got no luck with the initial approach. I noticed I could not use the default allocator as a direct allocator, cause I saw in some cases (-m rebind) direct functions were called for resources created using the rebind allocator, and this resulted in a crash.

For this reason, I opted for the "forwarding" approach, where the direct allocator calls functions of the current allocator, and I discovered there's not "perfect" forwarding, so I had to keep a bunch of direct functions in the resource allocator interface.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 295347.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 295350.

@antonio-lunarg
Copy link
Contributor Author

I just pushed a new version which adds two new functions to the resource allocator for specifying which device memory properties to use when binding memory. In some ways I think it clarifies a bit the intent, especially for binding staging buffers created during replay (which expects replay device memory properties).

Said that, there not much I can do to move MapResourceMemoryDirect and UnmapResourceMemoryDirect as each allocator implements those "directly", no forwarding here. So, if we conclude that this is just overcomplicating the memory allocator interface in general, I would go for adding some documentation on the *Direct functions in VulkanResourceAllocator.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5293 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5293 passed.

Add more info and rationale to the resource allocator direct method
documentation, incorporating the description from the original commit.
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 297380.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5318 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5318 passed.

@antonio-lunarg antonio-lunarg merged commit 704bc13 into LunarG:dev Nov 11, 2024
9 checks passed
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.

Can VulkanResourceAllocator “Direct” methods be simplified?
4 participants