On Tue 05 Oct 2021 at 13:12, Pavel Hofman <pavel.hofman@xxxxxxxxxxx> wrote:
> Dne 05. 10. 21 v 12:13 Jerome Brunet napsal(a):
>> On Tue 05 Oct 2021 at 12:00, Pavel Hofman <pavel.hofman@xxxxxxxxxxx>
>> 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@xxxxxxxxxxxxxx>
>>>> Cc: Pavel Hofman <pavel.hofman@xxxxxxxxxxx>
>>>> Fixes: e89bb4288378 ("usb: gadget: u_audio: add real feedback implementation")
>>>> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>>>> ---
>>>> 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.
