RESOLVED FIXED 141567
WebGL: Destroy the GLContext when a GPU restart has been detected.
https://bugs.webkit.org/show_bug.cgi?id=141567
Summary WebGL: Destroy the GLContext when a GPU restart has been detected.
Roger Fong
Reported 2015-02-13 10:44:02 PST
Attachments
patch (2.46 KB, patch)
2015-02-13 10:53 PST, Roger Fong
no flags
patch (6.49 KB, patch)
2015-02-19 15:10 PST, Roger Fong
dino: review+
patch (9.34 KB, patch)
2015-02-19 18:56 PST, Roger Fong
no flags
patch (11.95 KB, patch)
2015-02-23 23:50 PST, Roger Fong
no flags
patch (16.65 KB, patch)
2015-02-24 12:53 PST, Roger Fong
no flags
patch (17.39 KB, patch)
2015-02-24 13:11 PST, Roger Fong
no flags
patch (17.43 KB, patch)
2015-02-24 13:17 PST, Roger Fong
no flags
patch (17.63 KB, patch)
2015-02-24 13:32 PST, Roger Fong
dino: review+
Roger Fong
Comment 1 2015-02-13 10:53:04 PST
Dean Jackson
Comment 2 2015-02-13 11:12:21 PST
Comment on attachment 246534 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=246534&action=review > Source/WebCore/platform/graphics/mac/WebGLLayer.mm:93 > + GLint restartStatus = 0; > + CGLGetParameter(glContext, kCGLCPGPURestartStatus, &restartStatus); > + if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) { > + CGLDestroyContext(glContext); > + return; > + } > + As discussed in IRC, I think we need a little more intelligence here. 1. Check that this triggers a WebGL context lost (or make sure that this does that) 2. Ensure a page doesn't keep recreating the context in this case and submitting the same bad data. Maybe allow 3 lost contexts before crashing the process?
Roger Fong
Comment 3 2015-02-19 15:10:31 PST
Roger Fong
Comment 4 2015-02-19 15:26:59 PST
(In reply to comment #3) > Created attachment 246923 [details] > patch I've now triggered a context lost event and made sure that the context does not get restored. It also adopts the same reload policy as all other webcontent crashes. 3 tries and an error.
Dean Jackson
Comment 5 2015-02-19 15:38:41 PST
Comment on attachment 246923 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=246923&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:1445 > + > + WebGLRenderingContextBase* m_webglContext; Do we ever think there might be a time when the WebGL context goes away before the GC3D? > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:202 > + GLint abortOnBlacklist = 0; > + CGLSetParameter(m_contextObj, kCGLCPAbortOnGPURestartStatusBlacklisted, &abortOnBlacklist); You'll need to #if around these since they are only available on OS X 10.10+ > Source/WebCore/platform/graphics/mac/WebGLLayer.mm:180 > + GLint restartStatus = 0; > + CGLGetParameter(_context->platformGraphicsContext3D(), kCGLCPGPURestartStatus, &restartStatus); > + if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) { > + CGLDestroyContext(_context->platformGraphicsContext3D()); > + _context->forceContextLost(); > + } Similarly here. One concern I have is that we're only doing this test on display. Which means you could hose the system if you executed a lot of draw calls before allowing the system to draw. We might want to have this code in the drawing entry points, drawElements and drawArrays, but only test every X iterations or something. Anyway, if this makes an improvement on our hanging examples, let's stick with it.
Roger Fong
Comment 6 2015-02-19 17:58:16 PST
(In reply to comment #5) > Comment on attachment 246923 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246923&action=review > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1445 > > + > > + WebGLRenderingContextBase* m_webglContext; > > Do we ever think there might be a time when the WebGL context goes away > before the GC3D? Hmm, if the destructor for WebGLRenderingContextBase gets called we always also destroy the associated graphicsContext3D, so this shouldn't be an issue. I believe I added a nullcheck to the forceContextLost call just in case. Maybe an assert would be more appropriate though. > > > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:202 > > + GLint abortOnBlacklist = 0; > > + CGLSetParameter(m_contextObj, kCGLCPAbortOnGPURestartStatusBlacklisted, &abortOnBlacklist); > > You'll need to #if around these since they are only available on OS X 10.10+ > > > Source/WebCore/platform/graphics/mac/WebGLLayer.mm:180 > > + GLint restartStatus = 0; > > + CGLGetParameter(_context->platformGraphicsContext3D(), kCGLCPGPURestartStatus, &restartStatus); > > + if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) { > > + CGLDestroyContext(_context->platformGraphicsContext3D()); > > + _context->forceContextLost(); > > + } > > Similarly here. > > One concern I have is that we're only doing this test on display. Which > means you could hose the system if you executed a lot of draw calls before > allowing the system to draw. We might want to have this code in the drawing > entry points, drawElements and drawArrays, but only test every X iterations > or something. How does 5 iterations sound? > > Anyway, if this makes an improvement on our hanging examples, let's stick > with it. Hmm if I do stick this stuff in drawArrays I feel like I won't need to have it in the display method, since the draw methods will always come first. Does that sound right? Although, I suppose, it could help though if there is only 1 draw call, we'll probably want to catch the error before having to attempt to draw more frames. (Thus in the display step), so maybe ew want both.
Roger Fong
Comment 7 2015-02-19 18:56:04 PST
Roger Fong
Comment 8 2015-02-23 23:50:05 PST
Created attachment 247207 [details] patch Checks GPU status in draw method calls as well. Also does the appropriate gpu checking for iOS, which uses a slightly different API.
Roger Fong
Comment 9 2015-02-24 12:53:31 PST
WebKit Commit Bot
Comment 10 2015-02-24 12:56:00 PST
Attachment 247256 [details] did not pass style-queue: ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:36: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:37: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Roger Fong
Comment 11 2015-02-24 13:11:29 PST
WebKit Commit Bot
Comment 12 2015-02-24 13:14:09 PST
Attachment 247258 [details] did not pass style-queue: ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:36: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:37: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Roger Fong
Comment 13 2015-02-24 13:17:33 PST
WebKit Commit Bot
Comment 14 2015-02-24 13:19:27 PST
Attachment 247259 [details] did not pass style-queue: ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:40: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Roger Fong
Comment 15 2015-02-24 13:32:03 PST
WebKit Commit Bot
Comment 16 2015-02-24 13:34:13 PST
Attachment 247262 [details] did not pass style-queue: ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:40: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dean Jackson
Comment 17 2015-02-24 15:31:04 PST
Comment on attachment 247262 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=247262&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:1278 > + // Call to make during draw calls to check on the GPU's status Nit: missing . Naming comments coming. > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:68 > +const int checkGPUStatusDrawIterations = 5; Maybe GPUStatusCheckThreshold? > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:69 > +int GraphicsContext3D::drawCounter = 0; And checkCounter? I don't do names well, so whatever you think is best. > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:370 > +void GraphicsContext3D::checkGPUStatus() Suggest naming this checkGPUStatusIfNecessary or something. > Source/WebCore/platform/graphics/mac/WebGLLayer.mm:193 > + GLint restartStatus = 0; > + EAGLContext* currentContext = _context->platformGraphicsContext3D(); > + [currentContext getParameter:kEAGLCPGPURestartStatus to:&restartStatus]; > + if (restartStatus == kEAGLCPGPURestartStatusCaused || restartStatus == kEAGLCPGPURestartStatusBlacklisted) { > + [EAGLContext setCurrentContext:0]; > + [static_cast<EAGLContext*>(currentContext) release]; > + _context->forceContextLost(); > + } > #else > [super display]; > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000 > + GLint restartStatus = 0; > + CGLGetParameter(_context->platformGraphicsContext3D(), kCGLCPGPURestartStatus, &restartStatus); > + if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) { > + CGLSetCurrentContext(0); > + CGLDestroyContext(_context->platformGraphicsContext3D()); > + _context->forceContextLost(); > + } > +#endif Do you think we need to test here? These calls should be fine. It's the actual draw calls that you tested above that would cause a restart. Unless you disagree, I say remove these ones. > Source/WebCore/platform/spi/ios/OpenGLESSPI.h:2 > + * Copyright (C) 2014 Apple Inc. All rights reserved. 2015
Roger Fong
Comment 18 2015-02-24 20:20:34 PST
Roger Fong
Comment 19 2015-02-24 20:21:06 PST
(In reply to comment #18) > http://trac.webkit.org/changeset/180609 With Dean's fixed applied.
Ryosuke Niwa
Comment 20 2015-02-24 21:12:39 PST
Landed a Mavericks build fix attempt: https://trac.webkit.org/changeset/180614
Roger Fong
Comment 21 2015-02-24 21:22:49 PST
(In reply to comment #20) > Landed a Mavericks build fix attempt: > https://trac.webkit.org/changeset/180614 Thanks rniwa!
Note You need to log in before you can comment on or make changes to this bug.