Patchwork mplayer: fix compilation of NEON assembly in libavcodec

login
register
mail settings
Submitter Arnout Vandecappelle
Date Oct. 5, 2012, 11:03 p.m.
Message ID <1349478193-13673-1-git-send-email-arnout@mind.be>
Download mbox | patch
Permalink /patch/189623/
State Superseded
Headers show

Comments

Arnout Vandecappelle - Oct. 5, 2012, 11:03 p.m.
Compilation of the NEON assembly fails unless neon is enabled in the
compiler options.  This is probably not needed for all combinations of
gcc/binutils, but at least some need it and it certainly doesn't hurt.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

---
 .../multimedia/mplayer/mplayer-1.1-fix-neon.patch   |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Peter Korsgaard - Oct. 8, 2012, 8:27 p.m.
>>>>> "Arnout" == Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> writes:

 Arnout> Compilation of the NEON assembly fails unless neon is enabled in the
 Arnout> compiler options.  This is probably not needed for all combinations of
 Arnout> gcc/binutils, but at least some need it and it certainly doesn't hurt.

 Arnout> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 Arnout> ---
 Arnout>  .../multimedia/mplayer/mplayer-1.1-fix-neon.patch   |   19 +++++++++++++++++++
 Arnout>  1 file changed, 19 insertions(+)

 Arnout> diff --git a/package/multimedia/mplayer/mplayer-1.1-fix-neon.patch b/package/multimedia/mplayer/mplayer-1.1-fix-neon.patch
 Arnout> new file mode 100644
 Arnout> index 0000000..38c2adc
 Arnout> --- /dev/null
 Arnout> +++ b/package/multimedia/mplayer/mplayer-1.1-fix-neon.patch
 Arnout> @@ -0,0 +1,19 @@
 Arnout> +Compilation of the NEON assembly fails unless neon is enabled in the
 Arnout> +compiler options.  This is probably not needed for all combinations of
 Arnout> +gcc/binutils, but at least some need it and it certainly doesn't hurt.
 Arnout> +
 Arnout> +Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
 Arnout> +---
 Arnout> +diff -Nrup mplayer-1.1.orig/configure mplayer-1.1/configure
 Arnout> +--- mplayer-1.1.orig/configure	2012-10-06 00:31:57.706985824 +0200
 Arnout> ++++ mplayer-1.1/configure	2012-10-06 00:49:32.618948116 +0200
 Arnout> +@@ -2935,6 +2935,9 @@ if arm ; then
 Arnout> +     inline_asm_check '"vadd.i16 q0, q0, q0"' && neon=yes
 Arnout> +   fi
 Arnout> +   echores "$neon"
 Arnout> ++  if test $neon = "yes" ; then
 Arnout> ++    extra_cflags="$extra_cflags -mfpu=neon -mfloat-abi=softfp"

Why softfp? Is that just an optimization or is it needed? How does that
work with a hardfp toolchain/libraries?

Please document why this is needed (if it is) in the commit message and
resubmit, thanks.
Arnout Vandecappelle - Oct. 9, 2012, 9:29 p.m.
On 08/10/12 22:27, Peter Korsgaard wrote:
>   Arnout>  +diff -Nrup mplayer-1.1.orig/configure mplayer-1.1/configure
>   Arnout>  +--- mplayer-1.1.orig/configure	2012-10-06 00:31:57.706985824 +0200
>   Arnout>  ++++ mplayer-1.1/configure	2012-10-06 00:49:32.618948116 +0200
>   Arnout>  +@@ -2935,6 +2935,9 @@ if arm ; then
>   Arnout>  +     inline_asm_check '"vadd.i16 q0, q0, q0"'&&  neon=yes
>   Arnout>  +   fi
>   Arnout>  +   echores "$neon"
>   Arnout>  ++  if test $neon = "yes" ; then
>   Arnout>  ++    extra_cflags="$extra_cflags -mfpu=neon -mfloat-abi=softfp"
>
> Why softfp? Is that just an optimization or is it needed? How does that
> work with a hardfp toolchain/libraries?
>
> Please document why this is needed (if it is) in the commit message and
> resubmit, thanks.

  neon fpu (like vfp* fpu) can only be used with the softfp abi, so both are
needed.  This is kind of standard ARM knowledge :-)  Admittedly, it's silly
of gcc that it doesn't automatically set the float-abi to the only supported
one...

  Toolchains that hardcode -mfloat-abi=hard cannot have neon instructions,
I suppose.  I haven't tested it though.

  I also don't know what happens with an external toolchain that has
BR2_VFP_FLOAT set, because that option adds an -mfpu=vfp to the toolchain
command line.

  Regards,
  Arnout
Peter Korsgaard - Oct. 10, 2012, 9:13 a.m.
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >> Please document why this is needed (if it is) in the commit message and
 >> resubmit, thanks.

 Arnout>  neon fpu (like vfp* fpu) can only be used with the softfp abi,
 Arnout> so both are needed.  This is kind of standard ARM knowledge :-)

It is? I've never done any real work on platforms with neon, so it's
certainly a bit fussy to me, but I don't understand what the relation is
with the calling convention.

Aren't the new linaro toolchains hardfp and support neon?

Googling a bit I see stuff like:

http://wiki.meego.com/ARM/hardfp
http://sources.redhat.com/ml/crossgcc/2012-07/msg00091.html

http://wiki.debian.org/ArmHardFloatPort#GCC_floating-point_options
mentions that neon + hardfp abi isn't supported by GCC 4.4, but
otherwise doesn't mention that it cannot work.
Peter Korsgaard - Oct. 15, 2012, 6:53 a.m.
>>>>> "Peter" == Peter Korsgaard <jacmet@uclibc.org> writes:

Hi Arnout,

 >>> Please document why this is needed (if it is) in the commit message and
 >>> resubmit, thanks.

 Arnout> neon fpu (like vfp* fpu) can only be used with the softfp abi,
 Arnout> so both are needed.  This is kind of standard ARM knowledge :-)

 Peter> It is? I've never done any real work on platforms with neon, so it's
 Peter> certainly a bit fussy to me, but I don't understand what the relation is
 Peter> with the calling convention.

 Peter> Aren't the new linaro toolchains hardfp and support neon?

 Peter> Googling a bit I see stuff like:

 Peter> http://wiki.meego.com/ARM/hardfp
 Peter> http://sources.redhat.com/ml/crossgcc/2012-07/msg00091.html

 Peter> http://wiki.debian.org/ArmHardFloatPort#GCC_floating-point_options
 Peter> mentions that neon + hardfp abi isn't supported by GCC 4.4, but
 Peter> otherwise doesn't mention that it cannot work.

Any comments on this?
Arnout Vandecappelle - Oct. 15, 2012, 7:05 p.m.
On 15/10/12 08:53, Peter Korsgaard wrote:
>   Peter>  It is? I've never done any real work on platforms with neon, so it's
>   Peter>  certainly a bit fussy to me, but I don't understand what the relation is
>   Peter>  with the calling convention.
>
>   Peter>  Aren't the new linaro toolchains hardfp and support neon?
>
>   Peter>  Googling a bit I see stuff like:
>
>   Peter>  http://wiki.meego.com/ARM/hardfp
>   Peter>  http://sources.redhat.com/ml/crossgcc/2012-07/msg00091.html
>
>   Peter>  http://wiki.debian.org/ArmHardFloatPort#GCC_floating-point_options
>   Peter>  mentions that neon + hardfp abi isn't supported by GCC 4.4, but
>   Peter>  otherwise doesn't mention that it cannot work.
>
> Any comments on this?

  Sorry, I was going to test it with a Linaro toolchain but forgot about it.
Running it now.

  Regards,
  Arnout
Arnout Vandecappelle - Oct. 15, 2012, 8:49 p.m.
On 10/10/12 11:13, Peter Korsgaard wrote:
>   Arnout>   neon fpu (like vfp* fpu) can only be used with the softfp abi,
>   Arnout>  so both are needed.  This is kind of standard ARM knowledge:-)
>
> It is? I've never done any real work on platforms with neon, so it's
> certainly a bit fussy to me, but I don't understand what the relation is
> with the calling convention.
>
> Aren't the new linaro toolchains hardfp and support neon?

  So these are my conclusions.

- Neon with hard float abi is indeed possible - teaches me not to trust the
gcc manpages :-)  BTW, the float abi is not just the function calling convention,
but also how float variables are laid out in memory - that's why it has an
impact on assembly code as well.

- That means, you need mfloat-abi=softfp on soft abi libc (e.g. sourcery) and
mfloat-abi=hard on hard abi libc (e.g. linaro).  It will be tough for buildroot
to distinguish between them and select the right -mfloat-abi.  On Linaro,
-mfloat-abi=hard is the default so it can be left out, but on Sourcery,
-mfloat-abi=softfp is required (default is soft).

- Internal toolchains are soft abi unless
BR2_EXTRA_GCC_CONFIG_OPTIONS="--with-float=hard" so they also need the
-mfloat-abi=softfp

- So we could just document in the external toolchains which options should be
given to enable neon.

- The vadd.i16 test in mplayer's configure script would detect that neon is not
available (because the -mfloat-abi and -mfpu options are not in CFLAGS) --
except that the test is disabled because we explicitly specify --enable-neon.

- ffmpeg itself always does the test, even if we give --enable-neon.


  So I propose to document the NEON options in the external toolchains, and to
remove --enable-neon from mplayer.  I'm not sure what to do with internal
and crosstool-ng toolchains.


  Regards,
  Arnout

Patch

diff --git a/package/multimedia/mplayer/mplayer-1.1-fix-neon.patch b/package/multimedia/mplayer/mplayer-1.1-fix-neon.patch
new file mode 100644
index 0000000..38c2adc
--- /dev/null
+++ b/package/multimedia/mplayer/mplayer-1.1-fix-neon.patch
@@ -0,0 +1,19 @@ 
+Compilation of the NEON assembly fails unless neon is enabled in the
+compiler options.  This is probably not needed for all combinations of
+gcc/binutils, but at least some need it and it certainly doesn't hurt.
+
+Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
+---
+diff -Nrup mplayer-1.1.orig/configure mplayer-1.1/configure
+--- mplayer-1.1.orig/configure	2012-10-06 00:31:57.706985824 +0200
++++ mplayer-1.1/configure	2012-10-06 00:49:32.618948116 +0200
+@@ -2935,6 +2935,9 @@ if arm ; then
+     inline_asm_check '"vadd.i16 q0, q0, q0"' && neon=yes
+   fi
+   echores "$neon"
++  if test $neon = "yes" ; then
++    extra_cflags="$extra_cflags -mfpu=neon -mfloat-abi=softfp"
++  fi
+ 
+   echocheck "ARM THUMB"
+   if test $armthumb = "auto" ; then