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.

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

[core] Support optional ops in PassChannel #6716

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented Dec 12, 2024

Connections
Fixes #6700

Description
When attachment is read only, ops must not be provided (else validation error should be raised). To support this PassChannel is generic over L and S, so we can use same struct for LoadOp or Option<LoadOp>. Now core users will pass PassChannel<V, Option<LoadOp>, Option<StoreOp>> that will be be resolved to PassChannel<V, LoadOp, StoreOp>, which is used same as before internally. While resolving we also do validation.

Changes needed in servo: sagudev/servo@4aa142c.

Testing
CTS run in servo: #6716 (comment) and existing test.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
@sagudev sagudev changed the title Support optional ops in PassChannel [core] Support optional ops in PassChannel Dec 12, 2024
sagudev added a commit to sagudev/servo that referenced this pull request Dec 13, 2024
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
sagudev added a commit to sagudev/servo that referenced this pull request Dec 13, 2024
{"fail_fast": false, "matrix": [{"name": "WebGPU CTS", "workflow": "linux", "wpt_layout": "2020", "profile": "production", "unit_tests": false, "bencher": false, "wpt_args": "_webgpu"}]}
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
@sagudev
Copy link
Contributor Author

sagudev commented Dec 14, 2024

CTS:

OK /_webgpu/webgpu/cts.https.html?q=webgpu:api,validation,encoding,render_bundle:depth_stencil_readonly_mismatch:*

    PASS [expected FAIL] subtest: :depthStencilFormat="depth24plus-stencil8"
    PASS [expected FAIL] subtest: :depthStencilFormat="depth32float-stencil8"

OK /_webgpu/webgpu/cts.https.html?q=webgpu:api,validation,render_pass,attachment_compatibility:render_pass_or_bundle_and_pipeline,depth_stencil_read_only_write_state:*

    PASS [expected FAIL] subtest: :encoderType="render%20pass";format="stencil8"
    PASS [expected FAIL] subtest: :encoderType="render%20pass";format="depth16unorm"
    PASS [expected FAIL] subtest: :encoderType="render%20pass";format="depth32float"
    PASS [expected FAIL] subtest: :encoderType="render%20pass";format="depth24plus"
    PASS [expected FAIL] subtest: :encoderType="render%20pass";format="depth24plus-stencil8"
    PASS [expected FAIL] subtest: :encoderType="render%20pass";format="depth32float-stencil8"
    PASS [expected FAIL] subtest: :encoderType="render%20bundle";format="stencil8"
    PASS [expected FAIL] subtest: :encoderType="render%20bundle";format="depth16unorm"
    PASS [expected FAIL] subtest: :encoderType="render%20bundle";format="depth32float"
    PASS [expected FAIL] subtest: :encoderType="render%20bundle";format="depth24plus"
    And 2 more unexpected results...

OK /_webgpu/webgpu/cts.https.html?q=webgpu:api,validation,render_pass,render_pass_descriptor:depth_stencil_attachment,loadOp_storeOp_match_depthReadOnly_stencilReadOnly:*

    PASS [expected FAIL] subtest: :format="depth24plus-stencil8"
    PASS [expected FAIL] subtest: :format="depth32float-stencil8"

OK /_webgpu/webgpu/cts.https.html?q=webgpu:api,validation,resource_usages,texture,in_render_common:subresources,depth_stencil_attachment_and_bind_group:*

    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=0;bgLevelCount=1;bgLayer=1;bgLayerCount=1
    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=0;bgLevelCount=1;bgLayer=1;bgLayerCount=2
    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=1;bgLevelCount=1;bgLayer=0;bgLayerCount=1
    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=1;bgLevelCount=1;bgLayer=0;bgLayerCount=3
    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=1;bgLevelCount=1;bgLayer=1;bgLayerCount=1
    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=1;bgLevelCount=1;bgLayer=1;bgLayerCount=2
    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=1;bgLevelCount=2;bgLayer=0;bgLayerCount=1
    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=1;bgLevelCount=2;bgLayer=0;bgLayerCount=3
    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=1;bgLevelCount=2;bgLayer=1;bgLayerCount=1
    PASS [expected FAIL] subtest: :dsLevel=0;dsLayer=0;bgLevel=1;bgLevelCount=2;bgLayer=1;bgLayerCount=2
    And 23 more unexpected results...

OK /_webgpu/webgpu/cts.https.html?q=webgpu:api,validation,resource_usages,texture,in_render_misc:subresources,set_bind_group_on_same_index_depth_stencil_texture:*

    PASS [expected FAIL] subtest: :bindAspect="depth-only";depthStencilReadOnly=true
    PASS [expected FAIL] subtest: :bindAspect="stencil-only";depthStencilReadOnly=true

@sagudev sagudev marked this pull request as ready for review December 14, 2024 11:40
@sagudev sagudev requested review from crowlKats and a team as code owners December 14, 2024 11:40
Comment on lines -938 to -947
view.same_device(device)?;
check_multiview(view)?;
add_view(view, AttachmentErrorLocation::Depth)?;

let ds_aspects = view.desc.aspects();
if ds_aspects.contains(hal::FormatAspects::COLOR) {
return Err(RenderPassErrorInner::InvalidDepthStencilAttachmentFormat(
view.desc.format,
));
}
Copy link
Contributor Author

@sagudev sagudev Dec 14, 2024

Choose a reason for hiding this comment

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

This checks are now part of creation, in a followup I will move even more checks, see #6735 for more info.

Comment on lines +1478 to +1487
depth: if format.has_depth_aspect() {
depth_stencil_attachment.depth.resolve()?
} else {
Default::default()
},
stencil: if format.has_stencil_aspect() {
depth_stencil_attachment.stencil.resolve()?
} else {
Default::default()
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is rather unfortunate, but I didn't want to touch RenderPassInfo::start to much (at least not in this PR).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it would be nice to make some rusty types for these after validation.

@teoxoy teoxoy self-assigned this Dec 16, 2024
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Nice!

@teoxoy teoxoy merged commit 4da7c26 into gfx-rs:trunk Dec 16, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] PassChannel is not expresive enough to match spec
2 participants