Skip to content

hal/vulkan: Fix multi draw indirect emulating and respect maxDrawIndirectCount#8665

Merged
cwfitzgerald merged 2 commits intogfx-rs:trunkfrom
beicause:vk-fix-mdi
Dec 16, 2025
Merged

hal/vulkan: Fix multi draw indirect emulating and respect maxDrawIndirectCount#8665
cwfitzgerald merged 2 commits intogfx-rs:trunkfrom
beicause:vk-fix-mdi

Conversation

@beicause
Copy link
Contributor

@beicause beicause commented Dec 5, 2025

Description
Noted this when I was checking #8162

The offset argument of draw_indexed_indirect in hal/vulkan seems incorrect. It should + i * stride.
Also multi draw indirect should respect limits.maxDrawIndirectCount.

Actually, vkCmdDrawIndirectCount vkCmdDrawIndexedIndirectCount vkCmdDrawMeshTasksIndirectCountEXT also require that the count in the countBuffer must be <= maxDrawIndirectCount, altough on many devices it's u32::MAX(vulkan gpuinfo)

Testing
I assume the code is correct, I don't have a device to test this.

Squash or Rebase?
Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@cwfitzgerald cwfitzgerald self-assigned this Dec 10, 2025
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

I'm not @cwfitzgerald (the current assignee), but: We definitely need test coverage for this. This is fundamental enough to these specific APIs that I'd be shocked if there wasn't a CTS test path we could simply drop into cts_runner/test.lst it's necessary.

EDIT: This isn't covered by CTS, because it ain't WebGPU. 😅

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Dec 11, 2025

if there wasn't a CTS test path

MDI currently isn't in the spec, though there are plans to add it at some point.

We definitely need test coverage for this

+1

@ErichDonGubler
Copy link
Member

Welp, I'm shocked, and it's my fault. 🤦🏻‍♂️😆

Copy link
Member

@cwfitzgerald cwfitzgerald 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, lets get some more tests and one quibble

@beicause
Copy link
Contributor Author

Not sure how I should add tests for this as I’m not very familiar with this repository.

@cwfitzgerald
Copy link
Member

Information on the tests are available in https://github.com/gfx-rs/wgpu/blob/trunk/docs/testing.md if you're interested but honestly we can keep his as is for now, landing this fix is more important than getting testing for it. Lets just get that one change implemented and we can land this.

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) December 16, 2025 04:44
@cwfitzgerald cwfitzgerald merged commit bec772b into gfx-rs:trunk Dec 16, 2025
47 checks passed
@beicause beicause deleted the vk-fix-mdi branch December 16, 2025 05:36
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.

3 participants