diff mbox series

Bluetooth: ISO: fix timestamped HCI ISO data packet parsing

Message ID 1fd2d4523c139deda93aab2c31f1508d79c32472.1676921889.git.pav@iki.fi
State Accepted
Commit e16f2ec5cf3a500c5c082d4cad5bd8a234c86361
Headers show
Series Bluetooth: ISO: fix timestamped HCI ISO data packet parsing | expand

Commit Message

Pauli Virtanen Feb. 20, 2023, 7:38 p.m. UTC
Use correct HCI ISO data packet header struct when the packet has
timestamp. The timestamp, when present, goes before the other fields
(Core v5.3 4E 5.4.5), so the structs are not compatible.

Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    My hardware doesn't seem to produce timestamped packets, so this is not
    properly tested, except to the extent that it doesn't break the
    non-timestamped code path.
    
    Regardless, the current state of things looks wrong, so sending this to
    the list in any case.

 net/bluetooth/iso.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Pauli Virtanen Feb. 24, 2023, 7:52 p.m. UTC | #1
Hi Luiz,

ti, 2023-02-21 kello 14:08 -0800, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> > 
> > On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > 
> > > > Use correct HCI ISO data packet header struct when the packet has
> > > > timestamp. The timestamp, when present, goes before the other fields
> > > > (Core v5.3 4E 5.4.5), so the structs are not compatible.
> > > > 
> > > > Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > ---
> > > > 
> > > > Notes:
> > > >     My hardware doesn't seem to produce timestamped packets, so this is not
> > > >     properly tested, except to the extent that it doesn't break the
> > > >     non-timestamped code path.
> > > > 
> > > >     Regardless, the current state of things looks wrong, so sending this to
> > > >     the list in any case.
> > 
> > Perhaps we should first attempt to enable this in the emulator, Id use
> > BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
> > knowing the timestamp:
> > 
> > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752

Yes, it probably would be best, and probably not too complicated to do
at least if we are happy with not per spec timestamps.

> > > >  net/bluetooth/iso.c | 9 +++++++--
> > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > > index 2dabef488eaa..cb959e8eac18 100644
> > > > --- a/net/bluetooth/iso.c
> > > > +++ b/net/bluetooth/iso.c
> > > > @@ -1621,7 +1621,6 @@ static void iso_disconn_cfm(struct
> > > > hci_conn *hcon, __u8 reason)
> > > >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16
> > > > flags)
> > > >  {
> > > >         struct iso_conn *conn = hcon->iso_data;
> > > > -       struct hci_iso_data_hdr *hdr;
> > > >         __u16 pb, ts, len;
> > > > 
> > > >         if (!conn)
> > > > @@ -1643,6 +1642,8 @@ void iso_recv(struct hci_conn *hcon,
> > > > struct sk_buff *skb, u16 flags)
> > > >                 }
> > > > 
> > > >                 if (ts) {
> > > > +                       struct hci_iso_ts_data_hdr *hdr;
> > > > +
> > > >                         /* TODO: add timestamp to the packet?
> > > > */
> > 
> > Perhaps we should use skb_set_delivery_time to set the timestamp as
> > received by the controller? That said I don't know if the unit
> > would
> > be compatible.

The userspace I think is interested also in the sequence number and
packet status flags (if controller knows data is corrupted, that should
be indicated to userspace, similarly for missed packets). Would e.g.
putting all these into some custom cmsg struct be better here?

This timestamp is also based on controller clock. Not sure if
delivery_time is supposed to be in system clock not some unrelated
hardware clock.

Userspace may also want to set the packet sequence numbers and
timestamps itself when sending, to indicate in which interval it wanted
things to come out.

On systems where synchronization to Controller ISO clock is not
available (it IIUC cannot really be done via HCI and so this is likely
the situation on desktop/laptop machines), userspace needs some other
tool to keep its SDU timing in sync with the controller, and for e.g.
keeping a fixed number of packets queued to have some buffer for missed
deadlines, and making sure there is no timing offset between eg. left
and right earpieces of TWS headset.

This probably means things like information on the timing of the latest
Number of Completed Packets event for the handle, the number of
currently queued ISO packets on the controller, and the sequence number
of the latest packet queued on controller (to know how much is still in
socket buffer). Probably also LE Read TX Sync information to know
controller timestamp on a sent packet...

Does this go via some new ioctls? Or send empty packets with some cmsg
(so userspace can just poll until it needs to do something)?

It should also be decided how much the kernel is going to be involved
in getting the SDU timing, sequence numbers, etc. right, or whether
it's just left to the userspace.

Before trying to propose something here, I think I'd need to first make
some prototypes so that it becomes clearer how it would best work.


> > > >                         hdr = skb_pull_data(skb,
> > > > HCI_ISO_TS_DATA_HDR_SIZE);
> > > >                         if (!hdr) {
> > > > @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon,
> > > > struct sk_buff *skb, u16 flags)
> > > >                                 goto drop;
> > > >                         }
> > > > 
> > > > +                       len = __le16_to_cpu(hdr->slen);
> > > >                 } else {
> > > > +                       struct hci_iso_data_hdr *hdr;
> > > > +
> > > >                         hdr = skb_pull_data(skb,
> > > > HCI_ISO_DATA_HDR_SIZE);
> > > >                         if (!hdr) {
> > > >                                 BT_ERR("Frame is too short (len
> > > > %d)", skb->len);
> > > >                                 goto drop;
> > > >                         }
> > > > +
> > > > +                       len = __le16_to_cpu(hdr->slen);
> > > >                 }
> > > > 
> > > > -               len    = __le16_to_cpu(hdr->slen);
> > > >                 flags  = hci_iso_data_flags(len);
> > > >                 len    = hci_iso_data_len(len);
> > > > 
> > > > --
> > > > 2.39.2
> > > > 
> > 
> >
Luiz Augusto von Dentz Feb. 24, 2023, 8:41 p.m. UTC | #2
Hi Pauli,

On Fri, Feb 24, 2023 at 11:52 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> ti, 2023-02-21 kello 14:08 -0800, Luiz Augusto von Dentz kirjoitti:
> > > Hi Pauli,
> > >
> > > On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > >
> > > > > Use correct HCI ISO data packet header struct when the packet has
> > > > > timestamp. The timestamp, when present, goes before the other fields
> > > > > (Core v5.3 4E 5.4.5), so the structs are not compatible.
> > > > >
> > > > > Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > >     My hardware doesn't seem to produce timestamped packets, so this is not
> > > > >     properly tested, except to the extent that it doesn't break the
> > > > >     non-timestamped code path.
> > > > >
> > > > >     Regardless, the current state of things looks wrong, so sending this to
> > > > >     the list in any case.
> > >
> > > Perhaps we should first attempt to enable this in the emulator, Id use
> > > BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
> > > knowing the timestamp:
> > >
> > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752
>
> Yes, it probably would be best, and probably not too complicated to do
> at least if we are happy with not per spec timestamps.
>
> > > > >  net/bluetooth/iso.c | 9 +++++++--
> > > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > > > index 2dabef488eaa..cb959e8eac18 100644
> > > > > --- a/net/bluetooth/iso.c
> > > > > +++ b/net/bluetooth/iso.c
> > > > > @@ -1621,7 +1621,6 @@ static void iso_disconn_cfm(struct
> > > > > hci_conn *hcon, __u8 reason)
> > > > >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16
> > > > > flags)
> > > > >  {
> > > > >         struct iso_conn *conn = hcon->iso_data;
> > > > > -       struct hci_iso_data_hdr *hdr;
> > > > >         __u16 pb, ts, len;
> > > > >
> > > > >         if (!conn)
> > > > > @@ -1643,6 +1642,8 @@ void iso_recv(struct hci_conn *hcon,
> > > > > struct sk_buff *skb, u16 flags)
> > > > >                 }
> > > > >
> > > > >                 if (ts) {
> > > > > +                       struct hci_iso_ts_data_hdr *hdr;
> > > > > +
> > > > >                         /* TODO: add timestamp to the packet?
> > > > > */
> > >
> > > Perhaps we should use skb_set_delivery_time to set the timestamp as
> > > received by the controller? That said I don't know if the unit
> > > would
> > > be compatible.
>
> The userspace I think is interested also in the sequence number and
> packet status flags (if controller knows data is corrupted, that should
> be indicated to userspace, similarly for missed packets). Would e.g.
> putting all these into some custom cmsg struct be better here?
>
> This timestamp is also based on controller clock. Not sure if
> delivery_time is supposed to be in system clock not some unrelated
> hardware clock.
>
> Userspace may also want to set the packet sequence numbers and
> timestamps itself when sending, to indicate in which interval it wanted
> things to come out.
>
> On systems where synchronization to Controller ISO clock is not
> available (it IIUC cannot really be done via HCI and so this is likely
> the situation on desktop/laptop machines), userspace needs some other
> tool to keep its SDU timing in sync with the controller, and for e.g.
> keeping a fixed number of packets queued to have some buffer for missed
> deadlines, and making sure there is no timing offset between eg. left
> and right earpieces of TWS headset.

Yeah, btw I will be posting the patches for CSIP soon and I wonder if
you could check with this headset of yours, in fact we probably need
some changes to the likes of pipewire to detect the presence of
DeviceSet object and split the left and right streams.

> This probably means things like information on the timing of the latest
> Number of Completed Packets event for the handle, the number of
> currently queued ISO packets on the controller, and the sequence number
> of the latest packet queued on controller (to know how much is still in
> socket buffer). Probably also LE Read TX Sync information to know
> controller timestamp on a sent packet...
>
> Does this go via some new ioctls? Or send empty packets with some cmsg
> (so userspace can just poll until it needs to do something)?
>
> It should also be decided how much the kernel is going to be involved
> in getting the SDU timing, sequence numbers, etc. right, or whether
> it's just left to the userspace.
>
> Before trying to propose something here, I think I'd need to first make
> some prototypes so that it becomes clearer how it would best work.

I think we might want to use BT_PKT_STATUS, like is done for SCO, to
carry information such as controller timestamp, etc, have a look at
it:

https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/sco.c#n450

>
> > > > >                         hdr = skb_pull_data(skb,
> > > > > HCI_ISO_TS_DATA_HDR_SIZE);
> > > > >                         if (!hdr) {
> > > > > @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon,
> > > > > struct sk_buff *skb, u16 flags)
> > > > >                                 goto drop;
> > > > >                         }
> > > > >
> > > > > +                       len = __le16_to_cpu(hdr->slen);
> > > > >                 } else {
> > > > > +                       struct hci_iso_data_hdr *hdr;
> > > > > +
> > > > >                         hdr = skb_pull_data(skb,
> > > > > HCI_ISO_DATA_HDR_SIZE);
> > > > >                         if (!hdr) {
> > > > >                                 BT_ERR("Frame is too short (len
> > > > > %d)", skb->len);
> > > > >                                 goto drop;
> > > > >                         }
> > > > > +
> > > > > +                       len = __le16_to_cpu(hdr->slen);
> > > > >                 }
> > > > >
> > > > > -               len    = __le16_to_cpu(hdr->slen);
> > > > >                 flags  = hci_iso_data_flags(len);
> > > > >                 len    = hci_iso_data_len(len);
> > > > >
> > > > > --
> > > > > 2.39.2
> > > > >
> > >
> > >
>
>
Pauli Virtanen Feb. 24, 2023, 10:55 p.m. UTC | #3
Hi Luiz,

pe, 2023-02-24 kello 12:41 -0800, Luiz Augusto von Dentz kirjoitti:
> On Fri, Feb 24, 2023 at 11:52 AM Pauli Virtanen <pav@iki.fi> wrote:
> > ti, 2023-02-21 kello 14:08 -0800, Luiz Augusto von Dentz kirjoitti:
> > > > Hi Pauli,
> > > > 
> > > > On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > > > 
> > > > > > Use correct HCI ISO data packet header struct when the packet has
> > > > > > timestamp. The timestamp, when present, goes before the other fields
> > > > > > (Core v5.3 4E 5.4.5), so the structs are not compatible.
> > > > > > 
> > > > > > Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > > > ---
> > > > > > 
> > > > > > Notes:
> > > > > >     My hardware doesn't seem to produce timestamped packets, so this is not
> > > > > >     properly tested, except to the extent that it doesn't break the
> > > > > >     non-timestamped code path.
> > > > > > 
> > > > > >     Regardless, the current state of things looks wrong, so sending this to
> > > > > >     the list in any case.
> > > > 
> > > > Perhaps we should first attempt to enable this in the emulator, Id use
> > > > BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
> > > > knowing the timestamp:
> > > > 
> > > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752
> > 
> > Yes, it probably would be best, and probably not too complicated to do
> > at least if we are happy with not per spec timestamps.
> > 
> > > > > >  net/bluetooth/iso.c | 9 +++++++--
> > > > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > > > > index 2dabef488eaa..cb959e8eac18 100644
> > > > > > --- a/net/bluetooth/iso.c
> > > > > > +++ b/net/bluetooth/iso.c
> > > > > > @@ -1621,7 +1621,6 @@ static void iso_disconn_cfm(struct
> > > > > > hci_conn *hcon, __u8 reason)
> > > > > >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16
> > > > > > flags)
> > > > > >  {
> > > > > >         struct iso_conn *conn = hcon->iso_data;
> > > > > > -       struct hci_iso_data_hdr *hdr;
> > > > > >         __u16 pb, ts, len;
> > > > > > 
> > > > > >         if (!conn)
> > > > > > @@ -1643,6 +1642,8 @@ void iso_recv(struct hci_conn *hcon,
> > > > > > struct sk_buff *skb, u16 flags)
> > > > > >                 }
> > > > > > 
> > > > > >                 if (ts) {
> > > > > > +                       struct hci_iso_ts_data_hdr *hdr;
> > > > > > +
> > > > > >                         /* TODO: add timestamp to the packet?
> > > > > > */
> > > > 
> > > > Perhaps we should use skb_set_delivery_time to set the timestamp as
> > > > received by the controller? That said I don't know if the unit
> > > > would
> > > > be compatible.
> > 
> > The userspace I think is interested also in the sequence number and
> > packet status flags (if controller knows data is corrupted, that should
> > be indicated to userspace, similarly for missed packets). Would e.g.
> > putting all these into some custom cmsg struct be better here?
> > 
> > This timestamp is also based on controller clock. Not sure if
> > delivery_time is supposed to be in system clock not some unrelated
> > hardware clock.
> > 
> > Userspace may also want to set the packet sequence numbers and
> > timestamps itself when sending, to indicate in which interval it wanted
> > things to come out.
> > 
> > On systems where synchronization to Controller ISO clock is not
> > available (it IIUC cannot really be done via HCI and so this is likely
> > the situation on desktop/laptop machines), userspace needs some other
> > tool to keep its SDU timing in sync with the controller, and for e.g.
> > keeping a fixed number of packets queued to have some buffer for missed
> > deadlines, and making sure there is no timing offset between eg. left
> > and right earpieces of TWS headset.
> 
> Yeah, btw I will be posting the patches for CSIP soon and I wonder if
> you could check with this headset of yours, in fact we probably need
> some changes to the likes of pipewire to detect the presence of
> DeviceSet object and split the left and right streams.

I'll test the CSIP patchset with the Samsung TWS headset I have. I'm
also going to make the necessary changes to Pipewire, to see how it
works from our side.

FWIW, right now, there are issues in getting reliable BAP audio on this
TWS headset device (with one earbud only currently) --- I'm sending
unframed LC3 packets to the ISO socket with sub-ms timing accuracy (err
~ 0.2ms max, 50us std) and there are still frequent pops in sine wave.
Stuffing extra 2-4 packets to the socket at the start of playback seems
to reduce the problem, so maybe the tx sync issue is suspect.

However, no such problem with nrf5430 or ax210+bluez+pipewire as
receiver, so not really yet clear.

> > This probably means things like information on the timing of the latest
> > Number of Completed Packets event for the handle, the number of
> > currently queued ISO packets on the controller, and the sequence number
> > of the latest packet queued on controller (to know how much is still in
> > socket buffer). Probably also LE Read TX Sync information to know
> > controller timestamp on a sent packet...
> > 
> > Does this go via some new ioctls? Or send empty packets with some cmsg
> > (so userspace can just poll until it needs to do something)?
> > 
> > It should also be decided how much the kernel is going to be involved
> > in getting the SDU timing, sequence numbers, etc. right, or whether
> > it's just left to the userspace.
> > 
> > Before trying to propose something here, I think I'd need to first make
> > some prototypes so that it becomes clearer how it would best work.
> 
> I think we might want to use BT_PKT_STATUS, like is done for SCO, to
> carry information such as controller timestamp, etc, have a look at
> it:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/sco.c#n450


Yes, for RX that's probably what it should be.

> > 
> > > > > >                         hdr = skb_pull_data(skb,
> > > > > > HCI_ISO_TS_DATA_HDR_SIZE);
> > > > > >                         if (!hdr) {
> > > > > > @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon,
> > > > > > struct sk_buff *skb, u16 flags)
> > > > > >                                 goto drop;
> > > > > >                         }
> > > > > > 
> > > > > > +                       len = __le16_to_cpu(hdr->slen);
> > > > > >                 } else {
> > > > > > +                       struct hci_iso_data_hdr *hdr;
> > > > > > +
> > > > > >                         hdr = skb_pull_data(skb,
> > > > > > HCI_ISO_DATA_HDR_SIZE);
> > > > > >                         if (!hdr) {
> > > > > >                                 BT_ERR("Frame is too short (len
> > > > > > %d)", skb->len);
> > > > > >                                 goto drop;
> > > > > >                         }
> > > > > > +
> > > > > > +                       len = __le16_to_cpu(hdr->slen);
> > > > > >                 }
> > > > > > 
> > > > > > -               len    = __le16_to_cpu(hdr->slen);
> > > > > >                 flags  = hci_iso_data_flags(len);
> > > > > >                 len    = hci_iso_data_len(len);
> > > > > > 
> > > > > > --
> > > > > > 2.39.2
> > > > > > 
> > > > 
> > > > 
> > 
> > 
> 
>
patchwork-bot+bluetooth@kernel.org Feb. 27, 2023, 9:40 p.m. UTC | #4
Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon, 20 Feb 2023 19:38:24 +0000 you wrote:
> Use correct HCI ISO data packet header struct when the packet has
> timestamp. The timestamp, when present, goes before the other fields
> (Core v5.3 4E 5.4.5), so the structs are not compatible.
> 
> Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> 
> [...]

Here is the summary with links:
  - Bluetooth: ISO: fix timestamped HCI ISO data packet parsing
    https://git.kernel.org/bluetooth/bluetooth-next/c/e16f2ec5cf3a

You are awesome, thank you!
Luiz Augusto von Dentz March 3, 2023, 5:33 a.m. UTC | #5
Hi Pauli,

On Fri, Feb 24, 2023 at 2:55 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> pe, 2023-02-24 kello 12:41 -0800, Luiz Augusto von Dentz kirjoitti:
> > On Fri, Feb 24, 2023 at 11:52 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > ti, 2023-02-21 kello 14:08 -0800, Luiz Augusto von Dentz kirjoitti:
> > > > > Hi Pauli,
> > > > >
> > > > > On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
> > > > > > >
> > > > > > > Use correct HCI ISO data packet header struct when the packet has
> > > > > > > timestamp. The timestamp, when present, goes before the other fields
> > > > > > > (Core v5.3 4E 5.4.5), so the structs are not compatible.
> > > > > > >
> > > > > > > Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> > > > > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > > > > ---
> > > > > > >
> > > > > > > Notes:
> > > > > > >     My hardware doesn't seem to produce timestamped packets, so this is not
> > > > > > >     properly tested, except to the extent that it doesn't break the
> > > > > > >     non-timestamped code path.
> > > > > > >
> > > > > > >     Regardless, the current state of things looks wrong, so sending this to
> > > > > > >     the list in any case.
> > > > >
> > > > > Perhaps we should first attempt to enable this in the emulator, Id use
> > > > > BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
> > > > > knowing the timestamp:
> > > > >
> > > > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752
> > >
> > > Yes, it probably would be best, and probably not too complicated to do
> > > at least if we are happy with not per spec timestamps.
> > >
> > > > > > >  net/bluetooth/iso.c | 9 +++++++--
> > > > > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > > > > > index 2dabef488eaa..cb959e8eac18 100644
> > > > > > > --- a/net/bluetooth/iso.c
> > > > > > > +++ b/net/bluetooth/iso.c
> > > > > > > @@ -1621,7 +1621,6 @@ static void iso_disconn_cfm(struct
> > > > > > > hci_conn *hcon, __u8 reason)
> > > > > > >  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16
> > > > > > > flags)
> > > > > > >  {
> > > > > > >         struct iso_conn *conn = hcon->iso_data;
> > > > > > > -       struct hci_iso_data_hdr *hdr;
> > > > > > >         __u16 pb, ts, len;
> > > > > > >
> > > > > > >         if (!conn)
> > > > > > > @@ -1643,6 +1642,8 @@ void iso_recv(struct hci_conn *hcon,
> > > > > > > struct sk_buff *skb, u16 flags)
> > > > > > >                 }
> > > > > > >
> > > > > > >                 if (ts) {
> > > > > > > +                       struct hci_iso_ts_data_hdr *hdr;
> > > > > > > +
> > > > > > >                         /* TODO: add timestamp to the packet?
> > > > > > > */
> > > > >
> > > > > Perhaps we should use skb_set_delivery_time to set the timestamp as
> > > > > received by the controller? That said I don't know if the unit
> > > > > would
> > > > > be compatible.
> > >
> > > The userspace I think is interested also in the sequence number and
> > > packet status flags (if controller knows data is corrupted, that should
> > > be indicated to userspace, similarly for missed packets). Would e.g.
> > > putting all these into some custom cmsg struct be better here?
> > >
> > > This timestamp is also based on controller clock. Not sure if
> > > delivery_time is supposed to be in system clock not some unrelated
> > > hardware clock.
> > >
> > > Userspace may also want to set the packet sequence numbers and
> > > timestamps itself when sending, to indicate in which interval it wanted
> > > things to come out.
> > >
> > > On systems where synchronization to Controller ISO clock is not
> > > available (it IIUC cannot really be done via HCI and so this is likely
> > > the situation on desktop/laptop machines), userspace needs some other
> > > tool to keep its SDU timing in sync with the controller, and for e.g.
> > > keeping a fixed number of packets queued to have some buffer for missed
> > > deadlines, and making sure there is no timing offset between eg. left
> > > and right earpieces of TWS headset.
> >
> > Yeah, btw I will be posting the patches for CSIP soon and I wonder if
> > you could check with this headset of yours, in fact we probably need
> > some changes to the likes of pipewire to detect the presence of
> > DeviceSet object and split the left and right streams.
>
> I'll test the CSIP patchset with the Samsung TWS headset I have. I'm
> also going to make the necessary changes to Pipewire, to see how it
> works from our side.

I just posted an RFC set:

https://patchwork.kernel.org/project/bluetooth/list/?series=726293

Make sure you also have the set bellow, apparently this was the cause
of transport not showing up since bap_ready callback was called to
early before we had subscribed for notifications, etc:

https://patchwork.kernel.org/project/bluetooth/list/?series=726232

> FWIW, right now, there are issues in getting reliable BAP audio on this
> TWS headset device (with one earbud only currently) --- I'm sending
> unframed LC3 packets to the ISO socket with sub-ms timing accuracy (err
> ~ 0.2ms max, 50us std) and there are still frequent pops in sine wave.
> Stuffing extra 2-4 packets to the socket at the start of playback seems
> to reduce the problem, so maybe the tx sync issue is suspect.

Yeah, Im afraid the headset might be trying to minimize using a jitter
buffer so if we miss any internal it will cause such problem, the
other problem is that the controller may not be reporting the Number
of Packets Complete one by one, which cause us to send packets in
batches which end up removing the timing accuracy of the socket since
we have to wait the controller to acknowledge the packets previously
sent, perhaps we could try detecting this behavior and disable Number
of Packets Complete flow control, so we send based on the configured
interval no matter if we think there is a buffer available in the
controller or not.

> However, no such problem with nrf5430 or ax210+bluez+pipewire as
> receiver, so not really yet clear.
>
> > > This probably means things like information on the timing of the latest
> > > Number of Completed Packets event for the handle, the number of
> > > currently queued ISO packets on the controller, and the sequence number
> > > of the latest packet queued on controller (to know how much is still in
> > > socket buffer). Probably also LE Read TX Sync information to know
> > > controller timestamp on a sent packet...
> > >
> > > Does this go via some new ioctls? Or send empty packets with some cmsg
> > > (so userspace can just poll until it needs to do something)?
> > >
> > > It should also be decided how much the kernel is going to be involved
> > > in getting the SDU timing, sequence numbers, etc. right, or whether
> > > it's just left to the userspace.
> > >
> > > Before trying to propose something here, I think I'd need to first make
> > > some prototypes so that it becomes clearer how it would best work.
> >
> > I think we might want to use BT_PKT_STATUS, like is done for SCO, to
> > carry information such as controller timestamp, etc, have a look at
> > it:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/sco.c#n450
>
>
> Yes, for RX that's probably what it should be.
>
> > >
> > > > > > >                         hdr = skb_pull_data(skb,
> > > > > > > HCI_ISO_TS_DATA_HDR_SIZE);
> > > > > > >                         if (!hdr) {
> > > > > > > @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon,
> > > > > > > struct sk_buff *skb, u16 flags)
> > > > > > >                                 goto drop;
> > > > > > >                         }
> > > > > > >
> > > > > > > +                       len = __le16_to_cpu(hdr->slen);
> > > > > > >                 } else {
> > > > > > > +                       struct hci_iso_data_hdr *hdr;
> > > > > > > +
> > > > > > >                         hdr = skb_pull_data(skb,
> > > > > > > HCI_ISO_DATA_HDR_SIZE);
> > > > > > >                         if (!hdr) {
> > > > > > >                                 BT_ERR("Frame is too short (len
> > > > > > > %d)", skb->len);
> > > > > > >                                 goto drop;
> > > > > > >                         }
> > > > > > > +
> > > > > > > +                       len = __le16_to_cpu(hdr->slen);
> > > > > > >                 }
> > > > > > >
> > > > > > > -               len    = __le16_to_cpu(hdr->slen);
> > > > > > >                 flags  = hci_iso_data_flags(len);
> > > > > > >                 len    = hci_iso_data_len(len);
> > > > > > >
> > > > > > > --
> > > > > > > 2.39.2
> > > > > > >
> > > > >
> > > > >
> > >
> > >
> >
> >
>
> --
> Pauli Virtanen
diff mbox series

Patch

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 2dabef488eaa..cb959e8eac18 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1621,7 +1621,6 @@  static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
 void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 {
 	struct iso_conn *conn = hcon->iso_data;
-	struct hci_iso_data_hdr *hdr;
 	__u16 pb, ts, len;
 
 	if (!conn)
@@ -1643,6 +1642,8 @@  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 		}
 
 		if (ts) {
+			struct hci_iso_ts_data_hdr *hdr;
+
 			/* TODO: add timestamp to the packet? */
 			hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
 			if (!hdr) {
@@ -1650,15 +1651,19 @@  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 				goto drop;
 			}
 
+			len = __le16_to_cpu(hdr->slen);
 		} else {
+			struct hci_iso_data_hdr *hdr;
+
 			hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
 			if (!hdr) {
 				BT_ERR("Frame is too short (len %d)", skb->len);
 				goto drop;
 			}
+
+			len = __le16_to_cpu(hdr->slen);
 		}
 
-		len    = __le16_to_cpu(hdr->slen);
 		flags  = hci_iso_data_flags(len);
 		len    = hci_iso_data_len(len);