1194335 - Disable partial present on NVIDIA hardware

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

I have a theory that bug 1189940 is caused by partial present. I'd like to investigate this.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hey that's my theory too, but only because of the function names in the stack.. but I think everything goes through that function.  Is partial present a real win on real hardware?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #1)
> Hey that's my theory too, but only because of the function names in the
> stack.. but I think everything goes through that function.  Is partial
> present a real win on real hardware?

It should be on more bandwidth constrained hardware like non Iris Intel GPUs. I expect it makes less difference on NVIDIA hardware.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use a StackArray for RECTS so that we can see them in the minidumps

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/315e8590079c

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

In the Present1() docs, MSDN says "An app must confine its modifications to only the dirty regions that it passes to Present1" -- do we do that?  I seem to remember there were times when we'd do stuff like solid fills outside of the dirty region that we'd present, expecting to invalidate them fully if they're ever needed later.

Also this could be causing bug 1168018 (the animation certainly seems dirty-region related).

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/315e8590079c

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: Avoid partial present on Nvidia hardware

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 8663682 [details] [diff] [review]
Avoid partial present on Nvidia hardware

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

Let's see!

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +1090,5 @@
>      HRESULT hr = mSwapChain->QueryInterface((IDXGISwapChain1**)byRef(chain));
> +    nsString vendorID;
> +    services::GetGfxInfo()->GetAdapterVendorID(vendorID)
> +    if (SUCCEEDED(hr) && chain &&
> +        !vendorID.EqualsLiteral("0x10de")) {

If the bug isn't occurring with WARP let's take that into account here too, since vendorID will have the true adapter vendor regardless of the D3D device type. But the perf impact of this with WARP will be bad.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a17f2ca929d

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1a17f2ca929d

Comment 11

[Robert Kaiser]

Given that the main signatures of bug 1189940 have not occurred on Nightly since this landed, should we get this uplifted to higher channels? Or is bug 1208427 too much of an issue there?

Comment 12

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

Let's do an uplift to Aurora and Beta, 5% performance regression non-withstanding.  Clearly we have a stability issue with the partial present.  Jeff, can you request the uplifts?

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Milan, Bas: today is the go to build of beta 8, could you fill the uplift request asap so that we have it in beta 8? Thanks

Comment 14

[Robert Kaiser]

[Tracking Requested - why for this release]:
As this is the bug that is supposed to fix the #1 Win10 crash in bug 1189940, I'm requesting tracking here.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Tracking

Comment 16

[Bas Schouten (:bas.schouten)]

Comment on attachment 8663682 [details] [diff] [review]
Avoid partial present on Nvidia hardware

Approval Request Comment
[Feature/regressing bug #]: Partial Presentation
[User impact if declined]: Crashes on NVidia hardware
[Describe test coverage new/current, TreeHerder]: Nightly coverage
[Risks and why]: Extremely low, chance of a small performance regression.
[String/UUID change made/needed]: None

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Should be in 42 beta 8

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e6f7c5bd45c

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/341d7a3d7320

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Looks like it worked. Congrat!