Bug 187750 - Safari WebGL does not consistently provide correct GPU context on eGPU systems
Summary: Safari WebGL does not consistently provide correct GPU context on eGPU systems
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-17 23:52 PDT by Justin Fan
Modified: 2018-07-20 16:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (41.22 KB, patch)
2018-07-18 00:23 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (41.26 KB, patch)
2018-07-18 14:07 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (41.46 KB, patch)
2018-07-18 14:54 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (42.29 KB, patch)
2018-07-18 19:24 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.31 MB, application/zip)
2018-07-18 20:36 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.02 MB, application/zip)
2018-07-18 21:10 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.30 MB, application/zip)
2018-07-18 21:21 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.79 MB, application/zip)
2018-07-18 22:33 PDT, EWS Watchlist
no flags Details
Patch (42.95 KB, patch)
2018-07-19 14:54 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (44.15 KB, patch)
2018-07-19 17:05 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (43.40 KB, patch)
2018-07-19 17:23 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-sierra (3.21 MB, application/zip)
2018-07-19 19:18 PDT, EWS Watchlist
no flags Details
Patch (43.38 KB, patch)
2018-07-20 15:52 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2018-07-17 23:52:06 PDT
Safari WebGL does not consistently provide correct GPU context on eGPU systems
Comment 1 Justin Fan 2018-07-18 00:14:17 PDT
<rdar://problem/39531436>
Comment 2 Justin Fan 2018-07-18 00:23:12 PDT
Created attachment 345230 [details]
Patch
Comment 3 Per Arne Vollan 2018-07-18 07:09:21 PDT
Comment on attachment 345230 [details]
Patch

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

> Source/WebCore/page/Chrome.cpp:520
> +#if ENABLE(GRAPHICS_CONTEXT_3D)
> +    GraphicsContext3DManager::manager().screenDidChange(displayID);
> +#endif

I think this will change the GPU for all contexts in the process. There can be multiple contexts in multiple host windows. Shouldn't we only change the contexts belonging to this host window?
Comment 4 Dean Jackson 2018-07-18 13:46:03 PDT
Comment on attachment 345230 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).

This line goes under the title and before the description.

>> Source/WebCore/page/Chrome.cpp:520
>> +#endif
> 
> I think this will change the GPU for all contexts in the process. There can be multiple contexts in multiple host windows. Shouldn't we only change the contexts belonging to this host window?

I was hoping to avoid this for now, but yes, we should probably address it. I was really hoping to avoid having anything in platform know about HostWindow.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1286
> +    using PlatformDisplayID = uint32_t;
> +    void screenDidChange(PlatformDisplayID);

Maybe this should be #if PLATFORM(MAC)

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
> +    if (hostWindow && hostWindow->displayID())
> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));

Where did hostWindow come from? I think maybe this context should get its display id from the manager.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DManager.h:54
> +    static GraphicsContext3DManager& manager();

Should be called sharedManager()

> Source/WebCore/platform/mac/PlatformScreenMac.mm:119
> +    // The 0th renderer is the hardware renderer

End with a period.
Comment 5 Justin Fan 2018-07-18 14:07:44 PDT
Created attachment 345282 [details]
Patch
Comment 6 Justin Fan 2018-07-18 14:08:53 PDT
Made some changes to try to get other platforms to build; addressing Dean's comments now.
Comment 7 Justin Fan 2018-07-18 14:54:09 PDT
Created attachment 345288 [details]
Patch
Comment 8 Per Arne Vollan 2018-07-18 15:05:11 PDT
Comment on attachment 345288 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
> -#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> -    if (auto displayMask = primaryOpenGLDisplayMask()) {
> -        if (hostWindow && hostWindow->displayID())
> -            displayMask = displayMaskForDisplay(hostWindow->displayID());
> -        identifyAndSetCurrentGPU(pixelFormatObj, numPixelFormats, displayMask, m_contextObj);
> -    }
> +#if PLATFORM(MAC)
> +    if (hostWindow && hostWindow->displayID())
> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));

I think you need to use primaryOpenGLDisplayMask() if you don't have a host window or the display ID of the host window is invalid. Otherwise you will get the software renderer, I believe. This will give poor performance in those cases.
Comment 9 Per Arne Vollan 2018-07-18 15:24:42 PDT
Comment on attachment 345288 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
>> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));
> 
> I think you need to use primaryOpenGLDisplayMask() if you don't have a host window or the display ID of the host window is invalid. Otherwise you will get the software renderer, I believe. This will give poor performance in those cases.

Maybe you could create a primaryRendererID() function?
Comment 10 Per Arne Vollan 2018-07-18 17:34:27 PDT
Comment on attachment 345288 [details]
Patch

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

> Source/WebCore/platform/mac/PlatformScreenMac.mm:120
> +    // The 0th renderer is the hardware renderer.
> +    error = CGLDescribeRenderer(rendererInfo, 0, kCGLRPRendererID, &rendererID);

Is it guaranteed that the first renderer is the hardware renderer? Maybe you can assert that the renderer is accelerated by checking the kCGLRPAccelerated attribute?
Comment 11 Justin Fan 2018-07-18 18:09:50 PDT
Comment on attachment 345288 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
>>> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));
>> 
>> I think you need to use primaryOpenGLDisplayMask() if you don't have a host window or the display ID of the host window is invalid. Otherwise you will get the software renderer, I believe. This will give poor performance in those cases.
> 
> Maybe you could create a primaryRendererID() function?

If this context's window gets parented, it should change its renderer accordingly. So even if it's created with the software renderer at first, the user shouldn't see poor performance unless this is unable to identify the GPU driving the window. 

Let me see what happens on another device; I am having trouble getting ToT spade to work on my laptop.

>> Source/WebCore/platform/mac/PlatformScreenMac.mm:120
>> +    error = CGLDescribeRenderer(rendererInfo, 0, kCGLRPRendererID, &rendererID);
> 
> Is it guaranteed that the first renderer is the hardware renderer? Maybe you can assert that the renderer is accelerated by checking the kCGLRPAccelerated attribute?

This is reasonable; I'll do that.
Comment 12 Per Arne Vollan 2018-07-18 18:41:08 PDT
(In reply to Justin Fan from comment #11)
> Comment on attachment 345288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345288&action=review
> 
> >>> Source/WebCore/platform/graphics/cocoa/GraphicsContext3DCocoa.mm:262
> >>> +        identifyAndSetCurrentGPU(m_contextObj, pixelFormatObj, rendererIDForDisplay(hostWindow->displayID()));
> >> 
> >> I think you need to use primaryOpenGLDisplayMask() if you don't have a host window or the display ID of the host window is invalid. Otherwise you will get the software renderer, I believe. This will give poor performance in those cases.
> > 
> > Maybe you could create a primaryRendererID() function?
> 
> If this context's window gets parented, it should change its renderer
> accordingly. So even if it's created with the software renderer at first,
> the user shouldn't see poor performance unless this is unable to identify
> the GPU driving the window. 
> 
> Let me see what happens on another device; I am having trouble getting ToT
> spade to work on my laptop.
> 

Right, but wouldn't it be good to avoid the software renderer from the start? The primary renderer ID will be the correct one in most cases. It will only be wrong on a secondary screen. This will avoid a switch from the software renderer to a hardware renderer on every context creation.

> >> Source/WebCore/platform/mac/PlatformScreenMac.mm:120
> >> +    error = CGLDescribeRenderer(rendererInfo, 0, kCGLRPRendererID, &rendererID);
> > 
> > Is it guaranteed that the first renderer is the hardware renderer? Maybe you can assert that the renderer is accelerated by checking the kCGLRPAccelerated attribute?
> 
> This is reasonable; I'll do that.
Comment 13 Justin Fan 2018-07-18 19:24:51 PDT
Created attachment 345318 [details]
Patch
Comment 14 EWS Watchlist 2018-07-18 20:36:50 PDT
Comment on attachment 345318 [details]
Patch

Attachment 345318 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8582677

New failing tests:
webgl/max-active-contexts-webglcontextlost-prevent-default.html
webgl/max-active-contexts-oldest-context-lost.html
webgl/many-contexts-access-after-loss.html
Comment 15 EWS Watchlist 2018-07-18 20:36:51 PDT
Created attachment 345321 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 16 EWS Watchlist 2018-07-18 21:10:54 PDT
Comment on attachment 345318 [details]
Patch

Attachment 345318 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8582728

New failing tests:
webgl/max-active-contexts-oldest-context-lost.html
webgl/many-contexts-access-after-loss.html
Comment 17 EWS Watchlist 2018-07-18 21:10:55 PDT
Created attachment 345323 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 18 EWS Watchlist 2018-07-18 21:21:36 PDT
Comment on attachment 345318 [details]
Patch

Attachment 345318 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8582828

New failing tests:
webgl/max-active-contexts-oldest-context-lost.html
webgl/max-active-contexts-webglcontextlost-prevent-default.html
webgl/many-contexts-access-after-loss.html
Comment 19 EWS Watchlist 2018-07-18 21:21:38 PDT
Created attachment 345324 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 20 EWS Watchlist 2018-07-18 22:33:46 PDT
Comment on attachment 345318 [details]
Patch

Attachment 345318 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8583443

New failing tests:
webgl/many-contexts-access-after-loss.html
Comment 21 EWS Watchlist 2018-07-18 22:33:48 PDT
Created attachment 345328 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 22 Justin Fan 2018-07-19 14:54:03 PDT
Created attachment 345385 [details]
Patch
Comment 23 Dean Jackson 2018-07-19 15:27:38 PDT
Comment on attachment 345385 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:167
> +    ASSERT(!m_contexts.contains(contextWithWindow));

We should probably also ASSERT on the unlikely case that { context, nullptr } is present in m_contexts.

> Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:175
> +    ContextWithWindow contextWithWindow { context, nullptr };
> +    ASSERT(m_contexts.contains(contextWithWindow));
> +    m_contexts.removeFirst(contextWithWindow);

What am I misunderstanding here? You inserted the context with it's HostWindow pointer. But you're removing it with a nullptr. How is it being found?

(LATER) - oh, I see, it's got an operator==

In this case it might be better to use a HashMap of <GC3D*, HostWindow*>. You can still iterate over all the elements easily.
Comment 24 Justin Fan 2018-07-19 17:05:03 PDT
Created attachment 345402 [details]
Patch
Comment 25 Justin Fan 2018-07-19 17:06:56 PDT
Updated patch so that we never create a context, shared or not, with a nullptr hostWindow. 

So far I'd only been testing on iMac Pro. I'm still seeing unreliable behavior on my MacBook Pro. Let's not land this until I can figure out what's going on.
Comment 26 Justin Fan 2018-07-19 17:23:28 PDT
Created attachment 345407 [details]
Patch
Comment 27 Justin Fan 2018-07-19 17:47:03 PDT
On my laptop and the eGPU display, when asking the PixelFormatObj for the rendererID for a virtual screen, I get different numbers on some iterations through the virtual screens. Thus the preferred rendererIDs determined during process creation aren't matching any of them. 

99% sure this isn't happening on iMac Pro. Could be an issue with muxing mucking up contexts.
Comment 28 EWS Watchlist 2018-07-19 19:18:26 PDT
Comment on attachment 345407 [details]
Patch

Attachment 345407 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8594103

New failing tests:
http/wpt/offscreen-canvas/getContext-webgl.html
http/wpt/offscreen-canvas/transferToImageBitmap-webgl.html
Comment 29 EWS Watchlist 2018-07-19 19:18:28 PDT
Created attachment 345419 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 30 Justin Fan 2018-07-20 10:27:29 PDT
(In reply to Dean Jackson from comment #23)
> Comment on attachment 345385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345385&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext3DManager.cpp:167
> > +    ASSERT(!m_contexts.contains(contextWithWindow));
> 
> We should probably also ASSERT on the unlikely case that { context, nullptr
> } is present in m_contexts.

Oops, we can't actually do that because we can create offscreen canvases that aren't associated with a window. 

In that case the code should select the hardware renderer for the primary display.
Comment 31 Justin Fan 2018-07-20 15:52:10 PDT
Created attachment 345484 [details]
Patch
Comment 32 WebKit Commit Bot 2018-07-20 16:43:08 PDT
Comment on attachment 345484 [details]
Patch

Clearing flags on attachment: 345484

Committed r234074: <https://trac.webkit.org/changeset/234074>
Comment 33 WebKit Commit Bot 2018-07-20 16:43:10 PDT
All reviewed patches have been landed.  Closing bug.