Open Bug 1286653 Opened 8 years ago Updated 2 years ago

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

Categories

(Core :: Graphics, defect, P3)

Product:
Core
Component:
Graphics
Version:
50 Branch
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox50 --- affected

People

(Reporter: ernest, Unassigned)

References

Details

(Whiteboard: gfx-noted)

Attachments

(1 obsolete file)

No description provided.
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)
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/
Attachment #8772990 - Flags: review?(dvander)
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 }
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 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 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 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 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+
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)

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: ernest-yim → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: