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

Remaping descriptor sets in the gl backend #2071

Merged
merged 1 commit into from
Jun 23, 2018

Conversation

goddessfreya
Copy link
Contributor

I'll rebase off master when I'm done.

Uniforms in gl only have a bindings field, not a set one. This means that for shaders that use multiple sets to work, we must change where we are binding them.

See page 14 for what I mean: https://www.khronos.org/assets/uploads/developers/library/2016-vulkan-devday-uk/4-Using-spir-v-with-spirv-cross.pdf

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds
  • tested examples with the following backends:

@goddessfreya
Copy link
Contributor Author

goddessfreya commented May 30, 2018

Breaks the vulkan example and I got no clue why. Otherwise, I think this pull request is ready

Edit: Was because I was using the old shader

@goddessfreya goddessfreya changed the title [WIP]Remaping descriptor sets in the gl backend Remaping descriptor sets in the gl backend May 30, 2018
@kvark
Copy link
Member

kvark commented May 31, 2018

Merge branch 'master' into desc-remaping-gl

please rebase instead of merging

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Left my comments below

array_offset: 0,
descriptors: Some(
pso::Descriptor::Sampler(&sampler)
pso::Descriptor::CombinedImageSampler(&image_srv, i::Layout::ShaderReadOnlyOptimal, &sampler)
Copy link
Member

Choose a reason for hiding this comment

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

looks like we had a bug in here by specifying Undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only changed it because ShaderReadOnlyOptimal is more optimal. No clue if it's a bug.

@@ -15,13 +17,16 @@ pub type FrameBuffer = gl::types::GLuint;
pub type Surface = gl::types::GLuint;
pub type Texture = gl::types::GLuint;
pub type Sampler = gl::types::GLuint;
pub(crate) type SetID = gl::types::GLuint;
Copy link
Member

Choose a reason for hiding this comment

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

the whole module is private to the crate, so no need to have pub(crate) for all the things here, just pub should be good

@@ -15,13 +17,16 @@ pub type FrameBuffer = gl::types::GLuint;
pub type Surface = gl::types::GLuint;
pub type Texture = gl::types::GLuint;
pub type Sampler = gl::types::GLuint;
pub(crate) type SetID = gl::types::GLuint;
pub(crate) type BindingID = gl::types::GLuint;
Copy link
Member

Choose a reason for hiding this comment

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

note that we already have hal::pso::DescriptorSetIndex and hal::pso::DescriptorBinding. Maybe we could use them here instead of defining new types?

@@ -38,6 +43,79 @@ impl Fence {
}
}

#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq)]
pub(crate) enum BindingTypes {
Copy link
Member

Choose a reason for hiding this comment

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

sounds like we could just use hal::pso::DecriptorType instead of this enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hal::pso::DecriptorType has more types then we need. Once more of the binding types get implemented (I've only tested SampledImages) I expect a lot of them to get merged together (ex: SampledImages & SeperateSamplers).

}

#[derive(Clone, Debug)]
pub(crate) struct DescRemapData {
Copy link
Member

Choose a reason for hiding this comment

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

please add comments specifying what this is needed for

return None
}

let bindings = self.bindings.get(&btype).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

similarly, instead of checking for contains_key followed to unwrapping we could just chain all the things:

self.bindings
  .get(&btype)?
  .get(&set)?
  .get(&binding)

@@ -82,10 +160,17 @@ pub enum ImageView {
}

#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
pub struct DescriptorSetLayout;
pub(crate) enum DescSetBindings {
Buffer(BindingTypes, BindingID, RawBuffer, gl::types::GLintptr, gl::types::GLsizeiptr),
Copy link
Member

Choose a reason for hiding this comment

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

the last parameters are not obvious here, so better have them named, e.g.

Buffer {
  ty: ...,
  id: ...,
  buffer: ...,
}

IR: IntoIterator,
IR::Item: Borrow<(pso::ShaderStageFlags, Range<u32>)>,
{
n::PipelineLayout
n::PipelineLayout {
desc_remap_data: Arc::new(RwLock::new(n::DescRemapData::new())),
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, you chose to have Arc+RwLock here because creating a pipeline needs to modify those mappings. This is concerning, actually. Can't we have the remapping table constantly associated with the set of descriptor set layouts? In this case, it would be immutable and filled up right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into this when I get back.

Copy link
Contributor Author

@goddessfreya goddessfreya May 31, 2018

Choose a reason for hiding this comment

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

Ok, I just realized something. The mapping should be owned by the pipeline and not shared between pipelines (by using the same layout). If we have two pipelines sharing the same layout but with different shaders then we will get a suboptimal remapping.

The issue with making the pipelines own the remapping is that the pipeline doesn't get passed to bind_graphics_descriptor_sets, the pipeline layout does. The pipeline is never passed in the creation of the descriptors sets or their layout either. To store the remap data in it's correct place we'd either have to keep track of the current bound pipeline or pass in the pipeline to bind_graphics_descriptor_sets.

We can't store them with the the descriptor set layouts because the layouts are completely unrelated. The remapping is dependent on the binding points and sets set in the shaders of a single pipeline, and is only used when we bind the related descriptor sets after binding that pipeline.

Copy link
Member

Choose a reason for hiding this comment

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

If we have two pipelines sharing the same layout but with different shaders then we will get a suboptimal remapping.

This is fine by me. Pipeline layout is that mapping, semantically speaking. We shouldn't go out of our way to make it more efficient than the API suggests.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is still not the way I imagine it should be. The pipeline layout should not have internal mutability. When it's created, it has all the information required to map from descriptor binding/sets to GL resource slots, and that's the mapping it should carry. All the actual pipeline construction should use that mapping but not modify it.

#[derive(Clone, Debug)]
pub struct DescriptorSet {
layout: Vec<pso::DescriptorSetLayoutBinding>,
pub(crate) bindings: Arc<RwLock<Vec<DescSetBindings>>>,
Copy link
Member

Choose a reason for hiding this comment

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

to recap, if I understand correctly you want Arc+RwLock here because this is an internally mutable state that's changed by write_descriptor_sets, correct? I believe we could go for simpler RefCell here instead, given that Vulkan spec requires that write_descriptor_sets doesn't conflict with any descriptor usage (in flight) or another write/copy of descriptor sets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we could if rust wouldn't error with the following message:

error[E0277]: `std::cell::RefCell<std::vec::Vec<native::DescSetBindings>>` cannot be shared between threads safely
  --> src/backend/gl/src/lib.rs:44:6
   |
44 | impl hal::Backend for Backend {
   |      ^^^^^^^^^^^^ `std::cell::RefCell<std::vec::Vec<native::DescSetBindings>>` cannot be shared between threads safely
   |
   = help: within `native::DescriptorSet`, the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<std::vec::Vec<native::DescSetBindings>>`
   = note: required because it appears within the type `native::DescriptorSet`

Should I just manually implement the Sync trait?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, you are correct... RefCell is !Sync. Let's go with Arc<Mutex> then, it should be lighter than RwLock

let end = range.end.unwrap_or(buffer.size);
let size = (end - start) as _;

set.bindings
Copy link
Member

Choose a reason for hiding this comment

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

can we do this locking just once per set (higher up)?

@kvark
Copy link
Member

kvark commented May 31, 2018 via email

@goddessfreya
Copy link
Contributor Author

Uhh, arc doesn't fix it.

@goddessfreya
Copy link
Contributor Author

Changed RefCell to mutex.
Added _ before the unused variables at src/backend/gl/src/device.rs:1086 to 1104.

@goddessfreya goddessfreya changed the title Remaping descriptor sets in the gl backend [WIP] Remaping descriptor sets in the gl backend Jun 3, 2018
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Looks like this is almost ready to land. What is WIP in the subject for?

@@ -70,7 +70,7 @@ impl hal::Backend for Backend {
type ComputePipeline = native::ComputePipeline;
type GraphicsPipeline = native::GraphicsPipeline;
type PipelineLayout = native::PipelineLayout;
type DescriptorSetLayout = native::DescriptorSetLayout;
type DescriptorSetLayout = Vec<pso::DescriptorSetLayoutBinding>;
Copy link
Member

Choose a reason for hiding this comment

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

if you do need it to just be a vec, let's do pub type DescriptorSetLayout = Vec<pso::...>; in native module and use here and elsewhere consistently

@goddessfreya
Copy link
Contributor Author

Was hoping to get seperate image and samplers implemented too, but it would be ok to do them in a seperate PR.

@kvark
Copy link
Member

kvark commented Jun 5, 2018

@zegentzy please make commits more informative (e.g. not just "Requested changes") and address the CI complains to proceed.

@goddessfreya
Copy link
Contributor Author

goddessfreya commented Jun 5, 2018

I plan on squashing it with the first, I only kept it separate so you know which parts I changed.

Sorry about the CL complaints, I thought it would compile so I didn't bother checking. Lol.

Edit: Will fix them when I get back.

@goddessfreya goddessfreya force-pushed the desc-remaping-gl branch 2 times, most recently from c44bba3 to 55a158d Compare June 5, 2018 23:12
@@ -226,6 +226,8 @@ bitflags! {
const SAMPLER_LOD_BIAS = 0x2000;
/// Support setting border texel colors.
const SAMPLER_BORDER_COLOR = 0x4000;
/// No explicit layouts in shader support
const NO_EXPLICIT_LAYOUTS_IN_SHADER = 0x8000;
Copy link
Member

Choose a reason for hiding this comment

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

let's not use negative flags - you can see that everything else is positive

IR: IntoIterator,
IR::Item: Borrow<(pso::ShaderStageFlags, Range<u32>)>,
{
n::PipelineLayout
n::PipelineLayout {
desc_remap_data: Arc::new(RwLock::new(n::DescRemapData::new())),
Copy link
Member

Choose a reason for hiding this comment

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

I think this is still not the way I imagine it should be. The pipeline layout should not have internal mutability. When it's created, it has all the information required to map from descriptor binding/sets to GL resource slots, and that's the mapping it should carry. All the actual pipeline construction should use that mapping but not modify it.

@goddessfreya
Copy link
Contributor Author

A new issue has come to my attention, when build_combined_image_shaders() is called internally the binding number gets reset, so this will have to change until I add support seperate samplers and images.

@goddessfreya goddessfreya force-pushed the desc-remaping-gl branch 2 times, most recently from 28a8b25 to 6c1a9c6 Compare June 8, 2018 20:47
@goddessfreya goddessfreya changed the title [WIP] Remaping descriptor sets in the gl backend Remaping descriptor sets in the gl backend Jun 8, 2018
@goddessfreya
Copy link
Contributor Author

@kvark While this implementation of remaping and binding samplers isn't the most optimized (including the lack of the 1 sampler to image perf limitation discussed on gitter), it works. Since we don't change the exposed api or break any old behavior, please consider merging this as it is currently and leaving the perf optimizations latter.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Alright, this is strictly better than the old "implementation" :D
Thank you!
bors r+

@@ -423,7 +588,7 @@ impl d::Device<B> for Device {
.iter()
.filter_map(|&(stage, point_maybe)| {
point_maybe.map(|point| {
let shader_name = self.compile_shader(point, stage);
let shader_name = self.compile_shader(point, stage, &mut *desc.layout.desc_remap_data.write().unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't need those &mut * constructs here

bors bot added a commit that referenced this pull request Jun 20, 2018
2071: Remaping descriptor sets in the gl backend r=kvark a=ZeGentzy

I'll rebase off master when I'm done.

Uniforms in gl only have a bindings field, not a set one. This means that for shaders that use multiple sets to work, we must change where we are binding them.

See page 14 for what I mean: https://www.khronos.org/assets/uploads/developers/library/2016-vulkan-devday-uk/4-Using-spir-v-with-spirv-cross.pdf

PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:


Co-authored-by: Hal Gentz <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 20, 2018

Build failed

@grovesNL
Copy link
Contributor

Needs rebasing to get fast hash map changes from #2150:

329 |         let mut id_map = HashMap::<u32, (pso::DescriptorSetIndex, pso::DescriptorBinding)>::new();
    |                          ^^^^^^^ Use of undeclared type or module `HashMap`

You should be able to simply replace these with FastHashMap (from hal::backend::FastHashMap)

@goddessfreya
Copy link
Contributor Author

Sorry for the delay, I had to fix my computer. Two changes above.

@kvark
Copy link
Member

kvark commented Jun 22, 2018

It's good to go once rust-lang/rust#51699 is resolved...

@kvark
Copy link
Member

kvark commented Jun 22, 2018

bors r+

bors bot added a commit that referenced this pull request Jun 22, 2018
2071: Remaping descriptor sets in the gl backend r=kvark a=ZeGentzy

I'll rebase off master when I'm done.

Uniforms in gl only have a bindings field, not a set one. This means that for shaders that use multiple sets to work, we must change where we are binding them.

See page 14 for what I mean: https://www.khronos.org/assets/uploads/developers/library/2016-vulkan-devday-uk/4-Using-spir-v-with-spirv-cross.pdf

PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:


2164: [mtl] Borrowed commands r=grovesNL a=kvark

PR checklist:
- [ ] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [x] tested examples with the following backends:

r? @gfx-rs/metallists 

This PR attempts to have lightweight software commands that don't take any heap space or own ObjC objects. In most cases, where a command list is live-recorded and executed once, this should reduce the amount of work we do per command, which is especially important if those commands are thrown away (e.g. because we are not inside a render pass).

My expectation would be to see an improvement in #2161 due to us doing less work. The actual results are somewhat shocking: with v-sync enabled I'm getting the same 59-60 fps as usual. With v-sync OFF, I'm getting between 25 and 50 fps now (which is lower than the previous 50-70). Not sure what's going on, the instrumental profile doesn't give a clue. Please check out the code.

2166: Update example instructions in README.md r=kvark a=king6cong



Co-authored-by: Hal Gentz <[email protected]>
Co-authored-by: Dzmitry Malyshau <[email protected]>
Co-authored-by: king6cong <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 22, 2018

Build failed (retrying...)

bors bot added a commit that referenced this pull request Jun 22, 2018
2071: Remaping descriptor sets in the gl backend r=kvark a=ZeGentzy

I'll rebase off master when I'm done.

Uniforms in gl only have a bindings field, not a set one. This means that for shaders that use multiple sets to work, we must change where we are binding them.

See page 14 for what I mean: https://www.khronos.org/assets/uploads/developers/library/2016-vulkan-devday-uk/4-Using-spir-v-with-spirv-cross.pdf

PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:


Co-authored-by: Hal Gentz <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 22, 2018

Build failed

@kvark
Copy link
Member

kvark commented Jun 23, 2018

bors r+

@goddessfreya
Copy link
Contributor Author

@kvark Should be working now.

bors bot added a commit that referenced this pull request Jun 23, 2018
2071: Remaping descriptor sets in the gl backend r=kvark a=ZeGentzy

I'll rebase off master when I'm done.

Uniforms in gl only have a bindings field, not a set one. This means that for shaders that use multiple sets to work, we must change where we are binding them.

See page 14 for what I mean: https://www.khronos.org/assets/uploads/developers/library/2016-vulkan-devday-uk/4-Using-spir-v-with-spirv-cross.pdf

PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:


2149: [dx11] add memory flush/invalidate & image/buffer copies r=kvark a=fkaa

Main changes are adding more robust implementation of `Memory`, adding flush/invalidate and adding image/buffer copies. Implements `Memory` like the following (for `HOST_VISIBLE` memory):

```
    0.........................size
    +----------------------------+
    |          Memory            |
    +----------------------------+
    A..B  C.....D         E...F

    1 fixed-size `STAGING` buffer which gets used for reading back from
      resources.(and should be used to copy from/to on flush/invalidate):
      (0..size, ComPtr<Buffer>)

    1 `Vec<u8>` which covers the whole memory range (0..size). This is
      pointer we hand out to users. flush/invalidate moves the affected
      regions into our `STAGING` buffer to get read/uploaded.

    *N* Resources:
      (A..B, ComPtr<Resource>),
      (C..D, ComPtr<Resource>),
      (E..F, ComPtr<Resource>),
```

Implements copying between images and buffers. Image<->Image copies are mostly handled by `CopySubresourceRegion` but some formats, while same size, cant be copied with this method:

> Cannot invoke CopySubresourceRegion when the Formats of each Resource are not the same or at least castable to each other, unless one format is compressed (DXGI_FORMAT_R9G9B9E5_SHAREDEXP, or DXGI_FORMAT_BC[1,2,3,4,5]_* ) and the source format is similar to the dest according to: BC[1|4] ~= R16G16B16A16|R32G32, BC[2|3|5] ~= R32G32B32A32, R9G9B9E5_SHAREDEXP ~= R32. [ RESOURCE_MANIPULATION ERROR #281: ]

These has to be done through compute shaders instead. Image->Buffer & Buffer->Image copies also have to be done through compute shaders, as `CopySubresourceRegion` can only copy between resources of same type (Image<->Image, Buffer<->Buffer). The following formats have Buffer->Image and Image->Buffer copies implemented with these changes:

* `R8`
* `Rg8`
* `R16`
* `Rg16`
* `R32`

Gets about 400 tests passed and equal amount failed in `dEQP-VK.api.copy_and_blit.core.image_to_image.all_formats.color` (mostly because `CopySubresourceRegion` failing to copy between some formats as mentioned above)

Fixes #issue
PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:


2154: hal: Improve buffer documentation and cleanup error handling r=kvark a=msiglreith

Fixes #issue
PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:


Co-authored-by: Hal Gentz <[email protected]>
Co-authored-by: Felix Kaaman <[email protected]>
Co-authored-by: msiglreith <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 23, 2018

@bors bors bot merged commit d7e0676 into gfx-rs:master Jun 23, 2018
@goddessfreya goddessfreya deleted the desc-remaping-gl branch June 23, 2018 20:34
bors bot added a commit that referenced this pull request Jul 21, 2018
2072: [WIP] A third gfx example r=kvark a=ZeGentzy

Depends on #2071 and #2052 
PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:


Co-authored-by: Hal Gentz <[email protected]>
@kvark kvark mentioned this pull request May 29, 2019
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.

3 participants