[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-bluetooth
Subject: Re: [PATCH v3] Bluetooth: btusb: hci_event: handle msbc audio over USB Endpoints
From: Marcel Holtmann <marcel () holtmann ! org>
Date: 2019-11-28 15:10:18
Message-ID: 75C3AF78-F174-4BA2-A73B-7465C175ED38 () holtmann ! org
[Download RAW message or body]
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. This
> > > done from hci_sync_conn_complete_evt. The type of air mode is known
> > > during this event. For this reason the btusb is to be notifed
> > > about the TRANSPARENT air mode and the ALT setting 6 is selected.
> > > 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
> > >
> > > (am from https://www.spinics.net/lists/linux-bluetooth/msg76982.html)
> > >
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > > Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
> > > Signed-off-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > > Signed-off-by: Hsin-Yu Chao <hychao@chromium.org>
> > > Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> > > ---
> > > drivers/bluetooth/btusb.c | 157 ++++++++++++++++++++++++-------
> > > include/net/bluetooth/hci.h | 7 +-
> > > include/net/bluetooth/hci_core.h | 3 +
> > > net/bluetooth/hci_conn.c | 2 -
> > > net/bluetooth/hci_event.c | 9 ++
> > > 5 files changed, 137 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index 04a139e7793f..7a4260757822 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, bool packet_flow)
> > > +{
> > > + 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 (packet_flow) {
> > > + interval = 7;
> > > + packet_flow = false;
> > > + } else {
> > > + interval = 6;
> > > + 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;
> > > @@ -1376,9 +1414,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->usb_alt6_packet_flow);
> > > + else
> > > + __fill_isoc_descriptor(urb, skb->len,
> > > le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
> > > -
> > > skb->dev = (void *)hdev;
> > >
> > > return urb;
> > > @@ -1474,6 +1516,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);
> > > }
> > > }
> > > @@ -1521,11 +1564,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) {
> > > @@ -1540,44 +1642,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);
> > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > > index 5bc1e30dedde..8183394c2cc0 100644
> > > --- a/include/net/bluetooth/hci.h
> > > +++ b/include/net/bluetooth/hci.h
> > > @@ -49,9 +49,10 @@
> > > #define HCI_DEV_SETUP 9
> > >
> > > /* HCI notify events */
> > > -#define HCI_NOTIFY_CONN_ADD 1
> > > -#define HCI_NOTIFY_CONN_DEL 2
> > > -#define HCI_NOTIFY_VOICE_SETTING 3
> > > +#define HCI_NOTIFY_ENABLE_SCO_CVSD 1
> > > +#define HCI_NOTIFY_ENABLE_SCO_TRANSP 2
> > > +#define HCI_NOTIFY_CONN_DEL 3
> > > +#define HCI_NOTIFY_VOICE_SETTING 4
> >
> > hmmm. Can we keep CONN_ADD and CONN_DEL for the ACL and LE links. And then \
> > introduce ENABLE_SCO_CVSD, ENABLE_SCO_TRANSP and DISABLE_SCO. Right now this \
> > patch creates an imbalance.
> we have HCI_NOTIFY_CONN_DEL what is the reason the use DISABLE_SCO.
> where we need to notify this. Do I miss something?
as I said earlier, it is best to use CONN_ADD and CONN_DEL just for ACL/LE links. And \
use a separate ENABLE_SCO_* and DISABLE_SCO. Lets make a clean split between SCO and \
ACL/LE links.
Regards
Marcel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic