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

Hi Luiz,

ke, 2024-02-28 kello 15:31 -0500, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
>=20
> On Wed, Feb 28, 2024 at 3:11=E2=80=AFPM Pauli Virtanen <pav@iki.fi> wrote=
:
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > Pipewire side:
> > https://gitlab.freedesktop.org/pvir/pipewire/-/commits/iso-ts-test
> >=20
> > 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).
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > 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.
>=20
> 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:
>=20
> https://www.kernel.org/doc/html/latest/networking/timestamping.html
>=20
> 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
> >=20
> > 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
> >=20
> >  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(-)
> >=20
> > --
> > 2.44.0
> >=20
> >=20
>=20
>=20

--=20
Pauli Virtanen