Bug 168559 – Activate/deactivate high performance GPU when requested

• Component: WebGL
• Status: RESOLVED FIXED

[Dean Jackson]

Activate/deactivate high performance GPU when requested

[Dean Jackson]

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.

[Radar WebKit Bug Importer]

<rdar://problem/30592266>

[Dean Jackson]

Created attachment 302050 [details]
Patch

[Dean Jackson]

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.

[Build Bot]

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

[Build Bot]

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

[Jon Lee]

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.

[Jon Lee]

Comment on attachment 302050 [details]
Patch

Whoops; I missed Dean's earlier comment.

[Dean Jackson]

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.

[Dean Jackson]

Created attachment 302081 [details]
Patch

[Dean Jackson]

Ready for review now. I'm still thinking of the best way to test, given a full test requires particular hardware.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Jon Lee]

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.

[Jon Lee]

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?

[Jon Lee]

Comment on attachment 302081 [details]
Patch

Most of my comments could be addressed in a later patch. r=me with bug tests fixed.

[Dean Jackson]

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.

[Dean Jackson]

Committed r212633: <http://trac.webkit.org/changeset/212633>