[mtl] dynamic depth bias, callback coalescence, and more stats #2224
Conversation
src/backend/dx12/src/conv.rs
Outdated
| @@ -150,6 +150,11 @@ pub fn map_rasterizer(rasterizer: &pso::Rasterizer) -> D3D12_RASTERIZER_DESC { | |||
| use hal::pso::PolygonMode::*; | |||
| use hal::pso::FrontFace::*; | |||
|
|
|||
| let bias = match rasterizer.depth_bias { //TODO | |||
| @@ -20,11 +20,11 @@ pub fn bind_polygon_mode(gl: &gl::Gl, mode: pso::PolygonMode, bias: Option<pso:: | |||
| unsafe { gl.PolygonMode(gl::FRONT_AND_BACK, gl_draw) }; | |||
|
|
|||
| match bias { | |||
| Some(bias) => unsafe { | |||
| Some(pso::State::Static(bias)) => unsafe { | |||
There was a problem hiding this comment.
Isn't it possible for a dynamic depth bias to have been set at this point?
Otherwise, where would the dynamic depth bias be set?
There was a problem hiding this comment.
Yeah, the problem is that we haven't had a dynamic depth bias exposed at all, and I want to limit the scope of this PR to providing this in API and implementing in Metal and Vulkan only. Other backends are not regressed, they will just need to catch up.
|
@grovesNL I improved those TODO comments now |
| frame: usize, | ||
| } | ||
|
|
||
|
|
||
| pub struct CommandQueue { | ||
| shared: Arc<Shared>, | ||
| frame_wait: Option<FrameWaitReport>, | ||
| retained_buffers: Vec<metal::Buffer>, |
There was a problem hiding this comment.
How do the retained resources work with Send/Sync here? i.e. how do we know multiple threads aren't adding or removing retained resources at the same time?
| @@ -134,6 +134,7 @@ impl Swapchain { | |||
| } | |||
| hal::FrameSync::Fence(fence) => { | |||
| *fence.mutex.lock().unwrap() = true; | |||
| fence.condvar.notify_all(); | |||
There was a problem hiding this comment.
Could you explain this change a bit more? I'm trying to understand the synchronization changes. Was this a bug previously, or is it needed because of the changes in submit_raw?
|
Retained stuff of a queue is only appended in &mut self methods, so there is no send/sync concern.
The condvar signalling was just missing afaik.
… On Jul 14, 2018, at 11:27, Josh Groves ***@***.***> wrote:
@grovesNL commented on this pull request.
In src/backend/metal/src/command.rs:
> frame: usize,
}
pub struct CommandQueue {
shared: Arc<Shared>,
- frame_wait: Option<FrameWaitReport>,
+ retained_buffers: Vec<metal::Buffer>,
How do the retained resources work with Send/Sync here? i.e. how do we know multiple threads aren't adding or removing retained resources at the same time?
In src/backend/metal/src/window.rs:
> @@ -134,6 +134,7 @@ impl Swapchain {
}
hal::FrameSync::Fence(fence) => {
*fence.mutex.lock().unwrap() = true;
+ fence.condvar.notify_all();
Could you explain this change a bit more? I'm trying to understand the synchronization changes. Was this a bug previously, or is it needed because of the changes in submit_raw?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I believe the concerns are addressed, and we can revisit the code if new ones rise up. |
2224: [mtl] dynamic depth bias, callback coalescence, and more stats r=grovesNL a=kvark Fixes #2170 Expands our Metal stats to report the number of active and temporary command buffers. Coalesces the completion handlers, and finally fixes the depth bias issue we've been seeing (HAL breaking change). PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: Co-authored-by: Dzmitry Malyshau <[email protected]>
Fixes #2170
Expands our Metal stats to report the number of active and temporary command buffers. Coalesces the completion handlers, and finally fixes the depth bias issue we've been seeing (HAL breaking change).
PR checklist:
makesucceeds (on *nix)make reftestssucceeds