Bug 189361 – [macOS] Switching to discrete GPU should be done in the UI process

• Component: WebGL
• Status: RESOLVED FIXED

[Per Arne Vollan]

WindowServer access is required to switch GPU.

[Per Arne Vollan]

rdar://problem/43949622

[Per Arne Vollan]

Created attachment 349090 [details]
Patch

[EWS Watchlist]

Attachment 349090 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 349090 [details]
Patch

What happens if the Web Content process dies.

[Simon Fraser (smfr)]

Comment on attachment 349090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349090&action=review

This seems like a weird way to fix this bug.

> Source/WebKit/WebProcess/WebCoreSupport/mac/WebGPUClient.cpp:43
> +    WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessProxy::EnableHighPerformanceGPU(), 0);

Doesn't this have to be sendSync? I would expect that we should enable the dGPU before we continue setting up the GL context, otherwise we'll get a context switch later.

[Per Arne Vollan]

Created attachment 349145 [details]
Patch

[Per Arne Vollan]

(In reply to Tim Horton from comment #4)
> Comment on attachment 349090 [details]
> Patch
> 
> What happens if the Web Content process dies.

That's a good point, I'll look into handling that. Thanks for reviewing!

[EWS Watchlist]

Attachment 349145 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 349090 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349090&action=review
> 
> This seems like a weird way to fix this bug.
> 
> > Source/WebKit/WebProcess/WebCoreSupport/mac/WebGPUClient.cpp:43
> > +    WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessProxy::EnableHighPerformanceGPU(), 0);
> 
> Doesn't this have to be sendSync? I would expect that we should enable the
> dGPU before we continue setting up the GL context, otherwise we'll get a
> context switch later.

That's also a good point. But since the context is already is created at this point, maybe a sync message is not needed? Thanks for reviewing!

[Per Arne Vollan]

Created attachment 349147 [details]
Patch

[EWS Watchlist]

Attachment 349147 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 349150 [details]
Patch

[EWS Watchlist]

Attachment 349150 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 349150 [details]
Patch

Echoing what Tim and Simon said. The switch should happen before the context is created.

[Per Arne Vollan]

Created attachment 349337 [details]
Patch

[EWS Watchlist]

Attachment 349337 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

(In reply to Dean Jackson from comment #14)
> Comment on attachment 349150 [details]
> Patch
> 
> Echoing what Tim and Simon said. The switch should happen before the context
> is created.

The latest patch will switch to discrete GPU before creating the OpenGL context. I am also considering using ChromeClient instead of the new GPUClient to tell the UI process to switch GPU. This would make the patch smaller.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 349344 [details]
Patch

[EWS Watchlist]

Attachment 349344 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Created attachment 349351 [details]
Patch

[EWS Watchlist]

Attachment 349351 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 349351 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349351&action=review

> Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:133
> +CGLPixelFormatObj GraphicsContext3DManager::enableHighPerformanceGPU()

It seems odd to me that a function of this name would return a CGLPixelFormatObj (though I understand why it does it)? How about instead, it returns an opaque token, and then also has the added benefit of being a platform agnostic interface.

> Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:142
> +    CGLPixelFormatAttribute attributes[] = { kCGLPFAAccelerated, kCGLPFAColorSize, static_cast<CGLPixelFormatAttribute>(32), static_cast<CGLPixelFormatAttribute>(0) };
> +    GLint numPixelFormats = 0;
> +    auto ret = CGLChoosePixelFormat(attributes, &pixelFormatObj, &numPixelFormats);
> +    ASSERT_UNUSED(ret, ret == kCGLNoError);

I think this could use a comment explaining why this is the method we are using to "enable High Performance GPU".  Is this the recommended way of doing even if we were only using Metal?

[Per Arne Vollan]

Created attachment 349451 [details]
Patch

[EWS Watchlist]

Attachment 349451 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

(In reply to Sam Weinig from comment #22)
> Comment on attachment 349351 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349351&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:133
> > +CGLPixelFormatObj GraphicsContext3DManager::enableHighPerformanceGPU()
> 
> It seems odd to me that a function of this name would return a
> CGLPixelFormatObj (though I understand why it does it)? How about instead,
> it returns an opaque token, and then also has the added benefit of being a
> platform agnostic interface.
> 
> > Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:142
> > +    CGLPixelFormatAttribute attributes[] = { kCGLPFAAccelerated, kCGLPFAColorSize, static_cast<CGLPixelFormatAttribute>(32), static_cast<CGLPixelFormatAttribute>(0) };
> > +    GLint numPixelFormats = 0;
> > +    auto ret = CGLChoosePixelFormat(attributes, &pixelFormatObj, &numPixelFormats);
> > +    ASSERT_UNUSED(ret, ret == kCGLNoError);
> 
> I think this could use a comment explaining why this is the method we are
> using to "enable High Performance GPU".  Is this the recommended way of
> doing even if we were only using Metal?

I don't think this is the recommended way if Metal is being used.

I have updated the patch, thanks for reviewing!

[Per Arne Vollan]

Created attachment 349459 [details]
Patch

[EWS Watchlist]

Attachment 349459 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.cpp:29:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 349459 [details]
Patch

Can we hold off until discussing this tomorrow please?

[Dean Jackson]

In my testing this does not release the discrete GPU.

[Dean Jackson]

<rdar://problem/43949622>

[Dean Jackson]

Created attachment 351268 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 351268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351268&action=review

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1518
> +    bool m_hasMuxed { false };

I think this needs a better name. "muxing" to me implies a switch in either direction, and someone not familiar with the term won't know at all. Maybe m_hasSwitchedToDiscreteGPU?

> Source/WebCore/platform/graphics/GraphicsContext3DManager.h:47
> +WEBCORE_EXPORT bool hasMuxableGPU();

That sounds like there's one GPU that's muxable. It's the system that's muxable, not the GPU.

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:336
> -#endif // !PLATFORM(MAC)
> +#endif

We like the ending comments.

> Source/WebKit/UIProcess/mac/HighPerformanceGPUManager.h:32
> +// FIXME: This class is using OpenGL to control the muxing of GPUs. Ultimately
> +// we want to use Metal, but currently there isn't a way to "release" a
> +// discrete MTLDevice, such that the process muxes back to an integrated GPU.

This comment belongs in the implementation.

> Source/WebKit/WebProcess/WebCoreSupport/mac/WebGPUClient.cpp:53
> +    LOG(WebGL, "WebGPUClient::releaseHighPerformanceGPU() from WebProcess");
> +    WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessProxy::ReleaseHighPerformanceGPU(), 0);

Doesn't this need to do counting? For now there's just a single client (GraphicsContext3D()), but when we add more this needs to store a per-process count of things wanting the high performance GPU.

> Source/WebKit/WebProcess/WebCoreSupport/mac/WebGPUClient.h:32
> +class WebGPUClient : public WebCore::GPUClient {

This is not about WebGPU.

> Source/WebKitLegacy/mac/WebCoreSupport/WebGPUClient.h:32
> +class WebGPUClient : public WebCore::GPUClient {

This sounds like it's related to WebGPU which it is not.

[Dean Jackson]

Created attachment 351344 [details]
Patch

[Dean Jackson]

Comment on attachment 351268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351268&action=review

>> Source/WebCore/platform/graphics/GraphicsContext3D.h:1518
>> +    bool m_hasMuxed { false };
> 
> I think this needs a better name. "muxing" to me implies a switch in either direction, and someone not familiar with the term won't know at all. Maybe m_hasSwitchedToDiscreteGPU?

Changed.

>> Source/WebCore/platform/graphics/GraphicsContext3DManager.h:47
>> +WEBCORE_EXPORT bool hasMuxableGPU();
> 
> That sounds like there's one GPU that's muxable. It's the system that's muxable, not the GPU.

Now hasLowAndHighPowerGPUs()

>> Source/WebKit/UIProcess/mac/HighPerformanceGPUManager.h:32
>> +// discrete MTLDevice, such that the process muxes back to an integrated GPU.
> 
> This comment belongs in the implementation.

Moved.

>> Source/WebKit/WebProcess/WebCoreSupport/mac/WebGPUClient.cpp:53
>> +    WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessProxy::ReleaseHighPerformanceGPU(), 0);
> 
> Doesn't this need to do counting? For now there's just a single client (GraphicsContext3D()), but when we add more this needs to store a per-process count of things wanting the high performance GPU.

GC3DManager does the counting. It's a singleton per web process.

>> Source/WebKit/WebProcess/WebCoreSupport/mac/WebGPUClient.h:32
>> +class WebGPUClient : public WebCore::GPUClient {
> 
> This is not about WebGPU.

Changed names

[Dean Jackson]

Created attachment 351348 [details]
Patch

[EWS Watchlist]

Attachment 351348 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebSwitchingGPUClient.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 351350 [details]
Patch

[EWS Watchlist]

Attachment 351350 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebSwitchingGPUClient.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 351350 [details]
Patch

Attachment 351350 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9423453

New failing tests:
fast/workers/worker-exception-during-navigation.html

[EWS Watchlist]

Created attachment 351371 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Dean Jackson]

Created attachment 351407 [details]
Patch

[EWS Watchlist]

Attachment 351407 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebSwitchingGPUClient.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 351421 [details]
Patch

[EWS Watchlist]

Attachment 351421 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebSwitchingGPUClient.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 351434 [details]
Patch

[EWS Watchlist]

Attachment 351434 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebSwitchingGPUClient.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Committed r236773: <https://trac.webkit.org/changeset/236773>