WindowServer access is required to switch GPU.
rdar://problem/43949622
Created attachment 349090 [details] Patch
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 on attachment 349090 [details] Patch What happens if the Web Content process dies.
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.
Created attachment 349145 [details] Patch
(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!
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.
(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!
Created attachment 349147 [details] Patch
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.
Created attachment 349150 [details] Patch
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 on attachment 349150 [details] Patch Echoing what Tim and Simon said. The switch should happen before the context is created.
Created attachment 349337 [details] Patch
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.
(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!
Created attachment 349344 [details] Patch
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.
Created attachment 349351 [details] Patch
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 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?
Created attachment 349451 [details] Patch
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.
(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!
Created attachment 349459 [details] Patch
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 on attachment 349459 [details] Patch Can we hold off until discussing this tomorrow please?
In my testing this does not release the discrete GPU.
<rdar://problem/43949622>
Created attachment 351268 [details] Patch
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.
Created attachment 351344 [details] Patch
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
Created attachment 351348 [details] Patch
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.
Created attachment 351350 [details] Patch
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 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
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
Created attachment 351407 [details] Patch
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.
Created attachment 351421 [details] Patch
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.
Created attachment 351434 [details] Patch
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.
Committed r236773: <https://trac.webkit.org/changeset/236773>