Improve GLX blocklisting on Linux/X11

RESOLVED FIXED in Firefox 68

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: acomminos, Assigned: aosmond)

Tracking

(Blocks 2 bugs)

50 Branch
mozilla68
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 wontfix, firefox68 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 years ago
Currently, our blocklisting situation on Linux is very poor- we fail to provide any meaningful driver version information, vendor/device IDs, etc. that are necessary for using the downloadable blocklist.

We should aim to support blacklisting for Mesa (with separate rules for each of its DRI implementations), the proprietary NVIDIA driver, and fglrx. A baseline driver version for each libGL implementation should be established for sanity.

Mesa multiplexing should be accomplished using GLX_MESA_query_renderer and glXGetScreenDriver.
Comment hidden (mozreview-request)
Reporter

Comment 2

3 years ago
A brief overview of the high-level design goals in the patch;

- On Linux, we define a "driver" (vendor) as the userspace stack upon which libGL depends. This corresponds directly to Mesa + a DRI driver (i.e. mesa/i965), or just the NVIDIA proprietary driver (using existing PCI vendor IDs).
- Vendor IDs for mesa cards are dynamically defined as mesa/{DRI_DRIVER}, where DRI_DRIVER is the DRI implementation reported by the X server. Workarounds for software implementations are in place.
- The vendor target VendorMesaAll (mesa/all) is to be used for blacklisting all drivers for a particular mesa version. For mesa drivers that do not use DRI, nor report as being software accelerated via MESA_query_renderer, mesa/unknown is used.

Baseline drivers have been chosen for each of the major libGL implementors such that we can discard much of the previous blocklisting code.
Comment on attachment 8779867 [details]
Bug 1294232 - Refactor blocklisting on Linux to support the downloadable blocklist.

https://reviewboard.mozilla.org/r/70770/#review68150

::: widget/GfxDriverInfo.h:272
(Diff revision 1)
>  inline bool
>  ParseDriverVersion(const nsAString& aVersion, uint64_t *aNumericVersion)
>  {
>    *aNumericVersion = 0;
>  
> -#if defined(XP_WIN)
> +#if defined(XP_WIN) || defined(XP_LINUX)

The other defines are MOZ_X11 - should this one be XP_LINUX or MOZ_X11?
Reporter

Comment 4

3 years ago
(In reply to Milan Sreckovic [:milan] from comment #3)
> Comment on attachment 8779867 [details]
> Bug 1294232 - Refactor blocklisting on Linux to support the downloadable
> blocklist.
> 
> https://reviewboard.mozilla.org/r/70770/#review68150
> 
> ::: widget/GfxDriverInfo.h:272
> (Diff revision 1)
> >  inline bool
> >  ParseDriverVersion(const nsAString& aVersion, uint64_t *aNumericVersion)
> >  {
> >    *aNumericVersion = 0;
> >  
> > -#if defined(XP_WIN)
> > +#if defined(XP_WIN) || defined(XP_LINUX)
> 
> The other defines are MOZ_X11 - should this one be XP_LINUX or MOZ_X11?

Whoops, yes- thanks!
Comment hidden (mozreview-request)

Comment 6

3 years ago
mozreview-review
Comment on attachment 8779867 [details]
Bug 1294232 - Refactor blocklisting on Linux to support the downloadable blocklist.

https://reviewboard.mozilla.org/r/70770/#review72130

::: widget/GfxInfoX11.cpp:203
(Diff revision 2)
>      }
>  
> -    mAdapterDescription.Append(mVendor);
> -    mAdapterDescription.AppendLiteral(" -- ");
> -    mAdapterDescription.Append(mRenderer);
> +    // Scan the GL_VERSION string for the GL and driver versions.
> +    char* versionBuf = (char*) glVersion.get();
> +    char* token;
> +    while ((token = NS_strtok(" ", &versionBuf))) {

Please use Tokenize.h instead of strtok
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Andrew, do you think you'll get a chance to finish this?
Flags: needinfo?(andrew)
Reporter

Comment 10

3 years ago
I updated this patch a while ago to use nsWhitespaceTokenizer, I believe that was the main issue you had in the original patch. Was there anything else outstanding?

Thanks!
Flags: needinfo?(andrew) → needinfo?(jmuizelaar)
Indeed. I wonder how I missed that. I guess I owe you a review.
Flags: needinfo?(jmuizelaar)

Comment 12

3 years ago
mozreview-review
Comment on attachment 8779867 [details]
Bug 1294232 - Refactor blocklisting on Linux to support the downloadable blocklist.

https://reviewboard.mozilla.org/r/70770/#review77442
Attachment #8779867 - Flags: review?(jmuizelaar) → review+
[Tracking Requested - why for this release]: Without blocklisting, we risk breaking users with old driver versions (probably users on LTS versions of distributions would be most affected).
Reporter

Updated

3 years ago
Keywords: checkin-needed

Comment 14

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cf43cacdb262
Refactor blocklisting on Linux to support the downloadable blocklist. r=jrmuizel
Keywords: checkin-needed

Comment 15

3 years ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2d2897e4a74
Backed out changeset cf43cacdb262 for XPCShell failures
Sorry had to back out for XPCShell failures, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=3774421&repo=autoland#L2179
Flags: needinfo?(andrew)
Hi Milan, could you please help find an owner who can fix the failures and re-land this patch?
Flags: needinfo?(milan)
(In reply to Marco Castelluccio [:marco] from comment #13)
> [Tracking Requested - why for this release]: Without blocklisting, we risk
> breaking users with old driver versions (probably users on LTS versions of
> distributions would be most affected).

(In reply to Ritu Kothari (:ritu) from comment #17)
> Hi Milan, could you please help find an owner who can fix the failures and
> re-land this patch?

This applies to nightly only.  Linux acceleration is locked out of aurora+beta+release, so there is no need to track or rush.  We still want it, before we can enable this functionality riding the trains, but there is no rush.
(In reply to Iris Hsiao [:ihsiao] from comment #16)
> Sorry had to back out for XPCShell failures, e.g.,
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=3774421&repo=autoland#L2179

George, can you see if you can resolve these?
Assignee: andrew → gwright
Flags: needinfo?(andrew)
Too late for firefox 52, mass-wontfix.
Assignee: gw → nobody
Status: ASSIGNED → NEW
Assignee: nobody → aosmond
Assignee

Comment 21

3 months ago

Not sure where the failures were before, so let's try some things and make sure we haven't regressed in the last 3 years in general...

try (xpcshell on Windows/OSX): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d76091646e5ed6e1a51f2586ee6611340d71c91f
try (all linux64): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d76091646e5ed6e1a51f2586ee6611340d71c91f

Assignee

Updated

3 months ago
Priority: -- → P3
Assignee

Comment 23

3 months ago

Looks like we were not calling fire_glxtest_process() during the xpcshell startup, so GlxInfoX11::GetData would always fail.

Comment 24

3 months ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc80bc53119
Refactor blocklisting on Linux to support the downloadable blocklist. r=jrmuizel

Comment 26

3 months ago
Backout by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0669f92557f3
Backed out changeset 2dc80bc53119 for xpcshell failures in test_gfxBlacklist_No_Comparison.js
Assignee

Updated

2 months ago
Attachment #8779867 - Attachment is obsolete: true
Assignee

Comment 29

2 months ago

(In reply to Andrew Osmond [:aosmond] from comment #28)

Let's do this again.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=892c61cbed4aca0302280b177cfba0f0f530eab1

Can reproduce the test failure by using LIBGL_ALWAYS_SOFTWARE=1 when executing. It causes us to fail early, without checking the blocklist:

https://hg.mozilla.org/try/file/7e1a14c2f8dc/widget/GfxInfoX11.cpp#l362

Assignee

Comment 30

2 months ago

Fixed by making GlxInfoX11::SpoofVendorId check for mesa/llvmpipe, mesa/swrast and mesa/softpipe as the spoofed ID; the mIsAccelerated flag is reset to false for the previously listed, else true. That should be sufficient for any simulation purposes.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b684bf392c2c51afc6964c9715e5a7726f2f571

Comment 31

2 months ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8962b8d9b7a6
Refactor blocklisting on Linux to support the downloadable blocklist. r=jrmuizel

Comment 32

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.