New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ARM support #408

Merged
merged 1 commit into from Sep 25, 2016

Conversation

Projects
None yet
3 participants
@mmatyas
Contributor

mmatyas commented Sep 15, 2016

This PR adds ARM support to Webrender:

  • Fixes int-to-float conversions, causing shader compile errors
  • Instead of using OS name/android to determine OpenGL/GLES version, uses the architecture; use GLES if the target arch is ARM or AArch64

A screenshot of Servo running on Odroid XU3:

servo-wr2-arm

(running as ./servo -G es2 -w -Z wr-stats https://en.wikipedia.org/wiki/Rust)

As you can see, there are still some bugs, eg. missing hover effects or font drawing problems of wr-stats, but the rendering itself seems ok, and the performance is really good.
Also I'm not 100% sure about the GL parameters like GL_FORMAT_A and the GpuProfile, I'd be happy if someone could review them.

CC @glennw, @larsbergstrom


This change is Reviewable

@mmatyas

This comment has been minimized.

Show comment
Hide comment
@mmatyas

mmatyas Sep 15, 2016

Contributor

To enable using it in Servo, the OpenGL version handling will also need an update (in ports/glutin/window.rs), I'll send a follow-up PR if this patch lands.

Contributor

mmatyas commented Sep 15, 2016

To enable using it in Servo, the OpenGL version handling will also need an update (in ports/glutin/window.rs), I'll send a follow-up PR if this patch lands.

@glennw

Awesome work! I left a few comments about the format constant declarations.

Show outdated Hide outdated webrender/src/device.rs
@@ -19,28 +19,22 @@ use std::mem;
//use std::thread;
use webrender_traits::ImageFormat;
#[cfg(any(target_os = "linux", target_os = "windows", target_os = "macos"))]
#[cfg(any(target_arch = "arm", target_arch = "aarch64"))]

This comment has been minimized.

@glennw

glennw Sep 15, 2016

Member

These seem to be inverted from what they were previously?

@glennw

glennw Sep 15, 2016

Member

These seem to be inverted from what they were previously?

Show outdated Hide outdated webrender/src/device.rs
const GL_FORMAT_A: gl::GLuint = gl::ALPHA;
#[cfg(any(target_os = "linux", target_os = "windows", target_os = "macos"))]
#[cfg(any(target_arch = "arm", target_arch = "aarch64"))]

This comment has been minimized.

@glennw

glennw Sep 15, 2016

Member

These also seem to be inverted?

@glennw

glennw Sep 15, 2016

Member

These also seem to be inverted?

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 16, 2016

Contributor

☔️ The latest upstream changes (presumably #410) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors-servo commented Sep 16, 2016

☔️ The latest upstream changes (presumably #410) made this pull request unmergeable. Please resolve the merge conflicts.

@mmatyas

This comment has been minimized.

Show comment
Hide comment
@mmatyas

mmatyas Sep 16, 2016

Contributor

Thanks, I've fixed and rebased the patch. I've left the GL_FORMAT_BGRA lines as they were before, because changing that caused a surprising number of build errors in Servo, requiring changes in other dependencies (skia, gleam, ...), and even then it caused a shader error.
However, I couldn't test the PR with the rebase, because #410 causes a gl function was not loaded error for me...

Contributor

mmatyas commented Sep 16, 2016

Thanks, I've fixed and rebased the patch. I've left the GL_FORMAT_BGRA lines as they were before, because changing that caused a surprising number of build errors in Servo, requiring changes in other dependencies (skia, gleam, ...), and even then it caused a shader error.
However, I couldn't test the PR with the rebase, because #410 causes a gl function was not loaded error for me...

@glennw

This comment has been minimized.

Show comment
Hide comment
@glennw

glennw Sep 18, 2016

Member

Interesting - nothing in that PR calls a new function, so I'm not sure why it would cause that error. Could you get a backtrace of that error, which should identify which function isn't being loaded?

Member

glennw commented Sep 18, 2016

Interesting - nothing in that PR calls a new function, so I'm not sure why it would cause that error. Could you get a backtrace of that error, which should identify which function isn't being loaded?

@mmatyas

This comment has been minimized.

Show comment
Hide comment
@mmatyas

mmatyas Sep 21, 2016

Contributor

Sorry for the late reply, here's the backtrace:

$ RUST_BACKTRACE=1 ./servo -G es2 https://en.wikipedia.org/wiki/Rust
gl function was not loaded (thread main, at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/gleam-0.2.22/src/lib.rs:16)
stack backtrace:
   0: 0xb2c128e3 - backtrace::backtrace::libunwind::trace
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.2.3/src/backtrace/mod.rs:82
                 - backtrace::backtrace::trace<closure>
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.2.3/src/backtrace/mod.rs:70
   1: 0xb2c136ab - backtrace::capture::{{impl}}::new
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.2.3/src/lib.rs:96
   2: 0xb1f1a7b7 - servo::main::{{closure}}
                at /home/mmatyas/Servo/servo-arm/servo/components/servo/main.rs:121
   3: 0xb632badb - std::panicking::rust_panic_with_hook::hbcdd76ca45e6ae3d
   4: 0xb5c8663f - std::panicking::begin_panic<&str>
                at /buildslave/rust-buildbot/slave/nightly-dist-rustc-cross-host-linux/build/src/libstd/panicking.rs:383
   5: 0xb5c8b63f - gleam::ffi::missing_fn_panic
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/gleam-0.2.22/src/lib.rs:16
   6: 0xb1f40aab - gleam::ffi::ClearColor
                at /home/mmatyas/Servo/servo-arm/servo/target/arm-unknown-linux-gnueabihf/debug/build/gleam-3321b7699971b81b/out/gl_bindings.rs:1086
   7: 0xb1f409f7 - gleam::gl::clear_color
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/gleam-0.2.22/src/gl.rs:1298
   8: 0xb1f4dddf - glutin_app::window::{{impl}}::new
                at /home/mmatyas/Servo/servo-arm/servo/ports/glutin/window.rs:191
   9: 0xb1f546e3 - glutin_app::create_window
                at /home/mmatyas/Servo/servo-arm/servo/ports/glutin/lib.rs:48
  10: 0xb1f11007 - servo::main
                at /home/mmatyas/Servo/servo-arm/servo/components/servo/main.rs:138
  11: 0xb6331f5b - __rust_maybe_catch_panic
  12: 0xb632a94b - std::rt::lang_start::h6d20aa8156b6f2f5
  13: 0xb1f1ab17 - main
  14: 0xb16bf771 - __libc_start_main

It seems ClearColor fails?

The other dependencies (eg. gleam) were unchanged, maybe this needs some tweaking in them too.

Contributor

mmatyas commented Sep 21, 2016

Sorry for the late reply, here's the backtrace:

$ RUST_BACKTRACE=1 ./servo -G es2 https://en.wikipedia.org/wiki/Rust
gl function was not loaded (thread main, at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/gleam-0.2.22/src/lib.rs:16)
stack backtrace:
   0: 0xb2c128e3 - backtrace::backtrace::libunwind::trace
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.2.3/src/backtrace/mod.rs:82
                 - backtrace::backtrace::trace<closure>
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.2.3/src/backtrace/mod.rs:70
   1: 0xb2c136ab - backtrace::capture::{{impl}}::new
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.2.3/src/lib.rs:96
   2: 0xb1f1a7b7 - servo::main::{{closure}}
                at /home/mmatyas/Servo/servo-arm/servo/components/servo/main.rs:121
   3: 0xb632badb - std::panicking::rust_panic_with_hook::hbcdd76ca45e6ae3d
   4: 0xb5c8663f - std::panicking::begin_panic<&str>
                at /buildslave/rust-buildbot/slave/nightly-dist-rustc-cross-host-linux/build/src/libstd/panicking.rs:383
   5: 0xb5c8b63f - gleam::ffi::missing_fn_panic
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/gleam-0.2.22/src/lib.rs:16
   6: 0xb1f40aab - gleam::ffi::ClearColor
                at /home/mmatyas/Servo/servo-arm/servo/target/arm-unknown-linux-gnueabihf/debug/build/gleam-3321b7699971b81b/out/gl_bindings.rs:1086
   7: 0xb1f409f7 - gleam::gl::clear_color
                at /home/mmatyas/Servo/servo-arm/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/gleam-0.2.22/src/gl.rs:1298
   8: 0xb1f4dddf - glutin_app::window::{{impl}}::new
                at /home/mmatyas/Servo/servo-arm/servo/ports/glutin/window.rs:191
   9: 0xb1f546e3 - glutin_app::create_window
                at /home/mmatyas/Servo/servo-arm/servo/ports/glutin/lib.rs:48
  10: 0xb1f11007 - servo::main
                at /home/mmatyas/Servo/servo-arm/servo/components/servo/main.rs:138
  11: 0xb6331f5b - __rust_maybe_catch_panic
  12: 0xb632a94b - std::rt::lang_start::h6d20aa8156b6f2f5
  13: 0xb1f1ab17 - main
  14: 0xb16bf771 - __libc_start_main

It seems ClearColor fails?

The other dependencies (eg. gleam) were unchanged, maybe this needs some tweaking in them too.

@glennw

This comment has been minimized.

Show comment
Hide comment
@glennw

glennw Sep 21, 2016

Member

It looks like you are missing the -w (enable webrender) flag in the last comment?

Member

glennw commented Sep 21, 2016

It looks like you are missing the -w (enable webrender) flag in the last comment?

@mmatyas

This comment has been minimized.

Show comment
Hide comment
@mmatyas

mmatyas Sep 22, 2016

Contributor

Ah yes, you're right, but unfortunately Servo still produces the same error. I've also noticed that I forgot to update two "target_os="android" lines when I did the rebase, so I've just fixed that, but it didn't help either.

Contributor

mmatyas commented Sep 22, 2016

Ah yes, you're right, but unfortunately Servo still produces the same error. I've also noticed that I forgot to update two "target_os="android" lines when I did the rebase, so I've just fixed that, but it didn't help either.

@glennw

This comment has been minimized.

Show comment
Hide comment
@glennw

glennw Sep 23, 2016

Member

The missing function is glClearColor - this is the first GL function that is ever called, so it probably signals that the GL functions are all failing to load for some reason. Sometimes this also happens when the GL library loader gets linked twice (e.g. if there are two different versions of gleam present in the cargo.lock file).

Could you check what version(s) of gleam are present in the cargo.lock file?

Member

glennw commented Sep 23, 2016

The missing function is glClearColor - this is the first GL function that is ever called, so it probably signals that the GL functions are all failing to load for some reason. Sometimes this also happens when the GL library loader gets linked twice (e.g. if there are two different versions of gleam present in the cargo.lock file).

Could you check what version(s) of gleam are present in the cargo.lock file?

@mmatyas

This comment has been minimized.

Show comment
Hide comment
@mmatyas

mmatyas Sep 23, 2016

Contributor

In ./components/servo/Cargo.lock I only see references to gleam 0.2.22 ("checksum gleam 0.2.22 (registry+https://github.com/rust-lang/crates.io-index)" = "e299fc6b34e698955c6e7cbe7afef2b89774124c82d3636ab85c540edd9ad567").

Here's the linking step that rustc calls: http://pastebin.com/97fxDELF; I've tried things like adding -lGLESv2 manually, or leave only one occurrence of GL, EGL and X11, but that didn't help, I get the same ClearColor error.

Contributor

mmatyas commented Sep 23, 2016

In ./components/servo/Cargo.lock I only see references to gleam 0.2.22 ("checksum gleam 0.2.22 (registry+https://github.com/rust-lang/crates.io-index)" = "e299fc6b34e698955c6e7cbe7afef2b89774124c82d3636ab85c540edd9ad567").

Here's the linking step that rustc calls: http://pastebin.com/97fxDELF; I've tried things like adding -lGLESv2 manually, or leave only one occurrence of GL, EGL and X11, but that didn't help, I get the same ClearColor error.

@mmatyas

This comment has been minimized.

Show comment
Hide comment
@mmatyas

mmatyas Sep 23, 2016

Contributor

Looks like the problem was on my side: I made a completely clean build, and it seems to be working now; sorry for the inconvenience. I'll do some more testing and benchmarking next week.

Contributor

mmatyas commented Sep 23, 2016

Looks like the problem was on my side: I made a completely clean build, and it seems to be working now; sorry for the inconvenience. I'll do some more testing and benchmarking next week.

@glennw

This comment has been minimized.

Show comment
Hide comment
Member

glennw commented Sep 25, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 25, 2016

Contributor

📌 Commit e942cc2 has been approved by glennw

Contributor

bors-servo commented Sep 25, 2016

📌 Commit e942cc2 has been approved by glennw

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 25, 2016

Contributor

⚡️ Test exempted - status

Contributor

bors-servo commented Sep 25, 2016

⚡️ Test exempted - status

@bors-servo bors-servo merged commit e942cc2 into servo:master Sep 25, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test exempted
Details

bors-servo added a commit that referenced this pull request Sep 25, 2016

Auto merge of #408 - mmatyas:arm_support, r=glennw
Add ARM support

This PR adds ARM support to Webrender:

- Fixes int-to-float conversions, causing shader compile errors
- Instead of using OS name/`android` to determine OpenGL/GLES version, uses the architecture; use GLES if the target arch is ARM or AArch64

A screenshot of Servo running on Odroid XU3:

![servo-wr2-arm](https://cloud.githubusercontent.com/assets/4354863/18553633/6f8ddc7a-7b61-11e6-8c96-7a95525fc66f.png)

(running as `./servo -G es2 -w -Z wr-stats https://en.wikipedia.org/wiki/Rust`)

As you can see, there are still some bugs, eg. missing hover effects or font drawing problems of `wr-stats`, but the rendering itself seems ok, and the performance is really good.
Also I'm not 100% sure about the GL parameters like `GL_FORMAT_A` and the `GpuProfile`, I'd be happy if someone could review them.

CC @glennw, @larsbergstrom

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/408)
<!-- Reviewable:end -->
@glennw

This comment has been minimized.

Show comment
Hide comment
@glennw

glennw Sep 25, 2016

Member

Thanks!

Member

glennw commented Sep 25, 2016

Thanks!

bors-servo added a commit to servo/servo that referenced this pull request Sep 30, 2016

Auto merge of #13478 - mmatyas:arm_gles3, r=jdm
Use GL ES3 on ARM devices

<!-- Please describe your changes on the following line: -->
This makes WebRender work on ARM devices; see servo/webrender#408.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `PKG_CONFIG_ALLOW_CROSS=1 PKG_CONFIG_PATH=/usr/lib/arm-linux-gnueabihf/pkgconfig HARFBUZZ_NO_PKG_CONFIG=1 EXPAT_NO_PKG_CONFIG=1 FREETYPE2_NO_PKG_CONFIG=1 FONTCONFIG_NO_PKG_CONFIG=1 CC=arm-linux-gnueabihf-gcc CXX=arm-linux-gnueabihf-g++ ./mach build --dev --target=arm-unknown-linux-gnueabihf` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #10310.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13478)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Sep 30, 2016

Auto merge of #13478 - mmatyas:arm_gles3, r=jdm
Use GL ES3 on ARM devices

<!-- Please describe your changes on the following line: -->
This makes WebRender work on ARM devices; see servo/webrender#408.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `PKG_CONFIG_ALLOW_CROSS=1 PKG_CONFIG_PATH=/usr/lib/arm-linux-gnueabihf/pkgconfig HARFBUZZ_NO_PKG_CONFIG=1 EXPAT_NO_PKG_CONFIG=1 FREETYPE2_NO_PKG_CONFIG=1 FONTCONFIG_NO_PKG_CONFIG=1 CC=arm-linux-gnueabihf-gcc CXX=arm-linux-gnueabihf-g++ ./mach build --dev --target=arm-unknown-linux-gnueabihf` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #10310.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13478)
<!-- Reviewable:end -->

bors-servo added a commit to servo/servo that referenced this pull request Sep 30, 2016

Auto merge of #13478 - mmatyas:arm_gles3, r=jdm
Use GL ES3 on ARM devices

<!-- Please describe your changes on the following line: -->
This makes WebRender work on ARM devices; see servo/webrender#408.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `PKG_CONFIG_ALLOW_CROSS=1 PKG_CONFIG_PATH=/usr/lib/arm-linux-gnueabihf/pkgconfig HARFBUZZ_NO_PKG_CONFIG=1 EXPAT_NO_PKG_CONFIG=1 FREETYPE2_NO_PKG_CONFIG=1 FONTCONFIG_NO_PKG_CONFIG=1 CC=arm-linux-gnueabihf-gcc CXX=arm-linux-gnueabihf-g++ ./mach build --dev --target=arm-unknown-linux-gnueabihf` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #10310.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13478)
<!-- Reviewable:end -->

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 4, 2017

servo: Merge #13478 - Use GL ES3 on ARM devices (from mmatyas:arm_gle…
…s3); r=jdm

<!-- Please describe your changes on the following line: -->
This makes WebRender work on ARM devices; see servo/webrender#408.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `PKG_CONFIG_ALLOW_CROSS=1 PKG_CONFIG_PATH=/usr/lib/arm-linux-gnueabihf/pkgconfig HARFBUZZ_NO_PKG_CONFIG=1 EXPAT_NO_PKG_CONFIG=1 FREETYPE2_NO_PKG_CONFIG=1 FONTCONFIG_NO_PKG_CONFIG=1 CC=arm-linux-gnueabihf-gcc CXX=arm-linux-gnueabihf-g++ ./mach build --dev --target=arm-unknown-linux-gnueabihf` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #10310.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: a559c2035304b03d9662f8c71d0ddc4ad12f800e

xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Feb 4, 2017

servo: Merge #13478 - Use GL ES3 on ARM devices (from mmatyas:arm_gle…
…s3); r=jdm

<!-- Please describe your changes on the following line: -->
This makes WebRender work on ARM devices; see servo/webrender#408.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `PKG_CONFIG_ALLOW_CROSS=1 PKG_CONFIG_PATH=/usr/lib/arm-linux-gnueabihf/pkgconfig HARFBUZZ_NO_PKG_CONFIG=1 EXPAT_NO_PKG_CONFIG=1 FREETYPE2_NO_PKG_CONFIG=1 FONTCONFIG_NO_PKG_CONFIG=1 CC=arm-linux-gnueabihf-gcc CXX=arm-linux-gnueabihf-g++ ./mach build --dev --target=arm-unknown-linux-gnueabihf` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #10310.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: a559c2035304b03d9662f8c71d0ddc4ad12f800e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment