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

Added new create_heap functions to corell factory #1381

Closed

Conversation

ConorLPBoyle
Copy link

See #1379

@kvark
Copy link
Member

kvark commented Jul 18, 2017

Thank you for doing the investigation and filing this PR!

It's a bit unfortunate that we have to provide the queue type, since it is another derail from Vulkan API (#1354). I'll try to investigate a bit more, but so far there doesn't seem to be an easy workaround.

For the actual API change, one of the following would be preferred:

  1. Either create_xxx_heap returns Heap<SomePhantomType> for strong typing, or
  2. it's still create_heap but there is a parameter of enum type to say what resources are supposed to be created on it

Another side-task is to double-check with Metal to see if the type may be required there as well.
cc @msiglreith @JohnColanduoni

@msiglreith
Copy link
Contributor

Eh, looks like we need to stray away further from the light..
I would prefer going with the 2nd option and expose the limitation in Capabilities.
The currently implemented methods from the 1st option could still be provided for easier usage, especially for the render layer.

@@ -231,6 +260,21 @@ impl core::Factory<R> for Factory {
}
}

fn create_buffer_heap(&mut self, heap_type: &core::HeapType, size: u64) -> native::Heap {
self.create_flagged_heap(heap_type, size,
winapi::D3D12_HEAP_FLAGS(0) | winapi::D3D12_HEAP_FLAG_DENY_NON_RT_DS_TEXTURES | winapi::D3D12_HEAP_FLAG_DENY_RT_DS_TEXTURES)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are aliases which should be used here instead of manually assembling the flags:

  • D3D12_HEAP_FLAG_ALLOW_ONLY_BUFFERS
  • D3D12_HEAP_FLAG_ALLOW_ONLY_NON_RT_DS_TEXTURES
  • D3D12_HEAP_FLAG_ALLOW_ONLY_RT_DS_TEXTURES

@kvark
Copy link
Member

kvark commented Jul 19, 2017

@msiglreith

I would prefer going with the 2nd option and expose the limitation in Capabilities.
The currently implemented methods from the 1st option could still be provided for easier usage, especially for the render layer.

We happen to already use some of the strong typing in the coreLL, e.g. for command queues. I expect users to know ahead of time which heaps are going to be used for allocating of what types of resources, so having strong typing there wouldn't hurt, would it?

@msiglreith
Copy link
Contributor

We happen to already use some of the strong typing in the coreLL, e.g. for command queues. I expect users to know ahead of time which heaps are going to be used for allocating of what types of resources, so having strong typing there wouldn't hurt, would it?

Fair enough, my motivation was to give users additional control for memory management if possibe but it probably won't make any difference in reality.

@kvark
Copy link
Member

kvark commented Jul 19, 2017

We need to come up with a set of guidelines for typing, e.g. strong types are provided if:

  • user is expected to know them ahead of time
  • no overhead is implied

@JohnColanduoni
Copy link
Contributor

Metal heaps are much more hands off than either DX12 or Vulkan (they don't even allow you to specify allocation offsets), and only accept size and memory properties at creation. AFAIK the only restriction for heap resource allocation is that coherent memory cannot be allocated from a heap.

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.

4 participants