wgpu icon indicating copy to clipboard operation
wgpu copied to clipboard

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

Open ErichDonGubler opened this issue 7 months ago • 2 comments

Connections

  • Likely resolves #5285.
  • A case we need to handle in order to fix #4150.

Testing

  • TODO: Will prove that this works even when forced to align. This won't definitively prove that we're fixing cases that need this, but it will prove that we're not breaking anything with intermediate buffers.

Squash or Rebase?

rebase plz

Checklist

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

ErichDonGubler avatar May 23 '25 21:05 ErichDonGubler

I'm...honestly not sure why this is failing on the wgpu_gpu::regression::issue_6827::test_scatter test entry in CI. I think I need some help. Marking as ready for review; perhaps @teoxoy or @cwfitzgerald could help me out?

ErichDonGubler avatar Jun 13 '25 07:06 ErichDonGubler

The test is failing because of:

Panic: GraphicsCommandList::close failed: The parameter is incorrect. (0x80070057)

I'd run the test locally with the validation layers enabled to see if there is more info. I've seen the error come up in cases where we didn't uphold some D3D12 invariant while encoding.

teoxoy avatar Jun 16 '25 09:06 teoxoy

@teoxoy and I spent some time enabling debug layers, and even forcing the same version of WARP that CI is using in the test that's failing with only a WARP adapter. We're not able to reproduce this locally on either of our machines. 😓

ErichDonGubler avatar Jul 24 '25 16:07 ErichDonGubler

I wonder if this is a windows 10 runtime thing. There's something going wrong in the runtime on Win10, but Win11's runtime deals with it fine. We would need to investigate setting up the Agility SDK

cwfitzgerald avatar Jul 24 '25 17:07 cwfitzgerald

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 avatar Jul 24 '25 20:07 ErichDonGubler

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 avatar Oct 09 '25 00:10 ErichDonGubler

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 avatar Oct 09 '25 01:10 ErichDonGubler

I added baseline expectations (plus failures) for webgpu:api,operation,command_buffer,image_copy:undefined_params:* to our CTS list with 932567d242135677e779e06303f48bca8f46c007. 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!

ErichDonGubler avatar Oct 14 '25 13:10 ErichDonGubler

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

torokati44 avatar Oct 14 '25 13:10 torokati44

@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?

ErichDonGubler avatar Oct 16 '25 15:10 ErichDonGubler

@ErichDonGubler:

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

  2. No, not really, anymore, since https://github.com/ruffle-rs/ruffle/pull/21730 got merged (but not yet https://github.com/ruffle-rs/ruffle/pull/21895)

torokati44 avatar Oct 17 '25 16:10 torokati44