* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-11 8:14 ` [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet tailu.shi
@ 2026-03-11 8:54 ` Paul Menzel
2026-03-12 9:38 ` Tailu Shi
2026-03-11 16:24 ` Pauli Virtanen
1 sibling, 1 reply; 6+ messages in thread
From: Paul Menzel @ 2026-03-11 8:54 UTC (permalink / raw)
To: Tailu Shi
Cc: Marcel Holtmann, Luiz Augusto von Dentz, Pauli Virtanen,
Johan Hedberg, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, linux-bluetooth, netdev, linux-kernel
Dear Tailu,
Thank you for your patch. A formal request at the beginning to please
spell your name without the dot/period – maybe: Tailu Shi.
git config --global user.name "Tailu Shi"
git commit --amend -s --author="Tailu Shi <tailu.shi@samsung.com>"
Am 11.03.26 um 09:14 schrieb tailu.shi:
> The Bluetooth Core Specification defines the 'Time_Stamp' field of
> an HCI ISO packet as optional. However, it's mandatory to include
> the 'Time_Stamp' field when using HCI ISO packets per section 3.3
> 'HCI Feature Support Requirement' of TMAP specification.
> https://www.bluetooth.com/specifications/specs/html/?src=TMAP_v1.0.1/out/en/index-en.html#UUID-68138fa8-fd1f-1a1b-e552-ddcd8fede9ec
>
> To comply with TMAP, introduce a new socket option BT_PKT_ISO_TIMESTAMP
> that allows user application (e.g. PipeWire) to attach timestamp to the
> data it sends to the ISO socket within BT_SCM_PKT_ISO_TIMESTAMP CMSG.
>
> When the option is enabled, the kernel extracts the timestamp and copies
> it into the 'Time_Stamp' field of the outgoing HCI ISO packet.
>
> This also gives the controller the reference timing information
> required for ISO stream synchronization.
>
> A corresponding userspace change is required for full functionality.
> A reference implementation is available in PipeWire:
> https://gitlab.freedesktop.org/shitailu/pipewire/-/commit/5b3bd74e15febb974c8737a64f31fd17e18cd11b
Is the timestamp correctly visible in the btmon trace?
> Signed-off-by: tailu.shi <tailu.shi@samsung.com>
> ---
> include/net/bluetooth/bluetooth.h | 5 ++++
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 2 +-
> net/bluetooth/iso.c | 49 +++++++++++++++++++++++++++----
> 4 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 69eed69f7f26..f88c83d21c5e 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -259,6 +259,10 @@ struct bt_codecs {
>
> #define BT_SCM_PKT_SEQNUM 0x05
>
> +#define BT_PKT_ISO_TIMESTAMP 23
> +
> +#define BT_SCM_PKT_ISO_TIMESTAMP 0x06
> +
> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> @@ -409,6 +413,7 @@ enum {
> BT_SK_SUSPEND,
> BT_SK_PKT_STATUS,
> BT_SK_PKT_SEQNUM,
> + BT_SK_PKT_ISO_TIMESTAMP,
> };
>
> struct bt_sock_list {
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a7bffb908c1e..e39d192f8ef6 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -740,6 +740,7 @@ struct hci_conn {
> struct bt_iso_qos iso_qos;
> __u8 num_bis;
> __u8 bis[HCI_MAX_ISO_BIS];
> + bool iso_pkt_ts;
>
> unsigned long flags;
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 31308c1de4ec..4264c0229dfb 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3335,7 +3335,7 @@ static void hci_queue_iso(struct hci_conn *conn, struct sk_buff_head *queue,
>
> list = skb_shinfo(skb)->frag_list;
>
> - flags = hci_iso_flags_pack(list ? ISO_START : ISO_SINGLE, 0x00);
> + flags = hci_iso_flags_pack(list ? ISO_START : ISO_SINGLE, conn->iso_pkt_ts ? 0x01 : 0x00);
> hci_add_iso_hdr(skb, conn->handle, flags);
>
> if (!list) {
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index be145e2736b7..dd4d198bab4c 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -73,6 +73,9 @@ struct iso_pinfo {
> __u8 base_len;
> __u8 base[BASE_MAX_LENGTH];
> struct iso_conn *conn;
> + bool ts_flag;
> + __u32 ts;
> +
> };
>
> static struct bt_iso_qos default_qos;
> @@ -542,6 +545,7 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
> struct iso_conn *conn = iso_pi(sk)->conn;
> struct bt_iso_qos *qos = iso_sock_get_qos(sk);
> struct hci_iso_data_hdr *hdr;
> + struct hci_iso_ts_data_hdr *hdr_ts;
> int len = 0;
>
> BT_DBG("sk %p len %d", sk, skb->len);
> @@ -552,13 +556,20 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
> len = skb->len;
>
> /* Push ISO data header */
> - hdr = skb_push(skb, HCI_ISO_DATA_HDR_SIZE);
> - hdr->sn = cpu_to_le16(conn->tx_sn++);
> - hdr->slen = cpu_to_le16(hci_iso_data_len_pack(len,
> - HCI_ISO_STATUS_VALID));
> + if (iso_pi(sk)->ts_flag) {
> + hdr_ts = skb_push(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> + hdr_ts->ts = cpu_to_le32(iso_pi(sk)->ts);
> + hdr_ts->sn = cpu_to_le16(conn->tx_sn++);
> + hdr_ts->slen = cpu_to_le16(hci_iso_data_len_pack(len, HCI_ISO_STATUS_VALID));
> + } else {
> + hdr = skb_push(skb, HCI_ISO_DATA_HDR_SIZE);
> + hdr->sn = cpu_to_le16(conn->tx_sn++);
> + hdr->slen = cpu_to_le16(hci_iso_data_len_pack(len, HCI_ISO_STATUS_VALID));
> + }
>
> if (sk->sk_state == BT_CONNECTED) {
> hci_setup_tx_timestamp(skb, 1, sockc);
> + conn->hcon->iso_pkt_ts = iso_pi(sk)->ts_flag;
> hci_send_iso(conn->hcon, skb);
> } else {
> len = -ENOTCONN;
> @@ -1471,7 +1482,8 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> struct sock *sk = sock->sk;
> struct sk_buff *skb, **frag;
> struct sockcm_cookie sockc;
> - size_t mtu;
> + struct cmsghdr *cm;
> + size_t mtu, hlen;
> int err;
>
> BT_DBG("sock %p, sk %p", sock, sk);
> @@ -1485,10 +1497,23 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>
> hci_sockcm_init(&sockc, sk);
>
> + iso_pi(sk)->ts_flag = false;
> if (msg->msg_controllen) {
> err = sock_cmsg_send(sk, msg, &sockc);
> if (err)
> return err;
> +
> + for (cm = CMSG_FIRSTHDR(msg); cm; cm = CMSG_NXTHDR(msg, cm)) {
> + if (cm->cmsg_level != SOL_BLUETOOTH)
> + continue;
> + if (test_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags) &&
> + cm->cmsg_type == BT_SCM_PKT_ISO_TIMESTAMP &&
> + cm->cmsg_len == CMSG_LEN(sizeof(u32))) {
> + iso_pi(sk)->ts_flag = true;
> + iso_pi(sk)->ts = *(u32 *)CMSG_DATA(cm);
> + break;
> + }
> + }
> }
>
> lock_sock(sk);
> @@ -1502,7 +1527,8 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>
> release_sock(sk);
>
> - skb = bt_skb_sendmsg(sk, msg, len, mtu, HCI_ISO_DATA_HDR_SIZE, 0);
> + hlen = iso_pi(sk)->ts_flag ? HCI_ISO_TS_DATA_HDR_SIZE : HCI_ISO_DATA_HDR_SIZE;
> + skb = bt_skb_sendmsg(sk, msg, len, mtu, hlen, 0);
> if (IS_ERR(skb))
> return PTR_ERR(skb);
>
> @@ -1840,6 +1866,17 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
>
> break;
>
> + case BT_PKT_ISO_TIMESTAMP:
> + err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> + if (err)
> + break;
> +
> + if (opt)
> + set_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags);
> + else
> + clear_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags);
> + break;
> +
> default:
> err = -ENOPROTOOPT;
> break;
The diff looks good.
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-11 8:54 ` Paul Menzel
@ 2026-03-12 9:38 ` Tailu Shi
0 siblings, 0 replies; 6+ messages in thread
From: Tailu Shi @ 2026-03-12 9:38 UTC (permalink / raw)
To: Tailu Shi, Paul Menzel
Cc: Marcel Holtmann, Luiz Augusto von Dentz, Pauli Virtanen,
Johan Hedberg, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, linux-bluetooth, netdev, linux-kernel
Hi Paul,
Thank you for the review and the helpful suggestion about my name.
I have updated my Git configuration and amended the commit as you advised:
git config --global user.name "Tailu Shi"
git commit --amend -s --author="Tailu Shi <tailu.shi@samsung.com>"
On Wed, 11 Mar 2026 09:54:38 +0100 Paul Menzel wrote:
>> A corresponding userspace change is required for full functionality.
>> A reference implementation is available in PipeWire:
>> https://gitlab.freedesktop.org/shitailu/pipewire/-/commit/5b3bd74e15febb974c8737a64f31fd17e18cd11b
>
> Is the timestamp correctly visible in the btmon trace?
Yes, it can be seen in the btmon trace. The test was performed on the
kernel 7.0.0‑rc1+, using BlueZ 5.85 and PipeWire 1.5.84 with the
'bluez5.iso-tx-timestamp' configuration option enabled.
But seems the btmon v5.85 and v5.86 fails to parse it directly with
the command. However, the 'ts' field can be seen clearly with Wireshark.
The things are like below:
1) btmon v5.86
$ btmon -r dump.btsnoop -I |grep -A1 'ISO Data TX: Handle 2563'
< ISO Data TX: Handle 2563 [5/18] SN 2 #10999 [hci0] 128.571172
02 00 3c 00 00 00 00 00 00 00 00 00 00 00 00 00 ..<.............
--
2) Wireshark
Frame 19604: 72 bytes on wire (576 bits), 72 bytes captured (576 bits)
Bluetooth
Bluetooth Linux Monitor Transport
Bluetooth HCI ISO Packet
.... 1010 0000 0011 = Connection Handle: 0xa03
..10 .... .... .... = PB Flag: Complete SDU (0x2)
.1.. .... .... .... = Timestamp present: True
0... .... .... .... = Reserved: 0x0
..00 0000 0100 0100 = Data Length: 68
Data
[Connect in frame: 18836]
Bluetooth ISO Data
Timestamp: 805103660µs
Sequence Number: 84
.... 0000 0011 1100 = SDU Length: 60
SDU
Raw data:
0000 03 6a 44 00 2c e8 fc 2f 54 00 3c 00 00 00 00 00 .jD.,../T.<.....
0010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0040 38 24 f9 4a 0d 00 00 03 8$.J....
Thanks for your time again!
Best regards,
Tailu Shi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-11 8:14 ` [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet tailu.shi
2026-03-11 8:54 ` Paul Menzel
@ 2026-03-11 16:24 ` Pauli Virtanen
2026-03-13 12:01 ` Tailu Shi
1 sibling, 1 reply; 6+ messages in thread
From: Pauli Virtanen @ 2026-03-11 16:24 UTC (permalink / raw)
To: tailu.shi, Marcel Holtmann, Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi,
ke, 2026-03-11 kello 16:14 +0800, tailu.shi kirjoitti:
> The Bluetooth Core Specification defines the 'Time_Stamp' field of
> an HCI ISO packet as optional. However, it's mandatory to include
> the 'Time_Stamp' field when using HCI ISO packets per section 3.3
> 'HCI Feature Support Requirement' of TMAP specification.
> https://www.bluetooth.com/specifications/specs/html/?src=TMAP_v1.0.1/out/en/index-en.html#UUID-68138fa8-fd1f-1a1b-e552-ddcd8fede9ec
>
> To comply with TMAP, introduce a new socket option BT_PKT_ISO_TIMESTAMP
> that allows user application (e.g. PipeWire) to attach timestamp to the
> data it sends to the ISO socket within BT_SCM_PKT_ISO_TIMESTAMP CMSG.
>
> When the option is enabled, the kernel extracts the timestamp and copies
> it into the 'Time_Stamp' field of the outgoing HCI ISO packet.
>
> This also gives the controller the reference timing information
> required for ISO stream synchronization.
These timestamps are in controller clock domain, so generic host like
Linux appears to need to perform clock synchronization via
HCI_OP_LE_READ_ISO_TX_SYNC to be able to provide timestamps.
I had in the works a not fully finished patch series adding the same
thing + LE Read ISO TX Sync + tests. I sent these now to the list in
their current state, maybe they are useful if you are working on this
https://lore.kernel.org/linux-bluetooth/cover.1773245666.git.pav@iki.fi/T/
https://lore.kernel.org/linux-bluetooth/cover.1773245672.git.pav@iki.fi/T/
The tests pass, but not really tested to satisfaction on real
controllers.
From what I saw, it seems several controllers like Intel AX210 and some
Realtek ones don't implement LE Read ISO TX Sync correctly (returns 0
timestamp), so in those cases it AFAICS it's not possible to provide
host timestamps.
Getting the clock sync via LE Read ISO TX Sync to work properly across
controllers is maybe not so easy, as it appears the Core Specification
is somewhat unclear how it should work.
> A corresponding userspace change is required for full functionality.
> A reference implementation is available in PipeWire:
> https://gitlab.freedesktop.org/shitailu/pipewire/-/commit/5b3bd74e15febb974c8737a64f31fd17e18cd11b
This sends timestamps in host clock domain and has no clock sync, but I
guess this is just for testing.
> Signed-off-by: tailu.shi <tailu.shi@samsung.com>
> ---
> include/net/bluetooth/bluetooth.h | 5 ++++
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 2 +-
> net/bluetooth/iso.c | 49 +++++++++++++++++++++++++++----
> 4 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 69eed69f7f26..f88c83d21c5e 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -259,6 +259,10 @@ struct bt_codecs {
>
> #define BT_SCM_PKT_SEQNUM 0x05
>
> +#define BT_PKT_ISO_TIMESTAMP 23
> +
> +#define BT_SCM_PKT_ISO_TIMESTAMP 0x06
> +
> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> @@ -409,6 +413,7 @@ enum {
> BT_SK_SUSPEND,
> BT_SK_PKT_STATUS,
> BT_SK_PKT_SEQNUM,
> + BT_SK_PKT_ISO_TIMESTAMP,
> };
>
> struct bt_sock_list {
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a7bffb908c1e..e39d192f8ef6 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -740,6 +740,7 @@ struct hci_conn {
> struct bt_iso_qos iso_qos;
> __u8 num_bis;
> __u8 bis[HCI_MAX_ISO_BIS];
> + bool iso_pkt_ts;
>
> unsigned long flags;
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 31308c1de4ec..4264c0229dfb 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3335,7 +3335,7 @@ static void hci_queue_iso(struct hci_conn *conn, struct sk_buff_head *queue,
>
> list = skb_shinfo(skb)->frag_list;
>
> - flags = hci_iso_flags_pack(list ? ISO_START : ISO_SINGLE, 0x00);
> + flags = hci_iso_flags_pack(list ? ISO_START : ISO_SINGLE, conn->iso_pkt_ts ? 0x01 : 0x00);
> hci_add_iso_hdr(skb, conn->handle, flags);
>
> if (!list) {
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index be145e2736b7..dd4d198bab4c 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -73,6 +73,9 @@ struct iso_pinfo {
> __u8 base_len;
> __u8 base[BASE_MAX_LENGTH];
> struct iso_conn *conn;
> + bool ts_flag;
> + __u32 ts;
I think these are not needed, they can be passed as arguments to
iso_send_frame().
Adding them to iso_pinfo() probably causes a data race if two processes
sendmsg() at the same time.
> +
> };
>
> static struct bt_iso_qos default_qos;
> @@ -542,6 +545,7 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
> struct iso_conn *conn = iso_pi(sk)->conn;
> struct bt_iso_qos *qos = iso_sock_get_qos(sk);
> struct hci_iso_data_hdr *hdr;
> + struct hci_iso_ts_data_hdr *hdr_ts;
> int len = 0;
>
> BT_DBG("sk %p len %d", sk, skb->len);
> @@ -552,13 +556,20 @@ static int iso_send_frame(struct sock *sk, struct sk_buff *skb,
> len = skb->len;
>
> /* Push ISO data header */
> - hdr = skb_push(skb, HCI_ISO_DATA_HDR_SIZE);
> - hdr->sn = cpu_to_le16(conn->tx_sn++);
> - hdr->slen = cpu_to_le16(hci_iso_data_len_pack(len,
> - HCI_ISO_STATUS_VALID));
> + if (iso_pi(sk)->ts_flag) {
> + hdr_ts = skb_push(skb, HCI_ISO_TS_DATA_HDR_SIZE);
> + hdr_ts->ts = cpu_to_le32(iso_pi(sk)->ts);
> + hdr_ts->sn = cpu_to_le16(conn->tx_sn++);
> + hdr_ts->slen = cpu_to_le16(hci_iso_data_len_pack(len, HCI_ISO_STATUS_VALID));
> + } else {
> + hdr = skb_push(skb, HCI_ISO_DATA_HDR_SIZE);
> + hdr->sn = cpu_to_le16(conn->tx_sn++);
> + hdr->slen = cpu_to_le16(hci_iso_data_len_pack(len, HCI_ISO_STATUS_VALID));
> + }
>
> if (sk->sk_state == BT_CONNECTED) {
> hci_setup_tx_timestamp(skb, 1, sockc);
> + conn->hcon->iso_pkt_ts = iso_pi(sk)->ts_flag;
> hci_send_iso(conn->hcon, skb);
> } else {
> len = -ENOTCONN;
> @@ -1471,7 +1482,8 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> struct sock *sk = sock->sk;
> struct sk_buff *skb, **frag;
> struct sockcm_cookie sockc;
> - size_t mtu;
> + struct cmsghdr *cm;
> + size_t mtu, hlen;
> int err;
>
> BT_DBG("sock %p, sk %p", sock, sk);
> @@ -1485,10 +1497,23 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>
> hci_sockcm_init(&sockc, sk);
>
> + iso_pi(sk)->ts_flag = false;
> if (msg->msg_controllen) {
> err = sock_cmsg_send(sk, msg, &sockc);
> if (err)
> return err;
> +
> + for (cm = CMSG_FIRSTHDR(msg); cm; cm = CMSG_NXTHDR(msg, cm)) {
> + if (cm->cmsg_level != SOL_BLUETOOTH)
> + continue;
> + if (test_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags) &&
> + cm->cmsg_type == BT_SCM_PKT_ISO_TIMESTAMP &&
> + cm->cmsg_len == CMSG_LEN(sizeof(u32))) {
> + iso_pi(sk)->ts_flag = true;
> + iso_pi(sk)->ts = *(u32 *)CMSG_DATA(cm);
> + break;
> + }
> + }
I would put this in a separate function.
> }
>
> lock_sock(sk);
> @@ -1502,7 +1527,8 @@ static int iso_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>
> release_sock(sk);
>
> - skb = bt_skb_sendmsg(sk, msg, len, mtu, HCI_ISO_DATA_HDR_SIZE, 0);
> + hlen = iso_pi(sk)->ts_flag ? HCI_ISO_TS_DATA_HDR_SIZE : HCI_ISO_DATA_HDR_SIZE;
> + skb = bt_skb_sendmsg(sk, msg, len, mtu, hlen, 0);
> if (IS_ERR(skb))
> return PTR_ERR(skb);
>
> @@ -1840,6 +1866,17 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
>
> break;
>
> + case BT_PKT_ISO_TIMESTAMP:
> + err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> + if (err)
> + break;
> +
> + if (opt)
> + set_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags);
> + else
> + clear_bit(BT_SK_PKT_ISO_TIMESTAMP, &bt_sk(sk)->flags);
> + break;
> +
> default:
> err = -ENOPROTOOPT;
> break;
--
Pauli Virtanen
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-11 16:24 ` Pauli Virtanen
@ 2026-03-13 12:01 ` Tailu Shi
2026-03-13 20:50 ` Pauli Virtanen
0 siblings, 1 reply; 6+ messages in thread
From: Tailu Shi @ 2026-03-13 12:01 UTC (permalink / raw)
To: Pauli Virtanen
Cc: Tailu Shi, Marcel Holtmann, Luiz Augusto von Dentz, Paul Menzel,
linux-bluetooth, linux-kernel
Hi Pauli,
Thanks for the detailed review and for sharing your RFC patches.
On Wed, 11 Mar 2026 18:24:23 +0200 Pauli Virtanen wrote:
> These timestamps are in controller clock domain, so generic host like
> Linux appears to need to perform clock synchronization via
> HCI_OP_LE_READ_ISO_TX_SYNC to be able to provide timestamps.
I agree that, according to Core v6.2 Vol 6 Part G Section 3, the timestamp
must be derived from the Controller's clock. However, Section 3.3 also defines
a falllback where the controller decides the CIS/BIS event when the
host‑supplied timestamp is not based on the Controller's clock. It looks
contradictory.
Section 3.3 further states that the host may retrieve the timestamp via
HCI_LE_Read_ISO_TX_Sync command. The wording makes it optional. And also
as you have noted, many controller returns 0 for it, leaving the practical
challenges to make it work properly.
That's why we considered leaving it to the userspace application.
> I had in the works a not fully finished patch series adding the same
> thing + LE Read ISO TX Sync + tests. I sent these now to the list in
> their current state, maybe they are useful if you are working on this
>
> https://lore.kernel.org/linux-bluetooth/cover.1773245666.git.pav@iki.fi/T/
>
> https://lore.kernel.org/linux-bluetooth/cover.1773245672.git.pav@iki.fi/T/
>
> The tests pass, but not really tested to satisfaction on real
> controllers.
>
> From what I saw, it seems several controllers like Intel AX210 and some
> Realtek ones don't implement LE Read ISO TX Sync correctly (returns 0
> timestamp), so in those cases it AFAICS it's not possible to provide
> host timestamps.
>
> Getting the clock sync via LE Read ISO TX Sync to work properly across
> controllers is maybe not so easy, as it appears the Core Specification
> is somewhat unclear how it should work.
I have reviewed the series you posted and I think it both complete and
elegant. Since my earlier work is now covered by your commit (08692f3...),
I will withdraw my patch.
I've cherry-picked your series and kept the PipeWire logic that passes the
timestamp via BT_SCM_PKT_ISO_TIMESTAMP CMSG. The implementation works on my
platform (based on the Kernel 7.0.0-rc1, BlueZ 5.85 and PipeWire 1.5.84).
The timestamp can be observed in the btmon trace.
Partial HCI parsed by Wireshark:
Frame 32436: 72 bytes on wire (576 bits), 72 bytes captured (576 bits)
Bluetooth
Bluetooth Linux Monitor Transport
Bluetooth HCI ISO Packet
.... 1010 0000 0101 = Connection Handle: 0xa05
..10 .... .... .... = PB Flag: Complete SDU (0x2)
.1.. .... .... .... = Timestamp present: True
0... .... .... .... = Reserved: 0x0
..00 0000 0100 0100 = Data Length: 68
Data
[Connect in frame: 9436]
Bluetooth ISO Data
Timestamp: 469412333us
Sequence Number: 3595
.... 0000 0011 1100 = SDU Length: 60
SDU
Please feel free to add the following Tested-by line if it helps:
Tested-by: Tailu Shi <tailu.shi@samsung.com>
May I know if you have any plan to merge this series? And also please feel
free to let me know if there's anything I can help to move this forward.
> This sends timestamps in host clock domain and has no clock sync, but I
> guess this is just for testing.
I think it's better to have a complete implementation solution. So if possible,
I would like to improve it and push it forward. Please feel free to let me
know if you have any other opinions.
> I think these are not needed, they can be passed as arguments to
> iso_send_frame().
>
> Adding them to iso_pinfo() probably causes a data race if two processes
> sendmsg() at the same time.
Thank you for identifying this potential data race issue. It was not considered
in the current patch, and I will incorporate your suggestion in future development.
> I would put this in a separate function.
Agreed. It will make code clear. I will keep this code style in future development.
Thank you again for the detailed review!
Best regards!
Tailu
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Bluetooth: ISO: add timestamp for outgoing HCI ISO packet
2026-03-13 12:01 ` Tailu Shi
@ 2026-03-13 20:50 ` Pauli Virtanen
0 siblings, 0 replies; 6+ messages in thread
From: Pauli Virtanen @ 2026-03-13 20:50 UTC (permalink / raw)
To: Tailu Shi; +Cc: Luiz Augusto von Dentz, linux-bluetooth
Hi,
pe, 2026-03-13 kello 20:01 +0800, Tailu Shi kirjoitti:
[clip]
> I agree that, according to Core v6.2 Vol 6 Part G Section 3, the
> timestamp must be derived from the Controller's clock. However,
> Section 3.3 also defines a falllback where the controller decides the
> CIS/BIS event when the host‑supplied timestamp is not based on the
> Controller's clock. It looks contradictory.
TMAP likely assumes the controller implements the specification so that
providing correct time stamps would be possible, but when they in
practice don't, spec compliance seems dubious and not setting known-
wrong Time_Stamp on packets sounds safer.
[clip]
> I've cherry-picked your series and kept the PipeWire logic that passes the
> timestamp via BT_SCM_PKT_ISO_TIMESTAMP CMSG. The implementation works on my
> platform (based on the Kernel 7.0.0-rc1, BlueZ 5.85 and PipeWire 1.5.84).
> The timestamp can be observed in the btmon trace.
>
> Partial HCI parsed by Wireshark:
> Frame 32436: 72 bytes on wire (576 bits), 72 bytes captured (576 bits)
> Bluetooth
> Bluetooth Linux Monitor Transport
> Bluetooth HCI ISO Packet
> .... 1010 0000 0101 = Connection Handle: 0xa05
> ..10 .... .... .... = PB Flag: Complete SDU (0x2)
> .1.. .... .... .... = Timestamp present: True
> 0... .... .... .... = Reserved: 0x0
> ..00 0000 0100 0100 = Data Length: 68
> Data
> [Connect in frame: 9436]
> Bluetooth ISO Data
> Timestamp: 469412333us
> Sequence Number: 3595
> .... 0000 0011 1100 = SDU Length: 60
> SDU
>
> Please feel free to add the following Tested-by line if it helps:
> Tested-by: Tailu Shi <tailu.shi@samsung.com>
>
> May I know if you have any plan to merge this series? And also please feel
> free to let me know if there's anything I can help to move this forward.
>
> > This sends timestamps in host clock domain and has no clock sync, but I
> > guess this is just for testing.
>
> I think it's better to have a complete implementation solution. So if possible,
> I would like to improve it and push it forward. Please feel free to let me
> know if you have any other opinions.
I think we will want LE Read ISO TX Sync support at least, as this will
solve the CIS/BIS synchronization problem, as user application can then
know what ISO packets controller considers are playing at the same time
(if controller actually implements this command properly).
It would be good to have information how different manufacturers have
interpreted the specification vs. LE Read ISO TX Sync. Right now, I can
only read what Zephyr is doing, and tested some other controllers
return zeros. If you have access to controllers that implement this
command, it would be very useful to know their precise interpretation.
@Luiz: thoughts on the below?
For setting Time_Stamp, I think there should be plan how userspace
application uses the new APIs to produce working sound on different
controllers, without having to maintain a quirk list for each
controller. I think quirk lists should be in the kernel driver.
Depending on what hardware is doing, we could add driver quirks along
the lines of
HCI_QUIRK_BROKEN_LE_READ_ISO_TX_SYNC, /* always zero / dummy value */
HCI_QUIRK_LE_READ_ISO_TX_SYNC_HOST_PREV, /* most recent from host */
HCI_QUIRK_LE_READ_ISO_TX_SYNC_NOCP, /* most recent completed */
HCI_QUIRK_LE_READ_ISO_TX_SYNC_HOST_SEQ, /* uses host seqnum */
and should affect what skb is reported in hci_cc_le_read_iso_tx_sync(),
and if SOF_TIMESTAMPING_TX_HARDWARE capability is reported in ethtool.
(Eg. IIUC Zephyr HCI appears to interpret LE Read ISO TX Sync meaning
as follows:
- The sequence number is controller-assigned.
- The response corresponds to the ISO data packet that
the controller *most recently received from host*.
This interpretation makes sense, but is different from my patch and
iiuc also different from what Luiz had in mind.)
If no quirk is set but the command is supported, probably should emit
visible error in kernel log, or something along such lines, so the
driver authors can see something is not right.
But knowing what is the precise hardware behavior for each
chip/firmware seems a bit involved, probably should have some testing
tool for this.
***
What it seems userspace can do:
Userspace application would check via ethtool
SOF_TIMESTAMPING_TX_HARDWARE capability to know whether controller
handles Time_Stamp in a non-broken way. (For testing, there could be a
switch to force it to use timestamps regardless.)
It could then rely that the HW timestamp corresponds to specific packet
it sent earlier. It could assume that in its clock domain, the HW
timestamp is after SND timestamp, before COMPLETION timestamp, and
likely close to COMPLETION.
Since it doesn't know the exact correspondence, probably it would
always have to ensure a few packets are always queued in controller.
There also needs to be a way to recover from a situation where the
application is setting timestamps that are in the past in controller
clock domain.
The controller does not necessarily send them I think, but will report
them completed immediately (after older ones get flushed from its
buffers).
In this case LE Read ISO TX Sync timestamp would not advance, as no
packets get sent. Kernel would also not send HW timestamps to user
application in this case.
If the application when it doesn't receive HW timestamps from kernel,
just increments its send timestamp by SDU interval for each packet, it
eventually catches up the controller clock, and receives HW timestamp
and can resync.
Alternatively the user application may stop setting timestamps, in
which case controller shall send it in next SDU interval, and in this
case HW timestamp is received and user application can resync.
Or, alternatively, controller will decide the clock is out of sync and
send the SDU regardless, in which case user application will receive a
new HW timestamp and can again resync.
>
> > I think these are not needed, they can be passed as arguments to
> > iso_send_frame().
> >
> > Adding them to iso_pinfo() probably causes a data race if two processes
> > sendmsg() at the same time.
>
> Thank you for identifying this potential data race issue. It was not considered
> in the current patch, and I will incorporate your suggestion in future development.
>
> > I would put this in a separate function.
>
> Agreed. It will make code clear. I will keep this code style in future development.
>
>
> Thank you again for the detailed review!
>
> Best regards!
>
> Tailu
^ permalink raw reply [flat|nested] 6+ messages in thread