GrCacheController may interfere with virtual context state tracking |
|||||||||||||
Issue descriptionGrCacheController should set RasterDecoderContextState::need_context_state_reset because Skia may issue GL calls when cleaning up resources. This could change GL state, which is not reflected in gpu::gles2::ContextState. Without this change, it's possible that when OOP-R is enabled other clients of virtual context (e.g. DisplayCompositor or WebGL on Android) will get the wrong GL state.
,
Nov 29
enne,khusalsagar: Fix is in CQ: https://chromium-review.googlesource.com/c/chromium/src/+/1355438 I found this when working on a different change. I have no idea if this is an issue with your OOP-R field trial. You want merge request for M71?
,
Nov 29
Yes, I think that would be safest, if we can! More state resetting is only going to help potential crashes or correctness issues.
,
Nov 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7e3227fb00944820e8dbb5f27f24660c941db528 commit 7e3227fb00944820e8dbb5f27f24660c941db528 Author: Jonathan Backer <backer@chromium.org> Date: Thu Nov 29 20:09:06 2018 Set need need_context_state_reset when GrContext accessed When we access RasterDecoderContextState::gr_context, the RasterDecoderImpl::context_state may become inconsistent with actual GL context state and we should set need_context_state_reset Bug: 910274 Change-Id: I834ce6e4cf9b5fe549de432b3bc6c06fa2a6ccbd Reviewed-on: https://chromium-review.googlesource.com/c/1355438 Commit-Queue: Jonathan Backer <backer@chromium.org> Reviewed-by: Peng Huang <penghuang@chromium.org> Cr-Commit-Position: refs/heads/master@{#612332} [modify] https://crrev.com/7e3227fb00944820e8dbb5f27f24660c941db528/components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc [modify] https://crrev.com/7e3227fb00944820e8dbb5f27f24660c941db528/gpu/command_buffer/service/gr_cache_controller.cc
,
Nov 29
,
Nov 29
Requesting merge for M71. This is a safe to merge change (obviously correct) that should only help with stability and correctness of OOP-R, which I believe is at 50% field trial on M71 right now for Android.
,
Nov 29
This bug requires manual review: We are only 4 days from stable. Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 29
Thanks for nailing this down! We should definitely merge this, there are still crashes which have a spike with OOP-R (issue 877239 and 884659), likely from cases like these where we are missing skia changing GLContext state.
,
Nov 30
+penghuang It occurred to me this morning that the CL in comment #4 is more of a mitigation than a solution. Specifically, need_context_state_reset only really helps if the virtual context associated with a RasterDecoderImpl is current or about to be made current. Now, that may or may not be the case when GrCacheController is purging or freeing resources (e.g. maybe the display compositor is using GpuMain thread). I believe that this is likely to be an issue. Specifically, if Skia thinks that a resource (e.g. a texture) that it wants to free is bound, it will probably unbind it first. Because the context state is wrong, it may unbind something that a different client (e.g. display compositor) expects to be bound. A proper fix for this is probably gated on issue 902904 . We are pursuing option #2 from comment #7 on that bug. If we do that right, we should be able to reset context state properly in GrCacheController.
,
Dec 1
Approving merge to M71 branch 3578 based on comments #6 and #8. Please merge ASAP as we're cutting M71 stable RC soon today. Approving as requested by benmason@ (M71 Release TPM).
,
Dec 1
I'll do the merge now. Looks like there is a merge conflict, couldn't do it from gerrit directly.
,
Dec 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d1f5ab8a578b9163c9a3dae6f4b5ba2de7bbd2ae commit d1f5ab8a578b9163c9a3dae6f4b5ba2de7bbd2ae Author: Khushal <khushalsagar@chromium.org> Date: Sat Dec 01 01:03:41 2018 Set need need_context_state_reset when GrContext accessed When we access RasterDecoderContextState::gr_context, the RasterDecoderImpl::context_state may become inconsistent with actual GL context state and we should set need_context_state_reset TBR=backer@chromium.org (cherry picked from commit 7e3227fb00944820e8dbb5f27f24660c941db528) Bug: 910274 Change-Id: I834ce6e4cf9b5fe549de432b3bc6c06fa2a6ccbd Reviewed-on: https://chromium-review.googlesource.com/c/1355438 Commit-Queue: Jonathan Backer <backer@chromium.org> Reviewed-by: Peng Huang <penghuang@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#612332} Reviewed-on: https://chromium-review.googlesource.com/c/1356806 Reviewed-by: Khushal <khushalsagar@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#864} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/d1f5ab8a578b9163c9a3dae6f4b5ba2de7bbd2ae/gpu/command_buffer/service/gr_cache_controller.cc
,
Dec 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d1f5ab8a578b9163c9a3dae6f4b5ba2de7bbd2ae Commit: d1f5ab8a578b9163c9a3dae6f4b5ba2de7bbd2ae Author: khushalsagar@chromium.org Commiter: khushalsagar@chromium.org Date: 2018-12-01 01:03:41 +0000 UTC Set need need_context_state_reset when GrContext accessed When we access RasterDecoderContextState::gr_context, the RasterDecoderImpl::context_state may become inconsistent with actual GL context state and we should set need_context_state_reset TBR=backer@chromium.org (cherry picked from commit 7e3227fb00944820e8dbb5f27f24660c941db528) Bug: 910274 Change-Id: I834ce6e4cf9b5fe549de432b3bc6c06fa2a6ccbd Reviewed-on: https://chromium-review.googlesource.com/c/1355438 Commit-Queue: Jonathan Backer <backer@chromium.org> Reviewed-by: Peng Huang <penghuang@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#612332} Reviewed-on: https://chromium-review.googlesource.com/c/1356806 Reviewed-by: Khushal <khushalsagar@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#864} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Dec 1
Re #9. My first thought here was that in this case we should reset the current virtual context along with marking the bit on RDCS so that the next time any virtual context is used, it will have to be made current and since it won't find any previous virtual context it will reset all state. Chatted with piman@ and he thought we were getting this behaviour already using |current_context_| global state. IIUC, the assumption was that making the real context current would update |current_context_| to point to the real context instead of the last active virtual context. And that would basically cause as stated above, the next time a virtual context is used it notices its not the |current_context_| and in the process of becoming current also sees that the current one is not a virtual context so resets everything. But stuff doesn't work this way. GLContextReal::IsCurrent implementations only check whether the native context is current, so we would never update the |current_context_| state. And GLContextVirtual::MakeVirtuallyCurrent only checks whether its underlying real context is current (through |current_real_context_|) and then checks its |current_virtual_context_| to see if state restoration is required. Its true that the above won't be an issue once RDCS is using a shared virtual context but just reseting the real context's |current_virtual_context_| to force full state restoration sounds like an easy short term fix? Unless something along the lines of what piman@ assumed was the case already is better.
,
Dec 3
@#14: I believe that this is just a paraphrase of what you said, but I think it helps to be concrete. Here is a concrete sequences of events that concerns me using virtual contexts: a) WebGL does some work: GLContextVirtual::MakeCurrent -> GLContext::MakeVirtuallyCurrent called before work starts b) GrCacheController::PurgeGrCache callback executes and performs freeGpuResources: GLContext::MakeCurrent called on the underlying driver context before work starts; this is roughly a no-op because it does not change current_real_context_; freeGpuResources may mangle GL state (e.g. unbind textures or framebuffers before deleting them) c) WebGL does some more work: GLContextVirtual::MakeCurrent -> GLContext::MakeVirtuallyCurrent called before work starts; MakeVirtuallyCurrent is a no-op because switched_real_contexts is false, virtual_context == current_virtual_context_, and virtual_context->IsCurrent is true (see https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gl_context_virtual.cc?rcl=5f53275bd8e2f76dfc3685ef958dc97d2232bb61&l=63) I chose WebGL as an example, but this applies to GPU Canvas 2D and the display compositor as well. As mentioned above, a way to fix this to have GrCacheController::PurgeCache set current_virtual_context_ to nullptr. Then is step (c), virtual_context != current_virtual_context_ in MakeVirtuallyCurrent and we get a complete state reset. I believe this will happen in the process of resolving issue 902904 .
,
Dec 3
I posted a CL here for resetting current_virtual_context_. https://chromium-review.googlesource.com/c/chromium/src/+/1358799
,
Dec 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7de4ba6177cd179157590a3d161ba7b9f8ec0613 commit 7de4ba6177cd179157590a3d161ba7b9f8ec0613 Author: Jonathan Backer <backer@chromium.org> Date: Mon Dec 03 18:49:14 2018 Force full restore on next MakeVirtuallyCurrent This CL is kept minimal to facilitate merge to M72 (and potentially M71). Details on comments 14 and 15 on https://crbug.com/910274 A more complete solution addressing SkiaRenderer,SkDDL will come when issue 902904 is resolved. Bug: 910274 Change-Id: Ie0f44443fa30a5ec87045bd138a3a8a08e17e35c Reviewed-on: https://chromium-review.googlesource.com/c/1358799 Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Jonathan Backer <backer@chromium.org> Cr-Commit-Position: refs/heads/master@{#613173} [modify] https://crrev.com/7de4ba6177cd179157590a3d161ba7b9f8ec0613/gpu/command_buffer/service/gr_cache_controller.cc [modify] https://crrev.com/7de4ba6177cd179157590a3d161ba7b9f8ec0613/ui/gl/gl_context.cc [modify] https://crrev.com/7de4ba6177cd179157590a3d161ba7b9f8ec0613/ui/gl/gl_context.h
,
Dec 3
Adding a merge request for CL referenced on comment #17 for M72 (which branched last week).
,
Dec 4
Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0399d926b8940362e65ed951547a569ab54f22d4 commit 0399d926b8940362e65ed951547a569ab54f22d4 Author: Jonathan Backer <backer@chromium.org> Date: Tue Dec 04 19:12:31 2018 Force full restore on next MakeVirtuallyCurrent This CL is kept minimal to facilitate merge to M72 (and potentially M71). Details on comments 14 and 15 on https://crbug.com/910274 A more complete solution addressing SkiaRenderer,SkDDL will come when issue 902904 is resolved. Bug: 910274 Change-Id: Ie0f44443fa30a5ec87045bd138a3a8a08e17e35c Reviewed-on: https://chromium-review.googlesource.com/c/1358799 Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Jonathan Backer <backer@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#613173}(cherry picked from commit 7de4ba6177cd179157590a3d161ba7b9f8ec0613) Reviewed-on: https://chromium-review.googlesource.com/c/1362002 Reviewed-by: Jonathan Backer <backer@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#39} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/0399d926b8940362e65ed951547a569ab54f22d4/gpu/command_buffer/service/gr_cache_controller.cc [modify] https://crrev.com/0399d926b8940362e65ed951547a569ab54f22d4/ui/gl/gl_context.cc [modify] https://crrev.com/0399d926b8940362e65ed951547a569ab54f22d4/ui/gl/gl_context.h
,
Dec 4
I'm marking this as Fixed for now. A better solution awaits issue 902904 .
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0399d926b8940362e65ed951547a569ab54f22d4 Commit: 0399d926b8940362e65ed951547a569ab54f22d4 Author: backer@chromium.org Commiter: backer@chromium.org Date: 2018-12-04 19:12:31 +0000 UTC Force full restore on next MakeVirtuallyCurrent This CL is kept minimal to facilitate merge to M72 (and potentially M71). Details on comments 14 and 15 on https://crbug.com/910274 A more complete solution addressing SkiaRenderer,SkDDL will come when issue 902904 is resolved. Bug: 910274 Change-Id: Ie0f44443fa30a5ec87045bd138a3a8a08e17e35c Reviewed-on: https://chromium-review.googlesource.com/c/1358799 Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Jonathan Backer <backer@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#613173}(cherry picked from commit 7de4ba6177cd179157590a3d161ba7b9f8ec0613) Reviewed-on: https://chromium-review.googlesource.com/c/1362002 Reviewed-by: Jonathan Backer <backer@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#39} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Dec 27
Thanks a lot for fixing this backer! |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by backer@chromium.org
, Nov 29Components: Internals>Compositing>OOP-Raster