[Skia] Fixes for ARMv6+ and ARMv4T

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla15
ARM
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
In gfx/skia/src/opts/SkBlitRow_opts_arm.cpp, smulbb is used when it's not supported on ARMv4T or ARMv5T. (gfx/skia/include/core/SkMath.h has guards for smulbb use, but they aren't in SkBlitRow_opts_arm.cpp)

In gfx/skia/src/opts/SkBitmapProcState_opts_arm.cpp, Some code is conditional on the ARM architecture being >= 6, but the macro it uses to do that isn't defined, so in practice, this code is not used on ARMv6 and ARMv7.
(Assignee)

Comment 1

5 years ago
Created attachment 620967 [details] [diff] [review]
Various Skia fixes for ARMv4T and ARMv6+
Attachment #620967 - Flags: review?(bjacob)
Comment on attachment 620967 [details] [diff] [review]
Various Skia fixes for ARMv4T and ARMv6+

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

Sorry, I'm not competent for this review. Neither with the ARM platform detection, nor with ARM assembly. Try derf?
Attachment #620967 - Flags: review?(bjacob)
(Assignee)

Updated

5 years ago
Attachment #620967 - Flags: review?(tterribe)
Comment on attachment 620967 [details] [diff] [review]
Various Skia fixes for ARMv4T and ARMv6+

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

The asm itself looks fine. I don't think your #if check is quite right (it misses some cases that gcc has used over the years).

But there are two things I want to know before I can r+ this:

1) gfx/skia has an update.sh and a list of patches that we apply to the upstream source. I assume that these changes need to be added to that script? If there's some other procedure we use for skia, or we're not really maintaining this script, let me know.

2) This only does compile-time detection. Our policy elsewhere in the tree is to do run-time detection. From my brief glance, it doesn't look like skia supports run-time detection at all. Is this correct? What's the story here?

::: gfx/skia/src/opts/SkBlitRow_opts_arm.cpp
@@ +673,5 @@
>  
>                    /* dst1_scale and dst2_scale*/
>                    "lsr    r9, r5, #24                \n\t" /* src >> 24 */
>                    "lsr    r10, r6, #24               \n\t" /* src >> 24 */
> +#if defined(__ARM_ARCH_4T__) || defined(__ARM_ARCH_5T__)

I think you want
#if __ARM_ARCH__ < 5 || __ARM_ARCH__ == 5 && !(defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) || defined(__ARM_ARCH_5TEJ__))

Not that we plan to support less than ARMv4, but AFAIK we don't even support less than ARMv6. See the tests I added to xpcom/glue/arm.h for reference.
Attachment #620967 - Flags: review?(tterribe) → review-
(Assignee)

Comment 4

5 years ago
(In reply to Timothy B. Terriberry (:derf) from comment #3)
> 2) This only does compile-time detection. Our policy elsewhere in the tree
> is to do run-time detection. From my brief glance, it doesn't look like skia
> supports run-time detection at all. Is this correct? What's the story here?

AFAIK, it doesn't. I am not upstream so I can't tell you if it's expected to be supported at some point or not.

> ::: gfx/skia/src/opts/SkBlitRow_opts_arm.cpp
> @@ +673,5 @@
> >  
> >                    /* dst1_scale and dst2_scale*/
> >                    "lsr    r9, r5, #24                \n\t" /* src >> 24 */
> >                    "lsr    r10, r6, #24               \n\t" /* src >> 24 */
> > +#if defined(__ARM_ARCH_4T__) || defined(__ARM_ARCH_5T__)
> 
> I think you want
> #if __ARM_ARCH__ < 5 || __ARM_ARCH__ == 5 && !(defined(__ARM_ARCH_5E__) ||
> defined(__ARM_ARCH_5TE__) || defined(__ARM_ARCH_5TEJ__))

I used that line because there's another place using smulbb using the same test. We could change both, I guess. Also note __ARM_ARCH__ doesn't exist, which the point of the other hunk, so it would need to be in a header. Which one would be the most appropriate?

> Not that we plan to support less than ARMv4, but AFAIK we don't even support
> less than ARMv6. See the tests I added to xpcom/glue/arm.h for reference.

Actually, Debian does support ARMv4T.
(In reply to Mike Hommey [:glandium] from comment #4)
> I used that line because there's another place using smulbb using the same
> test. We could change both, I guess. Also note __ARM_ARCH__ doesn't exist,
> which the point of the other hunk, so it would need to be in a header. Which
> one would be the most appropriate?

I'd make a new SKUtils_opts_arm.h (like the one for SSE2) and put them in there.

The reason I didn't like the original test is that if you don't know the architecture (because the appropriate #defines are not set or you're simply testing for the wrong ones), the default will be to use SMULBB, not the safer MUL. I think you should change it in both places. In fact, I'd probably put the whole test in that header and define a new macro specifically to test for compiler support for the EDSP instructions. That would encourage everyone (I'm assuming you're sending these patches upstream) to use the same test instead of cutting and pasting.

> > Not that we plan to support less than ARMv4, but AFAIK we don't even support
> > less than ARMv6. See the tests I added to xpcom/glue/arm.h for reference.
> 
> Actually, Debian does support ARMv4T.

Hmm, that is a good point. I wasn't sure if that had been dropped with the new armhf port, but I guess the eabi armel port is still being shipped. If that version is compiled specifically for ARMv4T, then I can see the problem.
(Assignee)

Comment 6

5 years ago
Created attachment 623063 [details] [diff] [review]
Various fixes for ARM without EDSP and ARMv6+

Also sent upstream on https://codereview.appspot.com/6195072

Should I put a reviewer there?
Attachment #623063 - Flags: review?(tterribe)
(Assignee)

Updated

5 years ago
Attachment #620967 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 623067 [details] [diff] [review]
Various Skia fixes for ARM without EDSP and ARMv6+

With update.sh changes.
Attachment #623067 - Flags: review?(tterribe)
(Assignee)

Updated

5 years ago
Attachment #623063 - Attachment is obsolete: true
Attachment #623063 - Flags: review?(tterribe)
Attachment #623067 - Flags: review?(tterribe) → review+
(In reply to Mike Hommey [:glandium] from comment #6)
> Should I put a reviewer there?

I don't know who the appropriate upstream reviewer is. If you don't get a response, maybe mattwoodrow or one of the other #gfx folks has some idea.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b809c95bd21
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Assignee: nobody → mh+mozilla
CC'ing george, as he's had previous experience upstreaming Skia patches.
https://hg.mozilla.org/mozilla-central/rev/2b809c95bd21
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.