From e2bd9c935ce467094438bd5404f855566538d68d Mon Sep 17 00:00:00 2001
From: Dzmitry Malyshau <kvarkus@gmail.com>
Date: Thu, 15 Jul 2021 01:37:49 -0400
Subject: [PATCH] hal/dx12: flip the root signature upside down

---
 wgpu-hal/src/dx12/device.rs | 33 ++++++++++++++-------------------
 wgpu-hal/src/dx12/mod.rs    | 30 +++++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs
index 44be34b455..27e3b37bbb 100644
--- a/wgpu-hal/src/dx12/device.rs
+++ b/wgpu-hal/src/dx12/device.rs
@@ -994,29 +994,22 @@ impl crate::Device<super::Api> for super::Device {
         // SRV/CBV/UAV tables are added to the signature first, then Sampler tables,
         // and finally dynamic uniform descriptors.
         //
-        // Dynamic uniform buffers are implemented as root descriptors.
-        // This allows to handle the dynamic offsets properly, which would not be feasible
-        // with a combination of root constant and descriptor table.
+        // Buffers with dynamic offsets are implemented as root descriptors.
+        // This is easier than trying to patch up the offset on the shader side.
         //
         // Root signature layout:
-        //     Root Constants: Register: Offest/4, Space: 0
-        //     ...
-        // DescriptorTable0: Space: 1 (SrvCbvUav)
-        // DescriptorTable0: Space: 1 (Sampler)
-        // Root Descriptors 0
-        // DescriptorTable1: Space: 2 (SrvCbvUav)
-        // Root Descriptors 1
+        // Root Constants: Register: Offset/4, Space: 0
         //     ...
+        // (bind group [3]) - Space: 1
+        //   View descriptor table, if any
+        //   Sampler descriptor table, if any
+        //   Root descriptors (for dynamic offset buffers)
+        // (bind group [2]) - Space: 2
+        // ...
+        // (bind group [0]) - Space: 4
 
-        //TODO: reverse the order, according to this advice in
+        //Note: lower bind group indices are put futher down the root signature. See:
         // https://microsoft.github.io/DirectX-Specs/d3d/ResourceBinding.html#binding-model
-        //> Furthermore, applications should generally sort the layout
-        //> of the root arguments in decreasing order of change frequency.
-        //> This way if some implementations need to switch to a different
-        //> memory storage scheme to version parts of a heavily populated
-        //> root arguments, the data that is changing at the highest frequency
-        //> (near the start of the root arguments) is most likely to run
-        //> as efficiently as possible.
 
         let root_constants: &[()] = &[];
 
@@ -1046,7 +1039,7 @@ impl crate::Device<super::Api> for super::Device {
 
         let mut bind_group_infos =
             arrayvec::ArrayVec::<super::BindGroupInfo, { crate::MAX_BIND_GROUPS }>::default();
-        for (index, bgl) in desc.bind_group_layouts.iter().enumerate() {
+        for (index, bgl) in desc.bind_group_layouts.iter().rev().enumerate() {
             let space = root_space_offset + index as u32;
             let mut info = super::BindGroupInfo {
                 tables: super::TableTypes::empty(),
@@ -1160,6 +1153,8 @@ impl crate::Device<super::Api> for super::Device {
 
         // Ensure that we didn't reallocate!
         debug_assert_eq!(ranges.len(), total_non_dynamic_entries);
+        // remember that we pushed them in reverse
+        bind_group_infos.reverse();
 
         let (blob, error) = self
             .library
diff --git a/wgpu-hal/src/dx12/mod.rs b/wgpu-hal/src/dx12/mod.rs
index 389b8d24a5..3ecc5d9901 100644
--- a/wgpu-hal/src/dx12/mod.rs
+++ b/wgpu-hal/src/dx12/mod.rs
@@ -1,7 +1,35 @@
 /*!
 # DirectX12 API internals.
 
-## Pipeline Layout
+Generally the mapping is straightforwad.
+
+## Resource transitions
+
+D3D12 API matches WebGPU internal states very well. The only
+caveat here is issuing a special UAV barrier whenever both source
+and destination states match, and they are for storage sync.
+
+## Memory
+
+For now, all resources are created with "committed" memory.
+
+## Resource binding
+
+See ['Device::create_pipeline_layout`] documentation for the structure
+of the root signature corresponding to WebGPU pipeline layout.
+
+Binding groups is mostly straightforward, with one big caveat:
+all bindings have to be reset whenever the pipeline layout changes.
+This is the rule of D3D12, and we can do nothing to help it.
+
+We detect this change at both [`crate::CommandEncoder::set_bind_group`]
+and [`crate::CommandEncoder::set_render_pipeline`] with
+[`crate::CommandEncoder::set_compute_pipeline`].
+
+For this reason, in order avoid repeating the binding code,
+we are binding everything in [`CommandEncoder::update_root_elements`].
+When the pipeline layout is changed, we reset all bindings.
+Otherwise, we pass a range corresponding only to the current bind group.
 
 !*/