Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2199-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1709150574.git.pav@iki.fi>
In-Reply-To: <cover.1709150574.git.pav@iki.fi>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Wed, 28 Feb 2024 15:31:08 -0500
Message-ID: <CABBYNZ+J_SUi_T987ND3hyN228zvX0gD62QNqk5D=GDc+nypTQ@mail.gmail.com>
Subject: Re: [RFC PATCH 0/3] Bluetooth: add transmission latency tracking for
 ISO & L2CAP
To: Pauli Virtanen <pav@iki.fi>
Cc: linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Pauli,

On Wed, Feb 28, 2024 at 3:11=E2=80=AFPM 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
>
>


--=20
Luiz Augusto von Dentz