Last Comment Bug 751814 - [Skia] Fixes for ARMv6+ and ARMv4T
: [Skia] Fixes for ARMv6+ and ARMv4T
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM All
: -- normal (vote)
: mozilla15
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-03 23:04 PDT by Mike Hommey [:glandium]
Modified: 2012-05-15 06:27 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Various Skia fixes for ARMv4T and ARMv6+ (4.42 KB, patch)
2012-05-03 23:05 PDT, Mike Hommey [:glandium]
tterribe: review-
Details | Diff | Review
Various fixes for ARM without EDSP and ARMv6+ (8.17 KB, patch)
2012-05-11 01:34 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Review
Various Skia fixes for ARM without EDSP and ARMv6+ (17.07 KB, patch)
2012-05-11 01:40 PDT, Mike Hommey [:glandium]
tterribe: review+
Details | Diff | Review

Description Mike Hommey [:glandium] 2012-05-03 23:04:20 PDT
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.
Comment 1 Mike Hommey [:glandium] 2012-05-03 23:05:22 PDT
Created attachment 620967 [details] [diff] [review]
Various Skia fixes for ARMv4T and ARMv6+
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-05-04 05:00:22 PDT
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?
Comment 3 Timothy B. Terriberry (:derf) 2012-05-04 06:48:23 PDT
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.
Comment 4 Mike Hommey [:glandium] 2012-05-04 08:47:13 PDT
(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.
Comment 5 Timothy B. Terriberry (:derf) 2012-05-04 09:26:05 PDT
(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.
Comment 6 Mike Hommey [:glandium] 2012-05-11 01:34:42 PDT
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?
Comment 7 Mike Hommey [:glandium] 2012-05-11 01:40:10 PDT
Created attachment 623067 [details] [diff] [review]
Various Skia fixes for ARM without EDSP and ARMv6+

With update.sh changes.
Comment 8 Timothy B. Terriberry (:derf) 2012-05-11 07:14:46 PDT
(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.
Comment 10 Matt Woodrow (:mattwoodrow) 2012-05-15 00:13:17 PDT
CC'ing george, as he's had previous experience upstreaming Skia patches.
Comment 11 Ed Morley [:emorley] 2012-05-15 06:27:50 PDT
https://hg.mozilla.org/mozilla-central/rev/2b809c95bd21

Note You need to log in before you can comment on or make changes to this bug.