From: "Pali Rohár" <pali.rohar@gmail.com>
To: Marcel Holtmann <marcel@holtmann.org>
Cc: Pavel Machek <pavel@ucw.cz>,
Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
"linux-bluetooth@vger.kernel.org"
<linux-bluetooth@vger.kernel.org>,
Johan Hedberg <johan.hedberg@gmail.com>
Subject: Re: HCI Set custom bandwidth for AuriStream SCO codec
Date: Tue, 19 Nov 2019 18:13:42 +0100 [thread overview]
Message-ID: <20191119171342.mwfzszu7xwabi7to@pali> (raw)
In-Reply-To: <1CFFA8EF-1B2A-466E-8901-BFB849F20442@holtmann.org>
[-- Attachment #1: Type: text/plain, Size: 8703 bytes --]
On Tuesday 19 November 2019 18:04:36 Marcel Holtmann wrote:
> Hi Pali,
>
> >>>>>>>>>>>> to be honest, I would rather see WBS implementation finally
> >>>>>>>>>>>> reach PA before we start digging into this.
> >>>>>>>>>>>
> >>>>>>>>>>> First I want to finish improving A2DP codec support in pulseaudio. Later
> >>>>>>>>>>> I can look at HSP/HFP profiles. Ideally it should have modular/plugin
> >>>>>>>>>>> extensible design. So the aim is that adding new codec would be very
> >>>>>>>>>>> simple, without need to hack something related to mSBC/WBC, AuriStream
> >>>>>>>>>>> or any other codec.
> >>>>>>>>>>
> >>>>>>>>>> Well HSP don't have support for codec negotiation, but yes a modular
> >>>>>>>>>> design is probably recommended.
> >>>>>>>>>>
> >>>>>>>>>>> But for AuriStream I need to set custom SCO parameters as described
> >>>>>>>>>>> below and currently kernel does not support it. This is why I'm asking
> >>>>>>>>>>> how kernel can export for userspace configuration of SCO parameters...
> >>>>>>>>>>
> >>>>>>>>>> We can always come up with socket options but we got to see the value
> >>>>>>>>>> it would bring since AuriStream don't look that popular among
> >>>>>>>>>> headsets, at least Ive never seem any device advertising it like
> >>>>>>>>>> apt-X, etc.
> >>>>>>>>>
> >>>>>>>>> Pali clearly has such device and he is willing to work on it. Surely
> >>>>>>>>> that means it is popular enough to be supported...?
> >>>>>>>>
> >>>>>>>> Just put AT+CSRSF=0,0,0,0,0,7 to google search and you would see that
> >>>>>>>> not only I have such device...
> >>>>>>>>
> >>>>>>>> So I would really would like to see that kernel finally stops blocking
> >>>>>>>> usage of this AuriStream codec.
> >>>>>>>
> >>>>>>> we need to figure out on how we do the kernel API to allow you this specific setting.
> >>>>>>
> >>>>>> Hi Marcel! Kernel API for userspace should be simple. Just add two
> >>>>>> ioctls for retrieving and setting structure with custom parameters:
> >>>>>>
> >>>>>> syncPktTypes = 0x003F
> >>>>>> bandwidth = 4000
> >>>>>> max_latency = 16
> >>>>>> voice_settings = 0x63
> >>>>>> retx_effort = 2
> >>>>>>
> >>>>>> Or add more ioctls, one ioctl per parameter. There is already only ioctl
> >>>>>> for voice settings and moreover it is whitelisted only for two values.
> >>>>>
> >>>>> it is not that simple actually. Most profiles define a certain set of parameters and then they try to configure better settings and only fallback to a specification defined default as last resort.
> >>>>
> >>>> Ok. I see that there is another "example" configuration for AuriStream
> >>>> with just different syncPktTypes = 0x02BF and bandwidth = 3850.
> >>>>
> >>>> So it really is not simple as it can be seen.
> >>>
> >>> currently the stepping for mSBC and CVSD are hard-coded in esco_param_cvsd and esco_param_msbc arrays in hci_conn.c and then selected by the ->setting parameter.
> >>>
> >>> So either we provide an new socket option (for example BT_VOICE_EXT) or we extend BT_VOICE to allow providing the needed information. However this needs to be flexible array size since we should then be able to encode multiple stepping that are tried in order.
> >>>
> >>> My preference is that we extend BT_VOICE and not introduce a new socket option. So feel free to propose how we can load the full tables into the SCO socket. I mean we are not really far off actually. The only difference is that currently the tables are in the hci_conn.c file and selected by the provided voice->setting. However nothing really stops us from providing the full table via user space.
> >>
> >> Ok. I will look at it and I will try to propose how to extend current
> >> BT_VOICE ioctl API for supporting all those new parameters.
> >
> > Below is inline MIME part with POC patch which try to implement a new
> > IOCTL (currently named BT_VOICE_SETUP) for configuring voice sco
> > settings.
> >
> > It uses flexible array of parameters <tx_bandwidth, rx_bandwidth,
> > voice_setting, pkt_type, max_latency, retrans_effort>, but with
> > maximally 10 array members (due to usage of static array storage). cvsd
> > codec uses 7 different fallback settings (see voice_setup_cvsd), so for
> > POC 10 should be enough.
> >
> > Because a new IOCL has different members then old BT_VOICE I rather
> > decided to introduce a new IOCTL and not hacking old IOCTL to accept two
> > different structures.
> >
> > Please let me know what do you think about this API, if this is a way
> > how to continue or if something different is needed.
> >
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index fabee6db0abb..0e9f4ac07220 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -122,6 +122,19 @@ struct bt_voice {
> > #define BT_SNDMTU 12
> > #define BT_RCVMTU 13
> >
> > +#define BT_VOICE_SETUP 14
> > +#define BT_VOICE_SETUP_ARRAY_SIZE 10
> > +struct bt_voice_setup {
> > + __u8 sco_capable:1;
> > + __u8 esco_capable:1;
> > + __u32 tx_bandwidth;
> > + __u32 rx_bandwidth;
> > + __u16 voice_setting;
> > + __u16 pkt_type;
> > + __u16 max_latency;
> > + __u8 retrans_effort;
> > +};
> > +
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 094e61e07030..8f3c161da1c4 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -477,7 +477,7 @@ struct hci_conn {
> > __u8 passkey_entered;
> > __u16 disc_timeout;
> > __u16 conn_timeout;
> > - __u16 setting;
> > + struct bt_voice_setup voice_setup[BT_VOICE_SETUP_ARRAY_SIZE];
> > __u16 le_conn_min_interval;
> > __u16 le_conn_max_interval;
> > __u16 le_conn_interval;
> > @@ -897,8 +897,8 @@ static inline struct hci_conn *hci_lookup_le_connect(struct hci_dev *hdev)
> > }
> >
> > int hci_disconnect(struct hci_conn *conn, __u8 reason);
> > -bool hci_setup_sync(struct hci_conn *conn, __u16 handle);
> > -void hci_sco_setup(struct hci_conn *conn, __u8 status);
> > +int hci_setup_sync(struct hci_conn *conn, __u16 handle);
> > +int hci_sco_setup(struct hci_conn *conn, __u8 status);
> >
> > struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
> > u8 role);
> > @@ -920,7 +920,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> > struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
> > u8 sec_level, u8 auth_type);
> > struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
> > - __u16 setting);
> > + struct bt_voice_setup *voice_setup);
> > int hci_conn_check_link_mode(struct hci_conn *conn);
> > int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
> > int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type,
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index bd4978ce8c45..0aa2ad98eb80 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -35,30 +35,6 @@
> > #include "smp.h"
> > #include "a2mp.h"
> >
> > -struct sco_param {
> > - u16 pkt_type;
> > - u16 max_latency;
> > - u8 retrans_effort;
> > -};
> > -
> > -static const struct sco_param esco_param_cvsd[] = {
> > - { EDR_ESCO_MASK & ~ESCO_2EV3, 0x000a, 0x01 }, /* S3 */
> > - { EDR_ESCO_MASK & ~ESCO_2EV3, 0x0007, 0x01 }, /* S2 */
> > - { EDR_ESCO_MASK | ESCO_EV3, 0x0007, 0x01 }, /* S1 */
> > - { EDR_ESCO_MASK | ESCO_HV3, 0xffff, 0x01 }, /* D1 */
> > - { EDR_ESCO_MASK | ESCO_HV1, 0xffff, 0x01 }, /* D0 */
> > -};
> > -
> > -static const struct sco_param sco_param_cvsd[] = {
> > - { EDR_ESCO_MASK | ESCO_HV3, 0xffff, 0xff }, /* D1 */
> > - { EDR_ESCO_MASK | ESCO_HV1, 0xffff, 0xff }, /* D0 */
> > -};
> > -
> > -static const struct sco_param esco_param_msbc[] = {
> > - { EDR_ESCO_MASK & ~ESCO_2EV3, 0x000d, 0x02 }, /* T2 */
> > - { EDR_ESCO_MASK | ESCO_EV3, 0x0008, 0x02 }, /* T1 */
> > -};
> > -
>
> can you split this into multiple logical patches. And ensure sending it with git send-email.
I just send it as is to know if such API make sense and should I
continue or not. Preparing patches for git send-email takes a lot of
time and I wanted to know if such API is OK or should be fully
rewritten. So I do not spend on something which does not make sense.
Above patch is not mean to be complete not ready for merge.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
next prev parent reply other threads:[~2019-11-19 17:13 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-04 17:15 HCI Set custom bandwidth for AuriStream SCO codec Pali Rohár
2019-05-06 15:16 ` Pali Rohár
2019-05-16 18:34 ` Pali Rohár
2019-05-19 8:16 ` Luiz Augusto von Dentz
2019-05-19 8:23 ` Pali Rohár
2019-05-19 8:45 ` Luiz Augusto von Dentz
2019-05-19 8:54 ` Pali Rohár
2019-05-19 21:21 ` Pavel Machek
2019-06-07 13:02 ` Pali Rohár
2019-06-07 15:19 ` Luiz Augusto von Dentz
2019-07-06 13:45 ` Marcel Holtmann
2019-07-08 12:25 ` Pali Rohár
2019-07-08 13:23 ` Marcel Holtmann
2019-07-08 21:06 ` Pali Rohár
2019-07-12 18:59 ` Marcel Holtmann
2019-07-18 10:09 ` Pali Rohár
2019-07-18 20:06 ` Marcel Holtmann
2019-11-21 23:00 ` Pali Rohár
2019-11-24 11:02 ` Marcel Holtmann
2019-10-27 22:09 ` Pali Rohár
2019-11-12 21:06 ` Pavel Machek
2019-11-13 9:22 ` Pali Rohár
2019-11-21 22:47 ` Pali Rohár
2019-11-19 17:04 ` Marcel Holtmann
2019-11-19 17:13 ` Pali Rohár [this message]
2019-11-19 23:47 ` Marcel Holtmann
2019-11-20 7:44 ` Pali Rohár
2019-11-21 22:44 ` Pali Rohár
2019-11-24 11:04 ` Marcel Holtmann
2019-11-24 11:13 ` Pali Rohár
2019-11-26 7:24 ` Marcel Holtmann
2019-11-26 7:46 ` Pali Rohár
2019-11-26 7:58 ` Marcel Holtmann
2019-11-26 8:00 ` Pali Rohár
2019-11-26 9:41 ` Luiz Augusto von Dentz
2019-11-26 9:58 ` Pali Rohár
2019-12-05 9:28 ` Pali Rohár
2019-12-11 14:40 ` Pali Rohár
2020-01-04 10:04 ` Marcel Holtmann
2020-01-04 10:37 ` Pali Rohár
2020-02-09 12:59 ` Pali Rohár
2020-02-19 12:09 ` David Heidelberg
2020-04-19 23:49 ` Bluetooth: Allow to use configure SCO socket codec parameters Pali Rohár
2020-04-20 23:54 ` Luiz Augusto von Dentz
2020-04-21 8:53 ` Pali Rohár
2020-05-14 19:49 ` Aleksandar Kostadinov
2020-05-15 22:46 ` Andrew Fuller
2020-05-15 23:08 ` Luiz Augusto von Dentz
2020-05-16 7:50 ` Aleksandar Kostadinov
2020-05-16 7:53 ` Pali Rohár
2020-05-18 16:43 ` Luiz Augusto von Dentz
2020-05-18 16:50 ` Pali Rohár
2020-05-27 12:18 ` Ujjwal Sharma
2020-05-27 15:48 ` Luiz Augusto von Dentz
2020-05-27 16:24 ` Ujjwal Sharma
2020-06-04 20:43 ` Pali Rohár
2020-07-13 16:46 ` Pasi Kärkkäinen
2020-09-29 21:04 ` Pali Rohár
2020-10-26 11:45 ` Joschi 127
2020-10-27 23:45 ` Paul Stejskal
2020-10-28 20:25 ` Joschi 127
2020-11-03 12:10 ` Joschi 127
2020-11-03 12:18 ` Pali Rohár
2020-11-03 12:43 ` Jan-Philipp Litza
2020-11-04 0:37 ` Luiz Augusto von Dentz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191119171342.mwfzszu7xwabi7to@pali \
--to=pali.rohar@gmail.com \
--cc=johan.hedberg@gmail.com \
--cc=linux-bluetooth@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
--cc=pavel@ucw.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).