Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 21 Nov 2019 23:47:38 +0100
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali.rohar@gmail.com>
To:     Pavel Machek <pavel@ucw.cz>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        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
Message-ID: <20191121224738.darrzzxk4zzronoc@pali>
References: <20190607130245.mv4ch6dxnuptzdki@pali>
 <ED456CCA-CF85-48D9-B7E9-9B0BF02A32FC@holtmann.org>
 <20190708122512.qqfvtm455ltxxg3h@pali>
 <E4A6E61C-DE37-4E5D-9401-71CCE4AE2419@holtmann.org>
 <20190708210616.x2dlnzjhnplu37bz@pali>
 <D0A44CC7-CABC-408A-894E-AAD700FA9B0D@holtmann.org>
 <20190718100939.bwl26qcfxe6ppcto@pali>
 <20191027220945.wmb3g55wtrmqbnmz@pali>
 <20191112210633.cjhpb7eczzta63mf@ucw.cz>
 <20191113092256.bxc6ixuk7ctrmt57@pali>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
        protocol="application/pgp-signature"; boundary="yjngod7q33go2uqp"
Content-Disposition: inline
In-Reply-To: <20191113092256.bxc6ixuk7ctrmt57@pali>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk


--yjngod7q33go2uqp
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wednesday 13 November 2019 10:22:56 Pali Roh=C3=A1r wrote:
> On Tuesday 12 November 2019 22:06:33 Pavel Machek wrote:
> > Hi!
> >=20
> > > > > >>>>>>>> But for AuriStream I need to set custom SCO parameters a=
s 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...
> > > > > >>>>>>>=20
> > > > > >>>>>>> We can always come up with socket options but we got to s=
ee the value
> > > > > >>>>>>> it would bring since AuriStream don't look that popular a=
mong
> > > > > >>>>>>> headsets, at least Ive never seem any device advertising =
it like
> > > > > >>>>>>> apt-X, etc.
> > > > > >>>>>>=20
> > > > > >>>>>> Pali clearly has such device and he is willing to work on =
it. Surely
> > > > > >>>>>> that means it is popular enough to be supported...?
> > > > > >>>>>=20
> > > > > >>>>> Just put AT+CSRSF=3D0,0,0,0,0,7 to google search and you wo=
uld see that
> > > > > >>>>> not only I have such device...
> > > > > >>>>>=20
> > > > > >>>>> So I would really would like to see that kernel finally sto=
ps blocking
> > > > > >>>>> usage of this AuriStream codec.
> > > > > >>>>=20
> > > > > >>>> we need to figure out on how we do the kernel API to allow y=
ou this specific setting.
> > > > > >>>=20
> > > > > >>> Hi Marcel! Kernel API for userspace should be simple. Just ad=
d two
> > > > > >>> ioctls for retrieving and setting structure with custom param=
eters:
> > > > > >>>=20
> > > > > >>> syncPktTypes =3D 0x003F
> > > > > >>> bandwidth =3D 4000
> > > > > >>> max_latency =3D 16
> > > > > >>> voice_settings =3D 0x63
> > > > > >>> retx_effort =3D 2
> > > > > >>>=20
> > > > > >>> Or add more ioctls, one ioctl per parameter. There is already=
 only ioctl
> > > > > >>> for voice settings and moreover it is whitelisted only for tw=
o values.
> > > > > >>=20
> > > > > >> 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.
> > > > > >=20
> > > > > > Ok. I see that there is another "example" configuration for Aur=
iStream
> > > > > > with just different syncPktTypes =3D 0x02BF and bandwidth =3D 3=
850.
> > > > > >=20
> > > > > > So it really is not simple as it can be seen.
> > > > >=20
> > > > > currently the stepping for mSBC and CVSD are hard-coded in esco_p=
aram_cvsd and esco_param_msbc arrays in hci_conn.c and then selected by the=
 ->setting parameter.
> > > > >=20
> > > > > So either we provide an new socket option (for example BT_VOICE_E=
XT) or we extend BT_VOICE to allow providing the needed information. Howeve=
r this needs to be flexible array size since we should then be able to enco=
de multiple stepping that are tried in order.
> > > > >=20
> > > > > 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 differ=
ence is that currently the tables are in the hci_conn.c file and selected b=
y the provided voice->setting. However nothing really stops us from providi=
ng the full table via user space.
> > > >=20
> > > > Ok. I will look at it and I will try to propose how to extend curre=
nt
> > > > BT_VOICE ioctl API for supporting all those new parameters.
> > >=20
> > > 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.
> > >=20
> > > 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). cv=
sd
> > > codec uses 7 different fallback settings (see voice_setup_cvsd), so f=
or
> > > POC 10 should be enough.
> > >=20
> > > 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.
> > >=20
> > > 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.
> >=20
> >=20
> > New ioctl sounds like good idea.
> >=20
> > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetoot=
h/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
> > > =20
> > > +#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;
> > > +};
> > > +
> >=20
> > Is this the new ioctl? I'd assume it should go to include/user/..
> > somewhere to be exported to userspace...?
>=20
> I put it into same file where is structure for old ioctl BT_VOICE.

Userspace headers are provided by bluez project. So once this API would
be in kernel, bluez headers needs to be extended.

> > Is it good idea to use __u8 :1 in user<->kernel API?
>=20
> I do not know. Should it be rather (C99) bool? Or just one __u8?

In next proposed API I used one __u8 with bitmask macros. So there are
no :1 anymore.

--=20
Pali Roh=C3=A1r
pali.rohar@gmail.com

--yjngod7q33go2uqp
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iF0EABECAB0WIQS4VrIQdKium2krgIWL8Mk9A+RDUgUCXdcUCAAKCRCL8Mk9A+RD
Uh3PAJ4pd62AHSXdXqS6fXMR+EegJzk+2ACdGhOqs65nYGXsNdJiEEsZMkcbOsI=
=v4fI
-----END PGP SIGNATURE-----

--yjngod7q33go2uqp--