Closed Bug 1297822 Opened 8 years ago Closed 8 years ago

Disable the GPU process on older versions of Windows 7

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

Direct3D 11 cannot draw to remote HWNDs on Windows 7 unless the Platform Update is installed. We don't have direct data on how many users this affects, but we can use Direct2D 1.1 presence as a proxy:

17.7% of users are on Windows 7.
37.4% of users are on Windows 7 SP1.
  Of these, 43.8% do not have Direct2D 1.1 installed.
17.7 + (37.4 * 0.438) =
-----------------------
About 34% of users have Windows 7 and cannot use Direct3D 11 in the GPU process.

Our options are: kick those users down to Direct3D 9 (which is what Chrome does), or turn off the GPU process on those platforms.

Ultimately this is a product decision, but in the interest of not shipping a regression for a large number of users, I think we should disable the GPU process on older Windows 7 systems. We can change it later, but it seems like a good place to start.
Whiteboard: [gfx-noted]
Assignee: nobody → dvander
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
This patch makes some significant changes to how we decide to use the GPU process.
 (1) It will not be used in Safe Mode.
 (2) It will not be used on Windows XP, Vista, or 7 Pre-SP1.
 (3) On Windows 7 SP1, we include a check for the Platform Update. This is not ideal - better to rely on it in the GPU process - but for v1 it seems okay.

Downside is that this will make our test surface a little worse on TreeHerder, since the Windows XP and 7 machines will stop testing the Basic Compositor in the GPU process. Granted, it should be very rare that we fail to grab a remote D3D11 device after passing all our UI-process checks. But it's still possible, and that scenario will be untested.

Maybe we should have the UI process nuke the GPU process if it gets back a basic compositor?
Attachment #8804518 - Flags: review?(matt.woodrow)
Comment on attachment 8804518 [details] [diff] [review]
patch

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1514,1 @@
>    MOZ_ASSERT(!InSafeMode());

Is this assert valid?
Comment on attachment 8804518 [details] [diff] [review]
patch

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1689,5 @@
> +      NS_LITERAL_CSTRING("FEATURE_FAILURE_NO_D3D11"));
> +    return false;
> +  }
> +
> +  // For Windows XP, we simply don't care enough to support this configuration.

At this point, there is no Windows XP E10S, so this doesn't make it worse.
Comment on attachment 8804518 [details] [diff] [review]
patch

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

::: gfx/thebes/DeviceManagerDx.cpp
@@ +703,5 @@
> +    return false;
> +  }
> +
> +  // Throw away the adapter since we're going to render in the GPU process.
> +  mAdapter = nullptr;

Side effects like this are a bit sad.

Could make the function bool EnsureAdapter(bool* aSupportsRemotePresent) and then have an explicit ClearAdapter that we can call if we're using the gpu process?

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1681,5 @@
> +    return false;
> +  }
> +
> +  // Don't use the GPU process if not using D3D11.
> +  if (!gfxConfig::IsEnabled(Feature::D3D11_COMPOSITING)) {

So we only ever get BasicCompositor in the GPU process if we allow d3d11, but fail to actually create the d3d11 compositor?

Nuking the GPU process if we detect a BasicCompositor seems less than ideal since there could be multiple compositors present, and some of them might still be d3d11.

Can we add a pref that overrides this check and then add a new reftest run that uses the same settings as reftest unaccelerated but with the new pref set too.
Attachment #8804518 - Flags: review?(matt.woodrow) → review+
(In reply to Milan Sreckovic [:milan] from comment #2)
> Comment on attachment 8804518 [details] [diff] [review]
> patch
> 
> Review of attachment 8804518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxWindowsPlatform.cpp
> @@ +1514,1 @@
> >    MOZ_ASSERT(!InSafeMode());
> 
> Is this assert valid?

Yeah, we bail out earlier in the callstack.
Attached patch bug1297822.patchSplinter Review
Good idea to add a new reftest column. This version adds a layers.gpu-process.dev.force-enabled pref.
Attachment #8804518 - Attachment is obsolete: true
Attachment #8804541 - Flags: review?(matt.woodrow)
Attachment #8804541 - Flags: review?(matt.woodrow) → review+
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75fe0970cbbc
Only use the GPU process when we expect a working Direct3D 11 compositor. (bug 1297822, r=mattwoodrow)
https://hg.mozilla.org/mozilla-central/rev/75fe0970cbbc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.