Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230621195407.977001-1-luiz.dentz@gmail.com> <66add9bf780aca773d41cce3ea42589388de2543.camel@iki.fi>
In-Reply-To: <66add9bf780aca773d41cce3ea42589388de2543.camel@iki.fi>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 21 Jun 2023 14:12:18 -0700
Message-ID: <CABBYNZ+nGaeGmiAdneF7UEwG0Q8D6Bi1mnGOY66HcM5JwE1hiw@mail.gmail.com>
Subject: Re: [PATCH v2] Bluetooth: hci_event: Fix parsing of CIS Established Event
To:     Pauli Virtanen <pav@iki.fi>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Pauli,

On Wed, Jun 21, 2023 at 1:33=E2=80=AFPM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> ke, 2023-06-21 kello 12:54 -0700, Luiz Augusto von Dentz kirjoitti:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > 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 =3D 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@intel.com>
> > ---
> >  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..6fca6d9f1b34 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 hc=
i_dev *hdev, void *data,
> >  {
> >       struct hci_evt_le_cis_established *ev =3D data;
> >       struct hci_conn *conn;
> > +     struct bt_iso_qos *qos;
> >       bool pending =3D false;
> >       u16 handle =3D __le16_to_cpu(ev->handle);
> >
> > @@ -6846,21 +6847,30 @@ static void hci_le_cis_estabilished_evt(struct =
hci_dev *hdev, void *data,
> >
> >       pending =3D test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags)=
;
> >
> > -     if (conn->role =3D=3D HCI_ROLE_SLAVE) {
> > -             __le32 interval;
> > +     qos =3D &conn->iso_qos;
> >
> > -             memset(&interval, 0, sizeof(interval));
> > +     /* Convert ISO Interval (1.25 ms slots) to latency (ms) */
> > +     qos->ucast.in.latency =3D le16_to_cpu(ev->interval) * 125 / 100;
> > +     /* Convert ISO Interval (1.25 ms slots) to latency (ms) */
> > +     qos->ucast.out.latency =3D le16_to_cpu(ev->interval) * 125 / 100;
> >
> > -             memcpy(&interval, ev->c_latency, sizeof(ev->c_latency));
> > -             conn->iso_qos.ucast.in.interval =3D le32_to_cpu(interval)=
;
> > -             memcpy(&interval, ev->p_latency, sizeof(ev->p_latency));
> > -             conn->iso_qos.ucast.out.interval =3D le32_to_cpu(interval=
);
> > -             conn->iso_qos.ucast.in.latency =3D le16_to_cpu(ev->interv=
al);
> > -             conn->iso_qos.ucast.out.latency =3D le16_to_cpu(ev->inter=
val);
> > -             conn->iso_qos.ucast.in.sdu =3D le16_to_cpu(ev->c_mtu);
> > -             conn->iso_qos.ucast.out.sdu =3D le16_to_cpu(ev->p_mtu);
> > -             conn->iso_qos.ucast.in.phy =3D ev->c_phy;
> > -             conn->iso_qos.ucast.out.phy =3D ev->p_phy;
> > +     switch (conn->role) {
> > +     case HCI_ROLE_SLAVE:
> > +             qos->ucast.in.interval =3D get_unaligned_le24(ev->c_laten=
cy);
> > +             qos->ucast.out.interval =3D get_unaligned_le24(ev->p_late=
ncy);
> > +             qos->ucast.in.sdu =3D le16_to_cpu(ev->c_mtu);
> > +             qos->ucast.out.sdu =3D le16_to_cpu(ev->p_mtu);
> > +             qos->ucast.in.phy =3D ev->c_phy;
> > +             qos->ucast.out.phy =3D ev->p_phy;
> > +             break;
>
> Are the ucast.latency and ucast.interval the right way around here?
>
> When I trying to use this in userspace, I expected ucast.interval
> contains the ISO interval, because in Set CIG Parameters we use
> ucast.interval to specify the SDU_Interval, and ucast.latency is used
> for the Transport_Latency_C_To_P and Transport_Latency_P_To_C.
>
> With real numbers the event (AX210<->AX210) looks like this
>
> > HCI Event: LE Meta Event (0x3e) plen 29               #3493 [hci0] 486.=
978955
>       LE Connected Isochronous Stream Established (0x19)
>         Status: Success (0x00)
>         Connection Handle: 2560
>         CIG Synchronization Delay: 4020 us (0x000fb4)
>         CIS Synchronization Delay: 4020 us (0x000fb4)
>         Central to Peripheral Latency: 94020 us (0x016f44)
>         Peripheral to Central Latency: 94020 us (0x016f44)

No idea why these values look like this, they are not what I expect
which is to match what the Central configured with Set CIG Parameters.

>         Central to Peripheral PHY: LE 2M (0x02)
>         Peripheral to Central PHY: LE 2M (0x02)
>         Number of Subevents: 2
>         Central to Peripheral Burst Number: 1
>         Peripheral to Central Burst Number: 1
>         Central to Peripheral Flush Timeout: 10
>         Peripheral to Central Flush Timeout: 10
>         Central to Peripheral MTU: 240
>         Peripheral to Central MTU: 120
>         ISO Interval: 8

These seems to be fine, so I wonder what is going on with CIS
Established event, this is what we are doing in the emulator:

        /* TODO: Figure out if these values makes sense */
        memcpy(evt.cig_sync_delay, le_cig->params.c_interval,
                sizeof(le_cig->params.c_interval));
        memcpy(evt.cis_sync_delay, le_cig->params.p_interval,
                sizeof(le_cig->params.p_interval));
        memcpy(evt.c_latency, &le_cig->params.c_interval,
                sizeof(le_cig->params.c_interval));
        memcpy(evt.p_latency, &le_cig->params.p_interval,
                sizeof(le_cig->params.p_interval));
        evt.c_phy =3D le_cig->cis[cis_idx].c_phy;
        evt.p_phy =3D le_cig->cis[cis_idx].p_phy;
        evt.nse =3D 0x01;
        evt.c_bn =3D 0x01;
        evt.p_bn =3D 0x01;
        evt.c_ft =3D 0x01;
        evt.p_ft =3D 0x01;
        evt.c_mtu =3D le_cig->cis[cis_idx].c_sdu;
        evt.p_mtu =3D le_cig->cis[cis_idx].p_sdu;
        evt.interval =3D (le_cig->params.c_latency + 1) / 1.25;

>
> > +     case HCI_ROLE_MASTER:
> > +             qos->ucast.out.interval =3D get_unaligned_le24(ev->c_late=
ncy);
> > +             qos->ucast.in.interval =3D get_unaligned_le24(ev->p_laten=
cy);
> > +             qos->ucast.out.sdu =3D le16_to_cpu(ev->c_mtu);
> > +             qos->ucast.in.sdu =3D le16_to_cpu(ev->p_mtu);
> > +             qos->ucast.out.phy =3D ev->c_phy;
> > +             qos->ucast.in.phy =3D ev->p_phy;
> > +             break;
> >       }
> >
> >       if (!ev->status) {
>
> --
> Pauli Virtanen


--=20
Luiz Augusto von Dentz