Skip to content

DX12: Align copies b/w textures and buffers when D3D12_FEATURE_DATA_D3D12_OPTIONS13.UnrestrictedBufferTextureCopyPitchSupported is false#7721

Merged
ErichDonGubler merged 6 commits intogfx-rs:trunkfrom
erichdongubler-mozilla:dx12-aligned-texbuf-copy-offset
Oct 14, 2025
Merged

Conversation

@ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented May 23, 2025

Connections

Testing

  • @teoxoy and I proved on some custom DLLs copied from an old Windows machine that this works, both on older WARP adapters and when forcefully done in updated DX12 runtimes. So, we're definitely not breaking anything! It's hard to set up an environment to specifically test this, though.

Squash or Rebase?

rebase plz

Checklist

  • If this contains user-facing changes, add a CHANGELOG.md entry.

@ErichDonGubler

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler marked this pull request as ready for review June 13, 2025 07:50
@ErichDonGubler ErichDonGubler requested a review from a team as a code owner June 13, 2025 07:50
@ErichDonGubler ErichDonGubler force-pushed the dx12-aligned-texbuf-copy-offset branch from 60d2d96 to 505438d Compare June 13, 2025 07:53
@teoxoy

This comment was marked as resolved.

@ErichDonGubler

This comment was marked as resolved.

@cwfitzgerald

This comment was marked as resolved.

@ErichDonGubler
Copy link
Member Author

Got the CI issue resolved! I was definitely doing something wrong. 👉🏻👈🏻 From community chat:

@ErichDonGubler:

I just paired with Teo, and we actually FIGGERED IT OUT!

@teoxoy and I sideloaded a bunch of DLLs from an ancient Windows 10 computer I had, which happened to have a WARP adapter that did not support low-alignment buffer-to-texture copies.

...turns out I accidentally swapped the alignment requirement branches. 😅

We had to work around:

  • Not having the right DLLs (because new versions have lower alignment needs) and stealing them from an old machine I happened to have around.
  • Crashing when trying to compile indirect buffer validation shaders (because old versions?)
  • Feature unification working against us when trying to use WGPU_VALIDATION_INDIRECT_CALL=0
  • Erich being dumb and necessitating debugging in the first place

It was an hours-long adventure, lemmetellyawhat.

@teoxoy:

It was fun :)

@cwfitzgerald:

oh boy!

that sounds like an adventure indeed

@ErichDonGubler ErichDonGubler force-pushed the dx12-aligned-texbuf-copy-offset branch 3 times, most recently from 335c9ab to fd850ff Compare July 25, 2025 21:16
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.

Looks good overall but we should also enable some CTS tests like the one in #5285.

@ErichDonGubler
Copy link
Member Author

Doing some branch pushes to check that webgpu:api,operation,command_buffer,image_copy:undefined_params:* fails before and after, and hopefully that it catches the bug @teoxoy found with region offsets being swapped on the tex-to-buf path. 🤞🏻

@ErichDonGubler
Copy link
Member Author

Alright, so:

I'll figure out the exact set of CTS tests to use tomorrow. If nothing else, I can use a (verbosely requested) subset of webgpu:api,operation,command_buffer,image_copy:undefined_params:*.

@ErichDonGubler ErichDonGubler requested a review from teoxoy October 9, 2025 01:13
@ErichDonGubler ErichDonGubler force-pushed the dx12-aligned-texbuf-copy-offset branch 3 times, most recently from 67c8472 to 78798fe Compare October 13, 2025 22:26
@ErichDonGubler ErichDonGubler force-pushed the dx12-aligned-texbuf-copy-offset branch 2 times, most recently from a78375d to 3f65b82 Compare October 14, 2025 12:59
@ErichDonGubler
Copy link
Member Author

I added baseline expectations (plus failures) for webgpu:api,operation,command_buffer,image_copy:undefined_params:* to our CTS list with 932567d. I have amended the current tip of the PR to remove fails-if(dx12) from some of the 2d variants of the added tests, thus proving that we fix something with this PR. Nice!

webgpu:api,operation,command_buffer,image_copy:undefined_params:initMethod="WriteTexture";checkMethod="FullCopyT2B";dimension="2d"
webgpu:api,operation,command_buffer,image_copy:undefined_params:initMethod="WriteTexture";checkMethod="FullCopyT2B";dimension="3d"
webgpu:api,operation,command_buffer,image_copy:undefined_params:initMethod="WriteTexture";checkMethod="PartialCopyT2B";dimension="1d"
fails-if(dx12) webgpu:api,operation,command_buffer,image_copy:undefined_params:initMethod="WriteTexture";checkMethod="PartialCopyT2B";dimension="2d"
Copy link
Member Author

Choose a reason for hiding this comment

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

It's odd that webgpu:api,operation,command_buffer,image_copy:undefined_params:initMethod="WriteTexture";checkMethod="PartialCopyT2B";dimension="2d" still fails after this PR's fix. I suspect there's more work to do here to make this correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, it seems the full path that's failing is a single subtest exercising a 3rd dimension for the "2d" case: copySize=[3,3,2];origin=["_undef_","_undef_","_undef_"];bytesPerRow=256;rowsPerImage=3

Definitely follow-up work for something more edge-casey. Whew!

@ErichDonGubler ErichDonGubler force-pushed the dx12-aligned-texbuf-copy-offset branch from 3f65b82 to 20d31b6 Compare October 14, 2025 13:28
@ErichDonGubler ErichDonGubler enabled auto-merge (rebase) October 14, 2025 13:28
Comment on lines +89 to +92
#### DX12

- Align copies b/w textures and buffers via a single intermediate buffer per copy when `D3D12_FEATURE_DATA_D3D12_OPTIONS13.UnrestrictedBufferTextureCopyPitchSupported` is `false`. By @ErichDonGubler in [#7721](https://github.com/gfx-rs/wgpu/pull/7721).

Copy link
Member Author

Choose a reason for hiding this comment

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

onuts, I almost forgot to move this out of the previous release section.

@cwfitzgerald: I'm strongly considering experimenting with adding some CI that fails a PR that adds things to the CHANGELOG outside of the Unreleased section, unless opted out by a label. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we don't have to make it a required check, either. It can simply be a signal that something might be wrong, at first.

@ErichDonGubler ErichDonGubler merged commit 54ded60 into gfx-rs:trunk Oct 14, 2025
41 checks passed
@ErichDonGubler ErichDonGubler deleted the dx12-aligned-texbuf-copy-offset branch October 14, 2025 13:39
@torokati44
Copy link
Contributor

Oh, is a backport of this to the 25/26/27 series, and corresponding patch releases feasible? 👀 It certainly would be appreciated IMHO!

@ErichDonGubler
Copy link
Member Author

@torokati44: Two things:

  1. For optional fixes like this, we generally expect the community to handle backporting effort (which should be light here, since you can omit the test coverage). So, if you're willing, then we look forward to a PR from you against v26 and v27. 😄
  2. In our maintainers' meeting today, we know we're willing to field backports to v26 (esp. for Bevy) and v27. Was there a reason you wanted to backport to v25, specifically?

@torokati44
Copy link
Contributor

@ErichDonGubler:

  1. Alright, I'll see what I can do w.r.t backporting PRs.

  2. No, not really, anymore, since chore: Update wgpu to 26, egui to match ruffle-rs/ruffle#21730 got merged (but not yet chore: Update wgpu to 27, egui to 0.33 ruffle-rs/ruffle#21895)

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.

[d3d12] Panic in end_encoding

4 participants