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

[Jerome Brunet <jbrunet@baylibre.com>]

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

> Dne 05. 10. 21 v 12:13 Jerome Brunet napsal(a):
>> 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.
>
> I am sorry I do not understand. You mention chapter 2.3.1.2.1 (I assume it
> is SERVICE INTERVAL PACKET SIZE CALCULATION in Fmts30-Errata.pdf).

Yes, UNIVERSAL SERIAL BUS DEVICE CLASS DEFINITION FOR AUDIO DATA FORMATS

Wording has changed in v3 but you can check v2 here (around 2.3.1):
https://www.usb.org/sites/default/files/Audio2.0_final.zip

It is the same thing, SIP becomes VFP.

> IIUC
> that chapter does not deal with async mode because exact samplerate values
> converted to sample numbers are used there.

In the section mentionned above, I don't see any reference to the sync mode.

> How does your new calculation
> take into account the upper range of the async rate, now increased to +25%
> by your second patch?

25% is only the upper limit of the *request* that can be made. What the
host can actually do is different, and the bandwidth is different.
There is comment in 2nd patch which is fairly important.

> The max packet size calculation is done prior to
> "tweaking" the rate via async feedback, IMO it should logically take into
> account the maximum possible increase (which the previous algorithm did via
> the fb_max (always > 0) adjustment).

Well ... maybe. Honestly, reading the spec, I am under the impression
that the extra bandwidth allocated is not *customisable*. AFAIU, one
should use the large VFP/SIP size. It this case, the 'fb_max' makes no sense.

... but maybe I mis-understand the spec, it's not easiest document to
read :/ .

>
> Maybe there is a difference in UAC3 which the Fmts30 specs seem to
> describe, I do not know. I just do not see how the possible increase of 
> packet size in async mode fits into this calculation.
>
>
> Thanks a lot,
>
> Pavel.