Re-run blacklist after device creation, to ensure proper blocking done

NEW
Assigned to

Status

()

Core
Graphics
P3
normal
a year ago
11 months ago

People

(Reporter: ernest, Assigned: ernest)

Tracking

50 Branch
Points:
---

Firefox Tracking Flags

(firefox50 affected)

Details

(Whiteboard: gfx-noted)

Attachments

(1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8772990 [details]
Bug 1286653 - Re-run blacklist on DXGI adapter used for device creation

Review commit: https://reviewboard.mozilla.org/r/65640/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65640/
Attachment #8772990 - Flags: review?(bgirard)
https://reviewboard.mozilla.org/r/65640/#review62632

::: gfx/thebes/gfxWindowsPlatform.cpp:2448
(Diff revision 1)
>      if (!gfxConfig::UseFallback(Fallback::USE_D3D11_WARP_COMPOSITOR)) {
>        AttemptD3D11DeviceCreation(d3d11);
> +
> +      // check device to see if blacklisted after device creation successful
> +      if (mD3D11Device)
> +      {

style nit: { for if and else block go on the same line.

::: gfx/thebes/gfxWindowsPlatform.cpp:2451
(Diff revision 1)
> +      // check device to see if blacklisted after device creation successful
> +      if (mD3D11Device)
> +      {
> +        nsCString message;
> +        nsCString failureId;
> +        if (!gfxPlatform::IsGfxInfoStatusOkay(nsIGfxInfo::FEATURE_DIRECT3D_11_LAYERS, &message, failureId)) {

Are we updating the device/driver information based on the device before we re-run this? If so where does that code live?
Attachment #8772990 - Flags: review?(bgirard)
(Assignee)

Comment 3

a year ago
Comment on attachment 8772990 [details]
Bug 1286653 - Re-run blacklist on DXGI adapter used for device creation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65640/diff/1-2/
(Assignee)

Updated

a year ago
Attachment #8772990 - Flags: review?(dvander)

Comment 4

a year ago
mozreview-review
Comment on attachment 8772990 [details]
Bug 1286653 - Re-run blacklist on DXGI adapter used for device creation

https://reviewboard.mozilla.org/r/65640/#review67352

Still reading but here are some initial comments.

::: gfx/thebes/gfxWindowsPlatform.cpp:2125
(Diff revision 2)
> +   adapter->GetDesc(&adapterDesc);
> +   vendorID.AppendPrintf("0x%04x", adapterDesc.VendorId);
> +   deviceID.AppendPrintf("0x%04x", adapterDesc.DeviceId);
> +
> +   nsCOMPtr<nsIGfxInfo> gfxInfo = services::GetGfxInfo();
> +   gfxInfo->Reset(vendorID,deviceID);

Can we move this code, up to the Reset call, to where we create the IDXGIAdapter1? That would make this code move to the GPU process much more easily.

(nit: space between "vendorID,deviceID")

::: gfx/thebes/gfxWindowsPlatform.cpp:2132
(Diff revision 2)
> +  // check device to see if blacklisted after device creation successful
> +  nsCString message;
> +  nsCString failureId;
> +  if (!gfxPlatform::IsGfxInfoStatusOkay(nsIGfxInfo::FEATURE_DIRECT3D_11_LAYERS, &message, failureId)) {
> +    d3d11.Disable(FeatureStatus::Blacklisted, message.get(), failureId);
> +  }

This looks fine but you'll want to return on the failure path, otherwise we'll still happily assign the D3D11 device and continue to use D3D11.

::: widget/windows/GfxInfo.cpp:586
(Diff revision 2)
>  
> +nsresult
> +GfxInfo::Reset(const nsAString& aVendorID, const nsAString& aDeviceID)
> +{
> +  nsresult rv = NS_OK;
> +  if (aVendorID.IsEmpty() || aDeviceID.IsEmpty()){

nit: space before {

::: widget/windows/GfxInfo.cpp:590
(Diff revision 2)
> +  nsresult rv = NS_OK;
> +  if (aVendorID.IsEmpty() || aDeviceID.IsEmpty()){
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +
> +  // should have some check here for gfxinfo even initialized

nit: remove commented code here

::: widget/windows/GfxInfo.cpp:625
(Diff revision 2)
> +    }
> +
> +    deviceIndex++;
> +  }
> +
> +  if (tempAdapterVendorID.Equals(aVendorID) && tempAdapterDeviceID.Equals(aDeviceID)){

nit: space before {

::: widget/windows/GfxInfo.cpp:631
(Diff revision 2)
> +    mDeviceID = tempDeviceID;
> +    mAdapterVendorID = tempAdapterVendorID;
> +    mAdapterDeviceID = tempAdapterDeviceID;
> +    mAdapterSubsysID = tempAdapterSubsysID;
> +  }
> +  else { // vendorid and deviceid given cannot be found

nit: else should be on same line as }
(Assignee)

Comment 5

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c02b3b3ef72
(Assignee)

Comment 6

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05b50fa80968
(Assignee)

Comment 7

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17911ab007e2
(Assignee)

Comment 8

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65eb21c5dd41
(Assignee)

Comment 9

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=841da4119937
(Assignee)

Comment 10

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7c09669945c
(Assignee)

Comment 11

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33a7fc70c18d
(Assignee)

Comment 12

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab016a2ee3c1
(Assignee)

Comment 13

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73eef8162e7e
(Assignee)

Comment 14

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49e9d514c4e4
(Assignee)

Comment 15

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d73577e363b7
Comment hidden (mozreview-request)

Comment 17

a year ago
mozreview-review
Comment on attachment 8772990 [details]
Bug 1286653 - Re-run blacklist on DXGI adapter used for device creation

https://reviewboard.mozilla.org/r/65640/#review68160

::: gfx/thebes/DeviceManagerD3D11.cpp:105
(Diff revision 3)
> +      nsString deviceID;
> +
> +      adapter->GetDesc(&adapterDesc);
> +
> +      if (!adapter) { // no dxgiadapter
> +        return;

Don't we want to set Disabled here on below?

Comment 18

a year ago
mozreview-review
Comment on attachment 8772990 [details]
Bug 1286653 - Re-run blacklist on DXGI adapter used for device creation

https://reviewboard.mozilla.org/r/65640/#review68192

::: gfx/thebes/DeviceManagerD3D11.cpp:97
(Diff revision 3)
>    }
>  
>    if (XRE_IsParentProcess()) {
> +
> +    // reset the primary adapter information, given the adapter from device creation
> +    if (RefPtr<IDXGIAdapter1> adapter = GetDXGIAdapter()) {

Instead, change this to:

RefPtr<IDXGIAdapter1> adapter = GetDXGIAdapter();
if (!adapter) {
  return;
}  

Early returns are easier to follow and let you dedent all the code below.
  
Also, as Benoit noted, you will need a d3d11.SetFailed() call in here so we log the failure.

::: gfx/thebes/DeviceManagerD3D11.cpp:104
(Diff revision 3)
> +      nsString vendorID;
> +      nsString deviceID;
> +
> +      adapter->GetDesc(&adapterDesc);
> +
> +      if (!adapter) { // no dxgiadapter

This check doesn't make sense, given you've already used |adapter| above.

::: gfx/thebes/DeviceManagerD3D11.cpp:111
(Diff revision 3)
> +      }
> +
> +      vendorID.AppendPrintf("0x%04x", adapterDesc.VendorId);
> +      deviceID.AppendPrintf("0x%04x", adapterDesc.DeviceId);
> +
> +      nsCOMPtr<nsIGfxInfo> gfxInfo = services::GetGfxInfo();

Change this to: "if (nsCOMPtr<nsIGfxInfo> gfxInfo = services::GetGfxInfo()) {" ...

... and wrap the rest of this code in the if-block. Otherwise this will crash on the GPU process. Later on we'll have to figure out how to adapt this for the GPU process, but for now this is okay.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

a year ago
mozreview-review
Comment on attachment 8772990 [details]
Bug 1286653 - Re-run blacklist on DXGI adapter used for device creation

https://reviewboard.mozilla.org/r/65638/#review68442

::: gfx/thebes/DeviceManagerD3D11.cpp:95
(Diff revision 5)
>                      NS_LITERAL_CSTRING("FEATURE_FAILURE_D3D11_SIM"));
>      return;
>    }
>  
>    if (XRE_IsParentProcess()) {
> +

nit: there's an extra newline here

::: gfx/thebes/DeviceManagerD3D11.cpp:104
(Diff revision 5)
> +    nsString vendorID;
> +    nsString deviceID;
> +
> +    adapter->GetDesc(&adapterDesc);
> +
> +    if (!adapter) { // no dxgiadapter

This check should be moved up to immediately after GetDXGIAdapter, otherwise it has no effect.

::: gfx/thebes/DeviceManagerD3D11.cpp:114
(Diff revision 5)
> +
> +    vendorID.AppendPrintf("0x%04x", adapterDesc.VendorId);
> +    deviceID.AppendPrintf("0x%04x", adapterDesc.DeviceId);
> +
> +    if (nsCOMPtr<nsIGfxInfo> gfxInfo = services::GetGfxInfo()) {
> +      gfxInfo->Reset(vendorID,deviceID);

nit: space needed after comma
Comment hidden (mozreview-request)

Comment 23

a year ago
mozreview-review
Comment on attachment 8772990 [details]
Bug 1286653 - Re-run blacklist on DXGI adapter used for device creation

https://reviewboard.mozilla.org/r/65640/#review68448
Attachment #8772990 - Flags: review?(dvander) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8772990 [details]
Bug 1286653 - Re-run blacklist on DXGI adapter used for device creation

https://reviewboard.mozilla.org/r/65640/#review68480

Looks good. Great work!

::: gfx/thebes/DeviceManagerD3D11.cpp:96
(Diff revision 6)
>      return;
>    }
>  
>    if (XRE_IsParentProcess()) {
> +    // reset the primary adapter information, given the adapter from device creation
> +    RefPtr<IDXGIAdapter1> adapter = GetDXGIAdapter();

We discuss this chunk on a call. We're ok with creating a DXGI Adapter and having it around (cached in mAdapter) even if we end up blacklisting. We're also removing the WARP fallback so this is fine as-is.

::: widget/nsIGfxInfo.idl:159
(Diff revision 6)
>    DOMString getWebGLParameter(in DOMString aParam);
>  
>    // only useful on X11
>    [noscript, notxpcom] void GetData();
>  
> +  [noscript] void Reset (in AString aVendorID, in AString aDeviceID);

I'd like to see a block comment explaining why we're reseting the vendor/device ID. Something like:
/\*\*
 \* At startup we sometimes have to make a guess what device we're going to be using. We can reset this once we've gotten an exact device.
 \*/
Attachment #8772990 - Flags: review?(bgirard) → review+
Comment hidden (mozreview-request)

Comment 26

a year ago
Pushed by b56girard@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9ba0dd5271fe
Re-run blacklist on DXGI adapter used for device creation r=BenWa,dvander
Had to back this out for permafailing xpcshell test test_TelemetryEnvironment.js | test_checkEnvironment on Windows 8 x64 debug:

https://hg.mozilla.org/integration/autoland/rev/e129019c9d4e

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=1864951&repo=autoland

The mocking of the vendorID fails now. CCing Alessio from the Telemetry team.
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] from comment #27)
> The mocking of the vendorID fails now. CCing Alessio from the Telemetry team.

This is failing because [1] is failing to set the mock vendor ID or because the mocked ID gets reset to "0x10de" (which I bet is the vendor ID of the real GFX card on the test machine!) for some reason.

Maybe the proposed patch breaks nsIGfxInfoDebug?

[1] - https://dxr.mozilla.org/mozilla-central/rev/6e191a55c3d23e83e6a2e72e4e80c1dc21516493/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#240
Priority: -- → P3
Whiteboard: gfx-noted
Flags: needinfo?(milan)
Attachment #8772990 - Attachment is obsolete: true
Flags: needinfo?(milan)
You need to log in before you can comment on or make changes to this bug.