| Message ID | cover.1709150574.git.pav@iki.fi (mailing list archive) |
|---|---|
| Headers | show |
| Series | Bluetooth: add transmission latency tracking for ISO & L2CAP | expand |
Hi Pauli, On Wed, Feb 28, 2024 at 3:11 PM Pauli Virtanen <pav@iki.fi> wrote: > > Add ISO/L2CAP socket ioctl() BTGETTXINFO which informs the user how long > it took the kernel and controller to complete their sendmsg(), and how > many sendmsg() are in socket and controller queues. > > This currently provides information of the latest packet only, in > principle there could be a ringbuffer containing few latest packets, not > clear if that would be useful. > > These patches allow fixing / working around controller(?) issue where > two ISO streams in same group get desynchronized. Having accurate > knowledge of the packet queue lengths, user application can drop packets > if it detects the ISO streams are not in sync. > > Pipewire side: > https://gitlab.freedesktop.org/pvir/pipewire/-/commits/iso-ts-test > > With this change, https://github.com/bluez/bluez/issues/515 is more or > less fixed, and the sound server can figure out the total latency to > audio rendering (tx latency + transport latency + presentation delay). > > For ISO, this can be changed to use LE Read ISO TX Sync, when the clock > and sequence number synchronization issues there are figured out, and a > quirk is added for controllers with nonfunctional implementation. > > For the L2CAP latency, I'll need to think a bit more what is the audio > use case. Motivation was that AVDTP delay report values appear to be off > by ~0..40 ms compared to observed audio latency and this amount can vary > per connection and time, so not explained by unaccounted code > algorithmic delays etc. Currently it's not clear if there is relation to > TX side latency, so it may be down to receiver side implementation. > > This needs a bit more work to figure out, but the L2CAP patch is anyway > here. Due to the possible fragmentation in ISO sendmsg(), it seems we > anyway need the tx_info_queue thing and can't easily do it by counting > packets, and L2CAP required part is small addition on top of that. That is not how it is normally done with sockets, normally this is done with use of SO_TIMESTAMPING which is then reported using the socket error queue: https://www.kernel.org/doc/html/latest/networking/timestamping.html Sorry to tell you just now, you might have lost a lot of time doing all of the changes, next time just drop and RFC early on with the general design so you don't spend too much time before getting any feedback. > TBD: iso-tester / l2cap-tester tests > > Pauli Virtanen (3): > Bluetooth: add transmission latency tracking for ISO and ACL > Bluetooth: ISO: add new ioctl() for reading tx latency > Bluetooth: L2CAP: add new ioctl() for reading tx latency > > include/net/bluetooth/bluetooth.h | 39 +++++++++++ > include/net/bluetooth/hci_core.h | 30 ++++++++ > net/bluetooth/hci_conn.c | 110 +++++++++++++++++++++++++++++- > net/bluetooth/hci_core.c | 14 ++++ > net/bluetooth/hci_event.c | 66 ++++++++++++++++++ > net/bluetooth/iso.c | 58 ++++++++++++++-- > net/bluetooth/l2cap_core.c | 12 ++++ > net/bluetooth/l2cap_sock.c | 50 +++++++++++++- > 8 files changed, 372 insertions(+), 7 deletions(-) > > -- > 2.44.0 > >
Hi Pauli, On Wed, Feb 28, 2024 at 3:31 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Pauli, > > On Wed, Feb 28, 2024 at 3:11 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > Add ISO/L2CAP socket ioctl() BTGETTXINFO which informs the user how long > > it took the kernel and controller to complete their sendmsg(), and how > > many sendmsg() are in socket and controller queues. > > > > This currently provides information of the latest packet only, in > > principle there could be a ringbuffer containing few latest packets, not > > clear if that would be useful. > > > > These patches allow fixing / working around controller(?) issue where > > two ISO streams in same group get desynchronized. Having accurate > > knowledge of the packet queue lengths, user application can drop packets > > if it detects the ISO streams are not in sync. > > > > Pipewire side: > > https://gitlab.freedesktop.org/pvir/pipewire/-/commits/iso-ts-test > > > > With this change, https://github.com/bluez/bluez/issues/515 is more or > > less fixed, and the sound server can figure out the total latency to > > audio rendering (tx latency + transport latency + presentation delay). > > > > For ISO, this can be changed to use LE Read ISO TX Sync, when the clock > > and sequence number synchronization issues there are figured out, and a > > quirk is added for controllers with nonfunctional implementation. > > > > For the L2CAP latency, I'll need to think a bit more what is the audio > > use case. Motivation was that AVDTP delay report values appear to be off > > by ~0..40 ms compared to observed audio latency and this amount can vary > > per connection and time, so not explained by unaccounted code > > algorithmic delays etc. Currently it's not clear if there is relation to > > TX side latency, so it may be down to receiver side implementation. > > > > This needs a bit more work to figure out, but the L2CAP patch is anyway > > here. Due to the possible fragmentation in ISO sendmsg(), it seems we > > anyway need the tx_info_queue thing and can't easily do it by counting > > packets, and L2CAP required part is small addition on top of that. > > That is not how it is normally done with sockets, normally this is > done with use of SO_TIMESTAMPING which is then reported using the > socket error queue: > > https://www.kernel.org/doc/html/latest/networking/timestamping.html > > Sorry to tell you just now, you might have lost a lot of time doing > all of the changes, next time just drop and RFC early on with the > general design so you don't spend too much time before getting any > feedback. There are some samples in the linux tree if you want to take a look: https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/+/refs/tags/v4.6/Documentation/networking/timestamping/txtimestamp.c The nice thing with that is that it is not a new API and it has a little bit more finer control since we can add support for the likes of SCM_TSTAMP_SCHED, SCM_TSTAMP_SND and SCM_TSTAMP_ACK, so we have the possible to instead the timing at different layers. > > TBD: iso-tester / l2cap-tester tests > > > > Pauli Virtanen (3): > > Bluetooth: add transmission latency tracking for ISO and ACL > > Bluetooth: ISO: add new ioctl() for reading tx latency > > Bluetooth: L2CAP: add new ioctl() for reading tx latency > > > > include/net/bluetooth/bluetooth.h | 39 +++++++++++ > > include/net/bluetooth/hci_core.h | 30 ++++++++ > > net/bluetooth/hci_conn.c | 110 +++++++++++++++++++++++++++++- > > net/bluetooth/hci_core.c | 14 ++++ > > net/bluetooth/hci_event.c | 66 ++++++++++++++++++ > > net/bluetooth/iso.c | 58 ++++++++++++++-- > > net/bluetooth/l2cap_core.c | 12 ++++ > > net/bluetooth/l2cap_sock.c | 50 +++++++++++++- > > 8 files changed, 372 insertions(+), 7 deletions(-) > > > > -- > > 2.44.0 > > > > > > > -- > Luiz Augusto von Dentz
Hi Luiz, ke, 2024-02-28 kello 15:31 -0500, Luiz Augusto von Dentz kirjoitti: > Hi Pauli, > > On Wed, Feb 28, 2024 at 3:11 PM Pauli Virtanen <pav@iki.fi> wrote: > > > > Add ISO/L2CAP socket ioctl() BTGETTXINFO which informs the user how long > > it took the kernel and controller to complete their sendmsg(), and how > > many sendmsg() are in socket and controller queues. > > > > This currently provides information of the latest packet only, in > > principle there could be a ringbuffer containing few latest packets, not > > clear if that would be useful. > > > > These patches allow fixing / working around controller(?) issue where > > two ISO streams in same group get desynchronized. Having accurate > > knowledge of the packet queue lengths, user application can drop packets > > if it detects the ISO streams are not in sync. > > > > Pipewire side: > > https://gitlab.freedesktop.org/pvir/pipewire/-/commits/iso-ts-test > > > > With this change, https://github.com/bluez/bluez/issues/515 is more or > > less fixed, and the sound server can figure out the total latency to > > audio rendering (tx latency + transport latency + presentation delay). > > > > For ISO, this can be changed to use LE Read ISO TX Sync, when the clock > > and sequence number synchronization issues there are figured out, and a > > quirk is added for controllers with nonfunctional implementation. > > > > For the L2CAP latency, I'll need to think a bit more what is the audio > > use case. Motivation was that AVDTP delay report values appear to be off > > by ~0..40 ms compared to observed audio latency and this amount can vary > > per connection and time, so not explained by unaccounted code > > algorithmic delays etc. Currently it's not clear if there is relation to > > TX side latency, so it may be down to receiver side implementation. > > > > This needs a bit more work to figure out, but the L2CAP patch is anyway > > here. Due to the possible fragmentation in ISO sendmsg(), it seems we > > anyway need the tx_info_queue thing and can't easily do it by counting > > packets, and L2CAP required part is small addition on top of that. > > That is not how it is normally done with sockets, normally this is > done with use of SO_TIMESTAMPING which is then reported using the > socket error queue: > > https://www.kernel.org/doc/html/latest/networking/timestamping.html > > Sorry to tell you just now, you might have lost a lot of time doing > all of the changes, next time just drop and RFC early on with the > general design so you don't spend too much time before getting any > feedback. Thanks, looks like I missed that was not just for RX timestamping. We then need to hang on to the skbs to be timestamped in hci_core until packet completion. Looks like skb_queue_* is used by drivers, so probably skb_get and ringbuffer like in this patchset then. > > TBD: iso-tester / l2cap-tester tests > > > > Pauli Virtanen (3): > > Bluetooth: add transmission latency tracking for ISO and ACL > > Bluetooth: ISO: add new ioctl() for reading tx latency > > Bluetooth: L2CAP: add new ioctl() for reading tx latency > > > > include/net/bluetooth/bluetooth.h | 39 +++++++++++ > > include/net/bluetooth/hci_core.h | 30 ++++++++ > > net/bluetooth/hci_conn.c | 110 +++++++++++++++++++++++++++++- > > net/bluetooth/hci_core.c | 14 ++++ > > net/bluetooth/hci_event.c | 66 ++++++++++++++++++ > > net/bluetooth/iso.c | 58 ++++++++++++++-- > > net/bluetooth/l2cap_core.c | 12 ++++ > > net/bluetooth/l2cap_sock.c | 50 +++++++++++++- > > 8 files changed, 372 insertions(+), 7 deletions(-) > > > > -- > > 2.44.0 > > > > > >