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

Remote command sink in Metal #2260

Merged
merged 5 commits into from
Jul 24, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
12 changes: 4 additions & 8 deletions src/backend/gl/src/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,15 +174,11 @@ pub struct DescriptorSet {
pub struct DescriptorPool {}

impl pso::DescriptorPool<Backend> for DescriptorPool {
fn allocate_sets<I>(&mut self, layouts: I) -> Vec<Result<DescriptorSet, pso::AllocationError>>
where
I: IntoIterator,
I::Item: Borrow<DescriptorSetLayout>,
{
layouts.into_iter().map(|layout| Ok(DescriptorSet {
layout: layout.borrow().clone(),
fn allocate_set(&mut self, layout: &DescriptorSetLayout) -> Result<DescriptorSet, pso::AllocationError> {
Ok(DescriptorSet {
layout: layout.clone(),
bindings: Arc::new(Mutex::new(Vec::new())),
})).collect()
})
}

fn free_sets<I>(&mut self, _descriptor_sets: I)
Expand Down
30 changes: 13 additions & 17 deletions src/backend/vulkan/src/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ pub struct DescriptorPool {
}

impl pso::DescriptorPool<Backend> for DescriptorPool {
fn allocate_sets<I>(&mut self, layout_iter: I) -> Vec<Result<DescriptorSet, pso::AllocationError>>
fn allocate_sets<I>(
&mut self, layout_iter: I, output: &mut Vec<DescriptorSet>
) -> Result<(), pso::AllocationError>
where
I: IntoIterator,
I::Item: Borrow<DescriptorSetLayout>,
Expand All @@ -117,27 +119,21 @@ impl pso::DescriptorPool<Backend> for DescriptorPool {
p_set_layouts: raw_layouts.as_ptr(),
};

let descriptor_sets = unsafe {
self.device.0.allocate_descriptor_sets(&info)
};

match descriptor_sets {
Ok(sets) => {
sets.into_iter()
.zip(layout_bindinds.into_iter())
.map(|(raw, bindings)| {
Ok(DescriptorSet { raw, bindings })
})
.collect()
}
Err(err) => vec![Err(match err {
unsafe { self.device.0.allocate_descriptor_sets(&info) }
.map(|sets| {
output.extend(sets
.into_iter()
.zip(layout_bindinds)
Copy link
Contributor

Choose a reason for hiding this comment

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

The change isn't part of this PR, but layout_bindinds is a typo here

.map(|(raw, bindings)| DescriptorSet { raw, bindings })
)
})
.map_err(|err| match err {
vk::Result::ErrorOutOfHostMemory => pso::AllocationError::OutOfHostMemory,
vk::Result::ErrorOutOfDeviceMemory => pso::AllocationError::OutOfDeviceMemory,
// TODO: Uncomment when ash updates to include VK_ERROR_OUT_OF_POOL_MEMORY(_KHR)
// vk::Result::ErrorOutOfPoolMemory => pso::AllocationError::OutOfPoolMemory,
_ => pso::AllocationError::FragmentedPool,
})]
}
})
}

fn free_sets<I>(&mut self, descriptor_sets: I)
Expand Down
21 changes: 15 additions & 6 deletions src/hal/src/pso/descriptor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,23 +119,32 @@ pub trait DescriptorPool<B: Backend>: Send + Sync + fmt::Debug {
/// Descriptors will become invalid once the pool is reset. Usage of invalidated descriptor sets results
/// in undefined behavior.
fn allocate_set(&mut self, layout: &B::DescriptorSetLayout) -> Result<B::DescriptorSet, AllocationError> {
self.allocate_sets(Some(layout)).remove(0)
let mut sets = Vec::with_capacity(1);
self.allocate_sets(Some(layout), &mut sets)
.map(|_| sets.remove(0))
}

/// Allocate one or multiple descriptor sets from the pool.
///
/// Each descriptor set will be allocated from the pool according to the corresponding set layout.
/// Descriptors will become invalid once the pool is reset. Usage of invalidated descriptor sets results
/// in undefined behavior.
fn allocate_sets<I>(&mut self, layouts: I) -> Vec<Result<B::DescriptorSet, AllocationError>>
fn allocate_sets<I>(&mut self, layouts: I, sets: &mut Vec<B::DescriptorSet>) -> Result<(), AllocationError>
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this signature could be confusing. For example, it's unclear from the signature whether the sets's existing contents are overridden or extended. Would it be possible to somehow return impl Iterator and let the caller decide what to do with the newly allocated sets? Then I guess we could save a Vec allocation in allocate_set too

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 took a few attempts to make this API nicer before, and failed. impl Iterator still has to be one concrete type, and different backends would not agree on it. Having it as an associated type of this trait then prevents any sort of default implementation... You are welcome to try it out :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cool to figure this out, because I think we could use that pattern in a few places. Anyway I don't think it's worth holding up this PR, we can change it later if we figure it out (or if impl Iterator widens type support)

where
I: IntoIterator,
I::Item: Borrow<B::DescriptorSetLayout>,
{
layouts
.into_iter()
.map(|layout| self.allocate_set(layout.borrow()))
.collect()
let base = sets.len();
for layout in layouts {
match self.allocate_set(layout.borrow()) {
Ok(set) => sets.push(set),
Err(e) => {
self.free_sets(sets.drain(base ..));
return Err(e)
}
}
}
Ok(())
}

/// Free the given descriptor sets provided as an iterator.
Expand Down