Re: [pulseaudio-discuss] [PATCH] sbc_math.h: add explicit check for ARMv6 instructions

Re: [pulseaudio-discuss] [PATCH] sbc_math.h: add explicit check for ARMv6 instructions

[Luiz Augusto von Dentz]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

Hi,

On Fri, Mar 25, 2011 at 1:34 PM, Paul Menzel
<paulepanter@users.sourceforge.net> wrote:
> Dear Arun,
>
>
> Am Samstag, den 19.03.2011, 16:14 +0530 schrieb Arun Raghavan:
>
>> On Wed, 2011-02-23 at 01:07 +0530, Arun Raghavan wrote:
>> [...]
>> > The correct fix for this, imo, is in bluez (there is a new
>> > sbc_primitives_armv6.h that can probably be used at least as a
>> > template). We need to do an sbc-udpate on the PA side anyway, and can
>> > pull this when we do.
>>
>> Could you see if the attached patch works for you? If it does, I can
>> push this to the bluez folks.
>
> My tag on the other thread [1] is ambiguous. The error is indeed caused
> by your patch. I will try to implement your recommended changes but will
> probably not get to it before Sunday.

Im including Siarhei Siamashka since he did most, if not all, of this
code and linux-bluetooth to cc.

Btw, the sbc subdir should be in sync with sbc subdir in BlueZ tree.


-- 
Luiz Augusto von Dentz
Computer Engineer

[-- Attachment #2: thumb2-fix.patch --]
[-- Type: text/x-patch, Size: 1361 bytes --]

diff --git a/src/modules/bluetooth/sbc/sbc_math.h b/src/modules/bluetooth/sbc/sbc_math.h
index b87bc81..35d5dcc 100644
--- a/src/modules/bluetooth/sbc/sbc_math.h
+++ b/src/modules/bluetooth/sbc/sbc_math.h
@@ -23,6 +23,8 @@
  *
  */
 
+#include "sbc_primitives_armv6.h"
+
 #define fabs(x) ((x) < 0 ? -(x) : (x))
 /* C does not provide an explicit arithmetic shift right but this will
    always be correct and every compiler *should* generate optimal code */
@@ -47,7 +49,7 @@ typedef int32_t sbc_fixed_t;
 
 #define SBC_FIXED_0(val) { val = 0; }
 #define MUL(a, b)        ((a) * (b))
-#ifdef __arm__
+#ifdef SBC_HAVE_THUMB2
 #define MULA(a, b, res) ({				\
 		int tmp = res;			\
 		__asm__(				\
diff --git a/src/modules/bluetooth/sbc/sbc_primitives_armv6.h b/src/modules/bluetooth/sbc/sbc_primitives_armv6.h
index 1862aed..e70469a 100644
--- a/src/modules/bluetooth/sbc/sbc_primitives_armv6.h
+++ b/src/modules/bluetooth/sbc/sbc_primitives_armv6.h
@@ -38,6 +38,12 @@
 #define SBC_HAVE_ARMV6 1
 #endif
 
+#if defined(__ARM_ARCH_6T2__ ) || defined(__ARM_ARCH_7__) || \
+	defined(__ARM_ARCH_7A__) || defined(__ARM_ARCH_7R__) || \
+	defined(__ARM_ARCH_7M__)
+#define SBC_HAVE_THUMB2 1
+#endif
+
 #if !defined(SBC_HIGH_PRECISION) && (SCALE_OUT_BITS == 15) && \
 	defined(__GNUC__) && defined(SBC_HAVE_ARMV6) && \
 	defined(__ARM_EABI__) && !defined(__thumb__) && \

Re: [pulseaudio-discuss] [PATCH] sbc_math.h: add explicit check for ARMv6 instructions

[Siarhei Siamashka]

On Mon, Mar 28, 2011 at 6:41 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Fri, Mar 25, 2011 at 1:34 PM, Paul Menzel
> <paulepanter@users.sourceforge.net> wrote:
>> Dear Arun,
>>
>>
>> Am Samstag, den 19.03.2011, 16:14 +0530 schrieb Arun Raghavan:
>>
>>> On Wed, 2011-02-23 at 01:07 +0530, Arun Raghavan wrote:
>>> [...]
>>> > The correct fix for this, imo, is in bluez (there is a new
>>> > sbc_primitives_armv6.h that can probably be used at least as a
>>> > template). We need to do an sbc-udpate on the PA side anyway, and can
>>> > pull this when we do.
>>>
>>> Could you see if the attached patch works for you? If it does, I can
>>> push this to the bluez folks.

-#ifdef __arm__
+#ifdef SBC_HAVE_THUMB2

I think this check can be just changed to
#if defined(__arm__) && (!defined(__thumb__) || defined(__thumb2__))

MLA instruction is available on all ARM processors at least since
armv4, unless compiling code for thumb1. A similar modification can be
also added to armv6 sbc encoder optimizations. Right now armv6
assembly optimizations are disabled for both thumb1 and thumb2, while
they could be still used with thumb2.

-- 
Best regards,
Siarhei Siamashka

Re: [pulseaudio-discuss] [PATCH] sbc_math.h: add explicit check for ARMv6 instructions

[Paul Menzel]

[-- Attachment #1: Type: text/plain, Size: 6771 bytes --]

Dear BlueZ folks,


Am Montag, den 28.03.2011, 18:41 +0300 schrieb Luiz Augusto von Dentz:

> On Fri, Mar 25, 2011 at 1:34 PM, Paul Menzel wrote:

> > Am Samstag, den 19.03.2011, 16:14 +0530 schrieb Arun Raghavan:
> >
> >> On Wed, 2011-02-23 at 01:07 +0530, Arun Raghavan wrote:
> >> [...]
> >> > The correct fix for this, imo, is in bluez (there is a new
> >> > sbc_primitives_armv6.h that can probably be used at least as a
> >> > template). We need to do an sbc-udpate on the PA side anyway, and can
> >> > pull this when we do.
> >>
> >> Could you see if the attached patch works for you? If it does, I can
> >> push this to the bluez folks.
> >
> > My tag on the other thread [1] is ambiguous. The error is indeed caused
> > by your patch. I will try to implement your recommended changes but will
> > probably not get to it before Sunday.
> 
> Im including Siarhei Siamashka since he did most, if not all, of this
> code and linux-bluetooth to cc.

to fix [1] I tried to add `#include "sbc_tables.h"` to
`src/modules/bluetooth/sbc/sbc_primitives.h`

        diff --git a/src/modules/bluetooth/sbc/sbc_primitives.h b/src/modules/bluetooth/
        index 3fec8d5..9544826 100644
        --- a/src/modules/bluetooth/sbc/sbc_primitives.h
        +++ b/src/modules/bluetooth/sbc/sbc_primitives.h
        @@ -24,6 +24,8 @@
          *
          */
         
        +#include "sbc_tables.h"
        +
         #ifndef __SBC_PRIMITIVES_H
         #define __SBC_PRIMITIVES_H

But this seems to result in a need for `sbc_math.h` which leads to a
circular inclusion.

        […]
        In file included from modules/bluetooth/sbc/sbc_primitives.h:27:0,
                         from modules/bluetooth/sbc/sbc_primitives_armv6.h:30,
                         from modules/bluetooth/sbc/sbc_math.h:27,
                         from modules/bluetooth/sbc/sbc.c:46:
        modules/bluetooth/sbc/sbc_tables.h:50:2: warning: implicit declaration of function 'ASR' [-Wimplicit-function-declaration]
        modules/bluetooth/sbc/sbc_tables.h:50:2: error: 'SCALE_SPROTO4_TBL' undeclared here (not in a function)
        modules/bluetooth/sbc/sbc_tables.h:58:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:58:2: error: (near initialization for 'sbc_proto_4_40m1[0]')
        modules/bluetooth/sbc/sbc_tables.h:58:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:58:2: error: (near initialization for 'sbc_proto_4_40m1[1]')
        modules/bluetooth/sbc/sbc_tables.h:58:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:58:2: error: (near initialization for 'sbc_proto_4_40m1[2]')
        modules/bluetooth/sbc/sbc_tables.h:58:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:58:2: error: (near initialization for 'sbc_proto_4_40m1[3]')
        modules/bluetooth/sbc/sbc_tables.h:59:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:59:2: error: (near initialization for 'sbc_proto_4_40m1[4]')
        modules/bluetooth/sbc/sbc_tables.h:59:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:59:2: error: (near initialization for 'sbc_proto_4_40m1[5]')
        modules/bluetooth/sbc/sbc_tables.h:59:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:59:2: error: (near initialization for 'sbc_proto_4_40m1[6]')
        modules/bluetooth/sbc/sbc_tables.h:59:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:59:2: error: (near initialization for 'sbc_proto_4_40m1[7]')
        modules/bluetooth/sbc/sbc_tables.h:60:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:60:2: error: (near initialization for 'sbc_proto_4_40m1[8]')
        modules/bluetooth/sbc/sbc_tables.h:60:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:60:2: error: (near initialization for 'sbc_proto_4_40m1[9]')
        modules/bluetooth/sbc/sbc_tables.h:60:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:60:2: error: (near initialization for 'sbc_proto_4_40m1[10]')
        modules/bluetooth/sbc/sbc_tables.h:60:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:60:2: error: (near initialization for 'sbc_proto_4_40m1[11]')
        modules/bluetooth/sbc/sbc_tables.h:61:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:61:2: error: (near initialization for 'sbc_proto_4_40m1[12]')
        modules/bluetooth/sbc/sbc_tables.h:61:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:61:2: error: (near initialization for 'sbc_proto_4_40m1[13]')
        modules/bluetooth/sbc/sbc_tables.h:61:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:61:2: error: (near initialization for 'sbc_proto_4_40m1[14]')
        modules/bluetooth/sbc/sbc_tables.h:61:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:61:2: error: (near initialization for 'sbc_proto_4_40m1[15]')
        modules/bluetooth/sbc/sbc_tables.h:62:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:62:2: error: (near initialization for 'sbc_proto_4_40m1[16]')
        modules/bluetooth/sbc/sbc_tables.h:62:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:62:2: error: (near initialization for 'sbc_proto_4_40m1[17]')
        modules/bluetooth/sbc/sbc_tables.h:62:2: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:62:2: error: (near initialization for 'sbc_proto_4_40m1[18]')
        modules/bluetooth/sbc/sbc_tables.h:63:1: error: initializer element is not constant
        modules/bluetooth/sbc/sbc_tables.h:63:1: error: (near initialization for 'sbc_proto_4_40m1[19]')
        [ goes on … ]

> Btw, the sbc subdir should be in sync with sbc subdir in BlueZ tree.

The question is how often should the updates be made. As far as I know
`make update-sbc` is only run once in a while.

        commit b676f89d8579c7ec1629892342a330f1e4c35657
        Author: Colin Guthrie <cguthrie@mandriva.org>
        Date:   Sun Mar 20 11:44:53 2011 +0000
        
            bluetooth: Run 'make update-sbc'
            
            Note that changes to ipc.h from 8f3ef04b had to be manually reapplied.


Thanks,

Paul


[1] https://tango.0pointer.de/pipermail/pulseaudio-discuss/2011-March/009594.html

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] sbc_math.h: add explicit check for ARMv6 instructions

[Colin Guthrie]

'Twas brillig, and Paul Menzel at 29/03/11 00:00 did gyre and gimble:
>         commit b676f89d8579c7ec1629892342a330f1e4c35657
>         Author: Colin Guthrie <cguthrie@mandriva.org>
>         Date:   Sun Mar 20 11:44:53 2011 +0000
>         
>             bluetooth: Run 'make update-sbc'
>             
>             Note that changes to ipc.h from 8f3ef04b had to be manually reapplied.
> 

I ran this after checking with Luiz first.

I've asked that the local changes to our ipc.h were pushed upstream
after doing so to but not sure of the status of that, Luiz, I think I
included a specific patch in the last mail... let me know if you want it
again :)

Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]

Re: [PATCH] sbc_math.h: add explicit check for ARMv6 instructions

[Luiz Augusto von Dentz]

Hi Colin,

On Tue, Mar 29, 2011 at 1:21 PM, Colin Guthrie <gmane@colin.guthr.ie> wrote:
> 'Twas brillig, and Paul Menzel at 29/03/11 00:00 did gyre and gimble:
>>         commit b676f89d8579c7ec1629892342a330f1e4c35657
>>         Author: Colin Guthrie <cguthrie@mandriva.org>
>>         Date:   Sun Mar 20 11:44:53 2011 +0000
>>
>>             bluetooth: Run 'make update-sbc'
>>
>>             Note that changes to ipc.h from 8f3ef04b had to be manually reapplied.
>>
>
> I ran this after checking with Luiz first.
>
> I've asked that the local changes to our ipc.h were pushed upstream
> after doing so to but not sure of the status of that, Luiz, I think I
> included a specific patch in the last mail... let me know if you want it
> again :)

I think it is better to add the a2dp-codecs.h header present on BlueZ
to PA too and remove the definitions Ive added to ipc.h, this should
make it easier to sync things. I just need some free time to make this
happen :D

-- 
Luiz Augusto von Dentz
Computer Engineer