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)

x86
Windows 10
defect
Not set
normal

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.
Whiteboard: gfx-noted
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.
See Also: → 1245843
Assignee: mchang → domfarolino
Attached patch part 1, expose API (obsolete) — Splinter Review
Multi-part bug. Part 1 exposes the API to trigger a device reset on all platforms.
Attachment #8871894 - Flags: review?(dvander)
Attached patch Part 1, expose API (obsolete) — Splinter Review
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)
Attached patch New bug1274663.patch (obsolete) — Splinter Review
Address dvander's comments.
Attachment #8871924 - Attachment is obsolete: true
Attachment #8871982 - Flags: review?(dvander)
Attached patch bug1274663.patch (obsolete) — Splinter Review
Think I finally got the diff of part 1 right.
Attachment #8871995 - Flags: review?(dvander)
Attachment #8871982 - Attachment is obsolete: true
Attachment #8871982 - Flags: review?(dvander)
Attached patch bug1274663.patch (obsolete) — Splinter Review
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+
Attached patch bug1274663-2.patch (obsolete) — Splinter Review
Adding a chrome mochitest to test that drawing on the screen after a device reset is visible.
Attachment #8873535 - Flags: review?(dvander)
Attached patch bug1274663.patch (obsolete) — Splinter Review
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+
Attached patch bug1274663-3.patch (obsolete) — Splinter Review
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+
Attached patch bug1274663-3.patch (obsolete) — Splinter Review
Attachment #8873588 - Attachment is obsolete: true
Attachment #8874569 - Flags: review?(milan)
Attached patch bug1274663-2.patch (obsolete) — Splinter Review
Added missing SimpleTest.finish()
Attachment #8873535 - Attachment is obsolete: true
Attachment #8873535 - Flags: review?(dvander)
Attachment #8874579 - Flags: review?(dvander)
Attachment #8874569 - Flags: review?(milan) → review+
Attached patch bug1274663-2.patch (obsolete) — Splinter Review
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+
Attached patch bug1274663-3.patch (obsolete) — Splinter Review
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+
Attachment #8877794 - Flags: review?(milan) → review+
Keywords: checkin-needed
Needs rebased patches. A link to a recent Try run would be nice too.
Keywords: checkin-needed
Rebased version of patch 1
Attachment #8873554 - Attachment is obsolete: true
Attachment #8878211 - Flags: review?(dvander)
Rebased version of part 2 (adding the test case)
Attachment #8876891 - Attachment is obsolete: true
Attachment #8878212 - Flags: review?(dvander)
Rebased patch 3, adding test device reset button to about support
Attachment #8877794 - Attachment is obsolete: true
Attachment #8878213 - Flags: review?(milan)
Attachment #8878213 - Flags: review?(milan) → review+
Attachment #8878211 - Flags: review?(dvander) → review+
Attachment #8878212 - Flags: review?(dvander) → review+
Keywords: checkin-needed
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: