[Impeller] fix barriers prior to mip generation. #53082
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
We do have tests that get close to this in blit_pass_unittests.cc. None of them hit this code at all. Can you add a test that executes BlitPassVK::OnGenerateMipmapCommand to make sure it doesn't crash or create validation errors? That's better than nothing.
|
I can definitely add a test case that hits this path. That said, I did not observe any validation errors locally unfortunately |
Yea, the test won't help us know that we've fixed the issue, but it will help us know that we didn't introduce a new one. I was hoping maybe the process of writing a non-perfect test might make the path to a better test more clear. |
There was a problem hiding this comment.
The sync validation only catches errors due such as write-after-write which can be trivially checked to be incorrect. This one is an issue with insufficient synchronization and harder to detect. For instance, we could have had a compute stage writing to this texture before before blitting from it. Or perhaps we had another form of synchronization like a semaphore.
Like you idea on tracking layouts at the queue level, perhaps we can track write and reads at the per-resource per-queue level too and automatically insert barriers?
LGTM modulo Aarons comments.
|
Longer term, yeah - automatic barrier determination is a must - especially for something like flutter GPU. That said, its a big lift to do so efficiently |
|
I checked and all of the backdrop blurs in aiks_blur_unttests will hit this case. The bug happens when a render pass texture has mips generated |
Co-authored-by: gaaclarke <[email protected]>
Could we potentially set up the barriers to be in the bad state before doing an aiks_blur_unttests test to reproduce the problem? |
|
The barriers are currently in a bad state |
Could we get that bug to show up in the golden output then by drawing at a particular mip level? Or is it non deterministic so we couldn't know for sure we grabbed bad output? |
|
Unfortunately its a synchronization issue/race, so I don't think we can guarantee it shows up |
Ahh yea, we'd probably have to make a test that just pounds on drawing and check that the output is the same each frame, so not really a golden test. Otherwise, a test that just tests the flags on a mocked up vulkan isn't so helpful. We don't really have anything that does that yet that I know of. Do you think we could reproduce the problem like that and would it be worth investing in? This bug is so subtle I could easily see this getting broken again at some point and not noticing it until much later, but investing in a potentially flaky test isn't an obvious slam dunk either. |
|
if all of our blurs start flickering, I think we just have to hope that we notice. I do not even know if its possible to reproduce in swiftshader. Its possible that the implicit synchronization from running everything on the CPU causes it to "just work". |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
test exception: The new logic is invariant so a test that mocks out vulkan isn't that informative. The code is executed as part of the blur tests, so it has some coverage to make sure it isn't completely wrong. The error manifests because of a synchronization problem that isn't caught by the validation layers. We are uncertain if pounding on drawing could even reproduce the problem with our swiftshader test harness.
jonahwilliams
added
the
autosubmit
…149290) flutter/engine@8d5d14a...4adf453 2024-05-29 [email protected] Roll Skia from aecf76e8eb9a to f23183f041fe (1 revision) (flutter/engine#53102) 2024-05-29 [email protected] Update CONTRIBUTING.md links (flutter/engine#53043) 2024-05-29 [email protected] Roll Skia from 32746679f7be to aecf76e8eb9a (1 revision) (flutter/engine#53100) 2024-05-29 [email protected] Roll Skia from b18b7d6af6e8 to 32746679f7be (2 revisions) (flutter/engine#53097) 2024-05-29 [email protected] [Impeller] revert changes to IO thread upload scheduling. (flutter/engine#53096) 2024-05-29 [email protected] [Impeller] fix barriers prior to mip generation. (flutter/engine#53082) 2024-05-29 [email protected] Roll Skia from e11a5c5b402e to b18b7d6af6e8 (2 revisions) (flutter/engine#53095) 2024-05-29 [email protected] Switch to FilterQuality.medium for images (flutter/engine#52984) 2024-05-29 [email protected] Roll Skia from a4f58c9dabc3 to e11a5c5b402e (1 revision) (flutter/engine#53092) 2024-05-29 [email protected] Roll Skia from ca77972f659e to a4f58c9dabc3 (2 revisions) (flutter/engine#53091) 2024-05-29 [email protected] [Impeller] relax conditions for SkRRect.isSimple conversion to impeller::RRect. (flutter/engine#53083) 2024-05-29 [email protected] Roll Skia from d1a6967c48a5 to ca77972f659e (1 revision) (flutter/engine#53090) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#149290) flutter/engine@8d5d14a...4adf453 2024-05-29 [email protected] Roll Skia from aecf76e8eb9a to f23183f041fe (1 revision) (flutter/engine#53102) 2024-05-29 [email protected] Update CONTRIBUTING.md links (flutter/engine#53043) 2024-05-29 [email protected] Roll Skia from 32746679f7be to aecf76e8eb9a (1 revision) (flutter/engine#53100) 2024-05-29 [email protected] Roll Skia from b18b7d6af6e8 to 32746679f7be (2 revisions) (flutter/engine#53097) 2024-05-29 [email protected] [Impeller] revert changes to IO thread upload scheduling. (flutter/engine#53096) 2024-05-29 [email protected] [Impeller] fix barriers prior to mip generation. (flutter/engine#53082) 2024-05-29 [email protected] Roll Skia from e11a5c5b402e to b18b7d6af6e8 (2 revisions) (flutter/engine#53095) 2024-05-29 [email protected] Switch to FilterQuality.medium for images (flutter/engine#52984) 2024-05-29 [email protected] Roll Skia from a4f58c9dabc3 to e11a5c5b402e (1 revision) (flutter/engine#53092) 2024-05-29 [email protected] Roll Skia from ca77972f659e to a4f58c9dabc3 (2 revisions) (flutter/engine#53091) 2024-05-29 [email protected] [Impeller] relax conditions for SkRRect.isSimple conversion to impeller::RRect. (flutter/engine#53083) 2024-05-29 [email protected] Roll Skia from d1a6967c48a5 to ca77972f659e (1 revision) (flutter/engine#53090) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
This fixes the flickering and rendering errors when running on Android seen in flutter/flutter#148412 .
The problem is the barriers proceding mip generation do not account for the input text having been the output of a render pass. Unfortunately no validation errors on this, and no Idea how to test it.