Implement proper GfxInfo-based driver blacklist on X11

RESOLVED FIXED in mozilla6

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla6
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 1 obsolete attachment)

1.35 KB, patch
karlt
: review+
Details | Diff | Splinter Review
6.03 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
5.12 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.45 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
4.35 KB, patch
karlt
: review+
Details | Diff | Splinter Review
3.12 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1.25 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1.50 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
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.
(Assignee)

Updated

6 years ago
Blocks: 624593
(Assignee)

Comment 1

6 years ago
Created attachment 522453 [details] [diff] [review]
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?
Attachment #522453 - Flags: review?(karlt)
(Assignee)

Comment 2

6 years ago
Created attachment 522457 [details] [diff] [review]
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.
Attachment #522457 - Flags: review?(joe)
(Assignee)

Comment 3

6 years ago
Here's a tryserver build, testing appreciated:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=9775b49ded2e
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bjacob@mozilla.com-9775b49ded2e/
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.
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?
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.
(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 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).
Attachment #522453 - Flags: review?(karlt) → review+

Comment 10

6 years ago
(In reply to comment #4)
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bjacob@mozilla.com-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.
Created attachment 522733 [details] [diff] [review]
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.
Attachment #522733 - Flags: review?(karlt)
Created attachment 522734 [details] [diff] [review]
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.
Attachment #522734 - Flags: review?(joe)
I launched a new tryserver build, should eventually be available at
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bjacob@mozilla.com-2956a4464393

TBPL:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=2956a4464393
(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.
(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?
(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.
(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 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.
Attachment #522733 - Flags: review?(karlt) → review+
(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.)
(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.
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.
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.
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.
Created attachment 523032 [details] [diff] [review]
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.
Attachment #523032 - Flags: review?(karlt)
Created attachment 523037 [details] [diff] [review]
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.
Attachment #523037 - Flags: review?(karlt)
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/bjacob@mozilla.com-48fa1d619ea3
(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 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.
Attachment #523032 - Flags: review?(karlt) → review+
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.
Attachment #523037 - Flags: review?(karlt) → review+
Blocks: 640071
Created attachment 526328 [details] [diff] [review]
Part 7: block R300/Gallium

As discussed in bug 624935.
Attachment #526328 - Flags: review?(karlt)
Created attachment 526371 [details] [diff] [review]
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.
Attachment #526371 - Flags: review?(karlt)
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);
}
Attachment #522457 - Flags: review?(joe) → review+
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?
Attachment #522734 - Flags: review?(joe) → review+
(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 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 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.
Attachment #526328 - Flags: review?(karlt) → review+

Comment 37

6 years ago
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)?
I'll make a tryserver build tomorrow and plan to land it next week.
(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?
(Assignee)

Updated

6 years ago
Attachment #526371 - Attachment is obsolete: true
Attachment #526371 - Flags: review?(karlt)
(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.
(Assignee)

Updated

6 years ago
Blocks: 624935
New tryserver build, should arrive in a few hours at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bjacob@mozilla.com-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

6 years ago
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.)
@ 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.
Created attachment 528249 [details] [diff] [review]
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
Attachment #528249 - Flags: review?(joe)
Oh, nice bugzilla 4.0 bug in comment 44! The TBPL and attachment links were interverted.
The other option would be to set the preferences used in testing to force use of webgl and/or opengl layers.
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.
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
This one is all green:
http://tbpl.mozilla.org/?tree=Try&rev=43b0f9a93717
Attachment #528249 - Flags: review?(joe) → review+
http://hg.mozilla.org/mozilla-central/rev/809d3205d3b8
http://hg.mozilla.org/mozilla-central/rev/221e1a343873
http://hg.mozilla.org/mozilla-central/rev/3f208499e8b4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Backed out due to crashes on Linux64 opt. But it was green on tryserver.
http://hg.mozilla.org/mozilla-central/rev/418b0b9985c6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
http://hg.mozilla.org/mozilla-central/rev/3d3e24239251
http://hg.mozilla.org/mozilla-central/rev/775378356e03
http://hg.mozilla.org/mozilla-central/rev/89b1fa4c4e3d
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 658915
Target Milestone: --- → mozilla6
Depends on: 659842

Updated

6 years ago
Assignee: nobody → bjacob
You need to log in before you can comment on or make changes to this bug.