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

Metal: new binding model and SPIRV support #1576

Merged
merged 9 commits into from
Oct 17, 2017
Merged

Conversation

kvark
Copy link
Member

@kvark kvark commented Oct 16, 2017

This PR grew out of scope (basic SPIRV-support), but that was necessary.
Major changes:

Also fixes #1581

New binding model

Assignment from (descriptor_set, binding) to MSL buffer/texture/sampler index is figured out for the pipeline layout and stored inside it as a hashmap.

A shader is not really compiled at create_shader_module, but rather - at PSO creation time, since this is where we know about the pipeline layout. The resource binding overrides of that layout are fed directly into SPIRV-cross compile options.

A command buffer caches all bound resources at the MSL level (not descriptor set level, like it previously did). Binding descriptor sets would update the caches and potentially set the active encoder values. Both argument and regular vertex buffers just participate without extra semantics at this stage, simplifying the command buffer handling and solving the question of how to map those to not conflict.

Copy link
Contributor

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

Seems to be sound.. good job!

@@ -233,13 +235,15 @@ fn main() {

//
let pipelines = {
#[cfg(any(feature = "vulkan", feature = "dx12", feature = "gl"))]
//TODO: remove the type annotations when we have support for
// inidirect argument buffers in SPIRV-Cross
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: inidirect -> indirect

self.samplers.clear();
}

fn add_buffer(&mut self, slot: usize, buffer: MTLBuffer, offset: usize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be also extended for multiple buffers for the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, totally

});
};
inner.viewport = Some(vp);
if let EncoderState::Render(ref encoder) = inner.encoder_state {
Copy link
Contributor

Choose a reason for hiding this comment

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

General question: Caching still required if we are already inside an encoder? Will the states in MTL be reset at encoder boundaries?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they will be reset. That's why we still fill the cache, so that when we exit a render pass and then enter another one, we can re-bind everything that is active.

}

command_buffer.encoder_state = EncoderState::Render(render_encoder);
command_buffer.begin_renderpass(render_encoder);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@kvark
Copy link
Member Author

kvark commented Oct 17, 2017

Huge thanks to @grovesNL for fixing the OSX Travis build issues with spirv_cross!
bors r=msiglreith

bors bot added a commit that referenced this pull request Oct 17, 2017
1576: [metal] New binding model and SPIRV support r=msiglreith a=kvark

This PR grew out of scope (basic SPIRV-support), but that was necessary.
Major changes:
  - Argument buffer support is turned into a run-time feature flag, disabled until SPIRV gets support for it (KhronosGroup/SPIRV-Cross#293)
  - Proper resource binding/state inheritance for command buffers
  - SPIRV-cross integration

Also fixes #1581

## New binding model
Assignment from (descriptor_set, binding) to MSL buffer/texture/sampler index is figured out for the pipeline layout and stored inside it as a hashmap.

A shader is not really compiled at `create_shader_module`, but rather - at PSO creation time, since this is where we know about the pipeline layout. The resource binding overrides of that layout are fed directly into SPIRV-cross compile options.

A command buffer caches all bound resources at the MSL level (not descriptor set level, like it previously did). Binding descriptor sets would update the caches and potentially set the active encoder values. Both argument and regular vertex buffers just participate without extra semantics at this stage, simplifying the command buffer handling and solving the question of how to map those to not conflict.
@kvark kvark changed the title [metal] New binding model and SPIRV support Metal: new binding model and SPIRV support Oct 17, 2017
@bors
Copy link
Contributor

bors bot commented Oct 17, 2017

Build succeeded

@bors bors bot merged commit 7132b64 into gfx-rs:master Oct 17, 2017
@kvark kvark deleted the metal-spirv branch October 17, 2017 16:21
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.

"gfx/examples/hal/quad" doesn't work on macbook with Iris Graphics with metal
3 participants