[ll][WIP] Gpu, Device and handles for render #1467

Bastacyclop · 2017-09-02T16:54:30Z

Working on #1466, starting with a fresh cleanup() implementation.
I commented out a lot of code to be able to compile it.

msiglreith

That was quick! Looking forward to future progress!
A few general points early on.

msiglreith · 2017-09-02T17:09:10Z

src/render/src/handle.rs

+macro_rules! resource_handle {
+    ($handle:ident, $guard:ident, $resource:ident) => {
+        #[derive(Clone, Debug)]
+        struct $guard<B: Backend>(Option<B::$resource>, GarbageSender<B>);


Would you mind explaining why we need Option<_> here?

I need ownership on Drop to send the resource on the garbage channel for future cleanup.

msiglreith · 2017-09-02T17:10:50Z

src/render/src/lib.rs

+
+pub struct Gpu<B: Backend> {
+    device: Device<B>,
+    queue: CommandQueue<B, core::General>,


should be General or Graphics. OpenGL backend will only expose graphics queues if there is no compute support,
Else I think every backend exposes general but need to double-check.

Would a third Compute-only alternative be useful for some use cases ?

I would say barely

msiglreith · 2017-09-02T17:12:13Z

src/render/src/lib.rs

+            .iter()
+            .map(|&(ref family, qtype)| (family, qtype, family.num_queues()) )
+            .collect::<Vec<_>>();
+


Somehow we need to filter for queues which can be used for presentation with the display surface.

might be easier with #1468

msiglreith · 2017-09-02T17:13:42Z

src/render/src/lib.rs

+    }
+}
+
+impl<B: Backend> ops::Deref for Gpu<B> {


We might rather expose explicit get_device/ref_device/..(?) functions than Deref.

This will become a bit more verbose for users that are not actually using the Device split, but I agree that the use of Deref might be a bit abusive here.

kvark

Nice start!

kvark · 2017-09-03T03:19:04Z

src/render/src/device.rs

+
+#[derive(Clone)]
+pub struct Device<B: Backend> {
+    pub(crate) device: B::Device,


convention would probably be to name it raw

kvark · 2017-09-03T03:19:20Z

src/render/src/device.rs

+pub struct Device<B: Backend> {
+    pub(crate) device: B::Device,
+    // TODO: could be shared instead of cloned
+    pub(crate) heap_types: Vec<HeapType>,


why do all of them need to be pub(crate)?

It was mainly to avoid writing a redundant Device::new but I can remove them.

kvark · 2017-09-03T03:19:34Z

src/render/src/device.rs

-
-impl<B: Backend, F: Device<B>> DeviceExt<B> for F {}
+    */
+}


nit: newline

kvark · 2017-09-03T03:22:43Z

src/render/src/handle.rs

+        }
+
+        #[derive(Clone, Debug)] // TODO: PartialEq, Eq, Hash
+        pub struct $handle<B: Backend>(Arc<$guard<B>>);


we might need more stuff carried out by the handle, e.g. stuff like BufferInfo, ProgramInfo, etc

kvark · 2017-09-03T03:23:14Z

src/render/src/handle.rs

+resource_handle! { ConstantBufferView, ConstantBufferViewGuard, ConstantBufferView }
+resource_handle! { ShaderResourceView, ShaderResourceViewGuard, ShaderResourceView }
+resource_handle! { UnorderedAccessView, UnorderedAccessViewGuard, UnorderedAccessView }
+resource_handle! { Sampler, SamplerGuard, Sampler }


please configure your editor to add new lines automatically

kvark · 2017-09-03T03:31:38Z

src/render/src/handle.rs

+    }
+}
+
+resource_handle! { ShaderLib, ShaderLibGuard, ShaderLib }


The macro parameters should be different, i.e. {Image, ImageInfo}.
The 3rd parameter matches the 1st, so no need to provide it.
The 2nd parameter could be avoided if we have mod guard { //all the guards with the same name as their resources, so that guard::Image is the one being a part of Image handle. This may require specifying all the handle types in a single macro invocation, so that you can open and close the guard mod once.

msiglreith · 2017-09-03T13:04:47Z

src/render/src/lib.rs

+        for garbage in self.garbage.try_iter() {
+            use handle::Garbage::*;
+            match garbage {
+                ShaderLib(sl) => dev.destroy_shader_lib(sl),


We lack documentation for the core api so you probably aren't aware of it but resources need to externally synchronized (in vulkan terms) on destruction. This means that they (or derivates like image views) must not be in flight anymore/processed by the GPU on destruction. This requires fencing to create a sync point between CPU-GPU. To hide costs and reduce blocking by fences the usual approach is to have basically a ringbuffer of 'frames' (e.g size of 3). The idea then would be to re-use existing sync points caused by fences from these frames and cleanup at frame boundaries with a bit of delay: e.g queue up for destruction at frame 0, waiting until beginning of the next frame 0 to destroy. This gives the GPU 2 frames time to catch up and signal frame 0 fence.
The concept extends to command buffers/pools as well. In our quad example we wait until the GPU idles after every frame which is slow but currently more a testing ground than best practice.

materials:

http://developer.download.nvidia.com/gameworks/events/GDC2016/Vulkan_Essentials_GDC16_tlorach.pdf (slide 25, graphical visualization)

https://www.khronos.org/assets/uploads/developers/library/2016-siggraph/3D-BOF-SIGGRAPH_Jul16.pdf (slide 54, example approach from UE4)

Thanks for the explanation. I think holding resources handles until synchronisation has been done would be a good way to ensure that cleanup is not done early.

kvark · 2017-09-04T01:59:55Z

src/render/src/handle.rs

+macro_rules! define_resources {
+    ($($name:ident: $info:path,)*) => {
+        pub enum Garbage<B: Backend> {
+            $( $name(B::$name), )*


kvark · 2017-09-04T02:00:43Z

src/render/src/handle.rs

+            $( $name(B::$name), )*
+        }
+
+        pub mod inner {


does it have to be pub?

Since I'm using a type alias for the handle and not a wrapper, I think so. It also allows to call resource() and info() without any boilerplate on our side. It's the equivalent of the old buffer::Raw for example.

kvark · 2017-09-04T02:01:42Z

src/render/src/handle.rs

+
+        pub mod raw {
+            use std::sync::Arc;
+            $( pub type $name<B> = Arc<super::inner::$name<B>>; )*


kvark · 2017-09-04T02:03:16Z

src/render/src/lib.rs

+            .iter()
+            .map(|&(ref family, qtype)| (family, qtype, family.num_queues()) )
+            .collect::<Vec<_>>();
+


might be easier with #1468

kvark · 2017-09-04T02:04:03Z

src/render/src/lib.rs

+            heap_types,
+            memory_heaps,
+            ..
+        } = adapter.open(&queue_descs);


note: there is open_with, which is a bit shorter

Bastacyclop · 2017-09-04T13:43:34Z

@kvark I think it's ready for merge unless you'd rather have me continue on this PR. The queue filtering can be resolved later IMO (after #1468 is merged ?).

kvark

great start, thank you!

Bastacyclop added the module: render label Sep 2, 2017

msiglreith reviewed Sep 2, 2017

View reviewed changes

kvark reviewed Sep 3, 2017

View reviewed changes

kvark changed the title ~~[WIP] Gpu, Device and handles for render~~ [ll][WIP] Gpu, Device and handles for render Sep 3, 2017

msiglreith reviewed Sep 3, 2017

View reviewed changes

Bastacyclop force-pushed the ll branch from 0228cd3 to 6f509a2 Compare September 3, 2017 15:10

kvark approved these changes Sep 4, 2017

View reviewed changes

[ll] Gpu, Device and handles for render

6dd6cce

Bastacyclop force-pushed the ll branch from 6f509a2 to 6dd6cce Compare September 4, 2017 13:38

kvark approved these changes Sep 4, 2017

View reviewed changes

kvark merged commit 39c7be5 into gfx-rs:ll Sep 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ll][WIP] Gpu, Device and handles for render #1467

[ll][WIP] Gpu, Device and handles for render #1467

Bastacyclop commented Sep 2, 2017 •

edited

Loading

msiglreith left a comment

msiglreith Sep 2, 2017

Bastacyclop Sep 2, 2017

msiglreith Sep 2, 2017

Bastacyclop Sep 2, 2017

msiglreith Sep 2, 2017

msiglreith Sep 2, 2017

kvark Sep 4, 2017

msiglreith Sep 2, 2017

Bastacyclop Sep 2, 2017

kvark left a comment

kvark Sep 3, 2017

kvark Sep 3, 2017

Bastacyclop Sep 3, 2017

kvark Sep 3, 2017

kvark Sep 3, 2017

kvark Sep 3, 2017

kvark Sep 3, 2017

msiglreith Sep 3, 2017 •

edited

Loading

Bastacyclop Sep 3, 2017

kvark Sep 4, 2017

kvark Sep 4, 2017

Bastacyclop Sep 4, 2017

kvark Sep 4, 2017

kvark Sep 4, 2017

kvark Sep 4, 2017

Bastacyclop commented Sep 4, 2017

kvark left a comment

[ll][WIP] Gpu, Device and handles for render #1467

[ll][WIP] Gpu, Device and handles for render #1467

Conversation

Bastacyclop commented Sep 2, 2017 • edited Loading

msiglreith 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

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

msiglreith Sep 3, 2017 • edited Loading

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

Bastacyclop commented Sep 4, 2017

kvark left a comment

Choose a reason for hiding this comment

Bastacyclop commented Sep 2, 2017 •

edited

Loading

msiglreith Sep 3, 2017 •

edited

Loading