Closed
Bug 1274663
Opened 8 years ago
Closed 7 years ago
Programatically enable/disable gpus to test our ability to recover from a TDR
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: mchang, Assigned: domfarolino)
References
(Blocks 1 open bug)
Details
(Whiteboard: gfx-noted)
Attachments
(3 files, 12 obsolete files)
34.46 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
4.22 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
2.49 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
So we can practice TDR recovery.
Reporter | ||
Updated•8 years ago
|
Whiteboard: gfx-noted
Reporter | ||
Updated•8 years ago
|
Summary: Pragmatically enable/disable gpus to test our ability to recover from a TDR → Programatically enable/disable gpus to test our ability to recover from a TDR
Note there's a few prefs to help test TDRs: gfx.testing.device-fail N!=0 will simulate D3D11CreateDevice returning null. (We should probably rename that to d3d11-device-fail). Note that this will prevent another D3D11 device from being created so the browser must be restarted to test it multiple times. gfx.testing.device-reset N!=0 will trigger the TDR-handling path by making DidRenderingDeviceReset return true. This will work even if not using acceleration. After the pref is automatically reset to 0.
Assignee: mchang → domfarolino
Assignee | ||
Comment 2•7 years ago
|
||
Multi-part bug. Part 1 exposes the API to trigger a device reset on all platforms.
Attachment #8871894 -
Flags: review?(dvander)
Assignee | ||
Comment 3•7 years ago
|
||
Part 1, expose API to trigger device reset
Attachment #8871894 -
Attachment is obsolete: true
Attachment #8871894 -
Flags: review?(dvander)
Attachment #8871924 -
Flags: review?(dvander)
Comment on attachment 8871924 [details] [diff] [review] Part 1, expose API Review of attachment 8871924 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, this is pretty much everything we'll need to start getting better automated and manual testing. I'd like to see one more iteration before signing off. ::: dom/base/nsContentList.h @@ -48,5 @@ > // nsINodeList > virtual int32_t IndexOf(nsIContent* aContent) override; > virtual nsIContent* Item(uint32_t aIndex) override; > > - uint32_t Length() const { Since you're not actually modifying this file (nsContentList.h), let's exclude it from the changeset. ::: gfx/ipc/GPUProcessManager.cpp @@ +345,5 @@ > return false; > } > > void > +GPUProcessManager::TriggerDeviceResetForTesting() { For function bodies in a .cpp file, the opening { goes on its own line. (Looks like a few other places will need to be fixed up as well.) ::: gfx/ipc/GPUProcessManager.h @@ +201,5 @@ > + void RegisterInProcessSession(InProcessCompositorSession* aSession); > + void UnregisterInProcessSession(InProcessCompositorSession* aSession); > + > + // Used to notify content windows and children of a device reset > + void OnDeviceReset(); It looks like this function is not defined or called, so it can probably be removed. ::: gfx/ipc/InProcessCompositorSession.cpp @@ +35,5 @@ > bool aUseExternalSurfaceSize, > const gfx::IntSize& aSurfaceSize, > uint32_t aNamespace) > { > + nit: extra newline got introduced @@ +50,5 @@ > + > +void > +InProcessCompositorSession::NotifySessionLost() > +{ > + MOZ_ASSERT(mWidget); No need to MOZ_ASSERT here, if it crashes it will be pretty obvious why. ::: gfx/layers/client/ClientLayerManager.cpp @@ -116,5 @@ > { > mMemoryPressureObserver->Destroy(); > ClearCachedResources(); > // Stop receiveing AsyncParentMessage at Forwarder. > - // After the call, the message is directly handled by LayerTransactionChild. ClientLayerManager.h only has whitespace changes so let's exclude it from this changeset. ::: gfx/layers/client/ClientPaintedLayer.cpp @@ -36,5 @@ > js::ProfileEntry::Category::GRAPHICS); > > NS_ASSERTION(ClientManager()->InDrawing(), > "Can only draw in drawing phase"); > - Same for ClientLayerManager.cpp. ::: widget/windows/nsWindowGfx.cpp @@ +178,5 @@ > gfxCriticalNote << "(nsWindow) Detected device reset: " << (int)resetReason; > > gfxWindowsPlatform::GetPlatform()->UpdateRenderMode(); > > + GPUProcessManager::Get()->OnProcessDeviceReset(nullptr); Did you mean OnInProcessDeviceReset() here?
Attachment #8871924 -
Flags: review?(dvander)
Assignee | ||
Comment 5•7 years ago
|
||
Address dvander's comments.
Attachment #8871924 -
Attachment is obsolete: true
Attachment #8871982 -
Flags: review?(dvander)
Assignee | ||
Comment 6•7 years ago
|
||
Think I finally got the diff of part 1 right.
Attachment #8871995 -
Flags: review?(dvander)
Assignee | ||
Updated•7 years ago
|
Attachment #8871982 -
Attachment is obsolete: true
Attachment #8871982 -
Flags: review?(dvander)
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9217b244ec49b8d151ecd4981e66affa58d47af2
Assignee | ||
Comment 8•7 years ago
|
||
Should fix the nsWindowGfx.cpp issue
Attachment #8871995 -
Attachment is obsolete: true
Attachment #8871995 -
Flags: review?(dvander)
Attachment #8872769 -
Flags: review?(dvander)
Comment on attachment 8872769 [details] [diff] [review] bug1274663.patch Review of attachment 8872769 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #8872769 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 10•7 years ago
|
||
Adding a chrome mochitest to test that drawing on the screen after a device reset is visible.
Attachment #8873535 -
Flags: review?(dvander)
Assignee | ||
Comment 11•7 years ago
|
||
Small style change to obsolete part 1 patch
Attachment #8872769 -
Attachment is obsolete: true
Attachment #8873554 -
Flags: review?(dvander)
Attachment #8873554 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 12•7 years ago
|
||
This adds an entry under the graphics section in about:support to trigger a device reset
Attachment #8873588 -
Flags: review?(milan)
Comment on attachment 8873588 [details] [diff] [review] bug1274663-3.patch r+ after a change to the patch description; mention that we're adding the command button to trigger a device reset in about:support in nightly & dev edition, rather than just about:support.
Attachment #8873588 -
Flags: review?(milan) → review+
Assignee | ||
Comment 14•7 years ago
|
||
Attachment #8873588 -
Attachment is obsolete: true
Attachment #8874569 -
Flags: review?(milan)
Assignee | ||
Comment 15•7 years ago
|
||
Added missing SimpleTest.finish()
Attachment #8873535 -
Attachment is obsolete: true
Attachment #8873535 -
Flags: review?(dvander)
Attachment #8874579 -
Flags: review?(dvander)
Updated•7 years ago
|
Attachment #8874569 -
Flags: review?(milan) → review+
Assignee | ||
Comment 16•7 years ago
|
||
Added GPU subsuite line to chrome.ini
Attachment #8874579 -
Attachment is obsolete: true
Attachment #8874579 -
Flags: review?(dvander)
Attachment #8876891 -
Flags: review?(dvander)
Attachment #8876891 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 17•7 years ago
|
||
One line change to make sure the button is not available on Mac so it doesn't totally nuke the UI as per my conversation with David.
Attachment #8874569 -
Attachment is obsolete: true
Attachment #8877794 -
Flags: review?(milan)
Attachment #8877794 -
Flags: review?(dvander)
Attachment #8877794 -
Flags: review?(dvander) → review+
Updated•7 years ago
|
Attachment #8877794 -
Flags: review?(milan) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 18•7 years ago
|
||
Needs rebased patches. A link to a recent Try run would be nice too.
Keywords: checkin-needed
Assignee | ||
Comment 19•7 years ago
|
||
Rebased version of patch 1
Attachment #8873554 -
Attachment is obsolete: true
Attachment #8878211 -
Flags: review?(dvander)
Assignee | ||
Comment 20•7 years ago
|
||
Rebased version of part 2 (adding the test case)
Attachment #8876891 -
Attachment is obsolete: true
Attachment #8878212 -
Flags: review?(dvander)
Assignee | ||
Comment 21•7 years ago
|
||
Rebased patch 3, adding test device reset button to about support
Attachment #8877794 -
Attachment is obsolete: true
Attachment #8878213 -
Flags: review?(milan)
Assignee | ||
Comment 22•7 years ago
|
||
Recent try submission: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d020dd47fc7233d18afde7cf1f5b152b44c5da52
Updated•7 years ago
|
Attachment #8878213 -
Flags: review?(milan) → review+
Attachment #8878211 -
Flags: review?(dvander) → review+
Attachment #8878212 -
Flags: review?(dvander) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 23•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/db5cc36763c1 Expose API to trigger device reset. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/03681158d59c Add mochitest. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/50f92f81d4da Add command button to trigger device reset in nightly & dev about:support. r=milan
Keywords: checkin-needed
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/db5cc36763c1 https://hg.mozilla.org/mozilla-central/rev/03681158d59c https://hg.mozilla.org/mozilla-central/rev/50f92f81d4da
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•