Skip to content

/gfx

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] Re-usable command buffers #1852

Merged
merged 4 commits into from Feb 27, 2018

Conversation

@kvark
Copy link
Member

commented Feb 26, 2018 •
edited

Fixes #1848
PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds (on Metal, while GL has an unrelated error to be investigated)
  • tested examples with the following backends: Metal

@kvark kvark force-pushed the kvark:mtl-cmd-reuse branch from 850e95d to d5b639b Feb 26, 2018

@kvark kvark changed the title [WIP][mtl] First implementation of software command buffers [mtl] First implementation of software command buffers Feb 26, 2018

@kvark kvark changed the title [mtl] First implementation of software command buffers [metal] Re-usable command buffers Feb 27, 2018

@mjadczak

mjadczak reviewed Feb 27, 2018
View changes

Copy link
Contributor

left a comment

Nice work! I also like the gradual move to things like BufferOffset instead of u64.
src/backend/metal/src/command.rs Outdated
},
Deferred {
passes: Vec<soft::Pass>,
encoding: bool,

This comment has been minimized.

Sign in to view
Copy link
@mjadczak

mjadczak Feb 27, 2018

Contributor

Is this supposed to indicate whether the buffer is currently being encoded? If so, maybe change to is_encoding or being_encoded, encoding suggests that there is some sort of encoding to the buffer itself
src/backend/metal/src/command.rs Outdated
};
self.reset_resources();
}

fn stop_ecoding(&mut self) {

This comment has been minimized.

Sign in to view
Copy link
@mjadczak

mjadczak Feb 27, 2018

Contributor

stop_encoding not ecoding
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 27, 2018

@mjadczak thanks for taking a look! Commends are addressed. Nicely spotted stop_ecoding :)
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 27, 2018

One of the things that really bothered me in the old Metal backend is the duplication of state-setting calls: on each state, we'd first modify the local state tracker and then check the current encoder, setting the state on it if available. But then, on starting a render/compute pass we'd go through the current state and set it (Vulkan has state inheritance, Metal does not), which used duplicate code, and it was growing pretty messy.

New implementation is straightforward - we compose a list of software commands, and then there are centralized functions that apply them either at the spot or at the beginning on the next relevant pass. Same commands are stored in the re-usable command lists. So, more use cases, but even less code paths :)
@grovesNL

grovesNL reviewed Feb 27, 2018
View changes

Copy link
Member

left a comment

Looks great! Seems much easier to read now.
src/backend/metal/src/command.rs
}
Cmd::DrawIndexed { ref index, primitive_type, ref indices, base_vertex, ref instances } => {
let index_offset = indices.start as pso::BufferOffset * match index.index_type {
MTLIndexType::UInt16 => 2,

This comment has been minimized.

Sign in to view
Copy link
@grovesNL

grovesNL Feb 27, 2018

Member

Not a regression, but I guess we should be careful about byte alignment somewhere here (indexBufferOffset must be aligned to 4 bytes)
src/backend/metal/src/command.rs
.cloned()
.enumerate()
.filter(|&(_, ref resource)| resource.is_some())
.map(|(i, texture)| soft::RenderCommand::BindTexture {

This comment has been minimized.

Sign in to view
Copy link
@grovesNL

grovesNL Feb 27, 2018

Member

Probably could do filter_map like the above for these cases (and cloning lazily), although this will change when we reuse storage anyway

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Feb 27, 2018

Author Member

I did start off filter_map and then figured that it's not as pretty as the new code, and cloning a None is cheap (for Some resources we need those cloned anyway).
src/backend/metal/src/command.rs
exec_render(encoder, &command);
}
}
CommandSink::Deferred { ref mut passes, is_encoding: true } => {

This comment has been minimized.

Sign in to view
Copy link
@grovesNL

grovesNL Feb 27, 2018

Member

Is there a valid case for Deferred where is_encoding would be false? Just wondering whether this should be an assert instead.

This comment has been minimized.

Sign in to view
Copy link
@msiglreith

msiglreith Feb 27, 2018

Member

I think it's valid as we could be outside of the renderpass.
@msiglreith

msiglreith approved these changes Feb 27, 2018
View changes

Copy link
Member

left a comment

LGTM
src/backend/metal/src/command.rs Outdated
}

impl CommandSink {
fn pre_render_commands<I>(&mut self, commands: I)

This comment has been minimized.

Sign in to view
Copy link
@msiglreith

msiglreith Feb 27, 2018

Member

Could you add an explanation here what pre-render commands are? It wasn't clear to me until seeing the calls further down. If I understand it correctly this refers to commands which may be outside of the renderpass. Also, a note about the internal state caching would be helpful here IMO, that we only execute commands if we are inside a renderpass else apply them to the internal cache.
src/backend/metal/src/command.rs
exec_render(encoder, &command);
}
}
CommandSink::Deferred { ref mut passes, is_encoding: true } => {

This comment has been minimized.

Sign in to view
Copy link
@msiglreith

msiglreith Feb 27, 2018

Member

I think it's valid as we could be outside of the renderpass.
src/backend/metal/src/command.rs Outdated
encoder.set_render_pipeline_state(pipeline_state);
}

fn begin_compute(&mut self) -> MTLSize {

This comment has been minimized.

Sign in to view
Copy link
@msiglreith

msiglreith Feb 27, 2018

Member

A note about the return value would be helpful I think, additionally, stating when this will be called as Vulkan doesn't have compute passes so far.

@kvark kvark force-pushed the kvark:mtl-cmd-reuse branch from ac6a32f to 734d48a Feb 27, 2018

@kvark

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 27, 2018

bors r=msiglreith,grovesNL

bors bot added a commit that referenced this pull request Feb 27, 2018

@bors
Merge #1852 
1852: [metal] Re-usable command buffers r=msiglreith,grovesNL a=kvark

Fixes #1848
PR checklist:
- [x] `make` succeeds (on *nix)
- [x] `make reftests` succeeds (on Metal, while GL has an unrelated error to be investigated)
- [x] tested examples with the following backends: Metal
Loading status checks…
d78bd11
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 27, 2018

Build succeeded

@bors bors bot merged commit 734d48a into gfx-rs:master Feb 27, 2018
3 checks passed

3 checks passed

bors Build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kvark kvark deleted the kvark:mtl-cmd-reuse branch Feb 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grovesNL grovesNL

@mjadczak mjadczak

@msiglreith msiglreith

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@kvark @grovesNL @mjadczak @msiglreith
You can’t perform that action at this time.