Bug 131472 – [GTK] Add llvmpipe (Mesa) to the JHBuild moduleset and force it when running layout tests

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

This should prevent crashes on NVidia drivers and ensure that all tests that use OpenGL render the same on all systems.

[Martin Robinson]

Created attachment 229096 [details]
Patch

[Philippe Normand]

Comment on attachment 229096 [details]
Patch

LGTM! But bots need to have  libxcb-xfixes0-dev installed before landing.

[Martin Robinson]

Created attachment 229548 [details]
Patch

[Martin Robinson]

Created attachment 229674 [details]
Patch

[Martin Robinson]

Committed r167510: <http://trac.webkit.org/changeset/167510>

[Philippe Normand]

Hum I suspect the Debug bot didn't like this change, can you please check what's going on Martin?
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r167519%20(41714)/results.html

[Martin Robinson]

Sure. I can take a look, though it will be a day or two before I'll have the opportunity to do it. Feel free to roll out the patch in the meantime. Sorry for the breakage, I didn't keep a close enough eye on the debug bot. I appreciate you pinging the bug.

[Philippe Normand]

Comment on attachment 229674 [details]
Patch

That one landed but I'm going to roll it out for now.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 131915

[Philippe Normand]

The patch also broke Rego's and my EWS because of missing llvm :)

[Carlos Alberto Lopez Perez]

(In reply to comment #10)
> The patch also broke Rego's and my EWS because of missing llvm :)

Carlos added LLVM to gtk/jhbuild-optional.modules on r169418 <http://trac.webkit.org/r169418>

Should we move LLVM to gtk/jhbuild.modules and retry this patch?

[Gwang Yoon Hwang]

Created attachment 233043 [details]
Patch

[Gwang Yoon Hwang]

After discussing with mrobinson, we decide to land this patch to secure pixel test before applying the threaded compositor.

So I modified this patch to move LLVM from jhbuild-optional.modules to jhbuild.modules
to use LLVM for Mesa as clopez suggested.

This modification includes build LLVM with a shared library support for the Mesa,
and bumping up the version of Mesa, because 10.0.2 was deprecated.

[Carlos Alberto Lopez Perez]

Now the tests (both layout tests and performance tests) can be run on the native X display instead of the default Xvfb by exporting the environment variable USE_NATIVE_XDISPLAY

On a system running other implementation of OpenGL than Mesa (for example, with the Nvidia or Ati/Amd propietary drivers) forcing Mesa when USE_NATIVE_XDISPLAY is set will break.

And I believe the same will happen when running the tests on Wayland with proprietary drivers (maybe not a problem now because there are not propietary drivers for wayland AFAIK, but probably in the future, I heard that propietary Nvidia drivers for wayland are coming).

To avoid that, I suggest to only force Mesa when the tests are ran with Xvfb.
This can be done by checking that none of the environment variables that trigger the other display drivers are set. For example:


-        if llvmpipe_libgl_path:
+        if not os.environ.get("WAYLAND_DISPLAY") and not os.environ.get("USE_NATIVE_XDISPLAY") and llvmpipe_libgl_path:


Another option (maybe cleaner) is to force mesa directly on the Xvfb driver initialization steps. That way if the other display drivers are selected (Wayland/Weston or Xorg) mesa won't be forced. For example:

--- a/Tools/Scripts/webkitpy/port/xvfbdriver.py
+++ b/Tools/Scripts/webkitpy/port/xvfbdriver.py
@@ -50,6 +50,9 @@ class XvfbDriver(Driver):
         Driver.__init__(self, *args, **kwargs)
         self._guard_lock = None
         self._startup_delay_secs = 1.0
+        llvmpipe_libgl_path = os.environ.get('LLVMPIPE_LIBGL_PATH')
+        if llvmpipe_libgl_path:
+            environment['LD_LIBRARY_PATH'] = '%s:%s' % (llvmpipe_libgl_path, os.environ.get('LD_LIBRARY_PATH', ''))

[Gwang Yoon Hwang]

(In reply to comment #14)
> Now the tests (both layout tests and performance tests) can be run on the native X display instead of the default Xvfb by exporting the environment variable USE_NATIVE_XDISPLAY
> 
> On a system running other implementation of OpenGL than Mesa (for example, with the Nvidia or Ati/Amd propietary drivers) forcing Mesa when USE_NATIVE_XDISPLAY is set will break.
> 
> And I believe the same will happen when running the tests on Wayland with proprietary drivers (maybe not a problem now because there are not propietary drivers for wayland AFAIK, but probably in the future, I heard that propietary Nvidia drivers for wayland are coming).

Good point. Especially for the performance test, we shouldn't break these drivers.

> Another option (maybe cleaner) is to force mesa directly on the Xvfb driver initialization steps. That way if the other display drivers are selected (Wayland/Weston or Xorg) mesa won't be forced. For example:
> 
> --- a/Tools/Scripts/webkitpy/port/xvfbdriver.py
> +++ b/Tools/Scripts/webkitpy/port/xvfbdriver.py
> @@ -50,6 +50,9 @@ class XvfbDriver(Driver):
>          Driver.__init__(self, *args, **kwargs)
>          self._guard_lock = None
>          self._startup_delay_secs = 1.0
> +        llvmpipe_libgl_path = os.environ.get('LLVMPIPE_LIBGL_PATH')
> +        if llvmpipe_libgl_path:
> +            environment['LD_LIBRARY_PATH'] = '%s:%s' % (llvmpipe_libgl_path, os.environ.get('LD_LIBRARY_PATH', ''))

I like this approach, because llvmpipe is used to get constant results of pixel test,
it is clear to use llvmpipe only when we are using Xvfb.

[Gwang Yoon Hwang]

Created attachment 233046 [details]
Patch

[Martin Robinson]

Comment on attachment 233046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233046&action=review

> Tools/gtk/jhbuild.modules:342
> +  <autotools id="llvm" autogenargs="--enable-optimized --disable-terminfo --disable-zlib --enable-shared">
> +    <branch repo="llvm.org" module="trunk" checkoutdir="llvm" version="r206311"
> +            revision="206311"/>
> +  </autotools>
> +

Will the LLVM from the system suffice. We only include dependencies in the main JHBuild if they directly affect the results, otherwise just expect that they are already installed on the system. If possible it should instead be added to Tools/gtk/install-dependencies into the JHBuild section.

[Carlos Alberto Lopez Perez]

(In reply to comment #16)
> Created an attachment (id=233046) [details]
> Patch

--- a/Tools/Scripts/webkitpy/port/xvfbdriver.py
+++ b/Tools/Scripts/webkitpy/port/xvfbdriver.py
@@ -87,6 +87,11 @@ class XvfbDriver(Driver):
 
         server_name = self._port.driver_name()
         environment = self._port.setup_environ_for_server(server_name)
+
+        llvmpipe_libgl_path = os.environ.get('LLVMPIPE_LIBGL_PATH')
+        if llvmpipe_libgl_path:
+            environment['LD_LIBRARY_PATH'] = '%s:%s' % (llvmpipe_libgl_path, os.environ.get('LD_LIBRARY_PATH', ''))
+


I believe that this should be done before executing Xvfb. Otherwise Xvfb would be executed with the system OpenGL libraries, meanwhile the browser would be executed with the OpenGL jhbuild Mesa. We want both (Xvfb and the browser) to be executed with the OpenGL jhbuild Mesa.

I suggest moving the code a couple of lines up. Just before:

       run_xvfb = ["Xvfb", ":%d" % display_id, "-screen",  "0", "800x600x%s" % self._xvfb_screen_depth(), "-nolisten", "tcp"]
        with open(os.devnull, 'w') as devnull:
            self._xvfb_process = self._port.host.executive.popen(run_xvfb, stderr=devnull)

[Gwang Yoon Hwang]

(In reply to comment #17)
> (From update of attachment 233046 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233046&action=review
> 
> > Tools/gtk/jhbuild.modules:342
> > +  <autotools id="llvm" autogenargs="--enable-optimized --disable-terminfo --disable-zlib --enable-shared">
> > +    <branch repo="llvm.org" module="trunk" checkoutdir="llvm" version="r206311"
> > +            revision="206311"/>
> > +  </autotools>
> > +
> 
> Will the LLVM from the system suffice. We only include dependencies in the main JHBuild if they directly affect the results, otherwise just expect that they are already installed on the system. If possible it should instead be added to Tools/gtk/install-dependencies into the JHBuild section.

The FTL needs the specific version of llvm, jhbuild-optional builds llvm itself.
I think it is better to share already existed llvm entry for Mesa.

[Gwang Yoon Hwang]

(In reply to comment #18)
> (In reply to comment #16)
> > Created an attachment (id=233046) [details] [details]

> I believe that this should be done before executing Xvfb. Otherwise Xvfb would be executed with the system OpenGL libraries, meanwhile the browser would be executed with the OpenGL jhbuild Mesa. We want both (Xvfb and the browser) to be executed with the OpenGL jhbuild Mesa.
> 

I'm not sure it could be a problem, but changed it to iron out the uncertainty.

[Gwang Yoon Hwang]

Created attachment 233050 [details]
Patch

[Martin Robinson]

(In reply to comment #19)

> The FTL needs the specific version of llvm, jhbuild-optional builds llvm itself.
> I think it is better to share already existed llvm entry for Mesa.


If it is installed via the optional JHBuild moduleset, Mesa will use it. If we ever ship FTL it will need to work on multiple LLVM versions, at which point we can remove it from the optional JHBuild moduleset.

The current way we use the default moduleset is to pull dependencies that don't directly affect test output from the system or the optional moduleset. I don't think this situation differs

[Carlos Alberto Lopez Perez]

(In reply to comment #20)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > Created an attachment (id=233046) [details] [details] [details]
> 
> > I believe that this should be done before executing Xvfb. Otherwise Xvfb would be executed with the system OpenGL libraries, meanwhile the browser would be executed with the OpenGL jhbuild Mesa. We want both (Xvfb and the browser) to be executed with the OpenGL jhbuild Mesa.
> > 
> 
> I'm not sure it could be a problem, but changed it to iron out the uncertainty.

Thanks.

I think it could be a problem otherwise, I happen to use the nvidia propietary drivers so I can test this:

1.a) Launch Xfvb with the system libraries (nvidia in my case):
$ Xvfb :2 -screen 0 800x600x24 -nolisten tcp
1.b) Test the direct rendering with the Mesa libraries (/usr/lib/mesa-diverted/x86_64-linux-gnu is where Debian "diverts" the libraries from Mesa when you install the nvidia propietary drivers):
$ LD_LIBRARY_PATH=/usr/lib/mesa-diverted/x86_64-linux-gnu DISPLAY=:2 glxinfo | grep "render.*:"
Error: couldn't find RGB GLX visual or fbconfig
Error: couldn't find RGB GLX visual or fbconfig

1.a) Now launch Xfvb with the mesa libraries:
$ LD_LIBRARY_PATH=/usr/lib/mesa-diverted/x86_64-linux-gnu Xvfb :2 -screen 0 800x600x24 -nolisten tcp
1.b) Test the direct rendering with the Mesa libraries:
$ LD_LIBRARY_PATH=/usr/lib/mesa-diverted/x86_64-linux-gnu DISPLAY=:2 glxinfo | grep "render.*:"
direct rendering: Yes
OpenGL renderer string: Gallium 0.4 on llvmpipe (LLVM 3.4, 256 bits)

[Carlos Alberto Lopez Perez]

(In reply to comment #22)
> (In reply to comment #19)
> 
> > The FTL needs the specific version of llvm, jhbuild-optional builds llvm itself.
> > I think it is better to share already existed llvm entry for Mesa.
> 
> 
> If it is installed via the optional JHBuild moduleset, Mesa will use it. If we ever ship FTL it will need to work on multiple LLVM versions, at which point we can remove it from the optional JHBuild moduleset.
> 
> The current way we use the default moduleset is to pull dependencies that don't directly affect test output from the system or the optional moduleset. I don't think this situation differs

Remember that the original patch r167510 got reverted because the missing LLVM broke the bots and the EWS.

If we are not going to ship LLVM on the moduleset we should at least inform to the bot and EWS owners to install LLVM before landing this.

install-dependencies is not ran automatically neither on the bots nor the EWS (that would require super user permissions)

[Martin Robinson]

(In reply to comment #24)

> If we are not going to ship LLVM on the moduleset we should at least inform to the bot and EWS owners to install LLVM before landing this.

This is always how we handle things. It's a little painful, but it doesn't happen very often. Perhaps we should run install-dependencies automatically.

[Gwang Yoon Hwang]

(In reply to comment #25)
> (In reply to comment #24)
> 
> > If we are not going to ship LLVM on the moduleset we should at least inform to the bot and EWS owners to install LLVM before landing this.
> 
> This is always how we handle things. It's a little painful, but it doesn't happen very often. Perhaps we should run install-dependencies automatically.

My concern about this is, if a system-installed LLVM and a LLVM from optional modulset exist at the same time, it could be confusing.

So, I think we can remove LLVM from the module set after all and just add it in the install-dependencies after FTL has been enabled without specific build of LLVM.

[Martin Robinson]

(In reply to comment #26)

> My concern about this is, if a system-installed LLVM and a LLVM from optional modulset exist at the same time, it could be confusing.

In what way do you think they will be confused or conflict?
 
> So, I think we can remove LLVM from the module set after all and just add it in the install-dependencies after FTL has been enabled without specific build of LLVM.

I recommend simply keeping it in the optional moduleset, if possible. I can help out making sure it is installed on all the bots before the patch lands.

[Gwang Yoon Hwang]

Created attachment 233136 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #27)
> (In reply to comment #26)
> 
> > My concern about this is, if a system-installed LLVM and a LLVM from optional modulset exist at the same time, it could be confusing.
> 
> In what way do you think they will be confused or conflict?
> 

There can be a human error to use system's LLVM when try to work on FTL if someone didn't install
LLVM from the optional modulset.

> 
> I recommend simply keeping it in the optional moduleset, if possible. I can help out making sure it is installed on all the bots before the patch lands.

It is right to use LLVM from the system package at the last, I agree to add the LLVM entry in the install-dependencies under the condition that all developers watch out about which LLVM they are using.

I modified the patch to add LLVM in the install-dependencies instead of jhbuild.modules.

[Martin Robinson]

Comment on attachment 233136 [details]
Patch

Let's give this a shot. Thanks!

[Carlos Alberto Lopez Perez]

There are the following errors on the EWS:

checking whether gcc supports -Werror=missing-prototypes... yes
checking whether gcc supports -fvisibility=hidden... yes
checking whether g++ supports -fvisibility=hidden... yes
checking whether C compiler accepts -msse4.1... yes
checking whether ld supports --gc-sections... yes
checking whether to enable assembly... yes, x86_64
checking for dlopen... no
checking for dlopen in -ldl... yes
checking for dladdr... yes
checking for clock_gettime... yes
checking for posix_memalign... yes
checking for the pthreads library -lpthreads... no
checking whether pthreads work without any flags... no
checking whether pthreads work with -Kthread... no
checking whether pthreads work with -kthread... no
checking for the pthreads library -llthread... no
checking whether pthreads work with -pthread... yes
checking for joinable pthread attribute... PTHREAD_CREATE_JOINABLE
checking if more special flags are required for pthreads... no
checking for PTHREAD_PRIO_INHERIT... no
configure: Shared GLAPI should not used with Xlib-GLX, disabling
checking for LIBDRM... yes
checking for LIBUDEV... no
checking for XLIBGL... yes
checking for mincore... yes
checking for XCB_DRI2... yes
checking for llvm-config... /usr/bin/llvm-config
checking for RADEON... no
configure: error: Package requirements (libdrm_radeon >= 2.4.54) were not met:

Requested 'libdrm_radeon >= 2.4.54' but version of libdrm_radeon is 2.4.52

Consider adjusting the PKG_CONFIG_PATH environment variable if you
installed software in a non-standard prefix.

Alternatively, you may set the environment variables RADEON_CFLAGS
and RADEON_LIBS to avoid the need to call pkg-config.
See the pkg-config man page for more details.
tall-setuid --disable-unit-tests --enable-unix-transport --enable-tcp-transport --enable-local-transport --with-xkb-path=/usr/share/X11/xkb --with-xkb-output=/var/lib/xkb --with-xkb-bin-directory=/usr/bin --enable-introspection 


Aren't we missing some dependencies?

[Martin Robinson]

(In reply to comment #31)

> checking for RADEON... no
> configure: error: Package requirements (libdrm_radeon >= 2.4.54) were not met:
> 
> Requested 'libdrm_radeon >= 2.4.54' but version of libdrm_radeon is 2.4.52
> 
> Consider adjusting the PKG_CONFIG_PATH environment variable if you
> installed software in a non-standard prefix.

Sounds like we want to specifically disable radeon support. We only want to build the software rasterizer.

[Carlos Alberto Lopez Perez]

(In reply to comment #32)
> Sounds like we want to specifically disable radeon support. We only want to build the software rasterizer.

This configure switch may help:

  --with-gallium-drivers[=DIRS...]
                          comma delimited Gallium drivers list, e.g.
                          "i915,ilo,nouveau,r300,r600,radeonsi,freedreno,svga,swrast"
                          [default=r300,r600,svga,swrast]

[Gwang Yoon Hwang]

(In reply to comment #33)
> (In reply to comment #32)
> > Sounds like we want to specifically disable radeon support. We only want to build the software rasterizer.
> 
> This configure switch may help:
> 
>   --with-gallium-drivers[=DIRS...]
>                           comma delimited Gallium drivers list, e.g.
>                           "i915,ilo,nouveau,r300,r600,radeonsi,freedreno,svga,swrast"
>                           [default=r300,r600,svga,swrast]


Yes, the problem was we didn't set any value to the --with-gallium-drivers= so we got the default value.
I'll re-upload the patch adds --with-gallium-drivers=swrast for the Mesa.

[Gwang Yoon Hwang]

Created attachment 233190 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 233190 [details]
Patch

Clearing flags on attachment: 233190

Committed r170040: <http://trac.webkit.org/changeset/170040>

[Gwang Yoon Hwang]

All reviewed patches have been landed.  Closing bug.