Re: [RFC PATCH 1/2] usb: gadget: uac2: fix maximum bandwidth calculation

[Jerome Brunet <jbrunet@baylibre.com>]

On Tue 05 Oct 2021 at 12:00, Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Dne 05. 10. 21 v 11:37 Jerome Brunet napsal(a):
>> This fixes the wMaxPacketSize of the audio gadget so it is in line with
>> USB Audio Format specification - section 2.3.1.2.1
>> Cc: Jack Pham <jackp@codeaurora.org>
>> Cc: Pavel Hofman <pavel.hofman@ivitera.com>
>> Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback implementation")
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>   There was a mistake in my previous mail, rounding depends on the
>>   synchronisation, not the stream direction.
>>   drivers/usb/gadget/function/f_uac2.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>> diff --git a/drivers/usb/gadget/function/f_uac2.c
>> b/drivers/usb/gadget/function/f_uac2.c
>> index ae29ff2b2b68..c152efa30def 100644
>> --- a/drivers/usb/gadget/function/f_uac2.c
>> +++ b/drivers/usb/gadget/function/f_uac2.c
>> @@ -554,7 +554,7 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
>>   	struct usb_endpoint_descriptor *ep_desc,
>>   	enum usb_device_speed speed, bool is_playback)
>>   {
>> -	int chmask, srate, ssize;
>> +	int chmask, srate, ssize, spf;
>>   	u16 max_size_bw, max_size_ep;
>>   	unsigned int factor;
>>   @@ -584,11 +584,12 @@ static int set_ep_max_packet_size(const struct
>> f_uac2_opts *uac2_opts,
>>   		ssize = uac2_opts->c_ssize;
>>   	}
>>   -	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
>> -		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
>> +	if (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ADAPTIVE)
>> +		spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>> +	else
>> +		spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;
>
> Please correct me if I am wrong, but does the change mean that
> uac2_opts->c_sync value has also impact on playback (EP-IN)
> wMaxPacketSize?

Duh :( - Thanks for catching this ! we only support async for playback

I guess you get the idea, I meant the rounding depends on the sync mode:
 ADAPTIVE: spf = DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
 ASYNC: spf = (srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1;

The important thing that you should round down for async (not up, as in
the patch you have sent)

Here is quick example with USB full speed
 - ADAPTIVE:
  * 48kHz: 48 samples/SIP (Service Interval Packet)
  * 44.1kHz: max 45 samples/SIP

 - ASYNC
  * 48kHz: small SIP=47samples - big SIP=49samples
  * 44.1kHz small SIP=44samples - big SIP=45samples

Your initial patch would not be correct for ASYNC@44.1kHz.
I think it would give a maximum size (big SIP) of 46 samples instead of
45. 

>
> Thanks,
>
> Pavel.
>>   -	max_size_bw = num_channels(chmask) * ssize *
>> -		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
>> +	max_size_bw = num_channels(chmask) * ssize * spf;
>>   	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
>>   						    max_size_ep));
>>