716766 - [Skia] Add runtime detection of NEON support

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

We should modify skia so that we can compile with NEON enabled, but still run on devices that don't support it.

There is an existing (badly rotted) patch at: http://code.google.com/p/chromium/issues/detail?id=67954

Anyone know enough NEON to attempt getting this working again?

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It appears that our existing code for doing this on android-ish is http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/arm.cpp#127 .  It would be cool if skia had a hook we could use to implemen NEON detection with our existing arm.h code, but something tells me that might not be a popular option upstream.

Comment 2

[Makoto Kato [:m_kato] (PTO: 9/26-9/30)]

Attachment: use __ARM_HAVE_OPTIONAL_NEON_SUPPORT

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

We can't quite do mandatory because we run on some devices that don't support NEON, but James has the details.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

We need to check with the Skia guys to figure out what's best here. I don't know how the optional neon bits work there.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

George, can you contact Skia people and ask them about this?

Comment 6

[George Wright (:gw280) (needinfo me!)]

Upstream should have resolved this issue by now, but I will double check with them. Leaving the needinfo open for me as I don't want to address this until we've updated our Skia revision.

Comment 7

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 743454 [details] [diff] [review]
use __ARM_HAVE_OPTIONAL_NEON_SUPPORT

Review of attachment 743454 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to look at this now as it shouldn't take too long, and we've finally updated our skia rev. r- mainly because we modify generate_mozbuild.py now instead of moz.build.

Comment 8

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-716766-Only-set-USE_ANDROID_NDK_CPU_FEATURES-if-.patch

Also submitted upstream @ https://codereview.chromium.org/189263015

Comment 9

[George Wright (:gw280) (needinfo me!)]

Attachment: 0003-Bug-716766-Enable-NEON-Skia-codepaths-r-snorp.patch

The fonthost hunks in the change to moz.build are due to regenerating moz.build after I created this patch: https://hg.mozilla.org/mozilla-central/rev/0f8652814898

The rest should be fairly self explanatory.

Comment 10

[George Wright (:gw280) (needinfo me!)]

https://tbpl.mozilla.org/?tree=Try&rev=8a92f75e4d6b

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think -mfpu=neon allows the compiler to use neon instructions whenever it wants, which is bad (since we run on devices without neon). I am not sure, though.

Comment 12

[George Wright (:gw280) (needinfo me!)]

No, it's correct, because those files are only ever used if Skia detects NEON is available at runtime.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Looks like armv6 is busted, otherwise I guess I'm ok with this.

Comment 14

[George Wright (:gw280) (needinfo me!)]

Yep, looks like I just need to add the neon files in:

if CONFIG['HAVE_ARM_NEON']:

And it'll all work fine. Just doing a local build test now.

Comment 15

[George Wright (:gw280) (needinfo me!)]

https://tbpl.mozilla.org/?tree=Try&rev=a317f269abd3

Comment 16

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-716766-Enable-NEON-Skia-codepaths-r-snorp.patch

Use BUILD_ARM_NEON to protect the NEON files.

Comment 17

[George Wright (:gw280) (needinfo me!)]

Updated try push with BUILD_ARM_NEON:
https://tbpl.mozilla.org/?tree=Try&rev=c914b7631fbb

Comment 18

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25031a8b8b77
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bd9dcf4de29

Comment 19

[George Wright (:gw280) (needinfo me!)]

Relevant upstream bug: https://codereview.chromium.org/189263015 (so the patch won't need to be held locally)

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/a582eefb45b2 for various android crashes: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=36159994&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36160179&tree=Mozilla-Inbound

Comment 21

[George Wright (:gw280) (needinfo me!)]

I don't think the crashes are related to this patch. snorp, can you give a second opinion?

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to George Wright (:gw280) from comment #21)
> I don't think the crashes are related to this patch. snorp, can you give a
> second opinion?

Yeah, doesn't look likely to me.

Comment 23

[George Wright (:gw280) (needinfo me!)]

OK, let's do this again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c752ec797e82
https://hg.mozilla.org/integration/mozilla-inbound/rev/db35f443e62e

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c752ec797e82
https://hg.mozilla.org/mozilla-central/rev/db35f443e62e