From: Sathish Narasimman <[email protected]>
For msbc encoded audio stream over usb transport, btusb driver
to be set to alternate settings 6 as per BT core spec 5.0. The
type of air mode is used to differenting which alt setting to be
used.
The changes are made considering some discussion over the similar
patch submitted earlier from Kuba Pawlak(link below)
https://www.spinics.net/lists/linux-bluetooth/msg64577.html
Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Sathish Narasimman <[email protected]>
Signed-off-by: Chethan T N <[email protected]>
Signed-off-by: Hsin-Yu Chao <[email protected]>
Signed-off-by: Amit K Bag <[email protected]>
---
drivers/bluetooth/btusb.c | 157 +++++++++++++++++++++++++++++---------
1 file changed, 121 insertions(+), 36 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index d9cd0677d41c..5563569f4473 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -483,6 +483,8 @@ struct btusb_data {
__u8 cmdreq;
unsigned int sco_num;
+ unsigned int air_mode;
+ bool usb_alt6_packet_flow;
int isoc_altsetting;
int suspend_count;
@@ -974,6 +976,42 @@ static void btusb_isoc_complete(struct urb *urb)
}
}
+static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len,
+ int mtu, struct btusb_data *data)
+{
+ int i, offset = 0;
+ unsigned int interval;
+
+ /* For msbc ALT 6 setting the host will send the packet at continuous
+ * flow. As per core spec 5, vol 4, part B, table 2.1. For ALT setting
+ * 6 the HCI PACKET INTERVAL should be 7.5ms for every usb packets.
+ * To maintain the rate we send 63bytes of usb packets alternatively for
+ * 7ms and 8ms to maintain the rate as 7.5ms.
+ */
+ if (data->usb_alt6_packet_flow) {
+ interval = 7;
+ data->usb_alt6_packet_flow = false;
+ } else {
+ interval = 6;
+ data->usb_alt6_packet_flow = true;
+ }
+
+ BT_DBG("interval:%d len %d mtu %d", interval, len, mtu);
+
+ for (i = 0; i < interval; i++) {
+ urb->iso_frame_desc[i].offset = offset;
+ urb->iso_frame_desc[i].length = offset;
+ }
+
+ if (len && i < BTUSB_MAX_ISOC_FRAMES) {
+ urb->iso_frame_desc[i].offset = offset;
+ urb->iso_frame_desc[i].length = len;
+ i++;
+ }
+
+ urb->number_of_packets = i;
+}
+
static inline void __fill_isoc_descriptor(struct urb *urb, int len, int mtu)
{
int i, offset = 0;
@@ -1377,9 +1415,13 @@ static struct urb *alloc_isoc_urb(struct hci_dev *hdev, struct sk_buff *skb)
urb->transfer_flags = URB_ISO_ASAP;
- __fill_isoc_descriptor(urb, skb->len,
+ if (data->isoc_altsetting == 6)
+ __fill_isoc_descriptor_msbc(urb, skb->len,
+ le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize),
+ data);
+ else
+ __fill_isoc_descriptor(urb, skb->len,
le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
-
skb->dev = (void *)hdev;
return urb;
@@ -1475,6 +1517,7 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
data->sco_num = hci_conn_num(hdev, SCO_LINK);
+ data->air_mode = evt;
schedule_work(&data->work);
}
}
@@ -1522,11 +1565,70 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
return 0;
}
+static int bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
+{
+ struct btusb_data *data = hci_get_drvdata(hdev);
+ int err;
+
+ if (data->isoc_altsetting != new_alts) {
+ unsigned long flags;
+
+ clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+ usb_kill_anchored_urbs(&data->isoc_anchor);
+
+ /* When isochronous alternate setting needs to be
+ * changed, because SCO connection has been added
+ * or removed, a packet fragment may be left in the
+ * reassembling state. This could lead to wrongly
+ * assembled fragments.
+ *
+ * Clear outstanding fragment when selecting a new
+ * alternate setting.
+ */
+ spin_lock_irqsave(&data->rxlock, flags);
+ kfree_skb(data->sco_skb);
+ data->sco_skb = NULL;
+ spin_unlock_irqrestore(&data->rxlock, flags);
+
+ err = __set_isoc_interface(hdev, new_alts);
+ if (err < 0)
+ return err;
+ }
+ if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
+ if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
+ clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+ else
+ btusb_submit_isoc_urb(hdev, GFP_KERNEL);
+ }
+
+ return 0;
+}
+
+static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data, int alt)
+{
+ struct usb_interface *intf = data->isoc;
+ int i;
+
+ BT_DBG("Looking for Alt no :%d", alt);
+
+ if (intf == NULL) {
+ BT_ERR("INterface NULL");
+ return NULL;
+ }
+
+ for (i = 0; i < intf->num_altsetting; i++) {
+ if (intf->altsetting[i].desc.bAlternateSetting == alt)
+ return &intf->altsetting[i];
+ }
+
+ return NULL;
+}
+
static void btusb_work(struct work_struct *work)
{
struct btusb_data *data = container_of(work, struct btusb_data, work);
struct hci_dev *hdev = data->hdev;
- int new_alts;
+ int new_alts = 0;
int err;
if (data->sco_num > 0) {
@@ -1541,44 +1643,27 @@ static void btusb_work(struct work_struct *work)
set_bit(BTUSB_DID_ISO_RESUME, &data->flags);
}
- if (hdev->voice_setting & 0x0020) {
- static const int alts[3] = { 2, 4, 5 };
-
- new_alts = alts[data->sco_num - 1];
- } else {
- new_alts = data->sco_num;
- }
-
- if (data->isoc_altsetting != new_alts) {
- unsigned long flags;
+ if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_CVSD) {
+ if (hdev->voice_setting & 0x0020) {
+ static const int alts[3] = { 2, 4, 5 };
- clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->isoc_anchor);
-
- /* When isochronous alternate setting needs to be
- * changed, because SCO connection has been added
- * or removed, a packet fragment may be left in the
- * reassembling state. This could lead to wrongly
- * assembled fragments.
- *
- * Clear outstanding fragment when selecting a new
- * alternate setting.
- */
- spin_lock_irqsave(&data->rxlock, flags);
- kfree_skb(data->sco_skb);
- data->sco_skb = NULL;
- spin_unlock_irqrestore(&data->rxlock, flags);
+ new_alts = alts[data->sco_num - 1];
+ } else {
+ new_alts = data->sco_num;
+ }
+ } else if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_TRANSP) {
- if (__set_isoc_interface(hdev, new_alts) < 0)
- return;
- }
+ data->usb_alt6_packet_flow = true;
- if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
- if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
- clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+ /* Check if Alt 6 is supported for Transparent audio*/
+ if (btusb_find_altsetting(data, 6))
+ new_alts = 6;
else
- btusb_submit_isoc_urb(hdev, GFP_KERNEL);
+ BT_ERR("%s Device does not support ALT setting 6", hdev->name);
}
+
+ if (bt_switch_alt_setting(hdev, new_alts) < 0)
+ BT_ERR("%s Set USB Alt: %d failed!", hdev->name, new_alts);
} else {
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
usb_kill_anchored_urbs(&data->isoc_anchor);
--
2.17.1
Hi,
Any more comments regarding this patch?
On Thu, Dec 5, 2019 at 2:02 PM Sathish Narsimman <[email protected]> wrote:
>
> From: Sathish Narasimman <[email protected]>
>
> For msbc encoded audio stream over usb transport, btusb driver
> to be set to alternate settings 6 as per BT core spec 5.0. The
> type of air mode is used to differenting which alt setting to be
> used.
> The changes are made considering some discussion over the similar
> patch submitted earlier from Kuba Pawlak(link below)
> https://www.spinics.net/lists/linux-bluetooth/msg64577.html
>
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Sathish Narasimman <[email protected]>
> Signed-off-by: Chethan T N <[email protected]>
> Signed-off-by: Hsin-Yu Chao <[email protected]>
> Signed-off-by: Amit K Bag <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 157 +++++++++++++++++++++++++++++---------
> 1 file changed, 121 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d9cd0677d41c..5563569f4473 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -483,6 +483,8 @@ struct btusb_data {
> __u8 cmdreq;
>
> unsigned int sco_num;
> + unsigned int air_mode;
> + bool usb_alt6_packet_flow;
> int isoc_altsetting;
> int suspend_count;
>
> @@ -974,6 +976,42 @@ static void btusb_isoc_complete(struct urb *urb)
> }
> }
>
> +static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len,
> + int mtu, struct btusb_data *data)
> +{
> + int i, offset = 0;
> + unsigned int interval;
> +
> + /* For msbc ALT 6 setting the host will send the packet at continuous
> + * flow. As per core spec 5, vol 4, part B, table 2.1. For ALT setting
> + * 6 the HCI PACKET INTERVAL should be 7.5ms for every usb packets.
> + * To maintain the rate we send 63bytes of usb packets alternatively for
> + * 7ms and 8ms to maintain the rate as 7.5ms.
> + */
> + if (data->usb_alt6_packet_flow) {
> + interval = 7;
> + data->usb_alt6_packet_flow = false;
> + } else {
> + interval = 6;
> + data->usb_alt6_packet_flow = true;
> + }
> +
> + BT_DBG("interval:%d len %d mtu %d", interval, len, mtu);
> +
> + for (i = 0; i < interval; i++) {
> + urb->iso_frame_desc[i].offset = offset;
> + urb->iso_frame_desc[i].length = offset;
> + }
> +
> + if (len && i < BTUSB_MAX_ISOC_FRAMES) {
> + urb->iso_frame_desc[i].offset = offset;
> + urb->iso_frame_desc[i].length = len;
> + i++;
> + }
> +
> + urb->number_of_packets = i;
> +}
> +
> static inline void __fill_isoc_descriptor(struct urb *urb, int len, int mtu)
> {
> int i, offset = 0;
> @@ -1377,9 +1415,13 @@ static struct urb *alloc_isoc_urb(struct hci_dev *hdev, struct sk_buff *skb)
>
> urb->transfer_flags = URB_ISO_ASAP;
>
> - __fill_isoc_descriptor(urb, skb->len,
> + if (data->isoc_altsetting == 6)
> + __fill_isoc_descriptor_msbc(urb, skb->len,
> + le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize),
> + data);
> + else
> + __fill_isoc_descriptor(urb, skb->len,
> le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
> -
> skb->dev = (void *)hdev;
>
> return urb;
> @@ -1475,6 +1517,7 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
>
> if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> data->sco_num = hci_conn_num(hdev, SCO_LINK);
> + data->air_mode = evt;
> schedule_work(&data->work);
> }
> }
> @@ -1522,11 +1565,70 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> return 0;
> }
>
> +static int bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> +{
> + struct btusb_data *data = hci_get_drvdata(hdev);
> + int err;
> +
> + if (data->isoc_altsetting != new_alts) {
> + unsigned long flags;
> +
> + clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> + usb_kill_anchored_urbs(&data->isoc_anchor);
> +
> + /* When isochronous alternate setting needs to be
> + * changed, because SCO connection has been added
> + * or removed, a packet fragment may be left in the
> + * reassembling state. This could lead to wrongly
> + * assembled fragments.
> + *
> + * Clear outstanding fragment when selecting a new
> + * alternate setting.
> + */
> + spin_lock_irqsave(&data->rxlock, flags);
> + kfree_skb(data->sco_skb);
> + data->sco_skb = NULL;
> + spin_unlock_irqrestore(&data->rxlock, flags);
> +
> + err = __set_isoc_interface(hdev, new_alts);
> + if (err < 0)
> + return err;
> + }
> + if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> + if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> + clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> + else
> + btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> + }
> +
> + return 0;
> +}
> +
> +static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data, int alt)
> +{
> + struct usb_interface *intf = data->isoc;
> + int i;
> +
> + BT_DBG("Looking for Alt no :%d", alt);
> +
> + if (intf == NULL) {
> + BT_ERR("INterface NULL");
> + return NULL;
> + }
> +
> + for (i = 0; i < intf->num_altsetting; i++) {
> + if (intf->altsetting[i].desc.bAlternateSetting == alt)
> + return &intf->altsetting[i];
> + }
> +
> + return NULL;
> +}
> +
> static void btusb_work(struct work_struct *work)
> {
> struct btusb_data *data = container_of(work, struct btusb_data, work);
> struct hci_dev *hdev = data->hdev;
> - int new_alts;
> + int new_alts = 0;
> int err;
>
> if (data->sco_num > 0) {
> @@ -1541,44 +1643,27 @@ static void btusb_work(struct work_struct *work)
> set_bit(BTUSB_DID_ISO_RESUME, &data->flags);
> }
>
> - if (hdev->voice_setting & 0x0020) {
> - static const int alts[3] = { 2, 4, 5 };
> -
> - new_alts = alts[data->sco_num - 1];
> - } else {
> - new_alts = data->sco_num;
> - }
> -
> - if (data->isoc_altsetting != new_alts) {
> - unsigned long flags;
> + if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_CVSD) {
> + if (hdev->voice_setting & 0x0020) {
> + static const int alts[3] = { 2, 4, 5 };
>
> - clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> - usb_kill_anchored_urbs(&data->isoc_anchor);
> -
> - /* When isochronous alternate setting needs to be
> - * changed, because SCO connection has been added
> - * or removed, a packet fragment may be left in the
> - * reassembling state. This could lead to wrongly
> - * assembled fragments.
> - *
> - * Clear outstanding fragment when selecting a new
> - * alternate setting.
> - */
> - spin_lock_irqsave(&data->rxlock, flags);
> - kfree_skb(data->sco_skb);
> - data->sco_skb = NULL;
> - spin_unlock_irqrestore(&data->rxlock, flags);
> + new_alts = alts[data->sco_num - 1];
> + } else {
> + new_alts = data->sco_num;
> + }
> + } else if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_TRANSP) {
>
> - if (__set_isoc_interface(hdev, new_alts) < 0)
> - return;
> - }
> + data->usb_alt6_packet_flow = true;
>
> - if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> - if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> - clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> + /* Check if Alt 6 is supported for Transparent audio*/
> + if (btusb_find_altsetting(data, 6))
> + new_alts = 6;
> else
> - btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> + BT_ERR("%s Device does not support ALT setting 6", hdev->name);
> }
> +
> + if (bt_switch_alt_setting(hdev, new_alts) < 0)
> + BT_ERR("%s Set USB Alt: %d failed!", hdev->name, new_alts);
> } else {
> clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> usb_kill_anchored_urbs(&data->isoc_anchor);
> --
> 2.17.1
>
Regards
Sathish N
Hi Sathish,
> For msbc encoded audio stream over usb transport, btusb driver
> to be set to alternate settings 6 as per BT core spec 5.0. The
> type of air mode is used to differenting which alt setting to be
> used.
> The changes are made considering some discussion over the similar
> patch submitted earlier from Kuba Pawlak(link below)
> https://www.spinics.net/lists/linux-bluetooth/msg64577.html
>
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Sathish Narasimman <[email protected]>
> Signed-off-by: Chethan T N <[email protected]>
> Signed-off-by: Hsin-Yu Chao <[email protected]>
> Signed-off-by: Amit K Bag <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 157 +++++++++++++++++++++++++++++---------
> 1 file changed, 121 insertions(+), 36 deletions(-)
I would really prefer to have some Tested-by lines from people that can verify that this patch works.
Luiz, do we. Need to address further details for mSBC support?
Regards
Marcel
Hi Marcel,
>
> Hi Sathish,
>
> > For msbc encoded audio stream over usb transport, btusb driver
> > to be set to alternate settings 6 as per BT core spec 5.0. The
> > type of air mode is used to differenting which alt setting to be
> > used.
> > The changes are made considering some discussion over the similar
> > patch submitted earlier from Kuba Pawlak(link below)
> > https://www.spinics.net/lists/linux-bluetooth/msg64577.html
> >
> > Reported-by: kbuild test robot <[email protected]>
> > Signed-off-by: Sathish Narasimman <[email protected]>
> > Signed-off-by: Chethan T N <[email protected]>
> > Signed-off-by: Hsin-Yu Chao <[email protected]>
> > Signed-off-by: Amit K Bag <[email protected]>
> > ---
> > drivers/bluetooth/btusb.c | 157 +++++++++++++++++++++++++++++---------
> > 1 file changed, 121 insertions(+), 36 deletions(-)
>
> I would really prefer to have some Tested-by lines from people that can verify that this patch works.
The patch is verified on Chrome OS( 4.14 and 4.19 Chrome kernel) and
Ubuntu with intel controllers. However it is always good to test with
some other controller as well.
>
> Luiz, do we. Need to address further details for mSBC support?
>
> Regards
>
> Marcel
>
Thanks and Regards
Chethan
gentle reminder.
On Mon, Jan 6, 2020 at 2:09 PM chethan tn <[email protected]> wrote:
>
> Hi Marcel,
> >
> > Hi Sathish,
> >
> > > For msbc encoded audio stream over usb transport, btusb driver
> > > to be set to alternate settings 6 as per BT core spec 5.0. The
> > > type of air mode is used to differenting which alt setting to be
> > > used.
> > > The changes are made considering some discussion over the similar
> > > patch submitted earlier from Kuba Pawlak(link below)
> > > https://www.spinics.net/lists/linux-bluetooth/msg64577.html
> > >
> > > Reported-by: kbuild test robot <[email protected]>
> > > Signed-off-by: Sathish Narasimman <[email protected]>
> > > Signed-off-by: Chethan T N <[email protected]>
> > > Signed-off-by: Hsin-Yu Chao <[email protected]>
> > > Signed-off-by: Amit K Bag <[email protected]>
> > > ---
> > > drivers/bluetooth/btusb.c | 157 +++++++++++++++++++++++++++++---------
> > > 1 file changed, 121 insertions(+), 36 deletions(-)
> >
> > I would really prefer to have some Tested-by lines from people that can verify that this patch works.
> The patch is verified on Chrome OS( 4.14 and 4.19 Chrome kernel) and
> Ubuntu with intel controllers. However it is always good to test with
> some other controller as well.
> >
> > Luiz, do we. Need to address further details for mSBC support?
> >
> > Regards
> >
> > Marcel
> >
> Thanks and Regards
> Chethan
Regards
Sathish N
From: Guillaume Martres <[email protected]>
Hi Sathish, Marcel,
> I would really prefer to have some Tested-by lines from people that can verify that this patch works.
Here's my experience: a recent pulseaudio MR [1] added support for mSBC. When I
tried it using my laptop (Intel AX200) and headphones (Bose Headphone 700), the
voice quality was extremely bad. I eventually realized that this could be fixed
by ignoring all zero-filled packets sent by the headphones when decoding
mSBC [2], after that everything worked fine.
Since then, I was made aware of the existence of this patchset, after applying
this patch (and [3] which apparently only exists as v5 and not v6), my
headphones stopped sending zero-filled packets, but:
- the recorded audio quality is again extremely bad and robotic.
- when audio is being played, the headphones do not emit any sound.
Note that I applied this patchset on top of the current Ubuntu 20.04 kernel [4],
let me know if it's worth retesting on top of the mainline kernel, or if there
is something else I could try.
Hope this helps,
Guillaume
[1]: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/merge_requests/254
[2]: https://gitlab.freedesktop.org/smarter/pulseaudio/-/commit/bb716b03002841e1092b4200935566d5c1a951fe
[3]: https://www.spinics.net/lists/linux-bluetooth/msg82149.html
[4]: https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/focal/commit/?h=Ubuntu-5.4.0-18.22&id=93dfa5b8e12fed29933f3451db44d88c0e4b5aed
On Sat, 21 Mar 2020 at 17:55, Guillaume Martres <[email protected]> wrote:
> Since then, I was made aware of the existence of this patchset, after applying
> this patch (and [3] which apparently only exists as v5 and not v6), my
> headphones stopped sending zero-filled packets, but:
> - the recorded audio quality is again extremely bad and robotic.
> - when audio is being played, the headphones do not emit any sound.
I figured out what the problem was: pulseaudio assumes an MTU of 48
but with this
patch it now ends up being 60, I've left a comment on the pulseaudio MR [1].
After fixing this, audio input and output seem to be working correctly, however
from time to time pulseaudio fails to decode an mSBC packet, but I don't know
whose fault that is.
[1]: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/merge_requests/254#note_444249
On Sat, 21 Mar 2020 at 17:55, Guillaume Martres <[email protected]> wrote:
>
> From: Guillaume Martres <[email protected]>
>
> Hi Sathish, Marcel,
>
> > I would really prefer to have some Tested-by lines from people that can verify that this patch works.
>
> Here's my experience: a recent pulseaudio MR [1] added support for mSBC. When I
> tried it using my laptop (Intel AX200) and headphones (Bose Headphone 700), the
> voice quality was extremely bad. I eventually realized that this could be fixed
> by ignoring all zero-filled packets sent by the headphones when decoding
> mSBC [2], after that everything worked fine.
>
> Since then, I was made aware of the existence of this patchset, after applying
> this patch (and [3] which apparently only exists as v5 and not v6), my
> headphones stopped sending zero-filled packets, but:
> - the recorded audio quality is again extremely bad and robotic.
> - when audio is being played, the headphones do not emit any sound.
>
> Note that I applied this patchset on top of the current Ubuntu 20.04 kernel [4],
> let me know if it's worth retesting on top of the mainline kernel, or if there
> is something else I could try.
>
> Hope this helps,
> Guillaume
>
>
> [1]: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/merge_requests/254
> [2]: https://gitlab.freedesktop.org/smarter/pulseaudio/-/commit/bb716b03002841e1092b4200935566d5c1a951fe
> [3]: https://www.spinics.net/lists/linux-bluetooth/msg82149.html
> [4]: https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/focal/commit/?h=Ubuntu-5.4.0-18.22&id=93dfa5b8e12fed29933f3451db44d88c0e4b5aed
Hi Sathish,
> For msbc encoded audio stream over usb transport, btusb driver
> to be set to alternate settings 6 as per BT core spec 5.0. The
> type of air mode is used to differenting which alt setting to be
> used.
> The changes are made considering some discussion over the similar
> patch submitted earlier from Kuba Pawlak(link below)
> https://www.spinics.net/lists/linux-bluetooth/msg64577.html
>
> Reported-by: kbuild test robot <[email protected]>
> Signed-off-by: Sathish Narasimman <[email protected]>
> Signed-off-by: Chethan T N <[email protected]>
> Signed-off-by: Hsin-Yu Chao <[email protected]>
> Signed-off-by: Amit K Bag <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 157 +++++++++++++++++++++++++++++---------
> 1 file changed, 121 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d9cd0677d41c..5563569f4473 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -483,6 +483,8 @@ struct btusb_data {
> __u8 cmdreq;
>
> unsigned int sco_num;
> + unsigned int air_mode;
> + bool usb_alt6_packet_flow;
> int isoc_altsetting;
> int suspend_count;
>
> @@ -974,6 +976,42 @@ static void btusb_isoc_complete(struct urb *urb)
> }
> }
>
> +static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len,
> + int mtu, struct btusb_data *data)
> +{
> + int i, offset = 0;
> + unsigned int interval;
> +
> + /* For msbc ALT 6 setting the host will send the packet at continuous
> + * flow. As per core spec 5, vol 4, part B, table 2.1. For ALT setting
> + * 6 the HCI PACKET INTERVAL should be 7.5ms for every usb packets.
> + * To maintain the rate we send 63bytes of usb packets alternatively for
> + * 7ms and 8ms to maintain the rate as 7.5ms.
> + */
> + if (data->usb_alt6_packet_flow) {
> + interval = 7;
> + data->usb_alt6_packet_flow = false;
> + } else {
> + interval = 6;
> + data->usb_alt6_packet_flow = true;
> + }
> +
> + BT_DBG("interval:%d len %d mtu %d", interval, len, mtu);
do you really want to keep this BT_DBG here?
> +
> + for (i = 0; i < interval; i++) {
> + urb->iso_frame_desc[i].offset = offset;
> + urb->iso_frame_desc[i].length = offset;
> + }
> +
> + if (len && i < BTUSB_MAX_ISOC_FRAMES) {
> + urb->iso_frame_desc[i].offset = offset;
> + urb->iso_frame_desc[i].length = len;
> + i++;
> + }
> +
> + urb->number_of_packets = i;
> +}
> +
> static inline void __fill_isoc_descriptor(struct urb *urb, int len, int mtu)
> {
> int i, offset = 0;
> @@ -1377,9 +1415,13 @@ static struct urb *alloc_isoc_urb(struct hci_dev *hdev, struct sk_buff *skb)
>
> urb->transfer_flags = URB_ISO_ASAP;
>
> - __fill_isoc_descriptor(urb, skb->len,
> + if (data->isoc_altsetting == 6)
> + __fill_isoc_descriptor_msbc(urb, skb->len,
> + le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize),
> + data);
> + else
> + __fill_isoc_descriptor(urb, skb->len,
> le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
> -
Your changes are now breaking the coding style. Please fix them.
> skb->dev = (void *)hdev;
>
> return urb;
> @@ -1475,6 +1517,7 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
>
> if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> data->sco_num = hci_conn_num(hdev, SCO_LINK);
> + data->air_mode = evt;
> schedule_work(&data->work);
> }
> }
> @@ -1522,11 +1565,70 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> return 0;
> }
>
> +static int bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> +{
> + struct btusb_data *data = hci_get_drvdata(hdev);
> + int err;
> +
> + if (data->isoc_altsetting != new_alts) {
> + unsigned long flags;
> +
> + clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> + usb_kill_anchored_urbs(&data->isoc_anchor);
> +
> + /* When isochronous alternate setting needs to be
> + * changed, because SCO connection has been added
> + * or removed, a packet fragment may be left in the
> + * reassembling state. This could lead to wrongly
> + * assembled fragments.
> + *
> + * Clear outstanding fragment when selecting a new
> + * alternate setting.
> + */
> + spin_lock_irqsave(&data->rxlock, flags);
> + kfree_skb(data->sco_skb);
> + data->sco_skb = NULL;
> + spin_unlock_irqrestore(&data->rxlock, flags);
> +
> + err = __set_isoc_interface(hdev, new_alts);
> + if (err < 0)
> + return err;
> + }
> + if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> + if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> + clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> + else
> + btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> + }
> +
> + return 0;
> +}
> +
> +static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data, int alt)
> +{
> + struct usb_interface *intf = data->isoc;
> + int i;
> +
> + BT_DBG("Looking for Alt no :%d", alt);
> +
> + if (intf == NULL) {
> + BT_ERR("INterface NULL");
I would not put an error here. And we check if (!intf).
> + return NULL;
> + }
> +
> + for (i = 0; i < intf->num_altsetting; i++) {
> + if (intf->altsetting[i].desc.bAlternateSetting == alt)
> + return &intf->altsetting[i];
> + }
> +
> + return NULL;
> +}
> +
> static void btusb_work(struct work_struct *work)
> {
> struct btusb_data *data = container_of(work, struct btusb_data, work);
> struct hci_dev *hdev = data->hdev;
> - int new_alts;
> + int new_alts = 0;
> int err;
>
> if (data->sco_num > 0) {
> @@ -1541,44 +1643,27 @@ static void btusb_work(struct work_struct *work)
> set_bit(BTUSB_DID_ISO_RESUME, &data->flags);
> }
>
> - if (hdev->voice_setting & 0x0020) {
> - static const int alts[3] = { 2, 4, 5 };
> -
> - new_alts = alts[data->sco_num - 1];
> - } else {
> - new_alts = data->sco_num;
> - }
> -
> - if (data->isoc_altsetting != new_alts) {
> - unsigned long flags;
> + if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_CVSD) {
> + if (hdev->voice_setting & 0x0020) {
> + static const int alts[3] = { 2, 4, 5 };
>
> - clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> - usb_kill_anchored_urbs(&data->isoc_anchor);
> -
> - /* When isochronous alternate setting needs to be
> - * changed, because SCO connection has been added
> - * or removed, a packet fragment may be left in the
> - * reassembling state. This could lead to wrongly
> - * assembled fragments.
> - *
> - * Clear outstanding fragment when selecting a new
> - * alternate setting.
> - */
> - spin_lock_irqsave(&data->rxlock, flags);
> - kfree_skb(data->sco_skb);
> - data->sco_skb = NULL;
> - spin_unlock_irqrestore(&data->rxlock, flags);
> + new_alts = alts[data->sco_num - 1];
> + } else {
> + new_alts = data->sco_num;
> + }
> + } else if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_TRANSP) {
>
> - if (__set_isoc_interface(hdev, new_alts) < 0)
> - return;
> - }
> + data->usb_alt6_packet_flow = true;
>
> - if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> - if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> - clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> + /* Check if Alt 6 is supported for Transparent audio*/
> + if (btusb_find_altsetting(data, 6))
> + new_alts = 6;
> else
> - btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> + BT_ERR("%s Device does not support ALT setting 6", hdev->name);
Please use bt_dev_err.
> }
> +
> + if (bt_switch_alt_setting(hdev, new_alts) < 0)
> + BT_ERR("%s Set USB Alt: %d failed!", hdev->name, new_alts);
Same here. bt_dev_err please.
> } else {
> clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> usb_kill_anchored_urbs(&data->isoc_anchor);
Regards
Marcel
Hi Marcel,
On Tue, Mar 31, 2020 at 4:08 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Sathish,
>
> > For msbc encoded audio stream over usb transport, btusb driver
> > to be set to alternate settings 6 as per BT core spec 5.0. The
> > type of air mode is used to differenting which alt setting to be
> > used.
> > The changes are made considering some discussion over the similar
> > patch submitted earlier from Kuba Pawlak(link below)
> > https://www.spinics.net/lists/linux-bluetooth/msg64577.html
> >
> > Reported-by: kbuild test robot <[email protected]>
> > Signed-off-by: Sathish Narasimman <[email protected]>
> > Signed-off-by: Chethan T N <[email protected]>
> > Signed-off-by: Hsin-Yu Chao <[email protected]>
> > Signed-off-by: Amit K Bag <[email protected]>
> > ---
> > drivers/bluetooth/btusb.c | 157 +++++++++++++++++++++++++++++---------
> > 1 file changed, 121 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index d9cd0677d41c..5563569f4473 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -483,6 +483,8 @@ struct btusb_data {
> > __u8 cmdreq;
> >
> > unsigned int sco_num;
> > + unsigned int air_mode;
> > + bool usb_alt6_packet_flow;
> > int isoc_altsetting;
> > int suspend_count;
> >
> > @@ -974,6 +976,42 @@ static void btusb_isoc_complete(struct urb *urb)
> > }
> > }
> >
> > +static inline void __fill_isoc_descriptor_msbc(struct urb *urb, int len,
> > + int mtu, struct btusb_data *data)
> > +{
> > + int i, offset = 0;
> > + unsigned int interval;
> > +
> > + /* For msbc ALT 6 setting the host will send the packet at continuous
> > + * flow. As per core spec 5, vol 4, part B, table 2.1. For ALT setting
> > + * 6 the HCI PACKET INTERVAL should be 7.5ms for every usb packets.
> > + * To maintain the rate we send 63bytes of usb packets alternatively for
> > + * 7ms and 8ms to maintain the rate as 7.5ms.
> > + */
> > + if (data->usb_alt6_packet_flow) {
> > + interval = 7;
> > + data->usb_alt6_packet_flow = false;
> > + } else {
> > + interval = 6;
> > + data->usb_alt6_packet_flow = true;
> > + }
> > +
> > + BT_DBG("interval:%d len %d mtu %d", interval, len, mtu);
>
> do you really want to keep this BT_DBG here?
>
Will move it to the beginning of fn
> > +
> > + for (i = 0; i < interval; i++) {
> > + urb->iso_frame_desc[i].offset = offset;
> > + urb->iso_frame_desc[i].length = offset;
> > + }
> > +
> > + if (len && i < BTUSB_MAX_ISOC_FRAMES) {
> > + urb->iso_frame_desc[i].offset = offset;
> > + urb->iso_frame_desc[i].length = len;
> > + i++;
> > + }
> > +
> > + urb->number_of_packets = i;
> > +}
> > +
> > static inline void __fill_isoc_descriptor(struct urb *urb, int len, int mtu)
> > {
> > int i, offset = 0;
> > @@ -1377,9 +1415,13 @@ static struct urb *alloc_isoc_urb(struct hci_dev *hdev, struct sk_buff *skb)
> >
> > urb->transfer_flags = URB_ISO_ASAP;
> >
> > - __fill_isoc_descriptor(urb, skb->len,
> > + if (data->isoc_altsetting == 6)
> > + __fill_isoc_descriptor_msbc(urb, skb->len,
> > + le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize),
> > + data);
> > + else
> > + __fill_isoc_descriptor(urb, skb->len,
> > le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
> > -
>
> Your changes are now breaking the coding style. Please fix them.
>
Will fix them in next version
> > skb->dev = (void *)hdev;
> >
> > return urb;
> > @@ -1475,6 +1517,7 @@ static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
> >
> > if (hci_conn_num(hdev, SCO_LINK) != data->sco_num) {
> > data->sco_num = hci_conn_num(hdev, SCO_LINK);
> > + data->air_mode = evt;
> > schedule_work(&data->work);
> > }
> > }
> > @@ -1522,11 +1565,70 @@ static inline int __set_isoc_interface(struct hci_dev *hdev, int altsetting)
> > return 0;
> > }
> >
> > +static int bt_switch_alt_setting(struct hci_dev *hdev, int new_alts)
> > +{
> > + struct btusb_data *data = hci_get_drvdata(hdev);
> > + int err;
> > +
> > + if (data->isoc_altsetting != new_alts) {
> > + unsigned long flags;
> > +
> > + clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > + usb_kill_anchored_urbs(&data->isoc_anchor);
> > +
> > + /* When isochronous alternate setting needs to be
> > + * changed, because SCO connection has been added
> > + * or removed, a packet fragment may be left in the
> > + * reassembling state. This could lead to wrongly
> > + * assembled fragments.
> > + *
> > + * Clear outstanding fragment when selecting a new
> > + * alternate setting.
> > + */
> > + spin_lock_irqsave(&data->rxlock, flags);
> > + kfree_skb(data->sco_skb);
> > + data->sco_skb = NULL;
> > + spin_unlock_irqrestore(&data->rxlock, flags);
> > +
> > + err = __set_isoc_interface(hdev, new_alts);
> > + if (err < 0)
> > + return err;
> > + }
> > + if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> > + if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> > + clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > + else
> > + btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct usb_host_interface *btusb_find_altsetting(struct btusb_data *data, int alt)
> > +{
> > + struct usb_interface *intf = data->isoc;
> > + int i;
> > +
> > + BT_DBG("Looking for Alt no :%d", alt);
> > +
> > + if (intf == NULL) {
> > + BT_ERR("INterface NULL");
>
> I would not put an error here. And we check if (!intf).
>
will remove the error check
> > + return NULL;
> > + }
> > +
> > + for (i = 0; i < intf->num_altsetting; i++) {
> > + if (intf->altsetting[i].desc.bAlternateSetting == alt)
> > + return &intf->altsetting[i];
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > static void btusb_work(struct work_struct *work)
> > {
> > struct btusb_data *data = container_of(work, struct btusb_data, work);
> > struct hci_dev *hdev = data->hdev;
> > - int new_alts;
> > + int new_alts = 0;
> > int err;
> >
> > if (data->sco_num > 0) {
> > @@ -1541,44 +1643,27 @@ static void btusb_work(struct work_struct *work)
> > set_bit(BTUSB_DID_ISO_RESUME, &data->flags);
> > }
> >
> > - if (hdev->voice_setting & 0x0020) {
> > - static const int alts[3] = { 2, 4, 5 };
> > -
> > - new_alts = alts[data->sco_num - 1];
> > - } else {
> > - new_alts = data->sco_num;
> > - }
> > -
> > - if (data->isoc_altsetting != new_alts) {
> > - unsigned long flags;
> > + if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_CVSD) {
> > + if (hdev->voice_setting & 0x0020) {
> > + static const int alts[3] = { 2, 4, 5 };
> >
> > - clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > - usb_kill_anchored_urbs(&data->isoc_anchor);
> > -
> > - /* When isochronous alternate setting needs to be
> > - * changed, because SCO connection has been added
> > - * or removed, a packet fragment may be left in the
> > - * reassembling state. This could lead to wrongly
> > - * assembled fragments.
> > - *
> > - * Clear outstanding fragment when selecting a new
> > - * alternate setting.
> > - */
> > - spin_lock_irqsave(&data->rxlock, flags);
> > - kfree_skb(data->sco_skb);
> > - data->sco_skb = NULL;
> > - spin_unlock_irqrestore(&data->rxlock, flags);
> > + new_alts = alts[data->sco_num - 1];
> > + } else {
> > + new_alts = data->sco_num;
> > + }
> > + } else if (data->air_mode == HCI_NOTIFY_ENABLE_SCO_TRANSP) {
> >
> > - if (__set_isoc_interface(hdev, new_alts) < 0)
> > - return;
> > - }
> > + data->usb_alt6_packet_flow = true;
> >
> > - if (!test_and_set_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> > - if (btusb_submit_isoc_urb(hdev, GFP_KERNEL) < 0)
> > - clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > + /* Check if Alt 6 is supported for Transparent audio*/
> > + if (btusb_find_altsetting(data, 6))
> > + new_alts = 6;
> > else
> > - btusb_submit_isoc_urb(hdev, GFP_KERNEL);
> > + BT_ERR("%s Device does not support ALT setting 6", hdev->name);
>
> Please use bt_dev_err.
>
will change it
> > }
> > +
> > + if (bt_switch_alt_setting(hdev, new_alts) < 0)
> > + BT_ERR("%s Set USB Alt: %d failed!", hdev->name, new_alts);
>
> Same here. bt_dev_err please.
>
> > } else {
> > clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > usb_kill_anchored_urbs(&data->isoc_anchor);
>
> Regards
>
> Marcel
>
Regards
Sathish N