linebender / vello

A GPU compute-centric 2D renderer.
http://linebender.org/vello/
Apache License 2.0
2.36k stars 136 forks source link

Metal validation errors when running piet-gpu in the Xcode debugger #166

Closed vincentisambart closed 1 year ago

vincentisambart commented 2 years ago

When trying to run piet-gpu in the Xcode debugger, I got the following error:

validateComputeFunctionArguments:745: failed assertion `Compute Function(main0): missing buffer binding at index 25 for spvBufferSizeConstants[0].'

And indeed if you look at for example binning.msl, you have a parameter constant uint* spvBufferSizeConstants [[buffer(25)]] even though there doesn't seem to be any buffer assigned to 25 in metal.rs's fn dispatch(). Having this fixed would make investigating problems on macOS easier.

As I asked on zulip, it seems to be related to the use of the --msl-decoration-binding flag passed to SPIRV-Cross.

By the way here's a step-by-step way to run the winit app in the Xcode debugger:

vincentisambart commented 2 years ago

Based on the zulip conversation, and looking at the SPIRV-Cross and MoltenVK source code, I made up a quick and dirty change to fn dispatch() and rerun the app in the Xcode debugger. I was then told by Xcode that a framebuffer-only texture should not be used as the destination of a texture copy so I also fixed that, and with these 2 changes no more validation error in Xcode. Here's the diff:

diff --git a/piet-gpu-hal/src/metal.rs b/piet-gpu-hal/src/metal.rs
index e3157d4..0c3b1a1 100644
--- a/piet-gpu-hal/src/metal.rs
+++ b/piet-gpu-hal/src/metal.rs
@@ -136,6 +136,7 @@ impl MtlInstance {
             let () = msg_send![ns_view, setWantsLayer: YES];
             let bounds: CGRect = msg_send![ns_view, bounds];
             let () = msg_send![metal_layer, setFrame: bounds];
+            metal_layer.set_framebuffer_only(false);

             if !ns_window.is_null() {
                 let scale_factor: CGFloat = msg_send![ns_window, backingScaleFactor];
@@ -464,6 +465,13 @@ impl crate::backend::CmdBuf<MtlDevice> for CmdBuf {
             encoder.set_texture(img_ix, Some(&image.texture));
             img_ix += 1;
         }
+        const BUFFER_SIZE_BUFFER_INDEX: usize = 25;
+        let buffer_size_constants: [u32; 1] = [0];
+        encoder.set_bytes(
+            BUFFER_SIZE_BUFFER_INDEX as NSUInteger,
+            mem::size_of_val(&buffer_size_constants) as NSUInteger,
+            &buffer_size_constants as *const u32 as _,
+        );
         let workgroup_count = metal::MTLSize {
             width: workgroup_count.0 as u64,
             height: workgroup_count.1 as u64,

Note that even without validation error, on a M1 Mac you still get incorrect colors, so #154 still needs to be fixed.

In regards to the diff above, I'm pretty sure set_framebuffer_only(false) is needed (as I also needed it when investigating #154), at least with the current way the texture is handled. Binding a buffer to 25 is also needed, but you might want to only do it in the pipelines that require it. And looking at MoltenVK it seems -[MTLComputeCommandEncoder setBytes:length:atIndex:] might not work on some old GPUs (though these GPUs might already not be supported by piet-gpu). Also I don't fully understand what that value is needed for, so is really 0 the correct value?

vincentisambart commented 1 year ago

With vello now using WGPU as a backend this is not relevant anymore. And just in case I tried running the current winit example in the Xcode debugger and it did not trigger any assertions, so closing this.