Re: HCI Set custom bandwidth for AuriStream SCO codec

["Pali Rohár" <pali.rohar@gmail.com>]

On Tuesday 12 November 2019 22:06:33 Pavel Machek wrote:
> Hi!
> 
> > > > >>>>>>>> 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.
> 
> 
> New ioctl sounds like good idea.
> 
> > 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;
> > +};
> > +
> 
> Is this the new ioctl? I'd assume it should go to include/user/..
> somewhere to be exported to userspace...?

I put it into same file where is structure for old ioctl BT_VOICE.

> Is it good idea to use __u8 :1 in user<->kernel API?

I do not know. Should it be rather (C99) bool? Or just one __u8?

-- 
Pali Rohár
pali.rohar@gmail.com