Hi Luiz,
to, 2023-06-22 kello 12:27 -0700, Luiz Augusto von Dentz kirjoitti:
> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
>
> The ISO Interval on CIS Established Event uses 1.25 ms slots:
>
> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
> page 2304:
>
> Time = N * 1.25 ms
>
> In addition to that this always update the QoS settings based on CIS
> Established Event.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> ---
> net/bluetooth/hci_event.c | 36 +++++++++++++++++++++++-------------
> 1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index b1aefe4bb751..049aa7f6a7c5 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -6822,6 +6822,7 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> {
> struct hci_evt_le_cis_established *ev = data;
> struct hci_conn *conn;
> + struct bt_iso_qos *qos;
> bool pending = false;
> u16 handle = __le16_to_cpu(ev->handle);
>
> @@ -6846,21 +6847,30 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
>
> pending = test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags);
>
> - if (conn->role == HCI_ROLE_SLAVE) {
> - __le32 interval;
> + qos = &conn->iso_qos;
>
> - memset(&interval, 0, sizeof(interval));
> + /* Convert ISO Interval (1.25 ms slots) to latency (ms) */
> + qos->ucast.in.latency = DIV_ROUND_CLOSEST(le16_to_cpu(ev->interval) *
> + 125, 100);
> + qos->ucast.out.latency = qos->ucast.in.latency;
In Set CIG Parameters:
u16 ucast.latency = Max_Transport_Latency (ms)
u32 ucast.interval = SDU_Interval (us)
Here:
u16 ucast.latency = ISO_Interval (ms)
u32 ucast.interval = Transport_Latency (us)
Currently BlueZ seems to not account for this swapping of the meanings
of the QoS fields, eg. in client/player.c it has
num = (qos.ucast.out.latency * 1000 / qos.ucast.out.interval);
...
ts.it_value.tv_nsec = qos->ucast.out.latency * 1000000
and writes num packets in each interval. In the AX210 example it seems
this would give num == 0. I guess this and other places that use these
need to be updated.
Since field meanings anyway change, would 1.25ms unit for the
ISO_Interval be better than 1ms so user side doesn't need to know how
kernel rounds the number and undo that to get the actual value?
> - memcpy(&interval, ev->c_latency, sizeof(ev->c_latency));
> - conn->iso_qos.ucast.in.interval = le32_to_cpu(interval);
> - memcpy(&interval, ev->p_latency, sizeof(ev->p_latency));
> - conn->iso_qos.ucast.out.interval = le32_to_cpu(interval);
> - conn->iso_qos.ucast.in.latency = le16_to_cpu(ev->interval);
> - conn->iso_qos.ucast.out.latency = le16_to_cpu(ev->interval);
> - conn->iso_qos.ucast.in.sdu = le16_to_cpu(ev->c_mtu);
> - conn->iso_qos.ucast.out.sdu = le16_to_cpu(ev->p_mtu);
> - conn->iso_qos.ucast.in.phy = ev->c_phy;
> - conn->iso_qos.ucast.out.phy = ev->p_phy;
> + switch (conn->role) {
> + case HCI_ROLE_SLAVE:
> + qos->ucast.in.interval = get_unaligned_le24(ev->c_latency);
> + qos->ucast.out.interval = get_unaligned_le24(ev->p_latency);
> + qos->ucast.in.sdu = le16_to_cpu(ev->c_mtu);
> + qos->ucast.out.sdu = le16_to_cpu(ev->p_mtu);
> + qos->ucast.in.phy = ev->c_phy;
> + qos->ucast.out.phy = ev->p_phy;
> + break;
> + case HCI_ROLE_MASTER:
> + qos->ucast.out.interval = get_unaligned_le24(ev->c_latency);
> + qos->ucast.in.interval = get_unaligned_le24(ev->p_latency);
> + qos->ucast.out.sdu = le16_to_cpu(ev->c_mtu);
> + qos->ucast.in.sdu = le16_to_cpu(ev->p_mtu);
> + qos->ucast.out.phy = ev->c_phy;
> + qos->ucast.in.phy = ev->p_phy;
> + break;
> }
>
> if (!ev->status) {
