Bug 189361 - [macOS] Switching to discrete GPU should be done in the UI process
Summary: [macOS] Switching to discrete GPU should be done in the UI process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-06 11:25 PDT by Per Arne Vollan
Modified: 2018-10-02 16:42 PDT (History)
13 users (show)

See Also:


Attachments
Patch (51.04 KB, patch)
2018-09-06 16:39 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (54.24 KB, patch)
2018-09-07 08:27 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (54.24 KB, patch)
2018-09-07 08:49 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (54.25 KB, patch)
2018-09-07 09:05 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (55.62 KB, patch)
2018-09-10 15:37 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (55.70 KB, patch)
2018-09-10 15:54 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (55.43 KB, patch)
2018-09-10 16:16 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (55.51 KB, patch)
2018-09-11 14:18 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (55.52 KB, patch)
2018-09-11 14:43 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (61.71 KB, patch)
2018-10-01 10:51 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (58.40 KB, patch)
2018-10-01 23:25 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (62.39 KB, patch)
2018-10-02 00:12 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (62.35 KB, patch)
2018-10-02 00:24 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews203 for win-future (12.67 MB, application/zip)
2018-10-02 05:18 PDT, EWS Watchlist
no flags Details
Patch (62.37 KB, patch)
2018-10-02 10:08 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (62.33 KB, patch)
2018-10-02 11:46 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (62.39 KB, patch)
2018-10-02 13:15 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-09-06 11:25:50 PDT
WindowServer access is required to switch GPU.
Comment 1 Per Arne Vollan 2018-09-06 11:27:21 PDT
rdar://problem/43949622
Comment 2 Per Arne Vollan 2018-09-06 16:39:35 PDT
Created attachment 349090 [details]
Patch
Comment 3 EWS Watchlist 2018-09-06 16:40:55 PDT
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.
Comment 4 Tim Horton 2018-09-06 16:42:03 PDT
Comment on attachment 349090 [details]
Patch

What happens if the Web Content process dies.
Comment 5 Simon Fraser (smfr) 2018-09-06 20:39:03 PDT
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.
Comment 6 Per Arne Vollan 2018-09-07 08:27:37 PDT
Created attachment 349145 [details]
Patch
Comment 7 Per Arne Vollan 2018-09-07 08:28:37 PDT
(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!
Comment 8 EWS Watchlist 2018-09-07 08:30:49 PDT
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.
Comment 9 Per Arne Vollan 2018-09-07 08:31:22 PDT
(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!
Comment 10 Per Arne Vollan 2018-09-07 08:49:40 PDT
Created attachment 349147 [details]
Patch
Comment 11 EWS Watchlist 2018-09-07 08:52:55 PDT
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.
Comment 12 Per Arne Vollan 2018-09-07 09:05:30 PDT
Created attachment 349150 [details]
Patch
Comment 13 EWS Watchlist 2018-09-07 09:07:50 PDT
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.
Comment 14 Dean Jackson 2018-09-07 11:27:42 PDT
Comment on attachment 349150 [details]
Patch

Echoing what Tim and Simon said. The switch should happen before the context is created.
Comment 15 Per Arne Vollan 2018-09-10 15:37:17 PDT
Created attachment 349337 [details]
Patch
Comment 16 EWS Watchlist 2018-09-10 15:40:34 PDT
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.
Comment 17 Per Arne Vollan 2018-09-10 15:42:25 PDT
(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!
Comment 18 Per Arne Vollan 2018-09-10 15:54:58 PDT
Created attachment 349344 [details]
Patch
Comment 19 EWS Watchlist 2018-09-10 15:57:45 PDT
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.
Comment 20 Per Arne Vollan 2018-09-10 16:16:37 PDT
Created attachment 349351 [details]
Patch
Comment 21 EWS Watchlist 2018-09-10 16:19:14 PDT
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.
Comment 22 Sam Weinig 2018-09-11 10:45:34 PDT
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?
Comment 23 Per Arne Vollan 2018-09-11 14:18:26 PDT
Created attachment 349451 [details]
Patch
Comment 24 EWS Watchlist 2018-09-11 14:20:38 PDT
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.
Comment 25 Per Arne Vollan 2018-09-11 14:21:26 PDT
(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!
Comment 26 Per Arne Vollan 2018-09-11 14:43:32 PDT
Created attachment 349459 [details]
Patch
Comment 27 EWS Watchlist 2018-09-11 14:47:57 PDT
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.
Comment 28 Simon Fraser (smfr) 2018-09-11 14:53:55 PDT
Comment on attachment 349459 [details]
Patch

Can we hold off until discussing this tomorrow please?
Comment 29 Dean Jackson 2018-09-25 10:20:58 PDT
In my testing this does not release the discrete GPU.
Comment 30 Dean Jackson 2018-10-01 10:18:33 PDT
<rdar://problem/43949622>
Comment 31 Dean Jackson 2018-10-01 10:51:57 PDT
Created attachment 351268 [details]
Patch
Comment 32 Simon Fraser (smfr) 2018-10-01 11:11:49 PDT
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.
Comment 33 Dean Jackson 2018-10-01 23:25:10 PDT
Created attachment 351344 [details]
Patch
Comment 34 Dean Jackson 2018-10-01 23:27:57 PDT
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
Comment 35 Dean Jackson 2018-10-02 00:12:12 PDT
Created attachment 351348 [details]
Patch
Comment 36 EWS Watchlist 2018-10-02 00:15:02 PDT
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.
Comment 37 Dean Jackson 2018-10-02 00:24:53 PDT
Created attachment 351350 [details]
Patch
Comment 38 EWS Watchlist 2018-10-02 00:26:07 PDT
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.
Comment 39 EWS Watchlist 2018-10-02 05:18:43 PDT
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
Comment 40 EWS Watchlist 2018-10-02 05:18:57 PDT
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
Comment 41 Dean Jackson 2018-10-02 10:08:52 PDT
Created attachment 351407 [details]
Patch
Comment 42 EWS Watchlist 2018-10-02 10:12:19 PDT
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.
Comment 43 Dean Jackson 2018-10-02 11:46:28 PDT
Created attachment 351421 [details]
Patch
Comment 44 EWS Watchlist 2018-10-02 11:48:31 PDT
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.
Comment 45 Dean Jackson 2018-10-02 13:15:43 PDT
Created attachment 351434 [details]
Patch
Comment 46 EWS Watchlist 2018-10-02 13:18:32 PDT
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.
Comment 47 Dean Jackson 2018-10-02 16:42:55 PDT
Committed r236773: <https://trac.webkit.org/changeset/236773>