[ll][WIP] Encoding and state transitions #1502

Bastacyclop · 2017-09-17T13:45:01Z

Working on #1466.
I'm not familiar with the concepts yet so tell me if I'm missing something.

kvark

This is great to see, awesome stuff @Bastacyclop !
PR is obviously WIP, and I'd like the API to change a bit. Looking forward to see it merged ;)

kvark · 2017-09-18T16:36:13Z

examples/render/quad_render/main.rs

+            image_extent: d::Extent { width, height, depth: 1 },
+        }]);
+
+    let init_submit = init_encoder.finish();


That's what I'm worried about: passing the encoder in the device creation doesn't make it safe: user is still responsible to actually submit it. So I think we should change to tokens right away:

resource creation returns a token

encoder can do init_resources(&[Token]), which consumes the tokens.

This is not safer but I believe is more universal.

That's also better for batching into a single barrier command :)

kvark · 2017-09-18T16:38:29Z

src/render/src/buffer.rs

+}
+
+impl<B: Backend> handle::raw::Buffer<B> {
+    pub fn stable_state(&self) -> core::buffer::State {


pub(crate) ?

kvark · 2017-09-18T16:40:28Z

src/render/src/device.rs

        usage: image::Usage,
        kind: image::Kind,
        mip_levels: image::Level,
        format: format::Format
    ) -> Result<handle::raw::Image<B>, image::CreationError>
-        where A: Allocator<B>
+        where A: Allocator<B>,
+              C: core::queue::Supports<core::Transfer>


uh, yeah, that certainly looks out of place here. Device shouldn't know anything about queues and their capabilities

kvark · 2017-09-18T16:42:01Z

src/render/src/encoder.rs

@@ -84,6 +90,41 @@ pub struct Submit<B: Backend, C> {
    pub(crate) pool: PoolDependency<B, C>
 }

+// TODO: coalescing?
+struct ImageStates {
+    states: Vec<core::image::State>,


We could probably just add the level/slice into the hashmap key instead of introducing this struct.
Do you think the struct gives us an advantage?

I was wondering if it would be worth it to coalesce the state slices instead of always having a state for each level/layer. Otherwise, I don't see any advantage.

Ok, let's have it this way for now, will reconsider later.

kvark · 2017-09-18T16:44:54Z

src/render/src/encoder.rs

+    ) {
+        let mut barriers = Vec::new();
+        for &(buffer, state) in buffer_states {
+            self.require_buffer_state(buffer, state)


nit: could be barriers.extend(self.require_buffer_state(...));

kvark · 2017-09-18T16:45:01Z

src/render/src/encoder.rs

-                           info.zoffset + info.depth]
-            });
+        for &(image, (level, layer), state) in image_states {
+            self.require_image_state(image, level, layer, state)


kvark · 2017-09-18T16:45:16Z

src/render/src/encoder.rs

+        }
+        let current_stage = mem::replace(&mut self.pipeline_stage, stage);
+        if (current_stage != stage) || !barriers.is_empty() {
+            println!("barrier: {:?}, {:?}", current_stage..stage, &barriers[..]);


please change to debug!

So you will want to keep it ?

removing works too, I don't mind either way

kvark · 2017-09-18T16:46:46Z

src/render/src/image.rs

-    pub fn to_raw_image_info(&self, mip: Level) -> RawImageInfo {
-        self.to_image_info(mip).convert(self.format)
+impl<B: Backend> handle::raw::Image<B> {
+    pub fn stable_state(&self) -> core::image::State {


I think we should store this as a constant instead of recomputing

A constant like a field in Info, or something more global ?

yep, a field in Info would work

kvark · 2017-09-22T21:53:14Z

examples/render/quad_render/main.rs

@@ -36,6 +37,13 @@ const QUAD: [Vertex; 6] = [
    Vertex { a_Pos: [ -0.5,-0.33 ], a_Uv: [0.0, 0.0] },
 ];

+gfx_descriptor_struct! {


kvark · 2017-09-22T21:54:17Z

examples/render/quad_render/main.rs

-                stage_flags: pso::STAGE_FRAGMENT,
-            }
-        ],
+    gfx_allocate_descriptor_structs!(context.mut_device();


hmm, not sure why this has to be a macro?

Since you allocate a (potential) batch of descriptor structs, the result can consist of different types and cannot be a vector (unless dynamically typed). We could probably have some tricky tuple imbrication using a builder but it seems worse to use.

We could also come up with another descriptor allocation API, obviously. It seemed like an easy way to avoid explicit pools while still allowing some batching.

kvark · 2017-09-22T21:57:28Z

src/render/src/device.rs

+    #[doc(hidden)]
+    pub fn allocate_descriptor_sets(
+        &mut self,
+        sets_bindings: &[&[core::pso::DescriptorSetLayoutBinding]],


why is this a 2d array?

You can allocate multiple sets in a single batch, allowing to share the underlying descriptor pool.

msiglreith · 2017-09-27T20:53:43Z

examples/render/quad_render/main.rs

        let extent = d::Extent { width: pixel_width as _, height: pixel_height as _, depth: 1 };
-        device.create_framebuffer(&render_pass, &[frame_rtv], &[], extent)
+        device.create_framebuffer(pipeline.render_pass(), &[rtv.resource()], &[], extent)


I don't know if you have planned it but how difficult would you estimate to add custom renderpass support?

I didn't think about it yet, I'm focusing on getting simple pipelines working for this PR.

msiglreith · 2017-09-27T20:54:33Z

examples/render/quad_render/main.rs

-        },
-    ]);
+    context.mut_device().update_descriptor_sets()
+        .write(sample_desc.sampled_image(), 0, vec![&image_srv])


Why do we need vec![..] here?

msiglreith · 2017-09-27T20:59:03Z

examples/render/quad_render/main.rs

+        }
+
+        submits.push(encoder.finish());
+        context.present(mem::replace(&mut submits, Vec::new()));


hm, do we have alternatives here? drain(..) or sth like this?

msiglreith · 2017-09-27T20:59:17Z

examples/render/quad_render/main.rs

    }

    println!("cleanup!");
-    device.destroy_descriptor_pool(srv_pool);


praise the garbage collector \o/

msiglreith · 2017-09-27T21:02:00Z

src/render/src/macros.rs

+                        };
+
+                        // TODO:
+                        let dependency = cpass::SubpassDependency {


Vulkan's spec (and probably we also) implicitly add subpass dependencies from EXTERNAl -> firstSubpass and lastSubpass -> EXTERNAL, which should cover your use-case also. So this dependencies shouldn't be needed.

kvark

Looks like it's shaping up, great! The trait magic will need to be reviewed again carefully.

kvark · 2017-09-27T20:37:29Z

examples/render/quad_render/main.rs

+    let sample_desc = context.mut_device().create_descriptors(1).pop().unwrap();
+    let pipe_init = pipe::Init {
+        sample: &sample_desc,
+        color: pso::ColorInfo {


any reason not to use any of the draw_state::preset::blend things?

No reason, I simply kept the original code here.

kvark · 2017-09-27T20:38:10Z

examples/render/quad_render/main.rs

-        element: pso::Element {
-            format: <Vec2<f32> as Formatted>::get_format(),
-            offset: 0,
+    let sample_desc = context.mut_device().create_descriptors(1).pop().unwrap();


are the mut_device calls final or temporary?

We could do let mut device = context.ref_device().clone(); at the beginning.

kvark · 2017-09-27T20:55:21Z

examples/render/quad_render/main.rs

-            write: pso::DescriptorWrite::Sampler(vec![sampler.resource()]),
-        },
-    ]);
+    context.mut_device().update_descriptor_sets()


looks great 👍

kvark · 2017-09-27T20:55:44Z

examples/render/quad_render/main.rs

-        },
-    ]);
+    context.mut_device().update_descriptor_sets()
+        .write(sample_desc.sampled_image(), 0, vec![&image_srv])


not sure why you'd want to have vec![] here as opposed to &[] though

kvark · 2017-09-27T20:56:05Z

examples/render/quad_render/main.rs

@@ -344,15 +265,15 @@ fn main() {

            cmd_buffer.set_viewports(&[viewport]);
            cmd_buffer.set_scissors(&[scissor]);
-            cmd_buffer.bind_graphics_pipeline(&pipelines[0].as_ref().unwrap());
+            cmd_buffer.bind_graphics_pipeline(pipeline.pipeline());


uh? pipeline.pipeline doesn't sound good

Yeah that should not stay once encoder.draw is done :p

kvark · 2017-09-27T20:58:42Z

src/render/src/device.rs

+    {
+        use core::DescriptorPool as CDP;
+
+        let bindings = &D::layout_bindings()[..];


looks like a case for associated constants here?

Problem is I can't make a &'static slice without macro tricks to get the binding index (maybe easier with a custom derive ?).

kvark · 2017-09-27T20:59:08Z

src/render/src/device.rs

+
+        let bindings = &D::layout_bindings()[..];
+        let layout = self.create_descriptor_set_layout(bindings);
+        let mut ranges = Vec::new();


idiomatic way to do this would be let ranges = bindigns.iter().map(...).collect::<Vec<_>>()

kvark · 2017-09-27T21:05:21Z

src/render/src/pso/mod.rs

-        }
-    }
+pub trait Bind {
+    fn desc_type() -> core::pso::DescriptorType;


associated constant?

kvark

Good stuff!

kvark · 2017-10-05T12:18:06Z

examples/render/quad_render/main.rs

        sampled_image: gfx::pso::SampledImage,
        sampler: gfx::pso::Sampler,
    }
 }

 gfx_graphics_pipeline! {
    pipe {
-        sample: gfx::pso::DescriptorSet<SampleDesc<B>>, // TODO remove <B>
+        desc: gfx::pso::DescriptorSet<desc::Set<B>>, // TODO: improve


we still do need to get rid of B here since no other PSO component is backend dependent

The macro could generate a desc::Component, how does that sound ?

kvark · 2017-10-05T12:22:47Z

src/render/src/device.rs

+        let buffer = self.raw.create_framebuffer(
+            pipeline.render_pass(), &rtv_res[..], &dsv_res[..], extent);
+        let info = handle::FrameBufferInfo {
+            rtvs: rtvs.iter().map(|&rtv| rtv.clone()).collect(),


could just have used rtvs.iter().cloned().collect() ?

That would require cloned().cloned() because this is a &&RTV.

kvark · 2017-10-05T12:26:58Z

src/render/src/encoder.rs

-        self.command_buffer.bind_samplers(&self.raw_pso_data.samplers);
-        self.draw_slice(slice, slice.instances);
+        // TODO: instances
+        data.begin_renderpass(self, pipeline).draw(vertices, 0..1);


starting and ending a render pas for each draw call is not perfect.
I think one way we cold make this nicer is to have encoders behave in the same way they do in Metal (it is a nice API, no matter what political negativity is around it):

have command buffers visible to the user

user creates a graphics encoder, which starts a render pass given the targets, and borrows the command buffer mutably

user submits a bunch of gaphics/draw commands

user drops the encoder, which ends the render pass and releases the command buffer borrow, which can now be submitted

cc @msiglreith

I would be totally happy if we can get a Metal-ish API, but I also see why you went with the renderpass for each pipeline approach. Sounds like a hard problem due to required compatibility between framebuffer, pipeline and renderpass.
We might have to dynamically compile renderpasses (with single renderpass) and pipelines here to get this working or require the user to do it upfront or a mix of the both approaches.

The way I see it:

user declares a render pass using a macro, like the other things. The information required for the macro is exactly what Vulkan considers to not be a part of render pass compatibility.

the "compatible" pieces would then be supplied at run-time during render pass creation by the user

the pipeline definition would then point to a render pass definition, thus statically enforcing which pipelines are going to be accepted in the graphics encoder corresponding to the given render pass definition (not the instance)

Does it make sense?

Sounds reasonable even though I'm unsure if it's that easy to split the renderpass definition in compatible and non-compatible parts..

It would be nice but I think it deserves a separate PR.

that's fair

kvark · 2017-10-05T12:27:32Z

src/render/src/handle.rs

@@ -230,7 +231,7 @@ define_resources! {
    PipelineLayout: (),
    GraphicsPipeline: (),
    // ComputePipeline
-    // FrameBuffer
+    FrameBuffer: ::handle::FrameBufferInfo<B>,


nit: would be nicer to not use global imports

kvark · 2017-10-05T12:28:42Z

src/render/src/macros.rs

+                    let mut binding = 0;
+                    let mut next_binding = || {let b = binding; binding += 1; b };
+                    (Set {
+                        $( $field: next_binding(), )*


nit: heh, not sure what we are getting by moving next_binding() up as opposed to just inlining it here

we avoid getting a warning for the last unused += operation.

kvark · 2017-10-05T12:30:26Z

src/render/src/pso/mod.rs

-    fn desc_type() -> core::pso::DescriptorType;
-    fn desc_count() -> usize;
+pub trait BindDesc {
+    const TYPE: core::pso::DescriptorType;


Bastacyclop · 2017-10-05T17:01:55Z

src/render/src/encoder.rs

+        -> core::memory::Barrier<'b, B>
+    {
+        let creation_state = (core::image::Access::empty(), ImageLayout::Undefined);
+        let levels = image.info().kind.get_num_levels();


Looks like this does not give the actual image levels, should I use image.info().mip_levels instead ?.

I'd expect them to match, plus minus the differences between 0 and 1

The way I see it, kind.get_num_levels() is how many mip levels you should have if you had mip mapping while info().mip_levels is how many mip levels you really have (so it is valid even if you do not have mip mapping).

I think you just need to remove Info::mip_levels completely. Image Kind is the full description of what an image has in terms of dimensions/levels.

Do we agree that:

You can have images with arbitrary mip levels (1 most likely, like for backbuffers).

kind.get_num_levels() will not return this number, but rather (1..).find(|level| dominant>>level <= 1).unwrap(), which is like the maximum number of mip levels you can have.

And that we need the actual level count and not the maximum level count ?

@Bastacyclop Sorry, I got confused. Used the get_num_layers lately and assumed get_num_levels does a similar thing. You are correct, it doesn't return the actually stored mipmap count, this method can only be used for estimation during the image creation, and is rather pointless afterwards.

So in your case using image.info().mip_levels is the way to proceed.

kvark

There is quite a few rough points, we'll surely need to spend more time polishing this out. The core of the PR is good though, just a few things before we proceed.

kvark · 2017-10-06T14:01:10Z

examples/render/quad_render/main.rs

+        desc: &desc,
+        color: pso::ColorInfo {
+            mask: state::MASK_ALL,
+            color: Some(state::BlendChannel {


nit: let's use the presets. We can do as a follow-up

kvark · 2017-10-06T14:02:15Z

examples/render/quad_render/main.rs

+        vertices: (),
+    };
+    let pipeline = device.create_graphics_pipeline(
+        pso::GraphicsShaderSet {
            vertex: pso::EntryPoint { entry: "main", module: &vs_module },


you know what would be really cool? to write:

vertex: vs_module["main"],

kvark · 2017-10-06T14:03:50Z

examples/render/quad_render/main.rs


-    context.mut_device()
-        .write_mapping(&vertex_buffer, 0..vertex_count)
+    device.write_mapping(&vertex_buffer, 0..vertex_count)


I wonder if you can actually do anything with that buffer, including the mapping, before it's initialized. E.g. doesn't Vulkan require the HOST access to be set?

kvark · 2017-10-06T14:04:54Z

examples/render/quad_render/main.rs


    println!("copy image data into staging buffer");

-    if let Ok(mut image_data) = context.mut_device()
-        .write_mapping(&image_upload_buffer, 0..upload_size)
+    if let Ok(mut image_data) = device.write_mapping(&image_upload_buffer, 0..upload_size)
    {


nit: move to the end of the previous line

kvark · 2017-10-06T14:05:37Z

examples/render/quad_render/main.rs

-        },
-    ]);
+    device.update_descriptor_sets()
+        .write(desc_data.sampled_image(&desc), 0, &[&image_srv as _])


I wonder if we can get rid of those X as _ things?

Unfortunately type inference does not seem to go through without it.

kvark · 2017-10-06T14:07:28Z

src/backend/dx12/src/device.rs

@@ -726,7 +726,7 @@ impl d::Device<B> for Device {
        color_attachments: &[&n::RenderTargetView],
        depth_stencil_attachments: &[&n::DepthStencilView],
        _extent: d::Extent,
-    ) -> Result<n::FrameBuffer, d::FramebufferError> {
+    ) -> Result<n::FrameBuffer, d::FrameBufferError> {


Let's keep it the old way. I believe saying Framebuffer and Renderpass consistently across core/backends would be nicer

They are named FrameBuffer and RenderPass in the backend trait though.

Let's rename the backend trait types then. I believe we did discuss this naming issue recently and agreed upon the Vulkan-like naming, right @msiglreith ?

Note: we'll rename it outside of this PR. It's already big enough

kvark · 2017-10-06T14:09:22Z

src/backend/gl/src/native.rs

@@ -96,6 +96,8 @@ pub struct DescriptorSetLayout;
 #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
 pub struct DescriptorSet;

+#[allow(missing_copy_implementations)]


I find this one to be the most annoying warning of all. I'd prefer to just put it as a top-level thing. In fact, it is already:

gfx/src/backend/gl/src/lib.rs

Line 4 in ad34a1e

#![allow(missing_docs, missing_copy_implementations)]

kvark · 2017-10-06T14:09:58Z

src/backend/metal/src/device.rs

@@ -512,7 +512,7 @@ impl core::Device<Backend> for Device {
            let base_ptr = buf.0.contents() as *mut u8;

            if base_ptr.is_null() {
-                return Err(mapping::Error::InvalidAccess);
+                return Err(unimplemented!());


why unimplemented?

We had removed core::mapping::Error::InvalidAccess because it was originally for render, but it seems it came back with another meaning. Will put it back.

kvark · 2017-10-06T14:12:17Z

src/core/src/device.rs

-    // TODO: view_image_as_depth_stencil
+    ///
+    #[allow(unused_variables)]
+    fn view_image_as_depth_stencil(&mut self, image: &B::Image, format: format::Format, range: image::SubresourceRange) -> Result<B::DepthStencilView, TargetViewError> {


should use SubresourceLayers

kvark · 2017-10-06T14:13:28Z

src/render/src/device.rs

+        let origin = image::Origin::User(memory);
+        let stable_access = core::image::Access::empty();
+        let stable_layout = match usage {
+            _ if usage.contains(COLOR_ATTACHMENT) =>


interesting matching

kvark · 2017-10-06T14:46:09Z

@Bastacyclop Here is one thing I think we should change w.r.t. typing.

Currently, Typed trait implements Deref/DerefMut for the wrapped type. I believe this is semantically incorrect. You only need deref when you call methods on the wrapped object. In our case there are no methods - the contained stuff is just opaque resource handles.

The way you use Deref is to be able to pass it down to functions that don't care about Typed, hence you sometimes get type inference failing - it's not supposed to use Deref there.

Instead, let's have Typed implementing AsRef, and have all the accepting methods be generic over R: AsRef<T>, calling as_ref() in the body explicitly. This will be more ergonomic for the user (less X as _ casting, better error messages, etc). Also, perhaps this will eliminate buffer::MaybeTyped trait.

Bastacyclop · 2017-10-06T14:52:09Z

AsRef sounds good, but it won't eliminate MaybeTyped because the latter is mainly used to say that raw operations are expecting u8 data.

kvark · 2017-10-06T15:06:29Z

@Bastacyclop ok, so it could be defined as

pub trait MaybeTyped<B: Backend>: AsRef<handle::raw::Buffer<B>> {
    type Data: Pod;
}

kvark · 2017-10-06T15:28:24Z

Ok, the branch got accidentally pushed to ll by mistake, and that branch was not protected (it is now!).

kvark requested changes Sep 18, 2017

View reviewed changes

Bastacyclop force-pushed the ll branch 2 times, most recently from 0c53fca to 7803718 Compare September 20, 2017 15:53

kvark reviewed Sep 22, 2017

View reviewed changes

Bastacyclop force-pushed the ll branch from e468599 to 09eb82e Compare September 27, 2017 20:31

msiglreith reviewed Sep 27, 2017

View reviewed changes

kvark reviewed Sep 27, 2017

View reviewed changes

kvark approved these changes Oct 5, 2017

View reviewed changes

Bastacyclop commented Oct 5, 2017

View reviewed changes

Bastacyclop force-pushed the ll branch from 0823e5a to 098388f Compare October 5, 2017 19:39

Bastacyclop added 3 commits October 6, 2017 14:38

[ll] Encoding and state transitions

1d51537

[ll][render] Descriptors and Pipelines

12abca3

[ll][render] Encoder::draw

6c7c376

Bastacyclop force-pushed the ll branch from 098388f to 6c7c376 Compare October 6, 2017 13:49

kvark requested changes Oct 6, 2017

View reviewed changes

Bastacyclop merged commit 6c7c376 into gfx-rs:ll Oct 6, 2017

[ll][WIP] Encoding and state transitions #1502

[ll][WIP] Encoding and state transitions #1502

Conversation

Bastacyclop commented Sep 17, 2017

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment