Repository navigation
[Impeller] Allow transfer writes after the Vulkan mipmap layout barrier - #194989
Conversation
BlitPassVK::OnGenerateMipmapCommand first moves every mip level to TransferDst, then blits into levels 1..N. That barrier allowed only transfer reads, so the blits' writes weren't ordered after the layout transition. Synchronization validation reports SYNC-HAZARD-WRITE-AFTER-WRITE on vkCmdBlitImage. Allow transfer writes, which is what the blits do in that layout.
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the destination access mask in BlitPassVK::OnGenerateMipmapCommand from eTransferRead to eTransferWrite to address a write-after-write synchronization hazard, and adds a unit test to verify the initial barrier configuration. The review feedback recommends replacing a reinterpret_cast with a static_cast in the new unit test when downcasting the command buffer, adhering to the Google C++ Style Guide.
| EXPECT_EQ(barriers[7].subresourceRange.levelCount, 6u); | ||
| } | ||
|
|
||
| TEST(BlitPassVKTest, MipmapGenerationInitialBarrierAllowsTransferWrites) { |
There was a problem hiding this comment.
I think the test we actually want for this is running the blit pass with the synchronization validation layers enabled. @andywolff I know you were just looking into this. How feasible is this? Do we have an issue for it, do we want to hand it off?
There was a problem hiding this comment.
I just filed #195502
I recommend we note this area in that issue as one of the ones we want to include in the new sync validation CI shard, and approve this PR to proceed without blocking on that work
|
#195698 has landed on main, you can rebase now |
|
Thank you! Rebased on main, and re-enabled Play/RendererTest.CanGenerateMipmaps/Vulkan in the sync validation list. |
|
LGTM but ci.yaml validation failed, which means you need to do a merge. If you push a merge commit instead of rebasing, it'll keep the approvals |
andywolff
added
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
…migration/phase0 The branch was 61 commits behind master. Merged rather than rebased: the branch's commits are pushed and under review (PR flutter#196005). Conflicts, resolved by hand: - dev/bots/test.dart: master's flutter#195863 balances tool_tests_commands and web_tool_tests by duration through _selectWeightedToolTests (one durations file, dev/bots/tool_test_durations.json, for the three tool shards); the Bazel branches of the three shard runners now cover the paths it selects, as before over selectIndexOfTotalSubshard's. Regenerated for master's rolls and renames: - build_defs/repositories/deps.lock.bzl (gen_deps.dart): the Dart SDK roll to c9153304136a and its package revisions, the Fuchsia SDK and test scripts. - build_defs/dart/pub.lock.bzl (gen_pub_lock.dart after `dart pub get` at the root and in lib/web_ui): the pubspec changes of master. - engine/src/flutter/lib/ui/BUILD.bazel (gn_to_bazel): the GLES compositing unit test renamed to impeller_gles_without_surface_unittests.cc. - engine/src/flutter/impeller/BUILD.bazel (gn_to_bazel): run_tests.py's Vulkan synchronization-validation list grew by the tests flutter#194989 fixed. `gn_to_bazel --check` is current. Adjusted by hand for master's renames: - dev/bots/BUILD.bazel and packages/flutter_tools/BUILD.bazel: the durations test is tool_test_durations_test over tool_test_durations.json and the test files of the commands, integration and web shards (`durations_shard_test_files`, was `integration_shard_test_files`). - dev/bots/test/test_test.dart: the `_bazel` subshard test expects master's "Selecting subshard 1 of 3 (N of M tests, estimated weight W)". Checked: gclient sync clean (Skia f96be51af95d, Dart c9153304136a); gn_to_bazel 282/282 and --check current; bazel_tools 46/46; bazel_ci_test.py 23; pylint clean; Bazel analysis of //:framework_tests //:tool_tests //:dart_tests //dev/bots:tests and of ci_host_debug_unopt (+tests), android_debug, wasm_release, ci_android_release_arm64; `bazel test //dev/bots:tests` green; dev/bots test_test.dart's Bazel tests green (its `--dry-run prints every test` test depends on SHARD being set in the environment, on master as here). Note: master's flutter#195868 adds the mac_unopt builds as standalone bringup builders under ci/builders/standalone/ (GN; the converter does not read that directory). mac_unopt.json itself still carries --bazel.
BlitPassVK::OnGenerateMipmapCommandfirst moves every mip level toTransferDst, then blits into levels 1..N. That first barrier allowed only transfer reads as its destination access, so the blits' writes weren't ordered after the layout transition.This changes the destination access to transfer writes, which is what the blits do in that layout. The destination stage was already the transfer stage.
Found with
VK_LAYER_KHRONOS_validation1.4.309 and synchronization validation on, on a Raspberry Pi 5 / CM5 (V3D 7.1, Mesa 26.2.1) running Impeller Vulkan through a custom embedder (flutter_pi).Fixes: #194987
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.