Closed Bug 716766 Opened 8 years ago Closed 6 years ago

[Skia] Add runtime detection of NEON support

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mattwoodrow, Assigned: gw280)

References

()

Details

(Whiteboard: [ARM-opt])

Attachments

(2 files, 2 obsolete files)

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.
Attachment #743454 - Flags: review?(gwright)
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)
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 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: snorp → gwright
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)
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.
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.
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.
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+
Relevant upstream bug: https://codereview.chromium.org/189263015 (so the patch won't need to be held locally)
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)
https://hg.mozilla.org/mozilla-central/rev/c752ec797e82
https://hg.mozilla.org/mozilla-central/rev/db35f443e62e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Duplicate of this bug: 792924
You need to log in before you can comment on or make changes to this bug.