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

d3d12: Refactor pipeline layouts to match Vulkan's compatibility rules. #1632

Merged
merged 3 commits into from Nov 21, 2017

Conversation

Projects
None yet
2 participants
@msiglreith
Copy link
Member

msiglreith commented Nov 17, 2017

Switching root signatures requires rebinding all tables. Rebinding will now be done automatically and data tracked internally to allow support of two different pipeline bind points.

We now cache the user data inside the command buffer and rebind the required parts on draw/dispatch calls. Upcoming push constant support was considered during implementation. This mostly follows the compatibility aspect from the RFC #1631. Actual compatibility check is not required and the user is supposed to update the corresponding parts following vulkan specs. Therefore a valid program in Vulkan will run in D3D12, and an invalid application might still run correctly in D3D12. Corresponding parts of the Vulkan API specified the behavior as implementation specific.

Next step is to unify the bind_graphics_descriptor_sets/bind_compute_descriptor_sets and bind_graphics_pipeline/bind_compute_pipeline calls.

d3d12: Refactor pipeline layouts to match Vulkan's compatibility rules.
Switching root signatures requires rebinding all tables. Rebinding will now be done automatically and data tracked internally to allow support of two different pipeline bind points.

@kvark kvark self-requested a review Nov 17, 2017

@msiglreith

This comment has been minimized.

Copy link
Member

msiglreith commented Nov 19, 2017

Next step is to unify the bind_graphics_descriptor_sets/bind_compute_descriptor_sets and bind_graphics_pipeline/bind_compute_pipeline calls.

Actually, I prefer the current way. Unification would require to introducte a BindPoint variable, which is no real improvement and we lose a bit of strong-type guarentees (e.g don't bind compute pipeline to graphics bind point).

@msiglreith msiglreith referenced this pull request Nov 19, 2017

Open

Vulkan Compatibility #1610

24 of 38 tasks complete
@kvark
Copy link
Member

kvark left a comment

Looks good in general! A few concerns before we proceed.

if self.active_bindpoint != BindPoint::Graphics {
// Switch to graphics bind point
assert!(self.gr_pipeline.pipeline.is_some(), "No graphics pipeline bound");
let (pipeline, _) = self.gr_pipeline.pipeline.unwrap();

This comment has been minimized.

@kvark

kvark Nov 20, 2017

Member

just doing expect would be more idiomatic (than checking for is_some() in an assert!)

let table_index = i + table_start;
if ((user_data.dirty_mask >> table_index) & 1) == 1 {
let slot = (i + table_slot_start) as _;
match user_data.data[i] {

This comment has been minimized.

@kvark

kvark Nov 20, 2017

Member

should probably just do let ptr = match ... {... };

}

/// Clear dirty flag.
fn clear(&mut self, i: usize) {

This comment has been minimized.

@kvark

kvark Nov 20, 2017

Member

should be renamed for clarity, maybe to clear_dirty

self.flush_compute_user_data();
}

fn flush_compute_user_data(&mut self) {

This comment has been minimized.

@kvark

kvark Nov 20, 2017

Member

seems to be awfully similar to flush_gr_user_data. Would it make sense to share the implementations?

This comment has been minimized.

@msiglreith

msiglreith Nov 20, 2017

Member

hmm, I can try to unify them (I remember that we have something similar done prior).

self.flush_gr_user_data();
}

fn flush_gr_user_data(&mut self) {

This comment has been minimized.

@kvark

kvark Nov 20, 2017

Member

naming is a bit inconsistent ("gr" versus "compute")

table_id += 1;
});
}
}

fn bind_compute_pipeline(&mut self, pipeline: &n::ComputePipeline) {
unsafe {
match self.comp_pipeline.pipeline {
Some((_, signature)) if signature == pipeline.signature => {

This comment has been minimized.

@kvark

kvark Nov 20, 2017

Member

just to clarify: so if the current pipeline is graphics, but the last compute pipeline matches this one, we still don't need to rebind anything?

This comment has been minimized.

@msiglreith

msiglreith Nov 20, 2017

Member

Pipelines only get bound on draw/dispatch calls to avoid redundant rebinds. We only need/know which pipeline to bind at these calls.
e.g

BindGraphicsPipeline() // the pipeline bind here would be useless
... // Fill graphics user data
BindComputePipeline()
... // Fill compute user data
dispatch()
dispatch()
draw()

Edit: Acutally I implemented it by rebinding, so nvm

This comment has been minimized.

@msiglreith

msiglreith Nov 20, 2017

Member

I will add a comment as it's sensible question

This comment has been minimized.

@msiglreith

msiglreith Nov 20, 2017

Member

Ok, now I think I understood the question correctly. As far as I can see from the dx12 docs, the graphics and compute root signature are independent so they shouldn't affect each other. The current pipeline state is independent from the root signature stuff afaik.

If a root signature is changed on a command list, all previous root signature bindings become stale and all newly expected bindings must be set before Draw/Dispatch

@msiglreith

This comment has been minimized.

Copy link
Member

msiglreith commented Nov 20, 2017

Should be ready 🚀
Appveyor failure seems to be unrelated to this PR.

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Nov 20, 2017

thank you!
bors r+

bors bot added a commit that referenced this pull request Nov 20, 2017

Merge #1632
1632: d3d12: Refactor pipeline layouts to match Vulkan's compatibility rules. r=kvark a=msiglreith

Switching root signatures requires rebinding all tables. Rebinding will now be done automatically and data tracked internally to allow support of two different pipeline bind points.

We now cache the user data inside the command buffer and rebind the required parts on draw/dispatch calls. Upcoming push constant support was considered during implementation. This mostly follows the compatibility aspect from the RFC #1631. Actual compatibility check is not required and the user is supposed to update the corresponding parts following vulkan specs. Therefore a valid program in Vulkan will run in D3D12, and an invalid application might still run correctly in D3D12. Corresponding parts of the Vulkan API specified the behavior as implementation specific.

Next step is to unify the `bind_graphics_descriptor_sets`/`bind_compute_descriptor_sets` and `bind_graphics_pipeline`/`bind_compute_pipeline` calls.
@msiglreith

This comment has been minimized.

Copy link
Member

msiglreith commented Nov 21, 2017

Looks like bors crashed
bors r=kvark

bors bot added a commit that referenced this pull request Nov 21, 2017

Merge #1632
1632: d3d12: Refactor pipeline layouts to match Vulkan's compatibility rules. r=kvark a=msiglreith

Switching root signatures requires rebinding all tables. Rebinding will now be done automatically and data tracked internally to allow support of two different pipeline bind points.

We now cache the user data inside the command buffer and rebind the required parts on draw/dispatch calls. Upcoming push constant support was considered during implementation. This mostly follows the compatibility aspect from the RFC #1631. Actual compatibility check is not required and the user is supposed to update the corresponding parts following vulkan specs. Therefore a valid program in Vulkan will run in D3D12, and an invalid application might still run correctly in D3D12. Corresponding parts of the Vulkan API specified the behavior as implementation specific.

Next step is to unify the `bind_graphics_descriptor_sets`/`bind_compute_descriptor_sets` and `bind_graphics_pipeline`/`bind_compute_pipeline` calls.
@kvark

This comment has been minimized.

Copy link
Member

kvark commented Nov 21, 2017

Perhaps, we need to disable MSVC/nightly build for now...

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Nov 21, 2017

(or at least, allow it to fail)

@msiglreith

This comment has been minimized.

Copy link
Member

msiglreith commented Nov 21, 2017

I would like to try again, giving that it built correctly on the staging branch. Due to new merges it might work now
bors r=kvark

bors bot added a commit that referenced this pull request Nov 21, 2017

Merge #1632
1632: d3d12: Refactor pipeline layouts to match Vulkan's compatibility rules. r=msiglreith a=msiglreith

Switching root signatures requires rebinding all tables. Rebinding will now be done automatically and data tracked internally to allow support of two different pipeline bind points.

We now cache the user data inside the command buffer and rebind the required parts on draw/dispatch calls. Upcoming push constant support was considered during implementation. This mostly follows the compatibility aspect from the RFC #1631. Actual compatibility check is not required and the user is supposed to update the corresponding parts following vulkan specs. Therefore a valid program in Vulkan will run in D3D12, and an invalid application might still run correctly in D3D12. Corresponding parts of the Vulkan API specified the behavior as implementation specific.

Next step is to unify the `bind_graphics_descriptor_sets`/`bind_compute_descriptor_sets` and `bind_graphics_pipeline`/`bind_compute_pipeline` calls.
@msiglreith

This comment has been minimized.

Copy link
Member

msiglreith commented Nov 21, 2017

bors keeps crashing 😕

{{:badmatch, {:error, :push}},
 [{BorsNG.GitHub, :push!, 3,
   [file: 'lib/github.ex', line: 47]},
  {BorsNG.Worker.Batcher, :complete_batch, 3,
   [file: 'lib/batcher.ex', line: 375]},
  {BorsNG.Worker.Batcher, :maybe_complete_batch, 1,
   [file: 'lib/batcher.ex', line: 362]},
  {BorsNG.Worker.Batcher, :handle_cast, 2,
   [file: 'lib/batcher.ex', line: 71]},
  {:gen_server, :try_dispatch, 4,
   [file: 'gen_server.erl', line: 616]},
  {:gen_server, :handle_msg, 6,
   [file: 'gen_server.erl', line: 686]},
  {:proc_lib, :init_p_do_apply, 3,
   [file: 'proc_lib.erl', line: 247]}]}

Manually merging?

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Nov 21, 2017

we'll give a friendly heads up to @notriddle and merge

@kvark kvark merged commit cff6d31 into gfx-rs:master Nov 21, 2017

1 of 3 checks passed

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

bors bot added a commit that referenced this pull request Nov 22, 2017

Merge #1634
1634: Specialization constants r=kvark a=msiglreith

Add support for specialization constants on pipeline creation for HAL, Vulkan and D3D12.
Specialization constants allow to change specially marked constants in the shader at compile time to allow some optimizations.

In contrast to Vulkan, we use a strong-typed API for defining the overrides for specialization constants as this is easier on the user side. Only scalar constants can be overriden adapting it for vulkan portability should be possible without API changes.

On D3D12, these are implemented by shifting the SPIR-V -> HLSL -> DXBC compilation process over to pipeline creation **if** we detect that the shader module contains specialization constants (or push constants as anticipation for future support). On pipeline creation the parsed SPIR-V module gets patched with the specialization constants and compiled.

`quad` example has been upated to use the new feature. The image/quad gets scaled by a specialization constant. Metal <-> Vulkan/D3D12 should now show slightly different sized quads as overridden and default values differ.

~~**WIP** as the branch sits on top of #1632 and spirv-cross changes are outstanding grovesNL/spirv_cross#37

@msiglreith msiglreith deleted the msiglreith:root_signature branch Dec 29, 2017

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