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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

So we can practice TDR recovery.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 2

[Dominic Farolino [:domfarolino]]

Attachment: part 1, expose API

Multi-part bug. Part 1 exposes the API to trigger a device reset on all platforms.

Comment 3

[Dominic Farolino [:domfarolino]]

Attachment: Part 1, expose API

Part 1, expose API to trigger device reset

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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?

Comment 5

[Dominic Farolino [:domfarolino]]

Attachment: New bug1274663.patch

Address dvander's comments.

Comment 6

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663.patch

Think I finally got the diff of part 1 right.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9217b244ec49b8d151ecd4981e66affa58d47af2

Comment 8

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663.patch

Should fix the nsWindowGfx.cpp issue

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8872769 [details] [diff] [review]
bug1274663.patch

Review of attachment 8872769 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 10

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663-2.patch

Adding a chrome mochitest to test that drawing on the screen after a device reset is visible.

Comment 11

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663.patch

Small style change to obsolete part 1 patch

Comment 12

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663-3.patch

This adds an entry under the graphics section in about:support to trigger a device reset

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 14

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663-3.patch

Comment 15

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663-2.patch

Added missing SimpleTest.finish()

Comment 16

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663-2.patch

Added GPU subsuite line to chrome.ini

Comment 17

[Dominic Farolino [:domfarolino]]

Attachment: 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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Needs rebased patches. A link to a recent Try run would be nice too.

Comment 19

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663-1.patch

Rebased version of patch 1

Comment 20

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663-2.patch

Rebased version of part 2 (adding the test case)

Comment 21

[Dominic Farolino [:domfarolino]]

Attachment: bug1274663-3.patch

Rebased patch 3, adding test device reset button to about support

Comment 22

[Dominic Farolino [:domfarolino]]

Recent try submission: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d020dd47fc7233d18afde7cf1f5b152b44c5da52

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/db5cc36763c1
https://hg.mozilla.org/mozilla-central/rev/03681158d59c
https://hg.mozilla.org/mozilla-central/rev/50f92f81d4da