Activate/deactivate high performance GPU when requested
On systems with more than one GPU, toggle the discrete GPU when there is WebGL content that requested high-performance. But also react to visibility and activity events, so that we can migrate back to the integrated GPU when in a background tab, etc.
<rdar://problem/30592266>
Created attachment 302050 [details] Patch
This patch isn't ready for formal review yet, because I still need to add code that delays the switch back from dGPU to iGPU. This will be a simple timer that waits 10 seconds, just in case we are swapping through tabs - we don't want to churn the GPU.
Comment on attachment 302050 [details] Patch Attachment 302050 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3151019 New failing tests: fast/canvas/webgl/context-creation-attributes.html
Created attachment 302056 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 302050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302050&action=review r- based on offline discussion. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:633 > + if (page) { Reverse the conditional and return early. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5889 > + m_context->setContextIsActive(newActivityState & ActivityState::IsVisible); This doesn't include occlusion. My own hacked version: ActivityState::Flags changed = oldActivityState ^ newActivityState; if (changed & (ActivityState::IsVisible | ActivityState::IsVisibleOrOccluded)) m_context->setContextIsActive((newActivityState & ActivityState::IsVisible)&&(newActivityState & ~ActivityState::IsVisibleOrOccluded)); almost worked. It would switch to iGPU at first, then a split second later switch back to dGPU. > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:65 > +static Vector<GraphicsContext3D*>& liveContexts() All of these static functions should be in a Manager-type class, I think. > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:68 > + return s_liveContexts; "contexts" instead of "active" or "live" ones. > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:135 > + CGLPixelFormatObj* highPerformanceLock = highPerformancePixelFormatObj(); We should refactor mechanics of retainHighPerformanceGraphics/releaseHighPerformanceGraphics from maintenance of the contexts list.
Comment on attachment 302050 [details] Patch Whoops; I missed Dean's earlier comment.
Comment on attachment 302050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302050&action=review >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:633 >> + if (page) { > > Reverse the conditional and return early. Fixed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5889 >> + m_context->setContextIsActive(newActivityState & ActivityState::IsVisible); > > This doesn't include occlusion. My own hacked version: > > ActivityState::Flags changed = oldActivityState ^ newActivityState; > if (changed & (ActivityState::IsVisible | ActivityState::IsVisibleOrOccluded)) > m_context->setContextIsActive((newActivityState & ActivityState::IsVisible)&&(newActivityState & ~ActivityState::IsVisibleOrOccluded)); > > almost worked. It would switch to iGPU at first, then a split second later switch back to dGPU. Do we want this? Visible or Occluded seems wrong. If the window is occluded, we want to turn off the dGPU. I'm not sure I understand the point of this flag, unless it is meant to mean partially occluded (although that sounds like visible to me!) >> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:65 >> +static Vector<GraphicsContext3D*>& liveContexts() > > All of these static functions should be in a Manager-type class, I think. I've fixed all this too.
Created attachment 302081 [details] Patch
Ready for review now. I'm still thinking of the best way to test, given a full test requires particular hardware.
Comment on attachment 302081 [details] Patch Attachment 302081 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3154943 New failing tests: fast/canvas/webgl/context-creation-attributes.html
Created attachment 302083 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 302081 [details] Patch Attachment 302081 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3154961 New failing tests: fast/canvas/webgl/context-creation-attributes.html
Created attachment 302084 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 302081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302081&action=review > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:501 > + addActivityStateChangeObserver(); Maybe it's a better idea to push the check into the call to avoid having to remember add the if in multiple places? Then rename as `addActivityStateChangeObserverIfNeeded` > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:637 > + m_addedActivityStateChangeObserver = true; I don't think this bool is needed at all? But if so, shouldn't this whole function include a check for `if (!m_addedActivityStateChangeObserver)`? Otherwise there's a possibility of calling `setContextVisibility` multiple times when this function is accidentally called multiple times. That said, I think there would be no harm in that if the list of contexts in the manager were a HashSet. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:640 > + // state change that would tell the context it is active. Do you mean "tell the context is it visible"? > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:642 > + m_context->setContextVisibility(true); In the accidental case this method gets calls twice, if the context is not visible, then you won't have a chance to turn visibility to false. Use `isVisibleAndActive` instead of `true`? Also, here you're checking for visible and active, but in `activityStateDidChange` you're only checking for visible. Shouldn't they use the same check? > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:647 > + if (m_addedActivityStateChangeObserver) { Don't think the bool is needed. If the observer isn't in the list it will just be a no-op. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5630 > + addActivityStateChangeObserver(); Could convert to `addActivityStateChangeObserverIfNecessary`. > Source/WebCore/platform/graphics/GraphicsContext3D.h:1148 > + GraphicsContext3DPowerPreference usedPowerPreference() const { return m_usedPowerPreference; } What does it mean "used" power preference? The manager is in control of honoring the preference. Maybe just "powerPreference"? > Source/WebCore/platform/graphics/GraphicsContext3D.h:1383 > + GraphicsContext3DPowerPreference m_usedPowerPreference { GraphicsContext3DPowerPreference::Default }; What does it mean "used" power preference? The manager is in control of honoring the preference. Maybe just "powerPreference"? > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:67 > +class GraphicsContext3DManager { Should split this to its own class, in a future patch. > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:90 > + Vector<GraphicsContext3D*> m_contextsRequiringHighPerformance; Couldn't these just be HashSets? > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:139 > + m_contexts.removeFirst(context); Should this function also clean up m_contextsRequiringHighPerformance? > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:149 > + ASSERT(context); Early return for `!hasMuxableGPU()`? I'm unsure that relying on proper setting of m_usedPowerPreference is enough for future-proofing. > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:163 > +{ Early return for `!hasMuxableGPU()`? > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:175 > +#if PLATFORM(MAC) Early return for `!hasMuxableGPU()`? > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:198 > + } else if (!m_contextsRequiringHighPerformance.size() && m_pixelFormatObj) { `!m_contextsRequiringHighPerformance.size()` is already true by this point. Remove. > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:204 > + static const int timeToKeepHighPerformanceGPUAliveInSeconds = 10; Is there a standard timeout we could use? Gavin, any suggestions? > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:569 > + manager().removeContext(this); Can we combine this? Removal of a context should force removal of the context in the high perf list.
Comment on attachment 302081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302081&action=review >> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:204 >> + static const int timeToKeepHighPerformanceGPUAliveInSeconds = 10; > > Is there a standard timeout we could use? Gavin, any suggestions? This will also require tests to wait for this long. Will there be a way to configure the timeout to 0 for tests?
Comment on attachment 302081 [details] Patch Most of my comments could be addressed in a later patch. r=me with bug tests fixed.
Comment on attachment 302081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302081&action=review >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:501 >> + addActivityStateChangeObserver(); > > Maybe it's a better idea to push the check into the call to avoid having to remember add the if in multiple places? > > Then rename as `addActivityStateChangeObserverIfNeeded` yes, thanks. done. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:637 >> + m_addedActivityStateChangeObserver = true; > > I don't think this bool is needed at all? But if so, shouldn't this whole function include a check for `if (!m_addedActivityStateChangeObserver)`? Otherwise there's a possibility of calling `setContextVisibility` multiple times when this function is accidentally called multiple times. > > That said, I think there would be no harm in that if the list of contexts in the manager were a HashSet. Removed the bool. I vaguely remember having a reason for it at some time :) >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:640 >> + // state change that would tell the context it is active. > > Do you mean "tell the context is it visible"? Yes. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:642 >> + m_context->setContextVisibility(true); > > In the accidental case this method gets calls twice, if the context is not visible, then you won't have a chance to turn visibility to false. Use `isVisibleAndActive` instead of `true`? > > Also, here you're checking for visible and active, but in `activityStateDidChange` you're only checking for visible. Shouldn't they use the same check? changed to isVisible. I've also changed to simply call m_context->setContextVisibility(page->isVisible()), although the thinking was that if the context is not visible, then there is no need to enable the dGPU. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:647 >> + if (m_addedActivityStateChangeObserver) { > > Don't think the bool is needed. If the observer isn't in the list it will just be a no-op. Removed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5630 >> + addActivityStateChangeObserver(); > > Could convert to `addActivityStateChangeObserverIfNecessary`. Done. >> Source/WebCore/platform/graphics/GraphicsContext3D.h:1148 >> + GraphicsContext3DPowerPreference usedPowerPreference() const { return m_usedPowerPreference; } > > What does it mean "used" power preference? The manager is in control of honoring the preference. Maybe just "powerPreference"? Renamed to 'powerPreferenceUsedForCreation' >> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:90 >> + Vector<GraphicsContext3D*> m_contextsRequiringHighPerformance; > > Couldn't these just be HashSets? m_contextsRequiringHighPerformance can be a HashSet. Changed. m_contexts cannot, since it needs to be ordered for recycling the oldest context. >> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:139 >> + m_contexts.removeFirst(context); > > Should this function also clean up m_contextsRequiringHighPerformance? Yes, added. And that meant i could remove some other code. >> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:149 >> + ASSERT(context); > > Early return for `!hasMuxableGPU()`? > > I'm unsure that relying on proper setting of m_usedPowerPreference is enough for future-proofing. Early return added. >> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:163 >> +{ > > Early return for `!hasMuxableGPU()`? I didn't do that one, because it means the addContextRequiringHighPerformance must have an equivalent check. I feel like it's ok to be less cautious in this method because it doesn't do bad things. >> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:175 >> +#if PLATFORM(MAC) > > Early return for `!hasMuxableGPU()`? Ditto. >> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:198 >> + } else if (!m_contextsRequiringHighPerformance.size() && m_pixelFormatObj) { > > `!m_contextsRequiringHighPerformance.size()` is already true by this point. Remove. Thanks. >>> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:204 >>> + static const int timeToKeepHighPerformanceGPUAliveInSeconds = 10; >> >> Is there a standard timeout we could use? Gavin, any suggestions? > > This will also require tests to wait for this long. Will there be a way to configure the timeout to 0 for tests? I've added a FIXME to this. Unfortunately this class doesn't have a reference to anything with a Settings object. And adding a reference would mean that the manager() call has to take a parameter. >> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:569 >> + manager().removeContext(this); > > Can we combine this? Removal of a context should force removal of the context in the high perf list. Yes, that's done.
Committed r212633: <http://trac.webkit.org/changeset/212633>