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

hal: dedicated buffer/image memory #2929

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

kvark
Copy link
Member

@kvark kvark commented Jul 31, 2019

Fixes #2511
Closes #2513

The idea is that backends can implement this more efficiently than the default implementation.

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds
  • tested examples with the following backends:
  • rustfmt run on changed code

@kvark kvark requested review from zakarumych and msiglreith July 31, 2019 00:34
@@ -29,6 +29,7 @@ ash = "0.29.0"
gfx-hal = { path = "../../hal", version = "0.2" }
smallvec = "0.6"
winit = { version = "0.19", optional = true }
#vk-mem = { version = "0.1", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

should be removed

@kvark
Copy link
Member Author

kvark commented Jul 31, 2019 via email

/// Create a dedicated allocation for a buffer.
///
/// Returns a memory object that can't be used for binding any resources.
unsafe fn allocate_buffer_memory(

Choose a reason for hiding this comment

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

To actually do that more efficiently, the function must return a view into memory with offset and size.

Copy link
Member Author

Choose a reason for hiding this comment

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

The user is expected to still call get_buffer_requirements before doing this, so the size is known. And the offset is known to be zero. Is this not enough?

Choose a reason for hiding this comment

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

Oh, so this will work only for dedicated allocations?

Choose a reason for hiding this comment

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

I thought we want to integrate vma into vulkan backend this way

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what we can do with VMA is:

  1. make it a compile time optional dependency
  2. turn Memory into an enum, with one variant for normal allocations and another - for VMA-owned ones. The code dealing with memory would need to match the enum, but I don't think it's a problem.
  3. the user is then expected to treat the Memory as dedicated allocation

Does that sound reasonable?

Copy link
Member Author

Choose a reason for hiding this comment

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

So the memory type exposed by the Vulkan backend would be:

enum Memory {
  Physical(vk::Memory),
  SubAllocated(vma::Allocation),
}

Mapping a Memory::SubAllocated will go though VMA and just give out a pointer without really doing any more Vulkan mapping, if I understand correctly, much like with Rendy-memory.

Choose a reason for hiding this comment

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

If VMA has no limitations here. Rendy has to map all non-dedicated mappable memory persistently to allow this, which is not optimal. I wonder how VMA does that.

Copy link
Member Author

Choose a reason for hiding this comment

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

which is not optimal

Is it, really? My understanding is that it's both optimal and recommended for CPU-visible memory.

Choose a reason for hiding this comment

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

It's not a problem when RAM is abundant, like on typical modern PC. But in other cases it may be not the best approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it doesn't map memory by default on creation, unless there is a specific flag provided. Relevant docs - https://gpuopen-librariesandsdks.github.io/VulkanMemoryAllocator/html/memory_mapping.html
It confirms that VMA acts roughly like rendy-memory, and the approach this PR is taking should work.

@kvark
Copy link
Member Author

kvark commented Aug 9, 2019

I suppose I can integrate VMA in this PR as a validation of the API.

@kvark kvark force-pushed the master branch 3 times, most recently from 3e1f8b1 to 375af89 Compare August 19, 2020 14:51
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.

Provide "committed resource" semantics in HAL
3 participants