2012-01-05 04:39:18

by Aj, SanthoshX

[permalink] [raw]
Subject: [PATCH] Breaks in A2DP playback during device search

>From 8ecb4074305613a113d1c8dd220ab856c5f7ebc1 Mon Sep 17 00:00:00 2001
From: Santhosh <[email protected]>
Date: Wed, 4 Jan 2012 12:44:37 +0530
Subject: [PATCH] To change the latency of controller
This change is done to fix the a2dp breaks as the IMC controller
expects the latency to be set explicitly by calling HCI QoS setup

---
audio/device.c | 7 +++
common/Android.mk | 3 +-
common/android_bluez.c | 111 ++++++++++++++++++++++++++++++++++++++---------
3 files changed, 98 insertions(+), 23 deletions(-)

diff --git a/audio/device.c b/audio/device.c
index 9662dec..3e30ea3 100644
--- a/audio/device.c
+++ b/audio/device.c
@@ -66,6 +66,9 @@
#define AVDTP_CONNECT_TIMEOUT 1
#define HEADSET_CONNECT_TIMEOUT 1

+#define QoS_LATENCY_12_MS 0x00002EE0 // 12 ms
+#define QoS_LATENCY_25_MS 0x000061A8 // 25 ms
+
typedef enum {
AUDIO_STATE_DISCONNECTED,
AUDIO_STATE_CONNECTING,
@@ -341,6 +344,9 @@ static void device_sink_cb(struct audio_device *dev,

switch (new_state) {
case SINK_STATE_DISCONNECTED:
+#ifdef ANDROID
+ android_set_qos_latency(&dev->dst,QoS_LATENCY_25_MS);
+#endif
if (dev->control) {
device_remove_control_timer(dev);
avrcp_disconnect(dev);
@@ -368,6 +374,7 @@ static void device_sink_cb(struct audio_device *dev,
break;
#ifdef ANDROID
android_set_high_priority(&dev->dst);
+ android_set_qos_latency(&dev->dst,QoS_LATENCY_12_MS);
#endif
if (dev->auto_connect) {
if (!dev->headset)
diff --git a/common/Android.mk b/common/Android.mk
index 74c0bec..38d6b23 100644
--- a/common/Android.mk
+++ b/common/Android.mk
@@ -13,7 +13,8 @@ LOCAL_SRC_FILES:= \

LOCAL_CFLAGS+= \
-O3 \
- -DNEED_DBUS_WATCH_GET_UNIX_FD
+ -DNEED_DBUS_WATCH_GET_UNIX_FD \
+ -DBOARD_HAVE_BLUETOOTH_IMC

ifeq ($(BOARD_HAVE_BLUETOOTH_BCM),true)
LOCAL_CFLAGS += \
diff --git a/common/android_bluez.c b/common/android_bluez.c
index be85563..3f3b849 100644
--- a/common/android_bluez.c
+++ b/common/android_bluez.c
@@ -78,28 +78,6 @@ static int write_flush_timeout(int fd, uint16_t handle,
return 0;
}

-#ifdef BOARD_HAVE_BLUETOOTH_BCM
-static int vendor_high_priority(int fd, uint16_t handle) {
- unsigned char hci_sleep_cmd[] = {
- 0x01, // HCI command packet
- 0x57, 0xfc, // HCI_Write_High_Priority_Connection
- 0x02, // Length
- 0x00, 0x00 // Handle
- };
-
- hci_sleep_cmd[4] = (uint8_t)handle;
- hci_sleep_cmd[5] = (uint8_t)(handle >> 8);
-
- int ret = write(fd, hci_sleep_cmd, sizeof(hci_sleep_cmd));
- if (ret < 0) {
- error("write(): %s (%d)]", strerror(errno), errno);
- return -1;
- } else if (ret != sizeof(hci_sleep_cmd)) {
- error("write(): unexpected length %d", ret);
- return -1;
- }
- return 0;
-}

static int get_hci_sock() {
int sock = socket(AF_BLUETOOTH, SOCK_RAW, BTPROTO_HCI);
@@ -167,6 +145,30 @@ out:
return ret;
}

+
+#ifdef BOARD_HAVE_BLUETOOTH_BCM
+static int vendor_high_priority(int fd, uint16_t handle) {
+ unsigned char hci_sleep_cmd[] = {
+ 0x01, // HCI command packet
+ 0x57, 0xfc, // HCI_Write_High_Priority_Connection
+ 0x02, // Length
+ 0x00, 0x00 // Handle
+ };
+
+ hci_sleep_cmd[4] = (uint8_t)handle;
+ hci_sleep_cmd[5] = (uint8_t)(handle >> 8);
+
+ int ret = write(fd, hci_sleep_cmd, sizeof(hci_sleep_cmd));
+ if (ret < 0) {
+ error("write(): %s (%d)]", strerror(errno), errno);
+ return -1;
+ } else if (ret != sizeof(hci_sleep_cmd)) {
+ error("write(): unexpected length %d", ret);
+ return -1;
+ }
+ return 0;
+}
+
/* Request that the ACL link to a given Bluetooth connection be high priority,
* for improved coexistance support
*/
@@ -202,3 +204,68 @@ int android_set_high_priority(bdaddr_t *ba) {
}

#endif
+
+#ifdef BOARD_HAVE_BLUETOOTH_IMC
+
+static int android_hci_qos_setup(int fd, uint16_t handle, uint32_t latency) {
+ unsigned char hci_qos_cmd[] = {
+
+ 0x01, // HCI command packet
+ 0x07, 0x08, // QoS Setup Command
+ 0x14, // Length
+ 0x00, 0x01, // Connection_Handle
+ 0x00, //Flags
+ 0x01, //Service_Type
+ 0x00, 0x00, 0xE1, 0x00, //Token_Rate
+ 0x00, 0x00, 0x00, 0x00, //Peak_Bandwidth
+ 0xE0, 0x2E, 0x00, 0x00, //Latency
+ 0x00, 0x00, 0x00, 0x00 //Delay_Variation
+ };
+
+ hci_qos_cmd[4] = (uint8_t)handle;
+ hci_qos_cmd[5] = (uint8_t)(handle >> 8);
+
+ hci_qos_cmd[16] = (uint8_t)latency;
+ hci_qos_cmd[17] = (uint8_t)(latency >> 8);
+ hci_qos_cmd[18] = (uint8_t)(latency >> 16);
+ hci_qos_cmd[19] = (uint8_t)(latency >> 24);
+
+ int ret = write(fd, hci_qos_cmd, sizeof(hci_qos_cmd));
+ if (ret < 0) {
+ error("write(): %s (%d)]", strerror(errno), errno);
+ return -1;
+ } else if (ret != sizeof(hci_qos_cmd)) {
+ error("write(): unexpected length %d", ret);
+ return -1;
+ }
+ return 0;
+}
+
+int android_set_qos_latency(bdaddr_t *ba, uint32_t latency) {
+ int ret;
+ int fd = get_hci_sock();
+ int acl_handle;
+
+ if (fd < 0)
+ return fd;
+
+ acl_handle = get_acl_handle(fd, ba);
+ if (acl_handle < 0) {
+ ret = acl_handle;
+ goto out;
+ }
+ ret = android_hci_qos_setup(fd, acl_handle, latency);
+ if (ret < 0)
+ goto out;
+
+out:
+ close(fd);
+
+ return ret;
+}
+#else
+int android_set_qos_latency(bdaddr_t *ba, uint32_t latency) {
+ return 0;
+}
+#endif
+
--
1.7.0.4



2012-01-06 23:29:07

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Breaks in A2DP playback during device search

Hi Amir,

> >> We encountered the same dilemma and suggested a solution using Flow
> >> specification.
> >>
> >> The Flow Specification HCI relates to an ACL handle and not a specific
> >> L2CAP channel. If we intend to configure the Flow Spec using socket
> >> option then different profiles might overrun each other settings.
> >>
> >> The other option of using the mgmt is not too good either. Since this
> >> command is "Link" related and not "Adapter/Device" related, the daemon
> >> dbus API would have to receive the ACL handle as parameter or introduce
> >> a new property on Device node which might not exist. Note that one can
> >> create an L2CAP socket without interfacing the daemon.
> >>
> >> My humble opinion is to set the Flow Spec per socket (l2cap/rfcomm)
> >> using socket options. Store it per channel and send accumulated flow
> >> spec on every change per ACL.
> >>
> >> - channel changed its flow sepc
> >> - channel with flow spec was closed
> >>
> >> All done in the kernel, no changes to the daemon.
> >
> > Thanks for your valuable inputs; as mentioned earlier please note we are
> > currently using Android Froyo (2.2.2) which has bluez version 4.47 &
> > kernel version 2.6.32.
> >
> > Using Flow specification is seems to be good. I will change it to flow spec
> > instead of qos setup command.
> >
> > I want to know one thing if we want to use setsockopt to send flow spec
> > (add new socket option SO_L2CAP_OQOS) just handle it l2cap.c to send hci
> > flow spec command,
> > that's what you are suggesting?
> >
>
> First as said before you must submit your patch based on latest
> bluetooth-next kernel (not on 2.6.32).
>
> Second, You should get acceptance on adding new socket option. As I
> remember there was an objection on adding new socket options though in
> this case I think it is the correct solution.

I am not against adding new socket options if they make sense. Of course
we are not just adding options until we have figured them out. Reason
here is that socket option are part of the stable ABI promise that the
Linux kernel makes. So we are cautious here.

That said, a proposal need to include how this applies to older
controller with limited set of HCI commands. And of course newer
upcoming specification that might change this should also be taken into
account.

> Here is a short RFC
> I would suggest to store the flow spec parameters in the l2cap_chan struct.
> Store the current active flow spec (also add pending_flow_spec) in the
> hci_conn struct. In hci_connect after
> the acl is connected call a new function hci_calc_flow_spec which goes
> over all l2cap channel (hci_chan->list) and accumulate flow spec params.
> Set the hci_conn pending flow spec and send the HCI cmd. In hci_event add
> case for the command complete and a "cs" function for handling the response.
> In this function set the active flow spec.
> When an l2cap channel disconnect and the ACL is still active you should call
> the hci_calc_flow_spec again to accumulate the params for the rest of
> l2cap channels connected. (You should work out the details).

As a high-level approach this sounds fine. The devil is always in the
details, but you only figure that out once you start writing the code
for it.

> BTW,
> The flow spec params in the hci_conn and l2cap_chan can help us in the
> future in building trafic shaping on the hci_sched_acl.

That would be nice indeed. Though maybe a bit overkill in the end. It
seems that our SO_PRIORITY handling worked out just fine.

Regards

Marcel



2012-01-06 21:57:10

by Marcel Holtmann

[permalink] [raw]
Subject: RE: [PATCH] Breaks in A2DP playback during device search

Hi Santhosh,

> > > > Subject: [PATCH] To change the latency of controller This change
> > is
> > > > done to fix the a2dp breaks as the IMC controller expects the
> > > > latency to be set explicitly by calling HCI QoS setup
> > > >
> > > > ---
> > > > audio/device.c | 7 +++
> > > > common/Android.mk | 3 +-
> > > > common/android_bluez.c | 111
> > > > ++++++++++++++++++++++++++++++++++++++-
> > > --------
> > > > 3 files changed, 98 insertions(+), 23 deletions(-)
> > >
> > > As was previously mentioned by others patches to this list are
> > > expected to be against the upstream git tree. This looks like
> > > something else. One thing I'd like to add though:
> > >
> > > > +static int vendor_high_priority(int fd, uint16_t handle) {
> > > > + unsigned char hci_sleep_cmd[] = {
> > > > + 0x01, // HCI command packet
> > > > + 0x57, 0xfc, // HCI_Write_High_Priority_Connection
> > > > + 0x02, // Length
> > > > + 0x00, 0x00 // Handle
> > > > + };
> > > > +
> > > > + hci_sleep_cmd[4] = (uint8_t)handle;
> > > > + hci_sleep_cmd[5] = (uint8_t)(handle >> 8);
> > > > +
> > > > + int ret = write(fd, hci_sleep_cmd, sizeof(hci_sleep_cmd));
> > > > + if (ret < 0) {
> > > > + error("write(): %s (%d)]", strerror(errno), errno);
> > > > + return -1;
> > > > + } else if (ret != sizeof(hci_sleep_cmd)) {
> > > > + error("write(): unexpected length %d", ret);
> > > > + return -1;
> > > > + }
> > > > + return 0;
> > > > +}
> > >
> > > When you need to need to create a mechanism which controls some
> > aspect
> > > of an RFCOMM, L2CAP or ACL link your first option should be to
> > > consider implementing it as a socket option. If that's not possible
> > > (though in this case I think it is) then the only other alternative
> > is
> > > to abstract it behind the adapter_ops interface since the core daemon
> > > shouldn't any longer do direct HCI access. So you'd implement sending
> > > this command in hciops and then add the appropriate mgmt command and
> > > add support for it into mgmtops.c and the kernel.
> >
> > I work in TI (with Chen Ganir and Ilia Kolominsky),
> >
> > We encountered the same dilemma and suggested a solution using Flow
> > specification.
> >
> > The Flow Specification HCI relates to an ACL handle and not a specific
> > L2CAP channel. If we intend to configure the Flow Spec using socket
> > option then different profiles might overrun each other settings.
> >
> > The other option of using the mgmt is not too good either. Since this
> > command is "Link" related and not "Adapter/Device" related, the daemon
> > dbus API would have to receive the ACL handle as parameter or introduce
> > a new property on Device node which might not exist. Note that one can
> > create an L2CAP socket without interfacing the daemon.
> >
> > My humble opinion is to set the Flow Spec per socket (l2cap/rfcomm)
> > using socket options. Store it per channel and send accumulated flow
> > spec on every change per ACL.
> >
> > - channel changed its flow sepc
> > - channel with flow spec was closed
> >
> > All done in the kernel, no changes to the daemon.
> >
> > Kind Regards,
> > Amir Hadar
> >
> >
>
> Thanks for your valuable inputs; as mentioned earlier please note we are
> currently using Android Froyo (2.2.2) which has bluez version 4.47 &
> kernel version 2.6.32.
>
> Using Flow specification is seems to be good. I will change it to flow spec
> instead of qos setup command.
>
> I want to know one thing if we want to use setsockopt to send flow spec
> (add new socket option SO_L2CAP_OQOS) just handle it l2cap.c to send hci flow spec command,
> that's what you are suggesting?

for the first attempt, I rather go a bit simpler and just trigger this
via SO_PRIORITY and HCI_PRIO_MAX.

We can however also add a general BT_FLOW_SPECIFICATION socket option. I
am looking forward for patches.

Also we have to fallback gracefully to HCI_Setup_QoS for older
controllers.

Regards

Marcel



2012-01-06 14:19:36

by Amir Hadar

[permalink] [raw]
Subject: Re: [PATCH] Breaks in A2DP playback during device search

Hi,

>> We encountered the same dilemma and suggested a solution using Flow
>> specification.
>>
>> The Flow Specification HCI relates to an ACL handle and not a specific
>> L2CAP channel. If we intend to configure the Flow Spec using socket
>> option then different profiles might overrun each other settings.
>>
>> The other option of using the mgmt is not too good either. Since this
>> command is "Link" related and not "Adapter/Device" related, the daemon
>> dbus API would have to receive the ACL handle as parameter or introduce
>> a new property on Device node which might not exist. Note that one can
>> create an L2CAP socket without interfacing the daemon.
>>
>> My humble opinion is to set the Flow Spec per socket (l2cap/rfcomm)
>> using socket options. Store it per channel and send accumulated flow
>> spec on every change per ACL.
>>
>> - channel changed its flow sepc
>> - channel with flow spec was closed
>>
>> All done in the kernel, no changes to the daemon.
>>
>> Kind Regards,
>> Amir Hadar
>>
>>
>
> Thanks for your valuable inputs; as mentioned earlier please note we are
> currently using Android Froyo (2.2.2) which has bluez version 4.47 &
> kernel version 2.6.32.
>
> Using Flow specification is seems to be good. I will change it to flow spec
> instead of qos setup command.
>
> I want to know one thing if we want to use setsockopt to send flow spec
> (add new socket option SO_L2CAP_OQOS) just handle it l2cap.c to send hci
> flow spec command,
> that's what you are suggesting?
>

First as said before you must submit your patch based on latest
bluetooth-next kernel (not on 2.6.32).

Second, You should get acceptance on adding new socket option. As I
remember there was an objection on adding new socket options though in
this case I think it is the correct solution.

Here is a short RFC
I would suggest to store the flow spec parameters in the l2cap_chan struct.
Store the current active flow spec (also add pending_flow_spec) in the
hci_conn struct. In hci_connect after
the acl is connected call a new function hci_calc_flow_spec which goes
over all l2cap channel (hci_chan->list) and accumulate flow spec params.
Set the hci_conn pending flow spec and send the HCI cmd. In hci_event add
case for the command complete and a "cs" function for handling the response.
In this function set the active flow spec.
When an l2cap channel disconnect and the ACL is still active you should call
the hci_calc_flow_spec again to accumulate the params for the rest of
l2cap channels connected. (You should work out the details).

BTW,
The flow spec params in the hci_conn and l2cap_chan can help us in the
future in building trafic shaping on the hci_sched_acl.

Amir

2012-01-06 06:58:21

by Aj, SanthoshX

[permalink] [raw]
Subject: RE: [PATCH] Breaks in A2DP playback during device search

Hi,

> -----Original Message-----
> From: Amir Hadar [mailto:[email protected]]
> Sent: Thursday, January 05, 2012 9:00 PM
> To: Hedberg, Johan; Aj, SanthoshX
> Cc: [email protected]; Holtmann, Marcel; Nair, Rashmi G
> Subject: RE: [PATCH] Breaks in A2DP playback during device search
>
> Hi,
>
> > -----Original Message-----
> > From: [email protected] [mailto:linux-bluetooth-
> > [email protected]] On Behalf Of Johan Hedberg
> > Sent: Thursday, January 05, 2012 4:20 PM
> > To: Aj, SanthoshX
> > Cc: [email protected]; Holtmann, Marcel; Nair, Rashmi G
> > Subject: Re: [PATCH] Breaks in A2DP playback during device search
> >
> > Hi,
> >
> > On Thu, Jan 05, 2012, Aj, SanthoshX wrote:
> > > From 8ecb4074305613a113d1c8dd220ab856c5f7ebc1 Mon Sep 17 00:00:00
> > > 2001
> > > From: Santhosh <[email protected]>
> > > Date: Wed, 4 Jan 2012 12:44:37 +0530
> > > Subject: [PATCH] To change the latency of controller This change
> is
> > > done to fix the a2dp breaks as the IMC controller expects the
> > > latency to be set explicitly by calling HCI QoS setup
> > >
> > > ---
> > > audio/device.c | 7 +++
> > > common/Android.mk | 3 +-
> > > common/android_bluez.c | 111
> > > ++++++++++++++++++++++++++++++++++++++-
> > --------
> > > 3 files changed, 98 insertions(+), 23 deletions(-)
> >
> > As was previously mentioned by others patches to this list are
> > expected to be against the upstream git tree. This looks like
> > something else. One thing I'd like to add though:
> >
> > > +static int vendor_high_priority(int fd, uint16_t handle) {
> > > + unsigned char hci_sleep_cmd[] = {
> > > + 0x01, // HCI command packet
> > > + 0x57, 0xfc, // HCI_Write_High_Priority_Connection
> > > + 0x02, // Length
> > > + 0x00, 0x00 // Handle
> > > + };
> > > +
> > > + hci_sleep_cmd[4] = (uint8_t)handle;
> > > + hci_sleep_cmd[5] = (uint8_t)(handle >> 8);
> > > +
> > > + int ret = write(fd, hci_sleep_cmd, sizeof(hci_sleep_cmd));
> > > + if (ret < 0) {
> > > + error("write(): %s (%d)]", strerror(errno), errno);
> > > + return -1;
> > > + } else if (ret != sizeof(hci_sleep_cmd)) {
> > > + error("write(): unexpected length %d", ret);
> > > + return -1;
> > > + }
> > > + return 0;
> > > +}
> >
> > When you need to need to create a mechanism which controls some
> aspect
> > of an RFCOMM, L2CAP or ACL link your first option should be to
> > consider implementing it as a socket option. If that's not possible
> > (though in this case I think it is) then the only other alternative
> is
> > to abstract it behind the adapter_ops interface since the core daemon
> > shouldn't any longer do direct HCI access. So you'd implement sending
> > this command in hciops and then add the appropriate mgmt command and
> > add support for it into mgmtops.c and the kernel.
>
> I work in TI (with Chen Ganir and Ilia Kolominsky),
>
> We encountered the same dilemma and suggested a solution using Flow
> specification.
>
> The Flow Specification HCI relates to an ACL handle and not a specific
> L2CAP channel. If we intend to configure the Flow Spec using socket
> option then different profiles might overrun each other settings.
>
> The other option of using the mgmt is not too good either. Since this
> command is "Link" related and not "Adapter/Device" related, the daemon
> dbus API would have to receive the ACL handle as parameter or introduce
> a new property on Device node which might not exist. Note that one can
> create an L2CAP socket without interfacing the daemon.
>
> My humble opinion is to set the Flow Spec per socket (l2cap/rfcomm)
> using socket options. Store it per channel and send accumulated flow
> spec on every change per ACL.
>
> - channel changed its flow sepc
> - channel with flow spec was closed
>
> All done in the kernel, no changes to the daemon.
>
> Kind Regards,
> Amir Hadar
>
>

Thanks for your valuable inputs; as mentioned earlier please note we are
currently using Android Froyo (2.2.2) which has bluez version 4.47 &
kernel version 2.6.32.

Using Flow specification is seems to be good. I will change it to flow spec
instead of qos setup command.

I want to know one thing if we want to use setsockopt to send flow spec
(add new socket option SO_L2CAP_OQOS) just handle it l2cap.c to send hci flow spec command,
that's what you are suggesting?

Regards
Santhosh AJ

2012-01-05 15:30:15

by Amir Hadar

[permalink] [raw]
Subject: RE: [PATCH] Breaks in A2DP playback during device search

Hi,

> -----Original Message-----
> From: [email protected] [mailto:linux-bluetooth-
> [email protected]] On Behalf Of Johan Hedberg
> Sent: Thursday, January 05, 2012 4:20 PM
> To: Aj, SanthoshX
> Cc: [email protected]; Holtmann, Marcel; Nair, Rashmi G
> Subject: Re: [PATCH] Breaks in A2DP playback during device search
>
> Hi,
>
> On Thu, Jan 05, 2012, Aj, SanthoshX wrote:
> > From 8ecb4074305613a113d1c8dd220ab856c5f7ebc1 Mon Sep 17 00:00:00 2001
> > From: Santhosh <[email protected]>
> > Date: Wed, 4 Jan 2012 12:44:37 +0530
> > Subject: [PATCH] To change the latency of controller
> > This change is done to fix the a2dp breaks as the IMC controller
> > expects the latency to be set explicitly by calling HCI QoS setup
> >
> > ---
> > audio/device.c | 7 +++
> > common/Android.mk | 3 +-
> > common/android_bluez.c | 111 ++++++++++++++++++++++++++++++++++++++-
> --------
> > 3 files changed, 98 insertions(+), 23 deletions(-)
>
> As was previously mentioned by others patches to this list are expected
> to be against the upstream git tree. This looks like something else. One
> thing I'd like to add though:
>
> > +static int vendor_high_priority(int fd, uint16_t handle) {
> > + unsigned char hci_sleep_cmd[] = {
> > + 0x01, // HCI command packet
> > + 0x57, 0xfc, // HCI_Write_High_Priority_Connection
> > + 0x02, // Length
> > + 0x00, 0x00 // Handle
> > + };
> > +
> > + hci_sleep_cmd[4] = (uint8_t)handle;
> > + hci_sleep_cmd[5] = (uint8_t)(handle >> 8);
> > +
> > + int ret = write(fd, hci_sleep_cmd, sizeof(hci_sleep_cmd));
> > + if (ret < 0) {
> > + error("write(): %s (%d)]", strerror(errno), errno);
> > + return -1;
> > + } else if (ret != sizeof(hci_sleep_cmd)) {
> > + error("write(): unexpected length %d", ret);
> > + return -1;
> > + }
> > + return 0;
> > +}
>
> When you need to need to create a mechanism which controls some aspect
> of an RFCOMM, L2CAP or ACL link your first option should be to consider
> implementing it as a socket option. If that's not possible (though in
> this case I think it is) then the only other alternative is to abstract
> it behind the adapter_ops interface since the core daemon shouldn't any
> longer do direct HCI access. So you'd implement sending this command in
> hciops and then add the appropriate mgmt command and add support for it
> into mgmtops.c and the kernel.

I work in TI (with Chen Ganir and Ilia Kolominsky),

We encountered the same dilemma and suggested a solution using Flow
specification.

The Flow Specification HCI relates to an ACL handle and not a specific
L2CAP channel. If we intend to configure the Flow Spec using socket
option then different profiles might overrun each other settings.

The other option of using the mgmt is not too good either. Since this
command is "Link" related and not "Adapter/Device" related, the daemon
dbus API would have to receive the ACL handle as parameter or introduce
a new property on Device node which might not exist. Note that one can
create an L2CAP socket without interfacing the daemon.

My humble opinion is to set the Flow Spec per socket (l2cap/rfcomm) using
socket options. Store it per channel and send accumulated flow spec on
every change per ACL.

- channel changed its flow sepc
- channel with flow spec was closed

All done in the kernel, no changes to the daemon.

Kind Regards,
Amir Hadar




2012-01-05 14:19:46

by Hedberg, Johan

[permalink] [raw]
Subject: Re: [PATCH] Breaks in A2DP playback during device search

Hi,

On Thu, Jan 05, 2012, Aj, SanthoshX wrote:
> From 8ecb4074305613a113d1c8dd220ab856c5f7ebc1 Mon Sep 17 00:00:00 2001
> From: Santhosh <[email protected]>
> Date: Wed, 4 Jan 2012 12:44:37 +0530
> Subject: [PATCH] To change the latency of controller
> This change is done to fix the a2dp breaks as the IMC controller
> expects the latency to be set explicitly by calling HCI QoS setup
>
> ---
> audio/device.c | 7 +++
> common/Android.mk | 3 +-
> common/android_bluez.c | 111 ++++++++++++++++++++++++++++++++++++++---------
> 3 files changed, 98 insertions(+), 23 deletions(-)

As was previously mentioned by others patches to this list are expected
to be against the upstream git tree. This looks like something else. One
thing I'd like to add though:

> +static int vendor_high_priority(int fd, uint16_t handle) {
> + unsigned char hci_sleep_cmd[] = {
> + 0x01, // HCI command packet
> + 0x57, 0xfc, // HCI_Write_High_Priority_Connection
> + 0x02, // Length
> + 0x00, 0x00 // Handle
> + };
> +
> + hci_sleep_cmd[4] = (uint8_t)handle;
> + hci_sleep_cmd[5] = (uint8_t)(handle >> 8);
> +
> + int ret = write(fd, hci_sleep_cmd, sizeof(hci_sleep_cmd));
> + if (ret < 0) {
> + error("write(): %s (%d)]", strerror(errno), errno);
> + return -1;
> + } else if (ret != sizeof(hci_sleep_cmd)) {
> + error("write(): unexpected length %d", ret);
> + return -1;
> + }
> + return 0;
> +}

When you need to need to create a mechanism which controls some aspect
of an RFCOMM, L2CAP or ACL link your first option should be to consider
implementing it as a socket option. If that's not possible (though in
this case I think it is) then the only other alternative is to abstract
it behind the adapter_ops interface since the core daemon shouldn't any
longer do direct HCI access. So you'd implement sending this command in
hciops and then add the appropriate mgmt command and add support for it
into mgmtops.c and the kernel.

Johan

2012-01-05 13:34:13

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Breaks in A2DP playback during device search

Hi,

On Thu, Jan 5, 2012 at 6:39 AM, Aj, SanthoshX <[email protected]> wrote:
> From 8ecb4074305613a113d1c8dd220ab856c5f7ebc1 Mon Sep 17 00:00:00 2001
> From: Santhosh <[email protected]>
> Date: Wed, 4 Jan 2012 12:44:37 +0530
> Subject: [PATCH] To change the latency of controller
> ?This change is done to fix the a2dp breaks as the IMC controller
> ?expects the latency to be set explicitly by calling HCI QoS setup
>
> ---
> ?audio/device.c ? ? ? ? | ? ?7 +++
> ?common/Android.mk ? ? ?| ? ?3 +-
> ?common/android_bluez.c | ?111 ++++++++++++++++++++++++++++++++++++++---------
> ?3 files changed, 98 insertions(+), 23 deletions(-)

This code is not upstream so we cannot really apply it, but the
problem exists so I would like a bit more feedback on what can be
done. The problem is that it is not known if this will work for other
controllers nor the relation it has when inquiry is active (e.g. does
it changes the inquiry settings somehow?).

Btw, both HCI_QoS_Setup and HCI_Flow_Specification are not supposed to
be configured directly but instead combine the QoS of L2CAP
connections using the ACL. QoS in L2CAP appears to be negotiate during
connection phase, but to really support this completely we would need
support for guaranteed service type which requires a much more complex
queuing discipline than what we currently have.

Perhaps we can tune inquiry parameters when connections exists so they
are not affected?

--
Luiz Augusto von Dentz

2012-01-05 12:07:46

by Ilia, Kolominsky

[permalink] [raw]
Subject: RE: [PATCH] Breaks in A2DP playback during device search

Hi
I`d suggest using HCI_Flow_Specification instead, since you
want to specifly params for outgoing traffic. That command
allows to do it in a more clearer way, since it accepts the
direction param.

> -----Original Message-----
> From: [email protected] [mailto:linux-bluetooth-
> [email protected]] On Behalf Of Aj, SanthoshX
> Sent: Thursday, January 05, 2012 6:39 AM
> To: [email protected]
> Cc: Holtmann, Marcel; Nair, Rashmi G
> Subject: [PATCH] Breaks in A2DP playback during device search
>
> From 8ecb4074305613a113d1c8dd220ab856c5f7ebc1 Mon Sep 17 00:00:00 2001
> From: Santhosh <[email protected]>
> Date: Wed, 4 Jan 2012 12:44:37 +0530
> Subject: [PATCH] To change the latency of controller
> This change is done to fix the a2dp breaks as the IMC controller
> expects the latency to be set explicitly by calling HCI QoS setup
>
> ---
> audio/device.c | 7 +++
> common/Android.mk | 3 +-
> common/android_bluez.c | 111 ++++++++++++++++++++++++++++++++++++++--
> -------
> 3 files changed, 98 insertions(+), 23 deletions(-)
>
> diff --git a/audio/device.c b/audio/device.c
> index 9662dec..3e30ea3 100644
> --- a/audio/device.c
> +++ b/audio/device.c
> @@ -66,6 +66,9 @@
> #define AVDTP_CONNECT_TIMEOUT 1
> #define HEADSET_CONNECT_TIMEOUT 1
>
> +#define QoS_LATENCY_12_MS 0x00002EE0 // 12 ms
> +#define QoS_LATENCY_25_MS 0x000061A8 // 25 ms
> +
> typedef enum {
> AUDIO_STATE_DISCONNECTED,
> AUDIO_STATE_CONNECTING,
> @@ -341,6 +344,9 @@ static void device_sink_cb(struct audio_device
> *dev,
>
> switch (new_state) {
> case SINK_STATE_DISCONNECTED:
> +#ifdef ANDROID
> + android_set_qos_latency(&dev->dst,QoS_LATENCY_25_MS);
> +#endif
> if (dev->control) {
> device_remove_control_timer(dev);
> avrcp_disconnect(dev);
> @@ -368,6 +374,7 @@ static void device_sink_cb(struct audio_device
> *dev,
> break;
> #ifdef ANDROID
> android_set_high_priority(&dev->dst);
> + android_set_qos_latency(&dev->dst,QoS_LATENCY_12_MS);
> #endif
> if (dev->auto_connect) {
> if (!dev->headset)
> diff --git a/common/Android.mk b/common/Android.mk
> index 74c0bec..38d6b23 100644
> --- a/common/Android.mk
> +++ b/common/Android.mk
> @@ -13,7 +13,8 @@ LOCAL_SRC_FILES:= \
>
> LOCAL_CFLAGS+= \
> -O3 \
> - -DNEED_DBUS_WATCH_GET_UNIX_FD
> + -DNEED_DBUS_WATCH_GET_UNIX_FD \
> + -DBOARD_HAVE_BLUETOOTH_IMC
>
> ifeq ($(BOARD_HAVE_BLUETOOTH_BCM),true)
> LOCAL_CFLAGS += \
> diff --git a/common/android_bluez.c b/common/android_bluez.c
> index be85563..3f3b849 100644
> --- a/common/android_bluez.c
> +++ b/common/android_bluez.c
> @@ -78,28 +78,6 @@ static int write_flush_timeout(int fd, uint16_t
> handle,
> return 0;
> }
>
> -#ifdef BOARD_HAVE_BLUETOOTH_BCM
> -static int vendor_high_priority(int fd, uint16_t handle) {
> - unsigned char hci_sleep_cmd[] = {
> - 0x01, // HCI command packet
> - 0x57, 0xfc, // HCI_Write_High_Priority_Connection
> - 0x02, // Length
> - 0x00, 0x00 // Handle
> - };
> -
> - hci_sleep_cmd[4] = (uint8_t)handle;
> - hci_sleep_cmd[5] = (uint8_t)(handle >> 8);
> -
> - int ret = write(fd, hci_sleep_cmd, sizeof(hci_sleep_cmd));
> - if (ret < 0) {
> - error("write(): %s (%d)]", strerror(errno), errno);
> - return -1;
> - } else if (ret != sizeof(hci_sleep_cmd)) {
> - error("write(): unexpected length %d", ret);
> - return -1;
> - }
> - return 0;
> -}
>
> static int get_hci_sock() {
> int sock = socket(AF_BLUETOOTH, SOCK_RAW, BTPROTO_HCI);
> @@ -167,6 +145,30 @@ out:
> return ret;
> }
>
> +
> +#ifdef BOARD_HAVE_BLUETOOTH_BCM
> +static int vendor_high_priority(int fd, uint16_t handle) {
> + unsigned char hci_sleep_cmd[] = {
> + 0x01, // HCI command packet
> + 0x57, 0xfc, // HCI_Write_High_Priority_Connection
> + 0x02, // Length
> + 0x00, 0x00 // Handle
> + };
> +
> + hci_sleep_cmd[4] = (uint8_t)handle;
> + hci_sleep_cmd[5] = (uint8_t)(handle >> 8);
> +
> + int ret = write(fd, hci_sleep_cmd, sizeof(hci_sleep_cmd));
> + if (ret < 0) {
> + error("write(): %s (%d)]", strerror(errno), errno);
> + return -1;
> + } else if (ret != sizeof(hci_sleep_cmd)) {
> + error("write(): unexpected length %d", ret);
> + return -1;
> + }
> + return 0;
> +}
> +
> /* Request that the ACL link to a given Bluetooth connection be high
> priority,
> * for improved coexistance support
> */
> @@ -202,3 +204,68 @@ int android_set_high_priority(bdaddr_t *ba) {
> }
>
> #endif
> +
> +#ifdef BOARD_HAVE_BLUETOOTH_IMC
> +
> +static int android_hci_qos_setup(int fd, uint16_t handle, uint32_t
> latency) {
> + unsigned char hci_qos_cmd[] = {
> +
> + 0x01, // HCI command packet
> + 0x07, 0x08, // QoS Setup Command
> + 0x14, // Length
> + 0x00, 0x01, // Connection_Handle
> + 0x00, //Flags
> + 0x01, //Service_Type
> + 0x00, 0x00, 0xE1, 0x00, //Token_Rate
> + 0x00, 0x00, 0x00, 0x00, //Peak_Bandwidth
> + 0xE0, 0x2E, 0x00, 0x00, //Latency
> + 0x00, 0x00, 0x00, 0x00 //Delay_Variation
> + };
> +
> + hci_qos_cmd[4] = (uint8_t)handle;
> + hci_qos_cmd[5] = (uint8_t)(handle >> 8);
> +
> + hci_qos_cmd[16] = (uint8_t)latency;
> + hci_qos_cmd[17] = (uint8_t)(latency >> 8);
> + hci_qos_cmd[18] = (uint8_t)(latency >> 16);
> + hci_qos_cmd[19] = (uint8_t)(latency >> 24);
> +
> + int ret = write(fd, hci_qos_cmd, sizeof(hci_qos_cmd));
> + if (ret < 0) {
> + error("write(): %s (%d)]", strerror(errno), errno);
> + return -1;
> + } else if (ret != sizeof(hci_qos_cmd)) {
> + error("write(): unexpected length %d", ret);
> + return -1;
> + }
> + return 0;
> +}
> +
> +int android_set_qos_latency(bdaddr_t *ba, uint32_t latency) {
> + int ret;
> + int fd = get_hci_sock();
> + int acl_handle;
> +
> + if (fd < 0)
> + return fd;
> +
> + acl_handle = get_acl_handle(fd, ba);
> + if (acl_handle < 0) {
> + ret = acl_handle;
> + goto out;
> + }
> + ret = android_hci_qos_setup(fd, acl_handle, latency);
> + if (ret < 0)
> + goto out;
> +
> +out:
> + close(fd);
> +
> + return ret;
> +}
> +#else
> +int android_set_qos_latency(bdaddr_t *ba, uint32_t latency) {
> + return 0;
> +}
> +#endif
> +
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html