Programatically enable/disable gpus to test our ability to recover from a TDR

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mchang, Assigned: domfarolino)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
x86
Windows 10
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(3 attachments, 12 obsolete attachments)

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
(Reporter)

Description

2 years ago
So we can practice TDR recovery.
(Reporter)

Updated

2 years ago
Whiteboard: gfx-noted
(Reporter)

Updated

2 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.
See Also: → bug 1245843
Assignee: mchang → domfarolino
(Assignee)

Comment 2

a year ago
Created attachment 8871894 [details] [diff] [review]
part 1, expose API

Multi-part bug. Part 1 exposes the API to trigger a device reset on all platforms.
Attachment #8871894 - Flags: review?(dvander)
(Assignee)

Comment 3

a year ago
Created attachment 8871924 [details] [diff] [review]
Part 1, expose API

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

a year ago
Created attachment 8871982 [details] [diff] [review]
New bug1274663.patch

Address dvander's comments.
Attachment #8871924 - Attachment is obsolete: true
Attachment #8871982 - Flags: review?(dvander)
(Assignee)

Comment 6

a year ago
Created attachment 8871995 [details] [diff] [review]
bug1274663.patch

Think I finally got the diff of part 1 right.
Attachment #8871995 - Flags: review?(dvander)
(Assignee)

Updated

a year ago
Attachment #8871982 - Attachment is obsolete: true
Attachment #8871982 - Flags: review?(dvander)
(Assignee)

Comment 8

a year ago
Created attachment 8872769 [details] [diff] [review]
bug1274663.patch

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

a year ago
Created attachment 8873535 [details] [diff] [review]
bug1274663-2.patch

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

a year ago
Created attachment 8873554 [details] [diff] [review]
bug1274663.patch

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

a year ago
Created attachment 8873588 [details] [diff] [review]
bug1274663-3.patch

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

a year ago
Created attachment 8874569 [details] [diff] [review]
bug1274663-3.patch
Attachment #8873588 - Attachment is obsolete: true
Attachment #8874569 - Flags: review?(milan)
(Assignee)

Comment 15

a year ago
Created attachment 8874579 [details] [diff] [review]
bug1274663-2.patch

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+
(Assignee)

Comment 16

a year ago
Created attachment 8876891 [details] [diff] [review]
bug1274663-2.patch

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

a year ago
Created attachment 8877794 [details] [diff] [review]
bug1274663-3.patch

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+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Needs rebased patches. A link to a recent Try run would be nice too.
Keywords: checkin-needed
(Assignee)

Comment 19

a year ago
Created attachment 8878211 [details] [diff] [review]
bug1274663-1.patch

Rebased version of patch 1
Attachment #8873554 - Attachment is obsolete: true
Attachment #8878211 - Flags: review?(dvander)
(Assignee)

Comment 20

a year ago
Created attachment 8878212 [details] [diff] [review]
bug1274663-2.patch

Rebased version of part 2 (adding the test case)
Attachment #8876891 - Attachment is obsolete: true
Attachment #8878212 - Flags: review?(dvander)
(Assignee)

Comment 21

a year ago
Created attachment 8878213 [details] [diff] [review]
bug1274663-3.patch

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+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 23

a year 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
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
Last Resolved: a year 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.