Hi Vlad,
On Tue, Apr 16, 2024 at 6:22 AM Vlad Pruteanu <vlad.pruteanu@xxxxxxx> wrote:
>
> Hello Luiz,
>
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> > Sent: Monday, April 15, 2024 6:07 PM
> > To: Vlad Pruteanu <vlad.pruteanu@xxxxxxx>
> > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Claudia Cristina Draghicescu
> > <claudia.rosu@xxxxxxx>; Mihai-Octavian Urzica <mihai-
> > octavian.urzica@xxxxxxx>; Silviu Florian Barbulescu
> > <silviu.barbulescu@xxxxxxx>; Iulia Tanasescu <iulia.tanasescu@xxxxxxx>;
> > Andrei Istodorescu <andrei.istodorescu@xxxxxxx>
> > Subject: [EXT] Re: [PATCH 1/1] Bluetooth: hci_event: Fix setting of unicast qos
> > interval
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report
> > this email' button
> >
> >
> > Hi Vlad,
> >
> > On Mon, Apr 15, 2024 at 10:45 AM Vlad Pruteanu
> > <vlad.pruteanu@xxxxxxx> wrote:
> > >
> > > qos->ucast interval reffers to the SDU interval, and should not
> > > be set to the interval value reported by the LE CIS Established
> > > event since the latter reffers to the ISO interval. These two
> > > interval are not the same thing:
> > >
> > > BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 6, Part G
> > >
> > > Isochronous interval:
> > > The time between two consecutive BIS or CIS events (designated
> > > ISO_Interval in the Link Layer)
> > >
> > > SDU interval:
> > > The nominal time between two consecutive SDUs that are sent or
> > > received by the upper layer.
> >
> > I assume they are not the same because the ISO interval can have more
> > than one subevents, but otherwise if BN=1 then it shall be aligned, so
> > we are probably missing the BN component here.
> >
> I don't think that there's any need for setting the SDU Interval of the qos
> here since it has already been set by the host prior to issuing the LE Set
> CIG Parameters command, so the controller will have to respect that
> value. Since the it has been set by the host, to be used by the controller,
> to me, it seems a little bit redundant to derive the SDU Interval
> once again based on parameters received on this event. I think that
> continuing to use the initial value set by the Host should suffice.
Yeah but how about the receiver case? Or you expected that we set the
QoS settings as a server as well? We need to confirm that this works
in both directions or actually I don't think this would work with the
likes of iso-test/isotester because there is no BAP layer seating
above it to configure the SDU interval it really needs to come from
the ISO socket itself.
> > > ---
> > > net/bluetooth/hci_event.c | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index 868ffccff773..83cf0e8a56cf 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -6824,10 +6824,6 @@ static void hci_le_cis_estabilished_evt(struct
> > hci_dev *hdev, void *data,
> > >
> > > pending = test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags);
> > >
> > > - /* Convert ISO Interval (1.25 ms slots) to SDU Interval (us) */
> > > - qos->ucast.in.interval = le16_to_cpu(ev->interval) * 1250;
> >
> > This most likely needs to be le16_to_cpu(ev->interval) * 1250 *
> > ev->bn, anyway it probably makes sense to indicate what the BN is
> > causing this problem.
> >
> > > - qos->ucast.out.interval = qos->ucast.in.interval;
> > >
> > > switch (conn->role) {
> > > case HCI_ROLE_SLAVE:
> > > /* Convert Transport Latency (us) to Latency (msec) */
> > > --
> > > 2.40.1
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
