diff mbox series

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

Message ID 20211005093729.628833-1-jbrunet@baylibre.com (mailing list archive)
State New, archived
Headers show
Series [RFC,1/2] usb: gadget: uac2: fix maximum bandwidth calculation | expand

Commit Message

Jerome Brunet Oct. 5, 2021, 9:37 a.m. UTC
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(-)

Comments

Pavel Hofman Oct. 5, 2021, 10 a.m. UTC | #1
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?

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));
>   
>
Jerome Brunet Oct. 5, 2021, 10:13 a.m. UTC | #2
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));
>>
Pavel Hofman Oct. 5, 2021, 11:12 a.m. UTC | #3
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). 
IIUC that chapter does not deal with async mode because exact samplerate 
values converted to sample numbers are used there. How does your new 
calculation take into account the upper range of the async rate, now 
increased to +25% by your second patch? 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).

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.
Jerome Brunet Oct. 5, 2021, 4:24 p.m. UTC | #4
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.
Pavel Hofman Oct. 6, 2021, 12:08 p.m. UTC | #5
Dne 05. 10. 21 v 18:24 Jerome Brunet napsal(a):
> 
> 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.

Jerome, thanks for your insight. IMO those sections describe calculation 
of numbers of audio frames in each packet (SIP/VFP) for a particular 
samplerate (be it 48kHz or 48.005kHz), not specifically about max packet 
size calculation for the base samplerate of 48kHz. Adaptive transfer 
which recovers audio clock via PLL requires the audioframes count 
variation as small as possible (therefore only the +/-1 packet allowed 
from the "average"). Please note the next subsection "Pitch Control" 
specifically says:

"Pitch control is restricted to adaptive endpoints only"

The way I (poorly) understand it is this: let's have requested 48kHz, 
i.e. 6 audioframes for every SIP with bInterval=1. The maxPacketSize 
must be AT LEAST 1 sample larger otherwise Win10 refuses enumeration 
(correctly, according to the USB specs) - therefore that + 1 in our 
patch. But the maxPacketSize for async endpoint must be actually larger 
to account for the max possible increase in samplerate. IIUC the host 
has no information on how much the device will actually increase the 
rate when running, but the MINIMUM allowable maxPacketSize is still 7 
audioframes. If the device specifies more (because its upper rate limit 
+ 1 exceeds 7), it's problem of the device that it reserves more 
bandwidth and that user plugged in such a "hungry" device. But the 
criterium is adhered to, at least 7 audioframes, both for async and 
adaptive. You may remember how you wondered why Win10 accepted a larger 
maxPacketSize value than was actually necessary. IMO this fits the picture.

The actual maxPacketSize calculated and reported by the device must take 
into account the possible increase in packet size it will possibly 
request from the host or it will possibly send to the host when the 
samplerate is increased on the device side. Otherwise EP-IN data from 
the device will be lost or not enough data will be sent by the host to 
EP-OUT.

Win10 accepts some range of the feedback message value which affects the 
possible range of the samplerate variation allowed by the device but IMO 
we cannot do much with it. The range is quite large 
https://groups.google.com/g/audio-widget/c/COAfYP2BCzw/m/5f0q2iqeAQAJ 
(see the unwrapped post in the list), IMO enough for any practical 
usage. If the feedback value is outside of this range, Win10 simply 
stops adjusting the rate. Tests of this resulted in the patch 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/usb/+/f5dfd98a80ff8d50cf4ae2820857d7f5a46cbab9 
. Feedback values, originally calculated for SIP 125us, fell below the 
range the Win10 driver expected for bInterval=4 (being actually 8x 
smaller) and Win10 kept sending 48samples every 1ms at 48kHz samplerate, 
no matter what the feedback value was when adjusted by the "Capture 
Pitch 1000000" ctl (still way below the lower allowed limit of the 
feedback value range for 48kHz and 1ms SIP).

Your patch #2 hard-coding the fb_max to +25% corresponds to the USB 
specs, but still IMO fb_max=1.25 must enter the maxPacketSize 
calculation to reserve the bandwidth when gadget side is allowed to 
request such range. It will result in useless over-reservation of the 
isochronous bandwidth as no practical usage will need +25% rate adjust. 
I find the currently chosen fb_max +0.5% OK, enough for any 
crystal-based clock, it can be changed/patched later if needed.

That's just how I view it, I may be completely wrong. Thanks a lot for 
your great help and involvement.

Best regards,

Pavel.
diff mbox series

Patch

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;
 
-	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));