645407 - Implement proper GfxInfo-based driver blacklist on X11

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Bug 639842 brinds a GfxInfo implementation. Use it to implement the driver blacklist as we do on other platforms. Whitelist newer Mesa and FGLRX, based on version (in Firefox 4, we were not able to safely get version information, so we could only whitelist NVIDIA). Get rid of MOZ_GLX_IGNORE_BLACKLIST.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 1: remove the old blacklisting and MOZ_GLX_IGNORE_BLACKLIST

Karl: there was a comment about a ScopedXErrorHandler being added because the first GLX call could generate a X error on some Mesa versions. Can you check that my patch is not going to break stuff in that respect?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 2: implement the new list (whitelist nvidia >= 257.21, Mesa >= 7.10, and recent FGLRX)

This patch makes GfxInfo::GetData() parse the GL strings to fill a few member variables: mIsMesa, mIsNVIDIA, mIsFGLRX, mMajorVersion, mMinorVersion.

For Mesa, all we get is a Mesa version, we don't get an actual driver version. So we have to pick a Mesa version, and from various discussions it's clear that the best criterion we can choose is: Mesa >= 7.10. For example, Ubuntu 10.10 ships with Mesa 7.9 and a version of the intel driver that crashes a lot on the webgl conformance tests, while Ubuntu 11.04 ships with Mesa 7.10 and a newer Intel driver and doesn't crash on WebGL conformance tests.

For NVIDIA, we whitelist >= 257.21. The idea is to do the same as we do on Windows. By the time Firefox 5 gets released, this version will be 1 year old.

For FGLRX, we don't get any version number besides the OpenGL version number. So we require OpenGL 3 support, which should effectively require a recent driver.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Here's a tryserver build, testing appreciated:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=9775b49ded2e

Comment 4

[Karl Tomlinson (:karlt)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-9775b49ded2e/

Comment 5

[Karl Tomlinson (:karlt)]

I wonder why I don't see bug 624935 with the tryserver build above and
OpenGL vendor string: X.Org R300 Project
OpenGL renderer string: Gallium 0.4 on ATI RV515
OpenGL version string: 2.1 Mesa 7.10.1
OpenGL shading language version string: 1.20

but I do get the crash with a debug build based on 7ccb164032a0.

Comment 6

[Karl Tomlinson (:karlt)]

The tryserver build seems to steer clear bug 589546 with
OpenGL vendor string: Mesa Project
OpenGL renderer string: Software Rasterizer
OpenGL version string: 2.1 Mesa 7.10.1
OpenGL shading language version string: 1.20

about:support says:
        Adapter Description
        --

        WebGL Renderer
        Blocked on your graphics card because of unresolved driver issues.

Does that mean that the test program failed?

Comment 7

[Karl Tomlinson (:karlt)]

Similarly bug 616416 is avoided with
OpenGL vendor string: VMware, Inc.
OpenGL renderer string: Gallium 0.4 on softpipe
OpenGL version string: 2.1 Mesa 7.10.1
OpenGL shading language version string: 1.20

with the same about:support line.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to comment #5)
> I wonder why I don't see bug 624935 with the tryserver build above and

I spoke too soon.  It is intermittent, but loading about:support sometimes triggers it.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 522453 [details] [diff] [review]
Part 1: remove the old blacklisting and MOZ_GLX_IGNORE_BLACKLIST

(In reply to comment #1)
> Karl: there was a comment about a ScopedXErrorHandler being added because the
> first GLX call could generate a X error on some Mesa versions. Can you check
> that my patch is not going to break stuff in that respect?

That ScopedXErrorHandler should be unnecessary now as glxtest will crash on the first call that queries the server.

Similarly, glxtest makes the workaround for bug 626192 unnecessary.

And r+ on this GLX_VENDOR test assuming part 2 implements feature blocking sufficiently.

I don't think Mesa 7.10 is ready to be whitelisted in general though.
r300 gallium still suffers bug 624935.
There's not much point whitelisting software Mesa because the only reason it doesn't crash the browser is that glxtest fails.
There's a risk that glxtest might pass and so the browser will crash.

I don't know about Intel and r600 Mesa.  Perhaps we could whitelist certain chipsets but I fear there might always be the risk that the driver for one chipset might fall back to software in some situations.

I don't know what evidence there is for fglrx.  Might be worth checking that Mats has an opengl version < 3 (bug 626192 comment 23).

Comment 10

[Robert Kaiser]

(In reply to comment #4)
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-9775b49ded2e/

FYI, this is what I get in about:support with my openSUSE Factory desktop that is currently confined to using vesafb due to a strange problem with my (Sandy Bridge) mainboard and the Intel drivers not liking each other:

Adapter Description: Mesa Project -- Software Rasterizer
Driver Version: 1.4 (2.1 Mesa 7.10)
WebGL Renderer: false
GPU Accelerated Windows: 0/1

I reliably seem to get a shutdown crash with this machine and this build, see bp-06c87db4-c774-440d-810d-9ea772110329 and bp-7294d1bc-3c87-4e5e-9d4f-f55e62110329 - accessing WebGL Earth or Web o' Wonder crashes when their front pages try to start loading, see bp-72c6bc9c-d0b6-4a0d-a731-277332110329 and bp-d3ee1c4a-a5dc-4d37-a47f-d87802110329

On my Laptop with and older Intel setup that works fine with Intel drivers and with openSUSE 11.4, I get this in about:support (and no shutdown crash):

Adapter Description: Tungsten Graphics, Inc -- Mesa DRI Intel(R) 945GM GEM 20100330 DEVELOPMENT
Driver Version: 1.4 Mesa 7.10
WebGL Renderer: Tungsten Graphics, Inc -- Mesa DRI Intel(R) 945GM GEM 20100330 DEVELOPMENT  -- 1.4 Mesa 7.10
GPU Accelerated Windows: 0/1

Just for a test spin, Flight of the Navigator runs, but with about a frame per 4 seconds rendered. Still, that's a start, I guess.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 3: use waitpid(), check exit/signal status, report in "Adapter Description" field

This applies Karl's comments in bug 639842 comment 29. We use waitpid() to wait only for the specific glxtest process, we check the exit status / signal and report that in the 'Adapter Description' field in about:support for easy debugging.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 4: allow to spoof GL strings with environment variables

This allows for much easier debugging.

Also removes 2 lines of code that are useless because strtol skips any initial whitespace.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

I launched a new tryserver build, should eventually be available at
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2956a4464393

TBPL:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=2956a4464393

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #6)
> The tryserver build seems to steer clear bug 589546 with
> OpenGL vendor string: Mesa Project
> OpenGL renderer string: Software Rasterizer
> OpenGL version string: 2.1 Mesa 7.10.1
> OpenGL shading language version string: 1.20
> 
> about:support says:
>         Adapter Description
>         --
> 
>         WebGL Renderer
>         Blocked on your graphics card because of unresolved driver issues.
> 
> Does that mean that the test program failed?

Yes. The new tryserver build (comment 13), thanks to the 'Part 3' patch, reports the status of the GLXtest process in about:support in the 'Adapter Description' field, so it will be much easier to understand.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #8)
> (In reply to comment #5)
> > I wonder why I don't see bug 624935 with the tryserver build above and
> 
> I spoke too soon.  It is intermittent, but loading about:support sometimes
> triggers it.

Even with my tryserver builds?
I mean, about:support doesn't do much besides creating a WebGL context which is just creating a GL context and doing a few GL get-calls. Context creation crashes should be caught by the glxtest process... hopefully?

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #9)
> I don't think Mesa 7.10 is ready to be whitelisted in general though.
> r300 gallium still suffers bug 624935.

That bug is about a raytracing demo. That's the kind of shader-intensive demo that also crashes certain Mac systems, so it's not a showstopper by itself. Unless you can reproduce this crash in simpler pages? IIUC you mention that you got this crash in about:support, is that correct? Is that with my tryserver builds?

Can you paste the GL strings for this r300 system?

> There's not much point whitelisting software Mesa because the only reason it
> doesn't crash the browser is that glxtest fails.
> There's a risk that glxtest might pass and so the browser will crash.

I'm missing something here. Are you saying that software Mesa is very crashy?

> I don't know about Intel and r600 Mesa.  Perhaps we could whitelist certain
> chipsets but I fear there might always be the risk that the driver for one
> chipset might fall back to software in some situations.

Note that for now, only WebGL is affected. For WebGL, software rendering is better than nothing (contrary to layers where it'd be worse than nothing).

> 
> I don't know what evidence there is for fglrx.  Might be worth checking that
> Mats has an opengl version < 3 (bug 626192 comment 23).

I replied to that bug. If needed we can require 3.3 since that's what the current version of FGLRX gives.

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to comment #10)
> Adapter Description: Mesa Project -- Software Rasterizer
> Driver Version: 1.4 (2.1 Mesa 7.10)
> WebGL Renderer: false
> GPU Accelerated Windows: 0/1
> 
> I reliably seem to get a shutdown crash with this machine and this build, see
> bp-06c87db4-c774-440d-810d-9ea772110329 and
> bp-7294d1bc-3c87-4e5e-9d4f-f55e62110329 - accessing WebGL Earth or Web o'
> Wonder crashes when their front pages try to start loading, see
> bp-72c6bc9c-d0b6-4a0d-a731-277332110329 and
> bp-d3ee1c4a-a5dc-4d37-a47f-d87802110329

Interestingly I was sometimes (intermittently) seeing X_GLXMakeCurrent: GLXBadContextTag errors (similar but different) with indirect rendering (LIBGL_ALWAYS_INDIRECT=1 and probably also with a remote display).

OpenGL vendor string: X.Org R300 Project
OpenGL renderer string: Gallium 0.4 on ATI RV515
OpenGL version string: 1.4 (2.1 Mesa 7.10.1)
OpenGL extensions:

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 522733 [details] [diff] [review]
Part 3: use waitpid(), check exit/signal status, report in "Adapter Description" field

This looks better, thanks, so I'll mark r+, but there are still two things that I think should be addressed.

1. waitpid() may return -1 on failure.
AFAIK the only case where we expect it to return -1 is when errno is set to EINTR, because waitpid() was interrupted by a signal.
In this case, the right thing to do is keep repeating waitpid().
If errno is something else, then AFAIK something unexpected has happened and the function should probably try to recover as gracefully as possible flagging that the verification failed.

2. Even when the strings have been written successfully, if glxtest_status is not 0, then glxtest has failed in some way.  I think this also should result in the features being disabled.

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to comment #15)
> (In reply to comment #8)
> > (In reply to comment #5)
> > > I wonder why I don't see bug 624935 with the tryserver build above and
> > 
> > I spoke too soon.  It is intermittent, but loading about:support sometimes
> > triggers it.
> 
> Even with my tryserver builds?

Yes.

> I mean, about:support doesn't do much besides creating a WebGL context which is
> just creating a GL context and doing a few GL get-calls. Context creation
> crashes should be caught by the glxtest process... hopefully?

Yes, hopefully, but this doesn't necessarily show up the first time.
(I suspect this might be a memory corruption or uninitialized read issue.)

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to comment #16)
> (In reply to comment #9)
> > I don't think Mesa 7.10 is ready to be whitelisted in general though.
> > r300 gallium still suffers bug 624935.
> 
> That bug is about a raytracing demo. That's the kind of shader-intensive demo
> that also crashes certain Mac systems, so it's not a showstopper by itself.
> Unless you can reproduce this crash in simpler pages? IIUC you mention that you
> got this crash in about:support, is that correct? Is that with my tryserver
> builds?

That was with the tryserver build, after loading the raytracing demo and then about:support.  (Sorry, I wasn't clear above.)
I saw this stack, which looks similar, after loading WebGL Earth (no raytracing demo) and about:support.

#0  XQueryExtension (dpy=0x3d6e63636d74757c, name=0x7fffef0e5b86 "GLX", 
    major_opcode=0x7fffffffab44, first_event=0x7fffffffab48, 
    first_error=0x7fffffffab4c)
    at /var/tmp/portage/x11-libs/libX11-1.4.1/work/libX11-1.4.1/src/QuExt.c:43
#1  0x00007ffff28aeee8 in XInitExtension (dpy=0x3d6e63636d74757c, 
    name=0x7fffef0e5b86 "GLX")
    at /var/tmp/portage/x11-libs/libX11-1.4.1/work/libX11-1.4.1/src/InitExt.c:47
#2  0x00007fffef0b94f4 in __glXInitialize (dpy=0x3d6e63636d74757c)
    at glxext.c:806
#3  0x00007fffef0df790 in dri2FlushFrontBuffer (
    driDrawable=<value optimized out>, loaderPrivate=0x7fffef0e5b86)
    at dri2_glx.c:460
#4  0x00007fff9219c890 in dri_st_framebuffer_flush_front (
    stfbi=<value optimized out>, statt=-284271738) at dri_drawable.c:104
#5  0x00007fff9219bc24 in dri_unbind_context (cPriv=<value optimized out>)
    at dri_context.c:152
#6  0x00007fff921987f6 in driUnbindContext (pcp=0x7fff9674fac0)
    at ../common/dri_util.c:117
#7  0x00007fffef0decdb in dri2_unbind_context (context=0x7fffb7ba1e60, 
    new=0x7fffef0e5b86) at dri2_glx.c:172
#8  0x00007fffef0b8e89 in MakeContextCurrent (dpy=0x7fffeca44000, 
    draw=62920442, read=62920442, gc_user=<value optimized out>)
    at glxcurrent.c:250

> Can you paste the GL strings for this r300 system?

Those are in comment 5.

> > There's not much point whitelisting software Mesa because the only reason it
> > doesn't crash the browser is that glxtest fails.
> > There's a risk that glxtest might pass and so the browser will crash.
> 
> I'm missing something here. Are you saying that software Mesa is very crashy?

Yes.  I see either bug 589546 or bug 616416 in builds without glxtest.

> > I don't know about Intel and r600 Mesa.  Perhaps we could whitelist certain
> > chipsets but I fear there might always be the risk that the driver for one
> > chipset might fall back to software in some situations.
> 
> Note that for now, only WebGL is affected. For WebGL, software rendering is
> better than nothing (contrary to layers where it'd be worse than nothing).

I completely agree that software fallback is a good thing.
My concern is that the software implementation is crashy, and while that is crashy, there may also be risk that chipset-specific implementations fall into the same issues.

I could be wrong here, so maybe we should try some of the better chipset-specific implementations and see.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

Regarding the software renderer: I could reproduce a segmentation fault locally when using the Mesa software renderer, and then I realized that the GLX extension was missing on my system, so a patch is coming to check for the GLX extension before we proceed. Trying to go on without checking for the GLX extension seems to explain a lot of crashes.

Regarding the r300 callstack in comment 20: it is crashing in XQueryExtension with "GLX" string so I am hoping that the same fix will fix this too.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Attached patch checking for GLX extension in bug 639842.
https://bugzilla.mozilla.org/attachment.cgi?id=523020

But I take back what I said about how it could fix your and Robert's crashes: the fact that you get GL strings shows that you have the GLX extension.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Hm except that since that patch is doing exactly what you're crashing in in comment 20, there's a chance that it could catch it.

Comment 24

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 5: check waitpid() retval, rework error handling a bit

Here is the requested waitpid() change. This is now reported along with other errors in the Adapter Description field, which prompted some reorganization.

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 6: remove now-useless checks and remnant of MOZ_GLX_IGNORE_BLACKLIST

(In reply to comment #9)
> Comment on attachment 522453 [details] [diff] [review]
> Part 1: remove the old blacklisting and MOZ_GLX_IGNORE_BLACKLIST
>
> That ScopedXErrorHandler should be unnecessary now as glxtest will crash on the
> first call that queries the server.
> 
> Similarly, glxtest makes the workaround for bug 626192 unnecessary.

This patch removes that stuff.

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

Pushed again to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=48fa1d619ea3

Builds will shortly be available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-48fa1d619ea3

Comment 27

[Karl Tomlinson (:karlt)]

(In reply to comment #21)
> Regarding the r300 callstack in comment 20: it is crashing in XQueryExtension
> with "GLX" string so I am hoping that the same fix will fix this too.

Mesa should not be checking XQueryExtension (again) during dri2FlushFrontBuffer/driUnbindContext/MakeContextCurrent
I suspect the dpy parameter contains garbage which is why __glXInitialize didn't find a record for the display and so tried to initialize it.

Comment 28

[Karl Tomlinson (:karlt)]

Comment on attachment 523032 [details] [diff] [review]
Part 5: check waitpid() retval, rework error handling a bit

>+    while(wait_for_glxtest_process) {

Add a space after while.

>+    bool error = waiting_for_glxtest_process_failed || exited_with_error_code || received_signal;

exited_with_error_code || received_signal == status != 0 always.
That fact could probably have been used to make some of this simpler, but this is correct as is. 

>+        if (waiting_for_glxtest_process_failed)
>+            mAdapterDescription.AppendLiteral(" (waitpid failed)");
>+        if (exited_with_error_code)
>             mAdapterDescription.AppendPrintf(" (exited with status %d)", WEXITSTATUS(glxtest_status));
>+        if (received_signal)

Explicit "else if"s for exited_with_error_code and received_signal would make the code's intentions clearer.

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 523037 [details] [diff] [review]
Part 6: remove now-useless checks and remnant of MOZ_GLX_IGNORE_BLACKLIST

Thanks for cleaning this up.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 7: block R300/Gallium

As discussed in bug 624935.

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 8: keep OpenGL layers blocked for now

OpenGL layers are not ready to be enabled by default on X11, see bug 650362. Also, whenever we enable them by default, we'll have to explicitly blacklist them on software renderers (searching for "Software" substring in renderer string may be enough).

Regarding usage of software Mesa for WebGL, I would like to have it enabled by default for a little period of time so that if the issue is serious, we can gather some crash data about it.

Comment 32

[Joe Drew (not getting mail)]

Comment on attachment 522457 [details] [diff] [review]
Part 2: implement the new list (whitelist nvidia >= 257.21, Mesa >= 7.10, and recent FGLRX)

>diff --git a/widget/src/xpwidgets/GfxInfoX11.cpp b/widget/src/xpwidgets/GfxInfoX11.cpp

>+    const char *whereToReadVersionNumbers = 0;

nsnull instead of 0.

>+    const char *Mesa_in_version_string = strstr(mVersion.get(), "Mesa");
>+    if (Mesa_in_version_string) {

>+    } else if (strstr(mVendor.get(), "NVIDIA Corporation")) {

>+        const char *NVIDIA_in_version_string = strstr(mVersion.get(), "NVIDIA");
>+        if (NVIDIA_in_version_string)

I'm not overjoyed at having one function-scoped variable and one if-scoped variable here. It'd be better if these were both the same, for greater parallelism; you could also include a comment saying that ATI's drivers don't have the same structure.

>+    } else if (strstr(mVendor.get(), "ATI Technologies Inc")) {

Has AMD not switched to using AMD in their drivers on Linux yet?

>+    // read major.minor version numbers
>+    if (whereToReadVersionNumbers) {
>+        // copy into writable buffer, for tokenization
>+        strncpy(buf, whereToReadVersionNumbers, buf_size);

I would prefer that this be using mozilla strings rather than C strings, but I won't r- for this. If you wanted to change it I would be happy!

>+        // now try to read major.minor version numbers. In case of failure, gracefully exit: these numbers have
>+        // been initialized as 0 anyways
>+        int version_numbers_read = 0;
>+        while(version_numbers_read < 2) {
>+            char *token = NS_strtok(".", &bufptr);
>+            if (!token)
>+                break;
>+            long version_number = strtol(token, 0, 10);
>+            if (version_numbers_read == 0)
>+                mMajorVersion = version_number;
>+            else if (version_numbers_read == 1)
>+                mMinorVersion = version_number;
>+            version_numbers_read++;
>+        }

This seems like a roundabout way of writing this parsing. I think I'd rather it be written more along the lines of

char *token = NS_strtok(".", &bufptr);
if (token) {
  mMajorVersion = strtol(token, 0, 10);
}

token = NS_strtok(".", &bufptr);
if (token) {
  mMinorVersion = strtol(token, 0, 10);
}

Comment 33

[Joe Drew (not getting mail)]

Comment on attachment 522734 [details] [diff] [review]
Part 4: allow to spoof GL strings with environment variables


>@@ -158,20 +169,16 @@ GfxInfo::GetData()

>     if (whereToReadVersionNumbers) {
>         // copy into writable buffer, for tokenization
>         strncpy(buf, whereToReadVersionNumbers, buf_size);
>         bufptr = buf;
> 
>-        // skip any initial spaces (e.g. the space in "Mesa 7.10")
>-        while (bufptr[0] == ' ')
>-            ++bufptr;

I don't think this section is supposed to be removed, is it?

Comment 34

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #33)
> I don't think this section is supposed to be removed, is it?

It is: strtol() already does that for us, so it was useless.

Comment 35

[Karl Tomlinson (:karlt)]

Comment on attachment 526371 [details] [diff] [review]
Part 8: keep OpenGL layers blocked for now

>+    // we're not quite ready to enable OpenGL layers on X11, see bug 650362
>+    if (aFeature == nsIGfxInfo::FEATURE_OPENGL_LAYERS) {
>+        *aStatus = nsIGfxInfo::FEATURE_DISCOURAGED;
>+        return NS_OK;
>+    }

nsBaseWidget::GetShouldAccelerate still has accelerateByDefault = PR_FALSE on X11, so is this only for full screen video?
Does bug 650362 affect that?

Comment 36

[Karl Tomlinson (:karlt)]

Comment on attachment 526328 [details] [diff] [review]
Part 7: block R300/Gallium

Yes r300 is crashing but I also see the same crash with r600 gallium (bug 624935 comment 5), which makes me suspect something more fundamental here.

Comment 37

[Robert Kaiser]

Hrm, waited too long to fetch newer try builds to test how they work on my Intel-based machines. Can you run another round once you update the patches (unless this is going to land very soon anyhow and I can Get a Nightly then for testing)?

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

I'll make a tryserver build tomorrow and plan to land it next week.

Comment 39

[Joe Drew (not getting mail)]

(In reply to comment #34)
> (In reply to comment #33)
> > I don't think this section is supposed to be removed, is it?
> 
> It is: strtol() already does that for us, so it was useless.

Can you remove it from the previous patch then?

Comment 40

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #35)
> Comment on attachment 526371 [details] [diff] [review]
> Part 8: keep OpenGL layers blocked for now
> 
> >+    // we're not quite ready to enable OpenGL layers on X11, see bug 650362
> >+    if (aFeature == nsIGfxInfo::FEATURE_OPENGL_LAYERS) {
> >+        *aStatus = nsIGfxInfo::FEATURE_DISCOURAGED;
> >+        return NS_OK;
> >+    }
> 
> nsBaseWidget::GetShouldAccelerate still has accelerateByDefault = PR_FALSE on
> X11, so is this only for full screen video?
> Does bug 650362 affect that?

Thanks for catching this! Let's forget about this patch, then.

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

New tryserver build, should arrive in a few hours at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-19e3a43fe54b

TBPL:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=19e3a43fe54b

Oranges are expected since this will likely block the NVIDIA version on the linux test slaves. But this dumps the driver info into the log, so we'll be able to special-case it for landing.

Joe: I applied most of your review comments. The only one I didn't apply was the one about using nsStringClasses because I hate them and this code is Unix-specific anyways.

Comment 42

[Robert Kaiser]

With the new try build, here's what my Sandy Bridge machine confined to run vesafb says now (without crashing):

Adapter Description: GLXtest process failed (received signal 11)
WebGL Renderer: Blocked on your graphics card because of unresolved driver issues.
GPU Accelerated Windows: 0/1

The laptop with the older Intel chip works the same as with the previous builds (Flight of the Navigator shows roughly 1 frame in 2 seconds) and has the following info:

Adapter Description: Tungsten Graphics, Inc -- Mesa DRI Intel(R) 945GM GEM 20100330 DEVELOPMENT x86/MMX/SSE2
Driver Version: 1.4 Mesa 7.10
WebGL Renderer: Tungsten Graphics, Inc -- Mesa DRI Intel(R) 945GM GEM 20100330 DEVELOPMENT x86/MMX/SSE2 -- 1.4 Mesa 7.10
GPU Accelerated Windows: 0/2

Looks to me like this patch series is effective. :)

(And I hope I'll be able to run Sandy Bridge with actual Intel drivers soon and see if it works as well, then.)

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

@ Robert, thanks for the update, it's great to know that it's actually working on other machines than mine.

---

So, the tryserver build dumping GL strings on test slaves gives me this:
NVIDIA Corporation -- GeForce 9400/PCI/SSE2 -- 3.2.0 NVIDIA 190.42

-> updated patch coming whitelisting the test slaves' setup.

Comment 44

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 8: whitelist our own linux test slaves' config

This is explained in a comment in the patch:

    // whitelist the linux test slaves' current configuration.
    // this is necessary as they're still using the slightly outdated 190.42 driver.
    // this isn't a huge risk, as at least this is the exact setting in which we do continuous testing,
    // and this only affects GeForce 9400 cards on linux on this precise driver version, which is very few users.
    // We do the same thing on Windows XP, see in widget/src/windows/GfxInfo.cpp

So this tryserver build is expected to be all green:

http://tbpl.mozilla.org/?tree=Try&rev=1a81c84895b7

Comment 45

[Benoit Jacob [:bjacob] (mostly away)]

Oh, nice bugzilla 4.0 bug in comment 44! The TBPL and attachment links were interverted.

Comment 46

[Karl Tomlinson (:karlt)]

The other option would be to set the preferences used in testing to force use of webgl and/or opengl layers.

Comment 47

[Benoit Jacob [:bjacob] (mostly away)]

That has already been considered in bug 628384, a patch was even written, but was ultimately rejected. See bug 628384 comment 14. Whitelisting the test slaves config seemed like the path of least resistance.

Comment 48

[Benoit Jacob [:bjacob] (mostly away)]

Linux/Qt build was busted because I was unconditionally using the crashreporter API. This one should be green:
http://tbpl.mozilla.org/?tree=Try&rev=06cfd100bb49

Comment 49

[Benoit Jacob [:bjacob] (mostly away)]

This one is all green:
http://tbpl.mozilla.org/?tree=Try&rev=43b0f9a93717

Comment 50

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/809d3205d3b8
http://hg.mozilla.org/mozilla-central/rev/221e1a343873
http://hg.mozilla.org/mozilla-central/rev/3f208499e8b4

Comment 51

[Benoit Jacob [:bjacob] (mostly away)]

Backed out due to crashes on Linux64 opt. But it was green on tryserver.
http://hg.mozilla.org/mozilla-central/rev/418b0b9985c6

Comment 52

[Benoit Jacob [:bjacob] (mostly away)]

New tryserver push, since PGO was disabled on linux today. At least this should give a good stack. I've not been able to reproduce the crash locally, not even with gcc 4.5.
http://tbpl.mozilla.org/?tree=Try&rev=151865e229d0

Comment 53

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/3d3e24239251
http://hg.mozilla.org/mozilla-central/rev/775378356e03
http://hg.mozilla.org/mozilla-central/rev/89b1fa4c4e3d