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.

Sign up for GitHub

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

[backdrop_dyn] Handle upstream pipeline failure #553

Merged
merged 1 commit into from
May 9, 2024
Merged

[backdrop_dyn] Handle upstream pipeline failure #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions crates/shaders/src/cpu/backdrop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// Copyright 2023 the Vello Authors
// SPDX-License-Identifier: Apache-2.0 OR MIT OR Unlicense

use vello_encoding::{ConfigUniform, Path, Tile};
use vello_encoding::{BumpAllocators, ConfigUniform, Path, Tile};

use super::CpuBinding;

fn backdrop_main(config: &ConfigUniform, paths: &[Path], tiles: &mut [Tile]) {
fn backdrop_main(config: &ConfigUniform, _: &BumpAllocators, paths: &[Path], tiles: &mut [Tile]) {
for drawobj_ix in 0..config.layout.n_draw_objects {
let path = paths[drawobj_ix as usize];
let width = path.bbox[2] - path.bbox[0];
Expand All @@ -24,7 +24,8 @@ fn backdrop_main(config: &ConfigUniform, paths: &[Path], tiles: &mut [Tile]) {

pub fn backdrop(_n_wg: u32, resources: &[CpuBinding]) {
let config = resources[0].as_typed();
let paths = resources[1].as_slice();
let mut tiles = resources[2].as_slice_mut();
backdrop_main(&config, &paths, &mut tiles);
let bump = resources[1].as_typed();
let paths = resources[2].as_slice();
let mut tiles = resources[3].as_slice_mut();
backdrop_main(&config, &bump, &paths, &mut tiles);
}
17 changes: 16 additions & 1 deletion shader/backdrop_dyn.wgsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,20 @@

// Prefix sum for dynamically allocated backdrops

#import bump
#import config
#import tile

@group(0) @binding(0)
var<uniform> config: Config;

@group(0) @binding(1)
var<storage> paths: array<Path>;
var<storage, read_write> bump: BumpAllocators;

@group(0) @binding(2)
var<storage> paths: array<Path>;

@group(0) @binding(3)
var<storage, read_write> tiles: array<Tile>;

let WG_SIZE = 256u;
Expand All @@ -26,6 +30,14 @@ fn main(
@builtin(global_invocation_id) global_id: vec3<u32>,
@builtin(local_invocation_id) local_id: vec3<u32>,
) {
// Abort if any of the prior stages failed.
if local_id.x == 0u {
sh_row_count[0] = atomicLoad(&bump.failed);
}
let failed = workgroupUniformLoad(&sh_row_count[0]);
if failed != 0u {
return;
}
let drawobj_ix = global_id.x;
var row_count = 0u;
if drawobj_ix < config.n_drawobj {
Expand All @@ -34,6 +46,9 @@ fn main(
sh_row_width[local_id.x] = path.bbox.z - path.bbox.x;
row_count = path.bbox.w - path.bbox.y;
sh_offset[local_id.x] = path.tiles;
} else {
// Explicitly zero the row width, just in case.
Copy link
Member

Choose a reason for hiding this comment

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

It isn't very clear to me what "just in case" here means.
I see that we're not doing a scan over sh_row_width, and so in theory we won't ever be reading this value.

To be clear, I think this change is fine - especially once we start to use gfx-rs/wgpu#5508.
But is this actually fixing an issue, or just programming defensively?

Copy link
Contributor

Choose a reason for hiding this comment

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

My analysis matches Daniel's. In any case, this code won't be around very long, I want to replace it with partition-wide prefix sum of the backdrop values, so being defensive seems preferable to over-optimizing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is indeed being defensive for the case where workgroup memory initialization might be turned off. The scan in the following lines is over sh_row_count but I think it's technically possible for the loop at the very bottom to read this sh_row_width value if el_ix happens to match local_id.x?

At any rate, I think I'll leave this in and land this as is.

sh_row_width[local_id.x] = 0u;
}
sh_row_count[local_id.x] = row_count;

Expand Down
2 changes: 1 addition & 1 deletion src/render.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ impl Render {
recording.dispatch(
shaders.backdrop,
wg_counts.backdrop,
[config_buf, path_buf, tile_buf],
[config_buf, bump_buf, path_buf, tile_buf],
);
recording.dispatch(
shaders.coarse,
Expand Down
2 changes: 1 addition & 1 deletion src/shaders.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ pub fn full_shaders(
);
let backdrop = add_shader!(
backdrop_dyn,
[Uniform, BufReadOnly, Buffer],
[Uniform, Buffer, BufReadOnly, Buffer],
CpuShaderType::Present(vello_shaders::cpu::backdrop)
);
let coarse = add_shader!(
Expand Down