Closed
Bug 716766
Opened 9 years ago
Closed 7 years ago
[Skia] Add runtime detection of NEON support
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mattwoodrow, Assigned: gw280)
References
()
Details
(Whiteboard: [ARM-opt])
Attachments
(2 files, 2 obsolete files)
1.10 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
8.22 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
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?
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•8 years ago
|
||
Attachment #743454 -
Flags: review?(gwright)
Updated•7 years ago
|
Assignee: nobody → milan
We can't quite do mandatory because we run on some devices that don't support NEON, but James has the details.
Assignee: milan → snorp
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.
George, can you contact Skia people and ask them about this?
Flags: needinfo?(gwright)
Assignee | ||
Comment 6•7 years ago
|
||
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.
Whiteboard: [ARM-opt]
Assignee | ||
Comment 7•7 years ago
|
||
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.
Attachment #743454 -
Flags: review?(gwright) → review-
Assignee | ||
Updated•7 years ago
|
Assignee: snorp → gwright
Assignee | ||
Comment 8•7 years ago
|
||
Also submitted upstream @ https://codereview.chromium.org/189263015
Attachment #8387890 -
Flags: review?(snorp)
Flags: needinfo?(gwright)
Assignee | ||
Comment 9•7 years ago
|
||
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.
Attachment #743454 -
Attachment is obsolete: true
Attachment #8387891 -
Flags: review?(snorp)
Assignee | ||
Comment 10•7 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8a92f75e4d6b
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.
Assignee | ||
Comment 12•7 years ago
|
||
No, it's correct, because those files are only ever used if Skia detects NEON is available at runtime.
Looks like armv6 is busted, otherwise I guess I'm ok with this.
Assignee | ||
Comment 14•7 years ago
|
||
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.
Assignee | ||
Comment 15•7 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a317f269abd3
Assignee | ||
Comment 16•7 years ago
|
||
Use BUILD_ARM_NEON to protect the NEON files.
Attachment #8387891 -
Attachment is obsolete: true
Attachment #8387891 -
Flags: review?(snorp)
Attachment #8388811 -
Flags: review?(snorp)
Attachment #8388811 -
Flags: review?(snorp) → review+
Attachment #8387890 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 17•7 years ago
|
||
Updated try push with BUILD_ARM_NEON: https://tbpl.mozilla.org/?tree=Try&rev=c914b7631fbb
Assignee | ||
Comment 18•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/25031a8b8b77 https://hg.mozilla.org/integration/mozilla-inbound/rev/9bd9dcf4de29
Assignee | ||
Comment 19•7 years ago
|
||
Relevant upstream bug: https://codereview.chromium.org/189263015 (so the patch won't need to be held locally)
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
Flags: needinfo?(gwright)
Assignee | ||
Comment 21•7 years ago
|
||
I don't think the crashes are related to this patch. snorp, can you give a second opinion?
Flags: needinfo?(gwright) → needinfo?(snorp)
(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.
Flags: needinfo?(snorp)
Assignee | ||
Comment 23•7 years ago
|
||
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•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c752ec797e82 https://hg.mozilla.org/mozilla-central/rev/db35f443e62e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•