Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2201-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1709150574.git.pav@iki.fi> <CABBYNZ+J_SUi_T987ND3hyN228zvX0gD62QNqk5D=GDc+nypTQ@mail.gmail.com>
In-Reply-To: <CABBYNZ+J_SUi_T987ND3hyN228zvX0gD62QNqk5D=GDc+nypTQ@mail.gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Wed, 28 Feb 2024 16:01:18 -0500
Message-ID: <CABBYNZ+PJTyWXdBE7Mm4YtjYDKc5eX6YfPk4_Sh8S5mdDf76AQ@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:31=E2=80=AFPM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> 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 lon=
g
> > 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, no=
t
> > 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 packet=
s
> > 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 of=
f
> > by ~0..40 ms compared to observed audio latency and this amount can var=
y
> > per connection and time, so not explained by unaccounted code
> > algorithmic delays etc. Currently it's not clear if there is relation t=
o
> > 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/+/r=
efs/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


--=20
Luiz Augusto von Dentz