Andrew Comminos [:acomminos] Reporter
	Description • 8 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.

	Andrew Comminos [:acomminos] Reporter
	Comment 2 • 8 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.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 3 • 8 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/#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?

	Andrew Comminos [:acomminos] Reporter
	Comment 4 • 8 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!

	Jeff Muizelaar [:jrmuizel]
	Comment 6 • 8 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

	Jeff Muizelaar [:jrmuizel]
	Comment 9 • 8 years ago

Andrew, do you think you'll get a chance to finish this?

	Andrew Comminos [:acomminos] Reporter
	Comment 10 • 8 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!

	Jeff Muizelaar [:jrmuizel]
	Comment 11 • 8 years ago

Indeed. I wonder how I missed that. I guess I owe you a review.

	Jeff Muizelaar [:jrmuizel]
	Comment 12 • 8 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

	Marco Castelluccio [:marco]
	Comment 13 • 8 years ago

[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).

	Pulsebot
	Comment 14 • 8 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

	Pulsebot
	Comment 15 • 8 years ago

Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2d2897e4a74
Backed out changeset cf43cacdb262 for XPCShell failures

	Iris Hsiao [:ihsiao]
	Comment 16 • 8 years ago

Sorry had to back out for XPCShell failures, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=3774421&repo=autoland#L2179

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 17 • 8 years ago

Hi Milan, could you please help find an owner who can fix the failures and re-land this patch?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 18 • 8 years ago

(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.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 19 • 8 years ago

(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?

	Julien Cristau [:jcristau]
	Comment 20 • 7 years ago

Too late for firefox 52, mass-wontfix.

	Pulsebot
	Comment 24 • 6 years 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

	Pulsebot
	Comment 26 • 6 years 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

	Pulsebot
	Comment 31 • 5 years 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