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.

Sign up for GitHub

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

hal: dedicated buffer/image memory #2929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/backend/vulkan/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


[target.'cfg(windows)'.dependencies]
winapi = { version = "0.3", features = ["libloaderapi", "windef", "winuser"] }
Expand Down
36 changes: 36 additions & 0 deletions src/hal/src/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,24 @@ pub trait Device<B: Backend>: fmt::Debug + Any + Send + Sync {
buf: &mut B::Buffer,
) -> Result<(), BindError>;

/// 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(
Copy link

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?

Copy link

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?

Copy link

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.

Copy link

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.

Copy link

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.

&self,
memory_type: MemoryTypeId,
buffer: &mut B::Buffer,
) -> Result<B::Memory, BindError> {
let req = self.get_buffer_requirements(buffer);
let memory = self.allocate_memory(memory_type, req.size)
.map_err(|err| match err {
AllocationError::OutOfMemory(oom) => BindError::OutOfMemory(oom),
AllocationError::TooManyObjects => BindError::OutOfBounds,
})?;
self.bind_buffer_memory(&memory, 0, buffer)
.map(|()| memory)
}

/// Destroy a buffer.
///
/// The buffer shouldn't be destroyed before any submitted command buffer,
Expand Down Expand Up @@ -457,6 +475,24 @@ pub trait Device<B: Backend>: fmt::Debug + Any + Send + Sync {
image: &mut B::Image,
) -> Result<(), BindError>;

/// Create a dedicated allocation for an image.
///
/// Returns a memory object that can't be used for binding any resources.
unsafe fn allocate_image_memory(
&self,
memory_type: MemoryTypeId,
image: &mut B::Image,
) -> Result<B::Memory, BindError> {
let req = self.get_image_requirements(image);
let memory = self.allocate_memory(memory_type, req.size)
.map_err(|err| match err {
AllocationError::OutOfMemory(oom) => BindError::OutOfMemory(oom),
AllocationError::TooManyObjects => BindError::OutOfBounds,
})?;
self.bind_image_memory(&memory, 0, image)
.map(|()| memory)
}

/// Destroy an image.
///
/// The image shouldn't be destroyed before any submitted command buffer,
Expand Down