Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
From: Marcel Holtmann <marcel@holtmann.org>
To: "Pali Rohár" <pali.rohar@gmail.com>
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:04:36 +0100
Message-ID: <1CFFA8EF-1B2A-466E-8901-BFB849F20442@holtmann.org> (raw)
In-Reply-To: <20191027220945.wmb3g55wtrmqbnmz@pali>

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.

Regards

Marcel


  parent reply index

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-04 17:15 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 [this message]
2019-11-19 17:13                               ` Pali Rohár
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

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=1CFFA8EF-1B2A-466E-8901-BFB849F20442@holtmann.org \
    --to=marcel@holtmann.org \
    --cc=johan.hedberg@gmail.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=luiz.dentz@gmail.com \
    --cc=pali.rohar@gmail.com \
    --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

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git