Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14069008:
GPU: Reduce MakeCurrent calls to fix Orange San Diego (Closed)
|
Created:
7 years, 8 months ago by epenner Modified:
7 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionGPU: Reduce MakeCurrent calls to fix Orange San Diego
We have hit several separate issues resulting from calling
MakeCurrent to change surfaces (when using virtual contexts).
This removes most cases of MakeCurrent that aren't needed,
and enables virtual contexts for Orange San Diego.
The last remaining case is ReleaseCurrent which isn't needed
for the Orange San Diego fix, and will be done in a follow up CL.
BUG=179250, 229643, 230896
NOTRY=true
No try since the trybots all passed and the last change is a one-liner.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198182
Patch Set 1 #
Total comments: 12
Patch Set 2 : Rebase. #Patch Set 3 : Reduce scope. Cleanup IsCurrent. #
Total comments: 1
Patch Set 4 : Merged work-around. #
Messages
Total messages: 20 (0 generated)
PTAL. It isn't much code in the end once I found all the causes of MakeCurrent. This works fine, and I'll do extensive testing on all our devices, but please check carefully if I've missed any assumptions. This results in zero MakeCurrent calls after startup.
lgtm but... Let me double check certain things won't break. On Desktop we'd like to consider using virtual contexts for Pepper. So, each Pepper instance will get one real context and then each user context will be a virtual context on that real context. That means MakeCurrent will need to be called at the appropriate times as there will be multiple real contexts. Just checking, I think the code will work as and in fact I think the code in gpu/command_buffer/tests/gl_virtual_contexts_unittest.cc will test this but just checking.
I don't think we can break ReleaseCurrent(). Besides the semantical breakage, it can cause real problems. For example it will cause us to leak surfaces: When you go to the home screen the view surface gets freed, and we have to release it from the shared, persistent context. Other offscreen contexts cannot keep using it at that point. I remember it was also particularly important, somehow related to that, that the ReleaseCurrent(NULL) in ~GpuCommandBufferStub() works as expected (to play nicely with the Surface::IsCurrent() logic?) - I think I might have even added that call in that place. But I'd have to refresh my memory on the details. All I remember is that things were a bit tricky to get right. That being said, since this is all to work around a driver bug that we do not understand intrinsically at all, I'm worried that trying to experimentally work around it might not lead to a bullet-proof workaround, because - we might miss corner cases - it will not keep reliably working around it as pieces change Plus it'll risk breaking things in general. The virtual context switch optimization I put in is a bit of a quick hack that works by avoiding context switches when it's guaranteed to be safe. But it's not designed at that level to guarantee the opposite assumption: That we never call MakeCurrent() again after we made a view surface current. For that, we should redesign things to have GLContexts share a common GLSurface. It might be possible to make our view surface the GpuChannelManager's DefaultOffscreenSurface. But even then it will go away at certain times. And we might have other view/EGL surfaces (for example if we wanted to render to a SurfaceTexture) that we have to switch to. So the latter might make sense only, if we know this will be good enough to work around this driver bug.
To add some background: in crbug.com/179250 Eric found that the driver's share group support is broken. However, when we use virtual contexts to workaround that, it has another bug where it eventually crashes somehow from using a single context with multiple surfaces. We have engaged with them and they are actually interested in fixing both (I think they have already fixed share groups and it's a matter of getting that pushed out widely - at the same time we have never worked on those devices, so to me it seems fine to wait for a fix). Otherwise, I'm definitely for reworking the virtual context (switching) logic if that helps to work around the bug, rather than hacking on the somewhat fragile system, which I'm responsible for :) I don't like how IsCurrent/SetCurrent currently works: the virtual GLCcontext should probably be set as current, same for the Surface. And it might make things easier if we can share the actual view GLSurface, we'd have to think it through in detail.
I agree with some of your points Daniel but don't agree that this doesn't have it's own merit. This is how virtual contexts should work (it shouldn't call MakeCurrent - that's the point). If we need more refactoring that's fine. The only real problem I'm hearing is that we leak surfaces? That seems pretty easy to solve?
This also solves 2 if not 3 to 4 separate issues (black flicker when switching tabs sounds familiar?)
Ok chatted a bit with Eric, and we'll investigate more. Added some comments inline to indicate (if we added a workaround flag) what would need to be kept working. (Also, we discussed having a more explicitly shared GLSurface. The trick is to synchronize shared view surface creation and tear-down with the offscreen contexts referencing the same GLSurface. Maybe we'd have to have some wrapper GLSurfaceVirtual that can change the handle it returns.) https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (left): https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:37: shared_context_->ReleaseCurrent(compatible_surface); This can probably just go away. https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:56: if (IsCurrent(surface)) shared_context_->ReleaseCurrent() still needs to be called at least when the actual shared view surface goes away, which is not straightforward to find out from here (for example ~GpuCommandBuffer stub passes NULL in here because it specifically has to release its GLSurface ref before calling into here, as ~GLSurface might need a current context). https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (right): https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:32: if (!IsCurrent(compatible_surface)) { shared_context_->MakeCurrent() here is called to find out if the surface is compatible with the real context (it's not for example on Linux if the pbuffer does not match view config). So we'd have to do it (at least for non-Android) regardless of what GLContextVirtual::IsCurrent() says (which is optimized to return 'true' falsely).
https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (left): https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:56: if (IsCurrent(surface)) On 2013/04/10 22:44:31, Daniel Sievers wrote: > shared_context_->ReleaseCurrent() still needs to be called at least when the > actual shared view surface goes away, which is not straightforward to find out > from here (for example ~GpuCommandBuffer stub passes NULL in here because it > specifically has to release its GLSurface ref before calling into here, as > ~GLSurface might need a current context). Or to generalize, ReleaseCurrent() would have to be called whenever *any* surface goes away that is still current. So maybe it should happen in a different place. But it does not have to be called here if an offscreen context + offscreen surface goes away and that offscreen surface is not current (but some shared surface is).
If you have any other big concerns let me know. However I don't really see this as requiring a lot of justification. IMO this seems like what we should be doing for virtual contexts (minimizing MakeCurrent calls).
If you have any other big concerns let me know. However I don't really see this as requiring a lot of justification. IMO this seems like what we should be doing for virtual contexts (minimizing MakeCurrent calls). https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (left): https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:37: shared_context_->ReleaseCurrent(compatible_surface); On 2013/04/10 22:44:31, Daniel Sievers wrote: > This can probably just go away. Done. https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:37: shared_context_->ReleaseCurrent(compatible_surface); On 2013/04/10 22:44:31, Daniel Sievers wrote: > This can probably just go away. SGTM https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:56: if (IsCurrent(surface)) On 2013/04/10 22:44:31, Daniel Sievers wrote: > shared_context_->ReleaseCurrent() still needs to be called at least when the > actual shared view surface goes away, which is not straightforward to find out > from here (for example ~GpuCommandBuffer stub passes NULL in here because it > specifically has to release its GLSurface ref before calling into here, as > ~GLSurface might need a current context). Okay I see the problem there. That's a pretty nasty dependency :P I'll think about but conceptually we simply want this right? if (!surface->GetBackingFrameBufferObject()) shared_context_->ReleaseCurrent(surface); Any objections assuming I can figure out the NULL case? That seems like it should be fixed anyway. https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (right): https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:32: if (!IsCurrent(compatible_surface)) { On 2013/04/10 22:44:31, Daniel Sievers wrote: > shared_context_->MakeCurrent() here is called to find out if the surface is > compatible with the real context (it's not for example on Linux if the pbuffer > does not match view config). > So we'd have to do it (at least for non-Android) regardless of what > GLContextVirtual::IsCurrent() says (which is optimized to return 'true' > falsely). Hmm, if you want to legitimately bind a pbuffer surface for rendering, and Surface::IsOffscreen() denotes such a surface, then it seems is IsCurrent is at fault? IsCurrent should return false, which would allow the offscreen surface to be truly made current for rendering? Alternatively, if the surface has GetBackingFrameBufferObject(), then the phony pbuffer surface backing should *not* need to be verified, right? That is implied to be a phony singleton pbuffer, which can be verified as compatible at startup. Can you foresee any other problems with this?
https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (left): https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:56: if (IsCurrent(surface)) On 2013/04/11 03:23:43, epennerAtGoogle wrote: > On 2013/04/10 22:44:31, Daniel Sievers wrote: > > shared_context_->ReleaseCurrent() still needs to be called at least when the > > actual shared view surface goes away, which is not straightforward to find out > > from here (for example ~GpuCommandBuffer stub passes NULL in here because it > > specifically has to release its GLSurface ref before calling into here, as > > ~GLSurface might need a current context). > > Okay I see the problem there. That's a pretty nasty dependency :P I'll think > about but conceptually we simply want this right? > if (!surface->GetBackingFrameBufferObject()) > shared_context_->ReleaseCurrent(surface); > > Any objections assuming I can figure out the NULL case? That seems like it > should be fixed anyway. Discussed optimizing this by moving the call site from ~GpuCommandBufferStub() to GLSurface::Destroy(), which is called only during surface destruction (so the context will only get released when *all* references to the surface go away) and further optimizing it for wrapper surface types (image transport) to skip release if they don't have their own real (E)GL surface (but for example use the shared offscreen surface as their handle). https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (right): https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:32: if (!IsCurrent(compatible_surface)) { Discussed optimizing this by having the caller pass in NULL for compatible_surface if that surface is the same that the shared real context was created with, and then skipping the test here (since we know it's compatible then).
I need to revive this as there is no three devices effected by this. For one device this patch provides a full fix to the problem. https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (left): https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:56: if (IsCurrent(surface)) On 2013/04/11 19:56:19, Daniel Sievers wrote: > On 2013/04/11 03:23:43, epennerAtGoogle wrote: > > On 2013/04/10 22:44:31, Daniel Sievers wrote: > > > shared_context_->ReleaseCurrent() still needs to be called at least when the > > > actual shared view surface goes away, which is not straightforward to find > out > > > from here (for example ~GpuCommandBuffer stub passes NULL in here because it > > > specifically has to release its GLSurface ref before calling into here, as > > > ~GLSurface might need a current context). > > > > Okay I see the problem there. That's a pretty nasty dependency :P I'll think > > about but conceptually we simply want this right? > > if (!surface->GetBackingFrameBufferObject()) > > shared_context_->ReleaseCurrent(surface); > > > > Any objections assuming I can figure out the NULL case? That seems like it > > should be fixed anyway. > > Discussed optimizing this by moving the call site from ~GpuCommandBufferStub() > to GLSurface::Destroy(), which is called only during surface destruction (so the > context will only get released when *all* references to the surface go away) and > further optimizing it for wrapper surface types (image transport) to skip > release if they don't have their own real (E)GL surface (but for example use the > shared offscreen surface as their handle). To clarify, are you suggesting that every derived Surface::Destroy function also does eglMakeCurrent(null,null) or equivalent if the surface is bound? Hmm, an alternative would be that we get a higher level callback when the app is minimized to do a real release current? Out of interest, what is the problem with just leaving the old surface bound until the new one arrives (on app minimize/restore)? The eglDestroySurface spec says the surface isn't destroyed until it is made not-current, which likely means the surface just becomes zero size but the context stays alive while the app is minimized. Have you ever tried it? https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gl_context_virtual.cc (right): https://codereview.chromium.org/14069008/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gl_context_virtual.cc:32: if (!IsCurrent(compatible_surface)) { On 2013/04/11 19:56:19, Daniel Sievers wrote: > Discussed optimizing this by having the caller pass in NULL for > compatible_surface if that surface is the same that the shared real context was > created with, and then skipping the test here (since we know it's compatible > then). Sorry but that won't work. Initialize always need to insure a context/surface current before for SetupForVirtualization(). So we always need the surface to be passed in. Also, it seems checking MakeCurrent to insure the pbuffer is compatible isn't correct to do in this function anyway, since the real context is already created and we can't recover from the error. So I think the best thing to do is move this validation outside of Initialize as I mentioned above, and leave the code as is, to insure a valid context/surface is current. If you look at the call-site, the real context is created with the default-offscreen-surface, so it's already guaranteed to be compatible with that surface.
Okay, the Orange San Diego only needs part of the original patch, and to enable the FBO unbind work-around. I've reduced the scope of this patch to only the reviewed parts so I can land the urgent part. If there is any remaining issues I'll address together with removing ReleaseCurrent.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/14069008/19001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
I further reduced this to only what is required for Orange. gman, can you have another look at GLContextVirtual::IsCurrent? I made it more succinct IMO. It also checks that the native surface is current instead of just checkout our TLS pointer which we were doing before. After that I'd like to land this part today and I'll remove the other MakeCurrent calls in future CLs one by one.
lgtm https://codereview.chromium.org/14069008/diff/49002/gpu/command_buffer/servic... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/14069008/diff/49002/gpu/command_buffer/servic... gpu/command_buffer/service/feature_info.cc:233: if (is_imagination) { I'd prefer to only set the switch once (merge this if and the one above) but up to you
On 2013/05/03 18:28:09, greggman wrote: > lgtm > > https://codereview.chromium.org/14069008/diff/49002/gpu/command_buffer/servic... > File gpu/command_buffer/service/feature_info.cc (right): > > https://codereview.chromium.org/14069008/diff/49002/gpu/command_buffer/servic... > gpu/command_buffer/service/feature_info.cc:233: if (is_imagination) { > I'd prefer to only set the switch once (merge this if and the one above) but up > to you Good call. I'll merge it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/14069008/61001
Message was sent while issue was closed.
Change committed as 198182 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
